diff --git a/backend/tests/app_preview_auth.rs b/backend/tests/app_preview_auth.rs index 2a69004a7b..cd84e6f278 100644 --- a/backend/tests/app_preview_auth.rs +++ b/backend/tests/app_preview_auth.rs @@ -22,7 +22,9 @@ //! - run mode against a deployed Viewer app rejects caller-supplied inline //! `raw_code` whose sha is not publisher-pinned (CVE-2026-22683 residual: //! the Viewer default-triggerable fallback let any caller / an operator run -//! arbitrary code as themselves, bypassing the content-hash pin). +//! arbitrary code as themselves, bypassing the content-hash pin), and +//! - a path-qualified `apps:run|write:` token reaches only that app +//! (the route layer is resource-blind, so the handler must path-check). use serde_json::json; use sqlx::{Pool, Postgres}; @@ -344,5 +346,47 @@ async fn test_app_preview_authorization(db: Pool) -> anyhow::Result<() "rejection must be the content-hash pin (unpinned app_script sha), got: {body}" ); + // 12. Path confinement: the scope picker mints `apps:run:` / + // `apps:write:`, but the route layer matches domain + action only, so the + // handler is the only place the path is enforced. A token scoped to `vapp` must + // not execute another app's components — those run under that app's identity. + for token in ["APPS_RUN_VAPP_TOKEN", "APPS_WRITE_VAPP_TOKEN"] { + let resp = authed(client().post(format!("{base}/u/test-user/private")), token) + .json(&json!({ "args": {}, "component": "comp", "path": "script/u/test-user/private" })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 403, + "{token} must not execute an app outside its scope path (got {status}): {body}" + ); + assert!( + body.contains("apps:run:u/test-user/private"), + "rejection must be the app path scope gate, got: {body}" + ); + } + + // 13. ...and must not over-block the app it IS scoped to: both tokens clear the + // scope gate for `vapp` and reach the policy (which then rejects the unpinned + // code, as in step 9). `apps:write` covering run is what keeps an app-editor + // token working without it also holding `apps:run`. + for token in ["APPS_RUN_VAPP_TOKEN", "APPS_WRITE_VAPP_TOKEN"] { + let resp = authed(client().post(format!("{base}/u/test-user/vapp")), token) + .json(&run_mode_raw_code) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "{token} must clear the scope gate for its own app and reach the policy (got {status}): {body}" + ); + assert!( + body.contains("forbidden by policy"), + "{token} must be stopped by the policy, not the scope gate, got: {body}" + ); + } + Ok(()) } diff --git a/backend/tests/fixtures/app_preview_auth.sql b/backend/tests/fixtures/app_preview_auth.sql index b94e08f3fd..62173d687c 100644 --- a/backend/tests/fixtures/app_preview_auth.sql +++ b/backend/tests/fixtures/app_preview_auth.sql @@ -19,6 +19,13 @@ INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin) VA INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin, scopes) VALUES (encode(sha256('APPS_RUN_TOKEN'::bytea), 'hex'), 'APPS_RUN_T', 'APPS_RUN_TOKEN', 'test2@windmill.dev', 'apps:run scoped token', false, '{apps:run}'); +-- Path-qualified app scopes, as the token scope picker mints them. Both must be +-- confined to `u/test-user/vapp` on the execution route (`apps:write` covers run +-- for the same app, never for another one). +INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin, scopes) VALUES + (encode(sha256('APPS_RUN_VAPP_TOKEN'::bytea), 'hex'), 'APPS_RUN_V', 'APPS_RUN_VAPP_TOKEN', 'test2@windmill.dev', 'apps:run path-scoped token', false, '{apps:run:u/test-user/vapp}'), + (encode(sha256('APPS_WRITE_VAPP_TOKEN'::bytea), 'hex'), 'APPS_WRIT_', 'APPS_WRITE_VAPP_TOKEN', 'test2@windmill.dev', 'apps:write path-scoped token', false, '{apps:write:u/test-user/vapp}'); + -- A private app owned by `test-user` with a persisted inline script. Used to -- assert that `test-user-2` cannot preview-execute another app's app_script id. INSERT INTO app (id, workspace_id, path, summary, policy, versions) VALUES diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 4161710aaf..8af7f58842 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -440,6 +440,9 @@ fn scope_contains(caller: &ScopeDefinition, requested: &ScopeDefinition) -> bool // 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") => {} + // Apps only: `write` covers `run` (see `ScopeDefinition::includes`), so an + // app-editor token can mint the narrower run-only credential. + ("write", "run") if caller.domain == "apps" => {} _ => return false, } @@ -1570,6 +1573,25 @@ mod tests { opt_scopes(Some(vec!["users:read", "if_jobs:filter_tags:default"])).as_deref() ) .is_ok()); + // Apps `write` covers `run`, so an app-editor token can mint the run-only + // credential for the same app — but only within its own resource subtree, + // and the equivalence stays Apps-only. + let app_editor = authed_with_scopes(Some(vec!["apps:write:u/me/a", "jobs:write"])); + assert!(ensure_scopes_within_caller( + &app_editor, + opt_scopes(Some(vec!["apps:run:u/me/a"])).as_deref() + ) + .is_ok()); + assert!(ensure_scopes_within_caller( + &app_editor, + opt_scopes(Some(vec!["apps:run:u/me/b"])).as_deref() + ) + .is_err()); + assert!(ensure_scopes_within_caller( + &app_editor, + opt_scopes(Some(vec!["jobs:run"])).as_deref() + ) + .is_err()); } #[test] diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 75cbc104ac..4738fec572 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -120,6 +120,10 @@ impl ScopeDefinition { match (self.action.as_str(), other.action.as_str()) { (a, b) if (a == "write" && b == "read") || (a == b) => {} + // Apps only: `write` can rewrite the app and its policy, so it also covers + // running its components. Not general — `jobs:write` must not grant + // `jobs:run`. The resource check below still confines it to the same app. + ("write", "run") if self.domain == "apps" => {} _ => return false, } @@ -802,6 +806,15 @@ fn scope_grants_access( && route_path.is_some_and(resource_metadata_route_allowed)); } + // Apps `write` covers `run` (see `ScopeDefinition::includes`). Like every domain + // here this layer is resource-blind; the Run handlers path-check the app. + if scope_domain == ScopeDomain::Apps + && scope_action == ScopeAction::Write + && required_action == ScopeAction::Run + { + return Ok(true); + } + if !scope_action.includes(&required_action) && !(scope_domain == ScopeDomain::Jobs && required_action == ScopeAction::Read @@ -1016,6 +1029,29 @@ mod tests { assert!(check_route_access(&sc, "/api/w/test/data_metrics/list", "GET").is_err()); } + /// `apps_u/execute_component` (and the S3 upload the same components drive) is a + /// Run action, so a scoped token needs `apps:run`. `apps:write` must keep reaching + /// it too: it can rewrite the app and its policy, so withholding execution from it + /// protects nothing while breaking every app-scoped token. + #[test] + fn apps_run_routes_accept_run_and_write_scopes() { + let execute = "/api/w/test/apps_u/execute_component/u/admin/app"; + for scope in ["apps:run", "apps:write"] { + assert!( + check_route_access(&[scope.to_string()], execute, "POST").is_ok(), + "{scope} must reach execute_component" + ); + } + assert!(check_route_access(&["apps:read".to_string()], execute, "POST").is_err()); + // The write-satisfies-run allowance is confined to the apps domain. + assert!(check_route_access( + &["jobs:write".to_string()], + "/api/w/test/jobs/run/p/u/admin/script", + "POST" + ) + .is_err()); + } + #[test] fn test_new_domain_parsing() { // Test that new domains are properly parsed diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index ab711ae1d2..ad5c47c683 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -2837,15 +2837,12 @@ async fn execute_component( Json(mut payload): Json, ) -> Result { let path = path.to_path(); - // Authorize FIRST, before touching the payload: confine the app embed token (the - // only credential handed to untrusted app JS, carrying `apps:run:`) to - // the app it was minted for. The route layer can't path-check the apps domain, so - // enforce it here. Scoped to embed tokens only — other callers (anonymous, cookie, - // plain external JWT) keep their existing access; the run is still policy-gated. + // Authorize before touching the payload: the route layer is resource-blind, so a + // path-scoped caller (app embed token, or a picker-minted `apps:run|write:`) + // is confined to its own app only here. No-op for unscoped callers; anonymous ones + // are policy-gated below. if let Some(authed) = opt_authed.as_ref() { - if windmill_api_auth::scopes::has_app_embed_sentinel(authed.scopes.as_deref()) { - check_scopes(authed, || format!("apps:run:{}", path))?; - } + check_scopes(authed, || format!("apps:run:{}", path))?; } // Only honor temp_script_refs for the inline-script preview path: // preview/editor mode (force_viewer_static_fields set, == `is_preview`), @@ -3372,14 +3369,10 @@ async fn upload_s3_file_from_app( Query(query): Query, request: axum::extract::Request, ) -> JsonResult { - // Confine an app embed token (untrusted app JS) to uploading for its OWN app. - // The route is reachable with `apps:run` (RUN_PATH_ACTIONS), so without this a - // token minted for app A could drive app B's upload policy. Mirrors - // execute_component / download_s3_file; other callers are unaffected. + // Same path confinement as `execute_component`: without it a token scoped to app A + // could drive app B's upload policy. if let Some(authed) = opt_authed.as_ref() { - if windmill_api_auth::scopes::has_app_embed_sentinel(authed.scopes.as_deref()) { - check_scopes(authed, || format!("apps:run:{}", path.to_path()))?; - } + check_scopes(authed, || format!("apps:run:{}", path.to_path()))?; } let policy = if let Some(file_key_regex) = query.force_viewer_file_key_regex { // `force_viewer_*` lets the caller supply a synthetic upload policy that @@ -3991,6 +3984,19 @@ struct AppS3FileQueryWithForceViewerAllowedS3Keys { pub force_viewer_allowed_s3_keys: Option, } +/// Confine a scoped caller to THIS app's S3 files, or it could read another app's +/// through that app's on-behalf policy. Either grant reads them: `apps:read:` +/// (app embed tokens) or `apps:run:` (a run-scoped token fetching back what its +/// own runs produced). Unscoped/anonymous callers fall through to the provenance gate. +#[cfg(feature = "parquet")] +fn check_app_s3_read_scope(opt_authed: &Option, path: &str) -> Result<()> { + let Some(authed) = opt_authed.as_ref() else { + return Ok(()); + }; + check_scopes(authed, || format!("apps:run:{}", path)) + .or_else(|_| check_scopes(authed, || format!("apps:read:{}", path))) +} + #[cfg(feature = "parquet")] async fn download_s3_file_from_app( OptAuthed(opt_authed): OptAuthed, @@ -4002,14 +4008,7 @@ async fn download_s3_file_from_app( let path = path.to_path(); - // Authorize the app path first: a scoped caller (notably an app embed token, - // which carries `apps:read:`) may only download files for the app it - // was minted for — otherwise it could read another app's S3 files via that app's - // on-behalf policy. Unscoped sessions / anonymous callers pass through (the - // latter still gated by the policy allowlist in `check_if_allowed_...`). - if let Some(authed) = opt_authed.as_ref() { - check_scopes(authed, || format!("apps:read:{}", path))?; - } + check_app_s3_read_scope(&opt_authed, path)?; let force_viewer_allowed_s3_keys = if let Some(force_viewer_allowed_s3_keys) = query.force_viewer_allowed_s3_keys.clone() @@ -4087,8 +4086,8 @@ fn app_s3_file_query(s3: String, storage: Option, sig: AppS3Sig) -> AppS } } -/// Shared entry for every app-scoped (`apps_u/*`) S3 display op: scope-confine an -/// app embed token, resolve the on-behalf identity per `execution_mode`, then run +/// Shared entry for every app-scoped (`apps_u/*`) S3 display op: confine a scoped +/// caller to this app, resolve the on-behalf identity per `execution_mode`, then run /// the provenance gate (`check_if_allowed_to_access_s3_file_from_app`) once before /// dispatching to the S3 helpers. #[cfg(feature = "parquet")] @@ -4099,9 +4098,7 @@ async fn app_s3_on_behalf_and_provenance( opt_authed: &Option, file_query: &AppS3FileQuery, ) -> Result { - if let Some(authed) = opt_authed.as_ref() { - check_scopes(authed, || format!("apps:read:{}", path))?; - } + check_app_s3_read_scope(opt_authed, path)?; let (on_behalf_authed, policy) = get_on_behalf_authed_from_app(db, path, w_id, opt_authed, None).await?; let read_authed = match check_if_allowed_to_access_s3_file_from_app( diff --git a/backend/windmill-api/src/token.rs b/backend/windmill-api/src/token.rs index c7229fe056..acb65d6cf0 100644 --- a/backend/windmill-api/src/token.rs +++ b/backend/windmill-api/src/token.rs @@ -143,14 +143,8 @@ fn build_standard_scope_domains() -> Vec { STANDARD_DOMAINS .iter() - .map(|(key, name, desc, req)| ScopeDomain { - name: name.to_string(), - description: if desc.is_empty() { - None - } else { - Some(desc.to_string()) - }, - scopes: vec![ + .map(|(key, name, desc, req)| { + let mut scopes = vec![ ScopeOption { value: format!("{key}:read"), label: "Read".to_string(), @@ -161,7 +155,26 @@ fn build_standard_scope_domains() -> Vec { label: "Write".to_string(), requires_resource_path: *req, }, - ], + ]; + // `apps_u/execute_component` and `apps_u/upload_s3_file` are classified as + // Run actions, so running a deployed app's components needs `apps:run`: + // without it here no supported token can be granted that access. + if *key == "apps" { + scopes.push(ScopeOption { + value: "apps:run".to_string(), + label: "Run".to_string(), + requires_resource_path: *req, + }); + } + ScopeDomain { + name: name.to_string(), + description: if desc.is_empty() { + None + } else { + Some(desc.to_string()) + }, + scopes, + } }) .collect() } @@ -258,6 +271,22 @@ mod tests { assert!(!values.contains(&"docs:write"), "docs has no write surface"); } + /// Running a deployed app's components is enforced as a Run action, so `apps:run` + /// must be selectable here or no supported token can be granted that access. + #[test] + fn apps_run_scope_is_exposed_and_path_selectable() { + let apps = ALL_SCOPES + .iter() + .find(|d| d.name == "Apps") + .expect("Apps domain must exist"); + let opt = apps + .scopes + .iter() + .find(|s| s.value == "apps:run") + .expect("apps:run must be selectable"); + assert!(opt.requires_resource_path, "apps:run is path-scoped"); + } + /// The `data_metrics` route enforces its own scope domain, so `data_metrics:read` /// must be grantable here or no token can ever reach it. It is read-only (the /// catalog is written at deploy) and path-selectable.