From 68cd7a44203739f2546bccb90a5d33356c152d4d Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 14 Sep 2026 20:51:14 +0200 Subject: [PATCH] fix: a draft-only app move refuses the other app kind; a session keeps an unknown base unknown Co-Authored-By: Claude Opus 5 (1M context) --- ...a8df58840c463d8aa412871bd9774d978bae.json} | 7 ++-- ...9bbe31e4e938d9f53eeb659cdc49f9be8f2c.json} | 7 ++-- backend/tests/drafts_move_taken.rs | 42 +++++++++++++++++++ backend/tests/fixtures/drafts_move_taken.sql | 8 ++++ backend/windmill-api/src/drafts.rs | 18 +++++++- .../sessions/sessionRuntime.svelte.ts | 12 +++--- 6 files changed, 81 insertions(+), 13 deletions(-) rename backend/.sqlx/{query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json => query-9b574bf93759d822066ac1629744a8df58840c463d8aa412871bd9774d978bae.json} (76%) rename backend/.sqlx/{query-0b247ac75331db89c41629d11aee5f13f78e2ac82265f84ea3f67925eb542140.json => query-e27e447e4d9607fe280807e396049bbe31e4e938d9f53eeb659cdc49f9be8f2c.json} (92%) diff --git a/backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json b/backend/.sqlx/query-9b574bf93759d822066ac1629744a8df58840c463d8aa412871bd9774d978bae.json similarity index 76% rename from backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json rename to backend/.sqlx/query-9b574bf93759d822066ac1629744a8df58840c463d8aa412871bd9774d978bae.json index f7f0b76a4f..52f1194dff 100644 --- a/backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json +++ b/backend/.sqlx/query-9b574bf93759d822066ac1629744a8df58840c463d8aa412871bd9774d978bae.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ = $2 AND email = $4) as \"at_target!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\" ", + "query": "SELECT\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3\n AND typ::text = ANY($6::text[]) AND email = $4) as \"at_target!\",\n EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5\n AND typ = $2 AND email = $4\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n ) as \"poisoned!\" ", "describe": { "columns": [ { @@ -54,7 +54,8 @@ }, "Text", "Text", - "Text" + "Text", + "TextArray" ] }, "nullable": [ @@ -62,5 +63,5 @@ null ] }, - "hash": "25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840" + "hash": "9b574bf93759d822066ac1629744a8df58840c463d8aa412871bd9774d978bae" } diff --git a/backend/.sqlx/query-0b247ac75331db89c41629d11aee5f13f78e2ac82265f84ea3f67925eb542140.json b/backend/.sqlx/query-e27e447e4d9607fe280807e396049bbe31e4e938d9f53eeb659cdc49f9be8f2c.json similarity index 92% rename from backend/.sqlx/query-0b247ac75331db89c41629d11aee5f13f78e2ac82265f84ea3f67925eb542140.json rename to backend/.sqlx/query-e27e447e4d9607fe280807e396049bbe31e4e938d9f53eeb659cdc49f9be8f2c.json index 3e3a9c98ef..376f7ec61c 100644 --- a/backend/.sqlx/query-0b247ac75331db89c41629d11aee5f13f78e2ac82265f84ea3f67925eb542140.json +++ b/backend/.sqlx/query-e27e447e4d9607fe280807e396049bbe31e4e938d9f53eeb659cdc49f9be8f2c.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "UPDATE draft\n SET path = $3,\n -- Both path keys, not just the typed one: the editors mirror the\n -- typed path into the other while it differs from the row's path,\n -- and the loaders prefer the mirror — left naming the old location\n -- it un-does this move on the next save. `create_missing = false`\n -- on both, so a draft carrying only one keeps only one.\n value = to_json(\n jsonb_set(\n jsonb_set(\n CASE WHEN $7::text IS NULL THEN to_jsonb(value)\n ELSE jsonb_set(to_jsonb(value), ARRAY['summary'], to_jsonb($7::text))\n END,\n ARRAY[$5::text], to_jsonb($3::text), false\n ),\n ARRAY[$8::text], to_jsonb($3::text), false\n )\n )\n WHERE workspace_id = $1\n AND path = $2\n AND typ = $4\n AND email = $6\n -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded\n -- here so the statement can't 500; reported below instead. Unlike the\n -- passive carry, rewriting the value IS this operation, so skipping it\n -- silently would move the row and leave its typed path stale.\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0\n -- Skipped on a summary-only edit, where the \"target\" row is this\n -- row and the guard would refuse the update against itself.\n AND ($2 = $3 OR NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ = $4 AND o.email = $6\n ))\n RETURNING id", + "query": "UPDATE draft\n SET path = $3,\n -- Both path keys, not just the typed one: the editors mirror the\n -- typed path into the other while it differs from the row's path,\n -- and the loaders prefer the mirror — left naming the old location\n -- it un-does this move on the next save. `create_missing = false`\n -- on both, so a draft carrying only one keeps only one.\n value = to_json(\n jsonb_set(\n jsonb_set(\n CASE WHEN $7::text IS NULL THEN to_jsonb(value)\n ELSE jsonb_set(to_jsonb(value), ARRAY['summary'], to_jsonb($7::text))\n END,\n ARRAY[$5::text], to_jsonb($3::text), false\n ),\n ARRAY[$8::text], to_jsonb($3::text), false\n )\n )\n WHERE workspace_id = $1\n AND path = $2\n AND typ = $4\n AND email = $6\n -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded\n -- here so the statement can't 500; reported below instead. Unlike the\n -- passive carry, rewriting the value IS this operation, so skipping it\n -- silently would move the row and leave its typed path stale.\n AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0\n -- Skipped on a summary-only edit, where the \"target\" row is this\n -- row and the guard would refuse the update against itself.\n AND ($2 = $3 OR NOT EXISTS (\n SELECT 1 FROM draft o\n WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[])\n AND o.email = $6\n ))\n RETURNING id", "describe": { "columns": [ { @@ -52,12 +52,13 @@ "Text", "Text", "Text", - "Text" + "Text", + "TextArray" ] }, "nullable": [ false ] }, - "hash": "0b247ac75331db89c41629d11aee5f13f78e2ac82265f84ea3f67925eb542140" + "hash": "e27e447e4d9607fe280807e396049bbe31e4e938d9f53eeb659cdc49f9be8f2c" } diff --git a/backend/tests/drafts_move_taken.rs b/backend/tests/drafts_move_taken.rs index df29d9f9e7..68a219d4d7 100644 --- a/backend/tests/drafts_move_taken.rs +++ b/backend/tests/drafts_move_taken.rs @@ -73,3 +73,45 @@ async fn test_rename_onto_a_draft_is_refused(db: Pool) -> anyhow::Resu Ok(()) } + +/// A classic app and a raw app deploy into the same table, so a draft-only move onto +/// the other kind's draft must be refused: deploying either path afterwards deletes +/// the caller's drafts of both kinds, taking the loser's item with it. +#[sqlx::test(fixtures("base", "drafts_move_taken"))] +async fn test_draft_move_refuses_the_other_app_kind(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/move/raw_app/u/test-user/mvtaken_raw" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "new_path": "u/test-user/mvtaken_app" })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 400, "move onto a classic app draft was allowed: {body}"); + assert!(body.contains("already have a draft"), "unexpected refusal: {body}"); + + // Both drafts are untouched. + let list: Vec = reqwest::Client::new() + .get(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/list" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .send() + .await? + .json() + .await?; + let mut kinds = list + .iter() + .filter(|d| matches!(d["kind"].as_str(), Some("app") | Some("raw_app"))) + .filter_map(|d| d["kind"].as_str()) + .collect::>(); + kinds.sort(); + assert_eq!(kinds, vec!["app", "raw_app"], "{list:?}"); + Ok(()) +} diff --git a/backend/tests/fixtures/drafts_move_taken.sql b/backend/tests/fixtures/drafts_move_taken.sql index f278036e56..debb27c734 100644 --- a/backend/tests/fixtures/drafts_move_taken.sql +++ b/backend/tests/fixtures/drafts_move_taken.sql @@ -15,3 +15,11 @@ INSERT INTO draft (workspace_id, path, typ, value, email) VALUES ('test-workspace', 'u/test-user/mvtaken_b', 'script', '{"path": "u/test-user/mvtaken_b", "summary": "B", "content": ""}', 'test@windmill.dev'); + +-- A draft-only classic app and a draft-only raw app of the same owner. They share +-- the `app` table, so one occupies the other's path. +INSERT INTO draft (workspace_id, path, typ, value, email) VALUES + ('test-workspace', 'u/test-user/mvtaken_app', 'app', + '{"summary": "classic", "value": {}}', 'test@windmill.dev'), + ('test-workspace', 'u/test-user/mvtaken_raw', 'raw_app', + '{"summary": "raw", "files": {}}', 'test@windmill.dev'); diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 87f3290f42..d99e8bee22 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -679,6 +679,17 @@ async fn move_draft( } } + // A classic app and a raw app share the `app` table, so a draft of either kind + // occupies the destination for both: deploying there deletes the caller's drafts + // of both kinds, taking the item that lost the collision with it. + let collision_typs: Vec<&str> = match kind { + UserDraftItemKind::App | UserDraftItemKind::RawApp => vec![ + UserDraftItemKind::App.as_str(), + UserDraftItemKind::RawApp.as_str(), + ], + _ => vec![kind.as_str()], + }; + // One transaction with the move record, so a save addressed to the old path // never sees the row gone without knowing where it went. let mut tx = db.begin().await?; @@ -714,7 +725,8 @@ async fn move_draft( -- row and the guard would refuse the update against itself. AND ($2 = $3 OR NOT EXISTS ( SELECT 1 FROM draft o - WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ = $4 AND o.email = $6 + WHERE o.workspace_id = $1 AND o.path = $3 AND o.typ::text = ANY($9::text[]) + AND o.email = $6 )) RETURNING id"#, &w_id, @@ -725,6 +737,7 @@ async fn move_draft( &authed.email, req.summary, mirror_field, + &collision_typs as &[&str], ) .fetch_optional(&mut *tx) .await?; @@ -745,7 +758,7 @@ async fn move_draft( let row = sqlx::query!( r#"SELECT EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $3 - AND typ = $2 AND email = $4) as "at_target!", + AND typ::text = ANY($6::text[]) AND email = $4) as "at_target!", EXISTS(SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $5 AND typ = $2 AND email = $4 AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0 @@ -755,6 +768,7 @@ async fn move_draft( new_path, &authed.email, path, + &collision_typs as &[&str], ) .fetch_one(&db) .await?; diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index eb2ecff6dd..4aab4cff08 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -849,11 +849,13 @@ function createRuntime(session: Session): SessionRuntime { path: result.path, custom_path: draftValue?.custom_path ?? result.custom_path, draft_path: draftValue?.draft_path, - // The draft's own base, else the head this checkout forks from (the - // standalone editor's loader does the same). - parent_version: - draftValue?.parent_version ?? - (Array.isArray(result.versions) ? result.versions[result.versions.length - 1] : undefined) + // Only a fresh checkout forks from the head; a draft keeps its own base, + // unknown included, or it would read as up to date. See loadScript. + parent_version: draftValue + ? draftValue.parent_version + : Array.isArray(result.versions) + ? result.versions[result.versions.length - 1] + : undefined } // Seed the per-tab last_sync from the server draft's timestamp so // later saves attach a matching last_sync and the server can reject