From 4b1bea8aed51eb9e24940d89d984ce32f375ab0c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 19 May 2026 17:40:55 +0000 Subject: [PATCH] fix: enforce auth guards on app component preview execution (#9235) * fix: enforce auth guards on app component preview execution Co-Authored-By: Claude Opus 4.7 (1M context) * fix: guard previewed runnable path and worker tag in app preview Co-Authored-By: Claude Opus 4.7 (1M context) * fix: validate app_script id ownership and keep root push isolation Co-Authored-By: Claude Opus 4.7 (1M context) * refactor: scope app preview guards to operator check + referenced runnables Co-Authored-By: Claude Opus 4.7 (1M context) * fix: require jobs:run scope and tag check on app preview (apps:run escalation) Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- ...349fa8bd7d23e4589b6936e0d000745cb3f34.json | 23 ++ backend/tests/app_preview_auth.rs | 258 ++++++++++++++++++ backend/tests/fixtures/app_preview_auth.sql | 34 +++ backend/windmill-api/src/apps.rs | 64 ++++- 4 files changed, 378 insertions(+), 1 deletion(-) create mode 100644 backend/.sqlx/query-8816bbe1ea9ea4f359e7a95857d349fa8bd7d23e4589b6936e0d000745cb3f34.json create mode 100644 backend/tests/app_preview_auth.rs create mode 100644 backend/tests/fixtures/app_preview_auth.sql diff --git a/backend/.sqlx/query-8816bbe1ea9ea4f359e7a95857d349fa8bd7d23e4589b6936e0d000745cb3f34.json b/backend/.sqlx/query-8816bbe1ea9ea4f359e7a95857d349fa8bd7d23e4589b6936e0d000745cb3f34.json new file mode 100644 index 0000000000..53fe03ce5f --- /dev/null +++ b/backend/.sqlx/query-8816bbe1ea9ea4f359e7a95857d349fa8bd7d23e4589b6936e0d000745cb3f34.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT a.path FROM app_script s JOIN app a ON a.id = s.app\n WHERE s.id = $1 AND a.workspace_id = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Int8", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "8816bbe1ea9ea4f359e7a95857d349fa8bd7d23e4589b6936e0d000745cb3f34" +} diff --git a/backend/tests/app_preview_auth.rs b/backend/tests/app_preview_auth.rs new file mode 100644 index 0000000000..4653303392 --- /dev/null +++ b/backend/tests/app_preview_auth.rs @@ -0,0 +1,258 @@ +//! Regression test for the app component preview authorization bypass. +//! +//! `POST /api/w/:workspace/apps_u/execute_component/:path` runs in "preview" +//! mode whenever the client supplies `force_viewer_static_fields`. In that +//! mode it accepts request-supplied `raw_code` and enqueues it as a +//! `Viewer`-mode job — i.e. it is the app-editor equivalent of +//! `/jobs/run/preview`. The bug was that this branch did not re-apply the +//! guards `/jobs/run/preview` enforces for arbitrary code execution, so an +//! authenticated Operator (a run-only user who must not be able to create +//! scripts/apps or run preview jobs) could enqueue arbitrary worker code with +//! a single request, escaping the Operator restriction entirely. +//! +//! This test pins down: +//! - an Operator is rejected from preview mode (the core fix; pre-fix this +//! enqueued a job and returned 200), +//! - a regular non-operator member can still run an editor preview (the fix +//! must not over-block the legitimate editor flow), +//! - preview is confined to paths the caller can read (defense-in-depth +//! against scoped tokens / cross-namespace preview), and +//! - run mode (no `force_viewer_static_fields`) is unaffected by the guard. + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + builder.header("Authorization", format!("Bearer {}", token)) +} + +/// A preview request: `force_viewer_static_fields` present + inline `raw_code`. +/// This is the exact shape an attacker (or the editor) sends. +fn preview_body(app_path: &str) -> serde_json::Value { + json!({ + "args": {}, + "component": "comp", + "raw_code": { + "language": "deno", + "content": "export function main() { return \"pwned\"; }", + "path": format!("{}/comp", app_path) + }, + "force_viewer_static_fields": {} + }) +} + +#[sqlx::test(fixtures("base", "app_preview_auth"))] +async fn test_app_preview_authorization(db: Pool) -> 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/apps_u/execute_component"); + + // 1. CORE REGRESSION: an Operator sends a preview request in their own + // namespace (so the *only* thing that can reject them is the Operator + // check itself). Pre-fix this returned 200 with an enqueued job UUID; + // post-fix it must be rejected. + let resp = authed( + client().post(format!("{base}/u/operator-user/myapp")), + "OPERATOR_TOKEN", + ) + .json(&preview_body("u/operator-user/myapp")) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 401, + "Operator must be rejected from app preview (got {status}): {body}" + ); + assert!( + body.contains("Operators cannot run preview jobs"), + "rejection must be the operator guard, got: {body}" + ); + + // 2. The fix must NOT over-block the legitimate editor flow: a regular + // non-operator member previewing in their own namespace still works + // (the endpoint returns the enqueued job UUID before any worker runs). + let resp = authed( + client().post(format!("{base}/u/test-user-2/myapp")), + "SECRET_TOKEN_2", + ) + .json(&preview_body("u/test-user-2/myapp")) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status.is_success(), + "non-operator editor preview must still succeed (got {status}): {body}" + ); + assert!( + uuid::Uuid::parse_str(body.trim()).is_ok(), + "successful preview must return a job UUID, got: {body}" + ); + + // 3. Inline `raw_code` preview is deliberately NOT path-gated: a + // non-operator can already run arbitrary inline code via + // `/jobs/run/preview`, so the app URL path string is irrelevant for the + // inline case. This pins that decision so an over-restrictive path check + // is not re-added for inline previews. + let resp = authed( + client().post(format!("{base}/u/test-user/secretapp")), + "SECRET_TOKEN_2", + ) + .json(&preview_body("u/test-user/secretapp")) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status.is_success(), + "inline raw_code preview must not be path-gated (got {status}): {body}" + ); + assert!( + uuid::Uuid::parse_str(body.trim()).is_ok(), + "inline preview should enqueue a job UUID, got: {body}" + ); + + // 4. Run mode (no `force_viewer_static_fields`) is unaffected by the new + // preview guard: an Operator hitting a deployed-app path still follows + // the pre-existing policy lookup (here: the app does not exist -> 404), + // proving the guard only gates preview mode. + let resp = authed( + client().post(format!("{base}/u/operator-user/nonexistent")), + "OPERATOR_TOKEN", + ) + .json(&json!({ + "args": {}, + "component": "comp", + "raw_code": { + "language": "deno", + "content": "export function main() { return 1; }", + "path": "u/operator-user/nonexistent/comp" + } + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 404, + "run mode must be unchanged (deployed app lookup -> 404, not the preview guard); got {status}: {body}" + ); + + // 5. Defense-in-depth: the guard must check the *runnable* being previewed, + // not just the app URL path. A caller pairs an allowed app path + // (`u/test-user-2/myapp`, own namespace) with a `path` pointing at a + // deployed runnable in another user's namespace. Without checking the + // runnable path this would resolve `script/u/test-user/private` with the + // root DB handle and enqueue it; it must be rejected by the path check. + let resp = authed( + client().post(format!("{base}/u/test-user-2/myapp")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "args": {}, + "component": "comp", + "path": "script/u/test-user/private", + "force_viewer_static_fields": {} + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "preview targeting a runnable outside the caller's namespace must be rejected even with an allowed app path (got {status}): {body}" + ); + + // 6. Defense-in-depth: a persisted inline-script preview selects code by the + // caller-controlled `app_script` id. Pairing an allowed app path with an + // id owned by another (private) app must be rejected — without the + // id-ownership check the worker would fetch and run that app's code. + let resp = authed( + client().post(format!("{base}/u/test-user-2/myapp")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "args": {}, + "component": "comp", + "id": 999777, + "raw_code": { + "language": "deno", + "content": "export function main() { return 1; }", + "path": "u/test-user-2/myapp/comp" + }, + "force_viewer_static_fields": {} + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "preview with an app_script id owned by another app must be rejected (got {status}): {body}" + ); + + // 7. The id-ownership check must NOT over-block a legitimate persisted + // inline-script preview: an id owned by an app in the caller's own + // namespace passes the guard and enqueues (returns a job UUID). + let resp = authed( + client().post(format!("{base}/u/test-user-2/ownapp")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "args": {}, + "component": "comp", + "id": 999778, + "raw_code": { + "language": "deno", + "content": "export function main() { return 1; }", + "path": "u/test-user-2/ownapp/comp" + }, + "force_viewer_static_fields": {} + })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status.is_success(), + "persisted preview for an app the caller owns must still succeed (got {status}): {body}" + ); + assert!( + uuid::Uuid::parse_str(body.trim()).is_ok(), + "successful persisted preview must return a job UUID, got: {body}" + ); + + // 8. Scope escalation: a token scoped to `apps:run` (but not `jobs:run`) + // can reach this route (it maps to the `apps` scope domain) and is not an + // Operator, but must NOT be able to enqueue arbitrary preview `raw_code`. + // `/jobs/run/preview` requires `jobs:run` for exactly this reason; the + // app preview path must enforce the same. Without the `jobs:run` check + // this enqueues a job (returns a UUID); with it, it is rejected (403). + let resp = authed( + client().post(format!("{base}/u/test-user-2/myapp")), + "APPS_RUN_TOKEN", + ) + .json(&preview_body("u/test-user-2/myapp")) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 403, + "apps:run-scoped token must not escalate to arbitrary preview code (got {status}): {body}" + ); + assert!( + body.contains("jobs:run"), + "rejection must be the jobs:run scope gate, got: {body}" + ); + + Ok(()) +} diff --git a/backend/tests/fixtures/app_preview_auth.sql b/backend/tests/fixtures/app_preview_auth.sql new file mode 100644 index 0000000000..9fcda61c51 --- /dev/null +++ b/backend/tests/fixtures/app_preview_auth.sql @@ -0,0 +1,34 @@ +-- Fixture for the app component preview authorization regression test. +-- Layered on top of `base` (which provides test-workspace, the admin +-- `test-user`/SECRET_TOKEN, and the non-operator `test-user-2`/SECRET_TOKEN_2). +-- Adds an Operator member so we can assert that Operators cannot reach the +-- arbitrary-code app preview path (`force_viewer_static_fields` + `raw_code`). + +INSERT INTO password(email, password_hash, login_type, super_admin, verified, name) + VALUES ('operator@windmill.dev', 'not-a-real-hash', 'password', false, true, 'Operator User'); + +INSERT INTO usr(workspace_id, email, username, is_admin, operator, role) VALUES + ('test-workspace', 'operator@windmill.dev', 'operator-user', false, true, 'Operator'); + +INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin) VALUES + (encode(sha256('OPERATOR_TOKEN'::bytea), 'hex'), 'OPERATOR_T', 'OPERATOR_TOKEN', 'operator@windmill.dev', 'operator token', false); + +-- A non-operator token scoped to `apps:run` but NOT `jobs:run`. It can reach +-- the `apps_u/execute_component` route (route maps to the `apps` scope domain) +-- but must not be able to enqueue arbitrary preview `raw_code`. +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}'); + +-- 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 + (999001, 'test-workspace', 'u/test-user/private', 'private app', '{}'::jsonb, '{}'); +INSERT INTO app_script (id, app, hash, code, code_sha256) VALUES + (999777, 999001, repeat('a', 64), 'export function main(){ return "secret" }', repeat('b', 64)); + +-- An app owned by `test-user-2` with its own persisted inline script, to assert +-- the id-ownership check does not over-block a legitimate persisted preview. +INSERT INTO app (id, workspace_id, path, summary, policy, versions) VALUES + (999002, 'test-workspace', 'u/test-user-2/ownapp', 'own app', '{}'::jsonb, '{}'); +INSERT INTO app_script (id, app, hash, code, code_sha256) VALUES + (999778, 999002, repeat('c', 64), 'export function main(){ return "ok" }', repeat('d', 64)); diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 9c64bb0da4..4ecbae879a 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -11,7 +11,7 @@ use crate::{ auth::{get_end_user_email, OptTokened}, db::{ApiAuthed, DB}, jobs::RunJobQuery, - users::{require_owner_of_path, OptAuthed}, + users::{require_owner_of_path, require_path_read_access_for_preview, OptAuthed}, utils::{check_scopes, WithStarredInfoQuery}, webhook_util::{WebhookMessage, WebhookShared}, HTTP_CLIENT, @@ -2138,6 +2138,54 @@ async fn execute_component( // tag from the deployed policy and ignore the request body. let is_preview = payload.force_viewer_static_fields.is_some(); + // Preview mode runs request-supplied code as a `Viewer`-mode job (the + // app-editor equivalent of `/jobs/run/preview`), so it enforces the same + // guards. Operators must never run preview jobs. `jobs:run` is required + // because this route is reachable with an `apps:run`-scoped token (the + // route maps to the `apps` scope domain), which must not be able to escalate + // to arbitrary code execution. The client-supplied inline `raw_code.tag` + // must stay within the caller's allowed worker tags. A preview can also + // *reference* an existing runnable the caller may not be allowed to read — a + // deployed script/flow via `payload.path` or a persisted `app_script` via + // `payload.id`, both resolved with the root DB handle — so those (and only + // those) are confined to paths the caller can read. Inline `raw_code` is not + // path-gated: a non-operator member can already run arbitrary inline code + // via `/jobs/run/preview`. + if is_preview { + let authed = opt_authed.as_ref().ok_or_else(|| { + Error::NotAuthorized("App component preview requires authentication".to_string()) + })?; + if authed.is_operator { + return Err(Error::NotAuthorized( + "Operators cannot run preview jobs for security reasons".to_string(), + )); + } + check_scopes(authed, || format!("jobs:run"))?; + if let Some(p) = payload.path.as_deref() { + let runnable_path = p + .strip_prefix("script/") + .or_else(|| p.strip_prefix("flow/")) + .unwrap_or(p); + require_path_read_access_for_preview(authed, &Some(runnable_path.to_string()))?; + } + if let Some(id) = payload.id { + let owner_path = sqlx::query_scalar!( + "SELECT a.path FROM app_script s JOIN app a ON a.id = s.app + WHERE s.id = $1 AND a.workspace_id = $2", + id, + &w_id, + ) + .fetch_optional(&db) + .await? + .ok_or_else(|| { + Error::NotAuthorized(format!( + "App script {id} does not belong to an app in this workspace" + )) + })?; + require_path_read_access_for_preview(authed, &Some(owner_path))?; + } + } + // Two cases here: // 1. The component is executed from the editor (i.e. in "preview" mode), then: // - The policy is set to default (in `Viewer` execution mode). @@ -2341,6 +2389,20 @@ async fn execute_component( ), _ => unreachable!(), }; + // Preview honors the client-supplied inline tag (`resolved_inline_tag`), so + // — like `/jobs/run/preview` — confine it to worker tags the caller may use + // (a `if_jobs:filter_tags`-restricted token must not escape its filter). + // `is_preview` implies an authed caller (the guard above returns otherwise). + if is_preview { + if let Some(authed) = opt_authed.as_ref() { + crate::jobs::check_tag_available_for_workspace(&db, &w_id, &tag, authed).await?; + } + } + // Identity is already resolved to the requesting user in preview mode (the + // policy is forced to `ExecutionMode::Viewer`, so the job runs as the + // caller). The enqueue stays root-isolated as before — switching the insert + // to user-RLS is not what contains the bypass (the auth guards above are) + // and would add unnecessary breakage risk to the legitimate editor flow. let tx = PushIsolationLevel::IsolatedRoot(db.clone()); let (email, permissioned_as) = if let Some(on_behalf_of) = on_behalf_of.as_ref() {