diff options
| -rw-r--r-- | CHANGELOG.md | 8 | ||||
| -rw-r--r-- | cmd/krino/explain.go | 3 | ||||
| -rw-r--r-- | cmd/krino/history_test.go | 9 | ||||
| -rw-r--r-- | cmd/krino/undo.go | 2 | ||||
| -rw-r--r-- | docs/design.md | 9 | ||||
| -rw-r--r-- | internal/engine/apply.go | 18 | ||||
| -rw-r--r-- | internal/engine/exclude_test.go | 26 | ||||
| -rw-r--r-- | internal/engine/undo_identity_test.go | 80 | ||||
| -rw-r--r-- | internal/extract/extract.go | 7 | ||||
| -rw-r--r-- | internal/extract/plain.go | 4 | ||||
| -rw-r--r-- | internal/extract/plain_test.go | 9 | ||||
| -rw-r--r-- | internal/journal/read.go | 6 | ||||
| -rw-r--r-- | internal/journal/read_test.go | 29 | ||||
| -rw-r--r-- | internal/plan/fuzz_test.go | 5 | ||||
| -rw-r--r-- | man/krino.1 | 1 | ||||
| -rw-r--r-- | man/krino.conf.5 | 4 |
16 files changed, 203 insertions, 17 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index f86844f..aa02136 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,15 +23,17 @@ Hostile names and contents - A content exclude holds when its content test cannot read the file (fails closed), so a protected file krino could not read is left alone. A file whose format has no text (an image, an archive) simply does not - match a content test, so it is not set aside. + match a content test, so it is not set aside; one that starts as text + and turns binary further on is unreadable, and is. - Extraction tools are only found through absolute PATH entries. Apply and undo - Every step is logged as soon as it has run; a step that ran but could not be logged is reported with where its file is now. SIGHUP is handled like SIGTERM, unless krino runs under `nohup`. An interrupt finishes the - current step and skips the rest of that file's chain. A run killed - mid-chain can be undone as far as it got. + current step and skips the rest of that file's chain (an undo finishes + the file it is reversing). A run killed mid-chain can be undone as far + as it got. - Undo reverses a file its run renamed and then moved, moved twice, or moved and then trashed (it refused them before). - Undo restores a trash entry only while it is still the file the run put diff --git a/cmd/krino/explain.go b/cmd/krino/explain.go index 8d5418e..5f9f6de 100644 --- a/cmd/krino/explain.go +++ b/cmd/krino/explain.go @@ -26,6 +26,9 @@ func cmdExplain(g *globals, args []string, stdout, stderr io.Writer) int { if fs.NArg() != 1 { return usageError(stderr, "usage: krino explain FILE") } + if code, refused := refuseUnusedFlags(g, stderr, "explain", false); refused { + return code + } minAge, setMinAge, err := minAgeOverride(g) if err != nil { return usageError(stderr, err.Error()) diff --git a/cmd/krino/history_test.go b/cmd/krino/history_test.go index 7c0f810..9c409bc 100644 --- a/cmd/krino/history_test.go +++ b/cmd/krino/history_test.go @@ -142,11 +142,14 @@ func TestFinalizeUndoPlanMarksUnapprovedAsDeclined(t *testing.T) { {File: "a"}, // index 0: approved {File: "b"}, // index 1: not approved -> declined {File: "c", Refused: "gone"}, // index 2: refused, never declined - }} + }, Cleanup: []engine.UndoFile{{File: "d"}}} out, _ := finalizeUndoPlan(up, map[int]bool{0: true}, 'c') if len(out.Files) != 3 { t.Fatalf("files = %+v, want all three carried through", out.Files) } + if len(out.Cleanup) != 1 || out.Cleanup[0].File != "d" { + t.Errorf("Cleanup = %+v, want the plan's directory cleanup carried over", out.Cleanup) + } if out.Files[0].Declined || out.Files[0].Refused != "" { t.Errorf("approved file changed: %+v", out.Files[0]) } @@ -542,6 +545,10 @@ func TestIgnoredGlobalFlagsAreRefused(t *testing.T) { {"-v", "log"}, {"--json", "undo", "-n"}, {"-v", "undo", "-n"}, + {"-y", "explain", dl}, + {"-n", "explain", dl}, + {"--json", "explain", dl}, + {"-v", "explain", dl}, } { if code, _, errOut := runCLI(t, args...); code != 2 || !strings.Contains(errOut, "does not take") { t.Errorf("krino %q: exit %d, stderr %q; want 2, refused", args, code, errOut) diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go index 1b5e6ae..4c71fdd 100644 --- a/cmd/krino/undo.go +++ b/cmd/krino/undo.go @@ -317,7 +317,7 @@ func releaseUndoLocks(locks []*lock.Lock) []error { // left out of the plan entirely, as review's [w] leaves a forward file // unlogged, and counted in notReviewed (review cli F2). func finalizeUndoPlan(up *engine.UndoPlan, approved map[int]bool, action rune) (plan *engine.UndoPlan, notReviewed int) { - out := &engine.UndoPlan{Run: up.Run} + out := &engine.UndoPlan{Run: up.Run, Cleanup: up.Cleanup} for i, f := range up.Files { if f.Refused == "" { yes, decided := approved[i] diff --git a/docs/design.md b/docs/design.md index 28c7c80..6b94fc8 100644 --- a/docs/design.md +++ b/docs/design.md @@ -204,7 +204,9 @@ cannot read the file (over `max-read`, a tool missing, failing or timing out), the exclude holds, and the file is set aside as "(content unreadable)" with the warning. A file whose format has no text at all (an image, an archive) is not unreadable: it contains no keyword, so its -content tests are simply false and raise no warning. An exclude protects files, so a file krino +content tests are simply false and raise no warning. A file of no known +extension that starts as text and holds binary data further on is +unreadable. An exclude protects files, so a file krino could not check is left alone. Mistakes in krino.conf's excludes are reported even while no directory is included. @@ -322,7 +324,7 @@ Nothing is deleted, and undo restores them. | Format | Method | |---|---| -| text | known text extensions, or detected from the first 8 KiB: valid UTF-8, or UTF-16 with a BOM, and no NUL bytes | +| text | known text extensions, or detected from the first 8 KiB: valid UTF-8, or UTF-16 with a BOM, and no NUL bytes. A file detected as text that holds a NUL or invalid UTF-8 further on cannot be read (§4.6, §5.4) | | pdf | `pdftotext -q -enc UTF-8 FILE -`, 30 s timeout | | docx xlsx pptx odt ods odp epub | zip plus streaming XML, Go standard library | | html xml | tags removed, entities decoded | @@ -715,7 +717,8 @@ Exit status: 0 success, including nothing to do and everything declined; A config error anywhere stops the whole run before any scanning: krino never acts on a config it only partly understood. Ctrl-C (SIGTERM, or SIGHUP unless started under `nohup`) during apply finishes the current step, logs it, skips -the rest of that file's chain, and stops. A second interrupt exits at once: +the rest of that file's chain, and stops; during an undo it finishes reversing +the file it is on, then stops. A second interrupt exits at once: the step in flight is not logged, and a copy in progress can leave a temporary `.krino-*` file and an empty directory behind. A second krino on the same directory waits for the lock, or fails immediately with `-y` (so a cron job never piles up). diff --git a/internal/engine/apply.go b/internal/engine/apply.go index f9cb9ef..46026ba 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -308,6 +308,14 @@ func (e *Engine) Runs(n int) ([]journal.Run, error) { type UndoPlan struct { Run string Files []UndoFile + + // Cleanup holds files with nothing left to reverse but directories the + // run made that something else occupied when the plan was built (re-review + // undo F3). They are not offered - that would repeat on every undo - but + // ApplyUndo removes any of those directories the other reversals leave + // empty, and logs it (plan 10 re-check R1). A front end that rebuilds the + // plan must carry Cleanup over. + Cleanup []UndoFile } // UndoFile is the reversal of one file's chain, last original step first. @@ -414,6 +422,7 @@ func (e *Engine) PlanUndo(runID string) (*UndoPlan, error) { // the run made that something else still occupies: offering them // would repeat on every undo (re-review undo F3). An empty one is // still offered, and removed. + up.Cleanup = append(up.Cleanup, uf) continue } up.Files = append(up.Files, uf) @@ -445,7 +454,9 @@ func onlyOccupiedDirectoryRemovals(steps []UndoStep) bool { func isUndoRun(entries []journal.Entry) bool { any := false for _, en := range entries { - if en.Action == "run-start" || en.Action == "run-end" { + // A damaged line says nothing about which kind of run this is (plan + // 10 re-check R3). + if en.Action == "run-start" || en.Action == "run-end" || en.Action == "damaged" { continue } any = true @@ -873,6 +884,11 @@ func (e *Engine) ApplyUndo(ctx context.Context, up *UndoPlan, j *journal.Writer, } } + for _, f := range up.Cleanup { + for i, us := range f.Steps { + retries = append(retries, dirRetry{dir: us.Src, dirName: f.Dir, file: f.File, step: i + 1, log: true}) + } + } if err := e.retryDirRemovals(j, run, retries); err != nil { return result, fmt.Errorf("engine: apply undo: %w", err) } diff --git a/internal/engine/exclude_test.go b/internal/engine/exclude_test.go index 4bb1512..4e9f6e7 100644 --- a/internal/engine/exclude_test.go +++ b/internal/engine/exclude_test.go @@ -302,3 +302,29 @@ func TestNoTextFormatIsNoMatch(t *testing.T) { t.Errorf("explain: Excluded %q, want none", x.Excluded) } } + +// TestTextTurningBinaryFailsClosed: a file with no known extension whose +// first 8 KiB read as text but which holds a NUL further on is unreadable, +// not "no text": a content exclude still sets it aside (plan 10 re-check R4). +func TestTextTurningBinaryFailsClosed(t *testing.T) { + mixed := "confidential " + strings.Repeat("x", 9000) + "\x00tail" + h, _ := excludeTree(t, map[string]string{"mixed": mixed}) + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` +(path "~/dl") +(exclude (content "confidential")) +(rule "all" (move "Out")) +`}) + e, errs := Load(main) + if len(errs) > 0 { + t.Fatal(errs) + } + r, err := e.Match(context.Background(), e.Dirs[0]) + if err != nil { + t.Fatal(err) + } + for _, fm := range r.Matched { + if fm.File.Rel == "mixed" && (!strings.HasSuffix(fm.Excluded, "(content unreadable)") || len(fm.Rules) != 0) { + t.Errorf("mixed: Excluded %q, rules %d; want set aside as unreadable", fm.Excluded, len(fm.Rules)) + } + } +} diff --git a/internal/engine/undo_identity_test.go b/internal/engine/undo_identity_test.go index f9d6307..f42c5da 100644 --- a/internal/engine/undo_identity_test.go +++ b/internal/engine/undo_identity_test.go @@ -492,3 +492,83 @@ func TestUndoRefusesATrashEntryRecordedForAnotherPath(t *testing.T) { t.Errorf("Refused = %q; want the trash entry named as another file's", f.Refused) } } + +// TestFinishedUndoRemovesADirectoryLeftEmpty: a directory made by one file's +// chain and still holding another file is not offered on its own, but once +// that other file's reversal empties it, the undo removes it (plan 10 +// re-check R1). +func TestFinishedUndoRemovesADirectoryLeftEmpty(t *testing.T) { + e, run, h, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one", "b.pdf": "two"}}, + map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (move \"Out\"))\n"}) + out := filepath.Join(h, "dl", "Out") + undo := func(decline string) { + t.Helper() + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + for i := range up.Files { + up.Files[i].Declined = up.Files[i].File == decline + } + j, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + defer j.Close() + if _, err := e.ApplyUndo(context.Background(), up, j, journal.NewRunID(time.Now())); err != nil { + t.Fatal(err) + } + } + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + other := "" + for _, f := range up.Files { + made := false + for _, s := range f.Steps { + made = made || s.Action == "undo-mkdir" + } + if !made { + other = f.File + } + } + undo(other) // the file that made Out goes back; the other still holds Out + if _, err := os.Stat(out); err != nil { + t.Fatalf("Out went while %s still held it: %v", other, err) + } + undo("") // the other goes back, leaving Out empty + if _, err := os.Lstat(out); !os.IsNotExist(err) { + t.Errorf("Out is still there after the undo finished: %v", err) + } +} + +// TestUndoRunWithADamagedLineIsStillAnUndo: a damaged line in an undo run's +// log does not make that run look like an ordinary one that can be undone +// (plan 10 re-check R3). +func TestUndoRunWithADamagedLineIsStillAnUndo(t *testing.T) { + e, run, _, logPath := appliedRun(t, map[string]map[string]string{"dl": {"a.pdf": "one"}}, + map[string]string{"dl": "(path \"~/dl\")\n(rule \"r\" (rename \"r-{name}\") (move \"Out\"))\n"}) + up, err := e.PlanUndo(run) + if err != nil { + t.Fatal(err) + } + j, err := journal.Open(logPath) + if err != nil { + t.Fatal(err) + } + undoRun := journal.NewRunID(time.Now().Add(time.Second)) + if _, err := e.ApplyUndo(context.Background(), up, j, undoRun); err != nil { + t.Fatal(err) + } + j.Close() + f, err := os.OpenFile(logPath, os.O_APPEND|os.O_WRONLY, 0) + if err != nil { + t.Fatal(err) + } + f.WriteString(time.Now().UTC().Format(time.RFC3339) + "\t" + undoRun + "\tdl\ta.pdf\t9\tundo-mo\n") + f.Close() + if _, err := e.PlanUndo(undoRun); err == nil || !strings.Contains(err.Error(), "itself an undo") { + t.Errorf("PlanUndo(undo run with a damaged line) = %v; want refused as an undo", err) + } +} diff --git a/internal/extract/extract.go b/internal/extract/extract.go index ad51c00..376e09b 100644 --- a/internal/extract/extract.go +++ b/internal/extract/extract.go @@ -17,12 +17,17 @@ import ( // Version is the version of the text this package extracts. Bump it // whenever a change could make any format's text differ, so every keyword // cache built from the old text is discarded (Fingerprint). -const Version = 1 +const Version = 2 var ( // ErrUnsupported is returned when the format carries no text krino // knows how to extract. ErrUnsupported = errors.New("no text in this format") + // ErrMixed is returned for a file of no known extension whose first + // 8 KiB read as text but which holds a NUL or invalid UTF-8 further on. + // Unlike ErrUnsupported it is a read failure: the text it began with + // could hold a keyword (plan 10 re-check R4). + ErrMixed = errors.New("text at the start, binary data further on") // ErrTooLarge is returned when the file is larger than the configured // max-read; nothing is read in that case. ErrTooLarge = errors.New("larger than max-read") diff --git a/internal/extract/plain.go b/internal/extract/plain.go index d245a41..f7470be 100644 --- a/internal/extract/plain.go +++ b/internal/extract/plain.go @@ -38,7 +38,7 @@ func readDecoded(path string) (string, error) { // the sample alone (UTF-16 text is full of NUL bytes by design), but a // sample that merely looks like UTF-8 must hold for the WHOLE file — no // NUL byte anywhere, and no invalid UTF-8 anywhere past the sample — or -// the file is ErrUnsupported after all; the Latin-1 fallback in decode +// the file is ErrMixed, unreadable; the Latin-1 fallback in decode // never applies to a sniffed file, only to a file whose extension already // names it as text. D2: when the file continues past the sample (n == // sniffSize), the validity check is run against a trimmed copy with any @@ -81,7 +81,7 @@ func sniffText(path string) (string, error) { return decode(data), nil } if !utf8.Valid(data) || bytes.Contains(data, []byte{0}) { - return "", ErrUnsupported + return "", ErrMixed } return decode(data), nil } diff --git a/internal/extract/plain_test.go b/internal/extract/plain_test.go index 045ea26..f49caf8 100644 --- a/internal/extract/plain_test.go +++ b/internal/extract/plain_test.go @@ -154,12 +154,15 @@ func TestToolsListedInOrder(t *testing.T) { // but the file goes on to hold an invalid UTF-8 byte and a NUL past that // sample — sniffText must reject the whole file, not just decode what the // sample alone promised (it must not fall back to Latin-1 the way a known -// text extension would). +// text extension would). It is not "no text in this format" either: the +// text it began with could hold a keyword, so it is ErrMixed, a read +// failure, and a content exclude fails closed on it (plan 10 re-check R4). func TestSniffWholeFileMustBeValid(t *testing.T) { e := newWithPath("") data := append([]byte(strings.Repeat("x", 8192)), 0xFF, 0x00) - if _, err := text(t, e, file(t, "blob.data", data), 0); !errors.Is(err, ErrUnsupported) { - t.Errorf("got %v, want ErrUnsupported", err) + _, err := text(t, e, file(t, "blob.data", data), 0) + if !errors.Is(err, ErrMixed) || errors.Is(err, ErrUnsupported) { + t.Errorf("got %v, want ErrMixed and not ErrUnsupported", err) } } diff --git a/internal/journal/read.go b/internal/journal/read.go index 48cd6b0..9099c0d 100644 --- a/internal/journal/read.go +++ b/internal/journal/read.go @@ -172,10 +172,12 @@ func Entries(path, runID string) ([]Entry, error) { // runFieldOf best-effort extracts a line's Run column even when the line // otherwise fails to parse, so Entries can tell whether an unparsable line -// belonged to the run it was asked for. +// belonged to the run it was asked for. The column counts only when a tab +// ends it: a line cut inside it holds a prefix of some run's ID, which names +// no run (plan 10 re-check R2). func runFieldOf(line string) (string, bool) { f := strings.SplitN(line, "\t", 3) - if len(f) < 2 { + if len(f) < 3 { return "", false } return unescape(f[1]), true diff --git a/internal/journal/read_test.go b/internal/journal/read_test.go index fb6f77f..676b2bd 100644 --- a/internal/journal/read_test.go +++ b/internal/journal/read_test.go @@ -702,3 +702,32 @@ func TestReversedStepsCountsEveryUndoOfARun(t *testing.T) { } } } + +// TestEntriesRefusesALineCutInsideItsRunColumn: a crash that cuts the last +// line inside its run column leaves a prefix of some run's ID - it cannot +// be called another run's line, so inside this run's window it refuses the +// run (plan 10 re-check R2). +func TestEntriesRefusesALineCutInsideItsRunColumn(t *testing.T) { + path := filepath.Join(t.TempDir(), "krino.log") + w, _ := Open(path) + at := time.Date(2026, 9, 11, 10, 2, 3, 0, time.UTC) + run := "20260911T100203-ab12" + for _, e := range []Entry{ + {Time: at, Run: run, Action: "run-start", Status: "ok"}, + {Time: at, Run: run, Dir: "dl", File: "x.pdf", Step: 1, Action: "copy", Status: "ok", Src: "/a/x.pdf", Dst: "/b/x.pdf"}, + } { + if err := w.Append(e); err != nil { + t.Fatal(err) + } + } + w.Close() + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0) + if err != nil { + t.Fatal(err) + } + f.WriteString("2026-09-11T10:02:04Z\t20260911T10") + f.Close() + if got, err := Entries(path, run); err == nil { + t.Errorf("Entries = %+v, no error; a line cut inside its run column must refuse the run", got) + } +} diff --git a/internal/plan/fuzz_test.go b/internal/plan/fuzz_test.go index b17d3c4..c7261ec 100644 --- a/internal/plan/fuzz_test.go +++ b/internal/plan/fuzz_test.go @@ -25,6 +25,11 @@ func FuzzExpand(f *testing.F) { for _, s := range []string{"{name}", "{stem}{ext}", "{mtime:%Y/%m}", "{1}_{2}", "{{literal}}", "{", "}", "{now:%", "{0}", "{9}", "{99999999999999999999}"} { f.Add(s, "a.b.pdf") } + // Seeds that try to leave the destination, so plain go test checks + // containment too: a ".." capture, a "~" name, one below a literal. + f.Add("{2}", "a.pdf") + f.Add("Out/{2}/{1}", "a.pdf") + f.Add("{name}/x", "~") f.Fuzz(func(t *testing.T, tmpl, name string) { facts := Facts{ Name: name, diff --git a/man/krino.1 b/man/krino.1 index ecbadc2..000fd95 100644 --- a/man/krino.1 +++ b/man/krino.1 @@ -434,6 +434,7 @@ Interrupted .Pq Ic Ctrl-C or terminated; the current step, if any, is finished and logged first, and the rest of that file's steps are skipped. +An undo finishes reversing the file it is on, then stops. .El .Sh KNOWN LIMITATIONS .Ic krino explain diff --git a/man/krino.conf.5 b/man/krino.conf.5 index d86267d..01d0f3d 100644 --- a/man/krino.conf.5 +++ b/man/krino.conf.5 @@ -281,6 +281,8 @@ the exclude holds and the file is set aside as with a warning. A file whose format has no text at all, such as an image or an archive, is not unreadable: its content tests are false, with no warning. +A file of no known extension that starts as text and holds binary data +further on is unreadable. .Sh RULES .Bd -literal -offset indent (rule NAME ITEM...) @@ -496,6 +498,8 @@ Groups overlap; a file can belong to several. .It Sy text Known text extensions, or detected from the first 8 KiB: valid UTF-8, or UTF-16 with a BOM, and no NUL bytes. +A file detected as text that holds a NUL or invalid UTF-8 further on cannot +be read. .It Sy pdf .Ic pdftotext .Pq 30 second timeout . |