fix(documents): bound the rename cascade at the title and at the projected total (BUG-2798, BUG-2796)

internal/models/document.go

@@ -1,6 +1,11 @@
 package models
 
-import "time"
+import (
+	"fmt"
+	"strings"
+	"time"
+	"unicode/utf8"
+)
 
 // Valid document types
 var ValidDocTypes = []string{
@@ -72,6 +77,113 @@ type DocumentListParams struct {
 	Order  string
 }
 
+// MaxDocumentTitleRunes bounds a document title at write time.
+//
+// Two reasons, and the second is the load-bearing one:
+//
+//   - A title is emitted into every linking document by the rename cascade
+//     (`[[newTitle]]` per occurrence), so its length is an amplification
+//     factor on a body the renamer does not supply. Unbounded, one rename
+//     could project 10 GB from a 500 KB input — measured, 20,000x (BUG-2798).
+//   - 255 is the conventional identifier-ish bound and comfortably above any
+//     real title; the longest document title this codebase seeds is far short
+//     of it.
+//
+// RUNES, not bytes, because "255 characters" is what a user and a UI counter
+// mean. That makes the byte-level residual up to 4x this number, which is
+// precisely why the title bound is the cheap door and NOT the wall: the
+// cascade's own guard (store.MaxRenameCascadeRetainedBytes) is byte-accurate
+// and is what actually bounds the work.
+//
+// Enforced at WRITE time only. Titles already over the bound stay valid and
+// keep working until their next rename — no retro-breakage of stored data
+// (Dave's ruling, day-63).
+const MaxDocumentTitleRunes = 255
+
+// wikiTitleRoundTripFailure reports why emitting `[[title]]` the way
+// links.ReplaceTitle does — plain concatenation, no escaping — would produce a
+// bracket that does not read back as this title. Returns "" when the title
+// survives the round trip.
+//
+// This is BUG-2796 stated as a property rather than as a character blacklist,
+// and the distinction is not cosmetic: the first version of this function
+// banned `]`, `\` and `|` on the reasoning that all three "look like
+// wiki-link syntax", and the test below refuted two thirds of that. The rule
+// is therefore derived from the two mechanisms that actually consume a stored
+// bracket, both in web/src/lib/utils/markdown.ts:
+//
+//  1. THE GRAMMAR (markdown.ts:327, shared by renderMarkdown and
+//     wikiLinksToMarkdown since BUG-1744): a bracket body is
+//     `(?:\\.|[^\]\\])+` — escape pairs, or anything that is neither `]` nor
+//     `\`. A raw `]` ends the bracket early, which IS BUG-2796's defect:
+//     renaming to `A]] [[A` emitted `[[A]] [[A]]`, two brackets, neither
+//     resolving to the renamed document, and the rename reported success. A
+//     title ending in `\` fails the same way — the trailing backslash pairs
+//     with the first `]` of the terminator and the bracket never closes.
+//
+//  2. THE UNESCAPER (markdown.ts:753, `\\(\\|\]|\|)` → `$1`): resolution
+//     unescapes the body before comparing it to a title, and the editor may
+//     therefore STORE a link in escaped form. That matters twice over: a
+//     title containing `\\` or `\|` emitted raw comes back as a different
+//     string, AND a link stored as `[[Alpha\|Beta]]` is invisible to the
+//     rename cascade, which searches for the raw `[[Alpha|Beta]]` only.
+//
+// The second half is why `|` and a lone `\` are refused (codex round 9),
+// having been ALLOWED in the first version of this validator. The property
+// tested there was "does the renderer read this back as the same title", and
+// both characters pass it. That was the wrong property: a title also has to be
+// one whose links the cascade can FIND, or a rename silently leaves them
+// pointing at a name that no longer exists. The stricter property is the one
+// that matters, and it is the same mistake as validating against the renderer
+// while the cascade used an unescaped LIKE — checking the layer that displays
+// a title instead of the layer that has to maintain it.
+//
+// Deliberately NOT rejected, because the code these titles pass through
+// handles them and refusing them would be a validator inventing a defect:
+//
+//   - `[` — the grammar excludes only `]` and `\`, so `[[A[B]]` carries the
+//     body `A[B` intact, and the cascade's search term matches it literally.
+//     BUG-2796's filing proposed rejecting "`[[` or `]]`"; measured against
+//     the grammar, the `[[` half of that is overreach.
+//
+// Boundary, stated rather than papered over: this is derived from the SHARED
+// stored-syntax path in markdown.ts. The legacy documents surface has no
+// renderer of its own that I could locate — every wiki-link consumer found
+// routes through these two functions — so the rule is pinned to them.
+func wikiTitleRoundTripFailure(title string) string {
+	if strings.Contains(title, "]") {
+		return `Title may not contain "]" — it would end the [[wiki-links]] that point at this document early, ` +
+			`turning them into broken links`
+	}
+	if strings.HasSuffix(title, `\`) {
+		return `Title may not end with "\" — it would escape the closing bracket of the [[wiki-links]] that point ` +
+			`at this document`
+	}
+	if strings.ContainsAny(title, `\|`) {
+		return `Title may not contain "\" or "|" — a link to a title containing them can be stored in ` +
+			`escaped form, which the rename cascade would not find, silently leaving those links pointing ` +
+			`at a title that no longer exists`
+	}
+	return ""
+}
+
+// ValidateDocumentTitle checks a document title at write time. Returns a
+// message suitable for a 400 response, or "" when the title is acceptable.
+//
+// Covers BUG-2798 (length, which is an amplification factor on OTHER
+// documents' bodies, not merely a field-size preference) and BUG-2796
+// (wiki-link syntax the cascade emits raw). Both are write-time doors on the
+// same field, which is why they share one validator and one insertion point.
+func ValidateDocumentTitle(title string) string {
+	if title == "" {
+		return "Title is required"
+	}
+	if n := utf8.RuneCountInString(title); n > MaxDocumentTitleRunes {
+		return fmt.Sprintf("Title is too long: %d characters, maximum %d", n, MaxDocumentTitleRunes)
+	}
+	return wikiTitleRoundTripFailure(title)
+}
+
 func IsValidDocType(t string) bool {
 	for _, v := range ValidDocTypes {
 		if v == t {

internal/models/document_title_test.go

@@ -0,0 +1,183 @@
+package models
+
+import (
+	"regexp"
+	"strings"
+	"testing"
+)
+
+// The two mechanisms a stored wiki-link bracket passes through, mirrored from
+// web/src/lib/utils/markdown.ts so this test can derive the validator's rule
+// from them instead of from an opinion about which characters look dangerous.
+//
+// Duplicated rather than approximated — and duplication is exactly the risk
+// here, stated rather than papered over: these are STATIC copies, so a change
+// to the TypeScript does not reach them. Nothing in this repository fails when
+// the two drift (codex round 6). What this buys is that the Go rule is derived
+// from a written-down grammar rather than from an opinion, and that a reader
+// can check the two by eye at the cited line numbers. Closing the drift for
+// real would need a shared fixture driven from both languages.
+var (
+	// markdown.ts:327 — shared by renderMarkdown and wikiLinksToMarkdown.
+	storedWikiLinkBracket = regexp.MustCompile(`\[\[((?:\\.|[^\]\\])+)\]\]`)
+	// markdown.ts:753 — unescapeWikiBody, applied before title comparison.
+	wikiBodyEscape = regexp.MustCompile(`\\(\\|\]|\|)`)
+)
+
+// roundTrips reports whether emitting `[[title]]` the way links.ReplaceTitle
+// does — plain concatenation, no escaping — produces a bracket that reads back
+// as exactly this title.
+//
+// Both layers, because either alone gives a wrong answer. The grammar alone
+// accepts `A\\B` (a valid escape pair) which the unescaper then turns into
+// `A\B`, a different title. The unescaper alone accepts `A]B`, which the
+// grammar cuts short.
+func roundTrips(title string) bool {
+	emitted := "[[" + title + "]]"
+	m := storedWikiLinkBracket.FindStringSubmatch(emitted)
+	if m == nil || m[0] != emitted {
+		// No match, or a match over a PREFIX of the emission — the latter is
+		// BUG-2796's early-termination defect, where `[[A]] [[A]]` matched
+		// twice and neither match was the renamed document.
+		return false
+	}
+	return wikiBodyEscape.ReplaceAllString(m[1], "$1") == title
+}
+
+// TestDocumentTitleValidationMatchesTheRoundTripProperty is the justification
+// for the SYNTAX rule, in executable form: of the titles this table covers —
+// all of them within the length bound — the validator must reject one if and
+// only if its links would not survive being rewritten to it.
+//
+// Scoped to syntax deliberately. ValidateDocumentTitle also enforces length
+// and non-emptiness, and those are NOT round-trip properties: a 300-rune title
+// round-trips perfectly and is still refused, for the unrelated reason that it
+// is an amplification factor on other documents. Stating the biconditional
+// over the whole validator would be false (codex round 6); the length rule has
+// its own tests below.
+//
+// Both directions are asserted, because either alone permits a wrong answer.
+// Without the reject leg, a validator that accepted everything passes.
+// Without the accept leg, a validator that rejected `[`, `|` or a lone `\`
+// passes — and that is not hypothetical: the first version of this fix banned
+// `]`, `\` and `|` as a character class, and this test refuted two thirds of
+// it. `|` in particular is a title shape resolveWikiBody contains a dedicated
+// branch to support.
+func TestDocumentTitleValidationMatchesTheRoundTripProperty(t *testing.T) {
+	for _, tc := range []struct {
+		name  string
+		title string
+	}{
+		// Must be REJECTED — each breaks the round trip by a different
+		// mechanism.
+		{"bug-2796 repro", `A]] [[A`},
+		{"single close bracket", `Alpha]Beta`},
+		{"trailing backslash", `Alpha\`},
+		{"escaped backslash", `Alpha\\Beta`},
+		{"escaped pipe", `Alpha\|Beta`},
+
+		// Must be ACCEPTED — each looks like wiki-link syntax and none of
+		// them breaks anything.
+		{"open bracket", `Alpha[Beta`},
+		{"double open bracket", `Alpha[[Beta`},
+		{"collection-qualified shape", `collection/Title`},
+		{"colons", `Ratio 1:2`},
+		{"ordinary punctuation", `Title (draft) — v2`},
+		{"non-latin", `Ünïcödé título`},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			wantAccept := roundTrips(tc.title)
+			msg := ValidateDocumentTitle(tc.title)
+			gotAccept := msg == ""
+			if gotAccept != wantAccept {
+				t.Errorf("title %q: round-trips=%v but validator accepts=%v (message: %q)\n"+
+					"the validator and the renderers must agree — a rejected title that renders "+
+					"correctly is a refusal of valid input; an accepted title that does not is BUG-2796",
+					tc.title, wantAccept, gotAccept, msg)
+			}
+		})
+	}
+}
+
+// TestDocumentTitleBoundIsTheRuledNumber pins the VALUE, not just the
+// behaviour around it.
+//
+// Every other length test derives its inputs from MaxDocumentTitleRunes, so
+// changing the constant to 512 would leave them all green (codex round 4).
+// That is fine for arithmetic but wrong for this number: 255 is a product
+// decision Dave ruled on for BUG-2798, and a silent change to it is a silent
+// change to how much amplification the cheap door lets through. Changing it
+// should require editing this line and noticing why.
+//
+// Deliberately NOT done for store.MaxRenameCascadeRetainedBytes: that one is
+// mine, chosen with a measured receipt in its doc comment, and is expected to
+// be tuned if the measurements change.
+func TestDocumentTitleBoundIsTheRuledNumber(t *testing.T) {
+	if MaxDocumentTitleRunes != 255 {
+		t.Errorf("MaxDocumentTitleRunes = %d, want 255 (Dave's day-63 ruling on BUG-2798). "+
+			"If this is a deliberate product change, update the ruling reference too.",
+			MaxDocumentTitleRunes)
+	}
+}
+
+// TestDocumentTitleLengthBoundCountsRunesNotBytes pins the bound at 255
+// CHARACTERS, which is what the ruling says and what a UI counter shows.
+//
+// The multibyte legs are the counterfactual: 255 four-byte runes are 1020
+// bytes, so an implementation that reached for len() would reject them. That
+// implementation would be wrong in a user-visible way — an ordinary
+// 255-character title in a non-Latin script refused — while every ASCII-only
+// test stayed green.
+func TestDocumentTitleLengthBoundCountsRunesNotBytes(t *testing.T) {
+	for _, tc := range []struct {
+		name   string
+		title  string
+		accept bool
+	}{
+		{"at the bound, ascii", strings.Repeat("a", MaxDocumentTitleRunes), true},
+		{"one over, ascii", strings.Repeat("a", MaxDocumentTitleRunes+1), false},
+		{"at the bound, 2-byte runes", strings.Repeat("é", MaxDocumentTitleRunes), true},
+		{"at the bound, 4-byte runes", strings.Repeat("𝄞", MaxDocumentTitleRunes), true},
+		{"one over, 4-byte runes", strings.Repeat("𝄞", MaxDocumentTitleRunes+1), false},
+		{"empty", "", false},
+	} {
+		msg := ValidateDocumentTitle(tc.title)
+		if tc.accept && msg != "" {
+			t.Errorf("%s: rejected (%s), want accepted", tc.name, msg)
+		}
+		if !tc.accept && msg == "" {
+			t.Errorf("%s: accepted, want rejected", tc.name)
+		}
+	}
+}
+
+// TestDocumentTitleRejectsFormsTheCascadeCannotFind covers the two characters
+// whose refusal the round-trip property above does NOT explain — and that gap
+// is the finding, not an oversight in the table.
+//
+// `Alpha|Beta` and `Alpha\Beta` both round-trip perfectly: the renderer reads
+// each back as exactly the title it started from. The first version of this
+// validator accepted them for precisely that reason, and it was wrong (codex
+// round 9). A link to such a title can be STORED in escaped form —
+// `[[Alpha\|Beta]]` — which the rename cascade, searching for the raw
+// `[[Alpha|Beta]]`, will not find. The rename then succeeds and leaves those
+// links pointing at a title that no longer exists.
+//
+// So the property a title must satisfy is stricter than "the renderer reads it
+// back": it must also be one whose links the cascade can FIND. Validating
+// against the display layer while the maintenance layer disagrees is the same
+// mistake as the unescaped LIKE, met from the other side.
+func TestDocumentTitleRejectsFormsTheCascadeCannotFind(t *testing.T) {
+	for _, title := range []string{`Alpha|Beta`, `Alpha\Beta`} {
+		// The premise: each of these DOES round-trip. If that ever stops being
+		// true they are rejected for the ordinary syntax reason and this test
+		// is no longer testing what it says.
+		if !roundTrips(title) {
+			t.Fatalf("premise: %q must round-trip, or its rejection needs no separate justification", title)
+		}
+		if ValidateDocumentTitle(title) == "" {
+			t.Errorf("ValidateDocumentTitle(%q) accepted a title whose links the cascade cannot find; "+
+				"a rename would leave them stale", title)
+		}
+	}
+}

internal/server/handlers_documents.go

@@ -66,8 +66,8 @@ func (s *Server) handleCreateDocument(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if input.Title == "" {
-		writeError(w, http.StatusBadRequest, "bad_request", "Title is required")
+	if msg := models.ValidateDocumentTitle(input.Title); msg != "" {
+		writeError(w, http.StatusBadRequest, "bad_request", msg)
 		return
 	}
 	if input.DocType != "" && !models.IsValidDocType(input.DocType) {
@@ -137,6 +137,27 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// A title write here is the RENAME door — the one that cascades into every
+	// linking document — so it carries the same validation as create. Update
+	// had none at all before BUG-2798/BUG-2796: doc_type and status were
+	// checked and the field that drives the cascade was not.
+	//
+	// Only when the title actually CHANGES, though. Grandfathering existing
+	// over-limit titles is not a thing you get by validating at write time —
+	// it is a thing you get by not validating a write that is not a rename
+	// (codex round 10). A client PATCHing content with the full object, title
+	// included, is the ordinary shape of an edit; validating the echoed-back
+	// value would make every document with a legacy title uneditable rather
+	// than merely un-renameable, which is the opposite of the promise this
+	// fix's own comments make. The store applies the same test before
+	// cascading (documents.go, `*input.Title != existing.Title`), so this
+	// matches where the work actually happens.
+	if input.Title != nil && *input.Title != doc.Title {
+		if msg := models.ValidateDocumentTitle(*input.Title); msg != "" {
+			writeError(w, http.StatusBadRequest, "bad_request", msg)
+			return
+		}
+	}
 	if input.DocType != nil && !models.IsValidDocType(*input.DocType) {
 		writeError(w, http.StatusBadRequest, "bad_request", "Invalid doc_type")
 		return
@@ -148,6 +169,49 @@ func (s *Server) handleUpdateDocument(w http.ResponseWriter, r *http.Request) {
 
 	updated, err := s.store.UpdateDocument(doc.ID, input)
 	if err != nil {
+		// TYPED checks first, prose matching last. The UNIQUE-constraint arm
+		// below identifies its error by SUBSTRING, and the refusal error
+		// carries the caller's own title verbatim — so a document renamed to a
+		// title containing the words "UNIQUE constraint" came back as a 409
+		// title collision, telling the caller to pick a different name for a
+		// rename that was refused for size and would fail identically under
+		// any name (codex round 3 P2). Any sentinel this handler knows by
+		// identity is tested before an error is classified by what its text
+		// happens to contain.
+		//
+		// A size refusal is also PERMANENT-shaped, and must not join the
+		// retryable family below: retrying it unchanged fails identically
+		// until the workspace's content changes, so it gets a 4xx with no
+		// Retry-After, carrying the projection because that is the only
+		// actionable information. 413 follows this codebase's own precedent
+		// for refusing work whose COST is not in the request body
+		// (`image_too_large` in handlers_attachments_transform.go, where the
+		// request is likewise small and what is refused is what handling it
+		// would take). The precedent's quantity is output size and this
+		// guard's is retained content; what carries across is the shape —
+		// a small request refused for what it would cost, not for its size.
+		// The store re-checks the title under the rename lock, which is the
+		// authoritative point; this arm surfaces that check for the case the
+		// handler's pre-lock comparison could not see — a concurrent rename
+		// turning an echoed legacy title into a real one (codex round 11).
+		var badTitle *store.InvalidDocumentTitleError
+		if errors.As(err, &badTitle) {
+			writeError(w, http.StatusBadRequest, "bad_request", badTitle.Reason)
+			return
+		}
+		var tooLarge *store.RenameCascadeTooLargeError
+		if errors.As(err, &tooLarge) {
+			// Composed from TYPED fields, never by splicing err.Error(). The
+			// two figures are meant to reach the caller — the refusal has to
+			// be actionable — but the internal call path wrapped around them
+			// is not, and appending the error text published whatever any
+			// layer had prefixed (codex round 5).
+			writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large",
+				fmt.Sprintf("This rename would rewrite more linked content than the server will process in one "+
+					"operation: at least %d bytes, and the limit is %d. Reduce the number of documents linking "+
+					"this title, or shorten the new title, and try again.", tooLarge.Retained, tooLarge.Max))
+			return
+		}
 		if strings.Contains(err.Error(), "UNIQUE constraint") {
 			writeError(w, http.StatusConflict, "conflict", "A document with this title already exists in this workspace")
 			return