From 11dec4c853ec8c10601559706345761d63c80471 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 12:44:24 +0000 Subject: [PATCH 01/29] fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied string reached a Postgres text parameter, Postgres refused it, and the handler answered 500 — the honest answer is 400. WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this is a different fix rather than a wider middleware. ValidateQuery works because a decoded query value is a substring of the raw query with ASCII substitutions: the bad byte in the raw text IS the bad byte in the value. That property fails for a JSON body — the reachable NUL arrives as the six-character escape, all ordinary ASCII — so no request middleware can find it without decoding the body, which is the handler's job. MECHANISM, each premise measured against encoding/json rather than reasoned about: raw NUL inside a string -> decode ERR (invalid character in string literal) raw NUL after the value -> decode ERR the escape in a value -> decodes to a string CONTAINING a NUL the escape in a KEY -> same a DOUBLED backslash -> decodes to literal text, NO NUL the uppercase spelling -> not a JSON escape at all So the escape is the only vector and its substring is a sound FAST PATH (absent -> no NUL possible), but not a sufficient test: a doubled backslash carries the same six characters and decodes to text. In this product that is not hypothetical — items and documents store markdown, and a document about JSON escapes is an ordinary thing to write. The exact step is json.Decoder.Token(), which returns DECODED strings, covers object keys and arbitrary nesting (an item's fields blob), and needs no knowledge of the destination type. NOT REFLECTION over the decoded value, the other obvious design: it sees []byte fields AFTER base64 decoding, so a body carrying legitimate binary ({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not text. A token walk sees the base64 characters. No request struct has such a field today (searched: []byte with a json tag in internal/server and internal/models, non-test — only models.YjsUpdate.UpdateData, which no handler decodes from a body); the token walk is chosen so adding one later cannot silently start rejecting valid requests. BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole top-level value in memory — refill accumulates into dec.buf and grows it by doubling (encoding/json/stream.go) — so streaming never avoided the copy. Measured on the 64 MiB workspace-import shape, total allocation: stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode 512.5 MiB. Peak heap is order-dependent and does not discriminate; the first run of that measurement showed a 0.77x peak win that vanished when the legs were swapped, so only the allocation figure is claimed. POPULATION, measured on Postgres 17 through the real router with a control leg on every endpoint (92 mutating routes enumerated via chi.Walk; 13 probed): before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021) after: 0 of 13 — every NUL leg 400, every control leg unchanged Confirmed doors: workspace name, collection name, item title, item content, item fields value, item title via PATCH, comment body, agent role name, view name, document title, webhook secret, workspace import. workspace-token name is UNMEASURED, not clean — its control leg 500s on an unrelated FK in this fixture. The other 79 routes are unprobed, not claimed clean; the completeness argument is structural instead, and enforced by a test rather than asserted. SECOND DEFECT, named rather than slipped in: the six handlers that decoded straight off r.Body had no http.MaxBytesReader either — the cap decodeJSON has always applied — so each was an unbounded body read. Routing them through decodeJSON closes that too. COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the JSON value where Decode ignored it. Deliberate, same direction as this fix, and the only behaviour change beyond the refusal. Trailing whitespace still passes. An EMPTY body still returns a wrapped io.EOF, because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is exactly the wiring a helper-level change is blind to. No call site changed for the refusal itself: all 65 decodeJSON callers already turn a decode error into a 400 carrying err.Error(). Release note: a NUL character in a JSON request body now returns 400 instead of 500 on Postgres deployments. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 263 ++++++++++++++++++ .../server/handlers_attachments_transform.go | 5 +- internal/server/handlers_comments.go | 9 +- internal/server/handlers_items.go | 4 +- internal/server/handlers_oauth.go | 2 +- internal/server/handlers_reports.go | 5 +- internal/server/middleware_request_text.go | 102 +++++++ internal/server/server.go | 49 +++- 8 files changed, 419 insertions(+), 20 deletions(-) create mode 100644 internal/server/decode_json_nul_test.go diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go new file mode 100644 index 000000000..8b6d5a1de --- /dev/null +++ b/internal/server/decode_json_nul_test.go @@ -0,0 +1,263 @@ +package server + +import ( + "bytes" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// rawJSONRequest sends a RAW body string. A test cannot marshal a Go map +// here: marshalling would escape the backslash and send the literal text +// instead of the escape, so every probe would be testing the harmless case. +func rawJSONRequest(srv *Server, method, path, body string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, bytes.NewReader([]byte(body))) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "192.0.2.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr +} + +// escNULLiteral is the six-character JSON escape that decodes to a NUL, +// assembled from bytes. Written as a Go literal it is one backslash away from +// being the NUL itself, and every layer between an editor and the compiler is +// a chance for that to happen silently — which is the whole subject here. +var escNULLiteral = string([]byte{'\\', 'u', '0', '0', '0', '0'}) + +func TestBodyDecodesNUL(t *testing.T) { + esc := escNULLiteral + cases := []struct { + name string + body string + want bool + }{ + {"plain body, no escape anywhere", `{"title":"hello"}`, false}, + {"escape in a string value", `{"title":"a` + esc + `b"}`, true}, + {"escape as the whole value", `{"title":"` + esc + `"}`, true}, + {"escape in an OBJECT KEY", `{"a` + esc + `b":"v"}`, true}, + {"escape nested in a fields map", `{"fields":{"k":"a` + esc + `b"}}`, true}, + {"escape inside an array element", `{"tags":["ok","a` + esc + `b"]}`, true}, + + // The reason this is not a substring search. A doubled backslash is + // an escaped BACKSLASH followed by the literal characters u0000, so + // the decoded string holds no NUL. This product stores markdown, and + // a document explaining JSON escapes is an ordinary thing to write. + {"doubled backslash decodes to literal text, NOT a NUL", + `{"content":"write it as \\` + "u0000" + ` in JSON"}`, false}, + {"doubled backslash twice", `{"a":"\\` + "u0000" + `","b":"\\` + "u0000" + `"}`, false}, + + // A NUL escape that follows an escaped backslash IS real: the first + // two backslashes consume each other, so the third begins a fresh + // escape. The doubled-backslash cases above must not be read as + // "any preceding backslash makes it safe". + {"escaped backslash then a REAL escape", `{"a":"\\` + esc + `"}`, true}, + + {"other escapes are untouched", `{"a":"tab\there é \n"}`, false}, + {"base64-looking payload with no escape", `{"b":"AQACAAAA"}`, false}, + + // Malformed input answers false: the caller's decode reports the JSON + // error, so this function never has to phrase one. + {"malformed JSON carrying the escape", `{"a":"` + esc, false}, + {"empty body", ``, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := bodyDecodesNUL([]byte(tc.body)); got != tc.want { + t.Errorf("bodyDecodesNUL(%q) = %v, want %v", tc.body, got, tc.want) + } + }) + } +} + +// TestBodyDecodesNULAgreesWithTheDecoder is the differential check: for every +// case above, the verdict must match what actually lands in a decoded Go +// value. Asserting the function against hand-written expectations pins my +// reading of JSON escaping; asserting it against encoding/json pins the +// property the fix depends on. +func TestBodyDecodesNULAgreesWithTheDecoder(t *testing.T) { + esc := escNULLiteral + bodies := []string{ + `{"title":"hello"}`, + `{"title":"a` + esc + `b"}`, + `{"a` + esc + `b":"v"}`, + `{"fields":{"k":"a` + esc + `b"}}`, + `{"tags":["ok","a` + esc + `b"]}`, + `{"content":"write it as \\` + "u0000" + ` in JSON"}`, + `{"a":"\\` + esc + `"}`, + `{"a":"tab\there é \n"}`, + } + for _, body := range bodies { + var v any + if err := json.Unmarshal([]byte(body), &v); err != nil { + t.Fatalf("fixture %q does not decode: %v", body, err) + } + want := anyHasNUL(v) + if got := bodyDecodesNUL([]byte(body)); got != want { + t.Errorf("bodyDecodesNUL(%q) = %v, but the decoded value %s a NUL", + body, got, map[bool]string{true: "CONTAINS", false: "does not contain"}[want]) + } + } +} + +func anyHasNUL(v any) bool { + switch t := v.(type) { + case string: + return strings.ContainsRune(t, 0) + case map[string]any: + for k, sub := range t { + if strings.ContainsRune(k, 0) || anyHasNUL(sub) { + return true + } + } + case []any: + for _, sub := range t { + if anyHasNUL(sub) { + return true + } + } + } + return false +} + +// TestDecodeJSONRefusesNULThroughTheHandler exercises the BINDING, not the +// predicate: a real request through the real router, so the test has an +// opinion about whether decodeJSON is what handlers actually call. Its +// control leg is the same request with an ordinary value — without one, a 400 +// proves nothing, since a malformed body would produce the same status +// (this filing's original measurement made exactly that mistake). +// +// Runs on SQLite, where the underlying insert would SUCCEED with a truncated +// value rather than error. That is deliberate: on SQLite the refusal can only +// come from this fix, so a green here cannot be the database doing the work. +func TestDecodeJSONRefusesNULThroughTheHandler(t *testing.T) { + srv := testServer(t) + + rr := rawJSONRequest(srv, "POST", "/api/v1/workspaces/", + `{"name":"NUL probe","slug":"nulprobe","template":"startup"}`) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("fixture workspace: %d %s", rr.Code, rr.Body.String()) + } + rr = rawJSONRequest(srv, "POST", "/api/v1/workspaces/nulprobe/collections/", + `{"name":"Probes","slug":"probes"}`) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("fixture collection: %d %s", rr.Code, rr.Body.String()) + } + + const itemsPath = "/api/v1/workspaces/nulprobe/collections/probes/items" + esc := escNULLiteral + + control := rawJSONRequest(srv, "POST", itemsPath, `{"title":"a-plain-b"}`) + if control.Code != http.StatusCreated && control.Code != http.StatusOK { + t.Fatalf("control leg must succeed, got %d: %s", control.Code, control.Body.String()) + } + + for _, tc := range []struct { + name string + body string + }{ + {"title", `{"title":"a` + esc + `b"}`}, + {"content", `{"title":"ok","content":"a` + esc + `b"}`}, + {"fields value", `{"title":"ok","fields":{"k":"a` + esc + `b"}}`}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := rawJSONRequest(srv, "POST", itemsPath, tc.body) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "NUL") { + t.Errorf("400 body should name the cause, got: %s", rr.Body.String()) + } + }) + } + + // The doubled-backslash body must still be ACCEPTED end to end. This is + // the leg that fails if anyone ever "simplifies" the check into a + // substring search. + ok := rawJSONRequest(srv, "POST", itemsPath, + `{"title":"escapes","content":"write it as \\`+"u0000"+` in JSON"}`) + if ok.Code != http.StatusCreated && ok.Code != http.StatusOK { + t.Fatalf("literal escape text must be accepted, got %d: %s", ok.Code, ok.Body.String()) + } +} + +// TestDecodeJSONKeepsEmptyBodyEOF pins a contract a caller depends on: +// handlers_playbooks.go treats errors.Is(err, io.EOF) as "no arguments +// supplied" and runs the playbook anyway. json.Decoder answered io.EOF on an +// empty body; json.Unmarshal answers a SyntaxError, which that check cannot +// see. Without this, an empty-body playbook run starts returning 400. +func TestDecodeJSONKeepsEmptyBodyEOF(t *testing.T) { + for _, body := range []string{"", " ", "\n\t "} { + req := httptest.NewRequest("POST", "/", strings.NewReader(body)) + var v map[string]any + err := decodeJSON(req, &v) + if err == nil { + t.Fatalf("empty body %q: expected an error", body) + } + if !errors.Is(err, io.EOF) { + t.Errorf("empty body %q: error must wrap io.EOF, got %v", body, err) + } + } +} + +// TestNoJSONBodyDecoderOutsideTheChokepoint is the completeness claim, made +// ENFORCEABLE rather than asserted. +// +// The fix works because every JSON request body in this package reaches the +// store through decodeJSON/decodeJSONWithLimit. That was true of 65 call +// sites and false of six, which decoded straight off r.Body and inherited +// neither the NUL check nor the size cap decodeJSON has always applied. A +// seventh added later would silently reopen both, and nothing in a diff would +// point at it — so the invariant is checked here instead of remembered. +func TestNoJSONBodyDecoderOutsideTheChokepoint(t *testing.T) { + // The chokepoint itself reads the body; nothing else in the package may. + allowed := map[string]bool{"middleware_request_text.go": true} + + pattern := regexp.MustCompile(`json\.NewDecoder\((r|req)\.Body\)|io\.ReadAll\((r|req)\.Body\)`) + + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("read package dir: %v", err) + } + var offenders []string + scanned := 0 + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + scanned++ + if allowed[name] { + continue + } + src, err := os.ReadFile(filepath.Join(".", name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + for i, line := range strings.Split(string(src), "\n") { + if pattern.MatchString(line) { + offenders = append(offenders, name+":"+strconv.Itoa(i+1)+": "+strings.TrimSpace(line)) + } + } + } + + // Assert the scan actually looked at something. A test whose search + // silently matched no files would pass forever. + if scanned < 20 { + t.Fatalf("scan looked at only %d non-test .go files; the package is much larger, so the scan is broken", scanned) + } + if len(offenders) > 0 { + t.Errorf("request bodies must be decoded through decodeJSON/decodeJSONWithLimit "+ + "(BUG-2803: they apply the NUL refusal and the size cap). Found %d direct decoder(s):\n %s", + len(offenders), strings.Join(offenders, "\n ")) + } +} diff --git a/internal/server/handlers_attachments_transform.go b/internal/server/handlers_attachments_transform.go index 0fcdc3805..bc998a6ab 100644 --- a/internal/server/handlers_attachments_transform.go +++ b/internal/server/handlers_attachments_transform.go @@ -2,7 +2,6 @@ package server import ( "bytes" - "encoding/json" "errors" "fmt" "image" @@ -226,8 +225,8 @@ func (s *Server) handleTransformAttachment(w http.ResponseWriter, r *http.Reques } var req transformRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeError(w, http.StatusBadRequest, "bad_request", "Invalid JSON body") + if err := decodeJSON(r, &req); err != nil { + writeError(w, http.StatusBadRequest, "bad_request", err.Error()) return } diff --git a/internal/server/handlers_comments.go b/internal/server/handlers_comments.go index e5d4fd395..42fb7bae2 100644 --- a/internal/server/handlers_comments.go +++ b/internal/server/handlers_comments.go @@ -2,7 +2,6 @@ package server import ( "database/sql" - "encoding/json" "net/http" "strings" @@ -320,8 +319,8 @@ func (s *Server) handleCreateReply(w http.ResponseWriter, r *http.Request) { } var input models.CommentCreate - if err := json.NewDecoder(r.Body).Decode(&input); err != nil { - writeError(w, http.StatusBadRequest, "invalid_json", "Invalid JSON body") + if err := decodeJSON(r, &input); err != nil { + writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) return } if strings.TrimSpace(input.Body) == "" { @@ -428,8 +427,8 @@ func (s *Server) handleAddReaction(w http.ResponseWriter, r *http.Request) { var input struct { Emoji string `json:"emoji"` } - if err := json.NewDecoder(r.Body).Decode(&input); err != nil { - writeError(w, http.StatusBadRequest, "invalid_json", "Invalid JSON body") + if err := decodeJSON(r, &input); err != nil { + writeError(w, http.StatusBadRequest, "invalid_json", err.Error()) return } if strings.TrimSpace(input.Emoji) == "" { diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index efc0b6fe4..333894c8a 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -1969,8 +1969,8 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { TargetCollection string `json:"target_collection"` FieldOverrides map[string]any `json:"field_overrides"` } - if err := json.NewDecoder(r.Body).Decode(&input); err != nil { - writeError(w, http.StatusBadRequest, "invalid_body", "Invalid JSON body") + if err := decodeJSON(r, &input); err != nil { + writeError(w, http.StatusBadRequest, "invalid_body", err.Error()) return } if input.TargetCollection == "" { diff --git a/internal/server/handlers_oauth.go b/internal/server/handlers_oauth.go index 962c9a78f..709d10a76 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -227,7 +227,7 @@ func (s *Server) handleOAuthRegister(w http.ResponseWriter, r *http.Request) { } var input dcrRequest - if err := json.NewDecoder(r.Body).Decode(&input); err != nil { + if err := decodeJSON(r, &input); err != nil { writeDCRError(w, http.StatusBadRequest, "invalid_client_metadata", "Request body must be JSON: "+err.Error()) return diff --git a/internal/server/handlers_reports.go b/internal/server/handlers_reports.go index c779fa4ee..b443552ea 100644 --- a/internal/server/handlers_reports.go +++ b/internal/server/handlers_reports.go @@ -1,7 +1,6 @@ package server import ( - "encoding/json" "net/http" "strconv" "strings" @@ -144,8 +143,8 @@ func (s *Server) handleSaveReportLayout(w http.ResponseWriter, r *http.Request) } var layout models.ReportLayout - if err := json.NewDecoder(r.Body).Decode(&layout); err != nil { - writeError(w, http.StatusBadRequest, "bad_request", "Invalid layout JSON") + if err := decodeJSON(r, &layout); err != nil { + writeError(w, http.StatusBadRequest, "bad_request", err.Error()) return } diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 4b8b0847f..d89a5c627 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -1,6 +1,9 @@ package server import ( + "bytes" + "encoding/json" + "io" "net/http" "net/url" "strings" @@ -343,3 +346,102 @@ func validQueryText(rawQuery string) bool { } return true } + +// jsonNULEscape is the six-byte JSON escape that decodes to a NUL. It is +// built from bytes rather than written as a literal so that no layer between +// this source and the compiler can transform it into the character it +// describes — the same reason the tests construct it this way. +// +// It is the ONLY spelling. JSON forbids an unescaped control character inside +// a string, so a raw 0x00 byte never survives decoding (encoding/json answers +// `invalid character '\x00' in string literal`), and the uppercase \U form is +// not a JSON escape at all (`invalid character 'U' in string escape code`). +// Both measured against encoding/json, BUG-2803. +var jsonNULEscape = []byte{'\\', 'u', '0', '0', '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 — 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 jsonNULEscape — six ordinary ASCII characters — 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. +// +// WHY IT IS NOT A SUBSTRING SEARCH. Containing jsonNULEscape is necessary but +// NOT sufficient: `\\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 substring is used as a FAST PATH only, and it is sound in that +// direction: a body that does not contain it cannot decode to a NUL anywhere, +// because the escape is the only spelling (see jsonNULEscape). Bodies +// containing it — rare, and already unusual — pay for an exact answer. +// +// THE EXACT STEP IS json.Decoder.Token(), which hands back the DECODED string +// for every key and value in the document. It distinguishes the two cases +// above by construction rather than by pattern, it needs no knowledge of the +// destination type, and it reaches nested maps such as an item's `fields` +// blob, which 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. +func bodyDecodesNUL(raw []byte) bool { + if !bytes.Contains(raw, jsonNULEscape) { + return false + } + dec := json.NewDecoder(bytes.NewReader(raw)) + for { + tok, err := dec.Token() + if err != nil { + // io.EOF, or malformed input the caller's decode will report. + return false + } + if s, ok := tok.(string); ok && strings.ContainsRune(s, 0) { + return true + } + } +} + +// 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 + } + // MaxBytesReader.Close() is a no-op; setting this also lets the server + // return 413 automatically via the error the caller wraps. + r.Body = http.MaxBytesReader(nil, r.Body, maxBytes) + return io.ReadAll(r.Body) +} diff --git a/internal/server/server.go b/internal/server/server.go index 800113c37..952af91dd 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -6,7 +6,9 @@ import ( "crypto/subtle" "encoding/base64" "encoding/json" + "errors" "fmt" + "io" "io/fs" "log/slog" "net" @@ -2190,18 +2192,53 @@ func decodeJSON(r *http.Request, v interface{}) error { // default cap is too small — but always pass an explicit cap, never // remove the wrapper. func decodeJSONWithLimit(r *http.Request, v interface{}, maxBytes int64) error { - // http.MaxBytesReader.Close() is a no-op; the decoder leaves r.Body at - // EOF anyway. Setting this here also lets the server return a 413 - // automatically via the error we wrap below. - if r.Body != nil { - r.Body = http.MaxBytesReader(nil, r.Body, maxBytes) + raw, err := readBodyForDecode(r, maxBytes) + if err != nil { + return fmt.Errorf("invalid JSON: %w", err) } - if err := json.NewDecoder(r.Body).Decode(v); err != nil { + // An EMPTY (or whitespace-only) body must keep returning a wrapped + // io.EOF. json.Decoder.Decode answered io.EOF there and at least one + // caller depends on it — handlers_playbooks.go treats + // errors.Is(err, io.EOF) as "no arguments supplied" and runs anyway — + // while json.Unmarshal answers a SyntaxError instead, which that check + // cannot see. Found by TestPlaybookRunAcceptsEmptyBody, which is exactly + // the wiring a helper-level change is blind to. + if len(bytes.TrimSpace(raw)) == 0 { + return fmt.Errorf("invalid JSON: %w", io.EOF) + } + // Refuse a decoded NUL BEFORE unmarshalling, so the value never exists + // in a Go string that a handler could hand to the store. See + // bodyDecodesNUL for why the body needs its own rule and why the check + // cannot be a substring search. BUG-2803. + if bodyDecodesNUL(raw) { + return errJSONBodyNUL + } + // json.Unmarshal rather than a Decoder over the buffer: it is the + // cheaper of the two by ~2x in total allocation (see readBodyForDecode's + // measurement), and it REFUSES trailing non-whitespace after the JSON + // value where Decode silently ignores it. That second difference is a + // deliberate behaviour change in the same direction as this fix — + // malformed input is refused at the door rather than partly consumed — + // and it is the only compatibility change in BUG-2803. Trailing + // whitespace, which real clients do send, is still accepted. + if err := json.Unmarshal(raw, v); err != nil { return fmt.Errorf("invalid JSON: %w", err) } return nil } +// errJSONBodyNUL is returned by decodeJSON when a string in the request body +// decodes to a value containing a NUL. Every decodeJSON caller already turns +// a decode error into a 400 carrying err.Error(), so this reaches the client +// as a client error with a message naming the cause, at all 65 call sites, +// without touching any of them. +// +// The wording avoids writing the escape sequence literally: the message is +// rendered in terminals, logs and a browser, and a literal escape in an error +// string is the kind of thing an intermediate layer transforms. +var errJSONBodyNUL = errors.New( + "request body contains a NUL character in a JSON string (a u0000 escape); text values cannot contain NUL") + // getWorkspaceID resolves workspace slug/ID from the request. // If RequireWorkspaceAccess already resolved the workspace, reads from context. // Otherwise falls back to direct resolution (for unauthenticated paths). From 0e7faeb301becc489db46eb4788eda018ca11111 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 13:31:38 +0000 Subject: [PATCH 02/29] fix(server): follow the NUL refusal into JSON-encoded string fields (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 1 on #1220: the check scanned ONE JSON layer, and several fields cross the wire as JSON-ENCODED STRINGS rather than nested objects — an item's fields, a collection's schema, a workspace's settings. The OUTER decode of {"fields":"{...}"} yields the inner document as literal text, in which the escape is still six ordinary characters and no NUL exists, so the single-layer token walk passed it. MEASURED on Postgres 17 with a control leg on each, after the single-layer check was already in place: item.fields as a JSON-encoded string 500 control 201 collection.schema as a string 500 control 201 workspace.settings as a string 500 control 201 The error is DIFFERENT from the rest of this family, which is why it is worth reading rather than assuming: insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05) 22P05, not the 22021 the path and query halves produce. The outer string is pure ASCII so it never trips the text-encoding check; this is Postgres's own JSON parser refusing the escape inside a document bound for jsonb, which cannot represent a NUL. After this change all three answer 400 with their control legs unchanged. THE FIX: when a decoded string is itself a complete JSON object or array — the class this API re-parses downstream — walk it too, to a depth bound of 8. Recursion terminates on its own (each level is a strict substring of the one above); the bound keeps a hostile body from buying many full re-parses, and AT the bound the body is refused rather than passed uninspected, since the escape is known to be present and the walk has stopped looking. WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is structural, not destination-typed, so a plain TEXT field whose ENTIRE value is a valid JSON document carrying the escape is refused too, even though its column would have stored it. Prose ABOUT a JSON escape does not parse as a bare document, so the case is narrow, and a value of that shape breaks any consumer that parses it. The destination-typed alternative — an allow-list of the fields that arrive JSON-encoded — is exactly correct and goes stale in silence, which is the failure mode ValidateQuery's comment rejects when it explains why per-site query validators could not be written. Tests: nested documents (fields/schema/settings/array/twice-encoded), with controls for ordinary content, a doubled backslash INSIDE the nested document, a string that starts like JSON but does not parse, and prose that merely mentions the escape; the over-refusal pinned as a decision rather than left as an accident; the depth bound; and a wiring leg through the real router on SQLite, where the write would otherwise SUCCEED so a green cannot be the database doing the work. Fixtures build their JSON-encoded strings with encoding/json rather than hand-written backslashes, since the escaping rules are the subject under test. All four new tests fail with the recursion removed. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 131 +++++++++++++++++++++ internal/server/middleware_request_text.go | 71 ++++++++++- 2 files changed, 201 insertions(+), 1 deletion(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 8b6d5a1de..a360c13ef 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -261,3 +261,134 @@ func TestNoJSONBodyDecoderOutsideTheChokepoint(t *testing.T) { len(offenders), strings.Join(offenders, "\n ")) } } + +// jsonEncode returns s as a JSON string literal — the wire form of a field +// that crosses as a JSON-ENCODED STRING (an item's fields, a collection's +// schema, a workspace's settings). Using encoding/json to build it, rather +// than hand-writing the backslashes, is deliberate: the escaping rules are +// the subject under test and hand-written fixtures would pin my reading of +// them instead of the real ones. +func jsonEncode(t *testing.T, s string) string { + t.Helper() + b, err := json.Marshal(s) + if err != nil { + t.Fatalf("encode %q: %v", s, err) + } + return string(b) +} + +// TestBodyDecodesNULNestedDocuments covers codex round 1's P1 on BUG-2803: +// several fields arrive as JSON-ENCODED STRINGS, so the escape survives the +// OUTER decode as literal text and reappears when the destination re-parses +// the string as JSON. Postgres refuses that with SQLSTATE 22P05 (unsupported +// Unicode escape sequence) rather than the 22021 the rest of this family +// produces — a different error precisely because the outer string is pure +// ASCII and never trips the text-encoding check. +func TestBodyDecodesNULNestedDocuments(t *testing.T) { + esc := escNULLiteral + + innerWithNUL := `{"k":"a` + esc + `b"}` // decodes to a NUL when parsed + innerLiteral := `{"k":"a\\` + "u0000" + `b"}` // a doubled backslash: literal text + innerPlain := `{"k":"plain"}` + + cases := []struct { + name string + body string + want bool + }{ + {"fields as a JSON-encoded string carrying the escape", + `{"title":"x","fields":` + jsonEncode(t, innerWithNUL) + `}`, true}, + {"schema as a JSON-encoded string carrying the escape", + `{"name":"c","schema":` + jsonEncode(t, innerWithNUL) + `}`, true}, + {"settings as a JSON-encoded string carrying the escape", + `{"name":"w","settings":` + jsonEncode(t, innerWithNUL) + `}`, true}, + {"a JSON-encoded ARRAY carrying the escape", + `{"tags":` + jsonEncode(t, `["ok","a`+esc+`b"]`) + `}`, true}, + {"twice-encoded — a document inside a document", + `{"fields":` + jsonEncode(t, `{"inner":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, true}, + + // Controls. These must stay ACCEPTED: the recursion must not turn + // "contains the six characters somewhere" into a refusal. + {"fields as a JSON-encoded string, ordinary content", + `{"title":"x","fields":` + jsonEncode(t, innerPlain) + `}`, false}, + {"nested document whose escape is a DOUBLED backslash", + `{"fields":` + jsonEncode(t, innerLiteral) + `}`, false}, + {"a string that starts like JSON but does not parse", + `{"content":` + jsonEncode(t, `{"k":"a`+esc+`b"`) + `}`, false}, + {"prose mentioning the escape is not a JSON document", + `{"content":` + jsonEncode(t, `write a NUL as `+esc+` in JSON`) + `}`, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := bodyDecodesNUL([]byte(tc.body)); got != tc.want { + t.Errorf("bodyDecodesNUL(%s) = %v, want %v", tc.body, got, tc.want) + } + }) + } +} + +// TestBodyDecodesNULOverRefusesWholeJSONValues pins a DECISION, not an +// accident. The nesting test is structural rather than destination-typed, so +// a plain-text field whose ENTIRE value is a valid JSON document carrying the +// escape is refused even though its column would have stored it happily. +// +// It is here so that the trade is visible and a future change to it is a +// deliberate act: if someone makes the check destination-typed, this test +// should be updated, not deleted in passing. See stringIsJSONDocument for why +// the allow-list alternative was declined. +func TestBodyDecodesNULOverRefusesWholeJSONValues(t *testing.T) { + body := `{"content":` + jsonEncode(t, `{"k":"a`+escNULLiteral+`b"}`) + `}` + if !bodyDecodesNUL([]byte(body)) { + t.Error("a text field whose whole value is a JSON document carrying the escape " + + "is refused by design; if this changed deliberately, update the reasoning at " + + "stringIsJSONDocument rather than only this test") + } +} + +// TestBodyDecodesNULDepthBound pins the behaviour AT the recursion limit: +// past it the escape is known to be present and the walk has stopped looking, +// so the body is refused rather than passed uninspected. +func TestBodyDecodesNULDepthBound(t *testing.T) { + // Wrap a NUL-bearing document deeper than the limit allows. + doc := `{"k":"a` + escNULLiteral + `b"}` + for i := 0; i < maxJSONDocumentNesting+2; i++ { + doc = `{"n":` + jsonEncode(t, doc) + `}` + } + if !bodyDecodesNUL([]byte(doc)) { + t.Error("a body nested past maxJSONDocumentNesting must be refused, not passed uninspected") + } +} + +// TestDecodeJSONRefusesNestedNULThroughTheHandler is the wiring leg for the +// nested case, on SQLite for the same reason as the single-layer one: there +// the write would SUCCEED, so a green cannot be the database doing the work. +func TestDecodeJSONRefusesNestedNULThroughTheHandler(t *testing.T) { + srv := testServer(t) + + rr := rawJSONRequest(srv, "POST", "/api/v1/workspaces/", + `{"name":"Nested","slug":"nested","template":"startup"}`) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("fixture workspace: %d %s", rr.Code, rr.Body.String()) + } + rr = rawJSONRequest(srv, "POST", "/api/v1/workspaces/nested/collections/", + `{"name":"Probes","slug":"probes"}`) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("fixture collection: %d %s", rr.Code, rr.Body.String()) + } + + const itemsPath = "/api/v1/workspaces/nested/collections/probes/items" + + control := rawJSONRequest(srv, "POST", itemsPath, + `{"title":"nested control","fields":`+jsonEncode(t, `{"k":"plain"}`)+`}`) + if control.Code != http.StatusCreated && control.Code != http.StatusOK { + t.Fatalf("control leg must succeed, got %d: %s", control.Code, control.Body.String()) + } + + bad := rawJSONRequest(srv, "POST", itemsPath, + `{"title":"nested probe","fields":`+jsonEncode(t, `{"k":"a`+escNULLiteral+`b"}`)+`}`) + if bad.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for a JSON-encoded fields string carrying the escape, got %d: %s", + bad.Code, bad.Body.String()) + } +} diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index d89a5c627..c5772b3d6 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -408,9 +408,29 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // 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. func bodyDecodesNUL(raw []byte) bool { + return bodyDecodesNULAtDepth(raw, 0) +} + +// maxJSONDocumentNesting bounds how many times bodyDecodesNULAtDepth will +// descend into a string that is itself a JSON document. Each level must be a +// strict substring of the one above, so recursion terminates on its own; the +// bound exists to keep a hostile body from buying many full re-parses of a +// large payload. Eight is far past any shape this API produces — the deepest +// real case is one level (a `fields` blob inside a request body) — and a body +// nested deeper than this is refused, since bodyDecodesNULAtDepth answers +// "assume the worst" rather than "give up" at the limit. +const maxJSONDocumentNesting = 8 + +func bodyDecodesNULAtDepth(raw []byte, depth int) bool { if !bytes.Contains(raw, jsonNULEscape) { return false } + if depth >= maxJSONDocumentNesting { + // The escape IS present and we have stopped looking. Refusing is the + // safe direction: the alternative is to pass a body we declined to + // inspect. + return true + } dec := json.NewDecoder(bytes.NewReader(raw)) for { tok, err := dec.Token() @@ -418,10 +438,59 @@ func bodyDecodesNUL(raw []byte) bool { // io.EOF, or malformed input the caller's decode will report. return false } - if s, ok := tok.(string); ok && strings.ContainsRune(s, 0) { + s, ok := tok.(string) + if !ok { + continue + } + if strings.ContainsRune(s, 0) { return true } + if stringIsJSONDocument(s) && bodyDecodesNULAtDepth([]byte(s), depth+1) { + return true + } + } +} + +// stringIsJSONDocument reports whether a decoded string is itself a complete +// JSON object or array — the class of value this API re-parses downstream. +// +// WHY THE RECURSION EXISTS AT ALL. Several fields cross the wire as +// JSON-ENCODED STRINGS rather than as nested objects: an item's `fields`, a +// collection's `schema`, a workspace's `settings`. In +// `{"fields":"{\"k\":\"ab\"}"}` the OUTER decode yields the +// literal text of the inner document, in which the escape is still six +// ordinary characters and no NUL exists. The single-layer walk therefore +// passed it, and Postgres refused it later with a DIFFERENT error from the +// rest of this family: +// +// insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05) +// +// 22P05, not the 22021 the path and query halves produce. The outer string is +// pure ASCII so it never trips the text-encoding check; this is Postgres's own +// JSON parser refusing the escape inside a document bound for jsonb, which +// cannot represent a NUL. Measured on Postgres 17: item `fields`, collection +// `schema` and workspace `settings` each answered 500 with a 201 control leg, +// after the single-layer check was in place. Found by codex round 1 on +// BUG-2803, by asking what the destination TYPE does with the value — the +// angle the endpoint-and-field sweep never rotated to. +// +// WHAT THIS DELIBERATELY OVER-REFUSES, stated rather than left to be +// discovered. The test is structural, not destination-typed: a plain TEXT +// field whose ENTIRE value happens to be a valid JSON document carrying the +// escape is refused too, even though its column would have stored it. Prose +// ABOUT a JSON escape does not parse as a bare document, so the case is +// narrow, and a value of that exact shape breaks any consumer that parses it. +// +// The destination-typed alternative — an allow-list of the fields that arrive +// JSON-encoded — is exactly correct and goes stale in silence, which is the +// failure mode ValidateQuery's comment rejects when it explains why per-site +// query validators could not be written. +func stringIsJSONDocument(s string) bool { + t := strings.TrimSpace(s) + if len(t) == 0 || (t[0] != '{' && t[0] != '[') { + return false } + return json.Valid([]byte(t)) } // readBodyForDecode reads the whole request body so it can be scanned before From 9c5d1bfbd914d59a9a2e186a85e92c14a01c3e2b Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 13:51:37 +0000 Subject: [PATCH 03/29] test(server): build the NUL-bearing timeline fixture through the store (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the API on a premise its own comment stated: "a structured id comes from the item's fields blob, which nothing validates on write". BUG-2803 made that false — decodeJSON now refuses a body whose strings decode to a NUL, including one nested inside a JSON-encoded `fields` string — so the API can no longer produce the row and the test 400'd on its fixture. Repaired rather than deleted, because the DEFENCE it covers is still live: rows in this shape can predate the rule, and the store has no such check of its own, so a migration, an import or any future non-HTTP writer can still produce one. The timeline must keep refusing to hand out a cursor it would then reject. The fixture now writes the blob directly, injecting the six-character JSON escape rather than a raw NUL — the blob is JSON text and both backends reject a raw NUL in it; the NUL comes into existence when Go DECODES the blob, which is exactly how the timeline ends up with one inside an entry id. The test is not vacuous under the change: it asserts the NUL-bearing id took the positional fallback, so an injection that failed to produce a NUL fails the test rather than passing quietly. This is the CONVE-23 case — a change that falsifies existing prose owes a sweep for that prose. The stale sentence was found by the test failing, not by the sweep, which is the weaker of the two ways to find it. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- ...andlers_timeline_cursor_validation_test.go | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/internal/server/handlers_timeline_cursor_validation_test.go b/internal/server/handlers_timeline_cursor_validation_test.go index d64f1100c..858465b8d 100644 --- a/internal/server/handlers_timeline_cursor_validation_test.go +++ b/internal/server/handlers_timeline_cursor_validation_test.go @@ -178,12 +178,26 @@ func TestListBeforeTime_InvalidUTF8CursorIsADialectDivergence(t *testing.T) { } // The other direction of the same rule: the server must never EMIT a cursor it -// would then refuse. A structured id comes from the item's fields blob, which -// nothing validates on write, so a JSON \u0000 escape reaches the timeline as -// a real NUL on SQLite (Postgres's jsonb refuses it at the door — a -// one-backend hazard). Handing that out as next_before_id would wedge paging -// on the item: the client sends it back and gets a 400 from the validation -// above. +// would then refuse. A structured id comes from the item's fields blob, and a +// JSON NUL escape there reaches the timeline as a real NUL on SQLite +// (Postgres's jsonb refuses it at the door — a one-backend hazard). Handing +// that out as next_before_id would wedge paging on the item: the client sends +// it back and gets a 400 from the validation above. +// +// THE FIXTURE WRITES THE BLOB DIRECTLY, and it did not have to when this test +// was written. The original built the item through the API, on the premise — +// stated in this comment until BUG-2803 — that "nothing validates the fields +// blob on write". That premise is now false: decodeJSON refuses a request +// body whose strings decode to a NUL, including one nested inside a +// JSON-encoded `fields` string, so the API can no longer produce this row. +// +// The DEFENCE this test covers is still live, which is why the test is +// repaired rather than deleted. Rows in this shape can predate the rule, and +// the store has no such check of its own, so anything writing a blob directly +// — a migration, an import, a future non-HTTP writer — can still produce one. +// The timeline must keep refusing to hand out an unusable cursor for data it +// did not create. Writing the row through the store is what makes the test +// about the timeline's defence rather than about the request validator. func TestTimeline_NeverEmitsACursorItWouldRefuse(t *testing.T) { t.Parallel() srv := testServer(t) @@ -193,11 +207,25 @@ func TestTimeline_NeverEmitsACursorItWouldRefuse(t *testing.T) { // truncating limit — that is what makes it the emitted next_before_id // rather than merely an entry id. The clean one is the control: it // distinguishes "replaced the unusable id" from "stopped using raw ids". - notes := `[{"id":"note-\u0000-bad","summary":"middle","created_at":"2026-04-02T10:00:01Z"},` + + notes := `[{"id":"note-PLACEHOLDER-bad","summary":"middle","created_at":"2026-04-02T10:00:01Z"},` + `{"id":"note-clean","summary":"newest note","created_at":"2026-04-02T10:00:02Z"},` + `{"id":"note-oldest","summary":"oldest","created_at":"2026-04-02T10:00:00Z"}]` item := timelineItemWithStructured(t, srv, ws, notes, "") + // Swap the placeholder for the JSON NUL ESCAPE directly in the stored + // blob — the six characters, not a raw NUL byte. The blob is stored as + // JSON text and both backends hold a CHECK/type constraint that a raw NUL + // violates; the NUL only comes into existence when Go decodes the blob, + // which is precisely how the timeline ends up with one inside an entry id. + // The API refuses this body (BUG-2803); the store does not, which is the + // gap this test exists to cover. + if _, err := srv.store.DB().Exec( + `UPDATE items SET fields = REPLACE(fields, 'PLACEHOLDER', ?) WHERE id = ?`, + string([]byte{'\\', 'u', '0', '0', '0', '0'}), item.ID, + ); err != nil { + t.Fatalf("inject NUL into the stored fields blob: %v", err) + } + // The item's own `created` activity plus three notes; limit=3 truncates, // so the cursor is the third entry — the NUL-bearing note. resp := fetchTimeline(t, srv, ws, item.Slug, "limit=3") From a1dbdf1d8d53f92d8fa17e277913ecc1d53f2d45 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 13:52:44 +0000 Subject: [PATCH 04/29] docs(server): correct the timeline comment BUG-2803 falsified (CONVE-23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entryID fallback's comment said note and decision ids "come from the item's fields blob and nothing validates them on write". BUG-2803 made that half false: the HTTP API now refuses a request body whose strings decode to a NUL, including one nested inside a JSON-encoded `fields` string. The sentence was true when written and nothing in this branch's diff pointed at it. The fallback still has to exist, and the corrected comment says why: the STORE has no such check, so rows predating the rule — and anything writing a blob by another path, a migration, an import, a future non-HTTP writer — can still carry one. SWEPT AND DELIBERATELY LEFT: two nearby comments (handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go) also say "nothing validates them on write". Both are about id FORMAT and DUPLICATION — an imported artifact carrying a UUID-shaped id, a hand-written blob repeating one — and this change validates neither. In context those sentences remain true, so they are left alone rather than edited into noise. Sweep command: grep -rniE "nothing validates|not validated on write|no validation on write|unvalidated" --include=*.go internal/ cmd/ — six further hits, all about other subjects (github_pr raw writes, terminal schema keys, push payload format, decodeJSON's size bound). Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/handlers_timeline.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/server/handlers_timeline.go b/internal/server/handlers_timeline.go index 023b9e1e1..c10100c82 100644 --- a/internal/server/handlers_timeline.go +++ b/internal/server/handlers_timeline.go @@ -336,9 +336,16 @@ func structuredTimelineEntries(item *models.Item, before time.Time, beforeID str // A raw id must be usable as a CURSOR, not merely unique: the client // sends it back as `before_id` and the handler now refuses a value the // database would refuse (BUG-2774's validCursorID). These ids come - // from the item's fields blob and nothing validates them on write, so - // a JSON `\u0000` reaches here intact on SQLite — Postgres's jsonb - // rejects it at the door, which is why this is a one-backend hazard. + // from the item's fields blob, and a JSON NUL escape there reaches + // here intact on SQLite — Postgres's jsonb rejects it at the door, + // which is why this is a one-backend hazard. This comment used to + // say "nothing validates them on write"; since BUG-2803 the HTTP + // API does refuse a request body whose strings decode to a NUL, + // including one nested inside a JSON-encoded `fields` string. The + // store does not, so rows predating that rule, and anything writing + // a blob by another path (a migration, an import, a future + // non-HTTP writer), can still carry one — which is why this + // fallback stays. // Emitting such an id would make the server hand out a cursor it then // answers 400 to, wedging paging on that item (codex round 1). It gets // the positional fallback the empty and duplicate cases already take. From c369563e812b0d2cc449947a8d6890dfeae85a3b Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 14:12:06 +0000 Subject: [PATCH 05/29] fix(server): scope the nested-NUL walk to JSON-encoded fields (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 2 on #1220. The nesting check from the previous commit recursed into ANY string that parsed as a JSON document, on the argument that a structural test beats a destination-typed one. That argument was wrong in a way I had written down as an accepted trade and should have weighed as a defect: a plain-text `content` value holding a JSON snippet that merely MENTIONS the escape was accepted before this branch, is stored in a text column that has no problem with it, and was newly refused — including on RE-IMPORT of an export carrying it. Refusing input the server itself produced is a worse failure than the door the unscoped recursion was closing. Measured before the fix: a workspace whose item content held such a snippet exported 200 and re-imported 400. The walk now descends only under keys whose STRING value is a JSON document something downstream re-parses: config, events, fields, metadata, phase_data, plan_overrides, schema, settings, tags, traits. WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly this shape for query parameters: there the set of names is unbounded by design (parseItemListParams turns any unrecognised parameter into a field filter), so no list could be complete. Here the set is a closed property of the wire model — a field is JSON-encoded because a Go struct declares it as a string holding JSON — and TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models and fails when a new one appears. The list cannot go stale in silence. Over-inclusion is the safe direction and the list takes it: a listed key that is not really JSON-encoded costs one parse attempt and can only refuse a complete JSON document carrying the escape, while a missing key reopens a door. `traits` is listed for that reason — it carries JSON but its declaration has no comment saying so, which is exactly how the derivation test would have missed it, so the test asserts coverage in one direction only and the list is allowed to be a superset. The walk also changed shape: decoding into `any` and walking the value, rather than a token stream, because key context is needed to know which subtree is JSON-encoded. The []byte reasoning is unchanged and still holds — decoding into `any` never produces a []byte, so a base64 field is seen as its ASCII text rather than as decoded bytes that might contain a legitimate 0x00. Tests: text fields carrying a JSON document are ACCEPTED (five keys), with a leg proving the same document under a JSON-encoded key is still refused, so the pair differs only in the key; the derivation test; and the depth-bound fixture now nests under a JSON-encoded key at every level, since nesting under an ordinary key would never start the recursion and would have passed for the wrong reason. STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob already carries the escape still exports 200 and re-imports 400. That is data this fix cannot make importable without weakening the write-side refusal, and the disposition (repair sweep, flagged import, or documented acceptance) is a product ruling. Recorded on the item. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 83 ++++++++-- internal/server/middleware_request_text.go | 175 ++++++++++++++------- 2 files changed, 189 insertions(+), 69 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index a360c13ef..404612636 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -328,21 +328,70 @@ func TestBodyDecodesNULNestedDocuments(t *testing.T) { } } -// TestBodyDecodesNULOverRefusesWholeJSONValues pins a DECISION, not an -// accident. The nesting test is structural rather than destination-typed, so -// a plain-text field whose ENTIRE value is a valid JSON document carrying the -// escape is refused even though its column would have stored it happily. +// TestBodyDecodesNULLeavesTextFieldsAlone is codex round 2's finding on +// BUG-2803, pinned so it cannot come back. // -// It is here so that the trade is visible and a future change to it is a -// deliberate act: if someone makes the check destination-typed, this test -// should be updated, not deleted in passing. See stringIsJSONDocument for why -// the allow-list alternative was declined. -func TestBodyDecodesNULOverRefusesWholeJSONValues(t *testing.T) { - body := `{"content":` + jsonEncode(t, `{"k":"a`+escNULLiteral+`b"}`) + `}` - if !bodyDecodesNUL([]byte(body)) { - t.Error("a text field whose whole value is a JSON document carrying the escape " + - "is refused by design; if this changed deliberately, update the reasoning at " + - "stringIsJSONDocument rather than only this test") +// The first version of the nesting check recursed into ANY string that parsed +// as a JSON document. That refused a plain-text `content` value holding a +// JSON snippet which merely MENTIONS the escape — a value this server +// accepted before the fix, stores in a text column that has no problem with +// it, and emits again in an export. Refusing input the server itself produced +// is a worse failure than the narrow door the unscoped recursion closed, so +// the recursion is now scoped to the keys that actually carry JSON documents. +func TestBodyDecodesNULLeavesTextFieldsAlone(t *testing.T) { + doc := `{"k":"a` + escNULLiteral + `b"}` + for _, key := range []string{"content", "title", "summary", "body", "description"} { + body := `{"` + key + `":` + jsonEncode(t, doc) + `}` + if bodyDecodesNUL([]byte(body)) { + t.Errorf("%s is a text field: a JSON document in its value must not be re-parsed "+ + "(codex round 2 — the server emits such values in exports and must accept them back)", key) + } + } + // The same document under a JSON-ENCODED key is still refused, so the + // legs differ only in the key and this is not just "recursion removed". + if !bodyDecodesNUL([]byte(`{"fields":` + jsonEncode(t, doc) + `}`)) { + t.Error("under a JSON-encoded key the same document must still be refused") + } +} + +// TestJSONEncodedFieldKeysCoversTheModels keeps jsonEncodedFieldKeys from +// going stale in silence, which is the whole objection to a list-based rule. +// It derives the set from the wire model — a Go string field with a json tag +// whose comment says it holds JSON — and fails when one is not covered. A key +// missing from the list reopens a door; an extra key is harmless (see the +// over-inclusion note at jsonEncodedFieldKeys), so this asserts one direction +// only, deliberately. +func TestJSONEncodedFieldKeysCoversTheModels(t *testing.T) { + entries, err := os.ReadDir("../models") + if err != nil { + t.Fatalf("read models dir: %v", err) + } + // A string field, a json tag, and a trailing comment mentioning JSON. + decl := regexp.MustCompile("string\\s+`json:\"([a-z_]+)[\",][^`]*`\\s*//[^\\n]*JSON") + found := map[string]string{} + scanned := 0 + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + scanned++ + src, err := os.ReadFile(filepath.Join("../models", name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + for _, m := range decl.FindAllStringSubmatch(string(src), -1) { + found[m[1]] = name + } + } + if scanned < 10 || len(found) < 5 { + t.Fatalf("derivation looks broken: scanned %d files, found %d JSON-encoded fields", scanned, len(found)) + } + for key, file := range found { + if !jsonEncodedFieldKeys[key] { + t.Errorf("%s declares %q as a JSON-encoded string, but it is not in jsonEncodedFieldKeys — "+ + "a NUL escape nested inside it would reach the database unchecked (BUG-2803)", file, key) + } } } @@ -351,10 +400,14 @@ func TestBodyDecodesNULOverRefusesWholeJSONValues(t *testing.T) { // so the body is refused rather than passed uninspected. func TestBodyDecodesNULDepthBound(t *testing.T) { // Wrap a NUL-bearing document deeper than the limit allows. + // Nested under a JSON-ENCODED key at every level, since that is the only + // path the walk descends: nesting under an ordinary key would never start + // the recursion and the test would pass for the wrong reason. doc := `{"k":"a` + escNULLiteral + `b"}` for i := 0; i < maxJSONDocumentNesting+2; i++ { - doc = `{"n":` + jsonEncode(t, doc) + `}` + doc = `{"fields":` + jsonEncode(t, doc) + `}` } + doc = `{"fields":` + jsonEncode(t, doc) + `}` if !bodyDecodesNUL([]byte(doc)) { t.Error("a body nested past maxJSONDocumentNesting must be refused, not passed uninspected") } diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index c5772b3d6..b0b327cbb 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -408,60 +408,139 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // 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. func bodyDecodesNUL(raw []byte) bool { - return bodyDecodesNULAtDepth(raw, 0) -} - -// maxJSONDocumentNesting bounds how many times bodyDecodesNULAtDepth will -// descend into a string that is itself a JSON document. Each level must be a -// strict substring of the one above, so recursion terminates on its own; the -// bound exists to keep a hostile body from buying many full re-parses of a -// large payload. Eight is far past any shape this API produces — the deepest -// real case is one level (a `fields` blob inside a request body) — and a body -// nested deeper than this is refused, since bodyDecodesNULAtDepth answers -// "assume the worst" rather than "give up" at the limit. -const maxJSONDocumentNesting = 8 - -func bodyDecodesNULAtDepth(raw []byte, depth int) bool { if !bytes.Contains(raw, jsonNULEscape) { return false } - if depth >= maxJSONDocumentNesting { - // The escape IS present and we have stopped looking. Refusing is the - // safe direction: the alternative is to pass a body we declined to - // inspect. - return true + 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 } - dec := json.NewDecoder(bytes.NewReader(raw)) - for { - tok, err := dec.Token() - if err != nil { - // io.EOF, or malformed input the caller's decode will report. - return false + return valueDecodesNUL(v, false, 0) +} + +// maxJSONDocumentNesting bounds how many times valueDecodesNUL will descend +// into a string that is itself a JSON document. Each level must be a strict +// substring of the one above, so recursion terminates on its own; the bound +// keeps a hostile body from buying many full re-parses of a large payload. +// Eight is far past any shape this API produces — the deepest real case is +// one level, a `fields` blob inside a request body. +const maxJSONDocumentNesting = 8 + +// jsonEncodedFieldKeys are the wire keys whose STRING value is itself a JSON +// document that something downstream re-parses. They are the only keys under +// which valueDecodesNUL descends. +// +// WHY THE SCOPING EXISTS. The first version of this check recursed into ANY +// string that parsed as a JSON document, on the argument that the test should +// be structural rather than destination-typed. Codex round 2 on BUG-2803 +// showed what that costs: a plain-text `content` value holding a JSON snippet +// that mentions the escape was ACCEPTED before this fix, 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 a value the server itself +// emitted, and that nothing downstream would choke on, is a worse failure +// than the narrow door the recursion was closing. +// +// 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. That is enumerable, and +// TestJSONEncodedFieldKeysCoversTheModels derives the set from +// internal/models and fails when a new one appears, so the list cannot go +// stale in silence. +// +// OVER-INCLUSION IS THE SAFE DIRECTION and this list deliberately takes it: a +// key listed here that is NOT actually JSON-encoded costs one parse attempt +// and can only refuse a value that IS a complete JSON document carrying the +// escape. A key MISSING from it reopens a door. `traits` is here 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. +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, +} + +// valueDecodesNUL walks a decoded request body for a string that either +// CONTAINS a NUL or, under a JSON-encoded key, is a document whose own +// strings do. +// +// 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, which is ASCII. 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. +// +// inJSONEncodedField is inherited by the whole subtree below a listed key: a +// document nested inside a JSON-encoded document is re-parsed just as its +// parent is. +func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { + switch t := v.(type) { + case string: + if strings.ContainsRune(t, 0) { + return true } - s, ok := tok.(string) - if !ok { - continue + if !inJSONEncodedField || !strings.Contains(t, string(jsonNULEscape)) { + return false } - if strings.ContainsRune(s, 0) { + if depth >= maxJSONDocumentNesting { + // The escape IS present and we have stopped looking. Refusing is + // the safe direction: the alternative is to pass a document we + // declined to inspect. return true } - if stringIsJSONDocument(s) && bodyDecodesNULAtDepth([]byte(s), depth+1) { - return true + if !stringIsJSONDocument(t) { + return false + } + var inner any + if err := json.Unmarshal([]byte(strings.TrimSpace(t)), &inner); err != nil { + return false + } + return valueDecodesNUL(inner, true, depth+1) + case map[string]any: + for k, sub := range t { + if strings.ContainsRune(k, 0) { + return true + } + if valueDecodesNUL(sub, inJSONEncodedField || jsonEncodedFieldKeys[k], depth) { + return true + } + } + case []any: + for _, sub := range t { + if valueDecodesNUL(sub, inJSONEncodedField, depth) { + return true + } } } + return false } -// stringIsJSONDocument reports whether a decoded string is itself a complete -// JSON object or array — the class of value this API re-parses downstream. -// -// WHY THE RECURSION EXISTS AT ALL. Several fields cross the wire as -// JSON-ENCODED STRINGS rather than as nested objects: an item's `fields`, a -// collection's `schema`, a workspace's `settings`. In -// `{"fields":"{\"k\":\"ab\"}"}` the OUTER decode yields the -// literal text of the inner document, in which the escape is still six -// ordinary characters and no NUL exists. The single-layer walk therefore -// passed it, and Postgres refused it later with a DIFFERENT error from the -// rest of this family: +// stringIsJSONDocument reports whether a string is a complete JSON object or +// array — the shape a downstream consumer will re-parse. +// +// WHY THE RECURSION IT GATES EXISTS. Several fields cross the wire as +// JSON-ENCODED STRINGS rather than nested objects: an item's `fields`, a +// collection's `schema`, a workspace's `settings`. In such a body the OUTER +// decode yields the inner document as literal text, in which the escape is +// still six ordinary characters and no NUL exists. A single-layer walk +// therefore passed it, and Postgres refused it later with a DIFFERENT error +// from the rest of this family: // // insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05) // @@ -473,18 +552,6 @@ func bodyDecodesNULAtDepth(raw []byte, depth int) bool { // after the single-layer check was in place. Found by codex round 1 on // BUG-2803, by asking what the destination TYPE does with the value — the // angle the endpoint-and-field sweep never rotated to. -// -// WHAT THIS DELIBERATELY OVER-REFUSES, stated rather than left to be -// discovered. The test is structural, not destination-typed: a plain TEXT -// field whose ENTIRE value happens to be a valid JSON document carrying the -// escape is refused too, even though its column would have stored it. Prose -// ABOUT a JSON escape does not parse as a bare document, so the case is -// narrow, and a value of that exact shape breaks any consumer that parses it. -// -// The destination-typed alternative — an allow-list of the fields that arrive -// JSON-encoded — is exactly correct and goes stale in silence, which is the -// failure mode ValidateQuery's comment rejects when it explains why per-site -// query validators could not be written. func stringIsJSONDocument(s string) bool { t := strings.TrimSpace(s) if len(t) == 0 || (t[0] != '{' && t[0] != '[') { From 889519e9617dd5a54957edd9f45f411e8cfe39b1 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 14:36:47 +0000 Subject: [PATCH 06/29] fix(server): close the three body doors codex round 3 found (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three verified before fixing, none taken on the reviewer's word. 1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go parses pad-export.json itself rather than through decodeJSON, so the SAME workspace import — reached with Content-Type application/gzip instead of application/json — walked straight past the NUL check into Postgres. The bundle's export blob is now checked with bodyDecodesNUL before ImportWorkspace, answering the same 400. Test drives a real tar.gz through the router with a clean-bundle control leg, because this path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order tar, duplicate entries) and a bare 400 would prove nothing. 2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's test-email endpoint read `if err := decodeJSON(...); err != nil || input.To == ""` and fell back to the admin's own address, so a body carrying a NUL answered 200. An ABSENT body legitimately means "send it to me"; a body that is present and REFUSED is a different thing, and collapsing the two turns a validation error into a success. The two cases are now separated on errors.Is(err, io.EOF). 3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the package — and to any alias or helper. A completeness test that misses a live example is worse than none, because it reads as coverage. It now scans for the thing that cannot be spelled around, a reference to the request body at all, and requires every FILE touching one to be accounted for with a written reason. Both directions are asserted: an unaccounted file fails because a door may have opened, and an accounted file that no longer touches a body ALSO fails, so the list cannot rot into stale excuses that quietly cover a future reader. Verified with a positive control (an added body reference in an unlisted file fails) and a negative one (a stale entry fails). FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not JSON, so it never went through decodeJSON and inherited neither the NUL refusal nor the path/query rule — a body is neither. A raw NUL or invalid UTF-8 reached the store and Postgres answered 22021, which the handler turned into a 500 for what is a client error. It now applies bindableText, the same predicate ValidatePath and ValidateQuery use, and answers 400 invalid_body. Note the shape difference from the JSON half: there the ESCAPE is the vector because a decoder rejects a raw NUL; here the RAW BYTE is, because nothing is in the way. Each fix has a mutation run against it: disabling the bundle guard fails the bundle test, disabling the artifact guard fails the artifact test, and both controls still pass. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/artifact_import.go | 18 ++++ internal/server/decode_json_nul_test.go | 83 ++++++++++++------- internal/server/handlers_admin.go | 16 +++- internal/server/handlers_artifact_import.go | 3 + internal/server/handlers_artifact_test.go | 42 ++++++++++ internal/server/handlers_import_bundle.go | 12 +++ .../server/handlers_import_bundle_test.go | 65 +++++++++++++++ 7 files changed, 210 insertions(+), 29 deletions(-) diff --git a/internal/server/artifact_import.go b/internal/server/artifact_import.go index 0a1729e96..0996f7fe1 100644 --- a/internal/server/artifact_import.go +++ b/internal/server/artifact_import.go @@ -49,6 +49,13 @@ var ErrArtifactTooLarge = errors.New("artifact import: body exceeds size limit") // the YAML-bomb guard limits (node count, nesting depth, or anchors/aliases). var ErrArtifactUnsafeYAML = errors.New("artifact import: frontmatter rejected by safety limits") +// ErrArtifactUnbindableText is returned when the artifact body is not text the +// database can be asked to store — invalid UTF-8, or carrying a NUL. It is a +// client error (400), not a 500, for the reason BUG-2782 gives: the value +// cannot be stored under any encoding this product supports, so the caller +// sent something that cannot mean anything here. +var ErrArtifactUnbindableText = errors.New("artifact import: body contains invalid UTF-8 or a NUL byte") + // parseArtifactRequest is the guarded HTTP-boundary parse used by the import // handler. It applies three checks IN ORDER: // @@ -83,6 +90,17 @@ func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64 return artifact.Artifact{}, fmt.Errorf("artifact import: read body: %w", err) } + // (2a) The artifact body is TEXT bound for text columns, and this + // handler reads it directly rather than through decodeJSON, so it + // inherits neither BUG-2803's refusal nor the path/query rule (a body is + // neither). A raw NUL or invalid UTF-8 here reaches the store and + // Postgres answers 22021, which the handler turns into a 500 for what is + // a client error. Same predicate as ValidatePath and ValidateQuery. + // Found by the codex round 3 sweep over body readers (BUG-2803). + if !bindableText(string(data)) { + return artifact.Artifact{}, ErrArtifactUnbindableText + } + // (2) YAML-bomb guard on the frontmatter region only. if err := guardArtifactFrontmatter(data); err != nil { return artifact.Artifact{}, err diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 404612636..3b4245f34 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -10,7 +10,6 @@ import ( "os" "path/filepath" "regexp" - "strconv" "strings" "testing" ) @@ -209,26 +208,46 @@ func TestDecodeJSONKeepsEmptyBodyEOF(t *testing.T) { } } -// TestNoJSONBodyDecoderOutsideTheChokepoint is the completeness claim, made -// ENFORCEABLE rather than asserted. +// TestEveryRequestBodyReaderIsAccountedFor is the completeness claim, made +// ENFORCEABLE rather than asserted — and made honest after codex round 3 +// showed the first version could not see past two exact call shapes. // -// The fix works because every JSON request body in this package reaches the -// store through decodeJSON/decodeJSONWithLimit. That was true of 65 call -// sites and false of six, which decoded straight off r.Body and inherited -// neither the NUL check nor the size cap decodeJSON has always applied. A -// seventh added later would silently reopen both, and nothing in a diff would -// point at it — so the invariant is checked here instead of remembered. -func TestNoJSONBodyDecoderOutsideTheChokepoint(t *testing.T) { - // The chokepoint itself reads the body; nothing else in the package may. - allowed := map[string]bool{"middleware_request_text.go": true} - - pattern := regexp.MustCompile(`json\.NewDecoder\((r|req)\.Body\)|io\.ReadAll\((r|req)\.Body\)`) +// The fix works because a JSON request body reaches the store through +// decodeJSON/decodeJSONWithLimit, which refuse a decoded NUL and apply a size +// cap. The first version of this test scanned for `json.NewDecoder(r.Body)` +// and `io.ReadAll(r.Body)` only, so it was blind to +// `io.ReadAll(io.LimitReader(r.Body, n))` — a shape ALREADY PRESENT in the +// package — and to any alias, helper or future spelling. A completeness test +// that misses a live example is worse than none: it reads as coverage. +// +// So it now scans for the thing that cannot be spelled around — a reference to +// the request body at all — and requires every FILE that touches one to be +// accounted for here with a reason. A new body reader fails this test until +// someone writes down what it does, which is the point: the decision becomes +// deliberate instead of invisible. +// +// It asserts BOTH directions. An unaccounted file fails, because a door may +// have opened. An accounted file that no longer touches a body ALSO fails, so +// the list cannot rot into a set of stale excuses that quietly permits a +// future reader added to the same file. +func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { + accounted := map[string]string{ + "middleware_request_text.go": "the chokepoint itself: readBodyForDecode reads the body under the caller's cap so bodyDecodesNUL can scan it", + "handlers_import_bundle.go": "tar.gz bundle import — streams the body through gzip, and its pad-export.json is checked with bodyDecodesNUL before ImportWorkspace", + "handlers_attachments.go": "multipart upload — the body is binary blob content, not text, and must NOT be scanned for text validity", + "artifact_import.go": "raw artifact TEXT (not JSON) — checked with bindableText, the same predicate ValidatePath and ValidateQuery apply", + "handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores it; the real decode still happens through decodeJSON downstream", + "middleware_mcp_audit.go": "audit capture — records the body for the MCP audit log and restores it; decoding still happens in the MCP dispatcher", + "handlers_tokens.go": "a nil/ContentLength check only — it never reads the body", + } + + pattern := regexp.MustCompile(`\b(r|req)\.Body\b`) entries, err := os.ReadDir(".") if err != nil { t.Fatalf("read package dir: %v", err) } - var offenders []string + touches := map[string]bool{} scanned := 0 for _, e := range entries { name := e.Name() @@ -236,29 +255,37 @@ func TestNoJSONBodyDecoderOutsideTheChokepoint(t *testing.T) { continue } scanned++ - if allowed[name] { - continue - } src, err := os.ReadFile(filepath.Join(".", name)) if err != nil { t.Fatalf("read %s: %v", name, err) } - for i, line := range strings.Split(string(src), "\n") { - if pattern.MatchString(line) { - offenders = append(offenders, name+":"+strconv.Itoa(i+1)+": "+strings.TrimSpace(line)) - } + if pattern.Match(src) { + touches[name] = true } } - // Assert the scan actually looked at something. A test whose search - // silently matched no files would pass forever. + // The scan must have looked at something, and must have FOUND something. + // A pattern that silently matched nothing would pass forever. if scanned < 20 { t.Fatalf("scan looked at only %d non-test .go files; the package is much larger, so the scan is broken", scanned) } - if len(offenders) > 0 { - t.Errorf("request bodies must be decoded through decodeJSON/decodeJSONWithLimit "+ - "(BUG-2803: they apply the NUL refusal and the size cap). Found %d direct decoder(s):\n %s", - len(offenders), strings.Join(offenders, "\n ")) + if len(touches) < 3 { + t.Fatalf("scan found only %d files touching a request body; the pattern is broken", len(touches)) + } + + for name := range touches { + if _, ok := accounted[name]; !ok { + t.Errorf("%s reads the request body but is not accounted for in this test. "+ + "A body carrying JSON must go through decodeJSON/decodeJSONWithLimit, which apply "+ + "BUG-2803's NUL refusal and the size cap. If this reader is legitimate, add it here WITH "+ + "the reason it is safe.", name) + } + } + for name, why := range accounted { + if !touches[name] { + t.Errorf("%s is accounted for here (%q) but no longer reads a request body — remove the entry "+ + "so it cannot silently cover a future reader added to that file", name, why) + } } } diff --git a/internal/server/handlers_admin.go b/internal/server/handlers_admin.go index 852d55ab4..0e0cd3a45 100644 --- a/internal/server/handlers_admin.go +++ b/internal/server/handlers_admin.go @@ -2,7 +2,9 @@ package server import ( "encoding/json" + "errors" "fmt" + "io" "net/http" "github.com/PerpetualSoftware/pad/internal/models" @@ -182,7 +184,19 @@ func (s *Server) handleTestEmail(w http.ResponseWriter, r *http.Request) { var input struct { To string `json:"to"` } - if err := decodeJSON(r, &input); err != nil || input.To == "" { + // An ABSENT or empty body means "send it to me" — the endpoint is + // deliberately callable with no payload. A body that is present and + // REFUSED is a different thing and must not be silently reinterpreted as + // that default: collapsing the two swallowed BUG-2803's refusal, so a + // request carrying a NUL answered 200 (codex round 3). + if err := decodeJSON(r, &input); err != nil { + if !errors.Is(err, io.EOF) { + writeError(w, http.StatusBadRequest, "bad_request", err.Error()) + return + } + input.To = "" + } + if input.To == "" { // Default to the admin's own email input.To = user.Email } diff --git a/internal/server/handlers_artifact_import.go b/internal/server/handlers_artifact_import.go index f96e2c9b1..b9484c848 100644 --- a/internal/server/handlers_artifact_import.go +++ b/internal/server/handlers_artifact_import.go @@ -340,6 +340,9 @@ func writeArtifactParseError(w http.ResponseWriter, err error) { case errors.Is(err, ErrArtifactTooLarge): writeError(w, http.StatusRequestEntityTooLarge, "too_large", "Artifact body exceeds the size limit") + case errors.Is(err, ErrArtifactUnbindableText): + writeError(w, http.StatusBadRequest, "invalid_body", + "Artifact body contains invalid UTF-8 or a NUL byte") case errors.Is(err, ErrArtifactUnsafeYAML): writeError(w, http.StatusBadRequest, "unsafe_yaml", "Artifact frontmatter was rejected by the import safety limits") diff --git a/internal/server/handlers_artifact_test.go b/internal/server/handlers_artifact_test.go index e31572b25..d69836819 100644 --- a/internal/server/handlers_artifact_test.go +++ b/internal/server/handlers_artifact_test.go @@ -573,3 +573,45 @@ func fieldString(t *testing.T, fieldsJSON, key string) string { s, _ := m[key].(string) return s } + +// TestImportArtifactUnbindableTextRejected covers the last body reader the +// BUG-2803 sweep turned up (codex round 3). The artifact endpoint takes RAW +// TEXT, not JSON, so it never went through decodeJSON and inherited neither +// the NUL refusal nor the path/query rule — a body is neither a path nor a +// query. A raw NUL or invalid UTF-8 reached the store, and Postgres answered +// SQLSTATE 22021, which the handler turned into a 500 for what is a client +// error. +// +// Note the shape difference from the JSON half: here a RAW byte is the vector, +// because there is no JSON decoder in the way to reject it. The predicate is +// the same bindableText the path and query middlewares apply. +func TestImportArtifactUnbindableTextRejected(t *testing.T) { + srv := testServer(t) + ws := createWSForTest(t, srv) + + good := "---\npad_artifact: convention\nformat_version: 1\ntitle: Fine\n---\n\nbody\n" + + // Control: the identical artifact, no bad bytes. Without it a 400 says + // nothing — this endpoint answers 400 for malformed frontmatter too. + if rr := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/import-artifact", []byte(good)); rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("control artifact must import, got %d: %s", rr.Code, rr.Body.String()) + } + + for _, tc := range []struct { + name string + body []byte + }{ + {"NUL byte in the body", []byte(strings.Replace(good, "body", "bo\x00dy", 1))}, + {"invalid UTF-8 in the body", append([]byte(strings.Replace(good, "body", "bo", 1)), 0xff, '\n')}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/import-artifact", tc.body) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "invalid_body") { + t.Errorf("expected the invalid_body code, got %s", rr.Body.String()) + } + }) + } +} diff --git a/internal/server/handlers_import_bundle.go b/internal/server/handlers_import_bundle.go index a8d1384af..f66e4f8bd 100644 --- a/internal/server/handlers_import_bundle.go +++ b/internal/server/handlers_import_bundle.go @@ -274,6 +274,18 @@ func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID if err != nil { return nil, fmt.Errorf("read pad-export.json: %w", err) } + // The bundle path parses pad-export.json HERE rather than + // through decodeJSON, so it inherits none of that helper's + // checks — including BUG-2803's NUL refusal. A gzip import is + // the same door as the JSON one, reached by a different + // Content-Type, so it gets the same answer rather than a 500 + // from Postgres further down (codex round 3). + if bodyDecodesNUL(buf) { + return nil, &importStatusError{ + status: http.StatusBadRequest, code: "bad_bundle", + message: "Bundle pad-export.json could not be decoded: " + errJSONBodyNUL.Error(), + } + } var export models.WorkspaceExport if err := json.Unmarshal(buf, &export); err != nil { return nil, &importStatusError{ diff --git a/internal/server/handlers_import_bundle_test.go b/internal/server/handlers_import_bundle_test.go index 98deb82d2..6058f0d1d 100644 --- a/internal/server/handlers_import_bundle_test.go +++ b/internal/server/handlers_import_bundle_test.go @@ -632,3 +632,68 @@ func TestIsSafeBundleEntryName(t *testing.T) { }) } } + +// TestImportBundle_RefusesNULInExport is codex round 3's P1 on BUG-2803. The +// tar.gz bundle path parses pad-export.json itself rather than through +// decodeJSON, so it inherited none of that helper's checks: the SAME workspace +// import, reached with Content-Type application/gzip instead of +// application/json, walked straight past the NUL refusal and into Postgres. +// +// The control leg is the identical bundle with an ordinary value. Without it a +// 400 proves nothing here — this path answers 400 for a dozen unrelated +// reasons (bad gzip, out-of-order tar, duplicate entries). +func TestImportBundle_RefusesNULInExport(t *testing.T) { + src, srcSlug := testServerWithAttachments(t) + rr := doRequest(src, "GET", "/api/v1/workspaces/"+srcSlug+"/export", nil) + if rr.Code != http.StatusOK { + t.Fatalf("export src: %d %s", rr.Code, rr.Body.String()) + } + clean := rr.Body.String() + + // Put the escape inside the exported workspace NAME. Editing the JSON + // text directly is the point: the bundle is bytes on the wire, and this + // is the shape a hand-built bundle would carry. + esc := string([]byte{'\\', 'u', '0', '0', '0', '0'}) + withNUL := strings.Replace(clean, `"name":"`, `"name":"a`+esc+`b `, 1) + if withNUL == clean { + t.Fatal("fixture did not modify the export; the probe would be vacuous") + } + + bundle := func(t *testing.T, exportJSON string) []byte { + t.Helper() + var buf bytes.Buffer + gzw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gzw) + if err := tw.WriteHeader(&tar.Header{Name: "pad-export.json", Mode: 0o644, Size: int64(len(exportJSON))}); err != nil { + t.Fatalf("write header: %v", err) + } + if _, err := tw.Write([]byte(exportJSON)); err != nil { + t.Fatalf("write export: %v", err) + } + tw.Close() + gzw.Close() + return buf.Bytes() + } + + post := func(t *testing.T, name string, body []byte) *httptest.ResponseRecorder { + t.Helper() + dest, _ := testServerWithAttachments(t) + req := httptest.NewRequest("POST", "/api/v1/workspaces/import?name="+name, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/gzip") + req.RemoteAddr = "127.0.0.1:1234" + rec := httptest.NewRecorder() + dest.ServeHTTP(rec, req) + return rec + } + + if got := post(t, "CleanBundle", bundle(t, clean)); got.Code != http.StatusOK && got.Code != http.StatusCreated { + t.Fatalf("control bundle must import, got %d: %s", got.Code, got.Body.String()) + } + got := post(t, "NULBundle", bundle(t, withNUL)) + if got.Code != http.StatusBadRequest { + t.Errorf("bundle carrying a NUL escape: status=%d, want 400; body=%s", got.Code, got.Body.String()) + } + if !strings.Contains(got.Body.String(), "NUL") { + t.Errorf("the 400 should name the cause, got body=%s", got.Body.String()) + } +} From baa5bcd157d8c4c19ebf5cc7a7bd525fa1b99a2a Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 16:01:08 +0000 Subject: [PATCH 07/29] fix(server): the escape gate was unsound, and YAML has its own (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4, two P1s, both reproduced before fixing. 1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does the raw body contain the six-character escape". That is unsound: the BACKSLASH itself can be written as an escape, so a body carrying \u0000 contains no literal six-character sequence anywhere in its raw bytes, while the OUTER decode manufactures one inside the string — and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys) the second parse turns it into a real NUL. Measured through the real router before the fix: the oblique spelling answered 201 where the direct one answered 400. The mistake was applying a fact about how a NUL is spelled INSIDE a decoded string to the RAW BYTES, where the backslash can itself be an escape. That is the same layer-confusion this whole bug is made of, for the third round running. The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so a body with no backslash has decoded strings byte-identical to its raw bytes, and a raw NUL cannot survive the decoder — no backslash therefore means no NUL, at any depth, however spelled. Bodies WITH one pay for an exact answer, a larger set than before (any nested JSON carries a backslash-quote), which is the cost of being correct. The same correction applies to the per-string pre-filter one level down. 2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in the request bytes — and the YAML decode manufactures one. Measured before the fix: that artifact imported 201 with a NUL in the item title. The decoded artifact is now checked too: title, body, and every frontmatter field value, walked because a playbook's `arguments` is a nested structure rather than a scalar. Keys are checked as well as values, on the same precautionary grounds ValidateQuery states for query parameter names. Same shape as the JSON half in both cases: a value that is harmless until a SECOND parse, checked at the layer that can see it. Tests: the oblique spelling joins the nested-document table, and the YAML escape joins the artifact table. Each is mutation-verified — reverting the gate to the substring fails the oblique case only, and disabling the post-decode artifact check fails the YAML case only, with the raw-byte cases still killed by the raw check. That per-leg discrimination is the point: it shows each check earns its own keep rather than being covered by its neighbour. Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY spelling" is true of the escape and was being used to justify a filter on the raw bytes, which is a different claim. Both now say so explicitly. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/artifact_import.go | 53 +++++++++++++++ internal/server/decode_json_nul_test.go | 8 +++ internal/server/handlers_artifact_test.go | 7 ++ internal/server/middleware_request_text.go | 79 ++++++++++++++-------- 4 files changed, 120 insertions(+), 27 deletions(-) diff --git a/internal/server/artifact_import.go b/internal/server/artifact_import.go index 0996f7fe1..ce4717f45 100644 --- a/internal/server/artifact_import.go +++ b/internal/server/artifact_import.go @@ -111,6 +111,18 @@ func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64 if err != nil { return artifact.Artifact{}, err } + + // (4) The DECODED artifact must be bindable text too — the raw check in + // (2a) is not sufficient on its own. YAML has its own escape vocabulary: + // a double-quoted scalar `title: "a\0b"` carries no NUL in the request + // bytes, passes (2a), and manufactures one during the YAML decode. + // Measured before this check: such an artifact imported 201 with a NUL in + // the item title (codex round 4, BUG-2803). Same shape as the JSON half — + // a value that only becomes dangerous after a SECOND parse — so it gets + // the same answer, at the layer that can see it. + if !artifactIsBindableText(art) { + return artifact.Artifact{}, ErrArtifactUnbindableText + } return art, nil } @@ -212,3 +224,44 @@ func extractFrontmatterRegion(s string) (string, bool) { offset += nl + 1 } } + +// artifactIsBindableText reports whether every string a decoded artifact would +// carry into the store is text the database can be asked to hold. Title, body +// and every frontmatter field value are checked; field values are walked +// because a playbook's `arguments` is a nested structure, not a scalar. +// +// Keys are checked as well as values, on the same precautionary grounds +// ValidateQuery states for query parameter names: no failure was observed +// through a key, and why it would survive is unread, so it is checked rather +// than assumed safe. +func artifactIsBindableText(art artifact.Artifact) bool { + if !bindableText(art.Title) || !bindableText(art.Body) { + return false + } + for k, v := range art.Fields { + if !bindableText(k) || !anyValueIsBindableText(v) { + return false + } + } + return true +} + +func anyValueIsBindableText(v any) bool { + switch t := v.(type) { + case string: + return bindableText(t) + case map[string]any: + for k, sub := range t { + if !bindableText(k) || !anyValueIsBindableText(sub) { + return false + } + } + case []any: + for _, sub := range t { + if !anyValueIsBindableText(sub) { + return false + } + } + } + return true +} diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 3b4245f34..362bd8fe0 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -331,6 +331,14 @@ func TestBodyDecodesNULNestedDocuments(t *testing.T) { `{"name":"w","settings":` + jsonEncode(t, innerWithNUL) + `}`, true}, {"a JSON-encoded ARRAY carrying the escape", `{"tags":` + jsonEncode(t, `["ok","a`+esc+`b"]`) + `}`, true}, + // codex round 4: the BACKSLASH itself can be written as an escape, so + // the raw body carries no literal six-character sequence anywhere + // while the outer decode manufactures one inside the nested document. + // This is the case the old substring fast path let through — measured + // as a 201 through the real router before the gate became a backslash + // check. + {"escape spelled obliquely, via an escaped backslash", + `{"fields":"{\"k\":\"a` + string([]byte{'\\', 'u', '0', '0', '5', 'c'}) + `u0000b\"}"}`, true}, {"twice-encoded — a document inside a document", `{"fields":` + jsonEncode(t, `{"inner":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, true}, diff --git a/internal/server/handlers_artifact_test.go b/internal/server/handlers_artifact_test.go index d69836819..09b3f0167 100644 --- a/internal/server/handlers_artifact_test.go +++ b/internal/server/handlers_artifact_test.go @@ -603,6 +603,13 @@ func TestImportArtifactUnbindableTextRejected(t *testing.T) { }{ {"NUL byte in the body", []byte(strings.Replace(good, "body", "bo\x00dy", 1))}, {"invalid UTF-8 in the body", append([]byte(strings.Replace(good, "body", "bo", 1)), 0xff, '\n')}, + // codex round 4: YAML has its own escape vocabulary. A double-quoted + // scalar carries no NUL in the request bytes — the raw check passes — + // and manufactures one during the YAML decode. Measured before the + // post-decode check existed: this imported 201 with a NUL in the + // title. Same shape as the JSON half, where a value only becomes + // dangerous after a SECOND parse. + {"YAML-escaped NUL in the title", []byte("---\npad_artifact: convention\nformat_version: 1\ntitle: \"a" + string([]byte{'\\', '0'}) + "b\"\n---\n\nbody\n")}, } { t.Run(tc.name, func(t *testing.T) { rr := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/import-artifact", tc.body) diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index b0b327cbb..e30468d07 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -352,16 +352,21 @@ func validQueryText(rawQuery string) bool { // this source and the compiler can transform it into the character it // describes — the same reason the tests construct it this way. // -// It is the ONLY spelling. JSON forbids an unescaped control character inside -// a string, so a raw 0x00 byte never survives decoding (encoding/json answers -// `invalid character '\x00' in string literal`), and the uppercase \U form is -// not a JSON escape at all (`invalid character 'U' in string escape code`). -// Both measured against encoding/json, BUG-2803. +// It is the only spelling OF THE ESCAPE ITSELF. JSON forbids an unescaped +// control character inside a string, so a raw 0x00 byte never survives +// decoding (encoding/json answers `invalid character '\x00' in string +// literal`), and the uppercase \U form is not a JSON escape at all +// (`invalid character 'U' in string escape code`). Both measured against +// encoding/json, BUG-2803. +// +// THAT SENTENCE DOES NOT MAKE THE SUBSTRING A SOUND FILTER, which is the +// mistake this comment used to encode — see bodyDecodesNUL's gate. var jsonNULEscape = []byte{'\\', 'u', '0', '0', '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 — decodes to a -// string containing a NUL. +// 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 @@ -374,23 +379,43 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // job. BUG-2784 recorded this as the reason its rule stops at the query // string; this is the missing half. // -// WHY IT IS NOT A SUBSTRING SEARCH. Containing jsonNULEscape is necessary but -// NOT sufficient: `\\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 substring is used as a FAST PATH only, and it is sound in that -// direction: a body that does not contain it cannot decode to a NUL anywhere, -// because the escape is the only spelling (see jsonNULEscape). Bodies -// containing it — rare, and already unusual — pay for an exact answer. -// -// THE EXACT STEP IS json.Decoder.Token(), which hands back the DECODED string -// for every key and value in the document. It distinguishes the two cases -// above by construction rather than by pattern, it needs no knowledge of the -// destination type, and it reaches nested maps such as an item's `fields` -// blob, which a struct-shaped check would miss. +// 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 jsonNULEscape?" — 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 +// jsonNULEscape is not sufficient: `\\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 @@ -408,7 +433,7 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // 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. func bodyDecodesNUL(raw []byte) bool { - if !bytes.Contains(raw, jsonNULEscape) { + if !bytes.ContainsRune(raw, '\\') { return false } var v any @@ -495,11 +520,11 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { if strings.ContainsRune(t, 0) { return true } - if !inJSONEncodedField || !strings.Contains(t, string(jsonNULEscape)) { + if !inJSONEncodedField || !strings.ContainsRune(t, '\\') { return false } if depth >= maxJSONDocumentNesting { - // The escape IS present and we have stopped looking. Refusing is + // Escapes ARE present and we have stopped looking. Refusing is // the safe direction: the alternative is to pass a document we // declined to inspect. return true From 336d9784426fcc512b3e8b88dedceacb1c3fe4a9 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 16:06:54 +0000 Subject: [PATCH 08/29] fix(server): multipart text fields and the bundle manifest (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4's two P2s. Both are the same shape as the rest: a caller-supplied string reaching a text comparison through a door the earlier fixes did not cover. 1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from the JSON rule — its payload is binary blob content and must not be scanned for text validity — but its TEXT fields are a different thing. `item_id` goes to ResolveItem and into a database comparison exactly as the query-string channel does, and that channel has been validated at the transport since BUG-2784; the form channel was not. multipartValues now drops values that are not bindable text, which makes an unusable value indistinguishable from an absent one — the disposition resolveUploadItemID already applies to empty values. The uploaded FILENAME gets the same predicate, with a fallback to a generic name rather than a refusal: the bytes are fine, only the label is unusable. A NEGATIVE RESULT worth recording, because it changed the test: a RAW NUL in the multipart header is NOT the vector. Go's multipart reader refuses it as a malformed MIME header line before any handler sees it (measured: 400, "malformed MIME header line"). The reachable spelling is the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the header parser accepts and percent-decodes afterwards. The first version of this test used the raw form and was testing a vector that does not exist. 2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the tar.gz, parsed directly like pad-export.json was, so it needed the same check. Without it a NUL in a manifest string reached rehydrateAttachment, whose failure is logged and SKIPPED — so the import reported success while silently dropping the attachment. The skip-on-failure behaviour is pre-existing and deliberate (a partial restore beats none); refusing the bad INPUT is what stops it being reached this way. Left as it is, and named rather than quietly changed. A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would otherwise have shipped as coverage: the filename leg first asserted `!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL in the filename comes back as the six-character escape, not as a 0x00, so the check passed whether or not the fix was present. It did pass with the fallback disabled. Now it decodes the response and asserts the replacement name. The item_id leg had the mirror-image weakness: it asserted "not a 500", which is the Postgres-only symptom, so on SQLite it would have passed either way; it now asserts the request behaves exactly like the no-value control. Every fix in this commit has a mutation against it, and each kills only its own leg. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/handlers_attachments.go | 35 +++++- internal/server/handlers_attachments_test.go | 124 +++++++++++++++++++ internal/server/handlers_import_bundle.go | 12 ++ 3 files changed, 170 insertions(+), 1 deletion(-) diff --git a/internal/server/handlers_attachments.go b/internal/server/handlers_attachments.go index 812830b0e..12b0d104a 100644 --- a/internal/server/handlers_attachments.go +++ b/internal/server/handlers_attachments.go @@ -46,11 +46,35 @@ const multipartParseMemory = 1 << 20 // 1 MiB // ONLY. Unlike (*http.Request).FormValue it does not fall back to the // URL query string, which keeps the two item_id input channels distinct // so they can be resolved and cross-checked independently. +// multipartValues returns a multipart form field's TEXT values, dropping any +// that are not bindable text. +// +// The multipart body is exempt from BUG-2803's JSON rule for a good reason — +// its payload is binary blob content and must not be scanned for text +// validity — but its TEXT fields are a different thing: item_id goes to +// ResolveItem and into a database comparison exactly as the query-string +// channel does, and that channel has been validated at the transport since +// BUG-2784. A raw NUL arriving through the form instead of the query reached +// the store unchecked (codex round 4, BUG-2803). +// +// Dropping rather than erroring keeps this helper's signature and matches +// what callers already do with absent values: resolveUploadItemID treats +// absent and explicitly-empty alike, so an unusable value becomes "no value" +// and the request is answered by the same path that handles a missing one. A +// value that cannot name anything and one that was never sent are the same +// thing to the caller. func multipartValues(r *http.Request, key string) []string { if r.MultipartForm == nil { return nil } - return r.MultipartForm.Value[key] + raw := r.MultipartForm.Value[key] + out := make([]string, 0, len(raw)) + for _, v := range raw { + if bindableText(v) { + out = append(out, v) + } + } + return out } // maxUploadItemIDValues bounds how many item_id values one input channel @@ -301,7 +325,16 @@ func (s *Server) handleUploadAttachment(w http.ResponseWriter, r *http.Request) // Sanitize the filename: strip path components so a client can't // sneak directory traversal through the display name. We don't // store this in the storage backend — only in the DB row for UI. + // The uploaded filename is caller-supplied text bound for a text column, + // and a multipart header can carry a NUL through the RFC 5987 encoded + // form (filename*=UTF-8\'\'a%00.png). Same predicate as the path and + // query rules; falling back to a generic name rather than refusing the + // upload, because the bytes are fine and only the label is unusable + // (codex round 4, BUG-2803). filename := filepath.Base(header.Filename) + if !bindableText(filename) { + filename = "upload" + } if filename == "" || filename == "." || filename == "/" { filename = "upload.bin" } diff --git a/internal/server/handlers_attachments_test.go b/internal/server/handlers_attachments_test.go index cb24f04de..c59aedfa3 100644 --- a/internal/server/handlers_attachments_test.go +++ b/internal/server/handlers_attachments_test.go @@ -11,6 +11,7 @@ import ( "mime/multipart" "net/http" "net/http/httptest" + "net/textproto" "net/url" "os" "strings" @@ -876,3 +877,126 @@ func TestUpload_QuotaCheckResolves(t *testing.T) { var _ = io.Discard var _ = models.Attachment{} var _ = strings.Contains + +// TestUpload_MultipartTextFieldsAreBindableText covers codex round 4's P2 on +// BUG-2803. The multipart body is deliberately exempt from the JSON NUL rule +// — its payload is binary blob content and must not be scanned for text +// validity — but its TEXT fields are a different thing. `item_id` goes to +// ResolveItem and into a database comparison exactly as the query-string +// channel does, and that channel has been validated at the transport since +// BUG-2784; the form channel was not. The FILENAME is the same shape: a +// multipart header can carry a NUL through the RFC 5987 encoded form. +// +// Both legs have a control that differs only in the bad byte, so a pass +// cannot come from the upload failing for an unrelated reason. +func TestUpload_MultipartTextFieldsAreBindableText(t *testing.T) { + srv, slug := testServerWithAttachments(t) + + upload := func(t *testing.T, filename string, itemID *string) *httptest.ResponseRecorder { + t.Helper() + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + if itemID != nil { + // WriteField would reject nothing, which is the point: the raw + // bytes reach r.MultipartForm.Value verbatim. + _ = mw.WriteField("item_id", *itemID) + } + part, _ := mw.CreateFormFile("file", filename) + part.Write(realPNG()) + mw.Close() + + req := httptest.NewRequest("POST", "/api/v1/workspaces/"+slug+"/attachments", &buf) + req.Header.Set("Content-Type", mw.FormDataContentType()) + req.RemoteAddr = "127.0.0.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr + } + + // uploadWithRawDisposition writes the part header by hand so the test can + // use the RFC 5987 encoded filename form. A RAW NUL in the header is not + // the vector: Go's multipart reader refuses it as a malformed MIME header + // line before any handler sees it (measured — the request answers 400 + // with "malformed MIME header line"). The percent-encoded form is + // accepted by the header parser and decodes to the byte afterwards, which + // is what makes it the reachable spelling. + uploadWithRawDisposition := func(t *testing.T, disposition string) *httptest.ResponseRecorder { + t.Helper() + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + h := make(textproto.MIMEHeader) + h.Set("Content-Disposition", disposition) + h.Set("Content-Type", "image/png") + part, err := mw.CreatePart(h) + if err != nil { + t.Fatalf("create part: %v", err) + } + part.Write(realPNG()) + mw.Close() + + req := httptest.NewRequest("POST", "/api/v1/workspaces/"+slug+"/attachments", &buf) + req.Header.Set("Content-Type", mw.FormDataContentType()) + req.RemoteAddr = "127.0.0.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr + } + + t.Run("filename carrying a NUL does not reach the attachment row", func(t *testing.T) { + control := uploadWithRawDisposition(t, + `form-data; name="file"; filename*=UTF-8''clean.png`) + if control.Code != http.StatusCreated && control.Code != http.StatusOK { + t.Fatalf("control upload must succeed, got %d: %s", control.Code, control.Body.String()) + } + if !strings.Contains(control.Body.String(), "clean.png") { + t.Fatalf("control should keep its encoded filename, got %s", control.Body.String()) + } + + rr := uploadWithRawDisposition(t, + `form-data; name="file"; filename*=UTF-8''sh%00ot.png`) + if rr.Code != http.StatusCreated && rr.Code != http.StatusOK { + t.Fatalf("upload with an unusable filename should still succeed (the BYTES are fine), got %d: %s", + rr.Code, rr.Body.String()) + } + // Assert the REPLACEMENT, not the absence of a raw NUL byte. The + // response is JSON, so a NUL in the filename comes back as the + // six-character escape rather than as a 0x00 — a ContainsRune(body, + // 0) check passes whether or not the fix is present, and it did: + // disabling the fallback left this leg green until the assertion was + // changed. CONVE-12, caught by the mutation rather than by review. + var got struct { + Filename string `json:"filename"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &got); err != nil { + t.Fatalf("decode upload response: %v (body %s)", err, rr.Body.String()) + } + if strings.ContainsRune(got.Filename, 0) { + t.Errorf("the stored filename still carries a NUL: %q", got.Filename) + } + if got.Filename != "upload" { + t.Errorf("expected the generic fallback name, got %q", got.Filename) + } + }) + + t.Run("item_id carrying a NUL is not resolved", func(t *testing.T) { + // A NUL-bearing item_id cannot name anything, so it must be treated + // as no value rather than handed to the store. The control is a + // syntactically fine but non-existent ref, which the handler answers + // with a 4xx — if the NUL leg produced a 500 instead, the value + // reached the database. + // Assert it behaves like NO value, not merely "not a 500". A 500 is + // the Postgres-only symptom; on SQLite an unfiltered value would + // instead resolve to nothing and answer 4xx, so a >=500 check would + // pass on this backend whether or not the filter exists. + control := upload(t, "clean.png", nil) + if control.Code != http.StatusCreated && control.Code != http.StatusOK { + t.Fatalf("control (no item_id) must succeed, got %d: %s", control.Code, control.Body.String()) + } + bad := "TASK-1\x00" + rr := upload(t, "clean.png", &bad) + if rr.Code != control.Code { + t.Errorf("an unusable item_id must be treated as no value (like the control, %d), got %d: %s", + control.Code, rr.Code, rr.Body.String()) + } + }) +} diff --git a/internal/server/handlers_import_bundle.go b/internal/server/handlers_import_bundle.go index f66e4f8bd..64c0c5351 100644 --- a/internal/server/handlers_import_bundle.go +++ b/internal/server/handlers_import_bundle.go @@ -336,6 +336,18 @@ func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID if err != nil { return ws, fmt.Errorf("read manifest.json: %w", err) } + // The manifest is a second JSON document inside the bundle, + // parsed here rather than through decodeJSON, so it needs the + // same check pad-export.json gets a few lines up. Without it a + // NUL in a manifest string reached rehydrateAttachment, whose + // failure is logged and SKIPPED below — so the import reported + // success while silently dropping the attachment (codex round 4, + // BUG-2803). The skip-on-failure behaviour is pre-existing and + // deliberate (a partial restore beats none); refusing the bad + // INPUT is what stops it from being reached this way. + if bodyDecodesNUL(buf) { + return ws, fmt.Errorf("manifest decode: %w (workspace created but attachments not restored)", errJSONBodyNUL) + } var manifest models.AttachmentManifest if err := json.Unmarshal(buf, &manifest); err != nil { return ws, fmt.Errorf("manifest decode: %w (workspace created but attachments not restored)", err) From c04d16e7f3db702fabb46d50dd9f79e9d1197c8e Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 16:10:41 +0000 Subject: [PATCH 09/29] refactor(server): drop the now-unused escape constant (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate became a backslash check, which was the last production use of jsonNULEscape; golangci-lint's unused check failed on the next run. Its documentation was load-bearing, so the explanation moved into bodyDecodesNUL's comment rather than being deleted with the variable — including the distinction that made the old gate wrong (the escape has one spelling INSIDE a decoded string, which is not a claim about the raw bytes). Caught by re-running lint on the tip after the previous commit rather than trusting the run from the tip before it. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/middleware_request_text.go | 25 ++++++---------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index e30468d07..76b6598a1 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -347,22 +347,6 @@ func validQueryText(rawQuery string) bool { return true } -// jsonNULEscape is the six-byte JSON escape that decodes to a NUL. It is -// built from bytes rather than written as a literal so that no layer between -// this source and the compiler can transform it into the character it -// describes — the same reason the tests construct it this way. -// -// It is the only spelling OF THE ESCAPE ITSELF. JSON forbids an unescaped -// control character inside a string, so a raw 0x00 byte never survives -// decoding (encoding/json answers `invalid character '\x00' in string -// literal`), and the uppercase \U form is not a JSON escape at all -// (`invalid character 'U' in string escape code`). Both measured against -// encoding/json, BUG-2803. -// -// THAT SENTENCE DOES NOT MAKE THE SUBSTRING A SOUND FILTER, which is the -// mistake this comment used to encode — see bodyDecodesNUL's gate. -var jsonNULEscape = []byte{'\\', 'u', '0', '0', '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 @@ -373,7 +357,10 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // 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 jsonNULEscape — six ordinary ASCII characters — so +// 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 @@ -382,7 +369,7 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // 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 jsonNULEscape?" — is +// 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 @@ -405,7 +392,7 @@ var jsonNULEscape = []byte{'\\', 'u', '0', '0', '0', '0'} // which is the cost of being correct here. // // WHY THE EXACT STEP IS NOT A SUBSTRING SEARCH EITHER. Containing -// jsonNULEscape is not sufficient: `\\u0000` (an escaped backslash followed +// 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, From e94e57f66131613c3141d3647d8a400207822db2 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 16:34:42 +0000 Subject: [PATCH 10/29] fix(server): rune-safe truncation and User-Agent sanitising (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 5 was asked for the POPULATION rather than a confirmation — "enumerate every remaining way a caller-supplied string can reach a database text or jsonb parameter without passing a validity check" — and returned three residual classes with their sinks. Two are fixed here; the third is filed, because measuring it needs a fixture this unit should not grow. 1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string with a plain byte slice (name[:120], input.Name[:200]). If the boundary lands inside a multi-byte rune the result ends in a partial sequence and is no longer valid UTF-8 — so a value that PASSED the body check a few frames earlier arrives at the store unbindable, and Postgres answers 22021 for a request the server already accepted. This is the interesting one, because no input-side round could have found it: the defect is downstream of validation, and it is invisible with ASCII fixtures, which is what every test in that area used. truncateBindableText walks back off continuation bytes and drops the straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one walk-back cannot pass them all, and with a counterfactual leg asserting the naive slice really does produce unbindable output for the same input — without it the cases would pass against an implementation that did nothing. 2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent (three document paths, the connected-apps revoke) and sessions.user_agent (three login paths), and no rule here sees a header. The disposition is SANITISE, not refuse, and that is deliberate: a header is metadata this server chose to record, not something the caller asked for, so a malformed one must not turn an otherwise fine request into a 400. The two sites that HASH the header are left alone — sha256 over arbitrary bytes is well defined, and changing what is hashed would invalidate every stored UAHash. The filing's own earlier probe had recorded User-Agent as NOT reproducing on the item-create path. That was true and did not generalise; these are different sinks. 3. NOT FIXED, FILED: the OAuth form-encoded bodies (/oauth/token, /oauth/authorize/decide, /oauth/revoke, /oauth/introspect) parse url-encoded form data outside the shared body validator, with connection_name reaching oauth_connections.name and client_id reaching the oauth_clients.id lookup. This was the ORIGINAL subject of BUG-2803 before the filing was re-scoped, and it was recorded then as unreachable without a fosite-backed fixture. That is still true, and round 5's sink list is far more than the filing had. Filed rather than guessed at. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 94 ++++++++++++++++++++++ internal/server/handlers_auth.go | 6 +- internal/server/handlers_cloud.go | 2 +- internal/server/handlers_connected_apps.go | 4 +- internal/server/handlers_documents.go | 6 +- internal/server/handlers_oauth.go | 4 +- internal/server/middleware_request_text.go | 51 ++++++++++++ 7 files changed, 156 insertions(+), 11 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 362bd8fe0..8c84549f0 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -480,3 +480,97 @@ func TestDecodeJSONRefusesNestedNULThroughTheHandler(t *testing.T) { bad.Code, bad.Body.String()) } } + +// TestTruncateBindableText covers codex round 5's second class on BUG-2803: +// a value that PASSED the body check becomes unbindable on its way to the +// store, because a plain s[:n] can split a rune and leave a partial sequence. +// +// The failure is invisible with ASCII fixtures, which is why four review +// rounds aimed at the input side did not reach it — so these cases are built +// from multi-byte runes deliberately, and each asserts the OUTPUT is valid +// UTF-8 rather than merely short. +func TestTruncateBindableText(t *testing.T) { + // é is 2 bytes, 中 is 3, 𝄞 is 4 — one case per continuation length, so a + // walk-back that is off by one cannot pass them all. + for _, tc := range []struct { + name string + s string + limit int + }{ + {"two-byte rune straddling the cut", strings.Repeat("é", 80), 121}, + {"three-byte rune straddling the cut", strings.Repeat("中", 80), 121}, + {"four-byte rune straddling the cut", strings.Repeat("𝄞", 80), 121}, + {"cut exactly on a boundary", strings.Repeat("é", 80), 120}, + {"ascii", strings.Repeat("a", 300), 120}, + {"shorter than the limit", "café", 120}, + {"limit of zero", "café", 0}, + } { + t.Run(tc.name, func(t *testing.T) { + got := truncateBindableText(tc.s, tc.limit) + if len(got) > tc.limit { + t.Errorf("result is %d bytes, over the %d limit", len(got), tc.limit) + } + // The point of the whole helper: the RESULT must still be + // storable. A plain slice fails exactly here. + if !bindableText(got) { + t.Errorf("result is not bindable text: %q", got) + } + if !strings.HasPrefix(tc.s, got) { + t.Errorf("result %q is not a prefix of the input", got) + } + }) + } + + // The counterfactual, stated as a test rather than as a comment: the + // naive slice this replaces really does produce unbindable output for the + // same input. Without this leg the cases above would pass against an + // implementation that simply returned the input unchanged when short — + // they would never demonstrate that anything was wrong. + s := strings.Repeat("é", 80) + if bindableText(s[:121]) { + t.Fatal("the fixture cannot reproduce the defect: a plain byte slice of it is still valid UTF-8, " + + "so this test would pass against a broken implementation") + } +} + +// TestRequestUserAgentIsBindableText covers codex round 5's third class on +// BUG-2803: the User-Agent header reaches activities.user_agent and +// sessions.user_agent as text, and no rule in this file sees a header. +// +// The disposition here is SANITISE, not refuse — a header is metadata this +// server chose to record, not something the caller asked for, so a malformed +// one must not turn a fine request into a 400. That difference from every +// other check in this file is the thing worth pinning. +func TestRequestUserAgentIsBindableText(t *testing.T) { + for _, tc := range []struct { + name string + ua string + want string + }{ + {"ordinary", "pad-cli/1.0", "pad-cli/1.0"}, + {"non-ascii is preserved", "café/1.0 中", "café/1.0 中"}, + {"invalid UTF-8 is dropped", "pad\xffcli", "padcli"}, + {"NUL is dropped", "pad\x00cli", "padcli"}, + {"empty stays empty", "", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set("User-Agent", tc.ua) + got := requestUserAgent(req) + if got != tc.want { + t.Errorf("requestUserAgent = %q, want %q", got, tc.want) + } + if !bindableText(got) { + t.Errorf("result is not bindable text: %q", got) + } + }) + } + + // The counterfactual: the raw header really is unbindable, so these cases + // would not pass against a helper that simply returned it unchanged. + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set("User-Agent", "pad\xffcli") + if bindableText(req.Header.Get("User-Agent")) { + t.Fatal("the fixture cannot reproduce the defect: the raw header is already bindable text") + } +} diff --git a/internal/server/handlers_auth.go b/internal/server/handlers_auth.go index 70c19e94b..8fceec9b7 100644 --- a/internal/server/handlers_auth.go +++ b/internal/server/handlers_auth.go @@ -308,7 +308,7 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h "user_id", user.ID, "error", err) } - token, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL) + token, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), webSessionTTL) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Credentials updated but failed to refresh session. Please sign in again.") @@ -329,7 +329,7 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h } func (s *Server) createAuthSession(w http.ResponseWriter, r *http.Request, user *models.User, ttl time.Duration) (string, error) { - token, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), ttl) + token, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), ttl) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to create session") return "", err @@ -1313,7 +1313,7 @@ func (s *Server) handleResetPassword(w http.ResponseWriter, r *http.Request) { } // Create a fresh session so the user is logged in - sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL) + sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), webSessionTTL) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Password updated but failed to create session") return diff --git a/internal/server/handlers_cloud.go b/internal/server/handlers_cloud.go index 08a866cc0..7083fbb4e 100644 --- a/internal/server/handlers_cloud.go +++ b/internal/server/handlers_cloud.go @@ -205,7 +205,7 @@ func (s *Server) handleOAuthLogin(w http.ResponseWriter, r *http.Request) { // 4. Sanitize inputs input.Name = strings.TrimSpace(input.Name) if len(input.Name) > 200 { - input.Name = input.Name[:200] + input.Name = truncateBindableText(input.Name, 200) } if input.AvatarURL != "" { if u, err := url.Parse(input.AvatarURL); err != nil || (u.Scheme != "http" && u.Scheme != "https") { diff --git a/internal/server/handlers_connected_apps.go b/internal/server/handlers_connected_apps.go index fce406307..b5ad18a8c 100644 --- a/internal/server/handlers_connected_apps.go +++ b/internal/server/handlers_connected_apps.go @@ -217,7 +217,7 @@ func (s *Server) handleRevokeConnectedApp(w http.ResponseWriter, r *http.Request UserID: user.ID, Metadata: string(metaJSON), IPAddress: clientIP(r), - UserAgent: r.UserAgent(), + UserAgent: requestUserAgent(r), }); err != nil { slog.Warn("connected-apps: audit log write failed", "error", err, "user_id", user.ID, "connection_id", id) } @@ -359,7 +359,7 @@ func (s *Server) handleRenameConnectedApp(w http.ResponseWriter, r *http.Request } name := strings.TrimSpace(body.Name) if len(name) > 120 { - name = name[:120] + name = truncateBindableText(name, 120) } if err := s.store.RenameConnection(id, name); err != nil { writeInternalError(w, err) diff --git a/internal/server/handlers_documents.go b/internal/server/handlers_documents.go index 30b14e85e..c2490c206 100644 --- a/internal/server/handlers_documents.go +++ b/internal/server/handlers_documents.go @@ -423,7 +423,7 @@ func (s *Server) logActivityWithMetaReturningID(workspaceID, documentID, action Metadata: metadata, UserID: uid, IPAddress: clientIP(r), - UserAgent: r.Header.Get("User-Agent"), + UserAgent: requestUserAgent(r), }) // Bump last_write_at on the actor. Every action that flows through this // helper (created/updated/archived/restored/moved/commented) is a write. @@ -454,7 +454,7 @@ func (s *Server) logAuditEventForUser(action string, r *http.Request, userID str Metadata: metadata, UserID: userID, IPAddress: clientIP(r), - UserAgent: r.Header.Get("User-Agent"), + UserAgent: requestUserAgent(r), }) } @@ -483,7 +483,7 @@ func (s *Server) logWorkspaceAuditEvent(workspaceID, action string, r *http.Requ Metadata: metadata, UserID: currentUserID(r), IPAddress: clientIP(r), - UserAgent: r.Header.Get("User-Agent"), + UserAgent: requestUserAgent(r), }) } diff --git a/internal/server/handlers_oauth.go b/internal/server/handlers_oauth.go index 709d10a76..8b50a7e9a 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -987,7 +987,7 @@ func (s *Server) parseConsentPayload(r *http.Request, ar fosite.AuthorizeRequest // prompts the user to name it on first visit. name := strings.TrimSpace(r.FormValue("connection_name")) if len(name) > 120 { - name = name[:120] + name = truncateBindableText(name, 120) } // may_create_workspaces is a checkbox — present (value="1") if @@ -1784,7 +1784,7 @@ func (s *Server) renderConsent(w http.ResponseWriter, r *http.Request, ar fosite // cap in PG, but 120 keeps the connections-page card width sane). suggested := strings.TrimSpace(r.URL.Query().Get("suggested_name")) if len(suggested) > 120 { - suggested = suggested[:120] + suggested = truncateBindableText(suggested, 120) } data := consentData{ diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 76b6598a1..ab0747fcf 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -593,3 +593,54 @@ func readBodyForDecode(r *http.Request, maxBytes int64) ([]byte, error) { 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 sinks are text columns: activities.user_agent (documents and the +// connected-apps revoke path) and sessions.user_agent (three login paths). +// The two sites that HASH the header instead are left alone — sha256 over +// arbitrary bytes is well defined, and changing what is hashed would +// invalidate every stored UAHash. +// +// 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 { + ua := strings.ToValidUTF8(r.Header.Get("User-Agent"), "") + return strings.ReplaceAll(ua, "\x00", "") +} From 73ae90236a0e5447a083bdfefdc2eaf7c0a85da1 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 17:26:05 +0000 Subject: [PATCH 11/29] fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 6 plus one measurement of my own. Three changes, one of them a revert of something I got wrong in the previous commit. 1. THE GATE COST TOO MUCH, so it is narrower and still sound. The previous commit gated the walk on "does the raw body contain a backslash", which is correct but catches every body carrying nested JSON (each `\"` is a backslash). Measured on a ~377 KB import-shaped body: 60106 allocs/op with that gate versus 30073 with the walk disabled — the walk was running on ordinary traffic. The gate is now the four bytes that begin any \u escape for a character below U+0100. The argument: to manufacture the six-character NUL escape inside a decoded string, each of its characters arrives either literally from the raw bytes — in which case the raw contains the escape, which begins with that prefix — or from a \u escape of its own, and the three characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit below U+0100, so those escapes begin with it too. Back to 30073 allocs/op, identical to the walk-disabled build. That argument is the same KIND of reasoning that was wrong two rounds ago, so it does not stand on its own: a differential test runs the gated function against an UNGATED walk over a corpus built to attack it — oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a doubled backslash — and fails on any disagreement. It also asserts the corpus contains both answers, since agreement over a one-sided corpus would be vacuous. Reverting the gate to the old substring fails it. 2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and `fields` accept both a JSON-encoded STRING and their natural array/object form, and the walk propagated "this subtree is JSON-encoded" into containers — so a free-form tag whose whole value happened to be a JSON document was refused, though nothing re-parses it. Measured: refused before, accepted now, while the JSON-encoded spelling of the same field is still refused. The flag now marks only a direct STRING child of a listed key. 3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser last commit, before reading store.CreateSession. It HASHES the header and stores no text — the round-5 enumeration named "sessions.user_agent" and I took the name for a column. The change would have been actively harmful: login would store sha256(sanitised) while middleware_auth still compares sha256(RAW), so every session from a client with a non-UTF-8 User-Agent would fail validation. A sink named in a review is a pointer to verify, not a finding. The real sink is activities.user_agent, from three document paths and the connected-apps revoke. 4. And the wiring leg codex asked for, on that real sink: a request through the router with a malformed header, reading the STORED value out of the activities row, with a control asserting an ordinary header is kept VERBATIM. Unwiring the production call site fails it; the helper's unit test does not notice. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 163 +++++++++++++++++++++ internal/server/handlers_auth.go | 6 +- internal/server/middleware_request_text.go | 40 ++++- 3 files changed, 198 insertions(+), 11 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 8c84549f0..b68a20ebb 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -2,6 +2,7 @@ package server import ( "bytes" + "database/sql" "encoding/json" "errors" "io" @@ -348,6 +349,18 @@ func TestBodyDecodesNULNestedDocuments(t *testing.T) { `{"title":"x","fields":` + jsonEncode(t, innerPlain) + `}`, false}, {"nested document whose escape is a DOUBLED backslash", `{"fields":` + jsonEncode(t, innerLiteral) + `}`, false}, + // codex round 6: these fields also accept their NATURAL shape, in + // which the elements are ordinary strings the server marshals itself. + // Nothing re-parses them, so an element that merely LOOKS like a + // document must not be treated as one — this refused a free-form tag + // whose whole value happened to be JSON. + {"tags as a natural ARRAY whose element is a JSON document", + `{"title":"x","tags":["release",` + jsonEncode(t, innerWithNUL) + `]}`, false}, + {"fields as a natural OBJECT whose value is a JSON document", + `{"title":"x","fields":{"k":` + jsonEncode(t, innerWithNUL) + `}}`, false}, + // ...while the JSON-ENCODED spelling of the same field still is. + {"tags as a JSON-encoded STRING carrying the escape", + `{"title":"x","tags":` + jsonEncode(t, `["a`+esc+`b"]`) + `}`, true}, {"a string that starts like JSON but does not parse", `{"content":` + jsonEncode(t, `{"k":"a`+esc+`b"`) + `}`, false}, {"prose mentioning the escape is not a JSON document", @@ -574,3 +587,153 @@ func TestRequestUserAgentIsBindableText(t *testing.T) { t.Fatal("the fixture cannot reproduce the defect: the raw header is already bindable text") } } + +// TestBodyDecodesNULGateAgreesWithAnUngatedWalk is the instrument that keeps +// the fast path honest. +// +// The gate is an argument, not an observation: to manufacture the +// six-character NUL escape inside a decoded string, every one of its +// characters must arrive either literally from the raw bytes — in which case +// the raw contains the escape, which begins with \u00 — or from a \u escape +// of its own, and the three characters involved (backslash U+005C, 'u' +// U+0075, '0' U+0030) all sit below U+0100, so every such escape also begins +// with \u00. Hence: no \u00 in the raw bytes, no NUL at any depth, however +// spelled. +// +// The FIRST version of that argument was wrong in exactly this way — it said +// "the escape has only one spelling", which is true inside a decoded string +// and false of the raw bytes, and codex round 4 turned that into a live +// bypass. So the argument does not get to stand on its own reasoning: this +// test runs the gated function against an UNGATED walk over a corpus built to +// attack it, and any disagreement is a bypass. +func TestBodyDecodesNULGateAgreesWithAnUngatedWalk(t *testing.T) { + ungated := func(raw []byte) bool { + var v any + if err := json.Unmarshal(raw, &v); err != nil { + return false + } + return valueDecodesNUL(v, false, 0) + } + + esc := escNULLiteral // the six-character NUL escape + bs := string([]byte{'\\', 'u', '0', '0', '5', 'c'}) // escapes to a BACKSLASH + bsUpper := string([]byte{'\\', 'u', '0', '0', '5', 'C'}) // same, upper-case hex + uEsc := string([]byte{'\\', 'u', '0', '0', '7', '5'}) // escapes to the letter u + zero := string([]byte{'\\', 'u', '0', '0', '3', '0'}) // escapes to the digit 0 + doubled := `\\` + "u0000" // literal text, no NUL + + corpus := []string{ + `{"title":"plain"}`, + `{"title":"quotes \" and a backslash \\ but no escape"}`, + `{"fields":"{\"k\":\"plain\"}"}`, + `{"fields":"{\"k\":\"a` + esc + `b\"}"}`, + `{"fields":"{\"k\":\"a` + bs + `u0000b\"}"}`, + `{"fields":"{\"k\":\"a` + bsUpper + `u0000b\"}"}`, + `{"fields":"{\"k\":\"a\\` + uEsc + `0000b\"}"}`, + `{"fields":"{\"k\":\"a\\u00` + zero + `0b\"}"}`, + `{"fields":"{\"k\":\"a` + doubled + `b\"}"}`, + `{"content":"{\"k\":\"a` + esc + `b\"}"}`, + `{"title":"a` + esc + `b"}`, + `{"a` + esc + `b":"key"}`, + `{"tags":["ok","a` + esc + `b"]}`, + `{"fields":"[1,2,\"a` + esc + `b\"]"}`, + `{"title":"unicode é 中"}`, + `{"title":"a control escape that is not a NUL: \\u0001"}`, + `{"fields":"{\"k\":\"ab\"}"}`, + } + + for i, body := range corpus { + got, want := bodyDecodesNUL([]byte(body)), ungated([]byte(body)) + if got != want { + t.Errorf("corpus[%d] %s\n gated=%v ungated=%v — the fast path is a BYPASS for this input", + i, body, got, want) + } + } + + // The corpus must contain both answers, or agreement proves nothing. + var trues, falses int + for _, body := range corpus { + if ungated([]byte(body)) { + trues++ + } else { + falses++ + } + } + if trues < 3 || falses < 3 { + t.Fatalf("corpus is one-sided (%d refuse, %d accept); agreement would be vacuous", trues, falses) + } +} + +// TestDocumentActivityStoresBindableUserAgent is the WIRING leg for the +// User-Agent sanitiser (CONVE-19, and codex round 6's second finding: a unit +// test vouches for the helper, not for anything calling it). +// +// It drives a real request through the router with a malformed header and +// reads the STORED value out of the activities row, so reverting the +// production call site fails this even though the helper still works. +// +// It targets the ACTIVITY sink deliberately. The round-5 enumeration also +// named "sessions.user_agent", and I wired the three login paths to the +// sanitiser before reading store.CreateSession — which HASHES the header and +// stores no text at all. That change was reverted: login would have stored +// sha256(sanitised) while middleware_auth still compares sha256(RAW), +// breaking session validation for any client with a non-UTF-8 User-Agent. +// A sink named in a review is a pointer to verify, not a finding. +func TestDocumentActivityStoresBindableUserAgent(t *testing.T) { + srv := testServer(t) + ws := createWSForTest(t, srv) + + post := func(t *testing.T, ua, title string) { + t.Helper() + body, _ := json.Marshal(map[string]any{"title": title}) + req := httptest.NewRequest("POST", "/api/v1/workspaces/"+ws+"/documents/", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("User-Agent", ua) + req.RemoteAddr = "192.0.2.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("create document = %d: %s", rr.Code, rr.Body.String()) + } + } + + // Control first: an ordinary header must be stored VERBATIM, so a + // sanitiser that mangled everything would fail here rather than pass. + post(t, "pad-cli/1.0 café", "control doc") + post(t, "pad-cli/1.0 bad\xffbyte\x00here", "probe doc") + + rows, err := srv.store.DB().Query(`SELECT user_agent FROM activities WHERE user_agent IS NOT NULL`) + if err != nil { + t.Fatalf("read activities: %v", err) + } + defer rows.Close() + + var seenControl, seenSanitised int + for rows.Next() { + var ua sql.NullString + if err := rows.Scan(&ua); err != nil { + t.Fatalf("scan: %v", err) + } + if !ua.Valid { + continue + } + if !bindableText(ua.String) { + t.Errorf("a stored activity user_agent is not bindable text: %q", ua.String) + } + switch ua.String { + case "pad-cli/1.0 café": + seenControl++ + case "pad-cli/1.0 badbytehere": + seenSanitised++ + } + } + if err := rows.Err(); err != nil { + t.Fatalf("rows: %v", err) + } + if seenControl == 0 { + t.Error("the control header was not stored verbatim — the sanitiser is doing too much") + } + if seenSanitised == 0 { + t.Error("no activity carries the sanitised header; the call site is not wired to requestUserAgent") + } +} diff --git a/internal/server/handlers_auth.go b/internal/server/handlers_auth.go index 8fceec9b7..70c19e94b 100644 --- a/internal/server/handlers_auth.go +++ b/internal/server/handlers_auth.go @@ -308,7 +308,7 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h "user_id", user.ID, "error", err) } - token, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), webSessionTTL) + token, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Credentials updated but failed to refresh session. Please sign in again.") @@ -329,7 +329,7 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h } func (s *Server) createAuthSession(w http.ResponseWriter, r *http.Request, user *models.User, ttl time.Duration) (string, error) { - token, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), ttl) + token, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), ttl) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Failed to create session") return "", err @@ -1313,7 +1313,7 @@ func (s *Server) handleResetPassword(w http.ResponseWriter, r *http.Request) { } // Create a fresh session so the user is logged in - sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), webSessionTTL) + sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", "Password updated but failed to create session") return diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index ab0747fcf..299392989 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -347,6 +347,11 @@ func validQueryText(rawQuery string) bool { 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 @@ -420,7 +425,7 @@ func validQueryText(rawQuery string) bool { // 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. func bodyDecodesNUL(raw []byte) bool { - if !bytes.ContainsRune(raw, '\\') { + if !bytes.Contains(raw, unicodeEscapePrefix) { return false } var v any @@ -507,7 +512,7 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { if strings.ContainsRune(t, 0) { return true } - if !inJSONEncodedField || !strings.ContainsRune(t, '\\') { + if !inJSONEncodedField || !strings.Contains(t, string(unicodeEscapePrefix)) { return false } if depth >= maxJSONDocumentNesting { @@ -529,7 +534,19 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { if strings.ContainsRune(k, 0) { return true } - if valueDecodesNUL(sub, inJSONEncodedField || jsonEncodedFieldKeys[k], depth) { + // A listed key marks its value as a JSON document only when that + // value is a STRING. The same fields also accept their NATURAL + // shape — `"tags":["a","b"]`, `"fields":{"k":"v"}` — and in that + // shape the elements are ordinary strings the server marshals + // itself, so nothing re-parses them and an element that merely + // LOOKS like a document must not be treated as one. Propagating + // the flag into containers refused a free-form tag whose whole + // value happened to be a JSON document (codex round 6). + childEncoded := inJSONEncodedField + if _, isString := sub.(string); isString && jsonEncodedFieldKeys[k] { + childEncoded = true + } + if valueDecodesNUL(sub, childEncoded, depth) { return true } } @@ -631,11 +648,18 @@ func truncateBindableText(s string, maxBytes int) string { // fine, so this sanitizes rather than rejects, which is the opposite // disposition from the rest of this file and deliberately so. // -// The sinks are text columns: activities.user_agent (documents and the -// connected-apps revoke path) and sessions.user_agent (three login paths). -// The two sites that HASH the header instead are left alone — sha256 over -// arbitrary bytes is well defined, and changing what is hashed would -// invalidate every stored UAHash. +// 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 From 78716d679aa8b9280f0cbcdb8f0e92b4b2724bd3 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 17:45:43 +0000 Subject: [PATCH 12/29] fix(server): apply the key rule at every level, not once (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 7, both findings, the first confirmed by measurement. 1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into a JSON-encoded string it treated the WHOLE subtree below as JSON-encoded, so a value nested two levels down — an ordinary string inside a `fields` blob that happens to hold JSON text — was refused. That is a false rejection, and the measurement says so plainly. With the depth-2 check disabled, on Postgres 17: depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it) depth 2 (a string INSIDE the blob) -> 201 (accepted, no error) control -> 201 The handler parses `fields` ONCE. The inner text is re-escaped when the blob is written, so what Postgres receives has a doubled backslash and no escape at all. Only the document Postgres itself parses can carry a fatal one. The nested call now passes false rather than true, which makes this a KEY RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key INSIDE a document still recurses (pinned by a test), an ordinary one does not. Same correction as round 6's natural-shape fix, one level further in — I fixed the sibling case and left this one, which is CONVE-18's lesson about my own enumeration being a sample too. I checked whether anything re-parses a value inside the blob before loosening this, rather than assuming: `arguments` was the candidate, and parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather than a JSON string, so it is covered by the natural-shape rule and needs no second parse. 2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic client registration handler prefixed every decode failure with "Request body must be JSON". A body carrying a NUL is valid JSON, so that message sends a client hunting a syntax error it does not have. The two failures are now distinguished. Round 7 also reports no break in normal CLI, MCP or web-client request generation — they marshal JSON and encode paths and query parameters — which is the first thing any round has said about the client surface. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 14 ++++++++++++-- internal/server/handlers_oauth.go | 12 ++++++++++-- internal/server/middleware_request_text.go | 9 ++++++++- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index b68a20ebb..58df7f076 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -340,8 +340,18 @@ func TestBodyDecodesNULNestedDocuments(t *testing.T) { // check. {"escape spelled obliquely, via an escaped backslash", `{"fields":"{\"k\":\"a` + string([]byte{'\\', 'u', '0', '0', '5', 'c'}) + `u0000b\"}"}`, true}, - {"twice-encoded — a document inside a document", - `{"fields":` + jsonEncode(t, `{"inner":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, true}, + // Depth 2 under an ORDINARY key is ACCEPTED, and that is a measured + // decision rather than a gap: the handler parses `fields` once, so + // the inner text is re-escaped when the blob is written and Postgres + // never sees an escape. Probed on Postgres 17 with the check + // disabled — the same body imports 201. Only the document Postgres + // itself parses can carry a fatal one (codex round 7). + {"twice-encoded under an ordinary key is safe", + `{"fields":` + jsonEncode(t, `{"inner":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, false}, + // ...but a JSON-ENCODED key INSIDE the document still recurses, so + // this is a key rule applied at every level, not a depth limit. + {"twice-encoded under a JSON-encoded key still refuses", + `{"fields":` + jsonEncode(t, `{"schema":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, true}, // Controls. These must stay ACCEPTED: the recursion must not turn // "contains the six characters somewhere" into a refusal. diff --git a/internal/server/handlers_oauth.go b/internal/server/handlers_oauth.go index 8b50a7e9a..555848777 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -228,8 +228,16 @@ func (s *Server) handleOAuthRegister(w http.ResponseWriter, r *http.Request) { var input dcrRequest if err := decodeJSON(r, &input); err != nil { - writeDCRError(w, http.StatusBadRequest, "invalid_client_metadata", - "Request body must be JSON: "+err.Error()) + // Two different failures reach here and the message must not + // conflate them: a body that is not JSON, and a body that IS valid + // JSON but carries text the database cannot store (BUG-2803). The + // second one was being reported as "must be JSON", which sends a + // client looking for a syntax error it does not have (codex round 7). + msg := "Request body must be JSON: " + err.Error() + if errors.Is(err, errJSONBodyNUL) { + msg = err.Error() + } + writeDCRError(w, http.StatusBadRequest, "invalid_client_metadata", msg) return } diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 299392989..66ebedc41 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -528,7 +528,14 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { if err := json.Unmarshal([]byte(strings.TrimSpace(t)), &inner); err != nil { return false } - return valueDecodesNUL(inner, true, depth+1) + // false, not true: inside a re-parsed document the SAME key rule + // applies again. Inheriting it blanket-wise refused a value that is + // demonstrably safe — measured on Postgres, a NUL escape two levels + // deep imports 201, because the handler parses `fields` ONCE and the + // inner text is re-escaped when the blob is written, so Postgres + // never sees it as an escape. Only the document Postgres itself + // parses can carry a fatal one (codex round 7). + return valueDecodesNUL(inner, false, depth+1) case map[string]any: for k, sub := range t { if strings.ContainsRune(k, 0) { From d93a3737ec7a57f18c3ae0f8ea685ed21f99891d Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 18:03:52 +0000 Subject: [PATCH 13/29] fix(server): complete the artifact check, make the walk path-aware (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 8. It confirmed round 7's two fixes, then found two real defects and two inaccurate comments — the comment half being the angle the round was asked for. 1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText walked the decoded artifact by TYPE, so it never covered Provenance — whose strings are rendered into a Markdown footer appended to the stored content — and never matched Arguments, declared []map[string]any, a concrete slice type the walk's []any case does not match. A YAML NUL escape in either reached storage. It now MARSHALS the artifact and searches the output for the escape encoding/json produces. A type switch over a struct that grows is a list that goes stale in silence; marshalling covers every exported field, including ones added later. The one thing it cannot see is invalid UTF-8 (which marshals to U+FFFD), and it does not need to: step 2 rejects that in the request bytes, and YAML cannot manufacture it from valid input — its escapes name code points, where \0 names a NUL. Both new cases fail with the check disabled; the raw-byte cases still pass, killed by the raw check, so each leg is discriminating. 2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field literally named `schema` or `tags`. The walk consulted the wire-key list at every level, so `{"fields":{"schema":"..."}}` treated a user field name as a wire key and refused valid text holding a JSON example. The key list is now consulted only OUTSIDE caller data — not under a natural `fields` object, not inside an element of a `tags` array, not inside a re-parsed document. Combined with round 7's fix that makes the descent exactly one level deep BY CONSTRUCTION, which is why the depth counter is gone: with the flag no longer inherited, a bound could never fire, and dead protection reads as protection. The depth-bound test is replaced by one that pins the property directly — an escape IN the parsed document is refused, one BELOW it is accepted, and a wire-key-shaped user field does not restart the descent. 3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would believe without checking: - MaxBytesReader: Close FORWARDS to the underlying body rather than being a no-op, and with a nil writer there is no automatic 413 — the cap surfaces as a read error the callers turn into 400. Behaviour unchanged; only the claim was wrong. - parseArtifactRequest said "three checks" while implementing five, and its returns list omitted ErrArtifactUnbindableText. Both added by this branch, which is exactly the prose a change is most likely to falsify (CONVE-23). - errJSONBodyNUL claimed all 65 callers surface its message. The STATUS is uniform; the wording is not — several substitute a generic string. 4. And one in a test: the timeline fixture said both backends hold a CHECK constraint a raw NUL violates. items.fields is a plain TEXT column with no CHECK on SQLite. What was OBSERVED is "SQL logic error: malformed JSON"; the likely source is an expression index over json_extract, and that attribution is recorded as NOT verified rather than asserted. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/artifact_import.go | 95 ++++----- internal/server/decode_json_nul_test.go | 54 +++-- internal/server/handlers_artifact_test.go | 12 ++ ...andlers_timeline_cursor_validation_test.go | 16 +- internal/server/middleware_request_text.go | 186 +++++++++--------- internal/server/server.go | 12 +- 6 files changed, 215 insertions(+), 160 deletions(-) diff --git a/internal/server/artifact_import.go b/internal/server/artifact_import.go index ce4717f45..ea80d3a46 100644 --- a/internal/server/artifact_import.go +++ b/internal/server/artifact_import.go @@ -1,6 +1,8 @@ package server import ( + "bytes" + "encoding/json" "errors" "fmt" "io" @@ -57,18 +59,26 @@ var ErrArtifactUnsafeYAML = errors.New("artifact import: frontmatter rejected by var ErrArtifactUnbindableText = errors.New("artifact import: body contains invalid UTF-8 or a NUL byte") // parseArtifactRequest is the guarded HTTP-boundary parse used by the import -// handler. It applies three checks IN ORDER: +// handler. It applies five steps IN ORDER: // // 1. Byte cap on the raw body (http.MaxBytesReader), so an oversized body is // rejected before full materialization. -// 2. YAML-bomb guard: the frontmatter region is parsed into a yaml.Node tree +// 2. Raw text validity (bindableText), so invalid UTF-8 or a NUL BYTE is +// refused before anything parses it. +// 3. YAML-bomb guard: the frontmatter region is parsed into a yaml.Node tree // and walked, enforcing maxFrontmatterNodes / maxFrontmatterDepth / // maxFrontmatterAliases. This runs BEFORE the struct decode so an alias- // storm or deep-nesting document never reaches the expanding unmarshaler. -// 3. artifact.Decode, which produces the typed Artifact. +// 4. artifact.Decode, which produces the typed Artifact. +// 5. Decoded text validity, because YAML manufactures a NUL from \0 that +// step 2 cannot see in the request bytes. +// +// Steps 2 and 5 arrived with BUG-2803; this list said "three checks" until +// codex round 8 pointed out it was describing the version before them. // // Returns the decoded Artifact or a typed error: ErrArtifactTooLarge, -// ErrArtifactUnsafeYAML, or an artifact.* sentinel (ErrMalformed / +// ErrArtifactUnbindableText, ErrArtifactUnsafeYAML, or an artifact.* +// sentinel (ErrMalformed / // ErrUnknownKind / ErrUnsupportedVersion) wrapped for context. The import // handler maps these to HTTP statuses. func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64) (artifact.Artifact, error) { @@ -90,7 +100,7 @@ func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64 return artifact.Artifact{}, fmt.Errorf("artifact import: read body: %w", err) } - // (2a) The artifact body is TEXT bound for text columns, and this + // (2) The artifact body is TEXT bound for text columns, and this // handler reads it directly rather than through decodeJSON, so it // inherits neither BUG-2803's refusal nor the path/query rule (a body is // neither). A raw NUL or invalid UTF-8 here reaches the store and @@ -101,19 +111,19 @@ func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64 return artifact.Artifact{}, ErrArtifactUnbindableText } - // (2) YAML-bomb guard on the frontmatter region only. + // (3) YAML-bomb guard on the frontmatter region only. if err := guardArtifactFrontmatter(data); err != nil { return artifact.Artifact{}, err } - // (3) Typed decode. + // (4) Typed decode. art, err := artifact.Decode(data) if err != nil { return artifact.Artifact{}, err } - // (4) The DECODED artifact must be bindable text too — the raw check in - // (2a) is not sufficient on its own. YAML has its own escape vocabulary: + // (5) The DECODED artifact must be bindable text too — the raw check in + // (2) is not sufficient on its own. YAML has its own escape vocabulary: // a double-quoted scalar `title: "a\0b"` carries no NUL in the request // bytes, passes (2a), and manufactures one during the YAML decode. // Measured before this check: such an artifact imported 201 with a NUL in @@ -225,43 +235,40 @@ func extractFrontmatterRegion(s string) (string, bool) { } } -// artifactIsBindableText reports whether every string a decoded artifact would -// carry into the store is text the database can be asked to hold. Title, body -// and every frontmatter field value are checked; field values are walked -// because a playbook's `arguments` is a nested structure, not a scalar. +// artifactIsBindableText reports whether a decoded artifact carries a NUL in +// any string it would take into the store. +// +// It works by MARSHALLING the artifact and searching the result, rather than +// walking its fields by type. The hand-written walk this replaces missed two +// things a reviewer found immediately (codex round 8): Provenance, whose +// strings are rendered into a Markdown footer appended to the stored content, +// and Arguments, whose declared type is []map[string]any — a concrete slice +// type the walk's `[]any` case never matched. Both were reachable. A type +// switch over a struct that grows is a list that goes stale in silence, which +// is the same objection this file's jsonEncodedFieldKeys has to answer for +// and can only answer with a derivation test. Marshalling has no such gap: +// every exported field is covered, including ones added later. +// +// encoding/json escapes a NUL as the six-character sequence, so a decoded NUL +// anywhere in the artifact appears in the output. The search is for that +// sequence in bytes the MARSHALLER produced, not in caller-supplied text, so +// the ambiguity bodyDecodesNUL has to resolve — a doubled backslash meaning +// literal text — cannot arise here: a literal backslash in a value marshals +// to a doubled one. // -// Keys are checked as well as values, on the same precautionary grounds -// ValidateQuery states for query parameter names: no failure was observed -// through a key, and why it would survive is unread, so it is checked rather -// than assumed safe. +// SCOPE, stated because marshalling hides one thing: invalid UTF-8 in a Go +// string marshals to U+FFFD rather than surviving, so this cannot detect it. +// It does not need to. Step (2a) rejects invalid UTF-8 in the request bytes +// before the decode, and YAML cannot manufacture it from valid input — its +// escapes name code points (\xff is U+00FF, a valid rune), where \0 names a +// NUL. NUL is the class that survives the decode, and it is the class this +// checks. func artifactIsBindableText(art artifact.Artifact) bool { - if !bindableText(art.Title) || !bindableText(art.Body) { + encoded, err := json.Marshal(art) + if err != nil { + // An artifact that cannot be marshalled cannot be reasoned about; + // refuse rather than pass it on unexamined. return false } - for k, v := range art.Fields { - if !bindableText(k) || !anyValueIsBindableText(v) { - return false - } - } - return true -} - -func anyValueIsBindableText(v any) bool { - switch t := v.(type) { - case string: - return bindableText(t) - case map[string]any: - for k, sub := range t { - if !bindableText(k) || !anyValueIsBindableText(sub) { - return false - } - } - case []any: - for _, sub := range t { - if !anyValueIsBindableText(sub) { - return false - } - } - } - return true + return !bytes.Contains(encoded, []byte{'\\', 'u', '0', '0', '0', '0'}) } diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 58df7f076..fdb01a8b7 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -350,8 +350,11 @@ func TestBodyDecodesNULNestedDocuments(t *testing.T) { `{"fields":` + jsonEncode(t, `{"inner":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, false}, // ...but a JSON-ENCODED key INSIDE the document still recurses, so // this is a key rule applied at every level, not a depth limit. - {"twice-encoded under a JSON-encoded key still refuses", - `{"fields":` + jsonEncode(t, `{"schema":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, true}, + // ...and a key that LOOKS like a wire key inside caller data is not + // one: a collection may declare a user field named `schema`, so the + // list is consulted only outside caller data (codex round 8). + {"a user field named like a wire key does not restart the descent", + `{"fields":` + jsonEncode(t, `{"schema":`+jsonEncode(t, innerWithNUL)+`}`) + `}`, false}, // Controls. These must stay ACCEPTED: the recursion must not turn // "contains the six characters somewhere" into a refusal. @@ -453,21 +456,38 @@ func TestJSONEncodedFieldKeysCoversTheModels(t *testing.T) { } } -// TestBodyDecodesNULDepthBound pins the behaviour AT the recursion limit: -// past it the escape is known to be present and the walk has stopped looking, -// so the body is refused rather than passed uninspected. -func TestBodyDecodesNULDepthBound(t *testing.T) { - // Wrap a NUL-bearing document deeper than the limit allows. - // Nested under a JSON-ENCODED key at every level, since that is the only - // path the walk descends: nesting under an ordinary key would never start - // the recursion and the test would pass for the wrong reason. - doc := `{"k":"a` + escNULLiteral + `b"}` - for i := 0; i < maxJSONDocumentNesting+2; i++ { - doc = `{"fields":` + jsonEncode(t, doc) + `}` +// TestBodyDecodesNULDescendsExactlyOneLevel replaces an earlier depth-bound +// test. The walk no longer carries a depth counter, because it no longer +// needs one: the key list is consulted only outside caller data, so a +// document reached through a listed key is walked with the list disabled and +// nothing below it can start a second descent. +// +// This pins that property directly rather than pinning a bound, and it is the +// test that fails if someone reintroduces flag inheritance: an escape one +// level below the parsed document must be ACCEPTED (measured safe on Postgres +// — the inner text is re-escaped when the blob is written), while the same +// escape IN that document must be refused. +func TestBodyDecodesNULDescendsExactlyOneLevel(t *testing.T) { + inner := `{"k":"a` + escNULLiteral + `b"}` + + atTheParsedLayer := `{"fields":` + jsonEncode(t, inner) + `}` + if !bodyDecodesNUL([]byte(atTheParsedLayer)) { + t.Error("an escape in the document Postgres parses must be refused") + } + + oneDeeper := `{"fields":` + jsonEncode(t, `{"inner":`+jsonEncode(t, inner)+`}`) + `}` + if bodyDecodesNUL([]byte(oneDeeper)) { + t.Error("an escape BELOW the parsed document is safe and must be accepted") + } + + // A user field named like a wire key must not restart the descent. + shadowed := `{"fields":` + jsonEncode(t, `{"schema":`+jsonEncode(t, inner)+`}`) + `}` + if bodyDecodesNUL([]byte(shadowed)) { + t.Error("a user field named `schema` inside a fields blob is caller data, not a wire key") } - doc = `{"fields":` + jsonEncode(t, doc) + `}` - if !bodyDecodesNUL([]byte(doc)) { - t.Error("a body nested past maxJSONDocumentNesting must be refused, not passed uninspected") + natural := `{"fields":{"schema":` + jsonEncode(t, inner) + `}}` + if bodyDecodesNUL([]byte(natural)) { + t.Error("a user field named `schema` in a NATURAL fields object is caller data too") } } @@ -622,7 +642,7 @@ func TestBodyDecodesNULGateAgreesWithAnUngatedWalk(t *testing.T) { if err := json.Unmarshal(raw, &v); err != nil { return false } - return valueDecodesNUL(v, false, 0) + return valueDecodesNUL(v, false) } esc := escNULLiteral // the six-character NUL escape diff --git a/internal/server/handlers_artifact_test.go b/internal/server/handlers_artifact_test.go index 09b3f0167..8436b7788 100644 --- a/internal/server/handlers_artifact_test.go +++ b/internal/server/handlers_artifact_test.go @@ -609,6 +609,18 @@ func TestImportArtifactUnbindableTextRejected(t *testing.T) { // post-decode check existed: this imported 201 with a NUL in the // title. Same shape as the JSON half, where a value only becomes // dangerous after a SECOND parse. + // codex round 8: the first version of the post-decode check walked + // the artifact by TYPE and missed two reachable fields — arguments, + // whose declared type []map[string]any never matched the walk's + // []any case, and provenance, whose strings are rendered into a + // footer appended to the stored content. The check now marshals the + // artifact and searches the output, so every field is covered. + {"YAML-escaped NUL inside a playbook argument", []byte( + "---\npad_artifact: playbook\nformat_version: 1\ntitle: Args\narguments:\n - name: \"a" + + string([]byte{'\\', '0'}) + "b\"\n type: string\n---\n\nbody\n")}, + {"YAML-escaped NUL in provenance", []byte( + "---\npad_artifact: convention\nformat_version: 1\ntitle: Prov\nprovenance:\n workspace: \"a" + + string([]byte{'\\', '0'}) + "b\"\n author: someone\n---\n\nbody\n")}, {"YAML-escaped NUL in the title", []byte("---\npad_artifact: convention\nformat_version: 1\ntitle: \"a" + string([]byte{'\\', '0'}) + "b\"\n---\n\nbody\n")}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/server/handlers_timeline_cursor_validation_test.go b/internal/server/handlers_timeline_cursor_validation_test.go index 858465b8d..9c9a3b6a0 100644 --- a/internal/server/handlers_timeline_cursor_validation_test.go +++ b/internal/server/handlers_timeline_cursor_validation_test.go @@ -213,10 +213,18 @@ func TestTimeline_NeverEmitsACursorItWouldRefuse(t *testing.T) { item := timelineItemWithStructured(t, srv, ws, notes, "") // Swap the placeholder for the JSON NUL ESCAPE directly in the stored - // blob — the six characters, not a raw NUL byte. The blob is stored as - // JSON text and both backends hold a CHECK/type constraint that a raw NUL - // violates; the NUL only comes into existence when Go decodes the blob, - // which is precisely how the timeline ends up with one inside an entry id. + // blob — the six characters, not a raw NUL byte. The NUL only comes into + // existence when Go DECODES the blob, which is precisely how the timeline + // ends up with one inside an entry id. + // + // A raw NUL does not work here, and the reason is not the one this + // comment first gave: items.fields is a plain `TEXT NOT NULL DEFAULT + // '{}'` column with no CHECK constraint on SQLite. What was OBSERVED is + // that the update fails with "SQL logic error: malformed JSON"; the + // likely source is one of the expression indexes over json_extract(fields + // ...) rather than a column constraint, and that attribution is NOT + // verified. The claim corrected to what the run actually showed (codex + // round 8). // The API refuses this body (BUG-2803); the store does not, which is the // gap this test exists to cover. if _, err := srv.store.DB().Exec( diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 66ebedc41..e864a6011 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -434,47 +434,37 @@ func bodyDecodesNUL(raw []byte) bool { // function never has to phrase one. return false } - return valueDecodesNUL(v, false, 0) + return valueDecodesNUL(v, false) } -// maxJSONDocumentNesting bounds how many times valueDecodesNUL will descend -// into a string that is itself a JSON document. Each level must be a strict -// substring of the one above, so recursion terminates on its own; the bound -// keeps a hostile body from buying many full re-parses of a large payload. -// Eight is far past any shape this API produces — the deepest real case is -// one level, a `fields` blob inside a request body. -const maxJSONDocumentNesting = 8 - -// jsonEncodedFieldKeys are the wire keys whose STRING value is itself a JSON -// document that something downstream re-parses. They are the only keys under -// which valueDecodesNUL descends. +// 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 the test should -// be structural rather than destination-typed. Codex round 2 on BUG-2803 -// showed what that costs: a plain-text `content` value holding a JSON snippet -// that mentions the escape was ACCEPTED before this fix, is stored in a text +// 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 a value the server itself -// emitted, and that nothing downstream would choke on, is a worse failure -// than the narrow door the recursion was closing. +// 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. That is enumerable, and -// TestJSONEncodedFieldKeysCoversTheModels derives the set from -// internal/models and fails when a new one appears, so the list cannot go -// stale in silence. -// -// OVER-INCLUSION IS THE SAFE DIRECTION and this list deliberately takes it: a -// key listed here that is NOT actually JSON-encoded costs one parse attempt -// and can only refuse a value that IS a complete JSON document carrying the -// escape. A key MISSING from it reopens a door. `traits` is here 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. +// 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, @@ -488,78 +478,70 @@ var jsonEncodedFieldKeys = map[string]bool{ "traits": true, } -// valueDecodesNUL walks a decoded request body for a string that either -// CONTAINS a NUL or, under a JSON-encoded key, is a document whose own -// strings do. -// -// WHY A DECODED WALK RATHER THAN reflection over the destination struct. A +// 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 — +// 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, which is ASCII. 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. -// -// inJSONEncodedField is inherited by the whole subtree below a listed key: a -// document nested inside a JSON-encoded document is re-parsed just as its -// parent is. -func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { +// 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 { switch t := v.(type) { case string: - if strings.ContainsRune(t, 0) { - return true - } - if !inJSONEncodedField || !strings.Contains(t, string(unicodeEscapePrefix)) { - return false - } - if depth >= maxJSONDocumentNesting { - // Escapes ARE present and we have stopped looking. Refusing is - // the safe direction: the alternative is to pass a document we - // declined to inspect. - return true - } - if !stringIsJSONDocument(t) { - return false - } - var inner any - if err := json.Unmarshal([]byte(strings.TrimSpace(t)), &inner); err != nil { - return false - } - // false, not true: inside a re-parsed document the SAME key rule - // applies again. Inheriting it blanket-wise refused a value that is - // demonstrably safe — measured on Postgres, a NUL escape two levels - // deep imports 201, because the handler parses `fields` ONCE and the - // inner text is re-escaped when the blob is written, so Postgres - // never sees it as an escape. Only the document Postgres itself - // parses can carry a fatal one (codex round 7). - return valueDecodesNUL(inner, false, depth+1) + return strings.ContainsRune(t, 0) case map[string]any: for k, sub := range t { if strings.ContainsRune(k, 0) { return true } - // A listed key marks its value as a JSON document only when that - // value is a STRING. The same fields also accept their NATURAL - // shape — `"tags":["a","b"]`, `"fields":{"k":"v"}` — and in that - // shape the elements are ordinary strings the server marshals - // itself, so nothing re-parses them and an element that merely - // LOOKS like a document must not be treated as one. Propagating - // the flag into containers refused a free-form tag whose whole - // value happened to be a JSON document (codex round 6). - childEncoded := inJSONEncodedField - if _, isString := sub.(string); isString && jsonEncodedFieldKeys[k] { - childEncoded = true + if !inUserData && jsonEncodedFieldKeys[k] { + if str, isString := sub.(string); isString { + if 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, childEncoded, depth) { + if valueDecodesNUL(sub, inUserData) { return true } } case []any: for _, sub := range t { - if valueDecodesNUL(sub, inJSONEncodedField, depth) { + if valueDecodesNUL(sub, inUserData) { return true } } @@ -567,6 +549,21 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool { 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. +func nestedDocumentDecodesNUL(s string) bool { + if !strings.Contains(s, string(unicodeEscapePrefix)) || !stringIsJSONDocument(s) { + return false + } + var inner any + if err := json.Unmarshal([]byte(strings.TrimSpace(s)), &inner); err != nil { + return false + } + return valueDecodesNUL(inner, true) +} + // stringIsJSONDocument reports whether a string is a complete JSON object or // array — the shape a downstream consumer will re-parse. // @@ -612,8 +609,13 @@ func readBodyForDecode(r *http.Request, maxBytes int64) ([]byte, error) { if r.Body == nil { return nil, io.EOF } - // MaxBytesReader.Close() is a no-op; setting this also lets the server - // return 413 automatically via the error the caller wraps. + // 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) } diff --git a/internal/server/server.go b/internal/server/server.go index 952af91dd..0be99ac0a 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -2229,9 +2229,15 @@ func decodeJSONWithLimit(r *http.Request, v interface{}, maxBytes int64) error { // errJSONBodyNUL is returned by decodeJSON when a string in the request body // decodes to a value containing a NUL. Every decodeJSON caller already turns -// a decode error into a 400 carrying err.Error(), so this reaches the client -// as a client error with a message naming the cause, at all 65 call sites, -// without touching any of them. +// a decode error into a 400, so the refusal reaches the client as a client +// error at all 65 call sites without touching any of them. +// +// NOT every caller shows this message, and the earlier version of this +// comment claimed otherwise: many substitute a generic string of their own +// ("Invalid JSON body"). The status is uniform; the wording is not. Where a +// caller's own message would actively mislead — the OAuth registration +// endpoint's "Request body must be JSON", for a body that IS valid JSON — +// that caller distinguishes the two cases explicitly (codex round 8). // // The wording avoids writing the escape sequence literally: the message is // rendered in terminals, logs and a browser, and a literal escape in an error From b32b2293a328f8e96c50fd4e4df74ce144651323 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 18:13:10 +0000 Subject: [PATCH 14/29] fix(server): a regression this branch introduced, and the same trap again (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 9. Both findings are mine, one of them a regression from the round-8 restructure two commits ago. 1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the JSON-encoded branch for a listed key skipped the plain "does this string contain a NUL" check and asked only "does the document this string carries hold an escape". Those are different questions. So {"fields":"ab"} — a direct NUL in the fields value, the very first case this whole change closed — was accepted again. Both checks now run. The test pins all three legs: a direct NUL in the fields string, an escape inside the fields document, and an ordinary fields string that must still be accepted, so the first two cannot pass merely because everything under a listed key is refused. 2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It searched the MARSHALLED bytes for the escape sequence, and a value holding the six LITERAL characters marshals to a doubled backslash which still contains that sequence as a substring — so valid content was refused. Artifacts are documentation; text about a JSON escape is exactly what one carries. Worse than the bug: the comment I wrote asserted the ambiguity "cannot arise here". It was the same doubled-backslash case bodyDecodesNUL exists to resolve, one function away, and I wrote a sentence explaining why it did not apply instead of checking. The marshalled form is now decoded again and walked with the same machinery — the round trip is what makes every field reachable without a type switch, the walk is what makes the answer exact. Its test asserts literal escape TEXT is accepted in title, body and a field value, with a counterfactual leg asserting a real NUL in each of those places is still refused, so acceptance cannot come from the check doing nothing. Reverting either fix fails its test and only its test. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/artifact_import.go | 19 ++++++- internal/server/decode_json_nul_test.go | 64 ++++++++++++++++++++++ internal/server/middleware_request_text.go | 10 +++- 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/internal/server/artifact_import.go b/internal/server/artifact_import.go index ea80d3a46..ac4ca6fbb 100644 --- a/internal/server/artifact_import.go +++ b/internal/server/artifact_import.go @@ -1,7 +1,6 @@ package server import ( - "bytes" "encoding/json" "errors" "fmt" @@ -270,5 +269,21 @@ func artifactIsBindableText(art artifact.Artifact) bool { // refuse rather than pass it on unexamined. return false } - return !bytes.Contains(encoded, []byte{'\\', 'u', '0', '0', '0', '0'}) + // Searching the marshalled BYTES for the escape is wrong, and this + // function did it until codex round 9: a value holding the six LITERAL + // characters marshals to a doubled backslash, which still contains the + // six-character sequence as a substring, so valid content was refused. + // That is the same doubled-backslash trap bodyDecodesNUL exists to + // resolve — and an earlier version of this comment asserted it "cannot + // arise here", which was simply wrong. + // + // So the marshalled form is decoded again and walked with the same + // machinery, as caller data (no wire-key list applies to an artifact). + // The round trip is what makes every field reachable without a type + // switch; the walk is what makes the answer exact. + var v any + if err := json.Unmarshal(encoded, &v); err != nil { + return false + } + return !valueDecodesNUL(v, true) } diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index fdb01a8b7..273a3a164 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -13,6 +13,8 @@ import ( "regexp" "strings" "testing" + + "github.com/PerpetualSoftware/pad/internal/artifact" ) // rawJSONRequest sends a RAW body string. A test cannot marshal a Go map @@ -767,3 +769,65 @@ func TestDocumentActivityStoresBindableUserAgent(t *testing.T) { t.Error("no activity carries the sanitised header; the call site is not wired to requestUserAgent") } } + +// TestBodyDecodesNULUnderAJSONEncodedKeyChecksBothWays is codex round 9's P1, +// which was a REGRESSION this branch introduced in its round-8 restructure. +// +// Taking the JSON-encoded branch for a listed key used to skip the plain +// "does this string contain a NUL" check, so a direct NUL in a `fields` value +// — the very first door this change closed — was accepted again. The two +// questions are different and both must be asked: does the string ITSELF +// carry a NUL, and does the document it carries contain an escape. +func TestBodyDecodesNULUnderAJSONEncodedKeyChecksBothWays(t *testing.T) { + esc := escNULLiteral + + // A direct escape in the fields STRING (not inside a nested document): + // the outer decode turns it into a real NUL in that string. + direct := `{"title":"x","fields":"a` + esc + `b"}` + if !bodyDecodesNUL([]byte(direct)) { + t.Error("a NUL in the fields string itself must be refused") + } + + // And the nested form still is, so this is not just the plain check. + nested := `{"title":"x","fields":` + jsonEncode(t, `{"k":"a`+esc+`b"}`) + `}` + if !bodyDecodesNUL([]byte(nested)) { + t.Error("an escape inside the fields document must be refused") + } + + // Control: an ordinary fields string is accepted, so the legs above are + // not passing because everything under a listed key is refused. + if bodyDecodesNUL([]byte(`{"title":"x","fields":"plain"}`)) { + t.Error("an ordinary fields string must be accepted") + } +} + +// TestArtifactBindableTextAcceptsLiteralEscapeText is codex round 9's P2: the +// artifact check searched the MARSHALLED bytes for the escape, and a value +// holding the six LITERAL characters marshals to a doubled backslash which +// still contains that sequence — so valid content was refused. Artifacts are +// documentation; text about a JSON escape is exactly what one carries. +func TestArtifactBindableTextAcceptsLiteralEscapeText(t *testing.T) { + esc := escNULLiteral + ok := artifact.Artifact{ + Title: "Escapes", + Body: "write a NUL as " + esc + " in JSON", + Fields: map[string]any{ + "note": "also " + esc + " here", + }, + } + if !artifactIsBindableText(ok) { + t.Error("literal escape TEXT in an artifact is valid content and must be accepted") + } + + // The counterfactual: a real NUL in the same places is still refused, so + // the leg above is not passing because the check does nothing. + for name, bad := range map[string]artifact.Artifact{ + "title": {Title: "a\x00b"}, + "body": {Title: "t", Body: "a\x00b"}, + "fields": {Title: "t", Fields: map[string]any{"k": "a\x00b"}}, + } { + if artifactIsBindableText(bad) { + t.Errorf("a real NUL in %s must be refused", name) + } + } +} diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index e864a6011..b38af233f 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -522,7 +522,15 @@ func valueDecodesNUL(v any, inUserData bool) bool { } if !inUserData && jsonEncodedFieldKeys[k] { if str, isString := sub.(string); isString { - if nestedDocumentDecodesNUL(str) { + // 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 strings.ContainsRune(str, 0) || nestedDocumentDecodesNUL(str) { return true } continue From b628c4a6b1ed38d1cdcaefd3e062018839a44868 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 18:33:30 +0000 Subject: [PATCH 15/29] docs(backup): the one case where an export is not importable (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 12, an operational pass. It found no migration or config requirement, and two documentation gaps. docs/backup.md promises that application-level export/import is portable across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a workspace whose stored data contains a NUL exports fine and is refused on import. It can only affect data written before the rule existed and only on SQLite, which accepted it — a PostgreSQL instance never stored one. `pad db migrate-to-pg` has the SAME problem and reports it worse: it copies rows directly and never passes through the import guard, so a legacy row fails against PostgreSQL's JSONB parser partway through the copy rather than being refused up front. That is the likelier way an operator meets this, since it is the operation that puts an entire old SQLite database in front of PostgreSQL for the first time. Recorded on BUG-2810, which owns the preflight and repair. Round 12's other finding — that the PR's stated release note covered the JSON 500-to-400 change and none of the rest — is fixed in the PR body rather than in the tree. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- docs/backup.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/backup.md b/docs/backup.md index 9076b57bd..59fed808b 100644 --- a/docs/backup.md +++ b/docs/backup.md @@ -160,6 +160,27 @@ pad workspace import < my-workspace.json pad workspace import --name "imported-workspace" < my-workspace.json ``` +### One case where an export is not importable + +A workspace whose stored data contains a **NUL character** exports fine and is +refused on import, with a 400 naming the cause. This is not a corruption of +your backup — it is the import applying a rule the write path now applies too +(BUG-2803): a NUL cannot be stored in a text or JSON column, and PostgreSQL +refuses it outright. + +It can only affect data written **before** that rule existed, and only on +SQLite, which accepted it. A PostgreSQL instance never stored such a value. + +The same limitation applies to `pad db migrate-to-pg`, which copies rows +directly and does not go through the import guard: a row carrying a NUL will +fail against PostgreSQL's JSONB parser during the copy rather than being +reported up front. + +If you hit either, the affected value has to be repaired at the source before +the export or migration will go through. A preflight check and a repair path +are tracked as BUG-2810; until then the failing row is named in the error. + + This format is database-agnostic and can be used to: - Transfer workspaces between Pad instances - Create workspace templates From 799a8bfd9ea1261674d18109d19db90bba00cd0d Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 18:45:42 +0000 Subject: [PATCH 16/29] test(server): close two blind spots the tests themselves had (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 13, asked whether the new TESTS are sound. Five findings; these are the two that were self-contained. The other three are recorded on the item with what each needs. 1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only `.Body`, so FormValue / ParseForm / MultipartForm — which read the request body just as surely — were invisible. It therefore reported full coverage while the OAuth form-encoded handlers were entirely outside its view. Widened, and it immediately failed on handlers_oauth.go, which is the instrument working. That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the OAuth handlers read form-encoded bodies that no rule in this family covers (the transport rules see the query half of r.Form, not the body half), tracked as BUG-2811 and needing a fosite-backed fixture to measure. The test now STATES the gap instead of being blind to it, which is the difference between a completeness claim and a completeness appearance. 2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its assertions were: within the limit, bindable text, a prefix of the input. An empty string satisfies all three. It now also asserts that an input fitting the limit comes back UNCHANGED, and that no more than one rune (4 bytes) is lost to the boundary — so a truncator that drops too much fails, not just one that keeps too much. Both were found by asking whether a broken implementation would pass, which is the question CONVE-12 is about and which I had applied to the production code and not to these two tests. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 273a3a164..1d5cb04c1 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -242,9 +242,16 @@ func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { "handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores it; the real decode still happens through decodeJSON downstream", "middleware_mcp_audit.go": "audit capture — records the body for the MCP audit log and restores it; decoding still happens in the MCP dispatcher", "handlers_tokens.go": "a nil/ContentLength check only — it never reads the body", + "handlers_oauth.go": "KNOWN GAP, tracked as BUG-2811: the OAuth handlers read FORM-encoded bodies (r.Form/FormValue), which no rule in this family covers — the transport rules see the query half of r.Form and not the body half. Listed so this test states the gap instead of being blind to it; measuring it needs a fosite-backed fixture.", } - pattern := regexp.MustCompile(`\b(r|req)\.Body\b`) + // FormValue/PostFormValue/ParseForm/ParseMultipartForm read the request + // BODY too, and the first version of this scan looked only for .Body — so + // it reported full coverage while the OAuth form-body handlers, which + // BUG-2811 tracks, were entirely invisible to it (codex round 13). A + // completeness test with a blind spot is worse than none, because it + // reads as coverage. + pattern := regexp.MustCompile(`\b(r|req)\.(Body|FormValue|PostFormValue|ParseForm|ParseMultipartForm|MultipartForm|PostForm)\b`) entries, err := os.ReadDir(".") if err != nil { @@ -563,6 +570,18 @@ func TestTruncateBindableText(t *testing.T) { if !strings.HasPrefix(tc.s, got) { t.Errorf("result %q is not a prefix of the input", got) } + // And it must keep as much as the limit allows. Without this, + // an implementation returning "" for every input passes every + // assertion above — it is bindable, within the limit, and a + // prefix (codex round 13). + if len(tc.s) <= tc.limit { + if got != tc.s { + t.Errorf("input fits the limit and must be returned unchanged, got %q", got) + } + } else if lost := tc.limit - len(got); lost >= 4 { + t.Errorf("dropped %d bytes to respect a %d-byte limit; at most one rune (max 4 bytes) "+ + "should be lost to the boundary", lost, tc.limit) + } }) } From 83831acb66debcb19e11be015f56e77533bb2a34 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 18:50:55 +0000 Subject: [PATCH 17/29] test(server): the three remaining round-13 gaps (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 13's other three findings, all of the same shape: a test that would stay green with the production change reverted. 1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives containing only pad-export.json, so disabling the INDEPENDENT attachment- manifest check left the suite green. The new test builds a bundle with both entries, differing only in the manifest, so a refusal cannot come from the export half. Verified by disabling each check separately: only the matching test fails, so the two are independently covered. 2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg exercised decodeJSON, so reverting handlers_admin.go to default EVERY decode failure to the admin's own address passed them all. The new test drives the real endpoint with a wired mock sender and pins the distinction that used to collapse: an ABSENT body still means "send it to me" (control), an ordinary body still sends (control), and a body that is present and refused answers 400 rather than being reinterpreted as the default recipient. 3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs while letting malformed UTF-8 through would have passed it. It now drives both, which matters because invalid UTF-8 is the class that reaches Postgres as 22021 on a UTF8 database. Round 13 was asked whether the new TESTS are sound — deterministic, order-independent, and failing on broken code. It reported the fixtures isolated and found five ways they were not discriminating. Two were fixed in the previous commit; these are the rest. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 47 +++++++++++++ internal/server/handlers_attachments_test.go | 11 +++ .../server/handlers_import_bundle_test.go | 68 +++++++++++++++++++ 3 files changed, 126 insertions(+) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 1d5cb04c1..18be3ba6f 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -850,3 +850,50 @@ func TestArtifactBindableTextAcceptsLiteralEscapeText(t *testing.T) { } } } + +// TestTestEmailRefusesUnusableBody is the handler-level leg codex round 13 +// found missing. Reverting handlers_admin.go to `err != nil || input.To == ""` +// — which defaulted EVERY decode failure to the admin's own address — passed +// every other test in this file, because they only exercise decodeJSON. +// +// The distinction being pinned is between two things that used to collapse +// into one: an ABSENT body legitimately means "send it to me", while a body +// that is present and REFUSED must not be reinterpreted as that default. +func TestTestEmailRefusesUnusableBody(t *testing.T) { + srv := testServer(t) + token := bootstrapFirstUser(t, srv, "admin-mail@example.com", "Admin") + mock := mockMailerooEndpoint(t, http.StatusOK, true) + configureEmailForTest(srv, mock.URL, "http://localhost:7777") + + post := func(t *testing.T, body string) *httptest.ResponseRecorder { + t.Helper() + var r io.Reader + if body != "" { + r = strings.NewReader(body) + } + req := httptest.NewRequest("POST", "/api/v1/admin/test-email", r) + if body != "" { + req.Header.Set("Content-Type", "application/json") + } + req.Header.Set("Authorization", "Bearer "+token) + req.RemoteAddr = "127.0.0.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr + } + + // Control 1: no body at all still means "send it to me". + if rr := post(t, ""); rr.Code != http.StatusOK { + t.Fatalf("absent body must still send, got %d: %s", rr.Code, rr.Body.String()) + } + // Control 2: an ordinary body still works. + if rr := post(t, `{"to":"someone@example.com"}`); rr.Code != http.StatusOK { + t.Fatalf("ordinary body must send, got %d: %s", rr.Code, rr.Body.String()) + } + // The case: a body that is valid JSON but carries an unusable value. + rr := post(t, `{"to":"a`+escNULLiteral+`b@example.com"}`) + if rr.Code != http.StatusBadRequest { + t.Errorf("a refused body must answer 400, not be reinterpreted as the default recipient; got %d: %s", + rr.Code, rr.Body.String()) + } +} diff --git a/internal/server/handlers_attachments_test.go b/internal/server/handlers_attachments_test.go index c59aedfa3..1078f584a 100644 --- a/internal/server/handlers_attachments_test.go +++ b/internal/server/handlers_attachments_test.go @@ -992,6 +992,17 @@ func TestUpload_MultipartTextFieldsAreBindableText(t *testing.T) { if control.Code != http.StatusCreated && control.Code != http.StatusOK { t.Fatalf("control (no item_id) must succeed, got %d: %s", control.Code, control.Body.String()) } + // Both byte classes, not just the NUL: a filter that rejected NULs + // while letting malformed UTF-8 through would have passed the + // earlier version of this leg (codex round 13). Invalid UTF-8 is the + // class that reaches Postgres as 22021 on a UTF8 database. + for _, bad := range []string{"TASK-1\x00", "TASK-1\xff"} { + rr := upload(t, "clean.png", &bad) + if rr.Code != control.Code { + t.Errorf("an unusable item_id %q must be treated as no value (like the control, %d), got %d: %s", + bad, control.Code, rr.Code, rr.Body.String()) + } + } bad := "TASK-1\x00" rr := upload(t, "clean.png", &bad) if rr.Code != control.Code { diff --git a/internal/server/handlers_import_bundle_test.go b/internal/server/handlers_import_bundle_test.go index 6058f0d1d..b5944c10f 100644 --- a/internal/server/handlers_import_bundle_test.go +++ b/internal/server/handlers_import_bundle_test.go @@ -697,3 +697,71 @@ func TestImportBundle_RefusesNULInExport(t *testing.T) { t.Errorf("the 400 should name the cause, got body=%s", got.Body.String()) } } + +// TestImportBundle_RefusesNULInManifest is the leg codex round 13 found +// missing: TestImportBundle_RefusesNULInExport builds bundles containing only +// pad-export.json, so removing the INDEPENDENT manifest check left the suite +// green. The manifest is a second JSON document parsed the same way and needs +// its own coverage. +// +// Both bundles carry a valid export; they differ only in the manifest, so a +// refusal cannot come from the export half. +func TestImportBundle_RefusesNULInManifest(t *testing.T) { + src, srcSlug := testServerWithAttachments(t) + rr := doRequest(src, "GET", "/api/v1/workspaces/"+srcSlug+"/export", nil) + if rr.Code != http.StatusOK { + t.Fatalf("export src: %d %s", rr.Code, rr.Body.String()) + } + exportJSON := rr.Body.Bytes() + + esc := string([]byte{'\\', 'u', '0', '0', '0', '0'}) + cleanManifest := `{"version":1,"entries":[]}` + nulManifest := `{"version":1,"entries":[{"id":"00000000-0000-0000-0000-000000000001",` + + `"filename":"a` + esc + `b.png","mime":"image/png","size_bytes":1,"content_hash":"deadbeef"}]}` + + bundle := func(t *testing.T, manifest string) []byte { + t.Helper() + var buf bytes.Buffer + gzw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gzw) + for _, e := range []struct { + name string + body []byte + }{ + {"pad-export.json", exportJSON}, + {"attachments/manifest.json", []byte(manifest)}, + } { + if err := tw.WriteHeader(&tar.Header{Name: e.name, Mode: 0o644, Size: int64(len(e.body))}); err != nil { + t.Fatalf("write header %s: %v", e.name, err) + } + if _, err := tw.Write(e.body); err != nil { + t.Fatalf("write %s: %v", e.name, err) + } + } + tw.Close() + gzw.Close() + return buf.Bytes() + } + + post := func(t *testing.T, name string, body []byte) *httptest.ResponseRecorder { + t.Helper() + dest, _ := testServerWithAttachments(t) + req := httptest.NewRequest("POST", "/api/v1/workspaces/import?name="+name, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/gzip") + req.RemoteAddr = "127.0.0.1:1234" + rec := httptest.NewRecorder() + dest.ServeHTTP(rec, req) + return rec + } + + if got := post(t, "CleanManifest", bundle(t, cleanManifest)); got.Code != http.StatusOK && got.Code != http.StatusCreated { + t.Fatalf("control bundle must import, got %d: %s", got.Code, got.Body.String()) + } + got := post(t, "NULManifest", bundle(t, nulManifest)) + if got.Code < 400 { + t.Errorf("a manifest carrying a NUL escape must be refused, got %d: %s", got.Code, got.Body.String()) + } + if !strings.Contains(got.Body.String(), "NUL") { + t.Errorf("the refusal should name the cause, got body=%s", got.Body.String()) + } +} From 8bb1acb8118be5d316d58ce060f7ea0038a5d3f6 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 19:05:39 +0000 Subject: [PATCH 18/29] test(server): pin the wiring at every call site, not one (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 14 confirmed round 13's five, then found the same shape one level out: reverting a SINGLE call site back to the unsafe form left the whole suite green, because the surviving fixtures are ASCII and a helper's unit test does not care who calls it. TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY rather than adding a fixture per site (an OAuth connection, a cloud login, four audit paths). A byte-slice truncation of a caller string fails it, and so does a raw User-Agent read outside the exempt set. Both directions are checked: finding none of the SAFE form also fails, so a scan that silently matched nothing cannot pass forever. The User-Agent exemptions carry counts rather than being blanket, so a NEW raw read in an exempt file still fails. All four reads in handlers_auth.go are exempt because they feed a HASH — CreateSession hashes the header and stores no text — and sanitising before hashing would be actively harmful: login would store sha256(sanitised) while the session check still hashes the RAW header, failing validation for every client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw read is requestUserAgent itself. Verified by reverting one truncation call site and one User-Agent call site independently; each fails the test. Round 14's third finding is fixed behaviourally rather than statically, because the static scan cannot see it — handlers_oauth.go is already listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives the real dynamic-registration endpoint with cloud mode and an OAuth server wired, with a control leg registering successfully, and pins both the refusal and the message split: the body IS valid JSON, so the answer must not send a client hunting a syntax error. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 127 ++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 18be3ba6f..331971678 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -897,3 +897,130 @@ func TestTestEmailRefusesUnusableBody(t *testing.T) { rr.Code, rr.Body.String()) } } + +// TestTextSafeHelpersAreUsedAtEveryCallSite closes the gap codex round 14 +// named: reverting a single call site back to the unsafe form leaves every +// behavioural test green, because the surviving fixtures are ASCII and the +// helpers' own unit tests do not care who calls them. +// +// Testing each site behaviourally would need a fixture per site (an OAuth +// connection, a cloud login, four audit paths). This asserts the WIRING +// statically instead — the same technique as +// TestEveryRequestBodyReaderIsAccountedFor, and the same bargain: a reverted +// call site fails here immediately, and a new one has to be decided on +// deliberately rather than added invisibly. +// +// It asserts in both directions: an unsafe form anywhere fails, and finding +// none of the safe form also fails, so a scan that silently matched nothing +// cannot pass forever. +func TestTextSafeHelpersAreUsedAtEveryCallSite(t *testing.T) { + // A plain byte slice of a caller string can split a rune and produce + // invalid UTF-8; truncateBindableText is the only allowed form. + unsafeTruncate := regexp.MustCompile(`\b(name|suggested|input\.Name|Name)\s*=\s*\w+(\.\w+)*\[:\d+\]`) + // The raw header reaches a text column; requestUserAgent is the allowed + // form. The two HASH sites are exempt and named below. + unsafeUA := regexp.MustCompile(`(r|req)\.UserAgent\(\)|(r|req)\.Header\.Get\("User-Agent"\)`) + // Exempt sites, with counts, so a NEW raw read in one of these files + // still fails rather than hiding behind a blanket exemption. + uaHashExempt := map[string]int{ + // All four reads here feed a HASH, not a text column: one + // session-fingerprint comparison plus three CreateSession calls, + // and store.CreateSession hashes the header (sessions.ua_hash) and + // stores no text. sha256 over arbitrary bytes is well defined, and + // sanitising before hashing would be actively harmful — login would + // store sha256(sanitised) while the comparison still hashes the RAW + // header, so every session from a client with a non-UTF-8 + // User-Agent would fail validation. + "handlers_auth.go": 4, + // The same fingerprint comparison on the session-validation path. + "middleware_auth.go": 1, + // requestUserAgent itself: this is the one read that is allowed to + // be raw, because it is the thing doing the sanitising. + "middleware_request_text.go": 1, + } + + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("read package dir: %v", err) + } + var safeTruncateUses, safeUAUses int + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + src, err := os.ReadFile(filepath.Join(".", name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + text := string(src) + safeTruncateUses += strings.Count(text, "truncateBindableText(") + safeUAUses += strings.Count(text, "requestUserAgent(") + + for _, m := range unsafeTruncate.FindAllString(text, -1) { + t.Errorf("%s: %q slices a caller string by BYTES, which can split a rune and produce "+ + "invalid UTF-8 downstream of validation — use truncateBindableText (BUG-2803)", name, m) + } + found := len(unsafeUA.FindAllString(text, -1)) + if allowed := uaHashExempt[name]; found > allowed { + t.Errorf("%s: %d raw User-Agent read(s), %d exempt. A raw header reaching a text column "+ + "must go through requestUserAgent; only the session-hash comparisons are exempt (BUG-2803)", + name, found, allowed) + } + } + + if safeTruncateUses < 4 { + t.Errorf("found only %d truncateBindableText call sites; the scan or the wiring is broken", safeTruncateUses) + } + if safeUAUses < 4 { + t.Errorf("found only %d requestUserAgent call sites; the scan or the wiring is broken", safeUAUses) + } +} + +// TestOAuthRegisterRefusesNULBody is the last of codex round 14's three +// unwired-call-site findings. Reverting handlers_oauth.go to a bare +// json.NewDecoder — losing both the refusal and the size cap — would +// otherwise leave the suite green, and the static body-reader scan cannot +// catch it because that file is already listed for its FORM-body reads. +// +// It also pins the message split: a body carrying a NUL is valid JSON, so +// answering "Request body must be JSON" sends a client hunting a syntax error +// it does not have. +func TestOAuthRegisterRefusesNULBody(t *testing.T) { + srv := testServer(t) + srv.SetCloudMode("cloud-secret-for-test") + o, err := newTestOAuthServer(t, srv) + if err != nil { + t.Fatalf("oauth.NewServer: %v", err) + } + srv.SetOAuthServer(o) + + post := func(t *testing.T, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest("POST", "/oauth/register", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "192.0.2.7:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr + } + + // Control: a well-formed registration must still be accepted, so a 400 + // below cannot be the endpoint refusing everything. + control := post(t, `{"redirect_uris":["https://example.com/cb"],"client_name":"Probe"}`) + if control.Code >= 400 { + t.Fatalf("control registration must succeed, got %d: %s", control.Code, control.Body.String()) + } + + rr := post(t, `{"redirect_uris":["https://example.com/cb"],"client_name":"a`+escNULLiteral+`b"}`) + if rr.Code != http.StatusBadRequest { + t.Fatalf("a NUL-bearing registration must answer 400, got %d: %s", rr.Code, rr.Body.String()) + } + if strings.Contains(rr.Body.String(), "must be JSON") { + t.Errorf("the body IS valid JSON; the message must not send the client after a syntax error: %s", + rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "NUL") { + t.Errorf("the refusal should name the cause, got %s", rr.Body.String()) + } +} From d32e07261bfab0ea7f3bae1a16331b97c867343c Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 19:21:41 +0000 Subject: [PATCH 19/29] fix(server): match wire keys the way the decoder does (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 16, asked whether this change is consistent with its siblings in the same file and extensible by someone who did not write it. It found a live bypass instead. encoding/json matches an incoming key to a struct field by an exact match first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and {"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does. The walk looked the key up case-SENSITIVELY, so it skipped the nested document for a body the handler went on to accept, and the database answered the original 500. Measured before the fix: `fields` refused, `Fields` and `FIELDS` accepted. This is the same defect shape as everything else in this unit — a check that agrees with one layer's rules while the layer that actually consumes the value uses different ones — which is why the fix is a PREDICATE rather than a wider map: the map is the vocabulary, and the matching RULE belongs to the consumer. Someone adding a key should not also have to remember to add its spellings. The test drives six spellings including mixed case, with a control asserting an unlisted key stays caller data in any casing, so this is case-insensitive matching rather than matching everything. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 26 ++++++++++++++++++++++ internal/server/middleware_request_text.go | 24 +++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 331971678..7392a98a2 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -1024,3 +1024,29 @@ func TestOAuthRegisterRefusesNULBody(t *testing.T) { t.Errorf("the refusal should name the cause, got %s", rr.Body.String()) } } + +// TestBodyDecodesNULMatchesKeysLikeTheDecoder is codex round 16: encoding/json +// matches an incoming key to a struct field by an exact match first and a +// CASE-INSENSITIVE one otherwise, so `{"Fields":...}` lands in +// ItemCreate.Fields exactly as `{"fields":...}` does. A case-sensitive lookup +// in the walk skipped the nested check for a body the handler then accepted, +// and the database answered the original 500. +// +// Measured before the fix: `fields` refused, `Fields` and `FIELDS` accepted. +func TestBodyDecodesNULMatchesKeysLikeTheDecoder(t *testing.T) { + inner := `{"k":"a` + escNULLiteral + `b"}` + for _, key := range []string{"fields", "Fields", "FIELDS", "fIeLdS", "Schema", "TAGS"} { + body := `{"title":"x","` + key + `":` + jsonEncode(t, inner) + `}` + if !bodyDecodesNUL([]byte(body)) { + t.Errorf("key %q reaches the same struct field as its lower-case spelling and must be "+ + "walked the same way", key) + } + } + + // Control: a key that is not a wire key in ANY casing stays caller data, + // so this is case-insensitive matching rather than matching everything. + notAKey := `{"title":"x","Notes":` + jsonEncode(t, inner) + `}` + if bodyDecodesNUL([]byte(notAKey)) { + t.Error("an unlisted key must not be treated as JSON-encoded whatever its casing") + } +} diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index b38af233f..2895a3e97 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -478,6 +478,28 @@ var jsonEncodedFieldKeys = map[string]bool{ "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 + } + return jsonEncodedFieldKeys[strings.ToLower(k)] +} + // valueDecodesNUL walks a decoded request body for a string that contains a // NUL, descending into a JSON document carried as a string exactly once. // @@ -520,7 +542,7 @@ func valueDecodesNUL(v any, inUserData bool) bool { if strings.ContainsRune(k, 0) { return true } - if !inUserData && jsonEncodedFieldKeys[k] { + 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 From b01928711465aa4070b00a45c42244e24783ef02 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 28 Aug 2026 19:32:53 +0000 Subject: [PATCH 20/29] fix(server): fold keys the way encoding/json folds them (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 17, first of five findings. The previous commit fixed the ASCII half of key matching and left the Unicode half, which is this bug's own pattern one more time. encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing. U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the `schema` struct field while strings.ToLower("ſchema") is unchanged and missed the allowlist — a nested NUL under that spelling reached the handler undetected. Matching is now strings.EqualFold against each canonical key. The test carries both a lower-case fold spelling and an upper-case one alongside the ASCII cases, and keeps its control asserting an unlisted key stays caller data in any casing. The other four round-17 findings are recorded on the item rather than patched here: they are genuine layer disagreements (duplicate keys merging differently in a typed decode than in a map, a scan-failure disposition on inputs the typed decode tolerates, and unknown-field policy) whose fixes are design decisions rather than corrections, and this seat is near its context bar. Each is written up with the measurement it needs. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN --- internal/server/decode_json_nul_test.go | 6 +++++- internal/server/middleware_request_text.go | 13 ++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 7392a98a2..2a356291c 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -1035,7 +1035,11 @@ func TestOAuthRegisterRefusesNULBody(t *testing.T) { // Measured before the fix: `fields` refused, `Fields` and `FIELDS` accepted. func TestBodyDecodesNULMatchesKeysLikeTheDecoder(t *testing.T) { inner := `{"k":"a` + escNULLiteral + `b"}` - for _, key := range []string{"fields", "Fields", "FIELDS", "fIeLdS", "Schema", "TAGS"} { + // The last two are Unicode simple-fold spellings: encoding/json matches + // them to the same struct field, and lower-casing does NOT (codex round + // 17). U+017F LONG S folds to 's'; U+212A KELVIN SIGN folds to 'k'. + for _, key := range []string{"fields", "Fields", "FIELDS", "fIeLdS", "Schema", "TAGS", + "\u017Fchema", "\u017FCHEMA"} { body := `{"title":"x","` + key + `":` + jsonEncode(t, inner) + `}` if !bodyDecodesNUL([]byte(body)) { t.Errorf("key %q reaches the same struct field as its lower-case spelling and must be "+ diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 2895a3e97..813b0e9d4 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -497,7 +497,18 @@ func isJSONEncodedFieldKey(k string) bool { if jsonEncodedFieldKeys[k] { return true } - return jsonEncodedFieldKeys[strings.ToLower(k)] + // 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 From 2f05f7aa4b7b1a0fd962d4127532c6bb627b54a4 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 01:57:07 +0000 Subject: [PATCH 21/29] fix(server): pin that a NUL-bearing manifest refusal keeps the partial workspace (BUG-2803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 18. The comment on the manifest NUL branch said refusing the input "stops it from being reached this way" and stopped there, which reads as though the refusal undoes the import. It does not. A plain error with a non-nil workspace keeps the partial workspace, exactly as every other manifest failure in this loop does — the rollback branch fires only for *importStatusError, and mid-stream manifest failures intentionally keep what was imported (TASK-896). Returning a rollback-shaped error here would give NUL-bearing manifests different semantics from malformed ones, which is a change to the bundle-import contract rather than a fix to this bug. So the behaviour is unchanged and now DELIBERATE: the comment states it, and the test asserts the persisted state rather than only the HTTP answer. Mutation: routing the branch through *importStatusError makes the refusal roll back, and the new assertion fails naming the release note it would falsify. The pre-existing status/body assertions do not notice. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/handlers_import_bundle.go | 15 +++++++++- .../server/handlers_import_bundle_test.go | 28 ++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/internal/server/handlers_import_bundle.go b/internal/server/handlers_import_bundle.go index 64c0c5351..c32c063bb 100644 --- a/internal/server/handlers_import_bundle.go +++ b/internal/server/handlers_import_bundle.go @@ -344,7 +344,20 @@ func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID // success while silently dropping the attachment (codex round 4, // BUG-2803). The skip-on-failure behaviour is pre-existing and // deliberate (a partial restore beats none); refusing the bad - // INPUT is what stops it from being reached this way. + // INPUT is what stops it from being reached that way. + // + // IT DOES NOT ROLL BACK, and the earlier wording implied more + // than it delivers (codex round 18). A plain error with a + // non-nil workspace keeps the partial workspace, exactly as + // every other manifest failure below does — the rollback branch + // fires only for *importStatusError, and the comment there + // records that mid-stream manifest failures intentionally keep + // what was imported, tracked under TASK-896. Returning a + // rollback-shaped error HERE would give NUL-bearing manifests + // different semantics from malformed ones, which is a change to + // the bundle-import contract rather than a fix to this bug. The + // resulting state is pinned by a test and stated in the release + // note instead of being left incidental. if bodyDecodesNUL(buf) { return ws, fmt.Errorf("manifest decode: %w (workspace created but attachments not restored)", errJSONBodyNUL) } diff --git a/internal/server/handlers_import_bundle_test.go b/internal/server/handlers_import_bundle_test.go index b5944c10f..2916521d0 100644 --- a/internal/server/handlers_import_bundle_test.go +++ b/internal/server/handlers_import_bundle_test.go @@ -757,11 +757,37 @@ func TestImportBundle_RefusesNULInManifest(t *testing.T) { if got := post(t, "CleanManifest", bundle(t, cleanManifest)); got.Code != http.StatusOK && got.Code != http.StatusCreated { t.Fatalf("control bundle must import, got %d: %s", got.Code, got.Body.String()) } - got := post(t, "NULManifest", bundle(t, nulManifest)) + dest, _ := testServerWithAttachments(t) + req := httptest.NewRequest("POST", "/api/v1/workspaces/import?name=NULManifest", + bytes.NewReader(bundle(t, nulManifest))) + req.Header.Set("Content-Type", "application/gzip") + req.RemoteAddr = "127.0.0.1:1234" + got := httptest.NewRecorder() + dest.ServeHTTP(got, req) + if got.Code < 400 { t.Errorf("a manifest carrying a NUL escape must be refused, got %d: %s", got.Code, got.Body.String()) } if !strings.Contains(got.Body.String(), "NUL") { t.Errorf("the refusal should name the cause, got body=%s", got.Body.String()) } + + // And the PERSISTED state, which the HTTP answer alone does not tell you + // (codex round 18). A manifest refusal is NOT a rollback: it keeps the + // partial workspace, exactly as every other manifest failure does — the + // rollback branch fires only for *importStatusError, and mid-stream + // manifest failures intentionally keep what was imported (TASK-896). + // + // Asserting it here makes that contract deliberate rather than + // incidental: if someone later routes this rejection through the + // rollback branch, this test says so instead of staying green while the + // release note goes stale. + list := doRequest(dest, "GET", "/api/v1/workspaces", nil) + if list.Code != http.StatusOK { + t.Fatalf("list workspaces: %d %s", list.Code, list.Body.String()) + } + if !strings.Contains(list.Body.String(), "NULManifest") { + t.Errorf("a manifest refusal keeps the partial workspace (TASK-896); it is absent, so the "+ + "behaviour changed and the release note now says something false: %s", list.Body.String()) + } } From dbe6f5192ab22853342c008496d80c8428b7e795 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 01:58:59 +0000 Subject: [PATCH 22/29] docs(server): record the four map-model disagreements as dispositions, and pin them (BUG-2803) Lead ruling day-68 is land-and-follow: this branch lands on its measured commits, and the token-stream rewrite is the BUG-2812 unit's spec rather than a late restructure of an 18-commit branch under review pressure. That makes the four open findings from rounds 16-17 something to WRITE DOWN precisely, not something to leave in a trail comment. The doc comment on bodyDecodesNUL now carries all four, with the one root cause named: this scan decodes into map[string]any and the typed decode does not agree with that model about keys. Two under-refuse (duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec - both dissolve under a walk that never builds values. Two over-refuse (unknown fields; case-variant duplicates) and are ACCEPTED, because refusing is the safe direction. The asymmetry is stated rather than smoothed over: within the map model, (1) and (4) are one defect seen from two sides and only one of them fails safe. Finding (3) is an observable compatibility change - a forward-compatible field carrying a NUL escape now gets a 400 where it got a 200 - so it goes in the release note as well as here. A qualification only protects where the actor meets it. All four are pinned by a test, measured on this tip rather than carried over from the round-16/17 write-up. The two known-gap legs assert the WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc comment and the release note as what to update. Both gap legs carry a premise assertion - the same bodies with the disagreement mechanism removed ARE detected - without which they would pass against a check that detected nothing. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/decode_json_nul_test.go | 73 ++++++++++++++++++++++ internal/server/middleware_request_text.go | 58 +++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 2a356291c..c6951970c 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -1054,3 +1054,76 @@ func TestBodyDecodesNULMatchesKeysLikeTheDecoder(t *testing.T) { t.Error("an unlisted key must not be treated as JSON-encoded whatever its casing") } } + +// TestBodyDecodesNULKnownMapModelDisagreements pins the four measured +// disagreements between this scan's map[string]any model and encoding/json's +// typed decode (BUG-2803 rounds 16-17, lead ruling day-68: land-and-follow). +// They are documented on bodyDecodesNUL; this is the instrument that keeps +// that documentation and the release note from going stale. +// +// Two of the four are ACCEPTED over-refusals. The other two are KNOWN GAPS +// whose fix is the BUG-2812 token-walk unit — so those legs assert the WRONG +// answer on purpose. That is deliberate and it is the point: when BUG-2812 +// lands, this test FAILS, which is the signal to update the doc comment, the +// release note and this test together rather than discovering months later +// that the note describes a version of the check that no longer exists. +func TestBodyDecodesNULKnownMapModelDisagreements(t *testing.T) { + t.Run("accepted over-refusals", func(t *testing.T) { + // (3) An unknown field is scanned though no handler reads it. The + // scan has no destination type by design, so it cannot tell a + // forward-compatible field from a real one. Observable compatibility + // change; stated in the release note. + unknown := `{"title":"valid","future_field":"a` + escNULLiteral + `b"}` + if !bodyDecodesNUL([]byte(unknown)) { + t.Error("(3) an unknown field carrying a NUL escape is refused today; if that changed, " + + "update the disposition on bodyDecodesNUL and the release note's compatibility line") + } + + // (4) Case-variant duplicates: the typed decode keeps the LAST + // spelling and discards the NUL, the map scan sees both and refuses. + // Same root as (1), opposite direction. + caseDup := `{"title":"a` + escNULLiteral + `b","TITLE":"safe"}` + if !bodyDecodesNUL([]byte(caseDup)) { + t.Error("(4) a case-variant duplicate is refused today even though the decode drops the " + + "NUL spelling; if that changed, update the disposition on bodyDecodesNUL") + } + }) + + t.Run("known gaps owned by BUG-2812", func(t *testing.T) { + // (1) Duplicate keys: map[string]any REPLACES, encoding/json MERGES + // into an already-populated map field. The scan structurally cannot + // see the shadowed first occurrence. + dupKey := `{"fields_patch":{"orphan":"a` + escNULLiteral + `b"},"fields_patch":{"status":"open"}}` + if bodyDecodesNUL([]byte(dupKey)) { + t.Error("(1) now DETECTED — the map-model duplicate-key gap is closed. That is the " + + "BUG-2812 token walk landing: update bodyDecodesNUL's disposition block, the " + + "release note's filed-residuals line, and delete this leg") + } + + // (2) A scan failure lets a known-bad value through: the overflowing + // number fails the `any` unmarshal, this function returns false so the + // caller's decode owns the "invalid JSON" message, and the typed + // decode then SKIPS the unknown field and accepts the NUL title. + scanFail := `{"title":"a` + escNULLiteral + `b","ignored":1e999}` + if bodyDecodesNUL([]byte(scanFail)) { + t.Error("(2) now DETECTED — the scan-failure passthrough is closed. That is the " + + "BUG-2812 token walk landing: update bodyDecodesNUL's disposition block, the " + + "release note's filed-residuals line, and delete this leg") + } + + // Premise for both legs above: the SAME bodies with their + // disagreement mechanism removed ARE detected. Without this, the two + // assertions would pass against a bodyDecodesNUL that detected + // nothing at all, and would prove nothing about the gaps they name. + singleKey := `{"fields_patch":{"orphan":"a` + escNULLiteral + `b"}}` + if !bodyDecodesNUL([]byte(singleKey)) { + t.Fatal("premise failed: the duplicate-key body's payload is not detectable even when " + + "spelled once, so the (1) leg above is vacuous") + } + parseable := `{"title":"a` + escNULLiteral + `b","ignored":1}` + if !bodyDecodesNUL([]byte(parseable)) { + t.Fatal("premise failed: the scan-failure body's payload is not detectable even when " + + "the body parses, so the (2) leg above is vacuous") + } + }) +} diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 813b0e9d4..7f4314c0b 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -424,6 +424,64 @@ var unicodeEscapePrefix = []byte{'\\', 'u', '0', '0'} // 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 day-68: 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; patching them into the map model would be re-plumbing an +// 18-commit branch late under review pressure, and this branch's one +// regression came from exactly that. +// +// 1. DUPLICATE KEYS MERGE DIFFERENTLY (P1). For +// {"fields_patch":{"orphan":""},"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":"ab","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":""} 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":"","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 From adf43e4cbbf481714ade61c372f61536244de3b1 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 02:15:01 +0000 Subject: [PATCH 23/29] test(server): wire release-note item 10 to the router, with its before-state measured (BUG-2803) The disposition test proves bodyDecodesNUL RETURNS true for an unknown field carrying a NUL escape. The release note claims the API answers 400. Those are different claims and only the second one is what an operator or client author reads - CONVE-19, my own convention: a direct-call test vouches for the component, not its binding. Two legs, and the control is the load-bearing one. An unknown field with an ordinary value must still be ACCEPTED, so this pins "refused for the NUL" rather than "refused for being unknown". The handler does not reject unknown fields; if it ever started to, the note's explanation would be wrong while its status code stayed right, and no status-code-only assertion could see that. The before-state is measured rather than asserted from memory. Disabling the check makes the same request answer 201 - which is main's behaviour, since decodeJSONWithLimit there unmarshals straight into the typed value and the key is dropped. So "answers 400 where it answered 200" is a measurement in both directions, not a recollection of one. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/decode_json_nul_test.go | 54 +++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index c6951970c..73989181c 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -1127,3 +1127,57 @@ func TestBodyDecodesNULKnownMapModelDisagreements(t *testing.T) { } }) } + +// TestUnknownFieldRefusalThroughTheHandler is the WIRING leg for release-note +// item 10 (CONVE-19: a direct-call test vouches for the component, not its +// binding). TestBodyDecodesNULKnownMapModelDisagreements proves the scan +// RETURNS true for an unknown field carrying a NUL escape; the release note +// claims the API answers 400. Those are different claims, and only this one +// is the one an operator or client author reads. +// +// The control leg is what makes the 400 mean something: the SAME unknown +// field with an ordinary value must still be ACCEPTED, so this pins "refused +// for the NUL" rather than "refused for being unknown" — the handler does not +// reject unknown fields, and if it ever started to, the release note's +// explanation of the change would be wrong even though its status code +// stayed right. +func TestUnknownFieldRefusalThroughTheHandler(t *testing.T) { + srv := testServer(t) + + rr := rawJSONRequest(srv, "POST", "/api/v1/workspaces/", + `{"name":"Unknown field probe","slug":"unkprobe","template":"startup"}`) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("fixture workspace: %d %s", rr.Code, rr.Body.String()) + } + rr = rawJSONRequest(srv, "POST", "/api/v1/workspaces/unkprobe/collections/", + `{"name":"Probes","slug":"probes"}`) + if rr.Code != http.StatusOK && rr.Code != http.StatusCreated { + t.Fatalf("fixture collection: %d %s", rr.Code, rr.Body.String()) + } + + const itemsPath = "/api/v1/workspaces/unkprobe/collections/probes/items" + + // Control: an unknown field is ordinarily IGNORED, not refused. On main + // this is the only behaviour there is — decodeJSONWithLimit unmarshals + // straight into the typed value, which drops the key. + control := rawJSONRequest(srv, "POST", itemsPath, + `{"title":"ok","future_field":"harmless"}`) + if control.Code != http.StatusCreated && control.Code != http.StatusOK { + t.Fatalf("an unknown field with an ordinary value must still be accepted, got %d: %s", + control.Code, control.Body.String()) + } + + // Release-note item 10: the same unknown field carrying a NUL escape is + // now refused, though the value reaches nothing. The scan has no + // destination type by design and cannot tell a forward-compatible field + // from a real one. + got := rawJSONRequest(srv, "POST", itemsPath, + `{"title":"ok","future_field":"a`+escNULLiteral+`b"}`) + if got.Code != http.StatusBadRequest { + t.Fatalf("release note item 10 says an unknown field carrying a NUL escape answers 400; "+ + "got %d: %s — update the note or the check", got.Code, got.Body.String()) + } + if !strings.Contains(got.Body.String(), "NUL") { + t.Errorf("the 400 should name the cause, got: %s", got.Body.String()) + } +} From 22e51863e803f406434d73b541351ff27d1b9047 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 02:22:01 +0000 Subject: [PATCH 24/29] docs(backup): the NUL rule lives in the binary, not the database (BUG-2803, BUG-2813) Codex round 19, the fresh-angle deploy/rollback/mixed-version pass. docs/backup.md said a NUL-bearing row "can only affect data written before that rule existed, and only on SQLite". The second half is true. The first half is false, and the reason is the interesting part: the guard is in decodeJSONWithLimit, so the invariant is a property of the running BINARY, not of the database. On SQLite any window where an older binary serves the same database can still write one - a rollback after upgrading, a staged rollout with an old and a new instance sharing a database, a second older instance on the same file. The window closes, the guard returns, and the rows are already stored, behaving exactly like genuinely old ones. A rollback is an ordinary operational move, so this is not an exotic path. The doc now states the binary-version dependence, says which dialect is affected and why PostgreSQL is not (it refuses a NUL itself, at every version), and gives the operational answer: drain writes from older binaries before the new one serves, or roll forward rather than back. Store-layer enforcement - so the running build stops mattering - is filed as BUG-2813 rather than added here. It is a dialect-level change and the day-68 ruling on this unit is land-and-follow. The same false implication was carried by the PR's release note calling such a workspace "legacy"; corrected there too. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- docs/backup.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/docs/backup.md b/docs/backup.md index 59fed808b..13db0e257 100644 --- a/docs/backup.md +++ b/docs/backup.md @@ -168,8 +168,22 @@ your backup — it is the import applying a rule the write path now applies too (BUG-2803): a NUL cannot be stored in a text or JSON column, and PostgreSQL refuses it outright. -It can only affect data written **before** that rule existed, and only on -SQLite, which accepted it. A PostgreSQL instance never stored such a value. +**The rule lives in the binary, not in the database**, so "before the rule +existed" is a statement about which build served the write, not about a date. +On SQLite, any window in which an older binary serves the same database can +still create such rows: a rollback to the previous version, a staged rollout +where an old and a new instance share a database, or a second older instance +pointed at the same file. Once that window closes the guard is back, but the +rows are already stored, and they behave exactly like genuinely old ones. + +Only SQLite is affected. PostgreSQL refuses a NUL in a text or JSON column +itself, at every binary version, so a PostgreSQL instance never stored such a +value regardless of which build wrote it. + +If you want the guarantee rather than the guard, drain writes from older +binaries before the new one starts serving, or roll forward rather than back. +Enforcing the invariant below the HTTP layer, so the running build stops +mattering, is tracked as BUG-2813. The same limitation applies to `pad db migrate-to-pg`, which copies rows directly and does not go through the import guard: a row carrying a NUL will From 30765f082d346a6f2e24467dd2337815bb4311a6 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 02:24:29 +0000 Subject: [PATCH 25/29] docs(server): cite the ruling in house style, not the team-room day counter (BUG-2803) "lead ruling day-68" is the internal day counter, which means nothing to anyone reading this repo and is inconsistent with every other citation in it - the codebase cites a lead ruling by DATE or by BUG ref, never by day-N. Replaced with the bug ref, which is the part a reader can actually follow. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/decode_json_nul_test.go | 2 +- internal/server/middleware_request_text.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 73989181c..aa8efb25f 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -1057,7 +1057,7 @@ func TestBodyDecodesNULMatchesKeysLikeTheDecoder(t *testing.T) { // TestBodyDecodesNULKnownMapModelDisagreements pins the four measured // disagreements between this scan's map[string]any model and encoding/json's -// typed decode (BUG-2803 rounds 16-17, lead ruling day-68: land-and-follow). +// typed decode (BUG-2803 rounds 16-17; lead ruling: land-and-follow). // They are documented on bodyDecodesNUL; this is the instrument that keeps // that documentation and the release note from going stale. // diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 7f4314c0b..e3646e9ad 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -429,7 +429,7 @@ var unicodeEscapePrefix = []byte{'\\', 'u', '0', '0'} // // 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 day-68: land-and-follow). They are +// (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. From 035a75cfe04ce15c02b0b34c1a3d8b88faef1a06 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 02:47:32 +0000 Subject: [PATCH 26/29] docs(server): drop a commit count I had already measured as wrong, and stop asserting a cause I borrowed (BUG-2803) Two defects in a comment I wrote an hour ago, both of the kind this unit's trail keeps recording. "an 18-commit branch" - the branch was 20 commits at b0192871 when I counted it this session, and is more now. 18 came from the previous checkpoint's own miscount, which I had ALREADY identified and written up before I typed it again here. A number that arrives inside a sentence about something else does not feel like a claim, which is exactly why it survives. The count is incidental to the argument, so it is gone rather than corrected - a figure that has to be maintained to stay true is a liability in a doc comment. "this branch's one regression came from exactly that" - the ruling's reasoning, restated by me as a verified fact. The regression I know about came from wiring a fix off a reviewer-named sink list without reading the mechanism, which is adjacent to "restructuring late under review pressure" but is not the same mechanism, and I did not check whether it is the one the ruling meant. Now attributed to the ruling and stated as its reasoning, with the part I can defend - the review loop finding something in nearly every round indicates a design problem - carrying the argument. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/middleware_request_text.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index e3646e9ad..4832cdff4 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -441,9 +441,10 @@ var unicodeEscapePrefix = []byte{'\\', 'u', '0', '0'} // // 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; patching them into the map model would be re-plumbing an -// 18-commit branch late under review pressure, and this branch's one -// regression came from exactly 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":""},"fields_patch":{"status":"open"}}, From f94148e5d7f533baaeae5fa9b85c04e08c3e7282 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 03:18:54 +0000 Subject: [PATCH 27/29] fix(server): sanitise the MCP audit tool_name, and correct three claims wider than their evidence (BUG-2803) Codex round 20, asked for a POPULATION rather than a confirmation (CONVE-24). It returned a covered list AND four findings; this commit carries the two that belong to this unit plus the doc corrections. ## The door: MCP audit is a second reader, not a pass-through parseMCPRequestBody runs its OWN json.Unmarshal and binds the decoded method / params.name to mcp_audit_log.tool_name, TEXT NOT NULL. A six-character NUL escape therefore arrives as a real NUL: PostgreSQL refuses the audit INSERT with 22021 - the exact symptom this unit exists to remove - and SQLite stores an unprintable tool name. Nothing upstream catches it; the /mcp transport decodes the JSON-RPC envelope itself rather than through decodeJSON, so the body rule never sees the request. Measured before fixing: the decoded name reached the column intact. This unit's own completeness map had CERTIFIED that reader as safe, on the grounds that "decoding still happens in the MCP dispatcher". That is true and it does not bear on what this middleware persists - a correct description of a mechanism, with no question asked about what it does, sitting in the one artifact whose job is to say the population is covered. Corrected there too. Disposition is SANITISE, not refuse, following the User-Agent precedent from earlier in this unit, and the rule now lives in one extracted helper (sanitiseStoredText) with the reasoning attached: the body rule refuses because the caller asked to store that value; this serves metadata the SERVER elected to record, where failing the write would lose the audit row for precisely the request most worth auditing. Both caller-derived returns are cleaned inside parseMCPRequestBody, so both call sites - the ok path and the denied path - are covered at the choke point rather than at either caller. Both are tested: params.name AND the method path. Mutations un-sanitising each one compile and kill only their own leg. ## Three claims corrected, all wider than their evidence - "all 65 call sites" in server.go: measured 70. Removed rather than corrected, because the number has to be maintained to stay true and says nothing the sentence needs. - docs/backup.md said a NUL "cannot be stored in a text or JSON column" absolutely, two paragraphs above my own text explaining that SQLite accepts one. Now stated as what it is: an application rule Pad enforces on both dialects, which is exactly why it has to be enforced. - artifact_import.go said such a value "cannot be stored under any encoding this product supports". Refuses, not cannot - stating a policy as a capability tells the next reader SQLite enforces something it does not. ## Filed, not fixed BUG-2814 - guarded writes re-emit at-rest NULs (move/copy/restore/ fields-patch), propagating a legacy value to rows that never had one. Distinct from BUG-2813: that one is about writing a NUL while an old binary serves, this is the fixed binary SPREADING one already present. Both dissolve under the same store-layer enforcement, so they are filed to be designed together rather than patched at each of a long and moving list of re-emit sites. Declined: round 20 also reported the release-note assertions as unsupported. They live in the PR body, which a read-only sandbox cannot see - the claim is about the reviewer's visibility, not the diff. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- docs/backup.md | 6 +- internal/server/artifact_import.go | 13 +++- internal/server/decode_json_nul_test.go | 2 +- internal/server/middleware_mcp_audit.go | 24 +++++- internal/server/middleware_mcp_audit_test.go | 80 ++++++++++++++++++++ internal/server/middleware_request_text.go | 24 +++++- internal/server/server.go | 2 +- 7 files changed, 140 insertions(+), 11 deletions(-) diff --git a/docs/backup.md b/docs/backup.md index 13db0e257..1612c6acf 100644 --- a/docs/backup.md +++ b/docs/backup.md @@ -165,8 +165,10 @@ pad workspace import --name "imported-workspace" < my-workspace.json A workspace whose stored data contains a **NUL character** exports fine and is refused on import, with a 400 naming the cause. This is not a corruption of your backup — it is the import applying a rule the write path now applies too -(BUG-2803): a NUL cannot be stored in a text or JSON column, and PostgreSQL -refuses it outright. +(BUG-2803): Pad does not accept a NUL in a text or JSON value. That is an +application rule, not a universal storage fact — PostgreSQL does refuse a NUL +outright, but SQLite accepts one in a TEXT column, which is why the rule has to +be enforced rather than assumed, and why the paragraphs below matter. **The rule lives in the binary, not in the database**, so "before the rule existed" is a statement about which build served the write, not about a date. diff --git a/internal/server/artifact_import.go b/internal/server/artifact_import.go index ac4ca6fbb..f519998c3 100644 --- a/internal/server/artifact_import.go +++ b/internal/server/artifact_import.go @@ -52,9 +52,16 @@ var ErrArtifactUnsafeYAML = errors.New("artifact import: frontmatter rejected by // ErrArtifactUnbindableText is returned when the artifact body is not text the // database can be asked to store — invalid UTF-8, or carrying a NUL. It is a -// client error (400), not a 500, for the reason BUG-2782 gives: the value -// cannot be stored under any encoding this product supports, so the caller -// sent something that cannot mean anything here. +// client error (400), not a 500, for the reason BUG-2782 gives: this is a +// value Pad refuses to store, so the caller sent something that cannot mean +// anything here. +// +// "Refuses", not "cannot" — the distinction matters and the earlier wording +// blurred it (codex round 20). PostgreSQL rejects a NUL in a text or JSON +// column outright; SQLite would accept one in TEXT. So this is an application +// rule Pad applies on both dialects, not a storage limit it inherits from +// either. Stating it as a capability would tell the next reader that SQLite +// enforces something it does not. var ErrArtifactUnbindableText = errors.New("artifact import: body contains invalid UTF-8 or a NUL byte") // parseArtifactRequest is the guarded HTTP-boundary parse used by the import diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index aa8efb25f..1fc06b4d7 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -240,7 +240,7 @@ func TestEveryRequestBodyReaderIsAccountedFor(t *testing.T) { "handlers_attachments.go": "multipart upload — the body is binary blob content, not text, and must NOT be scanned for text validity", "artifact_import.go": "raw artifact TEXT (not JSON) — checked with bindableText, the same predicate ValidatePath and ValidateQuery apply", "handlers_cloud.go": "bodyHasCloudSecret PEEKS at the body and restores it; the real decode still happens through decodeJSON downstream", - "middleware_mcp_audit.go": "audit capture — records the body for the MCP audit log and restores it; decoding still happens in the MCP dispatcher", + "middleware_mcp_audit.go": "audit capture — parses the body ITSELF and binds the decoded method / params.name to mcp_audit_log.tool_name, so it is a second READER, not a pass-through. That the MCP dispatcher decodes the body again is true and says nothing about what this middleware persists — the earlier rationale here made exactly that mistake and certified it safe (codex round 20). parseMCPRequestBody now runs both caller-derived returns through sanitiseStoredText", "handlers_tokens.go": "a nil/ContentLength check only — it never reads the body", "handlers_oauth.go": "KNOWN GAP, tracked as BUG-2811: the OAuth handlers read FORM-encoded bodies (r.Form/FormValue), which no rule in this family covers — the transport rules see the query half of r.Form and not the body half. Listed so this test states the gap instead of being blind to it; measuring it needs a fosite-backed fixture.", } diff --git a/internal/server/middleware_mcp_audit.go b/internal/server/middleware_mcp_audit.go index 5f66b95eb..eb0fa3ea0 100644 --- a/internal/server/middleware_mcp_audit.go +++ b/internal/server/middleware_mcp_audit.go @@ -369,6 +369,26 @@ func (s *Server) recordMCPCallMetrics(tool, status, userID string, dur time.Dura // Returns ("(unknown)", "") on parse failure or empty body — gives // the audit reader a visible signal rather than silently dropping // the row. +// +// Both values that come from the CALLER's body are run through +// sanitiseStoredText before they leave this function, because they +// are bound to mcp_audit_log.tool_name (TEXT NOT NULL). This +// middleware runs its own json.Unmarshal, so a `\u0000` escape in +// `method` or `params.name` arrives here as a real NUL: PostgreSQL +// then refuses the audit INSERT with 22021 and SQLite stores an +// unprintable tool name. Nothing upstream catches it — the /mcp +// transport decodes the JSON-RPC envelope itself rather than through +// decodeJSON, so the body rule never sees this request (BUG-2803, +// codex round 20; that unit's completeness map had wrongly certified +// this reader as safe on the grounds that the dispatcher decodes the +// body again, which is true and does not bear on what is persisted +// here). +// +// Sanitising rather than refusing is deliberate: see sanitiseStoredText. +// The request carrying a malformed name is the one most worth having +// an audit row for. Both callers of this function — the ok path and +// the denied path — are covered because the cleaning happens here +// rather than at either call site. func parseMCPRequestBody(body []byte) (toolName, argsHash string) { if len(body) == 0 { return "(unknown)", "" @@ -381,7 +401,7 @@ func parseMCPRequestBody(body []byte) (toolName, argsHash string) { return "(unknown)", "" } if env.Method != "tools/call" { - return env.Method, "" + return sanitiseStoredText(env.Method), "" } var p struct { Name string `json:"name"` @@ -390,7 +410,7 @@ func parseMCPRequestBody(body []byte) (toolName, argsHash string) { if err := json.Unmarshal(env.Params, &p); err != nil || p.Name == "" { return "tools/call", "" } - return p.Name, hashCanonicalJSON(p.Arguments) + return sanitiseStoredText(p.Name), hashCanonicalJSON(p.Arguments) } // hashCanonicalJSON returns a SHA-256 hex of a canonicalized form of diff --git a/internal/server/middleware_mcp_audit_test.go b/internal/server/middleware_mcp_audit_test.go index 0f0d02073..c9fed2efc 100644 --- a/internal/server/middleware_mcp_audit_test.go +++ b/internal/server/middleware_mcp_audit_test.go @@ -377,3 +377,83 @@ func TestMCPAudit_ClassifyResult(t *testing.T) { } } } + +// TestMCPAudit_ToolNameCannotCarryANUL closes a door BUG-2803's own +// completeness map wrongly certified as safe (codex round 20). +// +// That map listed middleware_mcp_audit.go as "audit capture — records the body +// and restores it; decoding still happens in the MCP dispatcher". The first +// half is true and the second is irrelevant: parseMCPRequestBody runs its OWN +// json.Unmarshal and hands the DECODED params.name straight to +// mcp_audit_log.tool_name, a TEXT NOT NULL column. Whether the dispatcher +// decodes the body again has no bearing on what this middleware persists. +// +// The /mcp transport decodes the JSON-RPC envelope itself rather than through +// decodeJSON, so nothing upstream refuses the escape either. On PostgreSQL the +// insert fails with 22021 — the exact symptom this whole unit exists to +// remove; on SQLite it is stored and the audit log carries an unprintable +// tool name. +// +// The disposition is SANITISE, not refuse, following the User-Agent precedent +// established earlier in this unit: the audit row is metadata the SERVER chose +// to record about a request, and the request that carries a malformed value is +// precisely the one you most want a row for. Failing the audit write would +// lose that row, which is worse than recording a cleaned name. +func TestMCPAudit_ToolNameCannotCarryANUL(t *testing.T) { + srv, user, bearer := auditedMCPServer(t) + + post := func(t *testing.T, body string) { + t.Helper() + req := httptest.NewRequest("POST", "/mcp", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+bearer) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "192.0.2.1:1234" + srv.ServeHTTP(httptest.NewRecorder(), req) + } + + // Control FIRST, so the test asserts its own premise: an ordinary name is + // stored verbatim. Without this leg the assertion below would pass against + // a middleware that mangled or dropped every tool name. + post(t, `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"pad_item","arguments":{}}}`) + rows := waitForAuditRows(t, srv, user.ID, 1) + if rows[0].ToolName != "pad_item" { + t.Fatalf("premise failed: an ordinary tool name must round-trip verbatim, got %q", rows[0].ToolName) + } + + post(t, `{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"pad_item`+escNULLiteral+`evil","arguments":{}}}`) + rows = waitForAuditRows(t, srv, user.ID, 2) + + for _, row := range rows { + if strings.ContainsRune(row.ToolName, 0) { + t.Errorf("a decoded NUL reached mcp_audit_log.tool_name (%q). On PostgreSQL this insert "+ + "fails with 22021; the value must be sanitised before it is persisted", row.ToolName) + } + } + + // And the sanitised name must still IDENTIFY the tool — dropping the row + // or storing "(unknown)" would make the audit log useless for exactly the + // requests worth auditing. + var sanitised string + for _, row := range rows { + if row.ToolName != "pad_item" { + sanitised = row.ToolName + } + } + if want := "pad_itemevil"; sanitised != want { + t.Errorf("the NUL-bearing call should be recorded under a cleaned name %q, got %q", want, sanitised) + } + + // The OTHER path into tool_name. parseMCPRequestBody has two returns that + // carry caller text - params.name for tools/call, and the METHOD itself + // for everything else - and both are bound to the same column. Testing + // only the first would leave the second's sanitise call unkilled by any + // mutation, which is how a fixed surface count turns back into a defect. + post(t, `{"jsonrpc":"2.0","id":3,"method":"tools/li`+escNULLiteral+`st"}`) + rows = waitForAuditRows(t, srv, user.ID, 3) + for _, row := range rows { + if strings.ContainsRune(row.ToolName, 0) { + t.Errorf("a decoded NUL reached tool_name via the METHOD path (%q); params.name is not "+ + "the only return that carries caller text", row.ToolName) + } + } +} diff --git a/internal/server/middleware_request_text.go b/internal/server/middleware_request_text.go index 4832cdff4..7f03b5f43 100644 --- a/internal/server/middleware_request_text.go +++ b/internal/server/middleware_request_text.go @@ -774,6 +774,26 @@ func truncateBindableText(s string, maxBytes int) string { // 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 { - ua := strings.ToValidUTF8(r.Header.Get("User-Agent"), "") - return strings.ReplaceAll(ua, "\x00", "") + 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 { + return strings.ReplaceAll(strings.ToValidUTF8(s, ""), "\x00", "") } diff --git a/internal/server/server.go b/internal/server/server.go index 0be99ac0a..917f78741 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -2230,7 +2230,7 @@ func decodeJSONWithLimit(r *http.Request, v interface{}, maxBytes int64) error { // errJSONBodyNUL is returned by decodeJSON when a string in the request body // decodes to a value containing a NUL. Every decodeJSON caller already turns // a decode error into a 400, so the refusal reaches the client as a client -// error at all 65 call sites without touching any of them. +// error at every call site without touching any of them. // // NOT every caller shows this message, and the earlier version of this // comment claimed otherwise: many substitute a generic string of their own From c5ce999a8d12e47c8893f35dabfbb3f5ddccecf7 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 13:41:56 +0000 Subject: [PATCH 28/29] fix(server): sanitise before testing for emptiness, so the audit fallback survives (BUG-2803) Codex round 21 ranked this the most dangerous un-probed lens, and it is a boundary my own round-20 fix created. parseMCPRequestBody tested env.Method == "" and p.Name == "" BEFORE sanitising. A value made entirely of NUL escapes is non-empty as decoded and empty once cleaned, so it passed over the fallback and was then blanked - storing an empty tool_name in a TEXT NOT NULL column. That is exactly the silent drop the "(unknown)" / "tools/call" fallbacks exist to prevent; the function's own doc comment says so. Measured before fixing: both shapes returned an empty tool_name. Fixed by ordering rather than by adding guards - clean first, then test - so the invariant is structural instead of something each return has to remember. Same by-construction preference as the symmetric-gate fix earlier in this unit. Worth recording that my first patch was WRONG in a way that compiled: I put the sanitise above the json.Unmarshal that populates env, so the method would always have been empty. Caught by printing the patched function and reading it, not by trusting the script saying "patched". Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/middleware_mcp_audit.go | 25 ++++++++--- internal/server/middleware_mcp_audit_test.go | 45 ++++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/internal/server/middleware_mcp_audit.go b/internal/server/middleware_mcp_audit.go index eb0fa3ea0..d728aa3f8 100644 --- a/internal/server/middleware_mcp_audit.go +++ b/internal/server/middleware_mcp_audit.go @@ -397,20 +397,35 @@ func parseMCPRequestBody(body []byte) (toolName, argsHash string) { Method string `json:"method"` Params json.RawMessage `json:"params"` } - if err := json.Unmarshal(body, &env); err != nil || env.Method == "" { + if err := json.Unmarshal(body, &env); err != nil { return "(unknown)", "" } - if env.Method != "tools/call" { - return sanitiseStoredText(env.Method), "" + // SANITISE FIRST, THEN test for emptiness — not the other way round. A + // value made entirely of NUL escapes is non-empty as decoded and empty + // once cleaned, so checking first passed it over the fallback and then + // blanked it, storing an empty tool_name. That is precisely the silent + // drop these fallbacks exist to prevent (codex round 21 — the boundary + // the round-20 sanitise created and did not test). Cleaning before the + // test makes the invariant structural instead of something every return + // has to remember. + method := sanitiseStoredText(env.Method) + if method == "" { + return "(unknown)", "" + } + if method != "tools/call" { + return method, "" } var p struct { Name string `json:"name"` Arguments json.RawMessage `json:"arguments"` } - if err := json.Unmarshal(env.Params, &p); err != nil || p.Name == "" { + if err := json.Unmarshal(env.Params, &p); err != nil { return "tools/call", "" } - return sanitiseStoredText(p.Name), hashCanonicalJSON(p.Arguments) + if name := sanitiseStoredText(p.Name); name != "" { + return name, hashCanonicalJSON(p.Arguments) + } + return "tools/call", "" } // hashCanonicalJSON returns a SHA-256 hex of a canonicalized form of diff --git a/internal/server/middleware_mcp_audit_test.go b/internal/server/middleware_mcp_audit_test.go index c9fed2efc..19617bf58 100644 --- a/internal/server/middleware_mcp_audit_test.go +++ b/internal/server/middleware_mcp_audit_test.go @@ -457,3 +457,48 @@ func TestMCPAudit_ToolNameCannotCarryANUL(t *testing.T) { } } } + +// TestMCPAudit_SanitiseNeverEmptiesToolName covers the boundary the round-20 +// fix created and did not test (codex round 21, top-ranked un-probed lens). +// +// parseMCPRequestBody checks env.Method == "" and p.Name == "" BEFORE +// sanitising, so a value made ENTIRELY of NULs is non-empty at the fallback +// and empty by the time it is returned. tool_name is TEXT NOT NULL, so the +// row still inserts — with an empty identifier, which defeats the whole point +// of the "(unknown)" / "tools/call" fallbacks. That function's own doc comment +// says they exist to give the audit reader a visible signal rather than +// silently dropping the row; an empty string is the silent drop wearing a +// different shape. +// +// This is the round-20 fix's own boundary: I closed the NUL door and did not +// ask what the close does when it consumes the entire value. +func TestMCPAudit_SanitiseNeverEmptiesToolName(t *testing.T) { + nul := "\\u0000" // the escape, not the character + + for _, tc := range []struct { + name string + body string + want string + }{ + {"method is all NULs", `{"jsonrpc":"2.0","id":1,"method":"` + nul + `"}`, "(unknown)"}, + {"tools/call name is all NULs", + `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"` + nul + `"}}`, "tools/call"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, _ := parseMCPRequestBody([]byte(tc.body)) + if got == "" { + t.Fatalf("tool_name came back EMPTY; the fallback must survive sanitisation, want %q", tc.want) + } + if got != tc.want { + t.Errorf("tool_name = %q, want the fallback %q", got, tc.want) + } + }) + } + + // Premise: the same shapes with ordinary values still return the value + // itself, so the assertions above are about emptiness rather than about + // the fallback swallowing everything. + if got, _ := parseMCPRequestBody([]byte(`{"method":"tools/list"}`)); got != "tools/list" { + t.Fatalf("premise failed: an ordinary method must round-trip, got %q", got) + } +} From aeaddd3f9d05723a1c255b0363844a57b599b7f5 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 30 Aug 2026 14:06:39 +0000 Subject: [PATCH 29/29] fix(server): classify MCP audit on the raw method, and trim only JSON whitespace (BUG-2803) Codex round 22. Two P2s, both measured before fixing. ## A forgeable audit row - my own regression from the round-21 fix The round-21 change reordered sanitise-before-compare so the fallback would survive an all-NUL value. That reorder made the CLASSIFICATION read the sanitised method, so "tools/call" cleaned up INTO the literal "tools/call" and the parser then lifted params.name and hashed the arguments for a method that was never tools/call. Measured: tool_name="pad_item" with a full 64-character args_hash - an audit row indistinguishable from a genuine pad_item call, mintable by anyone who can send a request. Worse than the review described it. Fixed by splitting the two jobs, which were never the same job: dispatch decisions read what the client actually SENT; sanitising is for the value that gets STORED. The round-21 boundary is preserved - a method empty only after cleaning still falls back to "(unknown)". Fixing one boundary and creating another in the same function is worth naming: the reorder was correct for the case it addressed and I did not ask what else read that value. ## Go whitespace is not JSON whitespace The empty-body shortcut used bytes.TrimSpace, i.e. unicode.IsSpace, which strips \v, \f, U+00A0 and more. encoding/json accepts none of them. So a body of just \v trimmed to empty, returned io.EOF, and an EOF-tolerant caller - playbook run treats errors.Is(err, io.EOF) as "no arguments supplied" and runs anyway - took a syntactically invalid body for an ABSENT one. Now trims exactly the four bytes JSON calls whitespace. The test drives both directions, because only the pair discriminates: real JSON whitespace must still shortcut to EOF or the playbook contract breaks, and non-JSON whitespace must not or the divergence survives. Reverting to TrimSpace compiles and fails three legs. Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p --- internal/server/decode_json_nul_test.go | 49 ++++++++++++++++++++ internal/server/middleware_mcp_audit.go | 20 ++++++-- internal/server/middleware_mcp_audit_test.go | 32 +++++++++++++ internal/server/server.go | 10 +++- 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/internal/server/decode_json_nul_test.go b/internal/server/decode_json_nul_test.go index 1fc06b4d7..c70e23090 100644 --- a/internal/server/decode_json_nul_test.go +++ b/internal/server/decode_json_nul_test.go @@ -1181,3 +1181,52 @@ func TestUnknownFieldRefusalThroughTheHandler(t *testing.T) { t.Errorf("the 400 should name the cause, got: %s", got.Body.String()) } } + +// TestDecodeJSONTrimsOnlyJSONWhitespace pins that the empty-body shortcut uses +// JSON's whitespace set, not Go's (codex round 22). +// +// bytes.TrimSpace uses unicode.IsSpace, which strips \v, \f, U+00A0 and more. +// encoding/json accepts none of those. So a body of just "\v" trimmed to +// EMPTY here and returned io.EOF, and an EOF-tolerant caller — playbook run +// treats errors.Is(err, io.EOF) as "no arguments supplied" and runs anyway — +// took a syntactically invalid body for an ABSENT one. +// +// The four legs matter in pairs: real JSON whitespace must still shortcut to +// EOF (or the playbook contract breaks), and non-JSON whitespace must NOT (or +// the divergence is still there). +func TestDecodeJSONTrimsOnlyJSONWhitespace(t *testing.T) { + srv := testServer(t) + + for _, tc := range []struct { + name string + body string + wantEOF bool + }{ + {"space and tab and newline", " \t\r\n", true}, + {"empty", "", true}, + {"vertical tab", "\v", false}, + {"form feed", "\f", false}, + {"non-breaking space", "\u00a0", false}, + } { + t.Run(tc.name, func(t *testing.T) { + var v map[string]any + req := httptest.NewRequest("POST", "/x", strings.NewReader(tc.body)) + err := decodeJSON(req, &v) + if err == nil { + t.Fatalf("expected an error for body %q", tc.body) + } + gotEOF := errors.Is(err, io.EOF) + if gotEOF != tc.wantEOF { + if tc.wantEOF { + t.Errorf("body %q is JSON whitespace and must read as an ABSENT body (io.EOF), "+ + "or the playbook-run empty-body contract breaks; got %v", tc.body, err) + } else { + t.Errorf("body %q is NOT JSON whitespace — encoding/json would reject it — so it "+ + "must NOT be reported as an absent body; an EOF-tolerant caller would proceed "+ + "on invalid input. got io.EOF", tc.body) + } + } + }) + } + _ = srv +} diff --git a/internal/server/middleware_mcp_audit.go b/internal/server/middleware_mcp_audit.go index d728aa3f8..6149372a2 100644 --- a/internal/server/middleware_mcp_audit.go +++ b/internal/server/middleware_mcp_audit.go @@ -408,13 +408,23 @@ func parseMCPRequestBody(body []byte) (toolName, argsHash string) { // the round-20 sanitise created and did not test). Cleaning before the // test makes the invariant structural instead of something every return // has to remember. - method := sanitiseStoredText(env.Method) - if method == "" { + // CLASSIFY ON THE RAW METHOD, store the sanitised one. Sanitising first + // and then comparing let "tools/call" clean up INTO "tools/call", + // so the parser extracted params.name and hashed the arguments for a + // method that was never tools/call — producing an audit row identical to + // a genuine call (measured: tool_name="pad_item" with a full args_hash). + // That was a regression introduced by the round-21 fix, which reordered + // these two steps to keep the fallback alive; codex round 22 caught it. + // Cleaning is for the value that gets STORED. Dispatch decisions read + // what the client actually sent. + if env.Method != "tools/call" { + if method := sanitiseStoredText(env.Method); method != "" { + return method, "" + } + // Empty only after cleaning — keep the visible signal rather than + // storing "". This is the round-21 boundary, preserved. return "(unknown)", "" } - if method != "tools/call" { - return method, "" - } var p struct { Name string `json:"name"` Arguments json.RawMessage `json:"arguments"` diff --git a/internal/server/middleware_mcp_audit_test.go b/internal/server/middleware_mcp_audit_test.go index 19617bf58..15e01094e 100644 --- a/internal/server/middleware_mcp_audit_test.go +++ b/internal/server/middleware_mcp_audit_test.go @@ -502,3 +502,35 @@ func TestMCPAudit_SanitiseNeverEmptiesToolName(t *testing.T) { t.Fatalf("premise failed: an ordinary method must round-trip, got %q", got) } } + +// TestMCPAudit_RawMethodDecidesClassification pins that dispatch reads what the +// client SENT while only the stored value is cleaned (codex round 22). +// +// The round-21 fix sanitised before comparing, so "tools/call" cleaned up +// INTO the literal "tools/call" and the parser then extracted params.name and +// hashed the arguments. Measured before the fix: tool_name="pad_item" with a +// full 64-character args_hash — an audit row indistinguishable from a genuine +// pad_item call, forgeable by anyone who can send a request. +func TestMCPAudit_RawMethodDecidesClassification(t *testing.T) { + nul := "\u0000" // the escape, not the character + + name, hash := parseMCPRequestBody([]byte( + `{"method":"tools/` + nul + `call","params":{"name":"pad_item","arguments":{}}}`)) + if name == "pad_item" { + t.Errorf("a method that is not tools/call must NOT have params.name lifted out of it; " + + "tool_name came back as the inner tool name, which forges a genuine call") + } + if hash != "" { + t.Errorf("args_hash must be empty for a non-tools/call method, got %d chars", len(hash)) + } + + // Control: a genuine tools/call still classifies and still hashes, so the + // assertions above pin the classification rather than a parser that + // stopped working. + name, hash = parseMCPRequestBody([]byte( + `{"method":"tools/call","params":{"name":"pad_item","arguments":{}}}`)) + if name != "pad_item" || hash == "" { + t.Fatalf("premise failed: a genuine tools/call must still yield its name and a hash, got %q / %d chars", + name, len(hash)) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 917f78741..10e3c5889 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -2203,7 +2203,15 @@ func decodeJSONWithLimit(r *http.Request, v interface{}, maxBytes int64) error { // while json.Unmarshal answers a SyntaxError instead, which that check // cannot see. Found by TestPlaybookRunAcceptsEmptyBody, which is exactly // the wiring a helper-level change is blind to. - if len(bytes.TrimSpace(raw)) == 0 { + // Trim only the four bytes JSON itself calls whitespace. bytes.TrimSpace + // uses unicode.IsSpace, which also strips \v, \f, U+00A0 and friends — + // none of which encoding/json accepts. With TrimSpace a body of just + // "\v" looked EMPTY here and returned io.EOF, so an EOF-tolerant caller + // (playbook run, share links) treated a syntactically invalid body as an + // ABSENT one and proceeded. Same Go-versus-spec whitespace divergence + // that bites when a Go trim stands in for another grammar's definition + // (codex round 22). + if len(bytes.Trim(raw, " \t\r\n")) == 0 { return fmt.Errorf("invalid JSON: %w", io.EOF) } // Refuse a decoded NUL BEFORE unmarshalling, so the value never exists