From c04942e341e85b70489b0971523ae97d95a7c827 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Tue, 8 Sep 2026 12:56:58 +0200 Subject: [PATCH] fix: report a NUL-poisoned draft on move instead of 500ing Co-Authored-By: Claude Opus 5 (1M context) --- ...b9d0c37c27da9f9768f6520ac3c96b5765840.json | 66 +++++++++++++++++++ ...d84637926113f1f6aef6b58e24c88930f1432.json | 62 +++++++++++++++++ backend/windmill-api-flows/src/flows.rs | 3 +- backend/windmill-api-scripts/src/scripts.rs | 3 +- backend/windmill-api/src/apps.rs | 5 +- backend/windmill-api/src/drafts.rs | 30 +++++++-- 6 files changed, 158 insertions(+), 11 deletions(-) create mode 100644 backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json create mode 100644 backend/.sqlx/query-c902cf38cea14284032312f833ed84637926113f1f6aef6b58e24c88930f1432.json diff --git a/backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json b/backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json new file mode 100644 index 0000000000..f7f0b76a4f --- /dev/null +++ b/backend/.sqlx/query-25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840.json @@ -0,0 +1,66 @@ +{ + "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!\" ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "at_target!", + "type_info": "Bool" + }, + { + "ordinal": 1, + "name": "poisoned!", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + { + "Custom": { + "name": "draft_kind", + "kind": { + "Enum": [ + "script", + "flow", + "app", + "raw_app", + "resource", + "variable", + "trigger_schedule", + "trigger_webhook", + "trigger_default_email", + "trigger_email", + "trigger_http", + "trigger_websocket", + "trigger_postgres", + "trigger_kafka", + "trigger_nats", + "trigger_mqtt", + "trigger_sqs", + "trigger_gcp", + "trigger_azure", + "trigger_poll", + "trigger_cli", + "trigger_nextcloud", + "trigger_google", + "trigger_github", + "data_pipeline", + "trigger_amqp" + ] + } + } + }, + "Text", + "Text", + "Text" + ] + }, + "nullable": [ + null, + null + ] + }, + "hash": "25ce9aff8844fbfc1a9e16ac7c4b9d0c37c27da9f9768f6520ac3c96b5765840" +} diff --git a/backend/.sqlx/query-c902cf38cea14284032312f833ed84637926113f1f6aef6b58e24c88930f1432.json b/backend/.sqlx/query-c902cf38cea14284032312f833ed84637926113f1f6aef6b58e24c88930f1432.json new file mode 100644 index 0000000000..74fec3d7d8 --- /dev/null +++ b/backend/.sqlx/query-c902cf38cea14284032312f833ed84637926113f1f6aef6b58e24c88930f1432.json @@ -0,0 +1,62 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE draft\n SET path = $3,\n value = to_json(\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 )\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", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [ + "Text", + "Text", + "Text", + { + "Custom": { + "name": "draft_kind", + "kind": { + "Enum": [ + "script", + "flow", + "app", + "raw_app", + "resource", + "variable", + "trigger_schedule", + "trigger_webhook", + "trigger_default_email", + "trigger_email", + "trigger_http", + "trigger_websocket", + "trigger_postgres", + "trigger_kafka", + "trigger_nats", + "trigger_mqtt", + "trigger_sqs", + "trigger_gcp", + "trigger_azure", + "trigger_poll", + "trigger_cli", + "trigger_nextcloud", + "trigger_google", + "trigger_github", + "data_pipeline", + "trigger_amqp" + ] + } + } + }, + "Text", + "Text", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "c902cf38cea14284032312f833ed84637926113f1f6aef6b58e24c88930f1432" +} diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index ba3c0c7349..1f14762871 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -1404,8 +1404,9 @@ async fn update_flow( .await?; if outcome.left_behind > 0 { tracing::warn!( - "{} flow draft(s) stranded at {flow_path}: their owner already has a draft at {}", + "{} of {} flow draft(s) stranded at {flow_path}: their owner already has a draft at {}", outcome.left_behind, + outcome.moved + outcome.left_behind, &nf.path ); } diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index b84bff672c..27f20f2916 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -2157,8 +2157,9 @@ async fn create_script_internal<'c>( .await?; if outcome.left_behind > 0 { tracing::warn!( - "{} script draft(s) stranded at {p_path}: their owner already has a draft at {}", + "{} of {} script draft(s) stranded at {p_path}: their owner already has a draft at {}", outcome.left_behind, + outcome.moved + outcome.left_behind, &ns.path ); } diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index e890ee1c2d..1922f06e97 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -3469,8 +3469,9 @@ async fn update_app_internal<'a>( .await?; if outcome.left_behind > 0 { tracing::warn!( - "{} app draft(s) stranded at {path}: their owner already has a draft at {npath}", - outcome.left_behind + "{} of {} app draft(s) stranded at {path}: their owner already has a draft at {npath}", + outcome.left_behind, + outcome.moved + outcome.left_behind ); } } diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index acec427713..177fafa2b3 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -842,6 +842,11 @@ async fn move_draft( AND path = $2 AND typ = $4 AND email = $6 + -- A pre-sanitizer NUL escape makes `to_jsonb` raise 22P05. Excluded + -- here so the statement can't 500; reported below instead. Unlike the + -- passive carry, rewriting the value IS this operation, so skipping it + -- silently would move the row and leave its typed path stale. + AND position(chr(92) || 'u0000' in replace(value::text, chr(92) || chr(92), '')) = 0 -- Skipped on a summary-only edit, where the "target" row is this -- row and the guard would refuse the update against itself. AND ($2 = $3 OR NOT EXISTS ( @@ -861,17 +866,28 @@ async fn move_draft( .await?; if moved.is_none() { - let exists_at_target = sqlx::query_scalar!( - "SELECT 1 FROM draft WHERE workspace_id = $1 AND path = $2 AND typ = $3 AND email = $4", + 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!", + 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 + ) as "poisoned!" "#, &w_id, - new_path, kind as UserDraftItemKind, + new_path, &authed.email, + path, ) - .fetch_optional(&db) - .await? - .is_some(); - return Err(Error::BadRequest(if exists_at_target && new_path != path { + .fetch_one(&db) + .await?; + return Err(Error::BadRequest(if row.poisoned { + format!( + "'{path}' contains a NUL character and predates the sanitizer, so it cannot be \ + moved. Reopen it, re-save to rewrite it cleanly, then move it." + ) + } else if row.at_target && new_path != path { format!("You already have a draft at '{new_path}'") } else { format!("You have no draft at '{path}'")