diff --git a/backend/tests/preserve_on_behalf_of.rs b/backend/tests/preserve_on_behalf_of.rs index 36bb5173ee..7c738d3eba 100644 --- a/backend/tests/preserve_on_behalf_of.rs +++ b/backend/tests/preserve_on_behalf_of.rs @@ -1495,6 +1495,57 @@ async fn test_app_draft_policy_derives_on_behalf_of_email(db: Pool) -> Ok(()) } +/// A draft is stored unvalidated, so its principal is whatever its author typed. Resolving it +/// through the instance-wide fallback would let any member turn a guessed username into a +/// non-member superadmin's address. +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_app_draft_derivation_does_not_disclose_non_member_email( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + let path = "u/test-user-2/leak_probe"; + + // test-user-2 is an ordinary member; superadmin-external has no `usr` row here. + sqlx::query!( + "INSERT INTO draft (workspace_id, email, path, typ, value) + VALUES ($1, $2, $3, 'raw_app', $4)", + "test-workspace", + "test2@windmill.dev", + path, + json!({ + "path": path, + "summary": "Probe", + "value": { "type": "rawapp", "inline_script": null }, + "policy": { + "execution_mode": "anonymous", + "triggerables": {}, + "on_behalf_of": "u/superadmin-external" + } + }) + ) + .execute(&db) + .await?; + + for url in [ + format!("{base}/drafts/get_own/raw_app/{path}"), + format!("{base}/apps/get/p/{path}?get_draft=true&raw_app=true"), + ] { + let resp = authed(client().get(&url), "SECRET_TOKEN_2").send().await?; + assert_eq!(resp.status(), 200, "Should read own draft at {url}"); + let body = resp.text().await?; + assert!( + !body.contains("superadmin-external@windmill.dev"), + "Draft derivation must not disclose a non-member superadmin's address via {url}: {body}" + ); + } + + Ok(()) +} + /// Test schedule update preserves email/edited_by correctly #[sqlx::test(fixtures("preserve_on_behalf_of"))] async fn test_schedule_update_preserves_email(db: Pool) -> anyhow::Result<()> { diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 03420a5c73..99fdf48e9e 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -288,6 +288,55 @@ async fn derive_policy_on_behalf_of_email( Ok(()) } +/// The draft variant of the derivation. A draft is stored exactly as its editor sent it, with +/// no `resolve_on_behalf_of` pass, so its principal is caller-controlled: anyone who can save a +/// draft under their own `u/` path chooses it. `u/` is therefore resolved against workspace +/// membership alone, never `resolve_username_to_email`'s instance `password` fallback, which +/// would turn a guessed username into a non-member superadmin's address. A principal that names +/// no member is left alone rather than answered. +/// +/// `g/` and the bare-address form skip the lookup: the first is synthetic and the second is +/// already the address, so neither tells the caller anything their draft did not. +async fn derive_draft_on_behalf_of_email_in_place( + db: &DB, + w_id: &str, + obj: &mut serde_json::Map, +) -> Result { + let Some(permissioned_as) = obj + .get("on_behalf_of") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) + else { + return Ok(false); + }; + let email = match permissioned_as.strip_prefix(windmill_common::users::PERMISSIONED_AS_USER_PREFIX) + { + Some(username) => { + let member = sqlx::query_scalar!( + "SELECT email FROM usr WHERE workspace_id = $1 AND username = $2", + w_id, + username + ) + .fetch_optional(db) + .await?; + match member { + Some(email) => email, + None => return Ok(false), + } + } + None => { + windmill_common::users::get_email_from_permissioned_as_uncached( + &permissioned_as, + w_id, + db, + ) + .await? + } + }; + obj.insert("on_behalf_of_email".to_string(), json!(email)); + Ok(true) +} + /// Same derivation on an already-parsed policy. Reports whether it rewrote the address, so /// callers holding the raw form only pay to re-serialize when there was a principal to derive. async fn derive_on_behalf_of_email_in_place( @@ -328,7 +377,7 @@ async fn derive_draft_policy_on_behalf_of_email( else { return Ok(()); }; - derive_on_behalf_of_email_in_place(db, w_id, policy, true).await?; + derive_draft_on_behalf_of_email_in_place(db, w_id, policy).await?; Ok(()) } @@ -354,7 +403,7 @@ pub(crate) async fn derive_stored_draft_policy_on_behalf_of_email( let Some(policy) = obj.get_mut("policy").and_then(|p| p.as_object_mut()) else { return Ok(()); }; - if derive_on_behalf_of_email_in_place(db, w_id, policy, true).await? { + if derive_draft_on_behalf_of_email_in_place(db, w_id, policy).await? { value.0 = to_raw_value(&obj); } Ok(())