diff options
| -rw-r--r-- | docs/design.md | 10 | ||||
| -rw-r--r-- | internal/cond/eval.go | 17 | ||||
| -rw-r--r-- | internal/cond/eval_test.go | 14 | ||||
| -rw-r--r-- | internal/engine/engine.go | 9 | ||||
| -rw-r--r-- | internal/engine/exclude_test.go | 53 | ||||
| -rw-r--r-- | internal/engine/match.go | 25 | ||||
| -rw-r--r-- | man/krino.conf.5 | 8 |
7 files changed, 126 insertions, 10 deletions
diff --git a/docs/design.md b/docs/design.md index bea0e2f..3f10623 100644 --- a/docs/design.md +++ b/docs/design.md @@ -199,6 +199,13 @@ as "excluded" in the plan, listed with the form that matched under `-v`, and traced by `explain`; `check` lists every directory's exclusions. Since no rule has run yet, `(matched)` is never true inside an exclude. +An exclude fails closed: when evaluating it reaches a content test that +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. 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. + Unlike `ignore`, which never looks inside a file and never descends into an ignored directory, an exclude can test content and size, and is evaluated per file after the walk. @@ -248,7 +255,8 @@ UTF-8 is replaced by U+FFFD before folding. `path`, then `duplicate`, then `content`. Content is extracted at most once per file, and only if evaluation reaches a `content` test. 4. If a file's text cannot be extracted, `content` is false and the plan - shows a warning naming the rule that wanted it. + shows a warning naming the rule that wanted it. Inside an `exclude` the + whole exclude then holds instead (§4.6). ### 5.5 Duplicates diff --git a/internal/cond/eval.go b/internal/cond/eval.go index 3cec110..b0ee828 100644 --- a/internal/cond/eval.go +++ b/internal/cond/eval.go @@ -33,6 +33,11 @@ type Result struct { Captures []string // submatches of the first true, non-negated name test: [0] whole match, [1:] groups Reasons []string // what made it true, e.g. `type pdf`, `content "acme ltd"`, `name "\bacme\b"` Warnings []string // e.g. `content unreadable: needs pdftotext, not installed` + + // Unreadable is true when evaluation reached a content test that could + // not read the file. The test counts as false; an exclude uses this to + // hold anyway (review M11). + Unreadable bool } // Trace is the full evaluation of every node, for krino explain. @@ -47,9 +52,10 @@ type Trace struct { // first true, non-negated name test, and warnings de-duplicated in // first-seen order. type evalCtx struct { - captures []string - warned map[string]bool - warnings []string + captures []string + warned map[string]bool + warnings []string + unreadable bool } // warn records msg unless it has already been recorded. @@ -75,7 +81,7 @@ func (c *Cond) Eval(f Facts) Result { } ctx := &evalCtx{} match, reasons := c.eval(c.root, f, ctx, false) - return Result{Match: match, Captures: ctx.captures, Reasons: reasons, Warnings: ctx.warnings} + return Result{Match: match, Captures: ctx.captures, Reasons: reasons, Warnings: ctx.warnings, Unreadable: ctx.unreadable} } // eval evaluates one node against f, short-circuiting and/or in child @@ -122,6 +128,9 @@ func (c *Cond) eval(n *node, f Facts, ctx *evalCtx, negated bool) (bool, []strin ok, reason, warn, caps := c.evalLeaf(n, f) if warn != "" { ctx.warn(warn) + if n.kind == kContent { + ctx.unreadable = true + } } if !ok { return false, nil diff --git a/internal/cond/eval_test.go b/internal/cond/eval_test.go index 8891de7..88612ac 100644 --- a/internal/cond/eval_test.go +++ b/internal/cond/eval_test.go @@ -226,3 +226,17 @@ func TestNegatedCombinatorReason(t *testing.T) { t.Errorf("negated leaf = %+v, want its own label unchanged: %q", r, "not matched") } } + +// TestEvalReportsUnreadableContent: Result.Unreadable says a content test +// was reached and could not read the file - what lets an exclude fail +// closed (review M11) - and stays false when evaluation never reached the +// content test. +func TestEvalReportsUnreadableContent(t *testing.T) { + f := &fake{name: "a.pdf", rawErr: errors.New("larger than max-read")} + if r := eval(t, `(and (type pdf) (content "x"))`, Options{}, f); r.Match || !r.Unreadable { + t.Errorf("reached: Match %v Unreadable %v; want false, true", r.Match, r.Unreadable) + } + if r := eval(t, `(or (name "^a") (content "x"))`, Options{}, f); !r.Match || r.Unreadable { + t.Errorf("not reached: Match %v Unreadable %v; want true, false", r.Match, r.Unreadable) + } +} diff --git a/internal/engine/engine.go b/internal/engine/engine.go index b50728e..7a83ccd 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -89,6 +89,15 @@ func Load(mainFile string, names ...string) (*Engine, []*config.Diag) { // A mistake inside a krino.conf exclude is compiled once per directory, // but reported once. reported := map[string]bool{} + if len(cfg.Dirs) == 0 { + // With no directory to compile them for, krino.conf's excludes are + // still checked, so a mistake is reported before one is included + // (review cli F10). + for _, x := range cfg.Main.Excludes { + _, cerrs := cond.Compile(cfg.Main.File, x.When, cond.Options{IgnoreCase: true, Fold: true}) + errs = append(errs, cerrs...) + } + } for _, d := range cfg.Dirs { dir := &Dir{ Name: d.Name, diff --git a/internal/engine/exclude_test.go b/internal/engine/exclude_test.go index 8b26450..87fa16c 100644 --- a/internal/engine/exclude_test.go +++ b/internal/engine/exclude_test.go @@ -207,3 +207,56 @@ func TestExplainReportsExclusionAndSize(t *testing.T) { t.Errorf("Skip = %q, want too big", big.Skip) } } + +// TestExcludeFailsClosedOnUnreadableContent: an exclude meant to protect +// files holds when its content test cannot read a file (over max-read), so +// no rule acts on a file krino could not check (review M11, Łukasz's +// decision). +func TestExcludeFailsClosedOnUnreadableContent(t *testing.T) { + big := "confidential " + strings.Repeat("x", 2048) + h, dl := excludeTree(t, map[string]string{"big.txt": big, "small.txt": "nothing to hide"}) + main := writeConfig(t, h, `(include "dl")`, map[string]string{"dl": ` +(path "~/dl") +(max-read 1K) +(exclude (type txt) (content "confidential")) +(rule "old" (delete)) +`}) + 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 { + switch fm.File.Rel { + case "big.txt": + if !strings.HasSuffix(fm.Excluded, "(content unreadable)") || len(fm.Rules) != 0 { + t.Errorf("big.txt: Excluded %q, rules %d; want set aside as unreadable", fm.Excluded, len(fm.Rules)) + } + case "small.txt": + if fm.Excluded != "" || len(fm.Rules) != 1 { + t.Errorf("small.txt: Excluded %q, rules %d; want the rule", fm.Excluded, len(fm.Rules)) + } + } + } + x, err := e.Explain(context.Background(), filepath.Join(dl, "big.txt")) + if err != nil { + t.Fatal(err) + } + if !strings.HasSuffix(x.Excluded, "(content unreadable)") { + t.Errorf("explain: Excluded %q", x.Excluded) + } +} + +// TestLoadChecksMainExcludesWithoutDirectories: a mistake in krino.conf's +// (exclude ...) is reported even before any directory is included (review +// cli F10). +func TestLoadChecksMainExcludesWithoutDirectories(t *testing.T) { + h := sandbox(t) + main := writeConfig(t, h, "(include)\n(exclude (bogus 1))\n", nil) + if _, errs := Load(main); len(errs) == 0 { + t.Error("a broken krino.conf exclude was not reported") + } +} diff --git a/internal/engine/match.go b/internal/engine/match.go index 08d75af..fedc053 100644 --- a/internal/engine/match.go +++ b/internal/engine/match.go @@ -146,8 +146,8 @@ func evalFile(run *matchRun, file scan.File) FileMatch { for _, w := range res.Warnings { fm.Warnings = append(fm.Warnings, "exclude: "+w) } - if res.Match { - fm.Excluded = x.Text + if excluded := excludedBy(x, res); excluded != "" { + fm.Excluded = excluded return fm } } @@ -274,9 +274,10 @@ func (e *Engine) Explain(ctx context.Context, path string) (*Explanation, error) excluded := "" for _, x := range d.Excludes { trace := x.Cond.Explain(f) - excludes = append(excludes, ExcludeTrace{Text: x.Text, Match: trace.Value, Trace: trace}) - if trace.Value && excluded == "" { - excluded = x.Text + by := excludedBy(x, x.Cond.Eval(f)) + excludes = append(excludes, ExcludeTrace{Text: x.Text, Match: by != "", Trace: trace}) + if by != "" && excluded == "" { + excluded = by } } @@ -309,6 +310,20 @@ func (e *Engine) cacheFingerprint(d *Dir) string { return fmt.Sprintf("%s %s %s max-read=%d", e.Extract.Fingerprint(), norm.Fingerprint(), runtime.Version(), d.Settings.MaxRead) } +// excludedBy is what x sets a file aside as, given its evaluation: its text +// when it matched, its text marked "(content unreadable)" when a content +// test it reached could not read the file - an exclude protects files, so +// it fails closed (review M11) - or "" when it does not hold. +func excludedBy(x *Exclude, res cond.Result) string { + switch { + case res.Match: + return x.Text + case res.Unreadable: + return x.Text + " (content unreadable)" + } + return "" +} + // cacheFile is d's keyword cache file. func (e *Engine) cacheFile(d *Dir) string { return filepath.Join(e.CacheDir, d.Name+".cache") diff --git a/man/krino.conf.5 b/man/krino.conf.5 index b00967a..81bbb28 100644 --- a/man/krino.conf.5 +++ b/man/krino.conf.5 @@ -268,6 +268,14 @@ Unlike which never opens a file, an .Ic exclude can test size and content. +.Pp +An +.Ic exclude +fails closed: when a content test it reaches cannot read the file +.Pq over Ic max-read , No or a tool missing, failing or timing out , +the exclude holds and the file is set aside as +.Dq (content unreadable) , +with a warning. .Sh RULES .Bd -literal -offset indent (rule NAME ITEM...) |