From ae553facfd6cfe8a033fe8b905322a926d1064eb Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Tue, 10 Feb 2026 18:14:40 -0500 Subject: [PATCH] fix: enforce self-approval check on flow resume owner endpoint (#7886) * fix: enforce self-approval check on flow resume owner endpoint * sqlx * fix test * fix mcp fields * test self aproval allowed + other user approves flow --- ...1f7f387f5055c47f493271d26731336257384.json | 10 +- ...a7dd9524cdd806be1ad8e9ab88d4c88d3f82.json} | 10 +- ...d6c73f5e04ebfd8703dc631bc49f2e797dd4.json} | 12 +- ...0cb549a34b96554ae1872355b90304f5dcb76.json | 4 +- ...0d16ee847294fadc138593a1baa417cbb3652.json | 4 +- backend/tests/common/mod.rs | 46 +++ backend/tests/suspend_resume.rs | 325 ++++++++++++++++++ backend/windmill-api/src/jobs.rs | 15 +- backend/windmill-common/src/error.rs | 2 +- backend/windmill-mcp/src/server/endpoints.rs | 1 + backend/windmill-mcp/src/server/tools.rs | 1 + 11 files changed, 415 insertions(+), 15 deletions(-) rename backend/.sqlx/{query-66e66da2ed6eace5d7ec2a41a7b11ae255f5dc212d1ff41c2905b303c8c13b18.json => query-1a0ab65bbf2751f702fc696c1e32a7dd9524cdd806be1ad8e9ab88d4c88d3f82.json} (63%) rename backend/.sqlx/{query-485dc289a61a06595acae28d4968f3b6b2aeb6a8aee863dc999d6c8d58397814.json => query-5c6e158aee5db3c4bf41a0aedfa5d6c73f5e04ebfd8703dc631bc49f2e797dd4.json} (63%) diff --git a/backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json b/backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json index e7ed0aee65..d29a18c691 100644 --- a/backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json +++ b/backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json @@ -46,11 +46,11 @@ ] }, "nullable": [ - false, - false, - false, - false, - false, + true, + true, + true, + true, + true, true, true ] diff --git a/backend/.sqlx/query-66e66da2ed6eace5d7ec2a41a7b11ae255f5dc212d1ff41c2905b303c8c13b18.json b/backend/.sqlx/query-1a0ab65bbf2751f702fc696c1e32a7dd9524cdd806be1ad8e9ab88d4c88d3f82.json similarity index 63% rename from backend/.sqlx/query-66e66da2ed6eace5d7ec2a41a7b11ae255f5dc212d1ff41c2905b303c8c13b18.json rename to backend/.sqlx/query-1a0ab65bbf2751f702fc696c1e32a7dd9524cdd806be1ad8e9ab88d4c88d3f82.json index 6350d8a181..5b82c5288f 100644 --- a/backend/.sqlx/query-66e66da2ed6eace5d7ec2a41a7b11ae255f5dc212d1ff41c2905b303c8c13b18.json +++ b/backend/.sqlx/query-1a0ab65bbf2751f702fc696c1e32a7dd9524cdd806be1ad8e9ab88d4c88d3f82.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n WITH job_info AS (\n SELECT id, kind::text AS kind, parent_job\n FROM v2_job\n WHERE id = $1\n )\n SELECT\n q.id AS \"id!\",\n s.flow_status,\n q.suspend AS \"suspend!\",\n j.runnable_path AS script_path,\n (ji.kind IN ('flow', 'flowpreview')) AS \"is_flow_level!\"\n FROM job_info ji\n JOIN v2_job_queue q ON q.id = CASE\n WHEN ji.kind IN ('flow', 'flowpreview') THEN ji.id\n ELSE ji.parent_job\n END\n JOIN v2_job j ON j.id = q.id\n JOIN v2_job_status s ON s.id = q.id\n FOR UPDATE OF q\n ", + "query": "\n WITH job_info AS (\n SELECT id, kind::text AS kind, parent_job\n FROM v2_job\n WHERE id = $1\n )\n SELECT\n q.id AS \"id!\",\n s.flow_status,\n q.suspend AS \"suspend!\",\n j.runnable_path AS script_path,\n j.permissioned_as_email AS email,\n (ji.kind IN ('flow', 'flowpreview')) AS \"is_flow_level!\"\n FROM job_info ji\n JOIN v2_job_queue q ON q.id = CASE\n WHEN ji.kind IN ('flow', 'flowpreview') THEN ji.id\n ELSE ji.parent_job\n END\n JOIN v2_job j ON j.id = q.id\n JOIN v2_job_status s ON s.id = q.id\n FOR UPDATE OF q\n ", "describe": { "columns": [ { @@ -25,6 +25,11 @@ }, { "ordinal": 4, + "name": "email", + "type_info": "Varchar" + }, + { + "ordinal": 5, "name": "is_flow_level!", "type_info": "Bool" } @@ -39,8 +44,9 @@ true, false, true, + false, null ] }, - "hash": "66e66da2ed6eace5d7ec2a41a7b11ae255f5dc212d1ff41c2905b303c8c13b18" + "hash": "1a0ab65bbf2751f702fc696c1e32a7dd9524cdd806be1ad8e9ab88d4c88d3f82" } diff --git a/backend/.sqlx/query-485dc289a61a06595acae28d4968f3b6b2aeb6a8aee863dc999d6c8d58397814.json b/backend/.sqlx/query-5c6e158aee5db3c4bf41a0aedfa5d6c73f5e04ebfd8703dc631bc49f2e797dd4.json similarity index 63% rename from backend/.sqlx/query-485dc289a61a06595acae28d4968f3b6b2aeb6a8aee863dc999d6c8d58397814.json rename to backend/.sqlx/query-5c6e158aee5db3c4bf41a0aedfa5d6c73f5e04ebfd8703dc631bc49f2e797dd4.json index 041d969faf..d216daea9b 100644 --- a/backend/.sqlx/query-485dc289a61a06595acae28d4968f3b6b2aeb6a8aee863dc999d6c8d58397814.json +++ b/backend/.sqlx/query-5c6e158aee5db3c4bf41a0aedfa5d6c73f5e04ebfd8703dc631bc49f2e797dd4.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT j.id AS \"id!\", COALESCE(s.flow_status, s.workflow_as_code_status) as flow_status, q.suspend AS \"suspend!\", j.runnable_path as script_path\n FROM v2_job_queue q JOIN v2_job j USING (id) LEFT JOIN v2_job_status s USING (id)\n WHERE j.id = $1\n ", + "query": "\n SELECT j.id AS \"id!\", COALESCE(s.flow_status, s.workflow_as_code_status) as flow_status, q.suspend AS \"suspend!\", j.runnable_path as script_path, j.permissioned_as_email as email\n FROM v2_job_queue q JOIN v2_job j USING (id) LEFT JOIN v2_job_status s USING (id)\n WHERE j.id = $1\n ", "describe": { "columns": [ { @@ -22,6 +22,11 @@ "ordinal": 3, "name": "script_path", "type_info": "Varchar" + }, + { + "ordinal": 4, + "name": "email", + "type_info": "Varchar" } ], "parameters": { @@ -33,8 +38,9 @@ false, null, false, - true + true, + false ] }, - "hash": "485dc289a61a06595acae28d4968f3b6b2aeb6a8aee863dc999d6c8d58397814" + "hash": "5c6e158aee5db3c4bf41a0aedfa5d6c73f5e04ebfd8703dc631bc49f2e797dd4" } diff --git a/backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json b/backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json index 54e94cfb8f..99269c9851 100644 --- a/backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json +++ b/backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json @@ -18,8 +18,8 @@ "Left": [] }, "nullable": [ - false, - true + true, + false ] }, "hash": "b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76" diff --git a/backend/.sqlx/query-b4eb72b0274cbdce7490f63c36d0d16ee847294fadc138593a1baa417cbb3652.json b/backend/.sqlx/query-b4eb72b0274cbdce7490f63c36d0d16ee847294fadc138593a1baa417cbb3652.json index f1ba61b895..d16886016b 100644 --- a/backend/.sqlx/query-b4eb72b0274cbdce7490f63c36d0d16ee847294fadc138593a1baa417cbb3652.json +++ b/backend/.sqlx/query-b4eb72b0274cbdce7490f63c36d0d16ee847294fadc138593a1baa417cbb3652.json @@ -59,7 +59,9 @@ "failure", "command", "approval", - "preprocessor" + "preprocessor", + "schedule_handler_old", + "dynamic_skip" ] } } diff --git a/backend/tests/common/mod.rs b/backend/tests/common/mod.rs index d19fdac786..17e190fcb2 100644 --- a/backend/tests/common/mod.rs +++ b/backend/tests/common/mod.rs @@ -227,6 +227,52 @@ impl RunJob { uuid } + /// Push the job as a specific user (for testing permissions) + pub async fn push_as(self, db: &Pool, username: &str, email: &str) -> Uuid { + let RunJob { payload, args, scheduled_for_o, .. } = self; + let mut hm_args = std::collections::HashMap::new(); + for (k, v) in args { + hm_args.insert(k, windmill_common::worker::to_raw_value(&v)); + } + + let tx = PushIsolationLevel::IsolatedRoot(db.clone()); + let (uuid, tx) = windmill_queue::push( + db, + tx, + "test-workspace", + payload, + windmill_queue::PushArgs::from(&hm_args), + username, + email, + format!("u/{}", username), + /* token_prefix */ None, + scheduled_for_o, + /* schedule_path */ None, + /* parent_job */ None, + /* root job */ None, + /* flow_innermost_root_job */ None, + /* job_id */ None, + /* is_flow_step */ false, + /* same_worker */ false, + None, + true, + None, + None, + None, + None, + None, + false, + None, + None, + None, + ) + .await + .expect("push has to succeed"); + tx.commit().await.unwrap(); + + uuid + } + /// push the job, spawn a worker, wait until the job is in completed_job pub async fn run_until_complete( self, diff --git a/backend/tests/suspend_resume.rs b/backend/tests/suspend_resume.rs index 9c50488060..a3e4cc456f 100644 --- a/backend/tests/suspend_resume.rs +++ b/backend/tests/suspend_resume.rs @@ -237,6 +237,331 @@ mod suspend_resume { Ok(()) } + /// Test that self-approval is blocked when self_approval_disabled is true. + /// + /// This test verifies that when a flow has an approval step with self_approval_disabled=true, + /// the user who triggered the flow cannot approve it themselves via the owner endpoint + /// (POST /jobs/flow/resume/:id). + /// + /// Bug context: The owner endpoint was missing the approval condition check, allowing + /// users to bypass self-approval restrictions by using the UI resume button instead + /// of the HMAC-signed approval link. + #[cfg(feature = "enterprise")] + #[cfg(feature = "deno_core")] + #[sqlx::test(fixtures("base"))] + async fn test_self_approval_disabled_blocks_owner_resume(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // Flow with self_approval_disabled=true on the approval step + let flow_with_self_approval_disabled: FlowValue = serde_json::from_value(json!({ + "modules": [{ + "id": "a", + "value": { + "type": "rawscript", + "language": "deno", + "content": "export function main() { return 'step1'; }" + }, + "suspend": { + "required_events": 1, + "user_auth_required": true, + "self_approval_disabled": true + } + }, { + "id": "b", + "value": { + "type": "rawscript", + "language": "deno", + "content": "export function main() { return 'step2 - after approval'; }" + } + }] + })) + .unwrap(); + + // Push flow as NON-ADMIN user (test-user-2) - admins bypass self-approval check + // Use a path owned by test-user-2 so require_owner_of_path succeeds + let flow = RunJob::from(JobPayload::RawFlow { + value: flow_with_self_approval_disabled, + path: Some("u/test-user-2/test_approval".to_string()), + restarted_from: None, + }) + .push_as(&db, "test-user-2", "test2@windmill.dev") + .await; + + let queue = listen_for_queue(&db).await; + let db_ = db.clone(); + + in_test_worker( + &db, + async move { + let db = db_; + + // Wait for flow to suspend at approval step + wait_until_flow_suspends(flow, queue, &db).await; + + // Create a token for the same non-admin user who triggered the flow + // This simulates clicking "Resume" in the UI as the flow owner + // Args: db, w_id, owner, label, expires_in, email, job_id, perms, audit_span + let token = windmill_common::auth::create_token_for_owner( + &db, + "test-workspace", + "u/test-user-2", + "test-token", + 100, + "test2@windmill.dev", + &Uuid::nil(), + None, + None, + ) + .await + .unwrap(); + + // Try to resume via the owner endpoint (POST /jobs/flow/resume/:id) + // This should FAIL because self_approval_disabled=true and the user + // is the same as the one who triggered the flow + let response = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/jobs/flow/resume/{flow}" + )) + .header("Authorization", format!("Bearer {token}")) + .header("Content-Type", "application/json") + .body("{}") + .send() + .await + .unwrap(); + + let status = response.status(); + + // The request should be rejected with 403 Forbidden + // (currently this test FAILS because the bug allows self-approval) + assert!( + status == reqwest::StatusCode::FORBIDDEN, + "Self-approval should be blocked when self_approval_disabled=true. \ + Expected 403 Forbidden, got {}. Response: {}", + status, + response.text().await.unwrap_or_default() + ); + }, + port, + ) + .await; + + server.close().await.unwrap(); + Ok(()) + } + + /// Test that self-approval WORKS when self_approval_disabled is false (default behavior). + /// + /// This is the complementary test to test_self_approval_disabled_blocks_owner_resume. + /// When self_approval_disabled is NOT set, the flow owner should be able to approve + /// their own flow via the owner endpoint. + #[cfg(feature = "enterprise")] + #[cfg(feature = "deno_core")] + #[sqlx::test(fixtures("base"))] + async fn test_self_approval_allowed_when_not_disabled(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // Flow with user_auth_required but WITHOUT self_approval_disabled + let flow_without_self_approval_disabled: FlowValue = serde_json::from_value(json!({ + "modules": [{ + "id": "a", + "value": { + "type": "rawscript", + "language": "deno", + "content": "export function main() { return 'step1'; }" + }, + "suspend": { + "required_events": 1, + "user_auth_required": true + // self_approval_disabled is NOT set (defaults to false) + } + }, { + "id": "b", + "value": { + "type": "rawscript", + "language": "deno", + "content": "export function main() { return 'step2 - after approval'; }" + } + }] + })) + .unwrap(); + + // Push flow as non-admin user + let flow = RunJob::from(JobPayload::RawFlow { + value: flow_without_self_approval_disabled, + path: Some("u/test-user-2/test_approval_allowed".to_string()), + restarted_from: None, + }) + .push_as(&db, "test-user-2", "test2@windmill.dev") + .await; + + let queue = listen_for_queue(&db).await; + let db_ = db.clone(); + + in_test_worker( + &db, + async move { + let db = db_; + + // Wait for flow to suspend at approval step + wait_until_flow_suspends(flow, queue, &db).await; + + // Create a token for the same user who triggered the flow + let token = windmill_common::auth::create_token_for_owner( + &db, + "test-workspace", + "u/test-user-2", + "test-token", + 100, + "test2@windmill.dev", + &Uuid::nil(), + None, + None, + ) + .await + .unwrap(); + + // Try to resume via the owner endpoint - this SHOULD succeed + let response = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/jobs/flow/resume/{flow}" + )) + .header("Authorization", format!("Bearer {token}")) + .header("Content-Type", "application/json") + .body("{}") + .send() + .await + .unwrap(); + + let status = response.status(); + + // Self-approval should be allowed when self_approval_disabled is not set + assert!( + status.is_success(), + "Self-approval should be allowed when self_approval_disabled is not set. \ + Expected 2xx, got {}. Response: {}", + status, + response.text().await.unwrap_or_default() + ); + }, + port, + ) + .await; + + server.close().await.unwrap(); + Ok(()) + } + + /// Test that a DIFFERENT user can approve a flow even when self_approval_disabled is true. + /// + /// This verifies that the self_approval_disabled setting only blocks the flow trigger, + /// not other users. A different user should always be able to approve. + #[cfg(feature = "enterprise")] + #[cfg(feature = "deno_core")] + #[sqlx::test(fixtures("base"))] + async fn test_different_user_can_approve_when_self_approval_disabled(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // Flow with self_approval_disabled=true + let flow_with_self_approval_disabled: FlowValue = serde_json::from_value(json!({ + "modules": [{ + "id": "a", + "value": { + "type": "rawscript", + "language": "deno", + "content": "export function main() { return 'step1'; }" + }, + "suspend": { + "required_events": 1, + "user_auth_required": true, + "self_approval_disabled": true + } + }, { + "id": "b", + "value": { + "type": "rawscript", + "language": "deno", + "content": "export function main() { return 'step2 - after approval'; }" + } + }] + })) + .unwrap(); + + // Push flow as test-user-2 + let flow = RunJob::from(JobPayload::RawFlow { + value: flow_with_self_approval_disabled, + path: Some("u/test-user-2/test_approval_by_other".to_string()), + restarted_from: None, + }) + .push_as(&db, "test-user-2", "test2@windmill.dev") + .await; + + let queue = listen_for_queue(&db).await; + let db_ = db.clone(); + + in_test_worker( + &db, + async move { + let db = db_; + + // Wait for flow to suspend at approval step + wait_until_flow_suspends(flow, queue, &db).await; + + // Create a token for a DIFFERENT user (test-user, who is admin) + // This simulates a different person approving the flow + let token = windmill_common::auth::create_token_for_owner( + &db, + "test-workspace", + "u/test-user", + "test-token", + 100, + "test@windmill.dev", + &Uuid::nil(), + None, + None, + ) + .await + .unwrap(); + + // Try to resume via the owner endpoint as a different user - this SHOULD succeed + let response = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/jobs/flow/resume/{flow}" + )) + .header("Authorization", format!("Bearer {token}")) + .header("Content-Type", "application/json") + .body("{}") + .send() + .await + .unwrap(); + + let status = response.status(); + + // A different user should be able to approve even with self_approval_disabled + assert!( + status.is_success(), + "Different user should be able to approve even with self_approval_disabled=true. \ + Expected 2xx, got {}. Response: {}", + status, + response.text().await.unwrap_or_default() + ); + }, + port, + ) + .await; + + server.close().await.unwrap(); + Ok(()) + } + #[cfg(feature = "deno_core")] #[sqlx::test(fixtures("base"))] async fn cancel_after_suspend(db: Pool) -> anyhow::Result<()> { diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 4116049a40..b2cc8623d1 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -2133,6 +2133,16 @@ pub async fn resume_suspended_flow_as_owner( require_owner_of_path(&authed, flow_path)?; check_scopes(&authed, || format!("jobs:run:flows:{}", flow_path))?; + // Check approval conditions (self-approval, required groups, etc.) + if let Some(ref flow_status_value) = flow.flow_status { + if let Ok(flow_status) = + serde_json::from_value::(flow_status_value.clone()) + { + let trigger_email = flow.email.as_deref().unwrap_or(""); + conditionally_require_authed_user(Some(authed.clone()), flow_status, trigger_email)?; + } + } + let value = value.unwrap_or(serde_json::Value::Null); insert_resume_job( @@ -2411,6 +2421,7 @@ struct FlowInfo { flow_status: Option, suspend: i32, script_path: Option, + email: Option, } /// Get flow info from either a step job (by looking up its parent) or a flow job directly. @@ -2429,6 +2440,7 @@ async fn get_flow_info_for_resume(job_id: Uuid, db: &DB) -> error::Result<(FlowI s.flow_status, q.suspend AS "suspend!", j.runnable_path AS script_path, + j.permissioned_as_email AS email, (ji.kind IN ('flow', 'flowpreview')) AS "is_flow_level!" FROM job_info ji JOIN v2_job_queue q ON q.id = CASE @@ -2450,6 +2462,7 @@ async fn get_flow_info_for_resume(job_id: Uuid, db: &DB) -> error::Result<(FlowI flow_status: result.flow_status, suspend: result.suspend, script_path: result.script_path, + email: Some(result.email), }; Ok((flow_info, result.is_flow_level)) @@ -2462,7 +2475,7 @@ async fn get_suspended_flow_info<'c>( let flow = sqlx::query_as!( FlowInfo, r#" - SELECT j.id AS "id!", COALESCE(s.flow_status, s.workflow_as_code_status) as flow_status, q.suspend AS "suspend!", j.runnable_path as script_path + SELECT j.id AS "id!", COALESCE(s.flow_status, s.workflow_as_code_status) as flow_status, q.suspend AS "suspend!", j.runnable_path as script_path, j.permissioned_as_email as email FROM v2_job_queue q JOIN v2_job j USING (id) LEFT JOIN v2_job_status s USING (id) WHERE j.id = $1 "#, diff --git a/backend/windmill-common/src/error.rs b/backend/windmill-common/src/error.rs index fa1ac93f01..c5e4414fce 100644 --- a/backend/windmill-common/src/error.rs +++ b/backend/windmill-common/src/error.rs @@ -249,7 +249,7 @@ impl IntoResponse for Error { let status = match self { Self::NotFound(_) => axum::http::StatusCode::NOT_FOUND, Self::NotAuthorized(_) => axum::http::StatusCode::UNAUTHORIZED, - Self::RequireAdmin(_) => axum::http::StatusCode::FORBIDDEN, + Self::RequireAdmin(_) | Self::PermissionDenied(_) => axum::http::StatusCode::FORBIDDEN, Self::SqlErr { .. } | Self::BadRequest(_) | Self::AIError(_) diff --git a/backend/windmill-mcp/src/server/endpoints.rs b/backend/windmill-mcp/src/server/endpoints.rs index 2401b10757..362bbc84b3 100644 --- a/backend/windmill-mcp/src/server/endpoints.rs +++ b/backend/windmill-mcp/src/server/endpoints.rs @@ -62,6 +62,7 @@ pub fn endpoint_tool_to_mcp_tool(tool: &EndpointTool) -> Tool { output_schema: None, icons: None, annotations: Some(annotations), + execution: None, meta: None, execution: None, } diff --git a/backend/windmill-mcp/src/server/tools.rs b/backend/windmill-mcp/src/server/tools.rs index a9262cdd6f..821833978c 100644 --- a/backend/windmill-mcp/src/server/tools.rs +++ b/backend/windmill-mcp/src/server/tools.rs @@ -184,6 +184,7 @@ pub fn create_tool_from_item( idempotent_hint: Some(false), // Are not guaranteed to be idempotent open_world_hint: Some(true), // Can interact with external services }), + execution: None, meta: None, execution: None, }