diff --git a/backend/.sqlx/query-ce9492ecccdcf4747990a94208dc54efefdeb8261142cc75010a51f209c099a2.json b/backend/.sqlx/query-ce9492ecccdcf4747990a94208dc54efefdeb8261142cc75010a51f209c099a2.json new file mode 100644 index 0000000000..e3859d94e8 --- /dev/null +++ b/backend/.sqlx/query-ce9492ecccdcf4747990a94208dc54efefdeb8261142cc75010a51f209c099a2.json @@ -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" +} diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 314ba99450..dea1bd941e 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -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("") ); diff --git a/backend/windmill-api-integration-tests/tests/permissions.rs b/backend/windmill-api-integration-tests/tests/permissions.rs index c333cf924b..d9bd292a45 100644 --- a/backend/windmill-api-integration-tests/tests/permissions.rs +++ b/backend/windmill-api-integration-tests/tests/permissions.rs @@ -854,3 +854,79 @@ async fn test_operator_cannot_create_update(db: Pool) -> anyhow::Resul Ok(()) } + +/// Regression test for WIN-1986: WIN-1978's `g/` 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) -> 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/ 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(()) +}