Files
pad/internal/diff/bug3839_invalid_utf8_test.go
T
xarmian 09ecc5e02f fix(diff): version writers never diff text that is not valid UTF-8 (BUG-3839) (#2200)
* fix(diff): version writers never diff text that is not valid UTF-8 (BUG-3839)

go-diff works on runes and slices its input at byte offsets derived from
them, so text that is not valid UTF-8 near an edit either panics in
CreatePatch (slice bounds out of range) or builds a patch that does not
reproduce the old text: "hello world" -> "hello \xff world" reverse-applied
to "hello d", with no error. Any invalid byte does it, lone \xff and \x80 as
well as truncated sequences.

diff.ReversePatch returns (patch, ok), false when either side is not valid
UTF-8, and every version writer stores the full body in that case: the item
edit and the throttled rebase (items.go), the import's attachment remap
(attachments.go) and the legacy document edit (documents.go). The full body
is stored losslessly on SQLite; Postgres refuses invalid UTF-8 in the
column, so there the body write fails first and nothing is versioned. No
existing row is rewritten.

Requests cannot deliver such text (encoding/json coerces it, artifact import
rejects it, the materializer emits U+FFFD for lone surrogates), but SQLite
rows from older releases or direct edits can hold it, and an item like that
500ed on every save.

Tests: diff unit pins for both shapes, a census that production code calls
only ReversePatch, and a store pin per caller on SQLite and Postgres. With
the UTF-8 check removed every SQLite leg panics or stores a wrong version,
and the census names the reintroduced call.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* test(diff): LineCounts, FormatDiffSummary and Unified never panic on invalid UTF-8 (BUG-3839)

Pins the 200k-case fuzz measurement deterministically: the panic shapes as
seeds plus 20k generated pairs from the same seed-1 generator, with an
instrument showing the inputs reach go-diff's PatchApply panic.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* fix(diff): ApplyPatch returns an error where go-diff would panic (BUG-3839)

ApplyPatch refuses a base or a decoded patch text that is not valid UTF-8,
and turns a go-diff PatchApply panic into an error. The second covers valid
text: a well-formed patch applied over a base it was not made from panics in
about 1.5% of measured pairs, and the version walk does that after it skips
a row. Every caller (the item and document walks, the throttled-edit rebase,
the attachment remap, the tie diagnostic) already treats the error as a
patch that failed, so the row reads as patch_failed instead of a 500.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* test(store): the remap pin asserts the row it leaves over an invalid body (BUG-3839)

With ApplyPatch refusing an invalid base, the remap cannot resolve a diff
row over the invalid current body: it remaps the patch text in place, and
the row reads as patch_failed. The pin asserts exactly that; it is red when
the base check is removed (the row is then re-diffed from a fuzzy body).

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* refactor(diff): recover wraps only the go-diff call, and the remap pin is named for patch_failed (BUG-3839)

Lead rulings on BUG-3839 checkpoint 11.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* test(diff): the unchecked-patch census resolves the import name (BUG-3839)

Matches any selector on the file's local name for the diff package (alias,
method value), bare identifiers under a dot import, and skips web,
node_modules, .git and vendor only at the module root. A self-test runs the
matcher over each form.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* test(store): History past a broken row does not panic (BUG-3839)

The main-reproduction pin: a broken newest row, then an older row applied
over a base it was not made from. Red on main (d95f90850, slice bounds
panic), green here.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* fix(diff): ApplyPatch refuses a result that is not valid UTF-8 (BUG-3839)

Over a base it was not made from, go-diff can succeed and cut a rune in
half: valid text in, "\x80" out (#2200 review r2, hunter). The rebase would
write that body (Postgres refuses it, failing the edit) and History would
show it. Pinned in the diff fuzz leg and in a mangled_result leg of the item
History, document History and throttled-rebase store tests, on both
backends; red when the check is removed. The doc comment now says an empty
patch returns its base unchecked, which is exact.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS
2026-10-15 10:24:24 -04:00

190 lines
6.2 KiB
Go

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)
}
})
}
}