From dfeed9c5c2e39bf3e10eea4f69ea140ee9e7832f Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 16 May 2026 08:29:53 +0000 Subject: [PATCH] fix: actionable error when a custom_path is taken by an app in another workspace (#9190) Co-authored-by: Claude Opus 4.7 (1M context) --- ...757b244b280e1262dcbccd2fc5189c7b3d25b.json | 31 ++++ ...d9ae0ab27c8a76302a2570473046e28c91fdd.json | 29 ++++ ...a1d606d9725e20e9c2d76a0887fadfd87f8df.json | 25 --- .../tests/app_custom_path_cross_workspace.rs | 120 ++++++++++++++ .../app_custom_path_cross_workspace.sql | 153 ++++++++++++++++++ backend/windmill-api/src/apps.rs | 82 +++++++--- 6 files changed, 392 insertions(+), 48 deletions(-) create mode 100644 backend/.sqlx/query-5347ec9ab6de69a99e8823d2199757b244b280e1262dcbccd2fc5189c7b3d25b.json create mode 100644 backend/.sqlx/query-debd7470025cf64cd1191e604fed9ae0ab27c8a76302a2570473046e28c91fdd.json delete mode 100644 backend/.sqlx/query-fb1399c1dc171ec6bb24fee3477a1d606d9725e20e9c2d76a0887fadfd87f8df.json create mode 100644 backend/tests/app_custom_path_cross_workspace.rs create mode 100644 backend/tests/fixtures/app_custom_path_cross_workspace.sql diff --git a/backend/.sqlx/query-5347ec9ab6de69a99e8823d2199757b244b280e1262dcbccd2fc5189c7b3d25b.json b/backend/.sqlx/query-5347ec9ab6de69a99e8823d2199757b244b280e1262dcbccd2fc5189c7b3d25b.json new file mode 100644 index 0000000000..871cbd144e --- /dev/null +++ b/backend/.sqlx/query-5347ec9ab6de69a99e8823d2199757b244b280e1262dcbccd2fc5189c7b3d25b.json @@ -0,0 +1,31 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT workspace_id, path FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2) AND NOT (path = $3 AND workspace_id = $4) LIMIT 1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "Text", + "Text", + "Text" + ] + }, + "nullable": [ + false, + false + ] + }, + "hash": "5347ec9ab6de69a99e8823d2199757b244b280e1262dcbccd2fc5189c7b3d25b" +} diff --git a/backend/.sqlx/query-debd7470025cf64cd1191e604fed9ae0ab27c8a76302a2570473046e28c91fdd.json b/backend/.sqlx/query-debd7470025cf64cd1191e604fed9ae0ab27c8a76302a2570473046e28c91fdd.json new file mode 100644 index 0000000000..f907d4e959 --- /dev/null +++ b/backend/.sqlx/query-debd7470025cf64cd1191e604fed9ae0ab27c8a76302a2570473046e28c91fdd.json @@ -0,0 +1,29 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT workspace_id, path FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2) LIMIT 1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + false + ] + }, + "hash": "debd7470025cf64cd1191e604fed9ae0ab27c8a76302a2570473046e28c91fdd" +} diff --git a/backend/.sqlx/query-fb1399c1dc171ec6bb24fee3477a1d606d9725e20e9c2d76a0887fadfd87f8df.json b/backend/.sqlx/query-fb1399c1dc171ec6bb24fee3477a1d606d9725e20e9c2d76a0887fadfd87f8df.json deleted file mode 100644 index a2362be620..0000000000 --- a/backend/.sqlx/query-fb1399c1dc171ec6bb24fee3477a1d606d9725e20e9c2d76a0887fadfd87f8df.json +++ /dev/null @@ -1,25 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT EXISTS(SELECT 1 FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2) AND NOT (path = $3 AND workspace_id = $4))", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "exists", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Text", - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "fb1399c1dc171ec6bb24fee3477a1d606d9725e20e9c2d76a0887fadfd87f8df" -} diff --git a/backend/tests/app_custom_path_cross_workspace.rs b/backend/tests/app_custom_path_cross_workspace.rs new file mode 100644 index 0000000000..5b0002bb05 --- /dev/null +++ b/backend/tests/app_custom_path_cross_workspace.rs @@ -0,0 +1,120 @@ +//! Regression test for the cross-workspace custom_path conflict. +//! +//! When custom paths are instance-global (CLOUD_HOSTED unset and +//! `app_workspaced_route` off — the default for dedicated instances), a +//! custom_path is a single global route slot. The uniqueness check correctly +//! blocks two apps from claiming it, including the same logical app deployed +//! to two workspaces (staging/prod, git-sync). The bug was that the error +//! ("App with custom path already exists") gave the operator no idea +//! where the conflicting copy lived. This test pins down: +//! - a single-workspace edit keeping its own custom_path still succeeds +//! (the app's own row is excluded), +//! - a real conflict is still rejected, and +//! - the error now names the conflicting app's path and workspace. + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + builder.header("Authorization", format!("Bearer {}", token)) +} + +fn new_app(path: &str, custom_path: &str) -> serde_json::Value { + json!({ + "path": path, + "summary": "Test app", + "value": { "type": "rawapp", "inline_script": null }, + "policy": { "execution_mode": "anonymous", "triggerables": {} }, + "custom_path": custom_path + }) +} + +#[sqlx::test(fixtures("app_custom_path_cross_workspace"))] +async fn test_custom_path_cross_workspace_deploy(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws_a = format!("http://localhost:{port}/api/w/test-workspace"); + let ws_b = format!("http://localhost:{port}/api/w/test-workspace-2"); + + let app_path = "f/Newsletter/newsletter_composer"; + let custom_path = "newsletter"; + + // 1. Create the app with a custom path in workspace A. + let resp = authed(client().post(format!("{ws_a}/apps/create")), "SECRET_TOKEN") + .json(&new_app(app_path, custom_path)) + .send() + .await?; + assert_eq!( + resp.status(), + 201, + "create app in ws A should succeed: {}", + resp.text().await? + ); + + // 2. Editing the app in its own workspace, keeping the same custom path, + // must still succeed — the app's own row is excluded from the check. + // (This is the common single-workspace deploy; it must not regress.) + let resp = authed( + client().post(format!("{ws_a}/apps/update/{app_path}")), + "SECRET_TOKEN", + ) + .json(&json!({ + "summary": "Test app (edited)", + "value": { "type": "rawapp", "inline_script": null }, + "policy": { "execution_mode": "anonymous", "triggerables": {} }, + "custom_path": custom_path + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "editing an app in its own workspace keeping its custom path must succeed: {}", + resp.text().await? + ); + + // 3. Deploying the same app (same path) to a second workspace is a real + // conflict in global mode (one global route slot). It must be rejected, + // and the error must name the conflicting workspace + app so the + // operator knows what to resolve. + let resp = authed(client().post(format!("{ws_b}/apps/create")), "SECRET_TOKEN") + .json(&new_app(app_path, custom_path)) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "same custom path in another workspace is a global conflict: {body}" + ); + assert!( + body.contains("test-workspace") && body.contains(app_path), + "error must name the conflicting workspace and app, got: {body}" + ); + + // 4. A genuinely different app claiming the in-use custom path is still + // rejected, with the same actionable message. + let resp = authed(client().post(format!("{ws_a}/apps/create")), "SECRET_TOKEN") + .json(&new_app("f/Other/other_app", custom_path)) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 400, + "a different app must not steal an in-use custom path: {body}" + ); + assert!( + body.contains(app_path), + "error must name the app already using the custom path, got: {body}" + ); + + Ok(()) +} diff --git a/backend/tests/fixtures/app_custom_path_cross_workspace.sql b/backend/tests/fixtures/app_custom_path_cross_workspace.sql new file mode 100644 index 0000000000..9cb68d54d9 --- /dev/null +++ b/backend/tests/fixtures/app_custom_path_cross_workspace.sql @@ -0,0 +1,153 @@ +-- Fixture for app_custom_path_cross_workspace regression test. +-- Two workspaces sharing the same admin user, so the same logical app +-- (same `path`) can be deployed to both — exercising the instance-global +-- custom_path uniqueness behavior (CLOUD_HOSTED unset and +-- app_workspaced_route off, the default for dedicated instances). + +INSERT INTO workspace + (id, name, owner) + VALUES ('test-workspace', 'test-workspace', 'test-user'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('test-workspace', 'test@windmill.dev', 'test-user', true, 'Admin'); + +INSERT INTO workspace_key(workspace_id, kind, key) VALUES + ('test-workspace', 'cloud', 'test-key'); + +INSERT INTO workspace_settings (workspace_id) VALUES + ('test-workspace'); + +INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES + ('test-workspace', 'all', 'All users', '{}'); + +INSERT INTO password(email, password_hash, login_type, super_admin, verified, name, username) + VALUES ('test@windmill.dev', 'not-a-real-hash', 'password', true, true, 'Test User', 'test-user'); + +-- Second workspace, same admin user. Lets us deploy the same app path to +-- two workspaces, which is what triggered the custom_path conflict. +INSERT INTO workspace (id, name, owner) VALUES + ('test-workspace-2', 'test-workspace-2', 'test-user'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('test-workspace-2', 'test@windmill.dev', 'test-user', true, 'Admin'); + +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', '{}'); + +-- super_admin token so custom_path edits pass require_admin in both workspaces. +INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin) +VALUES (encode(sha256('SECRET_TOKEN'::bytea), 'hex'), 'SECRET_TOK', 'SECRET_TOKEN', 'test@windmill.dev', 'test token', true); + +GRANT ALL PRIVILEGES ON TABLE workspace_key TO windmill_admin; +GRANT ALL PRIVILEGES ON TABLE workspace_key TO windmill_user; + +CREATE FUNCTION "notify_insert_on_completed_job" () +RETURNS TRIGGER AS $$ +BEGIN + PERFORM pg_notify('completed', NEW.id::text); + RETURN NEW; +END; +$$ LANGUAGE PLPGSQL; + + CREATE TRIGGER "notify_insert_on_completed_job" + AFTER INSERT ON "v2_job_completed" + FOR EACH ROW +EXECUTE FUNCTION "notify_insert_on_completed_job" (); + + +CREATE FUNCTION "notify_queue" () +RETURNS TRIGGER AS $$ +BEGIN + PERFORM pg_notify('queued', NEW.id::text); + RETURN NEW; +END; +$$ LANGUAGE PLPGSQL; + + CREATE TRIGGER "notify_queue_after_insert" + AFTER INSERT ON "v2_job_queue" + FOR EACH ROW +EXECUTE FUNCTION "notify_queue" (); + + CREATE TRIGGER "notify_queue_after_flow_status_update" + AFTER UPDATE ON "v2_job_status" + FOR EACH ROW + WHEN (NEW.flow_status IS DISTINCT FROM OLD.flow_status) +EXECUTE FUNCTION "notify_queue" (); + +-- Apply phase 4: +DROP FUNCTION IF EXISTS v2_job_after_update CASCADE; +DROP FUNCTION IF EXISTS v2_job_completed_before_insert CASCADE; +DROP FUNCTION IF EXISTS v2_job_completed_before_update CASCADE; +DROP FUNCTION IF EXISTS v2_job_queue_after_insert CASCADE; +DROP FUNCTION IF EXISTS v2_job_queue_before_insert CASCADE; +DROP FUNCTION IF EXISTS v2_job_queue_before_update CASCADE; +DROP FUNCTION IF EXISTS v2_job_runtime_before_insert CASCADE; +DROP FUNCTION IF EXISTS v2_job_runtime_before_update CASCADE; +DROP FUNCTION IF EXISTS v2_job_status_before_insert CASCADE; +DROP FUNCTION IF EXISTS v2_job_status_before_update CASCADE; + +DROP VIEW IF EXISTS completed_job, completed_job_view, job, queue, queue_view CASCADE; + +ALTER TABLE v2_job_queue + DROP COLUMN IF EXISTS __parent_job CASCADE, + DROP COLUMN IF EXISTS __created_by CASCADE, + DROP COLUMN IF EXISTS __script_hash CASCADE, + DROP COLUMN IF EXISTS __script_path CASCADE, + DROP COLUMN IF EXISTS __args CASCADE, + DROP COLUMN IF EXISTS __logs CASCADE, + DROP COLUMN IF EXISTS __raw_code CASCADE, + DROP COLUMN IF EXISTS __canceled CASCADE, + DROP COLUMN IF EXISTS __last_ping CASCADE, + DROP COLUMN IF EXISTS __job_kind CASCADE, + DROP COLUMN IF EXISTS __env_id CASCADE, + DROP COLUMN IF EXISTS __schedule_path CASCADE, + DROP COLUMN IF EXISTS __permissioned_as CASCADE, + DROP COLUMN IF EXISTS __flow_status CASCADE, + DROP COLUMN IF EXISTS __raw_flow CASCADE, + DROP COLUMN IF EXISTS __is_flow_step CASCADE, + DROP COLUMN IF EXISTS __language CASCADE, + DROP COLUMN IF EXISTS __same_worker CASCADE, + DROP COLUMN IF EXISTS __raw_lock CASCADE, + DROP COLUMN IF EXISTS __pre_run_error CASCADE, + DROP COLUMN IF EXISTS __email CASCADE, + DROP COLUMN IF EXISTS __visible_to_owner CASCADE, + DROP COLUMN IF EXISTS __mem_peak CASCADE, + DROP COLUMN IF EXISTS __root_job CASCADE, + DROP COLUMN IF EXISTS __leaf_jobs CASCADE, + DROP COLUMN IF EXISTS __concurrent_limit CASCADE, + DROP COLUMN IF EXISTS __concurrency_time_window_s CASCADE, + DROP COLUMN IF EXISTS __timeout CASCADE, + DROP COLUMN IF EXISTS __flow_step_id CASCADE, + DROP COLUMN IF EXISTS __cache_ttl CASCADE; + +LOCK TABLE v2_job_queue IN ACCESS EXCLUSIVE MODE; +ALTER TABLE v2_job_completed + DROP COLUMN IF EXISTS __parent_job CASCADE, + DROP COLUMN IF EXISTS __created_by CASCADE, + DROP COLUMN IF EXISTS __created_at CASCADE, + DROP COLUMN IF EXISTS __success CASCADE, + DROP COLUMN IF EXISTS __script_hash CASCADE, + DROP COLUMN IF EXISTS __script_path CASCADE, + DROP COLUMN IF EXISTS __args CASCADE, + DROP COLUMN IF EXISTS __logs CASCADE, + DROP COLUMN IF EXISTS __raw_code CASCADE, + DROP COLUMN IF EXISTS __canceled CASCADE, + DROP COLUMN IF EXISTS __job_kind CASCADE, + DROP COLUMN IF EXISTS __env_id CASCADE, + DROP COLUMN IF EXISTS __schedule_path CASCADE, + DROP COLUMN IF EXISTS __permissioned_as CASCADE, + DROP COLUMN IF EXISTS __raw_flow CASCADE, + DROP COLUMN IF EXISTS __is_flow_step CASCADE, + DROP COLUMN IF EXISTS __language CASCADE, + DROP COLUMN IF EXISTS __is_skipped CASCADE, + DROP COLUMN IF EXISTS __raw_lock CASCADE, + DROP COLUMN IF EXISTS __email CASCADE, + DROP COLUMN IF EXISTS __visible_to_owner CASCADE, + DROP COLUMN IF EXISTS __tag CASCADE, + DROP COLUMN IF EXISTS __priority CASCADE; diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index a5e9146ddb..ad455ac46a 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1213,6 +1213,32 @@ async fn create_app( Ok((StatusCode::CREATED, path)) } +/// Actionable error when a custom path is already taken. In global mode (not +/// CLOUD_HOSTED and `app_workspaced_route` off) custom paths are unique across +/// the whole instance, so the conflicting copy may live in another workspace +/// (e.g. the same app deployed/git-synced to staging and prod) — name it so +/// the operator knows exactly what to remove. +fn custom_path_conflict_error( + custom_path: &str, + conflict_path: &str, + conflict_workspace: &str, + scoped: bool, +) -> Error { + if scoped { + Error::BadRequest(format!( + "Custom path '{}' is already used by app '{}' in this workspace", + custom_path, conflict_path + )) + } else { + Error::BadRequest(format!( + "Custom path '{}' is already used by app '{}' in workspace '{}'. \ + Custom paths must be unique across the whole instance unless the \ + 'app_workspaced_route' instance setting is enabled.", + custom_path, conflict_path, conflict_workspace + )) + } +} + async fn create_app_internal<'a>( authed: ApiAuthed, db: sqlx::Pool, @@ -1284,21 +1310,24 @@ async fn create_app_internal<'a>( } if let Some(custom_path) = &app.custom_path { require_admin(authed.is_admin, &authed.username)?; - let as_workspaced_route = APP_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed); + let scoped = + *CLOUD_HOSTED || APP_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed); - let exists = sqlx::query_scalar!( - "SELECT EXISTS(SELECT 1 FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2))", + let conflict = sqlx::query!( + "SELECT workspace_id, path FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2) LIMIT 1", custom_path, - if *CLOUD_HOSTED || as_workspaced_route { Some(w_id) } else { None } + if scoped { Some(w_id) } else { None } ) - .fetch_one(&mut *tx) - .await?.unwrap_or(false); + .fetch_optional(&mut *tx) + .await?; - if exists { - return Err(Error::BadRequest(format!( - "App with custom path {} already exists", - custom_path - ))); + if let Some(conflict) = conflict { + return Err(custom_path_conflict_error( + custom_path, + &conflict.path, + &conflict.workspace_id, + scoped, + )); } } sqlx::query!( @@ -1781,27 +1810,34 @@ async fn update_app_internal<'a>( if let Some(ncustom_path) = &ns.custom_path { require_admin(authed.is_admin, &authed.username)?; - let as_workspaced_route = - APP_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed); + let scoped = + *CLOUD_HOSTED || APP_WORKSPACED_ROUTE.load(std::sync::atomic::Ordering::Relaxed); if ncustom_path.is_empty() { sqlb.set("custom_path", "NULL"); } else { - let exists = sqlx::query_scalar!( - "SELECT EXISTS(SELECT 1 FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2) AND NOT (path = $3 AND workspace_id = $4))", + // Same predicate as before (the check is correct): the app's + // own row in this workspace is excluded, so a single-workspace + // edit still works. In global mode a copy of this app in + // another workspace is a genuine conflict (one global route) — + // surface which workspace so it can be resolved. + let conflict = sqlx::query!( + "SELECT workspace_id, path FROM app WHERE custom_path = $1 AND ($2::TEXT IS NULL OR workspace_id = $2) AND NOT (path = $3 AND workspace_id = $4) LIMIT 1", ncustom_path, - if *CLOUD_HOSTED || as_workspaced_route { Some(w_id) } else { None }, + if scoped { Some(w_id) } else { None }, path, w_id ) - .fetch_one(&mut *tx) - .await?.unwrap_or(false); + .fetch_optional(&mut *tx) + .await?; - if exists { - return Err(Error::BadRequest(format!( - "App with custom path {} already exists", - ncustom_path - ))); + if let Some(conflict) = conflict { + return Err(custom_path_conflict_error( + ncustom_path, + &conflict.path, + &conflict.workspace_id, + scoped, + )); } sqlb.set_str("custom_path", ncustom_path); }