diff --git a/docs/backup.md b/docs/backup.md index 9076b57b..1612c6ac 100644 --- a/docs/backup.md +++ b/docs/backup.md @@ -160,6 +160,43 @@ 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): 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. +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 +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 diff --git a/internal/server/artifact_import.go b/internal/server/artifact_import.go index 0a1729e9..f519998c 100644 --- a/internal/server/artifact_import.go +++ b/internal/server/artifact_import.go @@ -1,6 +1,7 @@ package server import ( + "encoding/json" "errors" "fmt" "io" @@ -49,19 +50,41 @@ 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: 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 -// 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) { @@ -83,16 +106,39 @@ func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64 return artifact.Artifact{}, fmt.Errorf("artifact import: read body: %w", err) } - // (2) YAML-bomb guard on the frontmatter region only. + // (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 + // 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 + } + + // (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 } + + // (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 + // 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 } @@ -194,3 +240,57 @@ func extractFrontmatterRegion(s string) (string, bool) { offset += nl + 1 } } + +// 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. +// +// 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 { + 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 + } + // 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 new file mode 100644 index 00000000..1fc06b4d --- /dev/null +++ b/internal/server/decode_json_nul_test.go @@ -0,0 +1,1183 @@ +package server + +import ( + "bytes" + "database/sql" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/artifact" +) + +// 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) + } + } +} + +// 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 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 — 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.", + } + + // 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 { + t.Fatalf("read package dir: %v", err) + } + touches := map[string]bool{} + 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(".", name)) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + if pattern.Match(src) { + touches[name] = true + } + } + + // 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(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) + } + } +} + +// 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}, + // 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}, + // 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. + // ...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. + {"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}, + // 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", + `{"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) + } + }) + } +} + +// TestBodyDecodesNULLeavesTextFieldsAlone is codex round 2's finding on +// BUG-2803, pinned so it cannot come back. +// +// 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) + } + } +} + +// 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") + } + 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") + } +} + +// 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()) + } +} + +// 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) + } + // 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) + } + }) + } + + // 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") + } +} + +// 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) + } + + 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") + } +} + +// 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) + } + } +} + +// 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()) + } +} + +// 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()) + } +} + +// 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"}` + // 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 "+ + "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") + } +} + +// 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: 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") + } + }) +} + +// 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()) + } +} diff --git a/internal/server/handlers_admin.go b/internal/server/handlers_admin.go index 852d55ab..0e0cd3a4 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 f96e2c9b..b9484c84 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 e31572b2..8436b778 100644 --- a/internal/server/handlers_artifact_test.go +++ b/internal/server/handlers_artifact_test.go @@ -573,3 +573,64 @@ 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')}, + // 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. + // 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) { + 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_attachments.go b/internal/server/handlers_attachments.go index 812830b0..12b0d104 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 cb24f04d..1078f584 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,137 @@ 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()) + } + // 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 { + 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_attachments_transform.go b/internal/server/handlers_attachments_transform.go index 0fcdc380..bc998a6a 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_cloud.go b/internal/server/handlers_cloud.go index 08a866cc..7083fbb4 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_comments.go b/internal/server/handlers_comments.go index e5d4fd39..42fb7bae 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_connected_apps.go b/internal/server/handlers_connected_apps.go index fce40630..b5ad18a8 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 30b14e85..c2490c20 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_import_bundle.go b/internal/server/handlers_import_bundle.go index a8d1384a..c32c063b 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{ @@ -324,6 +336,31 @@ 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 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) + } 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) diff --git a/internal/server/handlers_import_bundle_test.go b/internal/server/handlers_import_bundle_test.go index 98deb82d..2916521d 100644 --- a/internal/server/handlers_import_bundle_test.go +++ b/internal/server/handlers_import_bundle_test.go @@ -632,3 +632,162 @@ 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()) + } +} + +// 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()) + } + 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()) + } +} diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index efc0b6fe..333894c8 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 962c9a78..55584877 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -227,9 +227,17 @@ func (s *Server) handleOAuthRegister(w http.ResponseWriter, r *http.Request) { } var input dcrRequest - if err := json.NewDecoder(r.Body).Decode(&input); err != nil { - writeDCRError(w, http.StatusBadRequest, "invalid_client_metadata", - "Request body must be JSON: "+err.Error()) + if err := decodeJSON(r, &input); err != nil { + // 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 } @@ -987,7 +995,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 +1792,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/handlers_reports.go b/internal/server/handlers_reports.go index c779fa4e..b443552e 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/handlers_timeline.go b/internal/server/handlers_timeline.go index 023b9e1e..c10100c8 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. diff --git a/internal/server/handlers_timeline_cursor_validation_test.go b/internal/server/handlers_timeline_cursor_validation_test.go index d64f1100..9c9a3b6a 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,33 @@ 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 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( + `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") diff --git a/internal/server/middleware_mcp_audit.go b/internal/server/middleware_mcp_audit.go index 5f66b95e..eb0fa3ea 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 0f0d0207..c9fed2ef 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 4b8b0847..7f03b5f4 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,454 @@ 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 +// a NUL. +// +// WHY THE BODY NEEDS ITS OWN RULE, when ValidatePath and ValidateQuery already +// apply bindableText at the transport. Those work because a decoded path or +// query value is a substring of the raw request with ASCII substitutions: the +// bad byte in the raw text IS the bad byte in the value, so a middleware can +// find it without parsing. That property does not hold for a JSON body. The +// reachable NUL arrives as a six-character JSON escape (backslash, u, and +// four zeros — spelled out rather than written, since a literal is one +// transformation away from being the character it describes), all ordinary +// ASCII, so +// a transport-level scan for a NUL byte sees nothing, and no request +// middleware can find it without decoding the body, which is the handler's +// job. BUG-2784 recorded this as the reason its rule stops at the query +// string; this is the missing half. +// +// THE GATE IS A BACKSLASH, NOT THE ESCAPE SUBSTRING, and the difference is a +// real bypass rather than a stylistic one. +// +// The obvious fast path — "does the raw body contain that escape?" — is +// UNSOUND, and codex round 4 on BUG-2803 demonstrated it. The escape may be +// spelled obliquely: `\u005c` decodes to a BACKSLASH, so a body carrying +// `\u005cu0000` contains no literal six-character escape anywhere in its raw +// bytes, yet the OUTER decode manufactures one inside the string, and if that +// string is re-parsed as a JSON document (see jsonEncodedFieldKeys) the +// second parse turns it into a real NUL. Measured before the fix: that body +// answered 201 through the real router while the direct spelling answered +// 400. +// +// The mistake was applying a fact about how a NUL is spelled INSIDE a decoded +// string to the RAW BYTES, where the backslash itself can be written as an +// escape. It is the same layer-confusion this whole bug is made of, three +// rounds in a row. +// +// A backslash is the sound gate: every JSON escape mechanism requires one, so +// a body with no backslash anywhere has decoded strings byte-identical to its +// raw bytes, and a raw NUL cannot survive the decoder. No backslash therefore +// means no NUL, at any depth, however spelled. Bodies WITH a backslash pay for +// an exact answer — a larger set than before (any nested JSON carries `\"`), +// which is the cost of being correct here. +// +// WHY THE EXACT STEP IS NOT A SUBSTRING SEARCH EITHER. Containing +// the escape is not sufficient either: `\\u0000` (an escaped backslash followed +// by literal text) contains the same six characters and decodes to no NUL at +// all. Refusing on the substring alone would reject a legitimate value, and in +// THIS product that is not hypothetical — items and documents store markdown, +// and writing about a JSON escape sequence is an ordinary thing for a document +// to do. +// +// So the exact step is a walk over the DECODED body (valueDecodesNUL), which +// distinguishes those cases by construction rather than by pattern, needs no +// knowledge of the destination type, and reaches nested maps such as an item's +// `fields` blob that a struct-shaped check would miss. +// +// WHY NOT REFLECT OVER THE DECODED VALUE, which is the other obvious design. +// A reflective walk sees a []byte field AFTER base64 decoding, so a body +// carrying legitimate binary — `{"b":"AQAC"}` decodes to the bytes 01 00 02 — +// would be refused for a NUL that is not text and never reaches a text +// column. A token walk sees the base64 characters instead. No request struct +// has such a field today (searched: []byte fields with a json tag in +// internal/server and internal/models, non-test — the only hit is +// models.YjsUpdate.UpdateData, which no handler decodes from a body, since +// collab moves Yjs data over the WebSocket as binary). The token walk is +// chosen so that adding one later cannot silently start rejecting valid +// requests. +// +// A malformed body returns false rather than an error: the caller's decode +// runs next and reports the JSON error itself, so there is exactly one place +// that phrases "invalid JSON" and this function never has to agree with it. +// That choice has a consequence, and it is finding (2) below rather than a +// clean separation of concerns. +// +// WHAT THIS CHECK DOES NOT COVER — four measured disagreements between what +// this scan sees and what the typed decode does, left OPEN deliberately +// (BUG-2803 rounds 16-17; lead ruling: land-and-follow). They are +// recorded here because this is the function a reader consults before +// trusting the check, and an unqualified doc comment above an incomplete +// guard is how the next person inherits a false belief. +// +// The root cause of all four is one thing: this scan decodes into +// map[string]any, and encoding/json's typed decode does NOT agree with that +// model about keys. TWO UNDER-REFUSE (a NUL gets through) and TWO +// OVER-REFUSE (a legitimate body is rejected). +// +// UNDER-REFUSALS — these are the BUG-2812 unit's spec, not a TODO here. Both +// dissolve under a token-stream walk that never builds values, which is that +// unit's design. The ruling's reasoning for not folding that rewrite in: a +// review loop finding something in nearly every round says the pre-scan +// machinery has a DESIGN problem, and the answer to that is a unit of its +// own rather than a late restructure of a branch already deep in review. +// +// 1. DUPLICATE KEYS MERGE DIFFERENTLY (P1). For +// {"fields_patch":{"orphan":""},"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 + } + var v any + if err := json.Unmarshal(raw, &v); err != nil { + // Malformed: the caller's own decode reports the JSON error, so this + // function never has to phrase one. + return false + } + return valueDecodesNUL(v, false) +} + +// jsonEncodedFieldKeys are the REQUEST-BODY keys whose STRING value is itself +// a JSON document that something downstream re-parses. They are the only keys +// under which the walk descends into a second document. +// +// WHY THE SCOPING EXISTS. The first version of this check recursed into ANY +// string that parsed as a JSON document, on the argument that a structural +// test beats a destination-typed one. Codex round 2 on BUG-2803 showed what +// that costs: a plain-text `content` value holding a JSON snippet that merely +// MENTIONS the escape was ACCEPTED before this branch, is stored in a text +// column that has no problem with it, and was newly refused — including on +// re-import of an export carrying it. Refusing input the server itself +// produced is a worse failure than the narrow door the recursion closed. +// +// WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly this +// shape for query parameters: there the set of names is UNBOUNDED BY DESIGN +// (parseItemListParams turns any unrecognised parameter into a field filter), +// so no list could be complete. Here the set is a closed property of the wire +// model — a field is JSON-encoded because a Go struct declares it as a string +// holding JSON — and TestJSONEncodedFieldKeysCoversTheModels derives it from +// internal/models and fails when a new one appears. +// +// OVER-INCLUSION IS THE SAFE DIRECTION and this list takes it: a listed key +// that is not really JSON-encoded costs one parse attempt and can only refuse +// a complete JSON document carrying the escape, while a missing key reopens a +// door. `traits` is listed by hand for that reason — it carries JSON but its +// field declaration has no comment saying so, which is exactly how the +// derivation test would have missed it, so that test asserts coverage in one +// direction only and the list is allowed to be a superset. +var jsonEncodedFieldKeys = map[string]bool{ + "config": true, + "events": true, + "fields": true, + "metadata": true, + "phase_data": true, + "plan_overrides": true, + "schema": true, + "settings": true, + "tags": true, + "traits": true, +} + +// isJSONEncodedFieldKey matches a wire key the way the DECODER that consumes +// it does: case-insensitively. +// +// encoding/json matches an incoming key to a struct field by an exact match +// first and a CASE-INSENSITIVE one otherwise, so `{"Fields":...}` and +// `{"FIELDS":...}` both land in ItemCreate.Fields. A case-SENSITIVE lookup +// here therefore skipped the nested walk for a body the handler went on to +// accept, and the database answered the original 500 (codex round 16, +// measured: `fields` refused, `Fields` and `FIELDS` accepted). +// +// This is the same defect shape as the rest of BUG-2803 — a check that agrees +// with one layer's rules while the layer that actually consumes the value +// uses different ones — and it is the reason this predicate is a function +// rather than a bare map index: the map is the vocabulary, the MATCHING RULE +// belongs to the consumer. +func isJSONEncodedFieldKey(k string) bool { + if jsonEncodedFieldKeys[k] { + return true + } + // EqualFold, not ToLower. encoding/json matches with Unicode SIMPLE + // FOLDING, which is wider than lower-casing: U+017F LATIN SMALL LETTER + // LONG S folds to 's', so "ſchema" reaches the `schema` field while + // strings.ToLower("ſchema") is still "ſchema" and missed the allowlist + // (codex round 17). Fixing the ASCII half and leaving the Unicode half + // would have been this bug's own pattern one more time. + for canonical := range jsonEncodedFieldKeys { + if strings.EqualFold(k, canonical) { + return true + } + } + return false +} + +// valueDecodesNUL walks a decoded request body for a string that contains a +// NUL, descending into a JSON document carried as a string exactly once. +// +// inUserData says the walk has left the REQUEST's own structure and is inside +// caller data — the natural object under `fields`, an element of a `tags` +// array, or a re-parsed document. The key list is not consulted there, and +// both halves of that matter: +// +// - A collection may declare a user field literally named `schema` or +// `tags`. Treating `{"fields":{"schema":"..."}}`'s inner key as a wire key +// refused valid text that happened to hold a JSON example (codex round 8). +// - Below the document Postgres parses, an escape is harmless. Measured on +// Postgres 17 with the check disabled: a NUL escape two levels deep +// imports 201, because `fields` is parsed ONCE and the inner text is +// re-escaped when the blob is written, so Postgres never sees an escape +// (codex round 7). +// +// Together those make the descent exactly one level deep by construction, +// which is why there is no depth counter here. An earlier version had one, +// bounding a recursion that inherited the flag through the whole subtree; +// with the flag no longer inherited, a counter would be a bound that can +// never fire, and dead protection reads as protection. +// +// WHY A DECODED WALK RATHER THAN reflection over the destination struct: a +// reflective walk sees []byte fields AFTER base64 decoding, so a body +// carrying legitimate binary — {"b":"AQAC"} decodes to the bytes 01 00 02 — +// would be refused for a NUL that is not text and never reaches a text +// column. Decoding into `any` never produces a []byte, so the value seen here +// is the base64 TEXT. No request struct has such a field today (searched: +// []byte with a json tag in internal/server and internal/models, non-test — +// only models.YjsUpdate.UpdateData, which no handler decodes from a body); +// this shape is chosen so that adding one later cannot silently start +// rejecting valid requests. +func valueDecodesNUL(v any, inUserData bool) bool { + 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) { + return true + } + if !inUserData && isJSONEncodedFieldKey(k) { + if str, isString := sub.(string); isString { + // BOTH checks. The nested walk answers "is there an + // escape inside the document this string carries"; it + // does NOT answer "does this string itself contain a + // NUL", and taking the JSON-encoded branch used to skip + // the plain check entirely — so a direct NUL in a + // `fields` value was accepted, reopening the door this + // whole change exists to close (codex round 9, a + // regression introduced by the round-8 restructure). + if strings.ContainsRune(str, 0) || nestedDocumentDecodesNUL(str) { + return true + } + continue + } + // The field's NATURAL shape: an array or object whose + // elements the server marshals itself. Nothing re-parses + // them, so everything below is caller data. + if valueDecodesNUL(sub, true) { + return true + } + continue + } + if valueDecodesNUL(sub, inUserData) { + return true + } + } + case []any: + for _, sub := range t { + if valueDecodesNUL(sub, inUserData) { + return true + } + } + } + return false +} + +// nestedDocumentDecodesNUL walks a JSON document carried as a string — an +// item's `fields` blob, a collection's `schema` — for a string containing a +// NUL. This is the layer Postgres itself parses, so an escape here is fatal +// where one a level deeper is not. +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. +// +// 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) +// +// 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. +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 +// it is decoded, with the caller's size cap applied. +// +// Buffering is not a cost paid for the scan. json.Decoder.Decode already +// holds the entire top-level value in memory before it finishes — refill +// accumulates into dec.buf and grows it by DOUBLING (encoding/json/stream.go, +// `newBuf := make([]byte, len(dec.buf), 2*cap(dec.buf)+minRead)` plus a copy) +// — so streaming never avoided the copy, it just reallocated its way there. +// Measured on the 64 MiB workspace-import shape, total allocation: stream and +// decode 354.7 MiB, read-all and Unmarshal 256.5 MiB, read-all and Decode +// 512.5 MiB. Peak heap is indistinguishable between the first two and is +// order-dependent, so it does not discriminate. BUG-2803. +func readBodyForDecode(r *http.Request, maxBytes int64) ([]byte, error) { + if r.Body == nil { + return nil, io.EOF + } + // The nil ResponseWriter is deliberate and its consequence is worth + // stating, because the comment this replaces got it wrong twice over: + // MaxBytesReader.Close forwards to the underlying body rather than being + // a no-op, and with a nil writer there is no automatic 413 — hitting the + // cap surfaces as a read error, which decodeJSONWithLimit wraps and every + // caller turns into a 400. That is the existing behaviour, unchanged + // here; only the claim about it is corrected (codex round 8). + r.Body = http.MaxBytesReader(nil, r.Body, maxBytes) + return io.ReadAll(r.Body) +} + +// truncateBindableText cuts s to at most maxBytes bytes WITHOUT splitting a +// rune, so the result is still valid UTF-8. +// +// A plain s[:n] slices bytes. If byte n lands inside a multi-byte rune the +// result ends in a partial sequence, which is not valid UTF-8 — so a value +// that PASSED the body check a few frames earlier becomes unbindable on its +// way to the store, and Postgres answers 22021 for a request the server +// already accepted. Found by the codex round 5 enumeration on BUG-2803, which +// asked what could still reach a text parameter unvalidated and named +// truncation rather than any input path. +// +// The failure is invisible in testing with ASCII, which is why it survived +// four review rounds aimed at the input side: every fixture in this area uses +// ASCII names, and ASCII cannot reproduce it. +func truncateBindableText(s string, maxBytes int) string { + if len(s) <= maxBytes { + return s + } + cut := maxBytes + // Walk back off any continuation bytes (10xxxxxx) to the start of the + // rune that straddles the boundary, then drop that rune entirely. + for cut > 0 && s[cut]&0xC0 == 0x80 { + cut-- + } + return s[:cut] +} + +// requestUserAgent returns the User-Agent header as text the database can +// store, replacing any invalid UTF-8 and dropping any NUL. +// +// A header is not a path, a query or a body, so none of the rules above see +// it — and unlike those, it is not the caller ASKING for anything: it is +// metadata this server chose to record. Refusing the whole request because a +// header is malformed would answer 400 to a request whose actual subject is +// fine, so this sanitizes rather than rejects, which is the opposite +// disposition from the rest of this file and deliberately so. +// +// The sink is one text column: activities.user_agent, reached from three +// document paths and the connected-apps revoke. +// +// THE LOGIN PATHS ARE NOT SINKS, and I wired them before reading +// store.CreateSession, which is the mistake this note exists to stop +// recurring. It HASHES the header (sessions.ua_hash) and never stores the +// text — the round-5 enumeration listed "sessions.user_agent" and I took the +// name for a column. Sanitising there would have been actively harmful: +// login would store sha256(sanitised) while middleware_auth still compares +// sha256(RAW), so every session belonging to a client with a non-UTF-8 +// User-Agent would fail validation. Hashing arbitrary bytes is well defined +// and needs no help; the hash sites are deliberately untouched. +// +// Found by the codex round 5 enumeration on BUG-2803. The filing's own +// earlier probe had recorded User-Agent as NOT reproducing on the item-create +// path, which was true and did not generalise: these are different sinks. +func requestUserAgent(r *http.Request) string { + return sanitiseStoredText(r.Header.Get("User-Agent")) +} + +// sanitiseStoredText makes a caller-influenced string safe to bind to a text +// column, by removing what a text column cannot hold rather than by refusing +// the request. +// +// SANITISE VERSUS REFUSE is the whole decision here, and it turns on WHO ASKED +// FOR THE VALUE TO EXIST. The body rule refuses, because there the bad value +// is the thing the caller asked to store. This helper serves metadata the +// SERVER chose to record about a request — a User-Agent header, an MCP audit +// row — where the caller never asked for a write at all. Turning an otherwise +// fine request into a 400, or losing the audit row entirely, because of a +// field the server elected to keep would be the wrong trade: the request +// carrying a malformed value is precisely the one most worth recording. +// +// Extracted so the two callers share one rule rather than two copies that +// drift. Do NOT reach for this on a value the caller asked to persist — that +// is the body rule's job, and silently cleaning such a value would store +// something the caller did not send. +func sanitiseStoredText(s string) string { + return strings.ReplaceAll(strings.ToValidUTF8(s, ""), "\x00", "") +} diff --git a/internal/server/server.go b/internal/server/server.go index 800113c3..917f7874 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,59 @@ 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, so the refusal reaches the client as a client +// 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 +// ("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 +// 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).