diff --git a/internal/diff/bug3839_invalid_utf8_test.go b/internal/diff/bug3839_invalid_utf8_test.go new file mode 100644 index 000000000..6f8ae9c54 --- /dev/null +++ b/internal/diff/bug3839_invalid_utf8_test.go @@ -0,0 +1,189 @@ +package diff + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strings" + "testing" +) + +// BUG-3839: go-diff cannot patch text that is not valid UTF-8. Near an edit +// it panics or builds a patch that does not reproduce the old text; both +// shapes are pinned below through CreateReversePatch (unchecked), and +// ReversePatch refuses them. +var bug3839Cases = []struct { + name string + old, new string +}{ + // Panic shapes: an invalid byte, lone or a truncated sequence, next to + // an edit. + {"lone_ff", "a\xff", "a\xffb"}, + {"lone_80", "x\x80y", "\x80"}, + {"truncated_2", "a\xc3", "a\xc3b"}, + {"truncated_3", "\xe2\x82", "x\xe2\x82y"}, + // Wrong-patch shape: reverse-applies to "hello d", with no error. + {"wrong_patch", "hello world", "hello \xff world"}, +} + +func TestBUG3839_ReversePatchRefusesInvalidUTF8(t *testing.T) { + for _, tc := range bug3839Cases { + t.Run(tc.name, func(t *testing.T) { + if p, ok := ReversePatch(tc.old, tc.new); ok { + t.Fatalf("ReversePatch accepted text that is not valid UTF-8: %q", p) + } + // Instrument: unchecked, each case really does fail. + var p string + panicked := func() (r any) { + defer func() { r = recover() }() + p = CreateReversePatch(tc.old, tc.new) + return nil + }() + if panicked == nil { + if got, err := ApplyPatch(tc.new, p); err == nil && got == tc.old { + t.Fatalf("instrument: the unchecked patch round-trips, so this case shows nothing") + } + } + }) + } +} + +func TestBUG3839_ReversePatchRoundTripsValidText(t *testing.T) { + for _, tc := range [][2]string{ + {"hello world", "hello brave world"}, + {"café ☕ 😀 naïve", "café ☕ naïve 😀!"}, + {"", "new"}, + {"old", ""}, + } { + p, ok := ReversePatch(tc[0], tc[1]) + if !ok { + t.Fatalf("ReversePatch refused valid text %q -> %q", tc[0], tc[1]) + } + if got, err := ApplyPatch(tc[1], p); err != nil || got != tc[0] { + t.Fatalf("%q -> %q: reverse-applied to %q (%v)", tc[0], tc[1], got, err) + } + } +} + +// Production code calls ReversePatch, never the unchecked CreatePatch or +// CreateReversePatch, which tests still use to build fixtures from valid +// text. A census of every non-test Go file in the module. +func TestBUG3839_NoUncheckedPatchCallers(t *testing.T) { + root := filepath.Join("..", "..") + fset := token.NewFileSet() + var bad []string + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + // The web UI and the module's own tooling dirs, at the root only: + // a Go package named web elsewhere is still production code. + if filepath.Dir(path) == root { + switch d.Name() { + case "node_modules", ".git", "web", "vendor": + return filepath.SkipDir + } + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + f, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err + } + // Inside the package, ReversePatch and CreateReversePatch are the + // checked door and its unchecked twin. + if f.Name.Name == "diff" && filepath.Base(filepath.Dir(path)) == "diff" { + return nil + } + bad = append(bad, uncheckedPatchUses(fset, f)...) + return nil + }) + if err != nil { + t.Fatal(err) + } + if len(bad) > 0 { + t.Fatalf("production code uses an unchecked patch builder; use diff.ReversePatch (BUG-3839):\n%s", strings.Join(bad, "\n")) + } +} + +const diffImportPath = "github.com/PerpetualSoftware/pad/internal/diff" + +// uncheckedPatchUses lists every reference to CreatePatch or +// CreateReversePatch through this file's import of the package: a call, a +// method value, under any import name, or bare under a dot import. +func uncheckedPatchUses(fset *token.FileSet, f *ast.File) []string { + local, dot := "", false + for _, imp := range f.Imports { + if strings.Trim(imp.Path.Value, `"`) != diffImportPath { + continue + } + switch { + case imp.Name == nil: + local = "diff" + case imp.Name.Name == ".": + dot = true + case imp.Name.Name != "_": + local = imp.Name.Name + } + } + if local == "" && !dot { + return nil + } + unchecked := func(name string) bool { return name == "CreatePatch" || name == "CreateReversePatch" } + var out []string + // A selector's own name is never a dot-imported identifier. + sel := map[*ast.Ident]bool{} + ast.Inspect(f, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.SelectorExpr: + sel[x.Sel] = true + // A selector on a local variable that shadows the import name is + // a false positive here; none exists, and a hit says which file. + if id, ok := x.X.(*ast.Ident); ok && local != "" && id.Name == local && unchecked(x.Sel.Name) { + out = append(out, fset.Position(x.Pos()).String()+": "+x.Sel.Name) + } + case *ast.Ident: + if dot && !sel[x] && unchecked(x.Name) { + out = append(out, fset.Position(x.Pos()).String()+": "+x.Name) + } + } + return true + }) + return out +} + +// The census's own instrument: each way a file can reach the unchecked +// builders is found, and the checked door and a same-named local are not. +func TestBUG3839_UncheckedPatchCensusFindsEveryForm(t *testing.T) { + for _, tc := range []struct { + name string + src string + hits int + }{ + {"call", `import "` + diffImportPath + `"; func f() { diff.CreatePatch("a", "b") }`, 1}, + {"method_value", `import "` + diffImportPath + `"; var g = diff.CreateReversePatch`, 1}, + {"alias", `import d "` + diffImportPath + `"; func f() { d.CreatePatch("a", "b") }`, 1}, + {"dot", `import . "` + diffImportPath + `"; func f() { CreateReversePatch("a", "b"); _ = CreatePatch }`, 2}, + {"dot_inside_a_selector", `import . "` + diffImportPath + `"; func f() { _ = T{CreatePatch("a", "b")}.x; _ = other.CreatePatch }`, 1}, + {"checked_door", `import "` + diffImportPath + `"; func f() { diff.ReversePatch("a", "b") }`, 0}, + {"other_package_named_diff", `import "example.com/diff"; func f() { diff.CreatePatch("a", "b") }`, 0}, + {"alias_frees_the_name", `import d "` + diffImportPath + `"; func f(diff T) { diff.CreatePatch() }`, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "x.go", "package x\n"+tc.src, 0) + if err != nil { + t.Fatal(err) + } + if got := uncheckedPatchUses(fset, f); len(got) != tc.hits { + t.Fatalf("found %d uses, want %d: %v", len(got), tc.hits, got) + } + }) + } +} diff --git a/internal/diff/bug3839_no_panic_test.go b/internal/diff/bug3839_no_panic_test.go new file mode 100644 index 000000000..2f1c820ee --- /dev/null +++ b/internal/diff/bug3839_no_panic_test.go @@ -0,0 +1,164 @@ +package diff + +import ( + "math/rand" + "strings" + "testing" + "unicode/utf8" +) + +// BUG-3839: LineCounts, FormatDiffSummary and Unified are reached by text +// that may not be valid UTF-8 and are not guarded the way ReversePatch is. A +// 200k-case fuzz (seed 1, the generator below) found no panic in them while +// it found go-diff's PatchApply panicking at about 4%. This pins that +// measurement: the panic shapes as seeds, then the same generator, with an +// instrument showing the generator does reach the class. + +var bug3839Pieces = []string{"\xff", "\x80", "\xc3", "\xe2\x82", "\xf0\x9f\x98", "\xed\xa0\x80", "é", "😀", "\n", "a", "b", " "} + +// bug3839Pairs yields the seed shapes, then n generated pairs. +func bug3839Pairs(n int) [][2]string { + pairs := [][2]string{ + {"b\xe2\x82b\xff", "b"}, + {"\xff", ""}, + {"", "\x80"}, + {"\xe2\x82", "\xe2\x82\xac"}, + {"\xf0\x9f\x98", "\xf0\x9f\x98\x80"}, + {"\xed\xa0\x80", "a\xed\xa0\x80b"}, + {"x\n\xff\ny\n", "x\n\xc3\ny\n"}, + } + for _, tc := range bug3839Cases { + pairs = append(pairs, [2]string{tc.old, tc.new}) + } + r := rand.New(rand.NewSource(1)) + gen := func() string { + var b strings.Builder + for i, k := 0, r.Intn(12); i < k; i++ { + b.WriteString(bug3839Pieces[r.Intn(len(bug3839Pieces))]) + } + return b.String() + } + for i := 0; i < n; i++ { + o, nw := gen(), gen() + if r.Intn(2) == 0 { + nw = o[:r.Intn(len(o)+1)] + gen() + o[r.Intn(len(o)+1):] + } + pairs = append(pairs, [2]string{o, nw}) + } + return pairs +} + +func bug3839Panics(f func()) (panicked bool) { + defer func() { panicked = recover() != nil }() + f() + return false +} + +func TestBUG3839_DiffNeverPanicsOnInvalidUTF8(t *testing.T) { + pairs := bug3839Pairs(20000) + + // Instrument: the same inputs reach go-diff's panic. PatchApply is what + // ApplyPatch wrapped before it refused invalid text. + reached := 0 + for _, p := range pairs { + patches, err := dmp.PatchFromText("@@ -1,3 +1,4 @@\n a" + p[1] + "\n+b\n") + if err != nil { + continue + } + if bug3839Panics(func() { dmp.PatchApply(patches, p[0]) }) { + reached++ + } + } + t.Logf("instrument: %d of %d inputs panic go-diff's PatchApply", reached, len(pairs)) + if reached == 0 { + t.Fatal("instrument: no input reached go-diff's PatchApply panic, so a pass below shows nothing") + } + + for _, p := range pairs { + for name, f := range map[string]func(){ + "LineCounts": func() { LineCounts(p[0], p[1]) }, + "FormatDiffSummary": func() { FormatDiffSummary(p[0], p[1]) }, + "Unified": func() { Unified(p[0], p[1], 3) }, + "ApplyPatch": func() { _, _ = ApplyPatch(p[0], "@@ -1,3 +1,4 @@\n a"+p[1]+"\n+b\n") }, + } { + if bug3839Panics(f) { + t.Fatalf("%s(%q, %q) panicked", name, p[0], p[1]) + } + } + } +} + +// ApplyPatch refuses a base or a patch text that is not valid UTF-8 with an +// error, which the version walk reads as a patch that failed (BUG-3839). +func TestBUG3839_ApplyPatchRefusesInvalidUTF8(t *testing.T) { + valid := CreatePatch("ab", "abb") + for _, tc := range []struct{ name, base, patch string }{ + {"base_fuzz_first", "b\xe2\x82b\xff", "@@ -1,3 +1,4 @@\n ab\n+b\n"}, + {"base_lone_ff", "ab\xff", valid}, + {"base_truncated", "ab\xe2\x82", valid}, + {"patch_raw_byte", "ab", "@@ -1,2 +1,3 @@\n ab\n+\xff\n"}, + {"patch_escaped_byte", "ab", "@@ -1,2 +1,3 @@\n ab\n+%FF\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + var err error + if bug3839Panics(func() { _, err = ApplyPatch(tc.base, tc.patch) }) { + t.Fatal("ApplyPatch panicked") + } + if err == nil { + t.Fatal("ApplyPatch accepted text that is not valid UTF-8") + } + }) + } + if got, err := ApplyPatch("ab", valid); err != nil || got != "abb" { + t.Fatalf("valid patch: %q %v", got, err) + } + if got, err := ApplyPatch("a+b", CreatePatch("a+b", "a+b c%")); err != nil || got != "a+b c%" { + t.Fatalf("valid patch over + and %%: %q %v", got, err) + } +} + +// go-diff panics on VALID text too, when a well-formed patch is applied over +// a base it was not made from and the base holds multi-byte runes. The version +// walk does that after skipping a row; ApplyPatch reports it as an error. +func TestBUG3839_ApplyPatchOverAnotherValidBaseNeverPanicsOrMangles(t *testing.T) { + r := rand.New(rand.NewSource(1)) + pieces := []string{"é", "😀", "À", "\n", "a", "b", " ", "ab", "%", "+"} + gen := func() string { + var b strings.Builder + for i, k := 0, r.Intn(14); i < k; i++ { + b.WriteString(pieces[r.Intn(len(pieces))]) + } + return b.String() + } + reached, mangled := 0, 0 + for i := 0; i < 20000; i++ { + a, b, base := gen(), gen(), gen() + p := CreatePatch(a, b) + patches, err := dmp.PatchFromText(p) + if err != nil { + t.Fatal(err) + } + // Instrument: go-diff itself panics on this input. + if bug3839Panics(func() { dmp.PatchApply(patches, base) }) { + reached++ + } + // Instrument: go-diff returns text that is not valid UTF-8. + bug3839Panics(func() { + if out, _ := dmp.PatchApply(patches, base); !utf8.ValidString(out) { + mangled++ + } + }) + var got string + var applyErr error + if bug3839Panics(func() { got, applyErr = ApplyPatch(base, p) }) { + t.Fatalf("ApplyPatch(%q, CreatePatch(%q, %q)) panicked", base, a, b) + } + if applyErr == nil && !utf8.ValidString(got) { + t.Fatalf("ApplyPatch(%q, CreatePatch(%q, %q)) returned %q, which is not valid UTF-8", base, a, b, got) + } + } + t.Logf("instrument: of 20000 inputs, %d panic go-diff's PatchApply and %d make it return invalid UTF-8", reached, mangled) + if reached == 0 || mangled == 0 { + t.Fatal("instrument: the inputs do not reach both go-diff failures, so a pass shows nothing") + } +} diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 08b1d557e..047ea704a 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -4,7 +4,9 @@ package diff import ( "fmt" + "net/url" "strings" + "unicode/utf8" difflib "github.com/sergi/go-diff/diffmatchpatch" ) @@ -13,6 +15,7 @@ var dmp = difflib.New() // CreatePatch computes a compact patch that transforms oldText into newText. // The patch can be applied with ApplyPatch to recover newText from oldText. +// Both texts must be valid UTF-8 (BUG-3839; see ReversePatch). func CreatePatch(oldText, newText string) string { diffs := dmp.DiffMain(oldText, newText, true) patches := dmp.PatchMake(oldText, diffs) @@ -20,25 +23,82 @@ func CreatePatch(oldText, newText string) string { } // ApplyPatch applies a patch to baseText to produce the target text. -// Returns an error if the patch cannot be applied cleanly. +// Returns an error if the patch cannot be applied cleanly, or if the base or +// the patch's text is not valid UTF-8: go-diff slices such text at offsets it +// derives from runes, and panics or builds the wrong text (BUG-3839). +// +// go-diff also panics on valid text, when a patch is applied over a base it +// was not made from and the base holds multi-byte runes (about 1.5% of such +// pairs in the BUG-3839 measurement), or returns text that is not valid +// UTF-8. The version walk does exactly that after it skips a row, so both +// are errors here too. +// +// An empty patch returns baseText as it is, valid or not: it records a +// version byte-identical to its base, and go-diff is never reached. func ApplyPatch(baseText, patchText string) (string, error) { if patchText == "" { return baseText, nil } + if !utf8.ValidString(baseText) { + return "", fmt.Errorf("base text is not valid UTF-8") + } + if !patchTextValidUTF8(patchText) { + return "", fmt.Errorf("patch text is not valid UTF-8") + } patches, err := dmp.PatchFromText(patchText) if err != nil { return "", fmt.Errorf("parse patch: %w", err) } - result, applied := dmp.PatchApply(patches, baseText) + result, applied, err := patchApply(patches, baseText) + if err != nil { + return "", err + } // Check that all hunks applied for i, ok := range applied { if !ok { return "", fmt.Errorf("patch hunk %d failed to apply", i) } } + // Over a base it was not made from, go-diff can also succeed and cut a + // rune in half: valid text in, invalid text out. No version recorded that. + if !utf8.ValidString(result) { + return "", fmt.Errorf("applying the patch produced text that is not valid UTF-8") + } return result, nil } +// patchTextValidUTF8 reports whether every text line of a patch decodes to +// valid UTF-8. A line is escaped, so "%FF" is the byte 0xFF once parsed; this +// decodes each line the way PatchFromText does, whose result is unexported. +func patchTextValidUTF8(patchText string) bool { + for _, line := range strings.Split(patchText, "\n") { + if line == "" { + continue + } + switch line[0] { + case ' ', '-', '+': + // PatchFromText ignores the error too: a malformed line decodes + // to "", which is valid. + text, _ := url.QueryUnescape(strings.ReplaceAll(line[1:], "+", "%2b")) + if !utf8.ValidString(text) { + return false + } + } + } + return true +} + +// patchApply is go-diff's PatchApply with its panic returned as an error. +func patchApply(patches []difflib.Patch, base string) (result string, applied []bool, err error) { + defer func() { + if r := recover(); r != nil { + result, applied, err = "", nil, fmt.Errorf("go-diff panicked applying the patch: %v", r) + } + }() + result, applied = dmp.PatchApply(patches, base) + return result, applied, nil +} + // IsDiffSmaller returns true if storing a diff would be meaningfully smaller // than storing the full content. Returns false for near-complete rewrites // where the diff is >= 80% of the full content size. @@ -52,10 +112,28 @@ func IsDiffSmaller(patch, fullContent string) bool { // CreateReversePatch computes a patch that transforms newText back into oldText. // Used for reverse-diff version storage: current doc has latest content, // versions store patches to go backwards. +// +// Both texts must be valid UTF-8, as for CreatePatch; production code calls +// ReversePatch, which checks (BUG-3839, TestNoUncheckedPatchCallers). func CreateReversePatch(oldText, newText string) string { return CreatePatch(newText, oldText) } +// ReversePatch is CreateReversePatch for text that may not be valid UTF-8 +// (BUG-3839). go-diff works on runes and slices the input at byte offsets it +// derives from them, so invalid UTF-8 near an edit either panics (slice +// bounds out of range) or yields a patch that does not reproduce oldText: +// "hello world" -> "hello \xff world" reverse-applied to "hello d". ok is +// false when either side is not valid UTF-8, and the caller stores the full +// body instead. Never sanitize and diff: the version would then record a body +// it does not hold. +func ReversePatch(oldText, newText string) (patch string, ok bool) { + if !utf8.ValidString(oldText) || !utf8.ValidString(newText) { + return "", false + } + return CreatePatch(newText, oldText), true +} + // FormatDiffSummary returns a human-readable summary of changes. Its "lines" // are character-diff CHUNKS (a one-character edit reads +1/-1); LineCounts is // the line-level count the version rows store (PLAN-2348 U2). diff --git a/internal/store/attachments.go b/internal/store/attachments.go index 8457c0c86..7e814f26a 100644 --- a/internal/store/attachments.go +++ b/internal/store/attachments.go @@ -2047,7 +2047,7 @@ func (s *Store) planVersionChainRemapTx(tx *sql.Tx, workspaceID string, oldToNew if newBody != r.body || (r.isDiff && newSucc != oldSucc) { stored, isDiff := newBody, false if r.isDiff { - if patch := diff.CreateReversePatch(newBody, newSucc); diff.IsDiffSmaller(patch, newBody) { + if patch, ok := diff.ReversePatch(newBody, newSucc); ok && diff.IsDiffSmaller(patch, newBody) { stored, isDiff = patch, true } } diff --git a/internal/store/bug3839_broken_chain_test.go b/internal/store/bug3839_broken_chain_test.go new file mode 100644 index 000000000..13c945a51 --- /dev/null +++ b/internal/store/bug3839_broken_chain_test.go @@ -0,0 +1,60 @@ +package store_test + +import ( + "testing" + + "github.com/PerpetualSoftware/pad/internal/diff" + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3839: when a version row fails to apply, History keeps the body it had +// and applies the next older row over it, a base that row was not made from. +// go-diff panics on that for some multi-byte text, and the request was a 500. +// This walk has done that since the initial release. All text here is valid +// UTF-8; the older row reads as patch_failed instead. +func TestBUG3839_HistoryPastABrokenRowDoesNotPanic(t *testing.T) { + recorded, older, current := "a\né+abb😀", "%a+a😀ab", "é😀ab😀+%" + for _, backend := range contentBackends() { + t.Run(backend.name, func(t *testing.T) { + s := backend.open(t) + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + col, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Docs", Schema: `{"fields":[]}`}) + if err != nil { + t.Fatal(err) + } + item, err := s.CreateItem(ws.ID, col.ID, models.ItemCreate{Title: "Doc", Content: current, Fields: `{}`}) + if err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`DELETE FROM item_versions WHERE item_id = $1`, item.ID); err != nil { + t.Fatal(err) + } + for _, r := range []struct { + id, content, at string + seq int + }{ + // The newest row's patch does not apply to the current body. + {"r1", "@@ -1,3 +1,3 @@\n-zzz\n+yyy\n", "2026-10-02T00:00:00Z", 2}, + // The older row was made from the body r1 recorded. + {"r0", diff.CreatePatch(recorded, older), "2026-10-01T00:00:00Z", 1}, + } { + if _, err := s.DB().Exec(`INSERT INTO item_versions (id, item_id, content, is_diff, created_at, version_seq) VALUES ($1, $2, $3, $4, $5, $6)`, + r.id, item.ID, r.content, true, r.at, r.seq); err != nil { + t.Fatal(err) + } + } + vs, err := s.ListItemVersionsResolved(item.ID, current) + if err != nil || len(vs) != 2 { + t.Fatalf("versions: %d %v", len(vs), err) + } + for _, v := range vs { + if !v.Unreconstructable { + t.Fatalf("a row resolved over a base it was not made from: %q", v.Content) + } + } + }) + } +} diff --git a/internal/store/bug3839_invalid_utf8_version_test.go b/internal/store/bug3839_invalid_utf8_version_test.go new file mode 100644 index 000000000..5ed2f5c56 --- /dev/null +++ b/internal/store/bug3839_invalid_utf8_version_test.go @@ -0,0 +1,468 @@ +package store_test + +import ( + "strings" + "testing" + "unicode/utf8" + + "github.com/PerpetualSoftware/pad/internal/diff" + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-3839: go-diff cannot patch text that is not valid UTF-8. Near an edit it +// either panics (slice bounds out of range) or builds a patch that does not +// reproduce the version: "hello world" -> "hello \xff world" reverse-applied +// to "hello d". Every version writer now stores such a version as its full +// body. Requests cannot deliver invalid UTF-8 (encoding/json coerces it), but +// SQLite keeps it in rows an older release or a direct edit wrote, and the +// store writes whatever string it is given. +// +// Postgres refuses invalid UTF-8 in a text column, so there the body write +// itself fails, before any version is built: the PG legs pin that the store +// returns that error, does not panic, and leaves the item as it was. + +// bug3839Long is a body long enough that a small edit is stored as a patch. +func bug3839Long(tail string) string { + var b strings.Builder + for i := 0; i < 30; i++ { + b.WriteString("Paragraph about the deployment pipeline and its careful steps.\n\n") + } + b.WriteString(tail) + return b.String() +} + +// bug3839Shapes are the two failure shapes, as (before, after) bodies: the +// invalid byte adjacent to an edit, which panicked, and an invalid byte +// inserted into valid text, which built a wrong patch. +var bug3839Shapes = []struct { + name string + before, after string +}{ + {"panic", bug3839Long("a\xff"), bug3839Long("a\xffb")}, + {"wrong_patch", bug3839Long("hello world"), bug3839Long("hello \xff world")}, +} + +// newestVersion is the newest item version row's stored form. +func newestItemVersion(t *testing.T, s *store.Store, itemID string) (isDiff bool) { + t.Helper() + if err := s.DB().QueryRow(`SELECT is_diff FROM item_versions WHERE item_id = $1 ORDER BY created_at DESC, version_seq DESC LIMIT 1`, itemID).Scan(&isDiff); err != nil { + t.Fatal(err) + } + return isDiff +} + +// resolvesTo requires some item version to resolve to exactly want. +func itemVersionResolvesTo(t *testing.T, s *store.Store, itemID, current, want string) { + t.Helper() + vs, err := s.ListItemVersionsResolved(itemID, current) + if err != nil { + t.Fatal(err) + } + for _, v := range vs { + if v.Content == want && !v.Unreconstructable { + return + } + } + got := make([]string, 0, len(vs)) + for _, v := range vs { + got = append(got, v.Content[max(0, len(v.Content)-20):]) + } + t.Fatalf("no version resolves to the body it records; version tails: %q", got) +} + +// seedItemBody creates an item with a valid body, then sets the stored body +// to body directly, as a row an older release left behind. ok is false on +// Postgres, which refuses the write; that refusal is pinned. +func seedItemBody(t *testing.T, s *store.Store, pg bool, body string) (*models.Item, bool) { + t.Helper() + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + col, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Docs", Schema: `{"fields":[]}`}) + if err != nil { + t.Fatal(err) + } + item, err := s.CreateItem(ws.ID, col.ID, models.ItemCreate{Title: "Doc", Content: strings.ToValidUTF8(body, "?"), Fields: `{}`}) + if err != nil { + t.Fatal(err) + } + _, err = s.DB().Exec(`UPDATE items SET content = $1 WHERE id = $2`, body, item.ID) + if pg { + if utf8.ValidString(body) { + if err != nil { + t.Fatal(err) + } + return item, true + } + if err == nil { + t.Fatal("Postgres stored a body that is not valid UTF-8") + } + return item, false + } + if err != nil { + t.Fatal(err) + } + return item, true +} + +// UpdateItem (items.go, the version row for an edit) on both shapes. +func TestBUG3839_ItemEditOfInvalidUTF8StoresTheFullBody(t *testing.T) { + for _, backend := range contentBackends() { + for _, sh := range bug3839Shapes { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + item, seeded := seedItemBody(t, s, pg, sh.before) + if !seeded { + return // Postgres refused the stored body: the shape cannot exist there + } + after := sh.after + _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &after, LastModifiedBy: "alice", Source: "cli"}) + if pg { + if err == nil { + t.Fatal("Postgres accepted a body that is not valid UTF-8") + } + got, gerr := s.GetItem(item.ID) + if gerr != nil || got.Content != sh.before { + t.Fatalf("the refused write changed the item: %v", gerr) + } + return + } + if err != nil { + t.Fatal(err) + } + if newestItemVersion(t, s, item.ID) { + t.Fatal("the version of a body that is not valid UTF-8 was stored as a patch") + } + itemVersionResolvesTo(t, s, item.ID, after, sh.before) + }) + } + } +} + +// rebaseNewestVersionPatch (items.go): a throttled edit rebases the newest +// version's patch onto the new body, which is where the invalid text lands. +func TestBUG3839_ThrottledEditOntoInvalidUTF8RebasesToTheFullBody(t *testing.T) { + for _, backend := range contentBackends() { + for _, sh := range bug3839Shapes { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + a := bug3839Long("first draft") + item, _ := seedItemBody(t, s, pg, a) + write := func(body string) (*models.Item, error) { + return s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: "alice", Source: "web"}) + } + // First edit: a version of a, stored as a patch. + if _, err := write(strings.ToValidUTF8(sh.before, "?")); err != nil { + t.Fatal(err) + } + if !newestItemVersion(t, s, item.ID) { + t.Fatal("instrument: the first edit's version must be a reverse patch") + } + // Second edit, throttled: rebases that version onto sh.after. + out, err := write(sh.after) + if pg { + if err == nil { + t.Fatal("Postgres accepted a body that is not valid UTF-8") + } + return + } + if err != nil { + t.Fatal(err) + } + if !out.BodyEditedWithoutVersion { + t.Fatal("instrument: the second edit must be throttled, so the rebase runs") + } + if newestItemVersion(t, s, item.ID) { + t.Fatal("the rebased version of a body that is not valid UTF-8 was stored as a patch") + } + itemVersionResolvesTo(t, s, item.ID, sh.after, a) + }) + } + } +} + +// UpdateDocument (documents.go), the legacy documents' version row. +func TestBUG3839_DocumentEditOfInvalidUTF8StoresTheFullBody(t *testing.T) { + for _, backend := range contentBackends() { + for _, sh := range bug3839Shapes { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + doc, err := s.CreateDocument(ws.ID, models.DocumentCreate{Title: "Doc", Content: strings.ToValidUTF8(sh.before, "?"), CreatedBy: "user", Source: "web"}) + if err != nil { + t.Fatal(err) + } + _, err = s.DB().Exec(`UPDATE documents SET content = $1 WHERE id = $2`, sh.before, doc.ID) + if pg { + if utf8.ValidString(sh.before) { + if err != nil { + t.Fatal(err) + } + } else { + if err == nil { + t.Fatal("Postgres stored a body that is not valid UTF-8") + } + return + } + } else if err != nil { + t.Fatal(err) + } + after := sh.after + _, err = s.UpdateDocument(doc.ID, models.DocumentUpdate{Content: &after, LastModifiedBy: "user", Source: "cli"}) + if pg { + if err == nil { + t.Fatal("Postgres accepted a body that is not valid UTF-8") + } + return + } + if err != nil { + t.Fatal(err) + } + vs, err := s.ListVersionsResolved(doc.ID, after) + if err != nil || len(vs) == 0 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if vs[0].Content != sh.before { + t.Fatalf("the newest version resolves to a body it does not record (tail %q)", vs[0].Content[max(0, len(vs[0].Content)-20):]) + } + }) + } + } +} + +// planVersionChainRemapTx (attachments.go): an import's attachment remap +// re-diffs each version it can resolve against its remapped successor. Here +// the newest version's patch restores an attachment ref, and the item's +// current body carries an invalid byte next to it, as a direct edit or an +// older release would leave it. ApplyPatch refuses that base, so the remap cannot +// resolve the row and takes its unresolved branch: the patch text itself is +// remapped, and the row stays a diff that reads as patch_failed (lead ruling +// on BUG-3839 checkpoint 11). Before the +// refusal the patch applied fuzzily over the invalid byte, and the row was +// re-diffed from a body it never recorded. +func TestBUG3839_AttachmentRemapOverInvalidUTF8LeavesARowThatReadsPatchFailed(t *testing.T) { + oldID, newID := "00000000-0000-0000-0000-0000000000a1", "00000000-0000-0000-0000-0000000000b2" + ref := func(id string) string { return "![chart](pad-attachment:" + id + ")" } + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, bad string }{{"lone_ff", "\xff"}, {"truncated", "\xe2\x82"}} { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + validCurrent := bug3839Long("See the notes.") + older := bug3839Long("See " + ref(oldID) + " and the notes.") + item, _ := seedItemBody(t, s, pg, validCurrent) + patch := bug3839Patch(t, older, validCurrent) + if !strings.Contains(patch, oldID) { + t.Fatal("instrument: the patch must carry the attachment id, or a remap of its text shows nothing") + } + seedDiffVersion(t, s, item.ID, patch) + current := strings.Replace(validCurrent, " notes.", sh.bad+" notes.", 1) + if _, seeded := seedExisting(t, s, pg, item.ID, current); !seeded { + return + } + if err := s.RemapAttachmentReferencesInWorkspace(item.WorkspaceID, map[string]string{oldID: newID}); err != nil { + t.Fatal(err) + } + var stored string + var isDiff bool + if err := s.DB().QueryRow(`SELECT content, is_diff FROM item_versions WHERE id = 'v1'`).Scan(&stored, &isDiff); err != nil { + t.Fatal(err) + } + if !isDiff || stored != strings.ReplaceAll(patch, oldID, newID) { + t.Fatalf("v1 was not left a diff with its patch text remapped: is_diff=%v", isDiff) + } + vs, err := s.ListItemVersionsResolved(item.ID, strings.ReplaceAll(current, oldID, newID)) + if err != nil || len(vs) != 1 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if !vs[0].Unreconstructable || vs[0].UnreconstructableReason != "patch_failed" { + t.Fatalf("v1 reads unreconstructable=%v reason=%q", vs[0].Unreconstructable, vs[0].UnreconstructableReason) + } + }) + } + } +} + +// bug3839Patch is a reverse patch between valid texts, for fixtures. +func bug3839Patch(t *testing.T, older, newer string) string { + t.Helper() + p, ok := diff.ReversePatch(older, newer) + if !ok { + t.Fatal("fixture texts must be valid UTF-8") + } + return p +} + +// seedExisting sets an existing item's stored body directly; false on +// Postgres for a body that is not valid UTF-8 (refused, as seedItemBody). +func seedExisting(t *testing.T, s *store.Store, pg bool, itemID, body string) (string, bool) { + t.Helper() + _, err := s.DB().Exec(`UPDATE items SET content = $1 WHERE id = $2`, body, itemID) + if pg && !utf8.ValidString(body) { + if err == nil { + t.Fatal("Postgres stored a body that is not valid UTF-8") + } + return "", false + } + if err != nil { + t.Fatal(err) + } + return body, true +} + +// bug3839Unmade is a patch applied over a base it was not made from, valid +// UTF-8 throughout, on which go-diff's PatchApply panics (found by the +// BUG-3839 measurement; TestBUG3839_ApplyPatchOverAnotherValidBaseNeverPanics +// is the population). The version walk applies a patch over another base +// whenever it skips a row. +var bug3839Unmade = struct{ from, to, base string }{"a\né+abb😀", "%a+a😀ab", "é😀ab😀+%"} + +// bug3839Mangled is the same kind of pair on which go-diff instead succeeds +// and returns text that is not valid UTF-8 ("\x80"), from valid text (#2200 +// review r2). +var bug3839Mangled = struct{ from, to, base string }{"b%é😀+a\na%b 😀", "%é", "a \né\n😀"} + +// seedDiffVersion replaces the item's history with one unhashed diff row. +func seedDiffVersion(t *testing.T, s *store.Store, itemID, patch string) { + t.Helper() + if _, err := s.DB().Exec(`DELETE FROM item_versions WHERE item_id = $1`, itemID); err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`INSERT INTO item_versions (id, item_id, content, is_diff, created_at, version_seq) VALUES ('v1', $1, $2, $3, '2026-10-01T00:00:00Z', 1)`, + itemID, patch, true); err != nil { + t.Fatal(err) + } +} + +// History (ListItemVersionsResolved) over a diff row whose patch go-diff +// cannot apply: the current body is not valid UTF-8, or (both backends) the +// patch was made from another base. The row reads as patch_failed instead of +// panicking the request. +func TestBUG3839_ItemHistoryRefusesARowGoDiffCannotApply(t *testing.T) { + valid := bug3839Long("See the notes.") + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, current, patch string }{ + {"invalid_current", bug3839Long("See the\xe2\x82 notes."), bug3839Patch(t, bug3839Long("See notes."), valid)}, + {"unmade_base", bug3839Unmade.base, bug3839Patch(t, bug3839Unmade.to, bug3839Unmade.from)}, + {"mangled_result", bug3839Mangled.base, bug3839Patch(t, bug3839Mangled.to, bug3839Mangled.from)}, + } { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + item, _ := seedItemBody(t, s, pg, valid) + seedDiffVersion(t, s, item.ID, sh.patch) + if _, seeded := seedExisting(t, s, pg, item.ID, sh.current); !seeded { + return + } + vs, err := s.ListItemVersionsResolved(item.ID, sh.current) + if err != nil || len(vs) != 1 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if !vs[0].Unreconstructable || vs[0].UnreconstructableReason != "patch_failed" { + t.Fatalf("row reads unreconstructable=%v reason=%q content tail %q", vs[0].Unreconstructable, vs[0].UnreconstructableReason, vs[0].Content[max(0, len(vs[0].Content)-40):]) + } + }) + } + } +} + +// The legacy documents' walk (ListVersionsResolved) over the same rows. +func TestBUG3839_DocumentHistoryRefusesARowGoDiffCannotApply(t *testing.T) { + valid := bug3839Long("See the notes.") + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, current, patch string }{ + {"invalid_current", bug3839Long("See the\xe2\x82 notes."), bug3839Patch(t, bug3839Long("See notes."), valid)}, + {"unmade_base", bug3839Unmade.base, bug3839Patch(t, bug3839Unmade.to, bug3839Unmade.from)}, + {"mangled_result", bug3839Mangled.base, bug3839Patch(t, bug3839Mangled.to, bug3839Mangled.from)}, + } { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + if backend.name == "Postgres" && !utf8.ValidString(sh.current) { + return // Postgres cannot hold the body (seedItemBody pins the refusal) + } + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + doc, err := s.CreateDocument(ws.ID, models.DocumentCreate{Title: "Doc", Content: valid, CreatedBy: "user", Source: "web"}) + if err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`DELETE FROM versions WHERE document_id = $1`, doc.ID); err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`INSERT INTO versions (id, document_id, content, is_diff, created_at) VALUES ('dv1', $1, $2, $3, '2026-10-01T00:00:00Z')`, + doc.ID, sh.patch, true); err != nil { + t.Fatal(err) + } + vs, err := s.ListVersionsResolved(doc.ID, sh.current) + if err != nil || len(vs) != 1 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if !strings.HasPrefix(vs[0].Content, "[patch error: ") { + t.Fatalf("row resolved to %q", vs[0].Content[max(0, len(vs[0].Content)-40):]) + } + }) + } + } +} + +// rebaseNewestVersionPatch (items.go) applies the newest version's patch to +// the stored body before a throttled edit replaces it. When go-diff cannot +// apply it, the edit lands and the row is left as it was. +func TestBUG3839_ThrottledEditOverARowGoDiffCannotApplyLeavesTheRow(t *testing.T) { + valid := bug3839Long("See the notes.") + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, current, patch string }{ + {"invalid_current", bug3839Long("See the\xe2\x82 notes."), bug3839Patch(t, bug3839Long("See notes."), valid)}, + {"unmade_base", bug3839Unmade.base, bug3839Patch(t, bug3839Unmade.to, bug3839Unmade.from)}, + {"mangled_result", bug3839Mangled.base, bug3839Patch(t, bug3839Mangled.to, bug3839Mangled.from)}, + } { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + item, _ := seedItemBody(t, s, pg, valid) + write := func(body string) (*models.Item, error) { + return s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: "alice", Source: "web"}) + } + // A first edit makes alice's version row, so the next is throttled. + if _, err := write(bug3839Long("First draft.")); err != nil { + t.Fatal(err) + } + var vid string + if err := s.DB().QueryRow(`SELECT id FROM item_versions WHERE item_id = $1 ORDER BY created_at DESC, version_seq DESC LIMIT 1`, item.ID).Scan(&vid); err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`UPDATE item_versions SET content = $1, is_diff = $2, body_sha256 = NULL WHERE id = $3`, sh.patch, true, vid); err != nil { + t.Fatal(err) + } + if _, seeded := seedExisting(t, s, pg, item.ID, sh.current); !seeded { + return + } + out, err := write(bug3839Long("Second draft.")) + if err != nil { + t.Fatal(err) + } + if !out.BodyEditedWithoutVersion { + t.Fatal("instrument: the second edit must be throttled, so the rebase runs") + } + var content string + var isDiff bool + if err := s.DB().QueryRow(`SELECT content, is_diff FROM item_versions WHERE id = $1`, vid).Scan(&content, &isDiff); err != nil { + t.Fatal(err) + } + if content != sh.patch || !isDiff { + t.Fatalf("the row was rewritten: is_diff=%v", isDiff) + } + }) + } + } +} diff --git a/internal/store/documents.go b/internal/store/documents.go index ac4902182..ad35cefcc 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -404,8 +404,8 @@ func (s *Store) UpdateDocument(id string, input models.DocumentUpdate) (*models. // Falls back to full content if the diff isn't meaningfully smaller. versionContent := existing.Content isDiff := false - patch := diff.CreateReversePatch(existing.Content, *input.Content) - if diff.IsDiffSmaller(patch, existing.Content) { + // ok is false for text go-diff cannot patch (BUG-3839). + if patch, ok := diff.ReversePatch(existing.Content, *input.Content); ok && diff.IsDiffSmaller(patch, existing.Content) { versionContent = patch isDiff = true } diff --git a/internal/store/items.go b/internal/store/items.go index d6abb131d..ac2cc42fb 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -3075,13 +3075,15 @@ func (s *Store) updateItemWithParentLinkOnce( createdVersionID = vid versionContent := existing.Content isDiff := false - patch := diff.CreateReversePatch(existing.Content, *newBody) + // ok is false for a body that is not valid UTF-8, which go-diff + // cannot patch (BUG-3839): the full body is stored instead. + patch, patchOK := diff.ReversePatch(existing.Content, *newBody) // A reverse patch is resolved backwards from the ROW's body, which // on the applier path (ExternalContent, BUG-3327) is still the old // body until the tab's flush lands: a patch would resolve against // the wrong base in that window. The full body resolves the same // whatever the row holds. - if input.Content != nil && diff.IsDiffSmaller(patch, existing.Content) { + if input.Content != nil && patchOK && diff.IsDiffSmaller(patch, existing.Content) { versionContent = patch isDiff = true } @@ -6209,7 +6211,7 @@ func (s *Store) rebaseNewestVersionPatch(tx *sql.Tx, itemID, oldBody, newBody st return nil } stored, storedDiff := body, false - if patch := diff.CreateReversePatch(body, newBody); diff.IsDiffSmaller(patch, body) { + if patch, ok := diff.ReversePatch(body, newBody); ok && diff.IsDiffSmaller(patch, body) { stored, storedDiff = patch, true } added, removed := diff.LineCounts(body, newBody)