starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 00:02:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: a poisoned draft's path keys follow a move, legacy only bypasses routing on a delete, picker loads are generation-guarded

Browse Source 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-09-15 00:16:07 +02:00
co-authored by Claude Opus 5
parent 0a1dca929b
commit f007ce8e8e
6 changed files with 94 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/.sqlx/query-09e3f8fa8a9e64048971983584bfa00ab137c70b73f381452cadfae6e1cda934.json
Generated
+17
View File
@@ -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"
}
backend/.sqlx/query-5e5547ba06a57bb5c46300bd3af450a6444770629bb7bcebe46a33a574bb4931.json
Generated
-17
View File
@@ -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"
}
backend/tests/drafts_save_follows_move.rs
+33
View File
@@ -336,3 +336,36 @@ async fn test_redeploy_at_a_routed_path_ends_the_route(db: Pool<Postgres>) -> 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<Postgres>) -> 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(())
}
backend/windmill-api/src/drafts.rs
+3 -1
View File
@@ -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!(
backend/windmill-common/src/user_drafts.rs
+28 -23
View File
@@ -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,
frontend/src/lib/components/DiffDrawer.svelte
+13 -4
View File
@@ -94,16 +94,19 @@
let versionLoader: ((id: string) => Promise<Value | undefined>) | 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',