mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-11 08:07:15 +00:00
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) <noreply@anthropic.com> * fix: authorize force_cancel on the ancestor it actually kills Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: fail closed when the force_cancel ancestor walk is truncated Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
co-authored by
Claude Opus 5
Claude Opus 5
parent
9b55f1d67d
commit
4b7ab64a48
+30
@@ -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"
|
||||
}
|
||||
+27
@@ -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');
|
||||
|
||||
@@ -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<Postgres>) -> 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<Postgres>) -> 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(())
|
||||
}
|
||||
|
||||
@@ -526,27 +526,17 @@ async fn cancel_job_api(
|
||||
OptAuthed(opt_authed): OptAuthed,
|
||||
opt_tokened: OptTokened,
|
||||
Extension(db): Extension<DB>,
|
||||
Extension(user_db): Extension<UserDB>,
|
||||
Path((w_id, id)): Path<(String, Uuid)>,
|
||||
Json(CancelJob { reason }): Json<CancelJob>,
|
||||
) -> error::Result<String> {
|
||||
// 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<Uuid> {
|
||||
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<DB>,
|
||||
Extension(user_db): Extension<UserDB>,
|
||||
Path((w_id, id)): Path<(String, Uuid)>,
|
||||
Json(CancelJob { reason }): Json<CancelJob>,
|
||||
) -> error::Result<String> {
|
||||
// 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() {
|
||||
|
||||
Reference in New Issue
Block a user