diff --git a/internal/bodypatch/bodypatch.go b/internal/bodypatch/bodypatch.go index c369f28d3..8b627f181 100644 --- a/internal/bodypatch/bodypatch.go +++ b/internal/bodypatch/bodypatch.go @@ -7,6 +7,7 @@ package bodypatch import ( + "errors" "fmt" "strings" "unicode/utf8" @@ -56,6 +57,7 @@ func (e *AnchorAmbiguousError) Error() string { // ones before it, and returns the new body. All or nothing: the first edit // that cannot apply returns its error and no body. func Apply(body string, edits []Edit) (string, error) { + original := body if len(edits) == 0 { return "", fmt.Errorf("no edits") } @@ -96,9 +98,17 @@ func Apply(body string, edits []Edit) (string, error) { } body = body[:at[0]] + e.New + body[at[0]+len(e.Old):] } + if body == original { + return "", ErrNoChange + } return body, nil } +// ErrNoChange: the edits, applied in order, leave the body exactly as it was +// (retro CONVE-735 finding on #2055). A patch is a version; one that changes +// nothing is refused rather than advancing seq with no version behind it. +var ErrNoChange = errors.New("the edits change nothing") + // matches returns every start offset of sub in s, overlapping ones included: // "aa" occurs twice in "aaa", and a uniqueness check that counted it once // would let an edit land on whichever one a scan happened to find first. @@ -156,19 +166,26 @@ func ApplyWithContext(body string, edits []Edit) (string, []AppliedEdit, error) p := at[0] body = body[:p] + e.New + body[p+len(e.Old):] end := p + len(e.New) - out = append(out, AppliedEdit{Old: e.Old, New: e.New, CtxBefore: ctxBefore(body, p), CtxAfter: ctxAfter(body, end)}) + cb, ca := ctxBefore(body, p, true), ctxAfter(body, end, true) + if cb+e.New+ca == "" { + // The edit emptied its line (retro CONVE-735 finding on #2058): + // line-clipped context is empty on both sides, which would leave + // the undo no anchor at all. Reach across the line breaks instead. + cb, ca = ctxBefore(body, p, false), ctxAfter(body, end, false) + } + out = append(out, AppliedEdit{Old: e.Old, New: e.New, CtxBefore: cb, CtxAfter: ca}) } return body, out, nil } -// ctxBefore is up to ContextChars bytes before p, not crossing a newline, and -// never splitting a UTF-8 sequence. -func ctxBefore(s string, p int) string { +// ctxBefore is up to ContextChars bytes before p, not crossing a newline when +// clip is set, and never splitting a UTF-8 sequence. +func ctxBefore(s string, p int, clip bool) string { from := p - ContextChars if from < 0 { from = 0 } - if nl := strings.LastIndexByte(s[from:p], '\n'); nl >= 0 { + if nl := strings.LastIndexByte(s[from:p], '\n'); clip && nl >= 0 { from += nl + 1 } for from < p && !utf8.RuneStart(s[from]) { @@ -177,13 +194,14 @@ func ctxBefore(s string, p int) string { return s[from:p] } -// ctxAfter is up to ContextChars bytes from p, stopping before a newline. -func ctxAfter(s string, p int) string { +// ctxAfter is up to ContextChars bytes from p, stopping before a newline when +// clip is set. +func ctxAfter(s string, p int, clip bool) string { to := p + ContextChars if to > len(s) { to = len(s) } - if nl := strings.IndexByte(s[p:to], '\n'); nl >= 0 { + if nl := strings.IndexByte(s[p:to], '\n'); clip && nl >= 0 { to = p + nl } for to > p && to < len(s) && !utf8.RuneStart(s[to]) { diff --git a/internal/bodypatch/undo_test.go b/internal/bodypatch/undo_test.go index 35137fb6d..9bf0c2537 100644 --- a/internal/bodypatch/undo_test.go +++ b/internal/bodypatch/undo_test.go @@ -77,3 +77,13 @@ func TestContextIsBoundedAndLineClipped(t *testing.T) { t.Fatalf("context = %q / %q", a.CtxBefore, a.CtxAfter) } } + +// Retro CONVE-735 finding on #2058: an edit that deletes a line's only +// content left empty line-clipped context on both sides, so the inverse's +// anchor was "" and the undo was refused as an empty anchor. +func TestUndoOfALineEmptiedOut(t *testing.T) { + t.Run("middle line", func(t *testing.T) { roundTrip(t, "alpha\nbeta\ngamma\n", []Edit{{Old: "beta", New: ""}}) }) + t.Run("first line", func(t *testing.T) { roundTrip(t, "alpha\nbeta\n", []Edit{{Old: "alpha", New: ""}}) }) + t.Run("last line, no trailing newline", func(t *testing.T) { roundTrip(t, "alpha\nbeta", []Edit{{Old: "beta", New: ""}}) }) + t.Run("between blank lines", func(t *testing.T) { roundTrip(t, "a\n\nb\n\nc", []Edit{{Old: "b", New: ""}}) }) +} diff --git a/internal/server/handlers_items_patch.go b/internal/server/handlers_items_patch.go index b3db7d0f2..cd69d9c0f 100644 --- a/internal/server/handlers_items_patch.go +++ b/internal/server/handlers_items_patch.go @@ -166,6 +166,17 @@ func (s *Server) handlePatchItem(w http.ResponseWriter, r *http.Request) { return } req.Edits = bodypatch.Invert(applied) + for _, e := range req.Edits { + if e.Old == "" { + // The patch emptied the whole body: no text is left for an + // anchored undo to name (retro CONVE-735 finding on #2058). + // Every patch writes a version, so History can restore it. + writeError2(w, http.StatusBadRequest, "bad_request", + "that patch emptied the whole body, so there is no text left to anchor an undo on; nothing was written. Restore the version before it from the item's history instead", + map[string]interface{}{"undo": req.Undo}) + return + } + } p.undoOf, p.undoItemID = rec.ID, rec.ItemID } if len(req.Edits) == 0 { @@ -183,6 +194,15 @@ func (s *Server) handlePatchItem(w http.ResponseWriter, r *http.Request) { map[string]interface{}{"edit": i}) return } + // A no-op edit would advance seq and write no version, against the + // contract that every patch is a version (retro CONVE-735 finding on + // #2055). It is refused, writing nothing. + if e.Old == e.New { + writeError2(w, http.StatusBadRequest, "bad_request", + fmt.Sprintf("edit %d changes nothing (old and new are the same); nothing was written", i), + map[string]interface{}{"edit": i}) + return + } } p.edits = req.Edits key, kerr := idempotencyKey(r, req.OpKey) @@ -266,6 +286,10 @@ func (s *Server) writePatchRefusal(w http.ResponseWriter, ref string, err error) writeError2(w, http.StatusBadRequest, "bad_request", empty.Error(), map[string]interface{}{"edit": empty.Index}) return true } + if errors.Is(err, bodypatch.ErrNoChange) { + writeError2(w, http.StatusBadRequest, "bad_request", ref+": the edits change nothing (together they leave the body as it was); nothing was written", map[string]interface{}{"ref": ref}) + return true + } return false } diff --git a/internal/server/plan3578_retro_fixes_test.go b/internal/server/plan3578_retro_fixes_test.go new file mode 100644 index 000000000..85da434c9 --- /dev/null +++ b/internal/server/plan3578_retro_fixes_test.go @@ -0,0 +1,56 @@ +package server + +import ( + "net/http" + "strings" + "testing" +) + +// Retro CONVE-735 findings on #2055 and #2058. + +// A no-op edit, or edits that cancel out, used to report success and advance +// seq with no version behind it. They are refused and write nothing. +func TestPLAN3578_NoOpPatchIsRefused(t *testing.T) { + t.Parallel() + f := newPatchFixture(t, testServer(t), "alpha\nbeta\n") + before := f.current(t) + for name, e := range map[string][]map[string]any{ + "one edit, old == new": edits("beta", "beta"), + "two edits cancel out": edits("beta", "BETA", "BETA", "beta"), + } { + code, out := f.patch(t, map[string]any{"edits": e}) + if code != http.StatusBadRequest { + t.Fatalf("%s: %d %v", name, code, out) + } + } + if after := f.current(t); after.Seq != before.Seq || after.Content != before.Content { + t.Fatalf("a refused no-op wrote: seq %d → %d", before.Seq, after.Seq) + } +} + +// Deleting a line's only content left the undo with no anchor; it is +// undoable now. A patch that empties the WHOLE body still cannot be undone +// by anchor, and says so (History restores it). +func TestPLAN3578_UndoOfAnEmptiedLine(t *testing.T) { + t.Parallel() + f := newPatchFixture(t, testServer(t), "alpha\nbeta\ngamma\n") + code, out := f.patch(t, map[string]any{"edits": edits("beta", "")}) + if code != http.StatusOK { + t.Fatalf("patch: %d %v", code, out) + } + code, out = f.patch(t, map[string]any{"undo": out["patch_id"]}) + if code != http.StatusOK || f.current(t).Content != "alpha\nbeta\ngamma\n" { + t.Fatalf("undo: %d %v, body %q", code, out, f.current(t).Content) + } + + g := newPatchFixture(t, testServer(t), "only") + code, out = g.patch(t, map[string]any{"edits": edits("only", "")}) + if code != http.StatusOK { + t.Fatalf("emptying patch: %d %v", code, out) + } + code, out = g.patch(t, map[string]any{"undo": out["patch_id"]}) + msg, _ := out["error"].(map[string]any)["message"].(string) + if code != http.StatusBadRequest || !strings.Contains(msg, "history") { + t.Fatalf("undo of a whole-body clear: %d %v", code, out) + } +}