From ec1a04e39e9b89f57eecba37f065537673a1b3ea Mon Sep 17 00:00:00 2001 From: kipavy Date: Wed, 26 Aug 2026 22:18:19 +0000 Subject: [PATCH] fix(teams): require VIEW_SECRETS to read the legacy team vault blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /v1/teams/:team_id/sync-blob checked team membership only, while its writer requires six permissions including VIEW_SECRETS. The blob carries every object AND every secret in one ciphertext, so any member — a connect-only one included — could download the whole vault. Harmless only for as long as those members hold no vault key, which is exactly the gate issue #187 proposes to widen. Close the read side first. Also extracts the membership + Teams-tier + permission preamble the five team vault routes each repeated into require_vault_access(). --- src/routes/team_sync.rs | 122 ++++++++++++++++++++++++++++++---------- 1 file changed, 92 insertions(+), 30 deletions(-) diff --git a/src/routes/team_sync.rs b/src/routes/team_sync.rs index f5e3193..251931c 100644 --- a/src/routes/team_sync.rs +++ b/src/routes/team_sync.rs @@ -44,6 +44,24 @@ async fn require_teams_tier_for_vault(pool: &PgPool, team_id: Uuid) -> Result<() } } +/// Membership + Teams-tier + permission preamble shared by every team vault route. +/// +/// `action` names the attempted operation so the non-member warning stays greppable. +async fn require_vault_access( + pool: &PgPool, + team_id: Uuid, + user_id: Uuid, + action: &str, + permissions: &[i64], +) -> Result<(), StatusCode> { + if !is_team_member(pool, team_id, user_id).await? { + warn!(team_id = %team_id, user_id = %user_id, action, "Non-member tried to access team vault"); + return Err(StatusCode::FORBIDDEN); + } + require_teams_tier_for_vault(pool, team_id).await?; + crate::permissions::require_all_team_permissions(pool, team_id, user_id, permissions).await +} + // ─── GET /v1/teams/:team_id/vault-key ──────────────────────────────────────── #[derive(Serialize)] @@ -57,15 +75,11 @@ pub async fn get_my_vault_key( axum::Extension(auth): axum::Extension, Path(team_id): Path, ) -> Result, StatusCode> { - if !is_team_member(&pool, team_id, auth.0).await? { - warn!(team_id = %team_id, user_id = %auth.0, "Non-member tried to get vault key"); - return Err(StatusCode::FORBIDDEN); - } - require_teams_tier_for_vault(&pool, team_id).await?; - crate::permissions::require_all_team_permissions( + require_vault_access( &pool, team_id, auth.0, + "get_vault_key", &[crate::permissions::PERM_VIEW_SECRETS], ) .await?; @@ -106,15 +120,11 @@ pub async fn get_vault_key_holders( axum::Extension(auth): axum::Extension, Path(team_id): Path, ) -> Result>, StatusCode> { - if !is_team_member(&pool, team_id, auth.0).await? { - warn!(team_id = %team_id, user_id = %auth.0, "Non-member tried to list vault key holders"); - return Err(StatusCode::FORBIDDEN); - } - require_teams_tier_for_vault(&pool, team_id).await?; - crate::permissions::require_all_team_permissions( + require_vault_access( &pool, team_id, auth.0, + "list_vault_key_holders", &[crate::permissions::PERM_VIEW_SECRETS], ) .await?; @@ -166,15 +176,11 @@ pub async fn put_vault_keys( Path(team_id): Path, Json(body): Json, ) -> Result { - if !is_team_member(&pool, team_id, auth.0).await? { - warn!(team_id = %team_id, user_id = %auth.0, "Non-member tried to put vault keys"); - return Err(StatusCode::FORBIDDEN); - } - require_teams_tier_for_vault(&pool, team_id).await?; - crate::permissions::require_all_team_permissions( + require_vault_access( &pool, team_id, auth.0, + "put_vault_keys", &[ crate::permissions::PERM_VIEW_SECRETS, crate::permissions::PERM_COPY_SECRETS, @@ -248,11 +254,17 @@ pub async fn get_team_blob( axum::Extension(auth): axum::Extension, Path(team_id): Path, ) -> Result, StatusCode> { - if !is_team_member(&pool, team_id, auth.0).await? { - warn!(team_id = %team_id, user_id = %auth.0, "Non-member tried to get team blob"); - return Err(StatusCode::FORBIDDEN); - } - require_teams_tier_for_vault(&pool, team_id).await?; + // The legacy blob carries every object AND every secret in one ciphertext, so + // reading it requires the same secret-level rights its writer does. Members + // without PERM_VIEW_SECRETS read the vault through the object routes instead. + require_vault_access( + &pool, + team_id, + auth.0, + "get_team_blob", + &[crate::permissions::PERM_VIEW_SECRETS], + ) + .await?; let row = sqlx::query_as::<_, (Vec, DateTime)>( "SELECT blob, updated_at FROM team_sync_blobs WHERE team_id = $1", @@ -290,19 +302,14 @@ pub async fn put_team_blob( Path(team_id): Path, Json(body): Json, ) -> Result { - if !is_team_member(&pool, team_id, auth.0).await? { - warn!(team_id = %team_id, user_id = %auth.0, "Non-member tried to put team blob"); - return Err(StatusCode::FORBIDDEN); - } - require_teams_tier_for_vault(&pool, team_id).await?; - // Legacy whole-blob writes can replace every object and secret in a team // vault. Keep this endpoint for migration/bootstrap, but require broad // rights so lower-privilege roles cannot bypass object-level routes. - crate::permissions::require_all_team_permissions( + require_vault_access( &pool, team_id, auth.0, + "put_team_blob", &[ crate::permissions::PERM_EDIT_CONNECTIONS, crate::permissions::PERM_EDIT_IDENTITIES, @@ -395,6 +402,7 @@ mod tests { // ─── GET /v1/teams/:team_id/vault-key/holders (issue #41) ──────────────── use crate::auth::AuthUser; + use crate::permissions::PERM_CONNECT; use crate::test_pool_or_skip; use crate::test_support::{add_member, assign_role, seed_role, seed_team, seed_user}; use axum::extract::{Path, State}; @@ -455,6 +463,60 @@ mod tests { assert_eq!(res.unwrap_err(), StatusCode::FORBIDDEN); } + // ─── GET /v1/teams/:team_id/sync-blob (issue #187) ─────────────────────── + + async fn insert_team_blob(pool: &PgPool, team: Uuid, updated_by: Uuid) { + sqlx::query( + "INSERT INTO team_sync_blobs (team_id, blob, size_bytes, updated_by) \ + VALUES ($1, $2, $3, $4)", + ) + .bind(team) + .bind(b"ciphertext".to_vec()) + .bind(10_i32) + .bind(updated_by) + .execute(pool) + .await + .expect("insert team blob"); + } + + #[tokio::test] + async fn blob_forbidden_without_view_secrets_permission() { + let pool = test_pool_or_skip!(); + let owner = seed_user(&pool).await; + let team = seed_team(&pool, owner).await; + add_member(&pool, team, owner).await; + insert_team_blob(&pool, team, owner).await; + + // connect-only: no PERM_VIEW_SECRETS, so the whole-vault ciphertext — + // which carries every secret — must stay out of reach. + let connect_only = seed_user(&pool).await; + add_member(&pool, team, connect_only).await; + let role = seed_role(&pool, team, "connect-only", PERM_CONNECT).await; + assign_role(&pool, team, connect_only, role).await; + + let res = get_team_blob(State(pool.clone()), Extension(AuthUser(connect_only)), Path(team)).await; + + // `.err()` rather than `unwrap_err()`: TeamBlobResponse has no Debug. + assert_eq!(res.err(), Some(StatusCode::FORBIDDEN)); + } + + #[tokio::test] + async fn blob_readable_with_view_secrets_permission() { + let pool = test_pool_or_skip!(); + let owner = seed_user(&pool).await; + let team = seed_team(&pool, owner).await; + add_member(&pool, team, owner).await; + grant_view_secrets(&pool, team, owner).await; + insert_team_blob(&pool, team, owner).await; + + let res = get_team_blob(State(pool.clone()), Extension(AuthUser(owner)), Path(team)) + .await + .expect("blob ok") + .0; + + assert!(!res.blob.is_empty()); + } + #[tokio::test] async fn holders_forbidden_without_view_secrets_permission() { let pool = test_pool_or_skip!();