From 5b03f97069bd59e0b0892483bb3fc4a1bcb52e66 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 11 Nov 2026 08:40:47 +0000 Subject: [PATCH] Refuse an exclude pattern that does not compile at config load (closes #455) An exclude pattern that did not compile was skipped with a warning during the backup, which then stored what the user meant to leave out. Config.Validate now refuses it, naming the setting and quoting the pattern, for example exclude[2]: "build[" is not a valid exclude pattern: ... The compile step moves to a new package, internal/exclude, as exclude.Compile; Validate and the scanner both call it. The scanner's skip-and-warn path is gone; a pattern that does not compile there now fails the scanner. Model: opus-5-5 --- internal/config/config.go | 32 ++++++++++--- internal/config/exclude.go | 39 ++++++++++++++++ internal/config/exclude_test.go | 65 +++++++++++++++++++++++++++ internal/exclude/exclude.go | 77 ++++++++++++++++++++++++++++++++ internal/exclude/exclude_test.go | 73 ++++++++++++++++++++++++++++++ internal/snapshot/scanner.go | 77 +++++++------------------------- 6 files changed, 294 insertions(+), 69 deletions(-) create mode 100644 internal/config/exclude.go create mode 100644 internal/config/exclude_test.go create mode 100644 internal/exclude/exclude.go create mode 100644 internal/exclude/exclude_test.go diff --git a/internal/config/config.go b/internal/config/config.go index 4f97544..ba8b259 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -396,6 +396,8 @@ func Parse(data []byte) (*Config, error) { // never the value. An empty list is accepted, because only snapshot // create needs a recipient and it checks for one itself // - At least one snapshot must be configured with at least one path +// - Every exclude pattern, global or in a snapshot, must compile; errors +// name the setting and quote the pattern // - Storage must be configured (either storage_url or s3.* fields) // - The index path must be absolute: a relative one would resolve against // the working directory, so each directory would get its own index @@ -416,18 +418,18 @@ func (c *Config) Validate() error { } } - if len(c.Snapshots) == 0 { - return errNoSnapshots + err := c.validateSnapshots() + if err != nil { + return err } - for name, snap := range c.Snapshots { - if len(snap.Paths) == 0 { - return fmt.Errorf("%w: %q", errSnapshotNoPaths, name) - } + err = c.validateExcludes() + if err != nil { + return err } // Validate storage configuration - err := c.validateStorage() + err = c.validateStorage() if err != nil { return err } @@ -466,6 +468,22 @@ func (c *Config) Validate() error { return nil } +// validateSnapshots refuses a config with no snapshot or with a snapshot +// that has no path. +func (c *Config) validateSnapshots() error { + if len(c.Snapshots) == 0 { + return errNoSnapshots + } + + for name, snap := range c.Snapshots { + if len(snap.Paths) == 0 { + return fmt.Errorf("%w: %q", errSnapshotNoPaths, name) + } + } + + return nil +} + // validateAgeRecipient parses one age_recipients entry with the age library // and returns a value-free error on failure. A recipient string can be // sensitive (an operator may paste a secret key by mistake), so neither the diff --git a/internal/config/exclude.go b/internal/config/exclude.go new file mode 100644 index 0000000..d2271b3 --- /dev/null +++ b/internal/config/exclude.go @@ -0,0 +1,39 @@ +package config + +import ( + "fmt" + + "sneak.berlin/go/vaultik/internal/exclude" +) + +// validateExcludes refuses an exclude pattern, global or in a snapshot, +// that does not compile, naming the setting and quoting the pattern. +func (c *Config) validateExcludes() error { + err := validateExcludeList("exclude", c.Exclude) + if err != nil { + return err + } + + for _, name := range c.SnapshotNames() { + err = validateExcludeList("snapshots."+name+".exclude", + c.Snapshots[name].Exclude) + if err != nil { + return err + } + } + + return nil +} + +// validateExcludeList refuses the first pattern in patterns that does not +// compile. setting is the name of the list, as in "exclude". +func validateExcludeList(setting string, patterns []string) error { + for i, pattern := range patterns { + _, _, err := exclude.Compile(pattern) + if err != nil { + return fmt.Errorf("%s[%d]: %w", setting, i, err) + } + } + + return nil +} diff --git a/internal/config/exclude_test.go b/internal/config/exclude_test.go new file mode 100644 index 0000000..8a3e0d4 --- /dev/null +++ b/internal/config/exclude_test.go @@ -0,0 +1,65 @@ +package config_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "sneak.berlin/go/vaultik/internal/config" +) + +// TestLoadRefusesBadExcludePattern checks that an exclude pattern that does +// not compile, global or in a snapshot, fails at load with the setting and +// the pattern named, rather than the backup storing what it meant to leave +// out. +func TestLoadRefusesBadExcludePattern(t *testing.T) { + t.Parallel() + + const storage = "storage_url: file:///tmp/vaultik-test-storage\n" + + tests := []struct { + name string + config string + wantErr string + }{ + { + name: "global", + config: "exclude: ['*.tmp', '.cache', 'build[']\n" + + "snapshots:\n" + + " home:\n" + + " paths: [/tmp/vaultik-test-source]\n" + storage, + wantErr: `exclude[2]: "build[" is not a valid exclude pattern`, + }, + { + name: "snapshot", + config: "snapshots:\n" + + " home:\n" + + " paths: [/tmp/vaultik-test-source]\n" + + " exclude: ['/a/**/x[']\n" + storage, + wantErr: `snapshots.home.exclude[0]: "/a/**/x[" is not a valid exclude pattern`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "config.yml") + + err := os.WriteFile(path, []byte(tt.config), 0o600) + if err != nil { + t.Fatalf("write config: %v", err) + } + + _, err = config.Load(path) + if err == nil { + t.Fatalf("Load() returned nil, want an error containing %q", tt.wantErr) + } + + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("Load() error = %q, want it to contain %q", err, tt.wantErr) + } + }) + } +} diff --git a/internal/exclude/exclude.go b/internal/exclude/exclude.go new file mode 100644 index 0000000..e386cc9 --- /dev/null +++ b/internal/exclude/exclude.go @@ -0,0 +1,77 @@ +// Package exclude compiles exclude patterns into globs. +package exclude + +import ( + "errors" + "fmt" + "strings" + + "github.com/gobwas/glob" +) + +var errBadExcludePattern = errors.New("is not a valid exclude pattern") + +// Compile compiles one exclude pattern into the globs that match it. +// anchored reports whether the pattern matches only from the root of the +// source directory. An empty pattern yields no globs. Config.Validate and +// the scanner both compile patterns with it, so a pattern that loads is one +// the scanner can use. +func Compile(pattern string) ([]glob.Glob, bool, error) { + if pattern == "" { + return nil, false, nil + } + + // A leading "/" anchors the pattern to the source directory. + trimmed, anchored := strings.CutPrefix(pattern, "/") + + // A trailing "/" marks a directory and is not matched. + trimmed = strings.TrimSuffix(trimmed, "/") + + // A leading "**/" demands a "/" before the name and so misses an + // entry directly under the source path. It means "at every depth", + // which is what an unanchored pattern already does, so drop the + // prefix and match unanchored: as in gitignore, "**/x" and + // "/**/x" then also match a top-level "x". + if rest, found := strings.CutPrefix(trimmed, "**/"); found { + trimmed = rest + anchored = false + } + + // Likewise "/**/" inside a pattern demands two slashes, so + // "a/**/*.log" misses "a/x.log". As in gitignore it stands for + // zero or more directories, so also compile the pattern with it + // made a single "/". The glob library's {a,b} is no help: with a + // "**" inside it in mid-pattern, it misses paths it should match. + var globs []glob.Glob + + for _, variant := range doubleStarVariants(trimmed) { + g, err := glob.Compile(variant, '/') + if err != nil { + return nil, false, fmt.Errorf("%q %w: %w", + pattern, errBadExcludePattern, err) + } + + globs = append(globs, g) + } + + return globs, anchored, nil +} + +// doubleStarVariants returns pattern once for every way of keeping each +// "/**/" in it or making it a single "/". +func doubleStarVariants(pattern string) []string { + const doubleStar = "/**/" + + before, after, found := strings.Cut(pattern, doubleStar) + if !found { + return []string{pattern} + } + + var variants []string + + for _, rest := range doubleStarVariants(after) { + variants = append(variants, before+doubleStar+rest, before+"/"+rest) + } + + return variants +} diff --git a/internal/exclude/exclude_test.go b/internal/exclude/exclude_test.go new file mode 100644 index 0000000..a49b657 --- /dev/null +++ b/internal/exclude/exclude_test.go @@ -0,0 +1,73 @@ +package exclude_test + +import ( + "slices" + "strings" + "testing" + + "github.com/gobwas/glob" + "sneak.berlin/go/vaultik/internal/exclude" +) + +// TestCompile checks the error, the anchoring and the globs that Compile +// returns for one exclude pattern. +func TestCompile(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + pattern string + wantErr string + wantAnchored bool + // wantMatches lists paths that one of the globs must match. When + // it is empty, Compile must return no globs. + wantMatches []string + }{ + {name: "does not compile", pattern: "build[", + wantErr: `"build[" is not a valid exclude pattern`}, + {name: "empty", pattern: ""}, + {name: "leading slash anchors", pattern: "/build", + wantAnchored: true, wantMatches: []string{"build"}}, + {name: "double star after the slash clears anchoring", + pattern: "/**/build", wantMatches: []string{"build"}}, + {name: "double star in the middle", pattern: "a/**/*.log", + wantMatches: []string{"a/x.log", "a/b/x.log"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + globs, anchored, err := exclude.Compile(tt.pattern) + if tt.wantErr == "" && err != nil { + t.Fatalf("Compile(%q) error = %v, want none", tt.pattern, err) + } + + if tt.wantErr != "" && + (err == nil || !strings.HasPrefix(err.Error(), tt.wantErr)) { + t.Fatalf("Compile(%q) error = %v, want one starting with %q", + tt.pattern, err, tt.wantErr) + } + + if anchored != tt.wantAnchored { + t.Errorf("Compile(%q) anchored = %t, want %t", + tt.pattern, anchored, tt.wantAnchored) + } + + if len(tt.wantMatches) == 0 && len(globs) != 0 { + t.Errorf("Compile(%q) returned %d globs, want none", + tt.pattern, len(globs)) + } + + for _, path := range tt.wantMatches { + matched := slices.ContainsFunc(globs, func(g glob.Glob) bool { + return g.Match(path) + }) + if !matched { + t.Errorf("Compile(%q) globs do not match %q", + tt.pattern, path) + } + } + }) + } +} diff --git a/internal/snapshot/scanner.go b/internal/snapshot/scanner.go index 9bd36b7..7cc6ea3 100644 --- a/internal/snapshot/scanner.go +++ b/internal/snapshot/scanner.go @@ -20,6 +20,7 @@ import ( "sneak.berlin/go/vaultik/internal/blob" "sneak.berlin/go/vaultik/internal/chunker" "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/exclude" "sneak.berlin/go/vaultik/internal/log" "sneak.berlin/go/vaultik/internal/storage" "sneak.berlin/go/vaultik/internal/types" @@ -221,8 +222,12 @@ func NewScanner(cfg ScannerConfig) *Scanner { progress = NewProgressReporter() } - // Compile exclude patterns - compiledExclude := compileExcludePatterns(cfg.Exclude) + compiledExclude, err := compileExcludePatterns(cfg.Exclude) + if err != nil { + log.Error("Failed to compile exclude patterns", "error", err) + + return nil + } uiw := cfg.UI if uiw == nil { @@ -2318,51 +2323,18 @@ func wrapPermissionError(path string, err error) error { "with --skip-errors to continue past unreadable files)", path, err) } -// compileExcludePatterns compiles the exclude patterns into glob matchers -func compileExcludePatterns(patterns []string) []compiledPattern { +// compileExcludePatterns compiles the exclude patterns into glob matchers. +// Config.Validate has already refused a pattern that does not compile. +func compileExcludePatterns(patterns []string) ([]compiledPattern, error) { var compiled []compiledPattern for _, p := range patterns { - if p == "" { - continue + globs, anchored, err := exclude.Compile(p) + if err != nil { + return nil, err } - // Check if pattern is anchored (starts with /) - anchored := strings.HasPrefix(p, "/") - - pattern := p - if anchored { - pattern = p[1:] // Remove leading / - } - - // Remove trailing slash if present (directory indicator) - pattern = strings.TrimSuffix(pattern, "/") - - // A leading "**/" demands a "/" before the name and so misses an - // entry directly under the source path. It means "at every depth", - // which is what an unanchored pattern already does, so drop the - // prefix and match unanchored: as in gitignore, "**/x" and - // "/**/x" then also match a top-level "x". - if rest, found := strings.CutPrefix(pattern, "**/"); found { - pattern = rest - anchored = false - } - - // Likewise "/**/" inside a pattern demands two slashes, so - // "a/**/*.log" misses "a/x.log". As in gitignore it stands for - // zero or more directories, so also compile the pattern with it - // made a single "/". The glob library's {a,b} is no help: with a - // "**" inside it in mid-pattern, it misses paths it should match. - for _, variant := range doubleStarVariants(pattern) { - // For patterns without path separators, we need to match them as components - // e.g., ".git" should match ".git" anywhere in the path - g, err := glob.Compile(variant, '/') - if err != nil { - log.Warn("Invalid exclude pattern, skipping", "pattern", p, "error", err) - - break - } - + for _, g := range globs { compiled = append(compiled, compiledPattern{ pattern: g, anchored: anchored, @@ -2371,26 +2343,7 @@ func compileExcludePatterns(patterns []string) []compiledPattern { } } - return compiled -} - -// doubleStarVariants returns pattern once for every way of keeping each -// "/**/" in it or making it a single "/". -func doubleStarVariants(pattern string) []string { - const doubleStar = "/**/" - - before, after, found := strings.Cut(pattern, doubleStar) - if !found { - return []string{pattern} - } - - var variants []string - - for _, rest := range doubleStarVariants(after) { - variants = append(variants, before+doubleStar+rest, before+"/"+rest) - } - - return variants + return compiled, nil } // shouldExclude checks if a path should be excluded based on exclude patterns -- 2.54.0