kalugin66/windmill
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: enforce self-approval check on flow resume owner endpoint (#7886)

Browse Source 
* fix: enforce self-approval check on flow resume owner endpoint

* sqlx

* fix test

* fix mcp fields

* test self aproval allowed + other user approves flow
This commit is contained in:
Alexander Petric
2026-02-10 18:14:40 -05:00
committed by GitHub
parent 7c6f34596e
commit ae553facfd
11 changed files with 415 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json generated
View File

@@ -46,11 +46,11 @@
]
},
"nullable": [
false,
false,
false,
false,
false,
true,
true,
true,
true,
true,
true,
true
]

10
backend/.sqlx/query-66e66da2ed6eace5d7ec2a41a7b11ae255f5dc212d1ff41c2905b303c8c13b18.json → backend/.sqlx/query-1a0ab65bbf2751f702fc696c1e32a7dd9524cdd806be1ad8e9ab88d4c88d3f82.json generated
View File

@@ -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"
}

12
backend/.sqlx/query-485dc289a61a06595acae28d4968f3b6b2aeb6a8aee863dc999d6c8d58397814.json → backend/.sqlx/query-5c6e158aee5db3c4bf41a0aedfa5d6c73f5e04ebfd8703dc631bc49f2e797dd4.json generated
View File

@@ -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"
}

4
backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json generated
View File

@@ -18,8 +18,8 @@
"Left": []
},
"nullable": [
false,
true
true,
false
]
},
"hash": "b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76"

4
backend/.sqlx/query-b4eb72b0274cbdce7490f63c36d0d16ee847294fadc138593a1baa417cbb3652.json generated
View File

@@ -59,7 +59,9 @@
"failure",
"command",
"approval",
"preprocessor"
"preprocessor",
"schedule_handler_old",
"dynamic_skip"
]
}
}

46
backend/tests/common/mod.rs
View File

@@ -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<Postgres>, 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,

325
backend/tests/suspend_resume.rs
View File

@@ -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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>) -> anyhow::Result<()> {

15
backend/windmill-api/src/jobs.rs
View File

@@ -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::<FlowStatus>(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<serde_json::Value>,
suspend: i32,
script_path: Option<String>,
email: Option<String>,
}
/// 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
"#,

2
backend/windmill-common/src/error.rs
View File

@@ -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(_)

1
backend/windmill-mcp/src/server/endpoints.rs
View File

@@ -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,
}

1
backend/windmill-mcp/src/server/tools.rs
View File

@@ -184,6 +184,7 @@ pub fn create_tool_from_item<T: ToolableItem, B: McpBackend>(
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,
}