From 924f9c7e8d8863d9af40aee246a519b4be0e1ea2 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 19 Jun 2026 15:33:36 +0200 Subject: [PATCH] fix(backend): strip NUL bytes from draft values on write (#9673) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit draft.value is a json column (not jsonb), so a client could store a U+0000 escape in it. Any later text extraction (`->>` / `to_jsonb`) on such a value raises 22P05 "unsupported Unicode escape sequence" — one poisoned draft 500'd the whole GET /drafts/list, silently hiding the home-page "This workspace has N drafts" banner (and breaking the global drafts page). Prevent it at the source: sanitize the value in update_draft (the only path that writes client-supplied draft content) so a NUL never reaches the column. strip_json_nul does a single backslash-parity-aware byte pass that removes real NUL escapes (values and keys alike) while leaving a legitimate escaped backslash intact — O(n) with no serde_json::Value tree to allocate, important because the slow path is also hit by any value legitimately containing the text after a backslash (e.g. script source). The clean path is a single substring check. A SQL migration scrubs rows written before this, gated to genuinely-poisoned rows (a real NUL makes value::jsonb raise, distinguishing it from a legitimately escaped backslash). With the data clean, no read-side query needs to change. Tests: unit tests for the strip helper (escaped-backslash no-op, real+literal collision, odd-backslash-run parity, nested keys/values) and an integration test that POSTs a NUL-bearing draft and asserts it is stored and listed NUL-free (fails without the strip). Co-authored-by: Claude Opus 4.8 (1M context) --- ...60619113554_scrub_draft_value_nul.down.sql | 2 + ...0260619113554_scrub_draft_value_nul.up.sql | 30 +++++ backend/tests/drafts_nul.rs | 80 +++++++++++++ backend/tests/fixtures/drafts_nul.sql | 24 ++++ backend/windmill-api/src/drafts.rs | 112 ++++++++++++++++++ 5 files changed, 248 insertions(+) create mode 100644 backend/migrations/20260619113554_scrub_draft_value_nul.down.sql create mode 100644 backend/migrations/20260619113554_scrub_draft_value_nul.up.sql create mode 100644 backend/tests/drafts_nul.rs create mode 100644 backend/tests/fixtures/drafts_nul.sql diff --git a/backend/migrations/20260619113554_scrub_draft_value_nul.down.sql b/backend/migrations/20260619113554_scrub_draft_value_nul.down.sql new file mode 100644 index 0000000000..098a525396 --- /dev/null +++ b/backend/migrations/20260619113554_scrub_draft_value_nul.down.sql @@ -0,0 +1,2 @@ +-- Irreversible: a stripped NUL cannot be restored (and was never meaningful). +SELECT 1; diff --git a/backend/migrations/20260619113554_scrub_draft_value_nul.up.sql b/backend/migrations/20260619113554_scrub_draft_value_nul.up.sql new file mode 100644 index 0000000000..1ae927bae6 --- /dev/null +++ b/backend/migrations/20260619113554_scrub_draft_value_nul.up.sql @@ -0,0 +1,30 @@ +-- One-time cleanup of drafts whose `json` value carries a real U+0000 (NUL) +-- escape — storable only because `draft.value` is `json`, not `jsonb`. Such a +-- value makes any `->>`/`to_jsonb` extraction raise `22P05`, which 500'd +-- GET /drafts/list. New writes are sanitized in the application layer +-- (update_draft → strip_json_nul); this fixes rows written before that landed. +-- +-- Only genuinely-poisoned rows are touched: a real NUL makes `value::jsonb` +-- raise, which distinguishes it from a legitimately escaped backslash sequence +-- (which `jsonb` accepts). The text replace handles the real-world shape — a NUL +-- inside a text field. A contrived value where stripping the escape leaves +-- invalid JSON is left as-is (and can no longer be created). +DO $$ +DECLARE r RECORD; +BEGIN + FOR r IN + SELECT id, value FROM draft WHERE position(E'\\u0000' in value::text) > 0 + LOOP + BEGIN + PERFORM r.value::jsonb; -- not poisoned (legit escaped backslash): skip + EXCEPTION WHEN others THEN + BEGIN + UPDATE draft + SET value = replace(r.value::text, E'\\u0000', '')::json + WHERE id = r.id; + EXCEPTION WHEN others THEN + NULL; -- pathological shape; cannot strip in SQL, no longer creatable + END; + END; + END LOOP; +END $$; diff --git a/backend/tests/drafts_nul.rs b/backend/tests/drafts_nul.rs new file mode 100644 index 0000000000..d7314b8fb6 --- /dev/null +++ b/backend/tests/drafts_nul.rs @@ -0,0 +1,80 @@ +//! Regression test for NUL bytes in draft values. +//! +//! `draft.value` is a `json` column (not `jsonb`), so a U+0000 escape can be +//! stored and then make any `->>`/`to_jsonb` extraction raise `22P05` — one +//! poisoned draft 500'd `GET /drafts/list` (silently hiding the home-page +//! "This workspace has N drafts" banner). The fix sanitizes the value on write +//! (`update_draft` -> `strip_json_nul`) so a NUL never reaches the column; this +//! drives the real endpoint and asserts the stored + listed value is NUL-free. + +use serde_json::{json, Value}; +use sqlx::{Pool, Postgres}; + +use windmill_test_utils::*; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(b: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + b.header("Authorization", "Bearer DNUL_ADMIN_TOKEN") +} + +#[sqlx::test(fixtures("drafts_nul"))] +async fn test_draft_write_strips_nul(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/dnul-ws"); + + // Save a draft whose summary and content carry a real NUL. + let resp = authed(client().post(format!( + "{base}/drafts/update/script/u/dnul-admin/poison" + ))) + .json(&json!({ + "value": { + "summary": "hi\u{0}there", + "path": "u/dnul-admin/poison", + "content": "x\u{0}y" + } + })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "save should succeed: {}", + resp.text().await.unwrap_or_default() + ); + + // The stored value must be NUL-free (sanitized on write). + let stored: Value = authed(client().get(format!( + "{base}/drafts/get_own/script/u/dnul-admin/poison" + ))) + .send() + .await? + .json() + .await?; + let value = stored.get("value").expect("draft should exist"); + assert_eq!(value["summary"], "hithere"); + assert_eq!(value["content"], "xy"); + assert!( + !serde_json::to_string(value).unwrap().contains("\\u0000"), + "stored value still contains a NUL escape: {value}" + ); + + // The list endpoint uses raw `->>`; it works (200, no 500) because the + // stored data is clean, and the summary comes back stripped. + let items: Vec = authed(client().get(format!("{base}/drafts/list"))) + .send() + .await? + .json() + .await?; + let item = items + .iter() + .find(|d| d["path"] == "u/dnul-admin/poison") + .expect("saved draft should be listed"); + assert_eq!(item["summary"], "hithere"); + + Ok(()) +} diff --git a/backend/tests/fixtures/drafts_nul.sql b/backend/tests/fixtures/drafts_nul.sql new file mode 100644 index 0000000000..8f457ffbc3 --- /dev/null +++ b/backend/tests/fixtures/drafts_nul.sql @@ -0,0 +1,24 @@ +-- Fixture for the draft NUL-byte write-sanitization regression test. +-- Just a workspace + admin user + token; the test itself POSTs a draft whose +-- value carries a U+0000 and asserts it is stored (and listed) NUL-free. + +INSERT INTO workspace (id, name, owner) VALUES + ('dnul-ws', 'DNUL WS', 'dnul-admin'); + +INSERT INTO workspace_key (workspace_id, kind, key) VALUES + ('dnul-ws', 'cloud', 'dnul-key'); + +INSERT INTO workspace_settings (workspace_id) VALUES + ('dnul-ws'); + +INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES + ('dnul-ws', 'all', 'All users', '{}'); + +INSERT INTO password(email, password_hash, login_type, super_admin, verified, name, username) + VALUES ('dnul-admin@windmill.dev', 'x', 'password', true, true, 'DNUL Admin', 'dnul-admin'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('dnul-ws', 'dnul-admin@windmill.dev', 'dnul-admin', true, 'Admin'); + +INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin) + VALUES (encode(sha256('DNUL_ADMIN_TOKEN'::bytea), 'hex'), 'DNUL_ADMIN', 'DNUL_ADMIN_TOKEN', 'dnul-admin@windmill.dev', 't', true); diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 02c169f59e..5b47a06ec9 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -286,6 +286,10 @@ async fn update_draft( } else { serde_json::to_string(value).unwrap() }; + // `draft.value` is a `json` column, so a U+0000 (NUL) would persist as an + // escape and later make any `->>`/`to_jsonb` extraction raise `22P05`. + // Strip it here so a NUL never reaches the column. + let serialized = strip_json_nul(serialized); // Upsert. The conflict check rides on the DO UPDATE WHERE clause — // when the row is newer than `last_sync`, RETURNING yields nothing. // `created_at` defaults to `now()` but the migration overrides it ($8) @@ -453,6 +457,53 @@ async fn migrate_legacy_draft( } } +/// Remove every U+0000 (NUL) from a serialized JSON document so it is safe to +/// store in the `json`-typed `draft.value` (a NUL there would later make any +/// `->>`/`to_jsonb` extraction raise `22P05`). +/// +/// A NUL can only appear in JSON text as a backslash-u0000 escape, and a +/// backslash only ever occurs inside a string, so one backslash-parity-aware +/// pass removes every real NUL escape — covering values and keys alike — while +/// leaving a legitimate `\\u0000` (an escaped backslash followed by the literal +/// text `u0000`) intact. O(n) over the bytes with no `serde_json::Value` tree to +/// allocate, and the fast path (no such substring at all) returns the input +/// untouched. The slow path is reached not only by genuinely poisoned values but +/// by any value that legitimately contains `u0000` after a backslash (e.g. script +/// source), so it must stay allocation-light for potentially large drafts. +fn strip_json_nul(serialized: String) -> String { + if !serialized.contains("\\u0000") { + return serialized; + } + let bytes = serialized.as_bytes(); + let mut out: Vec = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] != b'\\' { + out.push(bytes[i]); + i += 1; + continue; + } + // Consume the whole run of backslashes. An even run is N/2 escaped + // backslashes and leaves the next char unescaped; an odd run ends in an + // escaping backslash, so a following `u0000` is a real NUL escape. + let run_start = i; + while i < bytes.len() && bytes[i] == b'\\' { + i += 1; + } + let run = i - run_start; + if run % 2 == 1 && bytes[i..].starts_with(b"u0000") { + // Drop the escaping backslash + `u0000`; keep the leading literal pairs. + out.extend(std::iter::repeat(b'\\').take(run - 1)); + i += 5; + } else { + out.extend(std::iter::repeat(b'\\').take(run)); + } + } + // Only whole ASCII backslash-u0000 escapes were removed, so the bytes remain + // valid UTF-8 (and valid JSON). + String::from_utf8(out).expect("removing a NUL escape preserves valid UTF-8") +} + /// For variable-kind drafts with `variable.is_secret == true`, encrypt /// `variable.value` with the workspace crypt key and mark it /// `$encrypted:` so the secret never persists in plaintext at rest. @@ -743,3 +794,64 @@ async fn require_can_read_path( } Err(Error::NotFound(format!("no draft visible at {path}"))) } + + +#[cfg(test)] +mod tests { + use super::strip_json_nul; + + // Parse the (NUL-free) result so assertions read clearly. + fn parsed(s: String) -> serde_json::Value { + serde_json::from_str(&s).expect("strip_json_nul must return valid JSON") + } + + #[test] + fn clean_value_is_returned_byte_for_byte() { + let s = r#"{"summary":"all good","n":1}"#.to_string(); + assert_eq!(strip_json_nul(s.clone()), s); + } + + #[test] + fn real_nul_in_value_is_stripped() { + let out = strip_json_nul(r#"{"summary":"hi\u0000there"}"#.to_string()); + assert!(!out.contains(r"\u0000")); + assert_eq!(parsed(out)["summary"], "hithere"); + } + + #[test] + fn legit_escaped_backslash_is_a_noop() { + // JSON "a\\u0000b" decodes to the 8-char string a,backslash,u,0,0,0,0,b + // — not a NUL — so the value is already clean and round-trips byte-for-byte. + let s = r#"{"summary":"a\\u0000b"}"#.to_string(); + assert_eq!(strip_json_nul(s.clone()), s); + } + + #[test] + fn collision_real_and_literal_both_handled() { + // "a" carries a real NUL; "b" carries the literal text backslash-u0000. + // The value walk strips the former and leaves the latter intact — the + // pathological case that needed a fallback in SQL is trivial in Rust. + let v = parsed(strip_json_nul(r#"{"a":"x\u0000y","b":"p\\u0000q"}"#.to_string())); + assert_eq!(v["a"], "xy"); + assert_eq!(v["b"], "p\\u0000q"); + } + + #[test] + fn nested_values_and_keys_are_cleaned() { + let out = strip_json_nul( + r#"{"o":{"k\u0000":["a\u0000b",{"deep\u0000":"v\u0000"}]}}"#.to_string(), + ); + assert!(!out.contains(r"\u0000")); + let v = parsed(out); + assert_eq!(v["o"]["k"][0], "ab"); + assert_eq!(v["o"]["k"][1]["deep"], "v"); + } + + #[test] + fn odd_backslash_run_keeps_literal_drops_nul() { + // JSON "a\\\u0000b" is an escaped backslash (kept) immediately followed by + // a real NUL escape (dropped) -> decodes to a,backslash,b. + let v = parsed(strip_json_nul(r#"{"x":"a\\\u0000b"}"#.to_string())); + assert_eq!(v["x"], "a\\b"); + } +}