Files
pad/internal/server/middleware_request_text.go
T
xarmian ef792bdf35 fix(store,server,textguard): close codex round 1 on S1 — seven findings
All seven verified before acting; two were production regressions I had
introduced and one was a defect older than this branch.

[High] The wrapper HID the optional connection interfaces. Measured: the raw
modernc conn implements Pinger, SessionResetter and Validator; the wrapped one
implemented none, so Ping succeeded without pinging, pooled connections stopped
being reset, and dead connections stayed in the pool. Each is now forwarded,
falling back to exactly what database/sql does for a conn lacking it.
NamedValueChecker is forwarded too, or the wrapper silently narrowed the
argument types pgx accepts.

[High] Statement-level driver.ErrSkip was wrong. At the CONN level ErrSkip is
the documented fallback signal; at the STATEMENT level database/sql propagates
it, so a base statement without the context interface would have failed rather
than degraded. Both now fall back to the positional form, refusing a NAMED
argument rather than binding it by position.

[High] Scalar JSON documents bypassed the check — and the database settles it:

	SELECT ('"a<escape>b"')::jsonb;
	ERROR:  unsupported Unicode escape sequence

A bare JSON string is a complete jsonb document. The object/array-only shape
test is right for the HTTP gate's "is this a nested document" question and
wrong for the store's "will a jsonb parser read this". Both now use a widened
form, so the gate refuses it at the door instead of leaving it to the store.

That widening broke the gate's independent oracle, which had been NARROWED in
BUG-2803 round 27 to make the two walkers agree. The disagreement was real; the
direction of the fix was not — nobody measured which one matched Postgres, and
it was the oracle. Restored, with the measurement in the comment. Two
implementations made to agree are not thereby correct.

[High] The 400 mapping was not the single funnel I claimed. createItemChecked,
bulk ops and the cross-workspace copy carry their own error envelopes by
design. One shared CLASSIFIER now serves all of them; the envelopes stay
different, the classification and wording do not. Copy keeps its
retry-discouraging message deliberately (PLAN-2357 DR-13).

[Medium] The over-refusal justification was FALSE as written. It claimed a
JSON-shaped text value carrying a live escape "is a value Postgres would refuse
the moment anything parsed it". Nothing parses a text column; Postgres stores
it fine. The trade is now stated honestly, and pinned: textguard.
StoreOverRefusals records the case, and a test fails when it is paid down.

[Medium] Duplicate JSON keys are a real under-refusal, inherited from the
shared predicate's map model. DOC-2823 requires Layer A NOT to fix it alone, so
it is recorded in textguard.KnownGaps with a test that fails when it CLOSES —
the signal that BUG-2812's token-walk landed.

[Medium] Test weaknesses, and the fixes caught a real one. The census now sees
ALTER TABLE ... ADD COLUMN. The store corpus leg now requires accepted cases to
SUCCEED, which immediately exposed a case passing for the wrong reason: a
non-JSON value written to the fields column failed on SQLite's own JSON parser,
never reaching the guard.

Full Go suite green on SQLite and Postgres 17; lint 0 issues.

Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm
2026-09-01 12:54:19 +00:00

804 lines
41 KiB
Go
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
package server
import (
"bytes"
"encoding/json"
"io"
"net/http"
"net/url"
"strings"
"unicode/utf8"
"github.com/PerpetualSoftware/pad/internal/textguard"
)
// ValidatePath rejects a request whose percent-DECODED URL path is not a
// value the database can be asked about.
//
// Every handler that resolves a workspace, collection, item, comment or
// attachment from a path segment hands that segment to the store verbatim,
// and the store binds it into a text comparison. Postgres refuses a text
// parameter that is not valid UTF-8 — under a UTF8 database encoding; see
// bindableText for the encoding table and what governs it — or that
// contains a NUL, under every encoding tested (SQLSTATE 22021 / 22P05), and the
// driver surfaces that as a query error. MOST handlers
// turn that into a 500, which is the defect; not all do, and the claim is
// deliberately not universal — handlers that collapse a resolution error
// into not-found already answer 404 (see handlers_timeline.go's
// `err != nil || item == nil`). The measurement below is what the
// distribution actually was.
//
// SQLite accepts both byte classes and simply matches nothing, so a request
// that reaches a store resolution answers 404 there instead. Not every
// request does, on either backend — an authorization or configuration gate
// that answers first keeps its own status, which is why the GET-only sweep
// below found 102 × 404 but also 5 × 403, 2 × 200, 1 × 401 and 1 × 503 on
// SQLite. The divergence is in what happens once the value REACHES the
// store, and it splits by BACKEND rather than by deployment: a SQLite
// install never sees the 500, and a Postgres install whose database
// encoding is UTF8 does — Pad Cloud and a self-hoster on Postgres alike,
// since a UTF8 database is the common self-host and Cloud case — though
// NOT because initdb always picks it, which is locale-dependent. (Under
// SQL_ASCII, Postgres accepts the invalid-UTF-8 bytes — but not a NUL; see
// bindableText below, where that split is measured and its scope bounded.)
// An operator's alerting reads it as the server breaking when a client
// sent a path that cannot name anything.
//
// Measured before this middleware existed, driving every route that carries
// a path parameter with one segment set to "bad-%FF-x" — one request per
// parameter position, real values in the other positions. GET routes alone:
// 111 probes, Postgres answered 500 to 94, SQLite to 0. All methods: 247
// probes, Postgres 191, SQLite 0. (Both figures appear in this branch's
// history; they are the same sweep at two widths, not a disagreement.) This
// is a cross-cutting input rule, not a per-handler bug, which is why it
// lives here rather than at ~112 `chi.URLParam` call sites that each have
// to remember.
//
// WHY THE DECODED PATH, not what chi hands the handler. chi routes on
// r.URL.RawPath when it is non-empty and on r.URL.Path otherwise, and Go
// populates RawPath only when the client's escaping is NOT already
// canonical. Go escapes 0xff as uppercase "%FF", so:
//
// /…/items/bad-%FF-x → RawPath empty → chi routes on Path → URLParam
// yields the raw 0xff byte → reaches the store
// /…/items/bad-%ff-x → RawPath set → chi routes on RawPath → URLParam
// yields the literal text "bad-%ff-x" → harmless
//
// So the reachable vector is the CANONICAL uppercase form any ordinary
// client or proxy emits, and the harmless one is the oddity. Validating
// r.URL.Path answers both the same way, removes a behavioural difference
// that hangs on hex-digit case, and does not depend on chi continuing to
// prefer RawPath.
//
// It cannot refuse Pad's own URLs — but NOT because they are all ASCII,
// which was this comment's first claim and is false. Most are: slugs come
// from store.slugify, which appends only [a-z0-9-]; ids are UUIDs or hex;
// issue refs are a collection prefix plus digits. The exception is the one
// that matters, because it is the case a careless rule would break:
// DELETE /comments/{commentID}/reactions/{emoji} carries an EMOJI, which
// the web client sends as encodeURIComponent(emoji). That is valid UTF-8
// and must keep working, which is exactly what the rule permits and what
// TestValidatePathAllowsValidText pins with an emoji segment.
//
// A path whose decoded form is valid UTF-8 AND CARRIES NO NUL — including
// non-ASCII — is left for the router to handle, unchanged by this
// middleware. The NUL half of that sentence is not redundant and was
// missing until BUG-2784's ninth review round: a NUL IS valid UTF-8, so
// "valid UTF-8 passes" is false for exactly the inputs the second half of
// bindableText exists to catch. The reject message a few lines below says
// the same thing in the other direction and has said so since BUG-2782 —
// this sentence simply did not match it. That is a
// statement about THIS middleware only, and not a promise that the handler
// sees the decoded text: by the RawPath rule above, "caf%C3%A9" reaches
// URLParam as "café" while the non-canonical "caf%c3%a9" reaches it as the
// literal text "caf%c3%a9", and an escaped "%2F" never becomes a separator.
// That is chi's pre-existing behaviour, unaffected either way by this
// change; it is written down here because the obvious reading of "passes
// through" is stronger than what is true.
//
// 400 rather than 404: not because the URI is malformed — "%FF" and "%00"
// are syntactically valid percent-encoded octets — but because the decoded
// value cannot be a resource identifier in this system at all. The answer
// does not depend on whether anything exists, so it is not an existence
// oracle. Scope is the PATH only; the query string is the separate rule
// ValidateQuery below applies, on the same terms and for the same reason
// (BUG-2784). This comment previously said the query string was validated
// at its points of use on BUG-2774's validCursorID model; that was the
// plan, and measuring the query surface retired it — see ValidateQuery's
// comment for why a per-site validator cannot be written there.
//
// The rejection must not be shaped differently from the errors the API
// writes for itself. Because this runs at the root, it short-circuits
// BEFORE the /api/v1 group's cors.Handler and jsonContentType, so a naive
// implementation answers with a JSON body typed text/plain and no CORS
// headers — which on a cross-origin deployment (PAD_CORS_ORIGINS set) the
// browser refuses to let the page read at all, turning a debuggable 400
// into an opaque network error. Hence `decorate`: setupRouter passes the
// SAME cors.Handler instance the group uses, and the rejection is served
// through it, so there is one CORS configuration and not two.
//
// BUG-2782.
func ValidatePath(decorate func(http.Handler) http.Handler) func(http.Handler) http.Handler {
var reject http.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Set explicitly rather than relying on jsonContentType, which is
// mounted below this middleware and never runs for a rejection.
// Without it net/http sniffs the JSON body as text/plain.
w.Header().Set("Content-Type", "application/json")
// 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.
writeError(w, http.StatusBadRequest, "invalid_path",
"Request path contains invalid UTF-8 or a NUL byte")
})
if decorate != nil {
reject = decorate(reject)
}
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if !bindableText(r.URL.Path) {
reject.ServeHTTP(w, r)
return
}
next.ServeHTTP(w, r)
})
}
}
// bindableText reports whether a decoded string can be bound into a text
// comparison. It is the shared predicate of both middlewares in this file:
// ValidatePath applies it to the decoded path, ValidateQuery to each
// decoded query key and value. The rule is derived from what the database
// refuses rather than from what a path or a parameter "should" look like —
// no length bound, no character allow-list — for the reason validCursorID
// records: a bound that can only fire on a legitimate value is not
// protection.
//
// "What the database refuses" is narrower than it sounds, and the phrasing
// inherited from validCursorID overstated it. The two classes do NOT behave
// alike across encodings, which an earlier version of this comment (and
// validCursorID's) got wrong by treating them as one case:
//
// UTF8 database SQL_ASCII database
// invalid UTF-8 byte refused (22021) ACCEPTED
// NUL byte refused (22021) refused (22021)
//
// Measured on postgres:17-alpine against a `CREATE DATABASE … ENCODING
// 'SQL_ASCII' TEMPLATE template0`: `SELECT length(E'bad-\xff-x')` returns 7,
// while `SELECT length(E'bad-\x00-x')` errors with `invalid byte sequence
// for encoding "SQL_ASCII": 0x00`. So SQL_ASCII relaxes the encoding check,
// not the NUL rule — a NUL terminates a C string and no encoding makes it
// storable in a text column.
//
// THE SCOPE OF THAT TABLE, stated because two review rounds pushed on it
// from opposite directions and the honest answer is narrower than either.
// Those rows were produced with `E'…'` escapes, which the SERVER expands —
// the byte never crosses the wire as itself. So they establish what a given
// DATABASE encoding does with a byte once it exists server-side, and they
// say nothing about client_encoding conversion, where a connection that
// declared a single-byte encoding could give 0xff a meaning instead of
// refusing it. Treat the table as covering the tested encodings and no
// others; it is not a general account of Postgres.
//
// None of that changes what this middleware is for, which is why the
// argument is recorded and then dropped rather than settled. Pad does not
// create or configure the database — nothing here issues CREATE DATABASE
// or sets client_encoding, so the encoding is the operator's, and a
// self-hoster can be on either row (initdb's default encoding is
// LOCALE-dependent, not always UTF8). What Pad's own connections declare
// was measured rather than assumed: `SHOW client_encoding` over the pgx
// pool this store opens reports UTF8 against a UTF8 server. The rule below
// is uniform across every one of these cases, which is the property that
// makes the deployment's configuration stop mattering.
// SQLite is looser again: sqlite3_bind_text accepts arbitrary byte
// sequences, and an embedded NUL truncates or is otherwise undefined
// rather than erroring.
//
// So this is not the intersection of two engines' rules, and it is not a
// rule the database hands us. It is the strictest reading, applied
// uniformly at the transport, so that a request cannot get one answer on
// SQLite and another on Postgres — which is the actual defect being fixed.
func bindableText(s string) bool {
return utf8.ValidString(s) && !strings.ContainsRune(s, 0)
}
// ValidateQuery is the query-string half of the same rule ValidatePath
// applies to the path: reject a request whose DECODED query keys or values
// are not values the database can be asked about.
//
// WHY A TRANSPORT RULE HERE, when BUG-2782 said the query string would be
// validated at its points of use. That was the plan, on BUG-2774's
// validCursorID model, and reading the mechanism retired it: the set of
// query parameter names that reaches the store is UNBOUNDED BY DESIGN.
// parseItemListParams folds every parameter it does not recognise into
// params.Fields, so `?email=`, `?type=` and `?anything-at-all=` become
// field filters and reach a text comparison exactly as `?search=` does.
// There is no finite list of points to validate, because the wildcard
// branch is what turns an undeclared name into a filter in the first
// place.
//
// WHY THIS RULE IS NOT A NARROWING of what callers may send, which is the
// objection BUG-2782's comment raised against validating the query at the
// transport. That objection is sound against a charset or ASCII rule:
// query values carry user search text and a tag or title fragment can be
// any language. It does not reach bindableText, which requires only valid
// UTF-8 with no NUL. Every legitimate value in this system 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. So the rule
// refuses nothing a client can legitimately send — which is the claim that
// matters here. It is NOT "exactly what Postgres refuses": it is stricter
// in two known ways, both deliberate and both written down rather than
// glossed. It refuses invalid UTF-8 that a SQL_ASCII database would have
// accepted (see bindableText's table), and it refuses a bad byte sitting
// in a query pair that url.ParseQuery discards, which no handler would
// ever have seen (see validQueryText).
//
// Measured on Postgres 17 (server_encoding UTF8) at 19330410, before this
// middleware existed: 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: and a single-address sweep answers 429 to
// everything.
//
// invalid-UTF-8 value: 432 probes → 276 × 200, 56 × 400, 100 × 500
// NUL value: 432 probes → 276 × 200, 56 × 400, 100 × 500
// control: 432 probes → 376 × 200, 56 × 400, 0 × 500
//
// Zero 500s in the control, so the 100 are attributable to the value.
// 98 of them are the two items-list endpoints (the parseItemListParams
// funnel); the rest are /search?q, /activity?action|actor|source,
// /attachments?collection and /items-index?collection. The error is
// `invalid byte sequence for encoding "UTF8": 0xff (SQLSTATE 22021)` —
// the same SQLSTATE as BUG-2782 and BUG-2774.
//
// KEYS ARE VALIDATED PRECAUTIONARILY, not because a bad key was observed to
// fail. The same sweep drove an invalid-UTF-8 parameter NAME at all eight
// endpoints and got 7 × 200 and 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 here supports; a sweep that did not reproduce is not a proof of
// absence. Checking them costs nothing on the path that matters (see the
// fast path in validQueryText) and removes the need to be right about it.
//
// 400 rather than 404, and the shared `decorate` CORS instance, for the
// reasons ValidatePath's comment gives — a rejection at the root would
// otherwise be shaped unlike every other API error.
//
// BUG-2784.
func ValidateQuery(decorate func(http.Handler) http.Handler) func(http.Handler) http.Handler {
var reject http.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
writeError(w, http.StatusBadRequest, "invalid_query",
"Request query string contains invalid UTF-8 or a NUL byte")
})
if decorate != nil {
reject = decorate(reject)
}
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if !validQueryText(r.URL.RawQuery) {
reject.ServeHTTP(w, r)
return
}
next.ServeHTTP(w, r)
})
}
}
// validQueryText reports whether every key and value a handler could read
// out of this raw query string is bindableText.
//
// WHAT IT CHECKS AGAINST. url.ParseQuery is the same call r.URL.Query()
// makes, so the pairs checked in the decode step are exactly the pairs a
// handler can see — there is no equivalent of the RawPath divergence
// ValidatePath's comment has to reason about, where the value checked and
// the value delivered can differ. ParseQuery's error is deliberately
// discarded: on a malformed escape it returns the pairs that DID parse and
// drops the rest, and r.URL.Query() discards the error identically, so
// rejecting the whole request on that error would refuse requests whose
// reachable parameters are all perfectly fine.
//
// THE RAW CHECK IS DELIBERATELY STRICTER THAN THE REACHABLE SET, and it is
// worth being exact about that rather than claiming the two coincide. It
// runs over the whole raw query before any decoding, so a bad byte sitting
// inside a pair that ParseQuery would DROP still rejects the request — e.g.
// `search=ok&ignored=<NUL>%`, where no handler could ever see the NUL.
// Kept, for two reasons. It is the ONLY check that runs at all on the fast
// path, and the only one that sees a raw unescaped byte in a pair the
// decode step DISCARDS. It is not the only check in general — when the
// decode step runs, a raw byte in a pair that SURVIVES parsing reaches
// bindableText through the loop below too — so neither "the only check
// that sees a raw 0xff" nor "everything surviving reaches the loop" is
// true on its own; which one applies depends on the fast path. And a legitimate
// request has no such byte anywhere in its query whether or not the pair
// carrying it survives parsing, so the extra strictness can only fire on a
// request that was already malformed — the safe direction to be wrong in.
//
// THE FAST PATH is not an optimisation detail, it is most requests. When
// the raw query holds no '%', decoding performs no percent-decoding at all.
// What it does do is split on '&', split each pair on '=', substitute ' '
// for '+', and — on an unescaped ';' — return an error and DROP the pairs
// it could not parse. Every decoded key and value is therefore a substring
// of the raw string with some ASCII '+' bytes swapped for ASCII ' ' bytes,
// and possibly fewer of them than the raw string contains. It is NOT a
// plain substring, which is what an earlier draft of this comment claimed,
// and dropping is not a case that needs handling because a subset of a
// checked set is still checked. The conclusion survives both corrections:
// splitting valid UTF-8 at ASCII boundaries yields valid UTF-8, replacing
// one ASCII byte with another preserves that, discarding pairs cannot
// introduce anything, and none of these can put a NUL into a string that
// had none.
func validQueryText(rawQuery string) bool {
if rawQuery == "" {
return true
}
if !bindableText(rawQuery) {
return false
}
if !strings.Contains(rawQuery, "%") {
return true
}
q, _ := url.ParseQuery(rawQuery)
for key, values := range q {
if !bindableText(key) {
return false
}
for _, v := range values {
if !bindableText(v) {
return false
}
}
}
return true
}
// unicodeEscapePrefix is the four bytes that begin any JSON \u escape for a
// character below U+0100. Built from bytes rather than written as a literal,
// for the reason the tests do the same.
var unicodeEscapePrefix = []byte{'\\', 'u', '0', '0'}
// bodyDecodesNUL reports whether any string a handler could read out of this
// JSON body — an object key or a value, at any nesting depth, including
// inside a JSON document carried as a string — decodes to a string containing
// a NUL.
//
// WHY THE BODY NEEDS ITS OWN RULE, when ValidatePath and ValidateQuery already
// apply bindableText at the transport. Those work because a decoded path or
// query value is a substring of the raw request with ASCII substitutions: the
// bad byte in the raw text IS the bad byte in the value, so a middleware can
// find it without parsing. That property does not hold for a JSON body. The
// reachable NUL arrives as a six-character JSON escape (backslash, u, and
// four zeros — spelled out rather than written, since a literal is one
// transformation away from being the character it describes), all ordinary
// ASCII, so
// a transport-level scan for a NUL byte sees nothing, and no request
// middleware can find it without decoding the body, which is the handler's
// job. BUG-2784 recorded this as the reason its rule stops at the query
// string; this is the missing half.
//
// THE GATE IS A BACKSLASH, NOT THE ESCAPE SUBSTRING, and the difference is a
// real bypass rather than a stylistic one.
//
// The obvious fast path — "does the raw body contain that escape?" — is
// UNSOUND, and codex round 4 on BUG-2803 demonstrated it. The escape may be
// spelled obliquely: `\u005c` decodes to a BACKSLASH, so a body carrying
// `\u005cu0000` contains no literal six-character escape anywhere in its raw
// bytes, yet the OUTER decode manufactures one inside the string, and if that
// string is re-parsed as a JSON document (see jsonEncodedFieldKeys) the
// second parse turns it into a real NUL. Measured before the fix: that body
// answered 201 through the real router while the direct spelling 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 itself can be written as an
// escape. It is the same layer-confusion this whole bug is made of, three
// rounds in a row.
//
// A backslash is the sound gate: every JSON escape mechanism requires one, so
// a body with no backslash anywhere 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 a backslash pay for
// an exact answer — a larger set than before (any nested JSON carries `\"`),
// which is the cost of being correct here.
//
// WHY THE EXACT STEP IS NOT A SUBSTRING SEARCH EITHER. Containing
// the escape is not sufficient either: `\\u0000` (an escaped backslash followed
// by literal text) contains the same six characters and decodes to no NUL at
// all. Refusing on the substring alone would reject a legitimate value, and in
// THIS product that is not hypothetical — items and documents store markdown,
// and writing about a JSON escape sequence is an ordinary thing for a document
// to do.
//
// So the exact step is a walk over the DECODED body (valueDecodesNUL), which
// distinguishes those cases by construction rather than by pattern, needs no
// knowledge of the destination type, and reaches nested maps such as an item's
// `fields` blob that a struct-shaped check would miss.
//
// WHY NOT REFLECT OVER THE DECODED VALUE, which is the other obvious design.
// A reflective walk sees a []byte field AFTER base64 decoding, so a body
// carrying legitimate binary — `{"b":"AQAC"}` decodes to the bytes 01 00 02 —
// would be refused for a NUL that is not text and never reaches a text
// column. A token walk sees the base64 characters instead. No request struct
// has such a field today (searched: []byte fields with a json tag in
// internal/server and internal/models, non-test — the only hit is
// models.YjsUpdate.UpdateData, which no handler decodes from a body, since
// collab moves Yjs data over the WebSocket as binary). The token walk is
// chosen so that adding one later cannot silently start rejecting valid
// requests.
//
// A malformed body returns false rather than an error: the caller's decode
// runs next and reports the JSON error itself, so there is exactly one place
// that phrases "invalid JSON" and this function never has to agree with it.
// That choice has a consequence, and it is finding (2) below rather than a
// clean separation of concerns.
//
// WHAT THIS CHECK DOES NOT COVER — four measured disagreements between what
// this scan sees and what the typed decode does, left OPEN deliberately
// (BUG-2803 rounds 16-17; lead ruling: land-and-follow). They are
// recorded here because this is the function a reader consults before
// trusting the check, and an unqualified doc comment above an incomplete
// guard is how the next person inherits a false belief.
//
// The root cause of all four is one thing: this scan decodes into
// map[string]any, and encoding/json's typed decode does NOT agree with that
// model about keys. TWO UNDER-REFUSE (a NUL gets through) and TWO
// OVER-REFUSE (a legitimate body is rejected).
//
// UNDER-REFUSALS — these are the BUG-2812 unit's spec, not a TODO here. Both
// dissolve under a token-stream walk that never builds values, which is that
// unit's design. The ruling's reasoning for not folding that rewrite in: a
// review loop finding something in nearly every round says the pre-scan
// machinery has a DESIGN problem, and the answer to that is a unit of its
// own rather than a late restructure of a branch already deep in review.
//
// 1. DUPLICATE KEYS MERGE DIFFERENTLY (P1). For
// {"fields_patch":{"orphan":"<NUL>"},"fields_patch":{"status":"open"}},
// decoding into map[string]any REPLACES the first value, so this scan
// sees only `status`; encoding/json MERGES into an already-populated map
// field, so the handler keeps both and persists the NUL. The `any` scan
// structurally cannot see the shadowed occurrence — no amount of care
// inside this model reaches it.
// 2. A SCAN FAILURE LETS A KNOWN-BAD VALUE THROUGH (P1). For
// {"title":"a<NUL>b","ignored":1e999}, the overflowing number makes the
// `any` unmarshal fail, this function returns false (see the paragraph
// above), and the typed decode then SKIPS the unknown field and accepts
// the body with its NUL title. Returning an error instead would reject
// bodies the handlers accept today, so the fix is not "refuse on scan
// failure" — it is not building values in the first place.
//
// OVER-REFUSALS — dispositions, ACCEPTED as-is, and the reason each is
// tolerable is that it refuses rather than admits:
//
// 3. UNKNOWN FIELDS ARE SCANNED THOUGH HANDLERS IGNORE THEM (P2).
// {"title":"valid","future_field":"<NUL>"} is refused although the value
// reaches nothing. Scoping the scan to known fields would need the
// destination type, which this function deliberately does not have (see
// WHY NOT REFLECT above) — so the alternative is not a smaller change,
// it is a different design. ACCEPTED, and it is an OBSERVABLE
// COMPATIBILITY CHANGE: a client sending a forward-compatible field with
// a NUL escape in it now gets a 400 where it got a 200. Stated in the
// release note for that reason, not only here.
// 4. CASE-VARIANT DUPLICATES OVER-REJECT (P2). For
// {"title":"<NUL>","TITLE":"safe"} the typed decode keeps `safe` and
// discards the NUL, while this scan sees both keys and refuses. Same
// root as (1), opposite direction: there the map model hides an
// occurrence, here it retains one the decode drops. ACCEPTED — refusing
// a body that deliberately spells one field twice in two cases costs a
// caller nothing real.
//
// The asymmetry is the honest summary: within the map model, (1) and (4) are
// the same defect seen from two sides, and only one of them fails safe.
func bodyDecodesNUL(raw []byte) bool {
if !bytes.Contains(raw, unicodeEscapePrefix) {
return false
}
var v any
if err := json.Unmarshal(raw, &v); err != nil {
// Malformed: the caller's own decode reports the JSON error, so this
// function never has to phrase one.
return false
}
return valueDecodesNUL(v, false)
}
// jsonEncodedFieldKeys are the REQUEST-BODY keys whose STRING value is itself
// a JSON document that something downstream re-parses. They are the only keys
// under which the walk descends into a second document.
//
// WHY THE SCOPING EXISTS. The first version of this check recursed into ANY
// string that parsed as a JSON document, on the argument that a structural
// test beats a destination-typed one. Codex round 2 on BUG-2803 showed what
// that costs: 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 narrow door the recursion closed.
//
// 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.
//
// OVER-INCLUSION IS THE SAFE DIRECTION and this 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 by hand for that reason — it carries JSON but its
// field declaration has no comment saying so, which is exactly how the
// derivation test would have missed it, so that test asserts coverage in one
// direction only and the list is allowed to be a superset.
var jsonEncodedFieldKeys = map[string]bool{
"config": true,
"events": true,
"fields": true,
"metadata": true,
"phase_data": true,
"plan_overrides": true,
"schema": true,
"settings": true,
"tags": true,
"traits": true,
}
// isJSONEncodedFieldKey matches a wire key the way the DECODER that consumes
// it does: case-insensitively.
//
// 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":...}` both land in ItemCreate.Fields. A case-SENSITIVE lookup
// here therefore skipped the nested walk for a body the handler went on to
// accept, and the database answered the original 500 (codex round 16,
// measured: `fields` refused, `Fields` and `FIELDS` accepted).
//
// This is the same defect shape as the rest of BUG-2803 — a check that agrees
// with one layer's rules while the layer that actually consumes the value
// uses different ones — and it is the reason this predicate is a function
// rather than a bare map index: the map is the vocabulary, the MATCHING RULE
// belongs to the consumer.
func isJSONEncodedFieldKey(k string) bool {
if jsonEncodedFieldKeys[k] {
return true
}
// EqualFold, not ToLower. encoding/json matches with Unicode SIMPLE
// FOLDING, which is wider than lower-casing: U+017F LATIN SMALL LETTER
// LONG S folds to 's', so "ſchema" reaches the `schema` field while
// strings.ToLower("ſchema") is still "ſchema" and missed the allowlist
// (codex round 17). Fixing the ASCII half and leaving the Unicode half
// would have been this bug's own pattern one more time.
for canonical := range jsonEncodedFieldKeys {
if strings.EqualFold(k, canonical) {
return true
}
}
return false
}
// valueDecodesNUL walks a decoded request body for a string that contains a
// NUL, descending into a JSON document carried as a string exactly once.
//
// inUserData says the walk has left the REQUEST's own structure and is inside
// caller data — the natural object under `fields`, an element of a `tags`
// array, or a re-parsed document. The key list is not consulted there, and
// both halves of that matter:
//
// - A collection may declare a user field literally named `schema` or
// `tags`. Treating `{"fields":{"schema":"..."}}`'s inner key as a wire key
// refused valid text that happened to hold a JSON example (codex round 8).
// - Below the document Postgres parses, an escape is harmless. Measured on
// Postgres 17 with the check disabled: a NUL escape two levels deep
// imports 201, because `fields` is parsed ONCE and the inner text is
// re-escaped when the blob is written, so Postgres never sees an escape
// (codex round 7).
//
// Together those make the descent exactly one level deep by construction,
// which is why there is no depth counter here. An earlier version had one,
// bounding a recursion that inherited the flag through the whole subtree;
// with the flag no longer inherited, a counter would be a bound that can
// never fire, and dead protection reads as protection.
//
// WHY A DECODED WALK RATHER THAN reflection over the destination struct: a
// reflective walk sees []byte fields AFTER base64 decoding, so a body
// carrying legitimate binary — {"b":"AQAC"} decodes to the bytes 01 00 02 —
// would be refused for a NUL that is not text and never reaches a text
// column. Decoding into `any` never produces a []byte, so the value seen here
// is the base64 TEXT. 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);
// this shape is chosen so that adding one later cannot silently start
// rejecting valid requests.
func valueDecodesNUL(v any, inUserData bool) bool {
// S1 (DOC-2823): once classing is switched OFF there is nothing
// request-specific left to do, and the walk is textguard's — literally the
// same traversal this function used to carry inline. Delegating rather than
// keeping a copy is the point of extracting the package: two walks of the
// same shape, in two layers, is the disagreement this cluster exists
// because of.
if inUserData {
return textguard.ValueDecodesNUL(v)
}
switch t := v.(type) {
case string:
return textguard.ContainsNUL(t)
case map[string]any:
for k, sub := range t {
if textguard.ContainsNUL(k) {
return true
}
if !inUserData && isJSONEncodedFieldKey(k) {
if str, isString := sub.(string); isString {
// BOTH checks. The nested walk answers "is there an
// escape inside the document this string carries"; it
// does NOT answer "does this string itself contain a
// NUL", and taking the JSON-encoded branch used to skip
// the plain check entirely — so a direct NUL in a
// `fields` value was accepted, reopening the door this
// whole change exists to close (codex round 9, a
// regression introduced by the round-8 restructure).
if textguard.ContainsNUL(str) || nestedDocumentDecodesNUL(str) {
return true
}
continue
}
// The field's NATURAL shape: an array or object whose
// elements the server marshals itself. Nothing re-parses
// them, so everything below is caller data.
if valueDecodesNUL(sub, true) {
return true
}
continue
}
if valueDecodesNUL(sub, inUserData) {
return true
}
}
case []any:
for _, sub := range t {
if valueDecodesNUL(sub, inUserData) {
return true
}
}
}
return false
}
// nestedDocumentDecodesNUL walks a JSON document carried as a string — an
// item's `fields` blob, a collection's `schema` — for a string containing a
// NUL. This is the layer Postgres itself parses, so an escape here is fatal
// where one a level deeper is not.
// ANY JSON DOCUMENT SHAPE, scalars included (codex round 1 on DOC-2823 S1).
//
// This used to test objects and arrays only, which is the right rule for
// deciding "is this a nested document worth walking" and the WRONG one for
// deciding "will a jsonb parser read this". A `fields` value that is a bare
// JSON string decoding to a NUL is a complete jsonb document Postgres refuses;
// the narrow rule let it past the gate to be caught at the store instead.
//
// Both layers now use the same shape test, which is what makes the differential
// corpus able to assert they AGREE rather than merely that something refuses
// eventually.
func nestedDocumentDecodesNUL(s string) bool { return textguard.DocumentDecodesNULAnyShape(s) }
// readBodyForDecode reads the whole request body so it can be scanned before
// it is decoded, with the caller's size cap applied.
//
// Buffering is not a cost paid for the scan. json.Decoder.Decode already
// holds the entire top-level value in memory before it finishes — refill
// accumulates into dec.buf and grows it by DOUBLING (encoding/json/stream.go,
// `newBuf := make([]byte, len(dec.buf), 2*cap(dec.buf)+minRead)` plus a copy)
// — so streaming never avoided the copy, it just reallocated its way there.
// Measured on the 64 MiB workspace-import shape, total allocation: stream and
// decode 354.7 MiB, read-all and Unmarshal 256.5 MiB, read-all and Decode
// 512.5 MiB. Peak heap is indistinguishable between the first two and is
// order-dependent, so it does not discriminate. BUG-2803.
func readBodyForDecode(r *http.Request, maxBytes int64) ([]byte, error) {
if r.Body == nil {
return nil, io.EOF
}
// The nil ResponseWriter is deliberate and its consequence is worth
// stating, because the comment this replaces got it wrong twice over:
// MaxBytesReader.Close forwards to the underlying body rather than being
// a no-op, and with a nil writer there is no automatic 413 — hitting the
// cap surfaces as a read error, which decodeJSONWithLimit wraps and every
// caller turns into a 400. That is the existing behaviour, unchanged
// here; only the claim about it is corrected (codex round 8).
r.Body = http.MaxBytesReader(nil, r.Body, maxBytes)
return io.ReadAll(r.Body)
}
// truncateBindableText cuts s to at most maxBytes bytes WITHOUT splitting a
// rune, so the result is still valid UTF-8.
//
// A plain s[:n] slices bytes. If byte n lands inside a multi-byte rune the
// result ends in a partial sequence, which is not valid UTF-8 — so a value
// that PASSED the body check a few frames earlier becomes unbindable on its
// way to the store, and Postgres answers 22021 for a request the server
// already accepted. Found by the codex round 5 enumeration on BUG-2803, which
// asked what could still reach a text parameter unvalidated and named
// truncation rather than any input path.
//
// The failure is invisible in testing with ASCII, which is why it survived
// four review rounds aimed at the input side: every fixture in this area uses
// ASCII names, and ASCII cannot reproduce it.
func truncateBindableText(s string, maxBytes int) string {
if len(s) <= maxBytes {
return s
}
cut := maxBytes
// Walk back off any continuation bytes (10xxxxxx) to the start of the
// rune that straddles the boundary, then drop that rune entirely.
for cut > 0 && s[cut]&0xC0 == 0x80 {
cut--
}
return s[:cut]
}
// requestUserAgent returns the User-Agent header as text the database can
// store, replacing any invalid UTF-8 and dropping any NUL.
//
// A header is not a path, a query or a body, so none of the rules above see
// it — and unlike those, it is not the caller ASKING for anything: it is
// metadata this server chose to record. Refusing the whole request because a
// header is malformed would answer 400 to a request whose actual subject is
// fine, so this sanitizes rather than rejects, which is the opposite
// disposition from the rest of this file and deliberately so.
//
// The sink is one text column: activities.user_agent, reached from three
// document paths and the connected-apps revoke.
//
// THE LOGIN PATHS ARE NOT SINKS, and I wired them before reading
// store.CreateSession, which is the mistake this note exists to stop
// recurring. It HASHES the header (sessions.ua_hash) and never stores the
// text — the round-5 enumeration listed "sessions.user_agent" and I took the
// name for a column. Sanitising there would have been actively harmful:
// login would store sha256(sanitised) while middleware_auth still compares
// sha256(RAW), so every session belonging to a client with a non-UTF-8
// User-Agent would fail validation. Hashing arbitrary bytes is well defined
// and needs no help; the hash sites are deliberately untouched.
//
// Found by the codex round 5 enumeration on BUG-2803. The filing's own
// earlier probe had recorded User-Agent as NOT reproducing on the item-create
// path, which was true and did not generalise: these are different sinks.
func requestUserAgent(r *http.Request) string {
return sanitiseStoredText(r.Header.Get("User-Agent"))
}
// sanitiseStoredText makes a caller-influenced string safe to bind to a text
// column, by removing what a text column cannot hold rather than by refusing
// the request.
//
// SANITISE VERSUS REFUSE is the whole decision here, and it turns on WHO ASKED
// FOR THE VALUE TO EXIST. The body rule refuses, because there the bad value
// is the thing the caller asked to store. This helper serves metadata the
// SERVER chose to record about a request — a User-Agent header, an MCP audit
// row — where the caller never asked for a write at all. Turning an otherwise
// fine request into a 400, or losing the audit row entirely, because of a
// field the server elected to keep would be the wrong trade: the request
// carrying a malformed value is precisely the one most worth recording.
//
// Extracted so the two callers share one rule rather than two copies that
// drift. Do NOT reach for this on a value the caller asked to persist — that
// is the body rule's job, and silently cleaning such a value would store
// something the caller did not send.
func sanitiseStoredText(s string) string {
clean, _ := sanitiseStoredTextChanged(s)
return clean
}
// sanitiseStoredTextChanged is sanitiseStoredText plus the fact a caller needs
// when the value is an IDENTITY rather than free text: whether anything was
// removed.
//
// Cleaning is LOSSY, so distinct inputs collapse onto the same output —
// "pad_<NUL>item" and "pad_item" both become "pad_item". For a User-Agent
// that is fine; nothing decides anything on it. For a value that NAMES what a
// request did, it is a forgery: an audit row or a metric label that cannot be
// told apart from the genuine one it imitates (codex round 23, after round 22
// closed the coarser version of the same hole).
//
// Callers that store an identity must use this and mark a changed value, so
// "was cleaned" stays visible instead of being silently absorbed. Callers that
// store descriptive text can keep using sanitiseStoredText.
func sanitiseStoredTextChanged(s string) (string, bool) {
clean := strings.ReplaceAll(strings.ToValidUTF8(s, ""), "\x00", "")
return clean, clean != s
}