fix: resolve a draft's principal against workspace membership only

This commit is contained in:
Ruben Fiszel
2026-08-01 23:25:39 +00:00
parent 33655970b9
commit 263901b34d
2 changed files with 102 additions and 2 deletions
+51
View File
@@ -1495,6 +1495,57 @@ async fn test_app_draft_policy_derives_on_behalf_of_email(db: Pool<Postgres>) ->
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<Postgres>,
) -> 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<Postgres>) -> anyhow::Result<()> {
+51 -2
View File
@@ -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<String, serde_json::Value>,
) -> Result<bool> {
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(())