From 40f0cab2adbdfbf1bfb12b7fbc3e419951fc8179 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Sat, 22 Aug 2026 12:01:25 +0200 Subject: [PATCH] fix: scope capture deletion to the workspace in the request path (#10795) * fix: scope capture deletion to the workspace in the request path Co-Authored-By: Claude Opus 5 * test: layer the capture fixture on base instead of duplicating it Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- ...4044f505f49edcd2ebeee4a77eae737f48795.json | 14 ----- ...1f786a5451847ca5c95b93203b75b6d56ae34.json | 15 +++++ backend/tests/capture_cross_workspace.rs | 55 +++++++++++++++++++ .../fixtures/capture_cross_workspace.sql | 25 +++++++++ backend/windmill-api/src/capture.rs | 12 ++-- 5 files changed, 103 insertions(+), 18 deletions(-) delete mode 100644 backend/.sqlx/query-7d5a161d916cb8d1485f8d72e6b4044f505f49edcd2ebeee4a77eae737f48795.json create mode 100644 backend/.sqlx/query-f1a6018b184967f0065847ce5e41f786a5451847ca5c95b93203b75b6d56ae34.json create mode 100644 backend/tests/capture_cross_workspace.rs create mode 100644 backend/tests/fixtures/capture_cross_workspace.sql diff --git a/backend/.sqlx/query-7d5a161d916cb8d1485f8d72e6b4044f505f49edcd2ebeee4a77eae737f48795.json b/backend/.sqlx/query-7d5a161d916cb8d1485f8d72e6b4044f505f49edcd2ebeee4a77eae737f48795.json deleted file mode 100644 index e1ab284262..0000000000 --- a/backend/.sqlx/query-7d5a161d916cb8d1485f8d72e6b4044f505f49edcd2ebeee4a77eae737f48795.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n DELETE FROM \n capture\n WHERE \n id = $1\n ", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Int8" - ] - }, - "nullable": [] - }, - "hash": "7d5a161d916cb8d1485f8d72e6b4044f505f49edcd2ebeee4a77eae737f48795" -} diff --git a/backend/.sqlx/query-f1a6018b184967f0065847ce5e41f786a5451847ca5c95b93203b75b6d56ae34.json b/backend/.sqlx/query-f1a6018b184967f0065847ce5e41f786a5451847ca5c95b93203b75b6d56ae34.json new file mode 100644 index 0000000000..2c67e4a0fd --- /dev/null +++ b/backend/.sqlx/query-f1a6018b184967f0065847ce5e41f786a5451847ca5c95b93203b75b6d56ae34.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "\n DELETE FROM\n capture\n WHERE\n id = $1\n AND workspace_id = $2\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int8", + "Text" + ] + }, + "nullable": [] + }, + "hash": "f1a6018b184967f0065847ce5e41f786a5451847ca5c95b93203b75b6d56ae34" +} diff --git a/backend/tests/capture_cross_workspace.rs b/backend/tests/capture_cross_workspace.rs new file mode 100644 index 0000000000..1ec90e62d4 --- /dev/null +++ b/backend/tests/capture_cross_workspace.rs @@ -0,0 +1,55 @@ +//! `DELETE /w/{workspace}/capture/{id}` must stay inside the workspace in the URL. +//! +//! Capture ids come from one instance-wide sequence and the capture RLS policies +//! key on the path segment only, never on `workspace_id` — so an id alone is not +//! an authorization boundary. A member of one workspace can name any id and, if +//! the row's path happens to sit inside their grants (`u//…`, +//! `g//…`, a same-named folder), reach a row belonging to a +//! workspace they are not a member of. + +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +async fn capture_workspace(db: &Pool, id: i64) -> Option { + sqlx::query_scalar::<_, String>("SELECT workspace_id FROM capture WHERE id = $1") + .bind(id) + .fetch_optional(db) + .await + .unwrap() +} + +#[sqlx::test(fixtures("base", "capture_cross_workspace"))] +async fn delete_capture_is_confined_to_the_url_workspace(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let base = format!( + "http://localhost:{}/api/w/test-workspace", + server.addr.port() + ); + let client = reqwest::Client::new(); + + // Capture 1 lives in test-workspace-2, which this member has no access to. + let resp = client + .delete(format!("{base}/capture/1")) + .header("Authorization", "Bearer SECRET_TOKEN_2") + .send() + .await?; + let status = resp.status(); + assert_eq!( + capture_workspace(&db, 1).await.as_deref(), + Some("test-workspace-2"), + "capture of another workspace was deleted (status {status})" + ); + + // The member's own capture in test-workspace still deletes. + let resp = client + .delete(format!("{base}/capture/2")) + .header("Authorization", "Bearer SECRET_TOKEN_2") + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + assert_eq!(capture_workspace(&db, 2).await, None); + + Ok(()) +} diff --git a/backend/tests/fixtures/capture_cross_workspace.sql b/backend/tests/fixtures/capture_cross_workspace.sql new file mode 100644 index 0000000000..b662716b58 --- /dev/null +++ b/backend/tests/fixtures/capture_cross_workspace.sql @@ -0,0 +1,25 @@ +-- Layers on `base`, which already provides test-workspace and `test-user-2`, a +-- plain non-admin member of it (token SECRET_TOKEN_2). +-- +-- Adds a second workspace holding a capture under `u/test-user-2/…`: the capture +-- policies key on the path segment alone, so that path is inside the member's +-- grants in *every* workspace. + +INSERT INTO workspace (id, name, owner) VALUES + ('test-workspace-2', 'test-workspace-2', 'test-user'); + +INSERT INTO workspace_key(workspace_id, kind, key) VALUES + ('test-workspace-2', 'cloud', 'test-key-2'); + +INSERT INTO workspace_settings (workspace_id) VALUES + ('test-workspace-2'); + +INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES + ('test-workspace-2', 'all', 'All users', '{}'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('test-workspace-2', 'test@windmill.dev', 'test-user', true, 'Admin'); + +INSERT INTO capture (id, workspace_id, path, created_by, main_args, is_flow, trigger_kind) VALUES + (1, 'test-workspace-2', 'u/test-user-2/victim', 'test-user', '{"secret": "other workspace payload"}'::jsonb, false, 'webhook'), + (2, 'test-workspace', 'u/test-user-2/own', 'test-user-2', '{}'::jsonb, false, 'webhook'); diff --git a/backend/windmill-api/src/capture.rs b/backend/windmill-api/src/capture.rs index 16363228eb..2e2058ea80 100644 --- a/backend/windmill-api/src/capture.rs +++ b/backend/windmill-api/src/capture.rs @@ -742,17 +742,21 @@ async fn get_capture( async fn delete_capture( authed: ApiAuthed, Extension(user_db): Extension, - Path((_, id)): Path<(String, i64)>, + Path((w_id, id)): Path<(String, i64)>, ) -> Result<()> { let mut tx = user_db.begin(&authed).await?; + // capture RLS only keys on the path segment, so without workspace_id an id from + // another workspace whose path collides with the caller's grants would be deleted. sqlx::query!( r#" - DELETE FROM + DELETE FROM capture - WHERE + WHERE id = $1 + AND workspace_id = $2 "#, - id + id, + &w_id, ) .execute(&mut *tx) .await?;