From c9c81f3b02e6cad9e3f7f598ea2ae4a7e21609b7 Mon Sep 17 00:00:00 2001 From: Rune Darrud Date: Mon, 10 Aug 2026 03:04:53 +0200 Subject: [PATCH] Fix defects found by a second adversarial review The member permissions object could not be written back. The read shape emits permissions null for anyone who is not a manage-all member, but the write models declared a plain map with serde(default), which only covers a missing field: an explicit null failed to deserialize and the request was rejected before the handler ran. Reading a member and sending it straight back therefore failed for every member except the manage-all case the previous test covered. The field is optional now, and the smoke test round-trips a plain member as well. Reinvite was the one member write that could still reach an owner, and it can change an owner from invited to accepted, so it takes the same guard as the rest. Group updates log before mutating, so a failure part way through the collection associations cannot leave an unaudited change. Co-Authored-By: Claude Opus 5 --- scripts/smoke_public_api_write.sh | 7 ++++++- src/api/core/public.rs | 20 ++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/scripts/smoke_public_api_write.sh b/scripts/smoke_public_api_write.sh index 18540d5a..2f002931 100755 --- a/scripts/smoke_public_api_write.sh +++ b/scripts/smoke_public_api_write.sh @@ -421,9 +421,14 @@ req GET "/api/public/members/$MANAGEALL" "$TOKEN" jqcheck "round-trip kept the custom role" '.type' "4" jqcheck "round-trip kept manage-all" '.permissions.editAnyCollection' "true" -# A plain member carries no permissions object. +# A plain member carries no permissions object, and must still round-trip: the read shape +# has to be acceptable as a write body, null permissions and all. req GET "/api/public/members/$MEMBER3" "$TOKEN" +jqcheck "a plain member has a permissions key" 'has("permissions")' "true" jqcheck "a plain member has null permissions" '.permissions' "null" +PLAIN=$(jq -c '{type: .type, externalId: .externalId, permissions: .permissions, collections: []}' "$TMP/body") +reqj PUT "/api/public/members/$MEMBER3" "$TOKEN" "$PLAIN" +check_eq "a plain member round-trips too" "$HTTP_CODE" "200" req DELETE "/api/public/members/$MANAGEALL" "$TOKEN" check_eq "clean up the manage-all member -> 200" "$HTTP_CODE" "200" diff --git a/src/api/core/public.rs b/src/api/core/public.rs index 310d6391..ebd40e23 100644 --- a/src/api/core/public.rs +++ b/src/api/core/public.rs @@ -500,8 +500,10 @@ struct MemberCreateData { collections: Vec, #[serde(default)] groups: Vec, + // Our own read shape emits null here for anyone who is not a manage-all member, and a + // client is expected to send a read straight back, so null has to deserialize. #[serde(default)] - permissions: HashMap, + permissions: Option>, } #[derive(Deserialize)] @@ -514,8 +516,10 @@ struct MemberUpdateData { #[serde(default)] collections: Vec, groups: Option>, + // Our own read shape emits null here for anyone who is not a manage-all member, and a + // client is expected to send a read straight back, so null has to deserialize. #[serde(default)] - permissions: HashMap, + permissions: Option>, } #[derive(Deserialize)] @@ -672,7 +676,8 @@ async fn post_member(data: Json, token: PublicToken, ip: auth: let org_id = token.0; let data = data.into_inner(); - let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &data.permissions) else { + let permissions = data.permissions.unwrap_or_default(); + let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &permissions) else { err!("Invalid type") }; deny_owner_grant(new_type)?; @@ -764,7 +769,8 @@ async fn put_member( let org_id = token.0; let data = data.into_inner(); - let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &data.permissions) else { + let permissions = data.permissions.unwrap_or_default(); + let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &permissions) else { err!("Invalid type") }; deny_owner_grant(new_type)?; @@ -866,6 +872,8 @@ async fn post_member_reinvite(member_id: MembershipId, token: PublicToken, conn: err_code!(format!("Member {member_id} not found in organization"), 404); }; + deny_owner_target(&member)?; + if member.status != MembershipStatus::Invited as i32 { err!("The user is already accepted or confirmed to the organization") } @@ -1011,10 +1019,10 @@ async fn put_group( // Member assignments are owned by "/public/groups//member-ids" and are // deliberately left untouched here. - set_group_collections(&group, &data.collections, &org_id, &conn).await?; - log_public_event(EventType::GroupUpdated as i32, &group.uuid, &org_id, &ip.ip, &conn).await; + set_group_collections(&group, &data.collections, &org_id, &conn).await?; + Ok(Json(group_to_json(&group))) }