mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-29 08:12:56 +00:00
fix(auth): require workspace membership for g/ owner tokens (WIN-1986)
WIN-1978's tightening of the `g/<groupname>` owner branch only verified that the group exists in the target workspace. The default `all` group is provisioned in every workspace, so any forged `owner=g/all` token paired with an arbitrary `workspace_id` still passed the existence check and authenticated as a group user there — including for an email that was not a member of that workspace. Additionally require the token holder's email to be a non-disabled member of the target workspace, mirroring the spirit of the `u/` branch's `usr` lookup. Adds an integration regression covering both the forged non-member case and the legitimate member case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
ff685eb2d3
commit
593522033a
+24
@@ -0,0 +1,24 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT EXISTS(\n SELECT 1 FROM group_\n WHERE workspace_id = $1 AND name = $2\n ) AND EXISTS(\n SELECT 1 FROM usr\n WHERE workspace_id = $1\n AND email = $3\n AND disabled = false\n )",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
"ordinal": 0,
|
||||
"name": "?column?",
|
||||
"type_info": "Bool"
|
||||
}
|
||||
],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text",
|
||||
"Text",
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": [
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "ce9492ecccdcf4747990a94208dc54efefdeb8261142cc75010a51f209c099a2"
|
||||
}
|
||||
@@ -302,13 +302,34 @@ impl AuthCache {
|
||||
None
|
||||
}
|
||||
} else if prefix == "g" {
|
||||
let group_exists = if super_admin {
|
||||
// Checking that the group exists in the
|
||||
// target workspace is necessary but not
|
||||
// sufficient: the `all` group is
|
||||
// provisioned in every workspace, so an
|
||||
// attacker with token-table write access
|
||||
// could pair owner=g/all with any
|
||||
// workspace_id and authenticate as a
|
||||
// group user there without being a
|
||||
// workspace member. Additionally require
|
||||
// that the token holder (by email) is a
|
||||
// non-disabled member of the target
|
||||
// workspace, mirroring the u/ branch.
|
||||
let group_check = if super_admin {
|
||||
true
|
||||
} else {
|
||||
sqlx::query_scalar!(
|
||||
"SELECT EXISTS(SELECT 1 FROM group_ WHERE workspace_id = $1 AND name = $2)",
|
||||
"SELECT EXISTS(
|
||||
SELECT 1 FROM group_
|
||||
WHERE workspace_id = $1 AND name = $2
|
||||
) AND EXISTS(
|
||||
SELECT 1 FROM usr
|
||||
WHERE workspace_id = $1
|
||||
AND email = $3
|
||||
AND disabled = false
|
||||
)",
|
||||
&w_id.as_ref().unwrap(),
|
||||
name,
|
||||
&email,
|
||||
)
|
||||
.fetch_one(&self.db)
|
||||
.await
|
||||
@@ -317,7 +338,7 @@ impl AuthCache {
|
||||
.unwrap_or(false)
|
||||
};
|
||||
|
||||
if group_exists {
|
||||
if group_check {
|
||||
let groups = vec![name.to_string()];
|
||||
let folders = get_folders_for_user(
|
||||
&w_id.unwrap(),
|
||||
@@ -345,7 +366,7 @@ impl AuthCache {
|
||||
})
|
||||
} else {
|
||||
tracing::warn!(
|
||||
"Token owner g/{} is not a group in workspace {}; rejecting auth",
|
||||
"Token owner g/{} is not a valid group/workspace-member combination for workspace {}; rejecting auth",
|
||||
name,
|
||||
w_id.as_deref().unwrap_or("")
|
||||
);
|
||||
|
||||
@@ -854,3 +854,79 @@ async fn test_operator_cannot_create_update(db: Pool<Postgres>) -> anyhow::Resul
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Regression test for WIN-1986: WIN-1978's `g/<groupname>` fix only
|
||||
/// validated that the group exists in the target workspace, but the
|
||||
/// default `all` group is provisioned in every workspace. An attacker
|
||||
/// with token-table write access could therefore pair `owner=g/all` with
|
||||
/// any workspace_id and authenticate as a group user there without being
|
||||
/// a workspace member. The fix additionally requires the token holder's
|
||||
/// email to be a non-disabled member of the target workspace.
|
||||
#[ignore]
|
||||
#[cfg(feature = "deno_core")]
|
||||
#[sqlx::test(migrations = "../migrations", fixtures("base", "permissions_test"))]
|
||||
async fn test_forged_g_all_owner_rejects_non_member(db: Pool<Postgres>) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
let server = ApiServer::start(db.clone()).await?;
|
||||
let port = server.addr.port();
|
||||
let base_url = format!("http://localhost:{}/api", port);
|
||||
|
||||
// The `all` group is provisioned for `test-workspace` by base.sql,
|
||||
// matching production behaviour. Insert a token whose email is NOT
|
||||
// a member of `test-workspace`, with owner=g/all and workspace_id
|
||||
// pointing at the target workspace. This simulates an attacker who
|
||||
// mutated the `token` table to forge cross-workspace access.
|
||||
let forged_token = "FORGED_GALL_NONMEMBER";
|
||||
let forged_hash = windmill_common::utils::calculate_hash(forged_token);
|
||||
let forged_prefix = &forged_token[..10.min(forged_token.len())];
|
||||
sqlx::query!(
|
||||
"INSERT INTO token (token_hash, token_prefix, email, label, super_admin, owner, workspace_id)
|
||||
VALUES ($1, $2, 'outsider@example.com', 'forged-gall', false, 'g/all', 'test-workspace')",
|
||||
forged_hash,
|
||||
forged_prefix,
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let client = create_client_for_user(port, forged_token).await;
|
||||
let resp = client
|
||||
.get(&format!("{base_url}/w/test-workspace/scripts/list"))
|
||||
.send()
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(
|
||||
resp.status() == reqwest::StatusCode::UNAUTHORIZED
|
||||
|| resp.status() == reqwest::StatusCode::FORBIDDEN,
|
||||
"Forged g/all owner with non-member email must be rejected against test-workspace; got {}",
|
||||
resp.status()
|
||||
);
|
||||
|
||||
// Legitimate sanity check: a g/<group> token whose email IS a
|
||||
// workspace member should still authenticate. Alice is in
|
||||
// test-workspace per the permissions_test fixture.
|
||||
let alice_gall_token = "ALICE_GALL_TOKEN_OK";
|
||||
let alice_gall_hash = windmill_common::utils::calculate_hash(alice_gall_token);
|
||||
let alice_gall_prefix = &alice_gall_token[..10.min(alice_gall_token.len())];
|
||||
sqlx::query!(
|
||||
"INSERT INTO token (token_hash, token_prefix, email, label, super_admin, owner, workspace_id)
|
||||
VALUES ($1, $2, 'alice@windmill.dev', 'gall-ok', false, 'g/all', 'test-workspace')",
|
||||
alice_gall_hash,
|
||||
alice_gall_prefix,
|
||||
)
|
||||
.execute(&db)
|
||||
.await?;
|
||||
|
||||
let alice_client = create_client_for_user(port, alice_gall_token).await;
|
||||
let resp = alice_client
|
||||
.get(&format!("{base_url}/w/test-workspace/scripts/list"))
|
||||
.send()
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(
|
||||
resp.status().is_success(),
|
||||
"g/all token for a real workspace member should authenticate; got {}",
|
||||
resp.status()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user