fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) - #1220
Draft
xarmian wants to merge 27 commits into
Draft
fix(server): refuse a decoded NUL in a JSON request body (BUG-2803)#1220xarmian wants to merge 27 commits into
xarmian wants to merge 27 commits into
Conversation
The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied
string reached a Postgres text parameter, Postgres refused it, and the
handler answered 500 — the honest answer is 400.
WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this
is a different fix rather than a wider middleware. ValidateQuery works
because a decoded query value is a substring of the raw query with ASCII
substitutions: the bad byte in the raw text IS the bad byte in the value.
That property fails for a JSON body — the reachable NUL arrives as the
six-character escape, all ordinary ASCII — so no request middleware can
find it without decoding the body, which is the handler's job.
MECHANISM, each premise measured against encoding/json rather than
reasoned about:
raw NUL inside a string -> decode ERR (invalid character in string literal)
raw NUL after the value -> decode ERR
the escape in a value -> decodes to a string CONTAINING a NUL
the escape in a KEY -> same
a DOUBLED backslash -> decodes to literal text, NO NUL
the uppercase spelling -> not a JSON escape at all
So the escape is the only vector and its substring is a sound FAST PATH
(absent -> no NUL possible), but not a sufficient test: a doubled
backslash carries the same six characters and decodes to text. In this
product that is not hypothetical — items and documents store markdown,
and a document about JSON escapes is an ordinary thing to write. The
exact step is json.Decoder.Token(), which returns DECODED strings, covers
object keys and arbitrary nesting (an item's fields blob), and needs no
knowledge of the destination type.
NOT REFLECTION over the decoded value, the other obvious design: it sees
[]byte fields AFTER base64 decoding, so a body carrying legitimate binary
({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not
text. A token walk sees the base64 characters. No request struct has such
a field today (searched: []byte with a json tag in internal/server and
internal/models, non-test — only models.YjsUpdate.UpdateData, which no
handler decodes from a body); the token walk is chosen so adding one
later cannot silently start rejecting valid requests.
BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole
top-level value in memory — refill accumulates into dec.buf and grows it
by doubling (encoding/json/stream.go) — so streaming never avoided the
copy. Measured on the 64 MiB workspace-import shape, total allocation:
stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode
512.5 MiB. Peak heap is order-dependent and does not discriminate; the
first run of that measurement showed a 0.77x peak win that vanished when
the legs were swapped, so only the allocation figure is claimed.
POPULATION, measured on Postgres 17 through the real router with a
control leg on every endpoint (92 mutating routes enumerated via
chi.Walk; 13 probed):
before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021)
after: 0 of 13 — every NUL leg 400, every control leg unchanged
Confirmed doors: workspace name, collection name, item title, item
content, item fields value, item title via PATCH, comment body, agent
role name, view name, document title, webhook secret, workspace import.
workspace-token name is UNMEASURED, not clean — its control leg 500s on
an unrelated FK in this fixture. The other 79 routes are unprobed, not
claimed clean; the completeness argument is structural instead, and
enforced by a test rather than asserted.
SECOND DEFECT, named rather than slipped in: the six handlers that
decoded straight off r.Body had no http.MaxBytesReader either — the cap
decodeJSON has always applied — so each was an unbounded body read.
Routing them through decodeJSON closes that too.
COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the
JSON value where Decode ignored it. Deliberate, same direction as this
fix, and the only behaviour change beyond the refusal. Trailing
whitespace still passes. An EMPTY body still returns a wrapped io.EOF,
because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no
arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is
exactly the wiring a helper-level change is blind to.
No call site changed for the refusal itself: all 65 decodeJSON callers
already turn a decode error into a 400 carrying err.Error().
Release note: a NUL character in a JSON request body now returns 400
instead of 500 on Postgres deployments.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…BUG-2803) Codex round 1 on #1220: the check scanned ONE JSON layer, and several fields cross the wire as JSON-ENCODED STRINGS rather than nested objects — an item's fields, a collection's schema, a workspace's settings. The OUTER decode of {"fields":"{...}"} yields the inner document as literal text, in which the escape is still six ordinary characters and no NUL exists, so the single-layer token walk passed it. MEASURED on Postgres 17 with a control leg on each, after the single-layer check was already in place: item.fields as a JSON-encoded string 500 control 201 collection.schema as a string 500 control 201 workspace.settings as a string 500 control 201 The error is DIFFERENT from the rest of this family, which is why it is worth reading rather than assuming: insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05) 22P05, not the 22021 the path and query halves produce. The outer string is pure ASCII so it never trips the text-encoding check; this is Postgres's own JSON parser refusing the escape inside a document bound for jsonb, which cannot represent a NUL. After this change all three answer 400 with their control legs unchanged. THE FIX: when a decoded string is itself a complete JSON object or array — the class this API re-parses downstream — walk it too, to a depth bound of 8. Recursion terminates on its own (each level is a strict substring of the one above); the bound keeps a hostile body from buying many full re-parses, and AT the bound the body is refused rather than passed uninspected, since the escape is known to be present and the walk has stopped looking. WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is structural, not destination-typed, so a plain TEXT field whose ENTIRE value is a valid JSON document carrying the escape is refused too, even though its column would have stored it. Prose ABOUT a JSON escape does not parse as a bare document, so the case is narrow, and a value of that shape breaks any consumer that parses it. The destination-typed alternative — an allow-list of the fields that arrive JSON-encoded — is exactly correct and goes stale in silence, which is the failure mode ValidateQuery's comment rejects when it explains why per-site query validators could not be written. Tests: nested documents (fields/schema/settings/array/twice-encoded), with controls for ordinary content, a doubled backslash INSIDE the nested document, a string that starts like JSON but does not parse, and prose that merely mentions the escape; the over-refusal pinned as a decision rather than left as an accident; the depth bound; and a wiring leg through the real router on SQLite, where the write would otherwise SUCCEED so a green cannot be the database doing the work. Fixtures build their JSON-encoded strings with encoding/json rather than hand-written backslashes, since the escaping rules are the subject under test. All four new tests fail with the recursion removed. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…e (BUG-2803) TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the API on a premise its own comment stated: "a structured id comes from the item's fields blob, which nothing validates on write". BUG-2803 made that false — decodeJSON now refuses a body whose strings decode to a NUL, including one nested inside a JSON-encoded `fields` string — so the API can no longer produce the row and the test 400'd on its fixture. Repaired rather than deleted, because the DEFENCE it covers is still live: rows in this shape can predate the rule, and the store has no such check of its own, so a migration, an import or any future non-HTTP writer can still produce one. The timeline must keep refusing to hand out a cursor it would then reject. The fixture now writes the blob directly, injecting the six-character JSON escape rather than a raw NUL — the blob is JSON text and both backends reject a raw NUL in it; the NUL comes into existence when Go DECODES the blob, which is exactly how the timeline ends up with one inside an entry id. The test is not vacuous under the change: it asserts the NUL-bearing id took the positional fallback, so an injection that failed to produce a NUL fails the test rather than passing quietly. This is the CONVE-23 case — a change that falsifies existing prose owes a sweep for that prose. The stale sentence was found by the test failing, not by the sweep, which is the weaker of the two ways to find it. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
The entryID fallback's comment said note and decision ids "come from the item's fields blob and nothing validates them on write". BUG-2803 made that half false: the HTTP API now refuses a request body whose strings decode to a NUL, including one nested inside a JSON-encoded `fields` string. The sentence was true when written and nothing in this branch's diff pointed at it. The fallback still has to exist, and the corrected comment says why: the STORE has no such check, so rows predating the rule — and anything writing a blob by another path, a migration, an import, a future non-HTTP writer — can still carry one. SWEPT AND DELIBERATELY LEFT: two nearby comments (handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go) also say "nothing validates them on write". Both are about id FORMAT and DUPLICATION — an imported artifact carrying a UUID-shaped id, a hand-written blob repeating one — and this change validates neither. In context those sentences remain true, so they are left alone rather than edited into noise. Sweep command: grep -rniE "nothing validates|not validated on write|no validation on write|unvalidated" --include=*.go internal/ cmd/ — six further hits, all about other subjects (github_pr raw writes, terminal schema keys, push payload format, decodeJSON's size bound). Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 2 on #1220. The nesting check from the previous commit recursed into ANY string that parsed as a JSON document, on the argument that a structural test beats a destination-typed one. That argument was wrong in a way I had written down as an accepted trade and should have weighed as a defect: a plain-text `content` value holding a JSON snippet that merely MENTIONS the escape was accepted before this branch, is stored in a text column that has no problem with it, and was newly refused — including on RE-IMPORT of an export carrying it. Refusing input the server itself produced is a worse failure than the door the unscoped recursion was closing. Measured before the fix: a workspace whose item content held such a snippet exported 200 and re-imported 400. The walk now descends only under keys whose STRING value is a JSON document something downstream re-parses: config, events, fields, metadata, phase_data, plan_overrides, schema, settings, tags, traits. WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly this shape for query parameters: there the set of names is unbounded by design (parseItemListParams turns any unrecognised parameter into a field filter), so no list could be complete. Here the set is a closed property of the wire model — a field is JSON-encoded because a Go struct declares it as a string holding JSON — and TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models and fails when a new one appears. The list cannot go stale in silence. Over-inclusion is the safe direction and the list takes it: a listed key that is not really JSON-encoded costs one parse attempt and can only refuse a complete JSON document carrying the escape, while a missing key reopens a door. `traits` is listed for that reason — it carries JSON but its declaration has no comment saying so, which is exactly how the derivation test would have missed it, so the test asserts coverage in one direction only and the list is allowed to be a superset. The walk also changed shape: decoding into `any` and walking the value, rather than a token stream, because key context is needed to know which subtree is JSON-encoded. The []byte reasoning is unchanged and still holds — decoding into `any` never produces a []byte, so a base64 field is seen as its ASCII text rather than as decoded bytes that might contain a legitimate 0x00. Tests: text fields carrying a JSON document are ACCEPTED (five keys), with a leg proving the same document under a JSON-encoded key is still refused, so the pair differs only in the key; the derivation test; and the depth-bound fixture now nests under a JSON-encoded key at every level, since nesting under an ordinary key would never start the recursion and would have passed for the wrong reason. STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob already carries the escape still exports 200 and re-imports 400. That is data this fix cannot make importable without weakening the write-side refusal, and the disposition (repair sweep, flagged import, or documented acceptance) is a product ruling. Recorded on the item. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
All three verified before fixing, none taken on the reviewer's word.
1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go
parses pad-export.json itself rather than through decodeJSON, so the
SAME workspace import — reached with Content-Type application/gzip
instead of application/json — walked straight past the NUL check into
Postgres. The bundle's export blob is now checked with bodyDecodesNUL
before ImportWorkspace, answering the same 400. Test drives a real
tar.gz through the router with a clean-bundle control leg, because this
path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order
tar, duplicate entries) and a bare 400 would prove nothing.
2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's
test-email endpoint read `if err := decodeJSON(...); err != nil ||
input.To == ""` and fell back to the admin's own address, so a body
carrying a NUL answered 200. An ABSENT body legitimately means "send it
to me"; a body that is present and REFUSED is a different thing, and
collapsing the two turns a validation error into a success. The two
cases are now separated on errors.Is(err, io.EOF).
3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It
scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was
blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the
package — and to any alias or helper. A completeness test that misses a
live example is worse than none, because it reads as coverage. It now
scans for the thing that cannot be spelled around, a reference to the
request body at all, and requires every FILE touching one to be
accounted for with a written reason. Both directions are asserted: an
unaccounted file fails because a door may have opened, and an accounted
file that no longer touches a body ALSO fails, so the list cannot rot
into stale excuses that quietly cover a future reader. Verified with a
positive control (an added body reference in an unlisted file fails) and
a negative one (a stale entry fails).
FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw
artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not
JSON, so it never went through decodeJSON and inherited neither the NUL
refusal nor the path/query rule — a body is neither. A raw NUL or
invalid UTF-8 reached the store and Postgres answered 22021, which the
handler turned into a 500 for what is a client error. It now applies
bindableText, the same predicate ValidatePath and ValidateQuery use, and
answers 400 invalid_body. Note the shape difference from the JSON half:
there the ESCAPE is the vector because a decoder rejects a raw NUL;
here the RAW BYTE is, because nothing is in the way.
Each fix has a mutation run against it: disabling the bundle guard fails
the bundle test, disabling the artifact guard fails the artifact test,
and both controls still pass.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…803) Codex round 4, two P1s, both reproduced before fixing. 1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does the raw body contain the six-character escape". That is unsound: the BACKSLASH itself can be written as an escape, so a body carrying \u0000 contains no literal six-character sequence anywhere in its raw bytes, while the OUTER decode manufactures one inside the string — and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys) the second parse turns it into a real NUL. Measured through the real router before the fix: the oblique spelling answered 201 where the direct one answered 400. The mistake was applying a fact about how a NUL is spelled INSIDE a decoded string to the RAW BYTES, where the backslash can itself be an escape. That is the same layer-confusion this whole bug is made of, for the third round running. The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so a body with no backslash has decoded strings byte-identical to its raw bytes, and a raw NUL cannot survive the decoder — no backslash therefore means no NUL, at any depth, however spelled. Bodies WITH one pay for an exact answer, a larger set than before (any nested JSON carries a backslash-quote), which is the cost of being correct. The same correction applies to the per-string pre-filter one level down. 2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in the request bytes — and the YAML decode manufactures one. Measured before the fix: that artifact imported 201 with a NUL in the item title. The decoded artifact is now checked too: title, body, and every frontmatter field value, walked because a playbook's `arguments` is a nested structure rather than a scalar. Keys are checked as well as values, on the same precautionary grounds ValidateQuery states for query parameter names. Same shape as the JSON half in both cases: a value that is harmless until a SECOND parse, checked at the layer that can see it. Tests: the oblique spelling joins the nested-document table, and the YAML escape joins the artifact table. Each is mutation-verified — reverting the gate to the substring fails the oblique case only, and disabling the post-decode artifact check fails the YAML case only, with the raw-byte cases still killed by the raw check. That per-leg discrimination is the point: it shows each check earns its own keep rather than being covered by its neighbour. Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY spelling" is true of the escape and was being used to justify a filter on the raw bytes, which is a different claim. Both now say so explicitly. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 4's two P2s. Both are the same shape as the rest: a caller-supplied string reaching a text comparison through a door the earlier fixes did not cover. 1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from the JSON rule — its payload is binary blob content and must not be scanned for text validity — but its TEXT fields are a different thing. `item_id` goes to ResolveItem and into a database comparison exactly as the query-string channel does, and that channel has been validated at the transport since BUG-2784; the form channel was not. multipartValues now drops values that are not bindable text, which makes an unusable value indistinguishable from an absent one — the disposition resolveUploadItemID already applies to empty values. The uploaded FILENAME gets the same predicate, with a fallback to a generic name rather than a refusal: the bytes are fine, only the label is unusable. A NEGATIVE RESULT worth recording, because it changed the test: a RAW NUL in the multipart header is NOT the vector. Go's multipart reader refuses it as a malformed MIME header line before any handler sees it (measured: 400, "malformed MIME header line"). The reachable spelling is the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the header parser accepts and percent-decodes afterwards. The first version of this test used the raw form and was testing a vector that does not exist. 2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the tar.gz, parsed directly like pad-export.json was, so it needed the same check. Without it a NUL in a manifest string reached rehydrateAttachment, whose failure is logged and SKIPPED — so the import reported success while silently dropping the attachment. The skip-on-failure behaviour is pre-existing and deliberate (a partial restore beats none); refusing the bad INPUT is what stops it being reached this way. Left as it is, and named rather than quietly changed. A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would otherwise have shipped as coverage: the filename leg first asserted `!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL in the filename comes back as the six-character escape, not as a 0x00, so the check passed whether or not the fix was present. It did pass with the fallback disabled. Now it decodes the response and asserts the replacement name. The item_id leg had the mirror-image weakness: it asserted "not a 500", which is the Postgres-only symptom, so on SQLite it would have passed either way; it now asserts the request behaves exactly like the no-value control. Every fix in this commit has a mutation against it, and each kills only its own leg. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
The gate became a backslash check, which was the last production use of jsonNULEscape; golangci-lint's unused check failed on the next run. Its documentation was load-bearing, so the explanation moved into bodyDecodesNUL's comment rather than being deleted with the variable — including the distinction that made the old gate wrong (the escape has one spelling INSIDE a decoded string, which is not a claim about the raw bytes). Caught by re-running lint on the tip after the previous commit rather than trusting the run from the tip before it. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 5 was asked for the POPULATION rather than a confirmation — "enumerate every remaining way a caller-supplied string can reach a database text or jsonb parameter without passing a validity check" — and returned three residual classes with their sinks. Two are fixed here; the third is filed, because measuring it needs a fixture this unit should not grow. 1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string with a plain byte slice (name[:120], input.Name[:200]). If the boundary lands inside a multi-byte rune the result ends in a partial sequence and is no longer valid UTF-8 — so a value that PASSED the body check a few frames earlier arrives at the store unbindable, and Postgres answers 22021 for a request the server already accepted. This is the interesting one, because no input-side round could have found it: the defect is downstream of validation, and it is invisible with ASCII fixtures, which is what every test in that area used. truncateBindableText walks back off continuation bytes and drops the straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one walk-back cannot pass them all, and with a counterfactual leg asserting the naive slice really does produce unbindable output for the same input — without it the cases would pass against an implementation that did nothing. 2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent (three document paths, the connected-apps revoke) and sessions.user_agent (three login paths), and no rule here sees a header. The disposition is SANITISE, not refuse, and that is deliberate: a header is metadata this server chose to record, not something the caller asked for, so a malformed one must not turn an otherwise fine request into a 400. The two sites that HASH the header are left alone — sha256 over arbitrary bytes is well defined, and changing what is hashed would invalidate every stored UAHash. The filing's own earlier probe had recorded User-Agent as NOT reproducing on the item-create path. That was true and did not generalise; these are different sinks. 3. NOT FIXED, FILED: the OAuth form-encoded bodies (/oauth/token, /oauth/authorize/decide, /oauth/revoke, /oauth/introspect) parse url-encoded form data outside the shared body validator, with connection_name reaching oauth_connections.name and client_id reaching the oauth_clients.id lookup. This was the ORIGINAL subject of BUG-2803 before the filing was re-scoped, and it was recorded then as unreachable without a fosite-backed fixture. That is still true, and round 5's sink list is far more than the filing had. Filed rather than guessed at. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…-2803) Codex round 6 plus one measurement of my own. Three changes, one of them a revert of something I got wrong in the previous commit. 1. THE GATE COST TOO MUCH, so it is narrower and still sound. The previous commit gated the walk on "does the raw body contain a backslash", which is correct but catches every body carrying nested JSON (each `\"` is a backslash). Measured on a ~377 KB import-shaped body: 60106 allocs/op with that gate versus 30073 with the walk disabled — the walk was running on ordinary traffic. The gate is now the four bytes that begin any \u escape for a character below U+0100. The argument: to manufacture the six-character NUL escape inside a decoded string, each of its characters arrives either literally from the raw bytes — in which case the raw contains the escape, which begins with that prefix — or from a \u escape of its own, and the three characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit below U+0100, so those escapes begin with it too. Back to 30073 allocs/op, identical to the walk-disabled build. That argument is the same KIND of reasoning that was wrong two rounds ago, so it does not stand on its own: a differential test runs the gated function against an UNGATED walk over a corpus built to attack it — oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a doubled backslash — and fails on any disagreement. It also asserts the corpus contains both answers, since agreement over a one-sided corpus would be vacuous. Reverting the gate to the old substring fails it. 2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and `fields` accept both a JSON-encoded STRING and their natural array/object form, and the walk propagated "this subtree is JSON-encoded" into containers — so a free-form tag whose whole value happened to be a JSON document was refused, though nothing re-parses it. Measured: refused before, accepted now, while the JSON-encoded spelling of the same field is still refused. The flag now marks only a direct STRING child of a listed key. 3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser last commit, before reading store.CreateSession. It HASHES the header and stores no text — the round-5 enumeration named "sessions.user_agent" and I took the name for a column. The change would have been actively harmful: login would store sha256(sanitised) while middleware_auth still compares sha256(RAW), so every session from a client with a non-UTF-8 User-Agent would fail validation. A sink named in a review is a pointer to verify, not a finding. The real sink is activities.user_agent, from three document paths and the connected-apps revoke. 4. And the wiring leg codex asked for, on that real sink: a request through the router with a malformed header, reading the STORED value out of the activities row, with a control asserting an ordinary header is kept VERBATIM. Unwiring the production call site fails it; the helper's unit test does not notice. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 7, both findings, the first confirmed by measurement. 1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into a JSON-encoded string it treated the WHOLE subtree below as JSON-encoded, so a value nested two levels down — an ordinary string inside a `fields` blob that happens to hold JSON text — was refused. That is a false rejection, and the measurement says so plainly. With the depth-2 check disabled, on Postgres 17: depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it) depth 2 (a string INSIDE the blob) -> 201 (accepted, no error) control -> 201 The handler parses `fields` ONCE. The inner text is re-escaped when the blob is written, so what Postgres receives has a doubled backslash and no escape at all. Only the document Postgres itself parses can carry a fatal one. The nested call now passes false rather than true, which makes this a KEY RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key INSIDE a document still recurses (pinned by a test), an ordinary one does not. Same correction as round 6's natural-shape fix, one level further in — I fixed the sibling case and left this one, which is CONVE-18's lesson about my own enumeration being a sample too. I checked whether anything re-parses a value inside the blob before loosening this, rather than assuming: `arguments` was the candidate, and parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather than a JSON string, so it is covered by the natural-shape rule and needs no second parse. 2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic client registration handler prefixed every decode failure with "Request body must be JSON". A body carrying a NUL is valid JSON, so that message sends a client hunting a syntax error it does not have. The two failures are now distinguished. Round 7 also reports no break in normal CLI, MCP or web-client request generation — they marshal JSON and encode paths and query parameters — which is the first thing any round has said about the client surface. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…UG-2803)
Codex round 8. It confirmed round 7's two fixes, then found two real
defects and two inaccurate comments — the comment half being the angle
the round was asked for.
1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText
walked the decoded artifact by TYPE, so it never covered Provenance —
whose strings are rendered into a Markdown footer appended to the stored
content — and never matched Arguments, declared []map[string]any, a
concrete slice type the walk's []any case does not match. A YAML NUL
escape in either reached storage.
It now MARSHALS the artifact and searches the output for the escape
encoding/json produces. A type switch over a struct that grows is a list
that goes stale in silence; marshalling covers every exported field,
including ones added later. The one thing it cannot see is invalid UTF-8
(which marshals to U+FFFD), and it does not need to: step 2 rejects that
in the request bytes, and YAML cannot manufacture it from valid input —
its escapes name code points, where \0 names a NUL.
Both new cases fail with the check disabled; the raw-byte cases still
pass, killed by the raw check, so each leg is discriminating.
2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field
literally named `schema` or `tags`. The walk consulted the wire-key list
at every level, so `{"fields":{"schema":"..."}}` treated a user field
name as a wire key and refused valid text holding a JSON example.
The key list is now consulted only OUTSIDE caller data — not under a
natural `fields` object, not inside an element of a `tags` array, not
inside a re-parsed document. Combined with round 7's fix that makes the
descent exactly one level deep BY CONSTRUCTION, which is why the depth
counter is gone: with the flag no longer inherited, a bound could never
fire, and dead protection reads as protection. The depth-bound test is
replaced by one that pins the property directly — an escape IN the
parsed document is refused, one BELOW it is accepted, and a
wire-key-shaped user field does not restart the descent.
3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would
believe without checking:
- MaxBytesReader: Close FORWARDS to the underlying body rather than
being a no-op, and with a nil writer there is no automatic 413 — the
cap surfaces as a read error the callers turn into 400. Behaviour
unchanged; only the claim was wrong.
- parseArtifactRequest said "three checks" while implementing five, and
its returns list omitted ErrArtifactUnbindableText. Both added by this
branch, which is exactly the prose a change is most likely to falsify
(CONVE-23).
- errJSONBodyNUL claimed all 65 callers surface its message. The STATUS
is uniform; the wording is not — several substitute a generic string.
4. And one in a test: the timeline fixture said both backends hold a
CHECK constraint a raw NUL violates. items.fields is a plain TEXT column
with no CHECK on SQLite. What was OBSERVED is "SQL logic error:
malformed JSON"; the likely source is an expression index over
json_extract, and that attribution is recorded as NOT verified rather
than asserted.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…gain (BUG-2803)
Codex round 9. Both findings are mine, one of them a regression from the
round-8 restructure two commits ago.
1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the
JSON-encoded branch for a listed key skipped the plain "does this string
contain a NUL" check and asked only "does the document this string
carries hold an escape". Those are different questions. So
{"fields":"a<NUL escape>b"} — a direct NUL in the fields value, the very
first case this whole change closed — was accepted again.
Both checks now run. The test pins all three legs: a direct NUL in the
fields string, an escape inside the fields document, and an ordinary
fields string that must still be accepted, so the first two cannot pass
merely because everything under a listed key is refused.
2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It
searched the MARSHALLED bytes for the escape sequence, and a value
holding the six LITERAL characters marshals to a doubled backslash which
still contains that sequence as a substring — so valid content was
refused. Artifacts are documentation; text about a JSON escape is
exactly what one carries.
Worse than the bug: the comment I wrote asserted the ambiguity "cannot
arise here". It was the same doubled-backslash case bodyDecodesNUL exists
to resolve, one function away, and I wrote a sentence explaining why it
did not apply instead of checking. The marshalled form is now decoded
again and walked with the same machinery — the round trip is what makes
every field reachable without a type switch, the walk is what makes the
answer exact.
Its test asserts literal escape TEXT is accepted in title, body and a
field value, with a counterfactual leg asserting a real NUL in each of
those places is still refused, so acceptance cannot come from the check
doing nothing.
Reverting either fix fails its test and only its test.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 12, an operational pass. It found no migration or config requirement, and two documentation gaps. docs/backup.md promises that application-level export/import is portable across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a workspace whose stored data contains a NUL exports fine and is refused on import. It can only affect data written before the rule existed and only on SQLite, which accepted it — a PostgreSQL instance never stored one. `pad db migrate-to-pg` has the SAME problem and reports it worse: it copies rows directly and never passes through the import guard, so a legacy row fails against PostgreSQL's JSONB parser partway through the copy rather than being refused up front. That is the likelier way an operator meets this, since it is the operation that puts an entire old SQLite database in front of PostgreSQL for the first time. Recorded on BUG-2810, which owns the preflight and repair. Round 12's other finding — that the PR's stated release note covered the JSON 500-to-400 change and none of the rest — is fixed in the PR body rather than in the tree. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 13, asked whether the new TESTS are sound. Five findings; these are the two that were self-contained. The other three are recorded on the item with what each needs. 1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only `.Body`, so FormValue / ParseForm / MultipartForm — which read the request body just as surely — were invisible. It therefore reported full coverage while the OAuth form-encoded handlers were entirely outside its view. Widened, and it immediately failed on handlers_oauth.go, which is the instrument working. That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the OAuth handlers read form-encoded bodies that no rule in this family covers (the transport rules see the query half of r.Form, not the body half), tracked as BUG-2811 and needing a fosite-backed fixture to measure. The test now STATES the gap instead of being blind to it, which is the difference between a completeness claim and a completeness appearance. 2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its assertions were: within the limit, bindable text, a prefix of the input. An empty string satisfies all three. It now also asserts that an input fitting the limit comes back UNCHANGED, and that no more than one rune (4 bytes) is lost to the boundary — so a truncator that drops too much fails, not just one that keeps too much. Both were found by asking whether a broken implementation would pass, which is the question CONVE-12 is about and which I had applied to the production code and not to these two tests. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 13's other three findings, all of the same shape: a test that would stay green with the production change reverted. 1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives containing only pad-export.json, so disabling the INDEPENDENT attachment- manifest check left the suite green. The new test builds a bundle with both entries, differing only in the manifest, so a refusal cannot come from the export half. Verified by disabling each check separately: only the matching test fails, so the two are independently covered. 2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg exercised decodeJSON, so reverting handlers_admin.go to default EVERY decode failure to the admin's own address passed them all. The new test drives the real endpoint with a wired mock sender and pins the distinction that used to collapse: an ABSENT body still means "send it to me" (control), an ordinary body still sends (control), and a body that is present and refused answers 400 rather than being reinterpreted as the default recipient. 3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs while letting malformed UTF-8 through would have passed it. It now drives both, which matters because invalid UTF-8 is the class that reaches Postgres as 22021 on a UTF8 database. Round 13 was asked whether the new TESTS are sound — deterministic, order-independent, and failing on broken code. It reported the fixtures isolated and found five ways they were not discriminating. Two were fixed in the previous commit; these are the rest. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 14 confirmed round 13's five, then found the same shape one level out: reverting a SINGLE call site back to the unsafe form left the whole suite green, because the surviving fixtures are ASCII and a helper's unit test does not care who calls it. TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY rather than adding a fixture per site (an OAuth connection, a cloud login, four audit paths). A byte-slice truncation of a caller string fails it, and so does a raw User-Agent read outside the exempt set. Both directions are checked: finding none of the SAFE form also fails, so a scan that silently matched nothing cannot pass forever. The User-Agent exemptions carry counts rather than being blanket, so a NEW raw read in an exempt file still fails. All four reads in handlers_auth.go are exempt because they feed a HASH — CreateSession hashes the header and stores no text — and sanitising before hashing would be actively harmful: login would store sha256(sanitised) while the session check still hashes the RAW header, failing validation for every client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw read is requestUserAgent itself. Verified by reverting one truncation call site and one User-Agent call site independently; each fails the test. Round 14's third finding is fixed behaviourally rather than statically, because the static scan cannot see it — handlers_oauth.go is already listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives the real dynamic-registration endpoint with cloud mode and an OAuth server wired, with a control leg registering successfully, and pins both the refusal and the message split: the body IS valid JSON, so the answer must not send a client hunting a syntax error. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 16, asked whether this change is consistent with its siblings
in the same file and extensible by someone who did not write it. It found
a live bypass instead.
encoding/json matches an incoming key to a struct field by an exact match
first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and
{"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does.
The walk looked the key up case-SENSITIVELY, so it skipped the nested
document for a body the handler went on to accept, and the database
answered the original 500.
Measured before the fix: `fields` refused, `Fields` and `FIELDS`
accepted.
This is the same defect shape as everything else in this unit — a check
that agrees with one layer's rules while the layer that actually consumes
the value uses different ones — which is why the fix is a PREDICATE
rather than a wider map: the map is the vocabulary, and the matching RULE
belongs to the consumer. Someone adding a key should not also have to
remember to add its spellings.
The test drives six spellings including mixed case, with a control
asserting an unlisted key stays caller data in any casing, so this is
case-insensitive matching rather than matching everything.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Codex round 17, first of five findings. The previous commit fixed the
ASCII half of key matching and left the Unicode half, which is this
bug's own pattern one more time.
encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing.
U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the
`schema` struct field while strings.ToLower("ſchema") is unchanged and
missed the allowlist — a nested NUL under that spelling reached the
handler undetected.
Matching is now strings.EqualFold against each canonical key. The test
carries both a lower-case fold spelling and an upper-case one alongside
the ASCII cases, and keeps its control asserting an unlisted key stays
caller data in any casing.
The other four round-17 findings are recorded on the item rather than
patched here: they are genuine layer disagreements (duplicate keys
merging differently in a typed decode than in a map, a scan-failure
disposition on inputs the typed decode tolerates, and unknown-field
policy) whose fixes are design decisions rather than corrections, and
this seat is near its context bar. Each is written up with the
measurement it needs.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…l workspace (BUG-2803) Codex round 18. The comment on the manifest NUL branch said refusing the input "stops it from being reached this way" and stopped there, which reads as though the refusal undoes the import. It does not. A plain error with a non-nil workspace keeps the partial workspace, exactly as every other manifest failure in this loop does — the rollback branch fires only for *importStatusError, and mid-stream manifest failures intentionally keep what was imported (TASK-896). Returning a rollback-shaped error here would give NUL-bearing manifests different semantics from malformed ones, which is a change to the bundle-import contract rather than a fix to this bug. So the behaviour is unchanged and now DELIBERATE: the comment states it, and the test asserts the persisted state rather than only the HTTP answer. Mutation: routing the branch through *importStatusError makes the refusal roll back, and the new assertion fails naming the release note it would falsify. The pre-existing status/body assertions do not notice. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
…, and pin them (BUG-2803) Lead ruling day-68 is land-and-follow: this branch lands on its measured commits, and the token-stream rewrite is the BUG-2812 unit's spec rather than a late restructure of an 18-commit branch under review pressure. That makes the four open findings from rounds 16-17 something to WRITE DOWN precisely, not something to leave in a trail comment. The doc comment on bodyDecodesNUL now carries all four, with the one root cause named: this scan decodes into map[string]any and the typed decode does not agree with that model about keys. Two under-refuse (duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec - both dissolve under a walk that never builds values. Two over-refuse (unknown fields; case-variant duplicates) and are ACCEPTED, because refusing is the safe direction. The asymmetry is stated rather than smoothed over: within the map model, (1) and (4) are one defect seen from two sides and only one of them fails safe. Finding (3) is an observable compatibility change - a forward-compatible field carrying a NUL escape now gets a 400 where it got a 200 - so it goes in the release note as well as here. A qualification only protects where the actor meets it. All four are pinned by a test, measured on this tip rather than carried over from the round-16/17 write-up. The two known-gap legs assert the WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc comment and the release note as what to update. Both gap legs carry a premise assertion - the same bodies with the disagreement mechanism removed ARE detected - without which they would pass against a check that detected nothing. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
…e-state measured (BUG-2803) The disposition test proves bodyDecodesNUL RETURNS true for an unknown field carrying a NUL escape. The release note claims the API answers 400. Those are different claims and only the second one is what an operator or client author reads - CONVE-19, my own convention: a direct-call test vouches for the component, not its binding. Two legs, and the control is the load-bearing one. An unknown field with an ordinary value must still be ACCEPTED, so this pins "refused for the NUL" rather than "refused for being unknown". The handler does not reject unknown fields; if it ever started to, the note's explanation would be wrong while its status code stayed right, and no status-code-only assertion could see that. The before-state is measured rather than asserted from memory. Disabling the check makes the same request answer 201 - which is main's behaviour, since decodeJSONWithLimit there unmarshals straight into the typed value and the key is dropped. So "answers 400 where it answered 200" is a measurement in both directions, not a recollection of one. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
…-2803, BUG-2813) Codex round 19, the fresh-angle deploy/rollback/mixed-version pass. docs/backup.md said a NUL-bearing row "can only affect data written before that rule existed, and only on SQLite". The second half is true. The first half is false, and the reason is the interesting part: the guard is in decodeJSONWithLimit, so the invariant is a property of the running BINARY, not of the database. On SQLite any window where an older binary serves the same database can still write one - a rollback after upgrading, a staged rollout with an old and a new instance sharing a database, a second older instance on the same file. The window closes, the guard returns, and the rows are already stored, behaving exactly like genuinely old ones. A rollback is an ordinary operational move, so this is not an exotic path. The doc now states the binary-version dependence, says which dialect is affected and why PostgreSQL is not (it refuses a NUL itself, at every version), and gives the operational answer: drain writes from older binaries before the new one serves, or roll forward rather than back. Store-layer enforcement - so the running build stops mattering - is filed as BUG-2813 rather than added here. It is a dialect-level change and the day-68 ruling on this unit is land-and-follow. The same false implication was carried by the PR's release note calling such a workspace "legacy"; corrected there too. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
…ounter (BUG-2803) "lead ruling day-68" is the internal day counter, which means nothing to anyone reading this repo and is inconsistent with every other citation in it - the codebase cites a lead ruling by DATE or by BUG ref, never by day-N. Replaced with the bug ref, which is the part a reader can actually follow. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
…d stop asserting a cause I borrowed (BUG-2803) Two defects in a comment I wrote an hour ago, both of the kind this unit's trail keeps recording. "an 18-commit branch" - the branch was 20 commits at b019287 when I counted it this session, and is more now. 18 came from the previous checkpoint's own miscount, which I had ALREADY identified and written up before I typed it again here. A number that arrives inside a sentence about something else does not feel like a claim, which is exactly why it survives. The count is incidental to the argument, so it is gone rather than corrected - a figure that has to be maintained to stay true is a liability in a doc comment. "this branch's one regression came from exactly that" - the ruling's reasoning, restated by me as a verified fact. The regression I know about came from wiring a fix off a reviewer-named sink list without reading the mechanism, which is adjacent to "restructuring late under review pressure" but is not the same mechanism, and I did not check whether it is the one the ruling meant. Now attributed to the ruling and stated as its reasoning, with the part I can defend - the review loop finding something in nearly every round indicates a design problem - carrying the argument. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
…ms wider than their evidence (BUG-2803) Codex round 20, asked for a POPULATION rather than a confirmation (CONVE-24). It returned a covered list AND four findings; this commit carries the two that belong to this unit plus the doc corrections. ## The door: MCP audit is a second reader, not a pass-through parseMCPRequestBody runs its OWN json.Unmarshal and binds the decoded method / params.name to mcp_audit_log.tool_name, TEXT NOT NULL. A six-character NUL escape therefore arrives as a real NUL: PostgreSQL refuses the audit INSERT with 22021 - the exact symptom this unit exists to remove - and SQLite stores an unprintable tool name. Nothing upstream catches it; the /mcp transport decodes the JSON-RPC envelope itself rather than through decodeJSON, so the body rule never sees the request. Measured before fixing: the decoded name reached the column intact. This unit's own completeness map had CERTIFIED that reader as safe, on the grounds that "decoding still happens in the MCP dispatcher". That is true and it does not bear on what this middleware persists - a correct description of a mechanism, with no question asked about what it does, sitting in the one artifact whose job is to say the population is covered. Corrected there too. Disposition is SANITISE, not refuse, following the User-Agent precedent from earlier in this unit, and the rule now lives in one extracted helper (sanitiseStoredText) with the reasoning attached: the body rule refuses because the caller asked to store that value; this serves metadata the SERVER elected to record, where failing the write would lose the audit row for precisely the request most worth auditing. Both caller-derived returns are cleaned inside parseMCPRequestBody, so both call sites - the ok path and the denied path - are covered at the choke point rather than at either caller. Both are tested: params.name AND the method path. Mutations un-sanitising each one compile and kill only their own leg. ## Three claims corrected, all wider than their evidence - "all 65 call sites" in server.go: measured 70. Removed rather than corrected, because the number has to be maintained to stay true and says nothing the sentence needs. - docs/backup.md said a NUL "cannot be stored in a text or JSON column" absolutely, two paragraphs above my own text explaining that SQLite accepts one. Now stated as what it is: an application rule Pad enforces on both dialects, which is exactly why it has to be enforced. - artifact_import.go said such a value "cannot be stored under any encoding this product supports". Refuses, not cannot - stating a policy as a capability tells the next reader SQLite enforces something it does not. ## Filed, not fixed BUG-2814 - guarded writes re-emit at-rest NULs (move/copy/restore/ fields-patch), propagating a legacy value to rows that never had one. Distinct from BUG-2813: that one is about writing a NUL while an old binary serves, this is the fixed binary SPREADING one already present. Both dissolve under the same store-layer enforcement, so they are filed to be designed together rather than patched at each of a long and moving list of re-emit sites. Declined: round 20 also reported the release-note assertions as unsupported. They live in the PR body, which a read-only sandbox cannot see - the claim is about the reviewer's visibility, not the diff. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects, not one. The body half of BUG-2782 (path) and BUG-2784 (query): a caller-supplied string reached a Postgres text parameter, Postgres refused it, and the handler answered 500 — the honest answer is 400. Found on the way: six handlers read request bodies with no size cap at all, and four sites truncated strings mid-rune, undoing validation that had already passed.
Why the transport rule could not simply be extended
ValidateQueryworks because a decoded query value is a substring of the raw query with ASCII substitutions: the bad byte in the raw text is the bad byte in the value. That fails for a JSON body — the reachable NUL arrives as a six-character escape, all ordinary ASCII — so no request middleware can find it without decoding the body, which is the handler's job.The check
At
decodeJSONWithLimit, before unmarshalling into the target: read the body, walk it, refuse a decoded NUL with a 400. No call site changed for the refusal — all 65decodeJSONcallers already turn a decode error into a 400.The gate. The walk runs only when the raw bytes contain
\u00, the prefix of any\uescape for a character below U+0100. Each character of the NUL escape arrives either literally (so the raw contains the escape, which starts with that prefix) or from a\uescape of its own — and backslash,uand0all sit below U+0100.That argument is instrumented, not trusted. An earlier version ("the escape has only one spelling") was true of a decoded string, false of raw bytes, and round 4 turned it into a live bypass:
\^@contains no literal escape yet decodes into one. A differential test runs the gated function against an ungated walk over a corpus built to attack it, and asserts the corpus is not one-sided. It has since caught a second unsound gate — see What did not ship.The walk decodes into
anyand recurses. It refuses a NUL in any string or object key. Under a REQUEST key whose string value is a JSON document something re-parses (config, events, fields, metadata, phase_data, plan_overrides, schema, settings, tags, traits) it walks that document too — the escape survives the outer decode as literal text and becomes a NUL on the second parse, which Postgres reports differently: 22P05, not the 22021 the rest of the family produces.Four things it deliberately does NOT do, each after a round found it refusing something legitimate:
[]byteafter base64 decoding, so legitimate binary containing0x00would be refused.tagsandfieldsalso arrive as arrays/objects the server marshals itself. Only a direct string child of a listed key is a document.schema; the key list is consulted only outside caller data, so a user field is never mistaken for a wire key.fieldsis parsed once and the inner text is re-escaped when the blob is written. Those last two make the descent exactly one level deep by construction, which is why there is no depth counter.TestJSONEncodedFieldKeysCoversTheModelsderives the key set frominternal/modelsand fails when a new one appears.Population — measured, with a control leg on every endpoint
92 mutating routes enumerated via
chi.Walk; 13 probed on Postgres 17 through the real router.workspace-token.nameis UNMEASURED, not clean — its control also 500s, on an unrelated FK. The other 79 routes are unprobed, not claimed clean; completeness is carried structurally instead, byTestEveryRequestBodyReaderIsAccountedFor, which requires every file touching a request body to be listed with a reason and fails in both directions.An earlier run said 9 of 13 — the control PATCH renamed the shared fixture item, so later probes 404'd and read as non-doors. A fixture artifact producing a negative result is exactly the shape this filing was re-scoped for.
Doors closed beyond the JSON body
pad-export.jsonand the attachment manifest itself. A NUL in the manifest reachedrehydrateAttachment, whose failure is logged and skipped, so the import reported success while dropping the attachment.title: "a\0b"carries no NUL in the request bytes and manufactures one on decode, importing 201 with a NUL in the item title. Both the raw bytes and the decoded artifact are now checked.item_idreaches ResolveItem exactly as the query channel does. (A raw NUL in the header is not the vector — Go refuses it as a malformed MIME header; the RFC 5987 encoded form is.)User-Agentintoactivities.user_agent. Sanitised, not refused — a header is metadata the server chose to record.Second defect: six unbounded body reads
Those six handlers had no
http.MaxBytesReader— the 2 MiB capdecodeJSONhas always applied. Each was an unbounded body read: a multi-GB POST to comment-create, item-move, report-layout, attachment-transform, comment-reactions or OAuth DCR streamed into a single allocation. A memory-exhaustion door independent of the NUL bug, closed here because it lives on the same six lines.Release note — measured doors closed, with filed residuals
Scope statement first, because "closed" unqualified would be wrong: this closes every door the
population sweep MEASURED, and leaves four named residuals filed rather than fixed (see Known,
filed, NOT fixed here). Two of those are known ways a NUL can still reach a text column.
Behaviour changes an operator or client author would notice:
invalid_body.pad-export.jsonor attachment manifests. A manifest refusal happens after the workspace row exists, so it leaves a partially created workspace, as other manifest failures already did.item_idand filename values that cannot be stored are dropped rather than passed through — an unusableitem_idbehaves as if absent, an unusable filename becomes a generic one.User-Agentvalues are sanitised (invalid UTF-8 and NULs removed) before reaching the activity log. Session UA hashing is unchanged.docs/backup.md;pad db migrate-to-pghas the same limitation and reports it later. Tracked as BUG-2810. Note therule lives in the BINARY, not the database, so this is not only about historical data — a rollback
or a mixed-version window on SQLite can still create such rows (BUG-2813). PostgreSQL is unaffected
at every version; it refuses a NUL itself.
{"title":"valid","future_field":"<NUL escape>"}answers 400 where it answered 200. Thecheck has no destination type by design (see Four things it deliberately does NOT do), so it
cannot tell a forward-compatible field from a real one. Accepted: refusing is the safe
direction. A client sending forward-compatible fields is the one affected.
Known, filed, NOT fixed here
migrate-to-pgfails mid-copy. Needs a preflight and a repair path; the disposition is a product ruling.mixed-version window can still write a NUL; the guard returns and the rows are already stored. Found by
round 19's deploy/rollback pass, which also caught
docs/backup.mdclaiming such rows "can only affectdata written before that rule existed" — false for exactly that reason. The DOC half is fixed in this PR;
store-layer enforcement is the filed follow-up, since it is a dialect-level change and the day-68 ruling
is land-and-follow.
map[string]anymodel disagreements (rounds 16-17), all documented onbodyDecodesNULand pinned byTestBodyDecodesNULKnownMapModelDisagreements. One root cause:this scan decodes into
map[string]anyandencoding/json's typed decode does not agree withthat model about keys. Two under-refuse and are BUG-2812's spec — duplicate keys merge
differently (the map REPLACES, the decode MERGES, so the scan cannot see the shadowed
occurrence), and a scan failure lets a known-bad value through (
1e999breaks theanyunmarshal, the typed decode then skips the unknown field and accepts the NUL title). Two
over-refuse and are ACCEPTED — unknown fields (release note 10) and case-variant duplicates.
Per the day-68 ruling these are NOT patched here: 17 finding-rounds in 19 says the pre-scan
machinery has a design problem, and restructuring it late under review pressure is what produced
this branch's one regression. The two known-gap test legs assert the WRONG answer on purpose, so
BUG-2812's fix makes them FAIL rather than letting this note go stale.
What did not ship, and why it is worth knowing
An escape-parity pre-filter would have killed BUG-2812's trigger without a walk: a backslash introduces an escape only when an even number precede it, so a doubled backslash could be skipped. It is unsound, and the existing tests failed the moment it landed — parity is layer-relative, and in a JSON-encoded field the escape carries one more backslash than it will have after the outer decode, so "even, therefore literal" at the raw layer becomes a live escape one decode later. The substring gate is the minimal sound filter.
Verification
ContainsRune(body, 0)passed with the fix disabled.go test ./...28 ok / 0 FAIL · Postgres 17 full suite 28 ok / 0 FAIL (own container, not the shared port) ·-raceoninternal/serverok ·golangci-lint0 issues ·gofmt0 files ·govulncheck0 called.Closes BUG-2803.