From f007ce8e8eaae63a9c7a19d59c483484f4f225c6 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 00:16:07 +0200 Subject: [PATCH] fix: a poisoned draft's path keys follow a move, legacy only bypasses routing on a delete, picker loads are generation-guarded Co-Authored-By: Claude Opus 5 (1M context) --- ...fa00ab137c70b73f381452cadfae6e1cda934.json | 17 +++++++ ...450a6444770629bb7bcebe46a33a574bb4931.json | 17 ------- backend/tests/drafts_save_follows_move.rs | 33 ++++++++++++ backend/windmill-api/src/drafts.rs | 4 +- backend/windmill-common/src/user_drafts.rs | 51 ++++++++++--------- frontend/src/lib/components/DiffDrawer.svelte | 17 +++++-- 6 files changed, 94 insertions(+), 45 deletions(-) create mode 100644 backend/.sqlx/query-09e3f8fa8a9e64048971983584bfa00ab137c70b73f381452cadfae6e1cda934.json delete mode 100644 backend/.sqlx/query-5e5547ba06a57bb5c46300bd3af450a6444770629bb7bcebe46a33a574bb4931.json diff --git a/backend/.sqlx/query-09e3f8fa8a9e64048971983584bfa00ab137c70b73f381452cadfae6e1cda934.json b/backend/.sqlx/query-09e3f8fa8a9e64048971983584bfa00ab137c70b73f381452cadfae6e1cda934.json new file mode 100644 index 0000000000..a4d174992e --- /dev/null +++ b/backend/.sqlx/query-09e3f8fa8a9e64048971983584bfa00ab137c70b73f381452cadfae6e1cda934.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft AS d\n SET path = $3::text,\n value = (\n SELECT CASE\n WHEN s.clean -> 'path' = to_jsonb($2::text)\n OR s.clean -> 'draft_path' = to_jsonb($2::text)\n THEN to_json(\n s.clean\n || CASE WHEN s.clean -> 'path' = to_jsonb($2::text)\n THEN jsonb_build_object('path', $3::text)\n ELSE '{}'::jsonb END\n || CASE WHEN s.clean -> 'draft_path' = to_jsonb($2::text)\n THEN jsonb_build_object('draft_path', $3::text)\n ELSE '{}'::jsonb END\n )\n ELSE d.value\n END\n FROM (SELECT replace(replace(replace(d.value::text, chr(92) || chr(92), chr(1)),\n chr(92) || 'u0000', ''), chr(1), chr(92) || chr(92))::jsonb AS clean) s\n )\n WHERE d.workspace_id = $1\n AND d.path = $2::text\n AND d.typ::text = ANY($4::text[])", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text", + "TextArray" + ] + }, + "nullable": [] + }, + "hash": "09e3f8fa8a9e64048971983584bfa00ab137c70b73f381452cadfae6e1cda934" +} diff --git a/backend/.sqlx/query-5e5547ba06a57bb5c46300bd3af450a6444770629bb7bcebe46a33a574bb4931.json b/backend/.sqlx/query-5e5547ba06a57bb5c46300bd3af450a6444770629bb7bcebe46a33a574bb4931.json deleted file mode 100644 index 2d3b82977c..0000000000 --- a/backend/.sqlx/query-5e5547ba06a57bb5c46300bd3af450a6444770629bb7bcebe46a33a574bb4931.json +++ /dev/null @@ -1,17 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE draft\n SET path = $3::text,\n value = CASE\n WHEN position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0\n THEN value\n WHEN to_jsonb(value) -> 'path' = to_jsonb($2::text)\n OR to_jsonb(value) -> 'draft_path' = to_jsonb($2::text)\n THEN to_json(\n to_jsonb(value)\n || CASE WHEN to_jsonb(value) -> 'path' = to_jsonb($2::text)\n THEN jsonb_build_object('path', $3::text)\n ELSE '{}'::jsonb END\n || CASE WHEN to_jsonb(value) -> 'draft_path' = to_jsonb($2::text)\n THEN jsonb_build_object('draft_path', $3::text)\n ELSE '{}'::jsonb END\n )\n ELSE value\n END\n WHERE workspace_id = $1\n AND path = $2::text\n AND typ::text = ANY($4::text[])", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Text", - "Text", - "Text", - "TextArray" - ] - }, - "nullable": [] - }, - "hash": "5e5547ba06a57bb5c46300bd3af450a6444770629bb7bcebe46a33a574bb4931" -} diff --git a/backend/tests/drafts_save_follows_move.rs b/backend/tests/drafts_save_follows_move.rs index d2a4b59d9b..6f3c7692ee 100644 --- a/backend/tests/drafts_save_follows_move.rs +++ b/backend/tests/drafts_save_follows_move.rs @@ -336,3 +336,36 @@ async fn test_redeploy_at_a_routed_path_ends_the_route(db: Pool) -> an ); Ok(()) } + +/// A draft written before the NUL sanitizer still has to follow a move: its path keys are +/// what a deploy of it would land on, so the carry rewrites them, sanitizing the value it +/// could not otherwise parse. +#[sqlx::test(fixtures("base", "drafts_save_follows_move"))] +async fn test_a_poisoned_draft_follows_a_rename(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // The teammate's row, rewritten the way a pre-sanitizer client left one: a real NUL + // escape in the content, both path keys naming the path the item is about to leave. + sqlx::query( + r#"UPDATE draft SET value = '{"path": "u/test-user/follow_a", "draft_path": "u/test-user/follow_a", + "parent_hash": "0000000000001b76", "summary": "A", "content": "a\u0000b"}' + WHERE email = 'test2@windmill.dev'"#, + ) + .execute(&db) + .await?; + + rename(port, HEAD_HASH, "u/test-user/follow_b").await?; + + let row: (String, String, String) = sqlx::query_as( + "SELECT value::jsonb ->> 'path', value::jsonb ->> 'draft_path', value::jsonb ->> 'content' + FROM draft WHERE email = 'test2@windmill.dev'", + ) + .fetch_one(&db) + .await?; + assert_eq!(row.0, "u/test-user/follow_b", "typed path did not follow"); + assert_eq!(row.1, "u/test-user/follow_b", "mirror did not follow"); + assert_eq!(row.2, "ab", "the NUL survived the rewrite"); + Ok(()) +} diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 82be067940..19a3ef16fc 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -404,7 +404,9 @@ async fn update_draft( let is_own_discard = req.value.is_none() && !req.legacy; // The caller's own draft-only move outranks the move of the deployed item. - let moved_to = if req.legacy { + // `legacy` names the workspace-level row, which no move record covers, and it is + // delete-only: an upsert writes the caller's own row and is routed like any other. + let moved_to = if req.legacy && req.value.is_none() { None } else { sqlx::query_scalar!( diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 469c46c51c..563cd83cf5 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -708,31 +708,36 @@ pub async fn move_drafts_for_path( "'{new_path}' already has a draft on it ({names}) — it must be moved or discarded first" ))); } - // `draft.value` is `json`, so `to_jsonb` raises 22P05 on a row still carrying a - // NUL escape from before the write-time sanitizer. Such a row moves on its path - // column alone: one poisoned draft must not abort someone else's rename. + // `draft.value` is `json`, so a NUL escape left by a pre-sanitizer write makes + // `to_jsonb` raise 22P05. `clean` is `strip_json_nul`'s parity rule in SQL (escaped + // backslashes parked on chr(1), which a `json` value's text cannot hold, so nothing + // collides with it; chr(92) spells the backslash so no escape sequence reaches this + // file). A row whose keys need re-pointing is therefore rewritten clean rather than + // left naming the old path, and one that needs nothing keeps its value byte for byte. sqlx::query!( - r#"UPDATE draft + r#"UPDATE draft AS d SET path = $3::text, - value = CASE - WHEN position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) > 0 - THEN value - WHEN to_jsonb(value) -> 'path' = to_jsonb($2::text) - OR to_jsonb(value) -> 'draft_path' = to_jsonb($2::text) - THEN to_json( - to_jsonb(value) - || CASE WHEN to_jsonb(value) -> 'path' = to_jsonb($2::text) - THEN jsonb_build_object('path', $3::text) - ELSE '{}'::jsonb END - || CASE WHEN to_jsonb(value) -> 'draft_path' = to_jsonb($2::text) - THEN jsonb_build_object('draft_path', $3::text) - ELSE '{}'::jsonb END - ) - ELSE value - END - WHERE workspace_id = $1 - AND path = $2::text - AND typ::text = ANY($4::text[])"#, + value = ( + SELECT CASE + WHEN s.clean -> 'path' = to_jsonb($2::text) + OR s.clean -> 'draft_path' = to_jsonb($2::text) + THEN to_json( + s.clean + || CASE WHEN s.clean -> 'path' = to_jsonb($2::text) + THEN jsonb_build_object('path', $3::text) + ELSE '{}'::jsonb END + || CASE WHEN s.clean -> 'draft_path' = to_jsonb($2::text) + THEN jsonb_build_object('draft_path', $3::text) + ELSE '{}'::jsonb END + ) + ELSE d.value + END + FROM (SELECT replace(replace(replace(d.value::text, chr(92) || chr(92), chr(1)), + chr(92) || 'u0000', ''), chr(1), chr(92) || chr(92))::jsonb AS clean) s + ) + WHERE d.workspace_id = $1 + AND d.path = $2::text + AND d.typ::text = ANY($4::text[])"#, w_id, old_path, new_path, diff --git a/frontend/src/lib/components/DiffDrawer.svelte b/frontend/src/lib/components/DiffDrawer.svelte index 123a52e514..43763ba937 100644 --- a/frontend/src/lib/components/DiffDrawer.svelte +++ b/frontend/src/lib/components/DiffDrawer.svelte @@ -94,16 +94,19 @@ let versionLoader: ((id: string) => Promise) | undefined = $state(undefined) let headLabel: string | undefined = $state(undefined) let loadingVersion = $state(false) + /** The version load the spinner belongs to. A response for anything else is stale — + * a slower earlier pick, or one outlived by a drawer reset — and neither replaces + * the diff nor clears the spinner, which the picker's `disabled` rides on. */ + let pendingVersionLoad: string | undefined = undefined async function selectVersion(id: string | undefined) { if (!id || !versionLoader || !data || data.mode !== 'normal') return selectedVersion = id + pendingVersionLoad = id loadingVersion = true try { const value = await versionLoader(id) - // A slower earlier request must not replace what the picker now shows, nor - // clear the spinner the newer one is still running under. - if (selectedVersion !== id) return + if (pendingVersionLoad !== id) return if (!value || !data || data.mode !== 'normal') return const opt = data.versions?.find((v) => v.id === id) data = { @@ -112,7 +115,10 @@ deployedLabel: opt?.isHead ? headLabel : opt?.label } } finally { - if (selectedVersion === id) loadingVersion = false + if (pendingVersionLoad === id) { + pendingVersionLoad = undefined + loadingVersion = false + } } } @@ -163,6 +169,9 @@ } = diff versionLoader = loadVersion headLabel = deployedLabel + // A load still in flight belongs to the diff being replaced. + pendingVersionLoad = undefined + loadingVersion = false selectedVersion = versions?.find((v) => v.isHead)?.id data = { mode: 'normal',