fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803)

Codex round 6 plus one measurement of my own. Three changes, one of them a revert of something I got wrong in the previous commit. 1. THE GATE COST TOO MUCH, so it is narrower and still sound. The previous commit gated the walk on "does the raw body contain a backslash", which is correct but catches every body carrying nested JSON (each `\"` is a backslash). Measured on a ~377 KB import-shaped body: 60106 allocs/op with that gate versus 30073 with the walk disabled — the walk was running on ordinary traffic. The gate is now the four bytes that begin any \u escape for a character below U+0100. The argument: to manufacture the six-character NUL escape inside a decoded string, each of its characters arrives either literally from the raw bytes — in which case the raw contains the escape, which begins with that prefix — or from a \u escape of its own, and the three characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit below U+0100, so those escapes begin with it too. Back to 30073 allocs/op, identical to the walk-disabled build. That argument is the same KIND of reasoning that was wrong two rounds ago, so it does not stand on its own: a differential test runs the gated function against an UNGATED walk over a corpus built to attack it — oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a doubled backslash — and fails on any disagreement. It also asserts the corpus contains both answers, since agreement over a one-sided corpus would be vacuous. Reverting the gate to the old substring fails it. 2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and `fields` accept both a JSON-encoded STRING and their natural array/object form, and the walk propagated "this subtree is JSON-encoded" into containers — so a free-form tag whose whole value happened to be a JSON document was refused, though nothing re-parses it. Measured: refused before, accepted now, while the JSON-encoded spelling of the same field is still refused. The flag now marks only a direct STRING child of a listed key. 3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser last commit, before reading store.CreateSession. It HASHES the header and stores no text — the round-5 enumeration named "sessions.user_agent" and I took the name for a column. The change would have been actively harmful: login would store sha256(sanitised) while middleware_auth still compares sha256(RAW), so every session from a client with a non-UTF-8 User-Agent would fail validation. A sink named in a review is a pointer to verify, not a finding. The real sink is activities.user_agent, from three document paths and the connected-apps revoke. 4. And the wiring leg codex asked for, on that real sink: a request through the router with a malformed header, reading the STORED value out of the activities row, with a control asserting an ordinary header is kept VERBATIM. Unwiring the production call site fails it; the helper's unit test does not notice. Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
2026-10-26 14:30:13 +00:00 · 2026-08-28 17:26:05 +00:00
parent e94e57f661
commit 73ae90236a
3 changed files with 198 additions and 11 deletions
@@ -2,6 +2,7 @@ package server

 import (
 	"bytes"
+	"database/sql"
 	"encoding/json"
 	"errors"
 	"io"
@@ -348,6 +349,18 @@ func TestBodyDecodesNULNestedDocuments(t *testing.T) {
 			`{"title":"x","fields":` + jsonEncode(t, innerPlain) + `}`, false},
 		{"nested document whose escape is a DOUBLED backslash",
 			`{"fields":` + jsonEncode(t, innerLiteral) + `}`, false},
+		// codex round 6: these fields also accept their NATURAL shape, in
+		// which the elements are ordinary strings the server marshals itself.
+		// Nothing re-parses them, so an element that merely LOOKS like a
+		// document must not be treated as one — this refused a free-form tag
+		// whose whole value happened to be JSON.
+		{"tags as a natural ARRAY whose element is a JSON document",
+			`{"title":"x","tags":["release",` + jsonEncode(t, innerWithNUL) + `]}`, false},
+		{"fields as a natural OBJECT whose value is a JSON document",
+			`{"title":"x","fields":{"k":` + jsonEncode(t, innerWithNUL) + `}}`, false},
+		// ...while the JSON-ENCODED spelling of the same field still is.
+		{"tags as a JSON-encoded STRING carrying the escape",
+			`{"title":"x","tags":` + jsonEncode(t, `["a`+esc+`b"]`) + `}`, true},
 		{"a string that starts like JSON but does not parse",
 			`{"content":` + jsonEncode(t, `{"k":"a`+esc+`b"`) + `}`, false},
 		{"prose mentioning the escape is not a JSON document",
@@ -574,3 +587,153 @@ func TestRequestUserAgentIsBindableText(t *testing.T) {
 		t.Fatal("the fixture cannot reproduce the defect: the raw header is already bindable text")
 	}
 }
+
+// TestBodyDecodesNULGateAgreesWithAnUngatedWalk is the instrument that keeps
+// the fast path honest.
+//
+// The gate is an argument, not an observation: to manufacture the
+// six-character NUL escape inside a decoded string, every one of its
+// characters must arrive either literally from the raw bytes — in which case
+// the raw contains the escape, which begins with \u00 — or from a \u escape
+// of its own, and the three characters involved (backslash U+005C, 'u'
+// U+0075, '0' U+0030) all sit below U+0100, so every such escape also begins
+// with \u00. Hence: no \u00 in the raw bytes, no NUL at any depth, however
+// spelled.
+//
+// The FIRST version of that argument was wrong in exactly this way — it said
+// "the escape has only one spelling", which is true inside a decoded string
+// and false of the raw bytes, and codex round 4 turned that into a live
+// bypass. So the argument does not get to stand on its own reasoning: this
+// test runs the gated function against an UNGATED walk over a corpus built to
+// attack it, and any disagreement is a bypass.
+func TestBodyDecodesNULGateAgreesWithAnUngatedWalk(t *testing.T) {
+	ungated := func(raw []byte) bool {
+		var v any
+		if err := json.Unmarshal(raw, &v); err != nil {
+			return false
+		}
+		return valueDecodesNUL(v, false, 0)
+	}
+
+	esc := escNULLiteral                                     // the six-character NUL escape
+	bs := string([]byte{'\\', 'u', '0', '0', '5', 'c'})      // escapes to a BACKSLASH
+	bsUpper := string([]byte{'\\', 'u', '0', '0', '5', 'C'}) // same, upper-case hex
+	uEsc := string([]byte{'\\', 'u', '0', '0', '7', '5'})    // escapes to the letter u
+	zero := string([]byte{'\\', 'u', '0', '0', '3', '0'})    // escapes to the digit 0
+	doubled := `\\` + "u0000"                                // literal text, no NUL
+
+	corpus := []string{
+		`{"title":"plain"}`,
+		`{"title":"quotes \" and a backslash \\ but no escape"}`,
+		`{"fields":"{\"k\":\"plain\"}"}`,
+		`{"fields":"{\"k\":\"a` + esc + `b\"}"}`,
+		`{"fields":"{\"k\":\"a` + bs + `u0000b\"}"}`,
+		`{"fields":"{\"k\":\"a` + bsUpper + `u0000b\"}"}`,
+		`{"fields":"{\"k\":\"a\\` + uEsc + `0000b\"}"}`,
+		`{"fields":"{\"k\":\"a\\u00` + zero + `0b\"}"}`,
+		`{"fields":"{\"k\":\"a` + doubled + `b\"}"}`,
+		`{"content":"{\"k\":\"a` + esc + `b\"}"}`,
+		`{"title":"a` + esc + `b"}`,
+		`{"a` + esc + `b":"key"}`,
+		`{"tags":["ok","a` + esc + `b"]}`,
+		`{"fields":"[1,2,\"a` + esc + `b\"]"}`,
+		`{"title":"unicode é 中"}`,
+		`{"title":"a control escape that is not a NUL: \\u0001"}`,
+		`{"fields":"{\"k\":\"ab\"}"}`,
+	}
+
+	for i, body := range corpus {
+		got, want := bodyDecodesNUL([]byte(body)), ungated([]byte(body))
+		if got != want {
+			t.Errorf("corpus[%d] %s\n  gated=%v ungated=%v — the fast path is a BYPASS for this input",
+				i, body, got, want)
+		}
+	}
+
+	// The corpus must contain both answers, or agreement proves nothing.
+	var trues, falses int
+	for _, body := range corpus {
+		if ungated([]byte(body)) {
+			trues++
+		} else {
+			falses++
+		}
+	}
+	if trues < 3 || falses < 3 {
+		t.Fatalf("corpus is one-sided (%d refuse, %d accept); agreement would be vacuous", trues, falses)
+	}
+}
+
+// TestDocumentActivityStoresBindableUserAgent is the WIRING leg for the
+// User-Agent sanitiser (CONVE-19, and codex round 6's second finding: a unit
+// test vouches for the helper, not for anything calling it).
+//
+// It drives a real request through the router with a malformed header and
+// reads the STORED value out of the activities row, so reverting the
+// production call site fails this even though the helper still works.
+//
+// It targets the ACTIVITY sink deliberately. The round-5 enumeration also
+// named "sessions.user_agent", and I wired the three login paths to the
+// sanitiser before reading store.CreateSession — which HASHES the header and
+// stores no text at all. That change was reverted: login would have stored
+// sha256(sanitised) while middleware_auth still compares sha256(RAW),
+// breaking session validation for any client with a non-UTF-8 User-Agent.
+// A sink named in a review is a pointer to verify, not a finding.
+func TestDocumentActivityStoresBindableUserAgent(t *testing.T) {
+	srv := testServer(t)
+	ws := createWSForTest(t, srv)
+
+	post := func(t *testing.T, ua, title string) {
+		t.Helper()
+		body, _ := json.Marshal(map[string]any{"title": title})
+		req := httptest.NewRequest("POST", "/api/v1/workspaces/"+ws+"/documents/", bytes.NewReader(body))
+		req.Header.Set("Content-Type", "application/json")
+		req.Header.Set("User-Agent", ua)
+		req.RemoteAddr = "192.0.2.1:1234"
+		rr := httptest.NewRecorder()
+		srv.ServeHTTP(rr, req)
+		if rr.Code != http.StatusOK && rr.Code != http.StatusCreated {
+			t.Fatalf("create document = %d: %s", rr.Code, rr.Body.String())
+		}
+	}
+
+	// Control first: an ordinary header must be stored VERBATIM, so a
+	// sanitiser that mangled everything would fail here rather than pass.
+	post(t, "pad-cli/1.0 café", "control doc")
+	post(t, "pad-cli/1.0 bad\xffbyte\x00here", "probe doc")
+
+	rows, err := srv.store.DB().Query(`SELECT user_agent FROM activities WHERE user_agent IS NOT NULL`)
+	if err != nil {
+		t.Fatalf("read activities: %v", err)
+	}
+	defer rows.Close()
+
+	var seenControl, seenSanitised int
+	for rows.Next() {
+		var ua sql.NullString
+		if err := rows.Scan(&ua); err != nil {
+			t.Fatalf("scan: %v", err)
+		}
+		if !ua.Valid {
+			continue
+		}
+		if !bindableText(ua.String) {
+			t.Errorf("a stored activity user_agent is not bindable text: %q", ua.String)
+		}
+		switch ua.String {
+		case "pad-cli/1.0 café":
+			seenControl++
+		case "pad-cli/1.0 badbytehere":
+			seenSanitised++
+		}
+	}
+	if err := rows.Err(); err != nil {
+		t.Fatalf("rows: %v", err)
+	}
+	if seenControl == 0 {
+		t.Error("the control header was not stored verbatim — the sanitiser is doing too much")
+	}
+	if seenSanitised == 0 {
+		t.Error("no activity carries the sanitised header; the call site is not wired to requestUserAgent")
+	}
+}
@@ -308,7 +308,7 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h
 			"user_id", user.ID, "error", err)
 	}

-	token, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), webSessionTTL)
+	token, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL)
 	if err != nil {
 		writeError(w, http.StatusInternalServerError, "internal_error",
 			"Credentials updated but failed to refresh session. Please sign in again.")
@@ -329,7 +329,7 @@ func (s *Server) rotateSessionsAfterCredentialChange(w http.ResponseWriter, r *h
 }

 func (s *Server) createAuthSession(w http.ResponseWriter, r *http.Request, user *models.User, ttl time.Duration) (string, error) {
-	token, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), ttl)
+	token, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), ttl)
 	if err != nil {
 		writeError(w, http.StatusInternalServerError, "internal_error", "Failed to create session")
 		return "", err
@@ -1313,7 +1313,7 @@ func (s *Server) handleResetPassword(w http.ResponseWriter, r *http.Request) {
 	}

 	// Create a fresh session so the user is logged in
-	sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), requestUserAgent(r), webSessionTTL)
+	sessionToken, err := s.store.CreateSession(user.ID, "web", clientIP(r), r.UserAgent(), webSessionTTL)
 	if err != nil {
 		writeError(w, http.StatusInternalServerError, "internal_error", "Password updated but failed to create session")
 		return
@@ -347,6 +347,11 @@ func validQueryText(rawQuery string) bool {
 	return true
 }

+// unicodeEscapePrefix is the four bytes that begin any JSON \u escape for a
+// character below U+0100. Built from bytes rather than written as a literal,
+// for the reason the tests do the same.
+var unicodeEscapePrefix = []byte{'\\', 'u', '0', '0'}
+
 // bodyDecodesNUL reports whether any string a handler could read out of this
 // JSON body — an object key or a value, at any nesting depth, including
 // inside a JSON document carried as a string — decodes to a string containing
@@ -420,7 +425,7 @@ func validQueryText(rawQuery string) bool {
 // runs next and reports the JSON error itself, so there is exactly one place
 // that phrases "invalid JSON" and this function never has to agree with it.
 func bodyDecodesNUL(raw []byte) bool {
-	if !bytes.ContainsRune(raw, '\\') {
+	if !bytes.Contains(raw, unicodeEscapePrefix) {
 		return false
 	}
 	var v any
@@ -507,7 +512,7 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool {
 		if strings.ContainsRune(t, 0) {
 			return true
 		}
-		if !inJSONEncodedField || !strings.ContainsRune(t, '\\') {
+		if !inJSONEncodedField || !strings.Contains(t, string(unicodeEscapePrefix)) {
 			return false
 		}
 		if depth >= maxJSONDocumentNesting {
@@ -529,7 +534,19 @@ func valueDecodesNUL(v any, inJSONEncodedField bool, depth int) bool {
 			if strings.ContainsRune(k, 0) {
 				return true
 			}
-			if valueDecodesNUL(sub, inJSONEncodedField || jsonEncodedFieldKeys[k], depth) {
+			// A listed key marks its value as a JSON document only when that
+			// value is a STRING. The same fields also accept their NATURAL
+			// shape — `"tags":["a","b"]`, `"fields":{"k":"v"}` — and in that
+			// shape the elements are ordinary strings the server marshals
+			// itself, so nothing re-parses them and an element that merely
+			// LOOKS like a document must not be treated as one. Propagating
+			// the flag into containers refused a free-form tag whose whole
+			// value happened to be a JSON document (codex round 6).
+			childEncoded := inJSONEncodedField
+			if _, isString := sub.(string); isString && jsonEncodedFieldKeys[k] {
+				childEncoded = true
+			}
+			if valueDecodesNUL(sub, childEncoded, depth) {
 				return true
 			}
 		}
@@ -631,11 +648,18 @@ func truncateBindableText(s string, maxBytes int) string {
 // fine, so this sanitizes rather than rejects, which is the opposite
 // disposition from the rest of this file and deliberately so.
 //
-// The sinks are text columns: activities.user_agent (documents and the
-// connected-apps revoke path) and sessions.user_agent (three login paths).
-// The two sites that HASH the header instead are left alone — sha256 over
-// arbitrary bytes is well defined, and changing what is hashed would
-// invalidate every stored UAHash.
+// The sink is one text column: activities.user_agent, reached from three
+// document paths and the connected-apps revoke.
+//
+// THE LOGIN PATHS ARE NOT SINKS, and I wired them before reading
+// store.CreateSession, which is the mistake this note exists to stop
+// recurring. It HASHES the header (sessions.ua_hash) and never stores the
+// text — the round-5 enumeration listed "sessions.user_agent" and I took the
+// name for a column. Sanitising there would have been actively harmful:
+// login would store sha256(sanitised) while middleware_auth still compares
+// sha256(RAW), so every session belonging to a client with a non-UTF-8
+// User-Agent would fail validation. Hashing arbitrary bytes is well defined
+// and needs no help; the hash sites are deliberately untouched.
 //
 // Found by the codex round 5 enumeration on BUG-2803. The filing's own
 // earlier probe had recorded User-Agent as NOT reproducing on the item-create