diff --git a/backend/.sqlx/query-7f832370916794ab0e5645053688c24678f1519d49ee7263a86dba71d45b8e8c.json b/backend/.sqlx/query-7f832370916794ab0e5645053688c24678f1519d49ee7263a86dba71d45b8e8c.json new file mode 100644 index 0000000000..173d8f2281 --- /dev/null +++ b/backend/.sqlx/query-7f832370916794ab0e5645053688c24678f1519d49ee7263a86dba71d45b8e8c.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO token\n (token_hash, token_prefix, token, email, label, expiration, super_admin, scopes, read_only)\n VALUES ($1, $2, $3, $4, $5, now() + ($6 || ' seconds')::interval, $7, $8, $9)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Varchar", + "Varchar", + "Varchar", + "Varchar", + "Text", + "Bool", + "TextArray", + "Bool" + ] + }, + "nullable": [] + }, + "hash": "7f832370916794ab0e5645053688c24678f1519d49ee7263a86dba71d45b8e8c" +} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 45bb54e258..4e1f516570 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -08e3b9b818f9f1b439e9a6ba477fd5fdbcab944e +3742e0659c5e97aab03b9efeea14cd94a3ac658a diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 99fa677cee..734753a0e0 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -235,6 +235,202 @@ where Ok(()) } +/// Returns the caller's "real" scope restrictions: every scope other than +/// `if_jobs:filter_tags:` tag filters. `None` means the token is unscoped and +/// has the full privileges of its user; `Some` means it is restricted to the +/// returned scopes. An empty or filter-tags-only scope list is treated as +/// unscoped, mirroring `check_scopes`/`check_route_access`. +fn scope_restrictions(scopes: Option<&[String]>) -> Option> { + let restrictions: Vec<&String> = scopes? + .iter() + .filter(|s| !s.starts_with("if_jobs:filter_tags:")) + .collect(); + (!restrictions.is_empty()).then_some(restrictions) +} + +/// Enforce monotonic privilege when a token lifecycle endpoint mints or rescopes +/// a credential on behalf of `authed`: the resulting credential must never be +/// more privileged than the caller's own token. +/// +/// - An unscoped caller may grant any scopes (this is the existing UI/CLI flow). +/// - A scope-restricted caller may only grant scopes that are a subset of its +/// own, and may never produce an unscoped credential. +/// +/// Without this, a `users:write` token could create or rescope a token to be +/// unscoped, and a `users:read` token could refresh into an unscoped session — +/// escaping its own restrictions. +pub fn ensure_scopes_within_caller( + authed: &ApiAuthed, + requested_scopes: Option<&[String]>, +) -> error::Result<()> { + if let Some(caller_restrictions) = scope_restrictions(authed.scopes.as_deref()) { + let Some(requested_restrictions) = scope_restrictions(requested_scopes) else { + return Err(Error::PermissionDenied( + "A scope-restricted token cannot create or update a token with broader (unscoped) \ + privileges" + .to_string(), + )); + }; + + // MCP scopes (`mcp:all`, `mcp:favorites`, `mcp:scripts:*`, etc.) use a + // custom format that ScopeDefinition::from_scope_string parses + // permissively but the MCP runtime interprets via its own parser + // (parse_mcp_scopes). The two views disagree — e.g. the generic parser + // accepts `mcp:scripts` as an unrestricted-resource scope, while the + // MCP runtime ignores it as unrecognized but interprets `mcp:scripts:*` + // as granting all scripts. So generic containment would silently allow + // `mcp:scripts` → `mcp:scripts:*` (a widening). Legitimate MCP token + // issuance goes through the OAuth gateway (mcp/oauth_server.rs), not + // these user-token endpoints, so require byte-identical match for MCP + // scopes here rather than trying to mirror MCP semantics in two places. + // Unparseable non-MCP caller scopes are intentionally dropped + // (fail-closed): a caller scope that fails to parse can only narrow + // the set of requested scopes that get covered, never widen it. + // Unparseable requested scopes surface as `BadRequest`, which is what + // we want — the client is sending garbage. + let parsed_caller: Vec = caller_restrictions + .iter() + .filter(|s| !s.starts_with("mcp:")) + .filter_map(|s| ScopeDefinition::from_scope_string(s).ok()) + .collect(); + let caller_mcp: std::collections::HashSet<&str> = caller_restrictions + .iter() + .filter(|s| s.starts_with("mcp:")) + .map(|s| s.as_str()) + .collect(); + + for requested in requested_restrictions { + if requested.starts_with("mcp:") { + if !caller_mcp.contains(requested.as_str()) { + return Err(Error::PermissionDenied(format!( + "A scope-restricted token cannot grant MCP scope '{requested}' unless the \ + caller holds the same scope verbatim" + ))); + } + continue; + } + let requested_scope = ScopeDefinition::from_scope_string(requested)?; + let covered = parsed_caller + .iter() + .any(|caller_scope| scope_contains(caller_scope, &requested_scope)); + if !covered { + return Err(Error::PermissionDenied(format!( + "A scope-restricted token cannot grant scope '{requested}' which exceeds its \ + own scopes" + ))); + } + } + } + + // `if_jobs:filter_tags:` fences which job tags a token can run on (enforced + // at job operations as `v2_job.tag = ANY(...)`), and is checked independently + // of domain/action/resource subset. A caller restricted by filter_tags must + // not be able to mint or rescope a credential that drops or widens the fence + // — even if the caller has no other scope restrictions (filter_tags-only + // tokens otherwise look "unscoped" to `scope_restrictions`). + if let Some(caller_tags) = first_filter_tags(authed.scopes.as_deref()) { + let Some(requested_tags) = first_filter_tags(requested_scopes) else { + return Err(Error::PermissionDenied( + "A token restricted by if_jobs:filter_tags cannot mint or rescope a token that \ + drops the tag restriction" + .to_string(), + )); + }; + let caller_set: std::collections::HashSet<&str> = caller_tags.iter().copied().collect(); + for tag in &requested_tags { + if !caller_set.contains(tag) { + return Err(Error::PermissionDenied(format!( + "A token restricted by if_jobs:filter_tags cannot grant tag '{tag}' which is \ + not within its own filter_tags" + ))); + } + } + } + + Ok(()) +} + +/// Tags from the first `if_jobs:filter_tags:` scope, matching the +/// semantics of [`get_scope_tags`] (which is what the job runtime consults). +/// Returns `None` if no such scope is present. +fn first_filter_tags(scopes: Option<&[String]>) -> Option> { + scopes?.iter().find_map(|s| { + s.strip_prefix("if_jobs:filter_tags:") + .map(|tags| tags.split(',').collect()) + }) +} + +/// Whether `caller` grants at least everything `requested` grants (directional +/// containment). +/// +/// This is intentionally NOT `ScopeDefinition::includes`: that method answers +/// "does this scope grant access to a required action" using OR semantics over +/// resources (any overlap counts, and a `*` on either side matches), which is +/// correct for access checks but unsafe for subset checks — it would let a +/// token scoped to `scripts:read:f/team/a` mint `scripts:read:*` or +/// `scripts:read:f/team/a,f/other/b`. Subset containment instead requires that +/// EVERY requested resource is covered by SOME caller resource. +fn scope_contains(caller: &ScopeDefinition, requested: &ScopeDefinition) -> bool { + if caller.domain != requested.domain { + return false; + } + + // write subsumes read; otherwise the action must match exactly. + match (caller.action.as_str(), requested.action.as_str()) { + (c, r) if c == r || (c == "write" && r == "read") => {} + _ => return false, + } + + if caller.domain == "jobs" && caller.action == "run" { + match (&caller.kind, &requested.kind) { + (Some(caller_kind), Some(requested_kind)) if caller_kind != requested_kind => { + return false + } + // Caller pinned to a kind, but the request covers any kind. + (Some(_), None) => return false, + _ => {} + } + } + + match (&caller.resource, &requested.resource) { + // Caller is unrestricted on resources: covers everything. + (None, _) => true, + // Caller is resource-restricted but the request is not: broader. + (Some(_), None) => false, + (Some(caller_resources), Some(requested_resources)) => { + resource_set_contains(caller_resources, requested_resources) + } + } +} + +/// Every resource in `requested` must be covered by some resource in `caller`. +fn resource_set_contains(caller: &[String], requested: &[String]) -> bool { + if caller.iter().any(|r| r == "*") { + return true; + } + requested + .iter() + .all(|req| req != "*" && caller.iter().any(|c| resource_covers(c, req))) +} + +/// Directional: does the single caller resource pattern cover `requested`? +/// `caller` may be an exact path or a `/*` subtree wildcard; `requested` +/// may itself be a subtree wildcard, in which case the whole requested subtree +/// must fall within the caller's subtree. +fn resource_covers(caller: &str, requested: &str) -> bool { + if caller == requested { + return true; + } + let Some(prefix) = caller.strip_suffix("/*") else { + // An exact caller resource only covers itself (handled above). + return false; + }; + let requested_base = requested.strip_suffix("/*").unwrap_or(requested); + requested_base == prefix + || (requested_base.starts_with(prefix) + && requested_base.as_bytes().get(prefix.len()) == Some(&b'/')) +} + /// Returns a predicate that checks whether `path` is within the token's /// scope for `{domain}:{action}:{path}`. For tokens without scope /// restrictions (no scopes at all, or only `if_jobs:filter_tags:*` scopes), @@ -574,6 +770,13 @@ impl NewToken { } } +/// Low-level token mint shared by trusted callers (the user-facing +/// `tokens/create` handler and internal mints such as native-trigger webhook +/// tokens). It does NOT enforce that `token_config.scopes` is within the +/// caller's own scopes — callers exposed to untrusted input must call +/// [`ensure_scopes_within_caller`] first (internal narrowing mints intentionally +/// skip it, since their scopes derive from the action being authorized, not the +/// caller's token). pub async fn create_token_internal( tx: &mut sqlx::PgConnection, db: &DB, @@ -908,4 +1111,278 @@ mod tests { assert!(allowed("u/alice/foo")); assert!(!allowed("u/alice/bar")); } + + fn opt_scopes(scopes: Option>) -> Option> { + scopes.map(|v| v.into_iter().map(String::from).collect()) + } + + // Regression tests for WIN-1999: scoped user tokens must not be able to + // mint or rescope credentials with broader privileges than themselves. + + #[test] + fn unscoped_caller_can_grant_anything() { + let authed = authed_with_scopes(None); + assert!(ensure_scopes_within_caller(&authed, None).is_ok()); + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["jobs:run:scripts"])).as_deref() + ) + .is_ok()); + } + + #[test] + fn filter_tags_only_caller_is_unrestricted_on_domain_action_dimension() { + // The domain/action/resource subset check treats filter-tags-only as + // unrestricted, mirroring check_scopes/check_route_access. The tag + // dimension is checked separately (see filter_tags_dimension_is_monotonic). + let authed = authed_with_scopes(Some(vec!["if_jobs:filter_tags:default"])); + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["users:write", "if_jobs:filter_tags:default"])).as_deref() + ) + .is_ok()); + } + + #[test] + fn filter_tags_dimension_is_monotonic() { + // Caller restricted to tag fence "a" cannot drop the fence … + let single = authed_with_scopes(Some(vec!["if_jobs:filter_tags:a"])); + assert!(ensure_scopes_within_caller(&single, None).is_err()); + assert!( + ensure_scopes_within_caller(&single, opt_scopes(Some(vec!["users:read"])).as_deref()) + .is_err(), + "minting a token without filter_tags must be rejected" + ); + // … cannot widen to a tag it lacks … + assert!(ensure_scopes_within_caller( + &single, + opt_scopes(Some(vec!["if_jobs:filter_tags:a,b"])).as_deref() + ) + .is_err()); + // … and cannot mint a token fenced on a disjoint tag. + assert!(ensure_scopes_within_caller( + &single, + opt_scopes(Some(vec!["if_jobs:filter_tags:b"])).as_deref() + ) + .is_err()); + // Narrowing or matching the tag fence is allowed. + let multi = authed_with_scopes(Some(vec!["if_jobs:filter_tags:a,b"])); + assert!(ensure_scopes_within_caller( + &multi, + opt_scopes(Some(vec!["if_jobs:filter_tags:a"])).as_deref() + ) + .is_ok()); + assert!(ensure_scopes_within_caller( + &multi, + opt_scopes(Some(vec!["if_jobs:filter_tags:a,b"])).as_deref() + ) + .is_ok()); + // A caller with a real scope plus a tag fence cannot drop just the fence. + let mixed = authed_with_scopes(Some(vec!["jobs:run:scripts", "if_jobs:filter_tags:a"])); + assert!(ensure_scopes_within_caller( + &mixed, + opt_scopes(Some(vec!["jobs:run:scripts"])).as_deref() + ) + .is_err()); + assert!(ensure_scopes_within_caller( + &mixed, + opt_scopes(Some(vec!["jobs:run:scripts", "if_jobs:filter_tags:a"])).as_deref() + ) + .is_ok()); + // An unrestricted caller may grant filter_tags freely. + let unscoped = authed_with_scopes(None); + assert!(ensure_scopes_within_caller( + &unscoped, + opt_scopes(Some(vec!["if_jobs:filter_tags:x"])).as_deref() + ) + .is_ok()); + } + + #[test] + fn scoped_caller_cannot_mint_unscoped_token() { + // Primitive 2 in the report: a users:write token minting an unscoped token. + let authed = authed_with_scopes(Some(vec!["users:write"])); + assert!(ensure_scopes_within_caller(&authed, None).is_err()); + // Empty scope list is effectively unscoped and must also be rejected. + assert!(ensure_scopes_within_caller(&authed, Some(&[])).is_err()); + // A scope list of only tag filters is effectively unscoped too. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["if_jobs:filter_tags:default"])).as_deref() + ) + .is_err()); + } + + #[test] + fn scoped_caller_cannot_remove_its_own_scopes() { + // Primitive 3 in the report: a users:write token setting its scopes to null. + let authed = authed_with_scopes(Some(vec!["users:write"])); + assert!(ensure_scopes_within_caller(&authed, None).is_err()); + } + + #[test] + fn scoped_caller_cannot_grant_scope_it_lacks() { + let authed = authed_with_scopes(Some(vec!["users:write"])); + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["jobs:run:scripts"])).as_deref() + ) + .is_err()); + } + + #[test] + fn scoped_caller_can_grant_subset_of_own_scopes() { + let authed = authed_with_scopes(Some(vec!["users:write", "jobs:run:scripts"])); + // Equal scope is allowed. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["jobs:run:scripts"])).as_deref() + ) + .is_ok()); + // write implies read, so a narrower read scope is allowed. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["users:read"])).as_deref() + ) + .is_ok()); + // Tag filters narrow further and are always permitted. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["users:read", "if_jobs:filter_tags:default"])).as_deref() + ) + .is_ok()); + } + + #[test] + fn scoped_caller_cannot_broaden_resource_scope() { + let authed = authed_with_scopes(Some(vec!["scripts:read:f/team/*"])); + // Narrower resource within the subtree is allowed. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["scripts:read:f/team/sub"])).as_deref() + ) + .is_ok()); + // A nested subtree within the caller's subtree is allowed. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["scripts:read:f/team/sub/*"])).as_deref() + ) + .is_ok()); + // The subtree root itself is allowed. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["scripts:read:f/team"])).as_deref() + ) + .is_ok()); + // A path outside the subtree is rejected. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["scripts:read:f/other/x"])).as_deref() + ) + .is_err()); + // read caller cannot grant write. + assert!(ensure_scopes_within_caller( + &authed, + opt_scopes(Some(vec!["scripts:write:f/team/db"])).as_deref() + ) + .is_err()); + } + + #[test] + fn mcp_scopes_require_byte_identical_match() { + // Regression for the access-grant-OR vs runtime-MCP-parser confusion: + // ScopeDefinition treats `mcp:scripts` as an unrestricted-resource scope + // and `mcp:scripts:*` as a strictly narrower one, so generic containment + // would silently allow widening. The MCP runtime however ignores + // `mcp:scripts` (unrecognized) while `mcp:scripts:*` grants all scripts. + // Legitimate MCP token issuance is the OAuth gateway, not these + // user-token endpoints, so MCP scopes must match the caller verbatim. + + // The bypass the reviewer flagged: malformed `mcp:scripts` would widen + // into the real `mcp:scripts:*` under generic containment. + let bypass = authed_with_scopes(Some(vec!["users:write", "mcp:scripts"])); + assert!(ensure_scopes_within_caller( + &bypass, + opt_scopes(Some(vec!["users:write", "mcp:scripts:*"])).as_deref() + ) + .is_err()); + + // A caller without any MCP scope cannot grant one (widening on the MCP + // dimension), even if the rest of the requested scopes are within reach. + let no_mcp = authed_with_scopes(Some(vec!["users:write"])); + assert!(ensure_scopes_within_caller( + &no_mcp, + opt_scopes(Some(vec!["users:write", "mcp:scripts:*"])).as_deref() + ) + .is_err()); + + // Byte-identical MCP scope passes; an additional non-matching MCP scope + // alongside it does not. + let mcp_caller = authed_with_scopes(Some(vec!["mcp:scripts:*"])); + assert!(ensure_scopes_within_caller( + &mcp_caller, + opt_scopes(Some(vec!["mcp:scripts:*"])).as_deref() + ) + .is_ok()); + assert!(ensure_scopes_within_caller( + &mcp_caller, + opt_scopes(Some(vec!["mcp:scripts:*", "mcp:flows:*"])).as_deref() + ) + .is_err()); + + // Even a narrowing within MCP semantics (`mcp:all` → `mcp:scripts:*`) + // is rejected by the byte-identical rule. This is intentional — these + // endpoints are not the legitimate path for narrowing MCP tokens. + let mcp_all = authed_with_scopes(Some(vec!["mcp:all"])); + assert!(ensure_scopes_within_caller( + &mcp_all, + opt_scopes(Some(vec!["mcp:scripts:*"])).as_deref() + ) + .is_err()); + } + + #[test] + fn scoped_caller_cannot_escalate_to_wildcard_or_superset() { + // Regression for the access-grant-OR vs subset-containment confusion: + // ScopeDefinition::includes would (incorrectly) allow all of these. + let star = authed_with_scopes(Some(vec!["scripts:read:f/team/a"])); + // Minting `*` from a single-path scope must be rejected. + assert!(ensure_scopes_within_caller( + &star, + opt_scopes(Some(vec!["scripts:read:*"])).as_deref() + ) + .is_err()); + // Minting a broader subtree must be rejected. + assert!(ensure_scopes_within_caller( + &star, + opt_scopes(Some(vec!["scripts:read:f/team/*"])).as_deref() + ) + .is_err()); + + // A comma-separated list that adds an uncovered resource must be rejected, + // even though one element overlaps the caller's scope. + let list = authed_with_scopes(Some(vec!["scripts:read:f/team/a"])); + assert!(ensure_scopes_within_caller( + &list, + opt_scopes(Some(vec!["scripts:read:f/team/a,f/other/b"])).as_deref() + ) + .is_err()); + + // A subset of a multi-resource caller scope is allowed. + let multi = authed_with_scopes(Some(vec!["scripts:read:f/team/a,f/team/b"])); + assert!(ensure_scopes_within_caller( + &multi, + opt_scopes(Some(vec!["scripts:read:f/team/a"])).as_deref() + ) + .is_ok()); + + // A wildcard caller covers any subset, but not `*`-less escalation rules apply + // only when the caller itself lacks `*`. + let wildcard = authed_with_scopes(Some(vec!["scripts:read:*"])); + assert!(ensure_scopes_within_caller( + &wildcard, + opt_scopes(Some(vec!["scripts:read:f/team/a"])).as_deref() + ) + .is_ok()); + } } diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 405efba4e5..95b9527a8e 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -1964,7 +1964,8 @@ async fn login( windmill_common::login_rate_limit::record_login_failure(&email); Err(Error::BadRequest("Invalid login".to_string())) } else { - let token = create_session_token(&email, super_admin, &mut tx, cookies).await?; + let token = + create_session_token(&email, super_admin, None, false, &mut tx, cookies).await?; let audit_author = AuditAuthor { email: email.clone(), @@ -2036,7 +2037,15 @@ async fn refresh_token( .await? .unwrap_or(false); - let new_token = create_session_token(&authed.email, super_admin, &mut tx, cookies).await?; + let new_token = create_session_token( + &authed.email, + super_admin, + authed.scopes.as_deref(), + authed.read_only, + &mut tx, + cookies, + ) + .await?; audit_log( &mut *tx, @@ -2066,6 +2075,8 @@ lazy_static::lazy_static! { pub async fn create_session_token<'c>( email: &str, super_admin: bool, + scopes: Option<&[String]>, + read_only: bool, tx: &mut sqlx::Transaction<'c, sqlx::Postgres>, cookies: Cookies, ) -> Result { @@ -2108,15 +2119,17 @@ pub async fn create_session_token<'c>( sqlx::query!( "INSERT INTO token - (token_hash, token_prefix, token, email, label, expiration, super_admin) - VALUES ($1, $2, $3, $4, $5, now() + ($6 || ' seconds')::interval, $7)", + (token_hash, token_prefix, token, email, label, expiration, super_admin, scopes, read_only) + VALUES ($1, $2, $3, $4, $5, now() + ($6 || ' seconds')::interval, $7, $8, $9)", t_hash, t_prefix, plaintext as Option<&str>, email, "session", &MAX_SESSION_VALIDITY_SECONDS.to_string(), - super_admin + super_admin, + scopes, + read_only, ) .execute(&mut **tx) .await?; @@ -2146,6 +2159,8 @@ async fn create_token( ) -> Result<(StatusCode, String)> { check_token_create_rate_limit(&authed.username)?; + windmill_api_auth::ensure_scopes_within_caller(&authed, token_config.scopes.as_deref())?; + let mut tx = db.begin().await?; let token = create_token_internal(&mut *tx, &db, &authed, token_config).await?; @@ -2353,6 +2368,8 @@ async fn update_token_scopes( Path(token_prefix): Path, Json(req): Json, ) -> Result { + windmill_api_auth::ensure_scopes_within_caller(&authed, req.scopes.as_deref())?; + let mut tx = db.begin().await?; let updated: Option = sqlx::query_scalar!(