From e35c25ef9ced05e6893622eadf1a0d5f06fbbce2 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 29 Oct 2026 02:57:10 -0700 Subject: [PATCH] fix: check a move's writes with the rights its token carries in the target The transfer re-authenticated the token against the target but then checked writes with the user's database membership there, so a JWT whose claims are narrower than that membership could copy a draft to a path it cannot save to directly. The identity the re-authentication returns is now the one the checks and the copy use. Co-Authored-By: Claude Opus 5.5 --- .../tests/drafts.rs | 35 ++++++++++++++----- .../windmill-api-workspaces/src/workspaces.rs | 2 +- backend/windmill-api/src/drafts.rs | 19 ++++------ 3 files changed, 34 insertions(+), 22 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/drafts.rs b/backend/windmill-api-integration-tests/tests/drafts.rs index 70b5518ad9..75ec588643 100644 --- a/backend/windmill-api-integration-tests/tests/drafts.rs +++ b/backend/windmill-api-integration-tests/tests/drafts.rs @@ -380,11 +380,11 @@ async fn test_transfer_drafts_to_fork(db: Pool) -> anyhow::Result<()> // A JWT has no `token` row: its own claims decide which workspaces it reaches. set_jwt_secret().await; - let jwt = |workspace_ids: Vec<&str>, job_id: Option<&str>| { + let jwt = |workspace_ids: Vec<&str>, job_id: Option<&str>, is_admin: bool| { windmill_common::jwt::encode_with_internal_secret(windmill_common::auth::JWTAuthClaims { email: me.to_string(), username: "test-user".to_string(), - is_admin: true, + is_admin, is_operator: false, groups: vec![], folders: vec![], @@ -402,18 +402,37 @@ async fn test_transfer_drafts_to_fork(db: Pool) -> anyhow::Result<()> }) }; let job_id = uuid::Uuid::new_v4().to_string(); - for (claims, status) in [ - (jwt(vec![WS], None).await?, 403), - (jwt(vec![WS, fork], Some(&job_id)).await?, 403), - (jwt(vec![WS, fork], None).await?, 200), + let r = c + .post(save_url(port, "script", "u/someone-else/x")) + .json(&json!({ "value": { "content": "x" } })) + .send() + .await?; + assert_eq!(r.status(), 200); + for (claims, path, status) in [ + (jwt(vec![WS], None, true).await?, "u/test-user/a", 403), + ( + jwt(vec![WS, fork], Some(&job_id), true).await?, + "u/test-user/a", + 403, + ), + (jwt(vec![WS, fork], None, true).await?, "u/test-user/a", 200), + // An admin in the fork's database, but the JWT does not claim it: the claims win. + ( + jwt(vec![WS, fork], None, false).await?, + "u/someone-else/x", + 401, + ), ] { let r = client_for(&format!("jwt_{claims}")) .post(&transfer_url) - .json(&json!({ "target_workspace": fork, "items": [{ "kind": "script", "path": "u/test-user/a" }] })) + .json( + &json!({ "target_workspace": fork, "items": [{ "kind": "script", "path": path }] }), + ) .send() .await?; - assert_eq!(r.status(), status); + assert_eq!(r.status(), status, "{path}: {}", r.text().await?); } + assert_eq!(fork_count("u/someone-else/x").await?, 0); // `a` was cloned into the fork: the whole transfer is refused and nothing moves. let body: serde_json::Value = c diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 70d230d08a..a5c3b5cefe 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -14244,7 +14244,7 @@ async fn seed_full_diff_scan( /// If the user is not a member of `workspace_id`, returns an authed with no /// folders/groups/operator/admin (except for superadmins, who stay admin) — /// i.e. they will only see what RLS explicitly allows for unknown users. -pub async fn load_workspace_authed( +async fn load_workspace_authed( db: &DB, base_authed: &ApiAuthed, workspace_id: &str, diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index b8d7dcc5b6..2f50715e51 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -1091,16 +1091,14 @@ async fn transfer_drafts( } // Authentication checked the token against `w_id` only. Re-resolving it against the // target refuses a token bound to `w_id`, whatever its kind (an external JWT has no - // `token` row). A job token stays refused even where its claims list both workspaces. - let token_valid_for_target = auth_cache + // `token` row), and gives the rights a request to the target would carry: a JWT's + // own claims, not the database's. A job token stays refused even where its claims + // list both workspaces. + let authed_there = auth_cache .get_authed(Some(target.to_string()), &tokened.token) .await - .is_some_and(|a| a.job_id.is_none()); - if !token_valid_for_target { - return Err(Error::PermissionDenied(format!( - "this token cannot act on {target}" - ))); - } + .filter(|a| a.job_id.is_none()) + .ok_or_else(|| Error::PermissionDenied(format!("this token cannot act on {target}")))?; // Secret variable drafts hold values encrypted with the source workspace's key. if items.iter().any(|(k, _)| *k == UserDraftItemKind::Variable) && windmill_common::variables::get_workspace_key(&w_id, &db).await? @@ -1110,11 +1108,6 @@ async fn transfer_drafts( "{target} has a different encryption key than {w_id}, so variable drafts cannot be copied to it" ))); } - // `authed` speaks for this workspace only; the copy writes into the fork, so its - // rights there are loaded separately. A non-member gets an empty authed, not an error. - let authed_there = - windmill_api_workspaces::workspaces::load_workspace_authed(&db, &authed, target) - .await?; let is_member = sqlx::query_scalar!( "SELECT EXISTS(SELECT 1 FROM usr WHERE workspace_id = $1 AND email = $2 AND disabled = false)", target,