From 4b7ab64a48d57b8b0f91ce181eee66b155159eb5 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 27 Jul 2026 10:59:50 +0200 Subject: [PATCH] fix: enforce per-job authorization on cancel and force_cancel endpoints (#10341) * fix: enforce per-job authorization on cancel and force_cancel Co-Authored-By: Claude Opus 5 (1M context) * fix: authorize force_cancel on the ancestor it actually kills Co-Authored-By: Claude Opus 5 (1M context) * fix: fail closed when the force_cancel ancestor walk is truncated Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...bfb0b5ccdd31740cd63f2d174b3cd1e18019d.json | 30 ++++++++ backend/tests/fixtures/jobs_read_auth.sql | 27 +++++++ backend/tests/jobs_read_auth.rs | 64 ++++++++++++++++- backend/windmill-api/src/jobs.rs | 72 ++++++++++++++----- 4 files changed, 173 insertions(+), 20 deletions(-) create mode 100644 backend/.sqlx/query-a6a9a8013ac8ea8ecba8a39c3c0bfb0b5ccdd31740cd63f2d174b3cd1e18019d.json diff --git a/backend/.sqlx/query-a6a9a8013ac8ea8ecba8a39c3c0bfb0b5ccdd31740cd63f2d174b3cd1e18019d.json b/backend/.sqlx/query-a6a9a8013ac8ea8ecba8a39c3c0bfb0b5ccdd31740cd63f2d174b3cd1e18019d.json new file mode 100644 index 0000000000..c10a455d24 --- /dev/null +++ b/backend/.sqlx/query-a6a9a8013ac8ea8ecba8a39c3c0bfb0b5ccdd31740cd63f2d174b3cd1e18019d.json @@ -0,0 +1,30 @@ +{ + "db_name": "PostgreSQL", + "query": "WITH RECURSIVE queued_ancestors AS (\n SELECT j.id, j.parent_job, 0 AS depth\n FROM v2_job j JOIN v2_job_queue q USING (id)\n WHERE j.id = $1 AND j.workspace_id = $2\n UNION ALL\n SELECT j.id, j.parent_job, a.depth + 1\n FROM queued_ancestors a\n JOIN v2_job j ON j.id = a.parent_job AND j.workspace_id = $2\n JOIN v2_job_queue q ON q.id = j.id\n WHERE a.depth < $3\n )\n SELECT id AS \"id!\", depth AS \"depth!\" FROM queued_ancestors ORDER BY depth DESC LIMIT 1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id!", + "type_info": "Uuid" + }, + { + "ordinal": 1, + "name": "depth!", + "type_info": "Int4" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Text", + "Int4" + ] + }, + "nullable": [ + null, + null + ] + }, + "hash": "a6a9a8013ac8ea8ecba8a39c3c0bfb0b5ccdd31740cd63f2d174b3cd1e18019d" +} diff --git a/backend/tests/fixtures/jobs_read_auth.sql b/backend/tests/fixtures/jobs_read_auth.sql index 456ac8c801..03ee69b1f4 100644 --- a/backend/tests/fixtures/jobs_read_auth.sql +++ b/backend/tests/fixtures/jobs_read_auth.sql @@ -229,3 +229,30 @@ INSERT INTO public.v2_job_completed (id, workspace_id, duration_ms, status, resu '{"mid": "MID_RESULT"}'), ('88888888-8888-8888-8888-888888888888', 'test-workspace', 1000, 'success'::job_status, '{"deep": "DEEP_STEP_INHERITED"}'); + +-- 6. QUEUED nesting for force-cancel: a hidden top flow (`f/secret/qtop`) whose +-- step is a sub-flow in the visible `shared` folder (`f/shared/qmid`). Force +-- cancel walks up to the highest queued ancestor, so force-cancelling the +-- sub-flow test-user-3 CAN see would kill the top flow they cannot. +INSERT INTO public.v2_job ( + id, workspace_id, created_by, created_at, permissioned_as, permissioned_as_email, + kind, script_lang, runnable_path, tag, visible_to_owner +) VALUES ( + '66666666-6666-6666-6666-666666666666', 'test-workspace', 'test-user-2', + '2023-01-01 00:00:00', 'u/test-user-2', 'test2@windmill.dev', + 'flow', 'deno', 'f/secret/qtop', 'flow', true +); +INSERT INTO public.v2_job ( + id, workspace_id, created_by, created_at, permissioned_as, permissioned_as_email, + kind, script_lang, runnable_path, tag, visible_to_owner, + parent_job, root_job, flow_innermost_root_job +) VALUES ( + '55555555-5555-5555-5555-555555555555', 'test-workspace', 'test-user-2', + '2023-01-01 00:00:00', 'u/test-user-2', 'test2@windmill.dev', + 'flow', 'deno', 'f/shared/qmid', 'flow', true, + '66666666-6666-6666-6666-666666666666', '66666666-6666-6666-6666-666666666666', + '66666666-6666-6666-6666-666666666666' +); +INSERT INTO public.v2_job_queue (id, workspace_id, scheduled_for, running, tag) VALUES + ('66666666-6666-6666-6666-666666666666', 'test-workspace', '2023-01-01 00:00:00', true, 'flow'), + ('55555555-5555-5555-5555-555555555555', 'test-workspace', '2023-01-01 00:00:00', true, 'flow'); diff --git a/backend/tests/jobs_read_auth.rs b/backend/tests/jobs_read_auth.rs index 1eab0bffee..e95748d8de 100644 --- a/backend/tests/jobs_read_auth.rs +++ b/backend/tests/jobs_read_auth.rs @@ -23,7 +23,11 @@ //! - the "app component" affordance survives: a viewer who *launched* a job //! (created_by) running as someone else's identity can still read its result, //! - unauthenticated behavior is unchanged: anonymous jobs readable, the -//! non-anonymous victim job rejected. +//! non-anonymous victim job rejected, +//! - `queue/cancel` and `queue/force_cancel` are gated by that same access, so +//! a viewer cannot kill a run hidden from them while its owner still can, and +//! force cancel gates on the ancestor it actually kills rather than the id in +//! the URL. use sqlx::{Pool, Postgres}; use windmill_test_utils::*; @@ -42,6 +46,9 @@ const RUNNING_JOB: &str = "77777777-7777-7777-7777-777777777777"; const EMBED_OWN_JOB: &str = "12121212-1212-1212-1212-121212121212"; // A QUEUED job launched by the embed viewer (created_by test-user) — cancelable by it. const EMBED_OWN_QUEUED: &str = "13131313-1313-1313-1313-131313131313"; +// Queued sub-flow test-user-3 can see (folder `shared`), whose parent top flow they +// cannot. Force cancel walks up to that parent. +const QUEUED_VISIBLE_MID: &str = "55555555-5555-5555-5555-555555555555"; // Secrets that must never leak to an unauthorized viewer. const RESULT_SECRET: &str = "RESULT_SECRET"; @@ -347,8 +354,8 @@ async fn test_single_job_read_authorization(db: Pool) -> anyhow::Resul // ---- APP EMBED TOKEN: cancellation confined to the app's own jobs. The token // may cancel a job it launched (created_by == viewer), but `cancel_job_api` - // denies (NotFound) a job created by someone else, even though cancel - // otherwise has no per-job ownership check. + // denies (NotFound) a job created by someone else, even one the (admin) + // viewer could otherwise cancel. let (status, body) = post( &base, &format!("queue/cancel/{EMBED_OWN_QUEUED}"), @@ -598,5 +605,56 @@ async fn test_single_job_read_authorization(db: Pool) -> anyhow::Resul "owner must see the running job as started (got {status}): {body}" ); + // ---- CANCEL / FORCE_CANCEL are gated by the same per-job access as reading: + // knowing the UUID of a run hidden from you must not let you kill it. ---- + for path in [ + format!("queue/cancel/{RUNNING_JOB}"), + format!("queue/force_cancel/{RUNNING_JOB}"), + ] { + let (status, body) = post(&base, &path, Some("SECRET_TOKEN_3")).await; + assert_eq!( + status, + reqwest::StatusCode::FORBIDDEN, + "viewer must not cancel another user's job ({path}, got {status}): {body}" + ); + } + // The owner still cancels their own job (no over-blocking). Keep this last: it + // takes RUNNING_JOB out of the queue. + let (status, body) = post( + &base, + &format!("queue/cancel/{RUNNING_JOB}"), + Some("SECRET_TOKEN_2"), + ) + .await; + assert!( + status.is_success(), + "owner must still cancel their own job (got {status}): {body}" + ); + + // Force cancel kills the highest queued ancestor, not the job named in the URL, so it + // must authorize that ancestor: the viewer can see the sub-flow (asserted first, or the + // denial below would prove nothing) but not the top flow force-cancelling it would kill. + let (status, body) = get( + &base, + &format!("get/{QUEUED_VISIBLE_MID}"), + Some("SECRET_TOKEN_3"), + ) + .await; + assert!( + status.is_success(), + "viewer must be able to read the sub-flow (got {status}): {body}" + ); + let (status, body) = post( + &base, + &format!("queue/force_cancel/{QUEUED_VISIBLE_MID}"), + Some("SECRET_TOKEN_3"), + ) + .await; + assert_eq!( + status, + reqwest::StatusCode::FORBIDDEN, + "viewer must not force-cancel up into a flow they cannot see (got {status}): {body}" + ); + Ok(()) } diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 4ee13226c5..f030aaa3d1 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -526,27 +526,17 @@ async fn cancel_job_api( OptAuthed(opt_authed): OptAuthed, opt_tokened: OptTokened, Extension(db): Extension, + Extension(user_db): Extension, Path((w_id, id)): Path<(String, Uuid)>, Json(CancelJob { reason }): Json, ) -> error::Result { - // App embed tokens (the sandboxed app iframe) may cancel ONLY jobs they launched - // — their app's component runs, stamped created_by == viewer. cancel_job_api has - // no other per-job ownership check, so without this an embed token (which carries - // the viewer's identity) could cancel any job by id. NotFound (not 403) so the - // untrusted app can't probe job existence. + // Cancelling needs the same per-job access as reading: own job, admin, or RLS-visible + // directly/through a flow ancestor — which also confines app embed tokens to the + // component runs they launched. No `view_token`: a share link grants read, never the + // right to kill someone else's run. Anonymous callers are instead confined to + // anonymous-created jobs by `cancel_job`'s `require_anonymous`. if let Some(authed) = opt_authed.as_ref() { - if windmill_api_auth::scopes::has_app_embed_sentinel(authed.scopes.as_deref()) { - let created_by = sqlx::query_scalar!( - "SELECT created_by FROM v2_job WHERE id = $1 AND workspace_id = $2", - id, - &w_id - ) - .fetch_optional(&db) - .await?; - if created_by.as_deref() != Some(authed.username.as_str()) { - return Err(Error::NotFound(format!("Job {id} not found"))); - } - } + require_job_update_read_access(&db, &user_db, authed, &w_id, &id, None).await?; } let tx = db.begin().await?; @@ -656,13 +646,61 @@ async fn cancel_persistent_script_api( Ok(()) } +/// Bounds the ancestor walk below so a cyclic `parent_job` chain cannot spin forever. +/// `cancel_job` itself is unbounded, so a chain longer than this would leave the two +/// disagreeing about which job gets killed — hence the fail-closed error. +const FORCE_CANCEL_MAX_ANCESTOR_DEPTH: i32 = 500; + +/// The job a force-cancel of `id` actually kills: `cancel_job(force_cancel = true)` walks +/// up to the highest still-queued ancestor and cancels that one instead. Falls back to +/// `id` when it is not queued (the cancel is then a no-op anyway). +async fn force_cancel_target(db: &DB, w_id: &str, id: Uuid) -> error::Result { + let target = sqlx::query!( + r#"WITH RECURSIVE queued_ancestors AS ( + SELECT j.id, j.parent_job, 0 AS depth + FROM v2_job j JOIN v2_job_queue q USING (id) + WHERE j.id = $1 AND j.workspace_id = $2 + UNION ALL + SELECT j.id, j.parent_job, a.depth + 1 + FROM queued_ancestors a + JOIN v2_job j ON j.id = a.parent_job AND j.workspace_id = $2 + JOIN v2_job_queue q ON q.id = j.id + WHERE a.depth < $3 + ) + SELECT id AS "id!", depth AS "depth!" FROM queued_ancestors ORDER BY depth DESC LIMIT 1"#, + id, + w_id, + FORCE_CANCEL_MAX_ANCESTOR_DEPTH, + ) + .fetch_optional(db) + .await?; + match target { + None => Ok(id), + // Truncated: we cannot prove which job the cancel would reach, so refuse rather + // than authorize an ancestor that may not be the one killed. + Some(r) if r.depth >= FORCE_CANCEL_MAX_ANCESTOR_DEPTH => Err(Error::internal_err(format!( + "flow nesting above job {id} is too deep to authorize a force cancel" + ))), + Some(r) => Ok(r.id), + } +} + async fn force_cancel( OptAuthed(opt_authed): OptAuthed, tokened: OptTokened, Extension(db): Extension, + Extension(user_db): Extension, Path((w_id, id)): Path<(String, Uuid)>, Json(CancelJob { reason }): Json, ) -> error::Result { + // Same per-job access as `cancel_job_api`, but on the job force-cancel actually kills. + // Read visibility is inherited *down* the flow chain, so gating on `id` would let a + // caller who can only see an inner step kill a root flow hidden from them. + if let Some(authed) = opt_authed.as_ref() { + let target = force_cancel_target(&db, &w_id, id).await?; + require_job_update_read_access(&db, &user_db, authed, &w_id, &target, None).await?; + } + let tx = db.begin().await?; let audit_author: AuditAuthor = match opt_authed.as_ref() {