mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-30 00:16:53 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
d363da770d
commit
e35c25ef9c
@@ -380,11 +380,11 @@ async fn test_transfer_drafts_to_fork(db: Pool<Postgres>) -> 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<Postgres>) -> 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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user