mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
7376023ff2
* fix(server): the request-body gate reads tokens with jsontext, so a NUL behind a repeated key or beside 1e999 is refused, and a repeated request member is refused on every body (BUG-2812) The body gate and the store's JSON-column guard decoded into map[string]any and walked the tree. The tree is a lossy model: it keeps one value per repeated key, so a NUL in a shadowed copy passed (and the typed decode could merge it into storage), and a number no float64 holds made the decode fail, so the gate answered "no NUL" for a body the typed decode then accepted. Walking that tree also cost about 2x on any body that merely mentioned an escape. - internal/jsonscan wraps github.com/go-json-experiment/json/jsontext (pinned v0.0.0-20260820222146-c27c302e5fc3, BSD-3, The Go Authors). It is the module's only importer; its doc comment names the exit to stdlib encoding/json/jsontext. A differential test pins its acceptance to encoding/json v1's, which found that jsontext refuses a lone surrogate v1 accepts; that case falls back to v1's unquote. - scanRequestBody reads every body once, recording a NUL and a repeated request member separately and reading to the end, so malformed input still reports nothing. Repeats count in the request's own structure, and not in caller data under JSON-encoded keys (BUG-2896 collapses those). Names are case-folded where the destination is a struct: decodeJSONBytes reflects its destination's TOP-LEVEL type, so a struct folds its members, a slice of structs each element's, and a map (admin settings, admin user limits) none. Below the top level only an exact repeat counts. decodeJSONBytes and both bundle paths refuse either. - textguard's document walk and artifactIsBindableText read tokens too, so every layer refuses the shadowed NUL together: the one textguard.KnownGaps case moves into Corpus. - The store's suspect path stays, redundant but correct, by ruling; its deletion is its own unit. Its instruments now pin what is true: no refused value is a suspect, and the remaining suspects are the doubled-backslash class the destination accepts. - The map-model walk survives only as the differential's oracle. * fix(server): update vendorHash and the Postgres-only suspect instruments for the token scan (BUG-2812) The Postgres-backed legs of the suspect path fired when the shared predicate learned to see a NUL behind a repeated key: that value is now a violation everywhere, and the store's own guard refuses it before the destination cast. The preflight legs now assert that, the unmigrated- table fixture uses a doubled-backslash literal (still a suspect), and PostgreSQL's native refusal stays measured by TestNativePostgresAgreesWithTheCorpus. vendorHash is the value this PR's Nix job recomputed.
947 lines
45 KiB
Go
947 lines
45 KiB
Go
package server
|
||
|
||
import (
|
||
"bytes"
|
||
"io"
|
||
"net/http"
|
||
"net/url"
|
||
"reflect"
|
||
"strings"
|
||
"unicode/utf8"
|
||
|
||
"github.com/PerpetualSoftware/pad/internal/jsonscan"
|
||
"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
|
||
// a field filter (params.Fields, or params.FieldsAnyOf for a comma list), 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'}
|
||
|
||
// THE REQUEST-BODY SCAN (scanRequestBody, below). Its NUL half 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 CHANGED IN BUG-2812. This used to decode the body into map[string]any
|
||
// and walk the tree, and the tree is a lossy model of the document. Four
|
||
// measured disagreements with the typed decode followed from that (BUG-2803
|
||
// rounds 16-17). The scan now reads TOKENS (package jsonscan), and each of the
|
||
// four has an answer:
|
||
//
|
||
// 1. A NUL IN A SHADOWED DUPLICATE, {"fields_patch":{"orphan":"<NUL>"},
|
||
// "fields_patch":{...}}: the tree kept one value per key and never saw the
|
||
// first. Every occurrence is a token, so it is REFUSED as a NUL.
|
||
// 2. A NUMBER NO float64 HOLDS, {"title":"a<NUL>b","ignored":1e999}: the tree
|
||
// decode failed, this answered "no NUL", and the typed decode skipped the
|
||
// unknown field and stored the title. A number is never converted, so the
|
||
// title is seen and REFUSED.
|
||
// 3. A NUL IN A FIELD NO HANDLER READS, {"title":"ok","future":"<NUL>"}:
|
||
// still refused, ACCEPTED as before. Scoping the scan to known fields
|
||
// needs the destination type, which this function deliberately lacks
|
||
// (WHY NOT REFLECT above). It is in the BUG-2803 release note.
|
||
// 4. A CASE-VARIANT DUPLICATE, {"title":"<NUL>","TITLE":"safe"}: refused as
|
||
// before, and now for a stated reason rather than as an accepted
|
||
// over-refusal. It is a repeated request member (below).
|
||
//
|
||
// REPEATED REQUEST MEMBERS ARE REFUSED (BUG-2812 ruling, day 80). The typed
|
||
// decode gives a repeat one of two meanings depending on the destination: a
|
||
// map member MERGES the occurrences, a struct or raw member keeps the LAST.
|
||
// A caller who sent two cannot know which, and BUG-3219 was a write that
|
||
// silently lost one. So a repeat anywhere in the REQUEST's own structure is
|
||
// refused. Two things bound "repeat":
|
||
//
|
||
// - Case folding applies where the destination is known to be a STRUCT,
|
||
// since encoding/json matches `TITLE` to a `title` field. Into a MAP,
|
||
// `Status` and `status` are two keys and only an exact repeat counts.
|
||
// decodeJSONBytes knows its destination, so it passes the TOP-LEVEL kind
|
||
// (repeatFoldFor): a struct folds its members; a slice of structs folds
|
||
// each element's (PUT /workspaces/reorder, the role board); a map
|
||
// (admin settings, admin user limits) folds nothing. Below the top level
|
||
// the scan does not know struct from map and counts exact repeats only,
|
||
// so a case-variant repeat inside a NESTED struct still takes the typed
|
||
// decode's last-wins answer. That is a known residual; the member scans on
|
||
// fields_patch and field_overrides (BUG-3219) are the precise check for
|
||
// the two nested members where it mattered.
|
||
// - Caller DATA is exempt: below a JSON-encoded field key (`fields`,
|
||
// `schema`, ...), a repeated key is the item's own content, which BUG-2896
|
||
// stores collapsed rather than refusing.
|
||
//
|
||
// This runs on EVERY body, not only one carrying an escape, because a refusal
|
||
// that depended on whether a body happened to mention an escape would be no
|
||
// rule at all. The NUL half still only looks where a NUL can be (bodies
|
||
// containing unicodeEscapePrefix). Cost, measured on the same bodies before
|
||
// and after: BUG-2812's trail.
|
||
|
||
// bodyFindings is what one scan of a request body found.
|
||
type bodyFindings struct {
|
||
nul bool
|
||
// repeat is the first repeated request member, as the body spelled its
|
||
// FIRST occurrence, or "" when there was none.
|
||
repeat string
|
||
}
|
||
|
||
// bodyDecodesNUL reports only the NUL half of scanRequestBody.
|
||
func bodyDecodesNUL(raw []byte) bool { return scanRequestBody(raw, foldMembers).nul }
|
||
|
||
// repeatFold says where the scan may fold member names, from what it knows of
|
||
// the destination's TOP-LEVEL type.
|
||
type repeatFold int
|
||
|
||
const (
|
||
// foldMembers: the top-level value decodes into a struct.
|
||
foldMembers repeatFold = iota
|
||
// foldElementMembers: into a slice or array of structs.
|
||
foldElementMembers
|
||
// foldNone: into a map, or a type the scan cannot classify.
|
||
foldNone
|
||
)
|
||
|
||
// repeatFoldFor classifies a decode destination by its top-level kind only.
|
||
// It reflects on the TYPE, never the value, so none of WHY NOT REFLECT's
|
||
// concerns (a []byte seen after base64) apply.
|
||
func repeatFoldFor(dst any) repeatFold {
|
||
t := reflect.TypeOf(dst)
|
||
for t != nil && t.Kind() == reflect.Pointer {
|
||
t = t.Elem()
|
||
}
|
||
if t == nil {
|
||
return foldNone
|
||
}
|
||
switch t.Kind() {
|
||
case reflect.Struct:
|
||
return foldMembers
|
||
case reflect.Slice, reflect.Array:
|
||
e := t.Elem()
|
||
for e.Kind() == reflect.Pointer {
|
||
e = e.Elem()
|
||
}
|
||
if e.Kind() == reflect.Struct {
|
||
return foldElementMembers
|
||
}
|
||
}
|
||
return foldNone
|
||
}
|
||
|
||
// scanRequestBody reads the whole body once. A body that is not exactly one
|
||
// well-formed JSON value reports nothing: the caller's own decode reports the
|
||
// JSON error, so there is one place that phrases "invalid JSON", and it is
|
||
// never this one. That is also why the scan does not stop at its first
|
||
// finding. A finding in a document that turns out to be malformed would put a
|
||
// NUL or repeat message on a body whose real problem is its syntax.
|
||
func scanRequestBody(raw []byte, fold repeatFold) bodyFindings {
|
||
w := bodyWalk{sc: jsonscan.NewScanner(raw), nulChecks: bytes.Contains(raw, unicodeEscapePrefix)}
|
||
w.sc.Raw = !w.nulChecks
|
||
k, str, err := w.sc.Next()
|
||
if err != nil || !w.value(k, str, false, fold == foldMembers, fold == foldElementMembers) {
|
||
return bodyFindings{}
|
||
}
|
||
if _, _, err := w.sc.Next(); err != io.EOF {
|
||
return bodyFindings{}
|
||
}
|
||
return w.found
|
||
}
|
||
|
||
type bodyWalk struct {
|
||
sc *jsonscan.Scanner
|
||
nulChecks bool
|
||
found bodyFindings
|
||
// spans and arena hold the member names of every object currently open,
|
||
// innermost last (see noteMember).
|
||
spans []memberSpan
|
||
arena []byte
|
||
}
|
||
|
||
// value walks one value whose first token is (k, str). It answers false for
|
||
// malformed input.
|
||
//
|
||
// inUserData says the walk has left the REQUEST's own structure and is inside
|
||
// caller data: the object or array under a JSON-encoded field key. The key
|
||
// list is not consulted there, and repeats are not refused there, for the
|
||
// reasons on jsonEncodedFieldKeys and the block above. Both halves of the first
|
||
// 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
|
||
// on BUG-2803).
|
||
// - 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 (codex round 7).
|
||
//
|
||
// Together those make the descent into a second document exactly one level
|
||
// deep by construction: only a string under a JSON-encoded key at request
|
||
// level is re-parsed.
|
||
//
|
||
// fold says this object's member names are case-folded before comparing (its
|
||
// destination is a struct); foldElems says the same of each element of this
|
||
// array. Both come from the top-level destination only.
|
||
func (w *bodyWalk) value(k jsonscan.Kind, str []byte, inUserData, fold, foldElems bool) bool {
|
||
switch k {
|
||
case jsonscan.String:
|
||
w.checkNUL(str)
|
||
return true
|
||
case jsonscan.Scalar:
|
||
return true
|
||
case jsonscan.ArrayStart:
|
||
for {
|
||
w.sc.Raw = !w.nulChecks
|
||
ek, estr, err := w.sc.Next()
|
||
if err != nil {
|
||
return false
|
||
}
|
||
if ek == jsonscan.ArrayEnd {
|
||
return true
|
||
}
|
||
if !w.value(ek, estr, inUserData, foldElems, false) {
|
||
return false
|
||
}
|
||
}
|
||
case jsonscan.ObjectStart:
|
||
members := w.openMembers()
|
||
defer w.closeMembers(members)
|
||
for {
|
||
// Names are always decoded: the repeat check compares them.
|
||
w.sc.Raw = false
|
||
nk, name, err := w.sc.Next()
|
||
if err != nil {
|
||
return false
|
||
}
|
||
if nk == jsonscan.ObjectEnd {
|
||
return true
|
||
}
|
||
if nk != jsonscan.String {
|
||
return false
|
||
}
|
||
w.checkNUL(name)
|
||
// Read everything the name decides BEFORE the next token: name is
|
||
// only valid until then.
|
||
encoded := !inUserData && encodedFieldKey(name)
|
||
if !inUserData {
|
||
w.noteMember(members, name, fold)
|
||
}
|
||
// A value's text matters only to the NUL half, and only when the
|
||
// body can hold a NUL at all.
|
||
w.sc.Raw = !w.nulChecks
|
||
vk, vstr, err := w.sc.Next()
|
||
if err != nil {
|
||
return false
|
||
}
|
||
if encoded {
|
||
if vk == jsonscan.String {
|
||
// BOTH checks (codex round 9 on BUG-2803): the string
|
||
// itself, and the document it carries.
|
||
w.checkNUL(vstr)
|
||
if w.nulChecks && !w.found.nul && nestedDocumentDecodesNUL(string(vstr)) {
|
||
w.found.nul = true
|
||
}
|
||
continue
|
||
}
|
||
// The field's natural shape: caller data from here down.
|
||
if !w.value(vk, vstr, true, false, false) {
|
||
return false
|
||
}
|
||
continue
|
||
}
|
||
if !w.value(vk, vstr, inUserData, false, false) {
|
||
return false
|
||
}
|
||
}
|
||
default:
|
||
// A closing delimiter where a value belongs.
|
||
return false
|
||
}
|
||
}
|
||
|
||
// memberSpan is one member name recorded in bodyWalk.arena: key is the bytes
|
||
// compared (folded where the destination is a struct), orig is the spelling a
|
||
// refusal reports.
|
||
type memberSpan struct{ keyStart, keyEnd, origStart, origEnd int }
|
||
|
||
// membersFrame marks where one object's names begin in the walk's shared
|
||
// stacks. Objects nest, and a child's names are dropped when it closes, so a
|
||
// parent's names stay contiguous from its own mark.
|
||
type membersFrame struct{ spans, arena int }
|
||
|
||
func (w *bodyWalk) openMembers() membersFrame {
|
||
return membersFrame{spans: len(w.spans), arena: len(w.arena)}
|
||
}
|
||
|
||
func (w *bodyWalk) closeMembers(f membersFrame) {
|
||
w.spans, w.arena = w.spans[:f.spans], w.arena[:f.arena]
|
||
}
|
||
|
||
// noteMember records a member name and reports a repeat. The names live in one
|
||
// buffer the whole walk reuses, rather than a map per object with a string per
|
||
// key: every body is walked now, and that allocation per key was most of the
|
||
// cost on the common body (BUG-2812 trail, perf table). The search is linear,
|
||
// which is cheaper than hashing for the few members a request object has; an
|
||
// object with thousands of members costs quadratic time in its own size,
|
||
// bounded by the body cap.
|
||
func (w *bodyWalk) noteMember(f membersFrame, name []byte, fold bool) {
|
||
origStart := len(w.arena)
|
||
w.arena = append(w.arena, name...)
|
||
keyStart, keyEnd := origStart, len(w.arena)
|
||
if fold {
|
||
keyStart = len(w.arena)
|
||
w.arena = append(w.arena, jsonscan.FoldName(name)...)
|
||
keyEnd = len(w.arena)
|
||
}
|
||
key := w.arena[keyStart:keyEnd]
|
||
for _, sp := range w.spans[f.spans:] {
|
||
if bytes.Equal(w.arena[sp.keyStart:sp.keyEnd], key) {
|
||
if w.found.repeat == "" {
|
||
w.found.repeat = string(w.arena[sp.origStart:sp.origEnd])
|
||
}
|
||
return
|
||
}
|
||
}
|
||
w.spans = append(w.spans, memberSpan{keyStart, keyEnd, origStart, origStart + len(name)})
|
||
}
|
||
|
||
// encodedFieldKey is isJSONEncodedFieldKey for a name the walk holds as bytes,
|
||
// with the common case answered without a string: an exact hit, or a name of
|
||
// lower-case ASCII, which case-folds only to itself, so the fold loop over the
|
||
// list cannot match it.
|
||
func encodedFieldKey(name []byte) bool {
|
||
if jsonEncodedFieldKeys[string(name)] {
|
||
return true
|
||
}
|
||
for _, c := range name {
|
||
if c >= utf8.RuneSelf || ('A' <= c && c <= 'Z') {
|
||
return isJSONEncodedFieldKey(string(name))
|
||
}
|
||
}
|
||
return false
|
||
}
|
||
|
||
func (w *bodyWalk) checkNUL(s []byte) {
|
||
if w.nulChecks && !w.found.nul && bytes.IndexByte(s, 0) >= 0 {
|
||
w.found.nul = true
|
||
}
|
||
}
|
||
|
||
// 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
|
||
}
|
||
|
||
// 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
|
||
}
|