fix(server): refuse invalid-UTF-8 and NUL query values at the transport (BUG-2784) - #1217
Merged
Conversation
…g beside it (BUG-2784) Pure rename, no content change. BUG-2784 adds a query-string rule that shares this file's predicate and its justification, and a file named for the path half would then describe half of what it holds. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…rt (BUG-2784) The query-string half of BUG-2782. A caller-supplied query value reached a Postgres text comparison, Postgres refused the parameter (SQLSTATE 22021), and the handler answered 500 — the honest answer is 400, because the caller asked about something that cannot exist. Measured on Postgres 17 at 1933041, before the fix: 8 GET endpoints x 54 parameter names (every name any handler reads), one request per pair, each from its own source IP because the api limiter is keyed on ip:. invalid-UTF-8 value: 432 probes -> 276 x 200, 56 x 400, 100 x 500 NUL value: 432 probes -> 276 x 200, 56 x 400, 100 x 500 control: 432 probes -> 376 x 200, 56 x 400, 0 x 500 Zero 500s in the control, so the 100 are attributable to the value. After the fix the two bad legs are 432 x 400 and the control is unchanged. WHY A TRANSPORT RULE, when BUG-2782 said the query string would be handled at its points of use on BUG-2774's validCursorID model. Reading the mechanism retired that plan: parseItemListParams folds every parameter it does not recognise into a field filter, so ?email=, ?type= and ?anything-at-all= reach a text comparison exactly as ?search= does. 98 of the 100 failures are those two endpoints. The set of names is unbounded by design, so there is no finite list of points to validate. WHY IT IS NOT A NARROWING of what callers may send, which was BUG-2782's objection to validating the query at the transport. That objection is sound against a charset rule and does not reach this one: bindableText requires only valid UTF-8 with no NUL, and every legitimate value here is text, which is valid UTF-8 in any language. A byte sequence that is not valid UTF-8 is not text that got narrowed; it is not text at all. Keys are validated PRECAUTIONARILY. The same sweep drove an invalid-UTF-8 parameter NAME at all eight endpoints and got 7 x 200 and 1 x 400 — no 500. Why a bad key survives where a bad value does not is unread, so "keys are safe" is a claim nothing supports; a sweep that did not reproduce is not a proof of absence, and checking them costs nothing. One existing expectation changes. The timeline's before_id guard answered invalid_cursor for these same two byte classes; ValidateQuery now answers invalid_query first. The 400 and the client-error contract are unchanged, the code is less specific, and that is the cost of covering a surface no per-site validator can. validCursorID itself is NOT dead: two of its three call sites apply it to ids read from the item's own fields blob, which no request middleware can see. Both facts are written where someone would otherwise have to rediscover them. Mutation matrix, 14 mutations, 14 detected: unwiring the middleware (fails the Postgres leg with the original 500, verified by running it), each half of bindableText, the raw-byte check, the fast path, keys-only, values-only, first-pair-only, the error code, the status, the CORS decorate, rejecting everything, and swapping the two middlewares' order. Filed separately rather than folded in: form-encoded POST bodies reach r.Form in the OAuth handlers and are a different door, still unmeasured. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Each builds its own testServer(t) — per-test SQLite in t.TempDir, own limiters and bus — so the isolation precondition holds. The Postgres leg is left serial, matching its sibling TestValidatePathPostgresNoInternalError. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Four findings, all real, none a runtime bypass or an over-rejection. 1. The Unicode-delivery test was VACUOUS. With one item in the workspace, a handler that ignored `search` — or a middleware that stripped the query — returned that item and passed. Now creates a decoy that must not match, asserts the EXACT result set, and asserts its own premise (the unfiltered list returns both, or the decoy discriminates nothing). Counterfactual run: with `Search` forced empty in parseItemListParams the test fails on every leg; before this change it passed. 2. The fast-path comment's proof was wrong where its conclusion was right. It named the '+' → ' ' substitution and then claimed every decoded value is a SUBSTRING of the raw query, which that substitution makes false. Rewritten as what actually holds: a substring with ASCII '+' swapped for ASCII ' ', which preserves both UTF-8 validity and NUL-freedom. 3. The "validating what survived is validating the reachable set" claim understated the code. The raw check runs BEFORE decoding, so a bad byte inside a pair url.ParseQuery drops still rejects the request even though no handler could see it. Documented as deliberate — it is the only check that sees a raw unescaped 0xff, and it can only fire on an already malformed request — and pinned with two table cases. 4. A doc comment still named validPathText after the rename to bindableText. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
Round 2 verified the round-1 fixes CLEAN and returned three P2 comment
findings, all of them prose claiming more than the code does.
1. "the only check that sees a raw unescaped 0xff" was too strong: a raw
byte in a pair that SURVIVES parsing also reaches bindableText through
the decode loop. The raw check is the only one for DROPPED pairs and for
the fast path, which is what the comment now says.
2. The fast-path description omitted that an unescaped ';' makes ParseQuery
error and drop pairs. Added, with why it needs no handling: a subset of
an already-checked set is still checked.
3. "under SQL_ASCII it would accept the same bytes" was false for half the
rule, and the error predates this branch — it came from validCursorID,
was inherited by BUG-2782's comment, and is now corrected in both. The
two classes diverge:
invalid UTF-8 -> refused under UTF8, ACCEPTED under SQL_ASCII
NUL -> refused under BOTH
Measured rather than taken from the review: on a SQL_ASCII database,
SELECT length(E'bad-\xff-x') returns 7 while SELECT length(E'bad-\x00-x')
errors `invalid byte sequence for encoding "SQL_ASCII": 0x00`. SQL_ASCII
relaxes the encoding check, not the NUL rule.
The enumeration half of round 2 found no remaining way for a path or query
byte to reach a text parameter without passing bindableText, and no
legitimate Pad request newly refused. What it did surface is a door on a
different surface entirely — an escaped NUL in a JSON body — which
falsified a claim in BUG-2803 and is corrected there, not here.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
… (BUG-2784)
Codex round 3 blocked the merge on four comment claims broader than what
was measured. No runtime finding. All four narrowed, and one of them
corrected in a different direction than the review proposed.
1. Two sentences said "Postgres refuses a text parameter that is not valid
UTF-8" with no qualification, which the SQL_ASCII row of bindableText's
own table contradicts. Both now say "under a UTF8 database encoding",
with the NUL half kept unconditional, which is what the table shows.
2. The review attributed that to a permissive client_encoding. MEASURED,
and it is not — the DATABASE encoding decides, both ways:
SQL_ASCII db, client_encoding=SQL_ASCII -> accepted (length 7)
SQL_ASCII db, client_encoding=UTF8 -> accepted (length 7)
UTF8 db, client_encoding=SQL_ASCII -> ERROR 0xff
The finding was right that the sentences were unqualified; its stated
mechanism was not. Both the table and that distinction are now in the
comment, so the next reader inherits the measurement and not the guess.
What Pad's own connections declare was measured too rather than assumed:
SHOW client_encoding over this store's pgx pool reports UTF8.
3. "rejects exactly what Postgres refuses" was false in two directions the
file documents elsewhere — it also refuses invalid UTF-8 a SQL_ASCII
database would accept, and bytes in query pairs ParseQuery discards.
Replaced with the claim that actually carries the design: it refuses
nothing a client can legitimately send.
4. "UTF8 is initdb's default" is locale-dependent. Now says so, and says
the measurements were taken at postgres:17-alpine's image defaults.
Also corrected in the same pass: the round-2 wording for the raw check
overshot in the other direction, claiming everything surviving parsing
reaches the decode loop, which is false on the no-'%' fast path where the
loop never runs. Both halves now name the path they apply to.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
… them (BUG-2784) Round 4 blocked on two claims. Both closed, and the second by RETRACTING an argument rather than winning it. 1. A second "UTF8 is initdb's default" survived at line 35 — I fixed the instance the previous round NAMED and did not grep the class, which is the exact failure CONVE-18 exists for. Grepped now; two sites, both qualified, and the re-grep is in the commit that follows the fix. 2. Round 4 said "the database encoding, not the connection's" is too absolute, because a connection declaring a single-byte encoding could convert 0xff rather than refuse it. Checking that killed my own counter-measurement: the table was produced with E'...' escapes, which the SERVER expands, so the byte never crosses the wire and those rows say nothing about client_encoding conversion in either direction. My round-3 rebuttal tested something other than what it claimed. So the "what governs" argument is gone rather than re-argued. The table now states what it actually covers — what a given DATABASE encoding does with a byte that already exists server-side, for the encodings tested — and says plainly that it is not a general account of Postgres. Also retired the same overreach at line 18, where the NUL row said "under any encoding" on the strength of two. Dropping it costs nothing, which is why it goes: the middleware's correctness never depended on which layer decides. The rule is uniform across every one of those configurations, and that uniformity is the property that makes the deployment's encoding stop mattering. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…is false (BUG-2784)
Round 5's only finding, and it is a false test contract rather than style,
so it blocks under the stopping rule the lead ratified.
TestValidQueryTextFastPathAgreesWithTheSlowPath asserted that the '%' fast
path can NEVER disagree with decoding every pair and checking each one.
That is false, verified rather than conceded:
raw="ignored=\xff;bad" validQueryText=false per-pair=true
ParseQuery err=invalid semicolon separator
url.ParseQuery refuses an unescaped ';' and drops EVERY pair, so the
per-pair comparator has nothing left to object to and returns true, while
the raw check rejects. The old corpus contained no semicolon, so a false
contract passed for want of one character.
The code is not wrong — the extra strictness is the behaviour documented
at validQueryText, and it is the safe direction. The TEST was wrong, and
in the way that matters most: it named an invariant the code does not have.
Reframed to the property that is actually true and actually load-bearing:
the fast path is never MORE PERMISSIVE than the per-pair check. It may not
let through anything the decode path would reject; it may reject more. The
semicolon cases are now in the corpus, so the input that falsified the old
contract is what exercises the new one.
Two guards against this test going hollow again:
- A premise assertion. If no corpus entry exercises the stricter
direction, the test FAILS rather than passing on a vacuous
one-directional check — which is the exact state the old version was
in without saying so.
- Counterfactual run: hoisting the fast path above the raw check makes
it fail on three separate inputs with "fast path is MORE PERMISSIVE".
It passes on the real code and fails on a permissive one.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…nce count (BUG-2784) Round 6 returned MERGE for the implementation and two findings, both about statements rather than behaviour. 1. handlers_timeline_cursor_validation_test.go's header claimed Postgres refuses invalid UTF-8, unqualified — which this branch's own comments now contradict, so CONVE-23 makes it this unit's to fix even though it predates the branch. Qualified to the UTF8 database encoding, with the NUL half left unconditional because that is what was measured. The review also read the store-level half as able to FAIL on a SQL_ASCII deployment. Checked, and it cannot: storetest.NewPostgres issues a bare CREATE DATABASE, which inherits TEMPLATE1's encoding and not that of the database PAD_TEST_POSTGRES_URL names, so the fixture returns a UTF8 database even when pointed at a SQL_ASCII one. Measured directly — creating a database from inside the SQL_ASCII database yields UTF8. The subtest now READS server_encoding and skips with an explanation rather than assuming it. That guard cannot fire under today's fixture, and the comment says so plainly rather than implying coverage it does not have; it earns its place by making a future fixture change report "premise does not apply" instead of an assertion failure about drivers. 2. The previous commit message said the counterfactual failed on THREE inputs. It fails on FOUR. I had read a `head -4`-truncated output and reported its line count as the population — a figure taken from a pipeline that cut it, which is the whole reason gate output should not be piped. Recounted with `grep -c`: four. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…e claim (BUG-2784)
Round 7: MERGE verdict, three statement-level findings. Two were further
instances of the qualification class rounds 3, 4 and 6 each found one of —
the third time this unit that fixing named sites did not fix the class.
Swept properly this time: grepped every "Postgres refuses/rejects" claim in
internal/ and cmd/ and checked each against the measured table. Sixteen
matches, thirteen about JSONB/empty-string/empty-list behaviour and out of
scope; the three encoding claims are the two below plus one already fixed.
- handlers_timeline_cursor_validation_test.go:54 said %FF and %00 are
both "what Postgres refuses". Now separates them (the NUL under every
encoding tested, the %FF only under UTF8) and adds the fact that makes
the table's expectations sound regardless: since BUG-2784 the transport
refuses both before any backend is consulted, which is why these hold
on SQLite too.
- handlers_timeline.go:89 said Postgres rejects the old "\xff" sentinel,
unqualified. Qualified, with the part the qualification must not
obscure: the synthesized sentinel was wrong on ANY encoding, and the
500 is only what a UTF8 database turned it into.
The third finding was a genuine scope error of mine, not a missing
qualifier. validCursorID's comment said its blob-derived call sites read
data that arrives "never from the request". False: a request BODY writes
that blob, and BUG-2803 — filed from this branch — is precisely an escaped
NUL reaching the store that way. What those call sites are actually beyond
is the reach of THIS middleware, which reads the path and query and never
a body. Corrected to say that, since the sentence was load-bearing for why
the function survives and a reader could have taken it as proof the blob is
unreachable by clients.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
… my own sweep claim (BUG-2784)
internal/store/comments.go:263 said Postgres rejects the old "\xff" cursor
sentinel as invalid UTF-8, unqualified. A SQL_ASCII database accepts it.
Qualified, with the point the qualifier must carry: the sentinel was not
universally fatal — fatal on the encoding most deployments run, silently
fine on the other, which is what makes a bug of this shape look
unreproducible.
THE PREVIOUS COMMIT'S SWEEP CLAIM WAS FALSE and is corrected here. It said
sixteen matches, "thirteen about JSONB/empty-string/empty-list behaviour and
out of scope; the three encoding claims are the two below plus one already
fixed." There were FOUR encoding claims. This line was in my own grep output
and I classified it as JSONB by reading the file name instead of the line.
So the commit that announced a class sweep did not complete the class — the
fourth round in a row this class evaded a fix, and the first time it did so
while I was asserting the opposite. A sweep claim is a measurement claim and
deserved the same standard as a figure: state what was checked and how the
classification was made, not that it was done.
Re-swept on the markers that actually identify the class rather than one
phrasing of it — "invalid UTF-8", "invalid byte sequence", "22021", "22P05",
"not valid UTF-8" — 18 non-test lines across 6 files, each read rather than
inferred from its path:
- 13 in middleware_request_text.go / handlers_timeline.go: qualified or
the measured table itself.
- 2 in comments.go: this fix.
- 1 in server.go: describes the middleware's own RULE, not Postgres.
- 2 in wiki_links.go: about a truncation not splitting a rune in a Go
string; no database claim.
Nothing about Postgres refusing invalid UTF-8 is now unqualified.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
…BUG-2784)
ValidatePath's comment said a path whose decoded form is valid UTF-8 is left
for the router unchanged. A NUL is valid UTF-8 and is rejected, so the
sentence was false for exactly the inputs the second half of bindableText
exists to catch.
Written by me in BUG-2782, forty lines above a reject message that has
always said the opposite correctly — "Not 'is not valid UTF-8': a NUL is
valid UTF-8 and is rejected here too, so that wording would be false for
half the inputs this refuses." I wrote that note to avoid this exact error
and then made it in the same file.
MARKER LESSON, which is the reusable part. Round 8's class sweep passed
over this because I swept the marker set "Postgres refuses/rejects …",
which identifies claims about the DATABASE. This is a claim about what the
RULE PERMITS — same subject, different vocabulary, and no overlap with that
grep. The review named the marker that would have caught it: "valid UTF-8"
or "NUL". Swept on those: 12 non-test-and-test lines across 5 files, each
read rather than pattern-matched.
- 1 false, this one.
- 3 elsewhere in this file: the emoji case, the "text is valid UTF-8 in
any language" implication, and the fast-path reasoning — all sound.
- handlers_timeline.go: the "g" sentinel note and the before_id error
message, which already names both halves.
- the test files and internal/mcp: all already spell out that a NUL is
valid UTF-8, which is why they never drifted.
A class has more than one vocabulary. Sweeping the phrasing I happened to
have used finds the instances I happened to have worded the same way.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
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.
Closes BUG-2784 — the query-string half of BUG-2782.
A caller-supplied query value reached a Postgres text comparison, Postgres refused the parameter, and the handler answered 500. The honest answer is 400: the caller asked about something that cannot exist.
Measured
8 GET endpoints × 54 parameter names — every name any handler reads — one request per pair, each from its own source IP because the api limiter is keyed on
ip:and a single-address sweep answers 429 to everything and measures the limiter instead of the defect. Postgres 17,server_encodingUTF8, at 1933041.ordinary-value)Zero 500s in the control, so the 100 are attributable to the value.
Why a transport rule, when BUG-2782 said points of use
BUG-2782 scoped itself to the path and planned the query for per-site validators on BUG-2774's
validCursorIDmodel. Reading the mechanism retired that plan.parseItemListParamsfolds every parameter it does not recognise into a field filter, so?email=,?type=and?anything-at-all=reach a text comparison exactly as?search=does — 98 of the 100 failures are those two endpoints. The set of names is unbounded by design, so there is no finite list of points to validate.BUG-2782's objection to a transport rule was that query values have a much wider legitimate domain than paths. True of a charset rule, and it does not reach this one.
bindableTextrequires only valid UTF-8 with no NUL; every legitimate value here is text, and text is valid UTF-8 in any language. A byte sequence that is not valid UTF-8 is not text that got narrowed — it is not text at all.Contract change, called out rather than buried
The timeline's
before_idguard answeredinvalid_cursorfor these two byte classes;ValidateQuerynow answersinvalid_queryfirst. Same 400, same client-error contract, less specific code — the accepted cost of covering a surface no per-site validator can.validCursorIDis not dead. Two of its three call sites apply it to structured-entry ids read from the item's own fields blob. That fact is recorded at the function definition, where someone would land before deleting it.Keys are precautionary
The sweep drove an invalid-UTF-8 parameter name at all eight endpoints: 7×200, 1×400, no 500. Why a bad key survives where a bad value does not is unread, so "keys are safe" is a claim nothing supports — a sweep that did not reproduce is not a proof of absence. They are checked anyway; it costs nothing.
Gates
go test ./...(SQLite) — pass-timeout=45m, on a private container (not the shared 5445, per the multi-seat rule) — 28 packages ok, 0 failgo test -race ./internal/server/— passmake lint— 0 issues ·make vuln— clean ·gofmt— cleanMutation matrix: 14 mutations, 14 detected. Unwiring the middleware;
validQueryTextalways true; each half ofbindableTextseparately; the raw-byte check; the fast path; keys-only; values-only; first-pair-only; the error code; the status; the CORS decorate; rejecting everything (caught by the control legs, which is what proves the controls are not decorative); and swapping the two middlewares' order.Unwiring was additionally run against the Postgres leg to verify the claim written in the code comment — it fails with the original defect:
Review history
Nine adversarial rounds. The first two found defects in code and tests; the rest found false statements, and the last of those was the sharpest.
searchreturned it and passed. Now creates a decoy, asserts the exact result set, and asserts its own premise. Counterfactual: forcingSearchempty fails every leg; before the fix it passed.E'…'escapes the server expands, so it never crossed the wire and could not have tested what I claimed. The disputed argument was deleted rather than re-argued — the middleware behaves identically either way.%-free fast path and the per-pair check. False:ignored=\xff;baddisagrees, becauseParseQuerydrops every pair on an unescaped;. The corpus contained no semicolon, so a false contract passed. Reframed to the true one-directional property (never more permissive), with a premise guard that fails if the corpus stops exercising it.Found by the enumeration, filed not folded
BUG-2803 — an escaped NUL in a JSON body reaches the store and 500s on every JSON write path. It falsified a claim in my own earlier filing (I had measured a raw NUL, which is malformed JSON, so the parser rejected the probe rather than the door being closed). Different surface, different mechanism — decoded JSON strings are invisible at the transport layer — so it is not a rider on this PR. Pulled into the release by Dave; queued after BUG-2793.
Release note: Invalid UTF-8 or NUL bytes in query parameters now return 400 instead of 500 on Postgres deployments.