From f635b3a609cad45bd63cef006734210de899cf50 Mon Sep 17 00:00:00 2001 From: ManManavadaria Date: Wed, 6 May 2026 19:09:44 +0000 Subject: [PATCH] fix: protect nested watch-root traversal from cross-trigger ignores and apply review fixes Signed-off-by: ManManavadaria --- pkg/watch/watcher_darwin.go | 2 +- pkg/watch/watcher_naive.go | 39 ++++++++++++++++++------ pkg/watch/watcher_naive_test.go | 54 +++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index 9486b9af3..e4b6552eb 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -115,7 +115,7 @@ func (d *fseventNotify) Errors() chan error { return d.errors } -func newWatcher(paths []string, _ PathMatcher) (Notify, error) { +func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { dw := &fseventNotify{ stream: &fsevents.EventStream{ Latency: 50 * time.Millisecond, diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 135e058ce..0ccd1f33c 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -116,7 +116,7 @@ func (d *naiveNotify) watchRecursively(dir string) error { if err != nil { if os.IsPermission(err) && d.shouldIgnore(path) { logrus.Debugf("Ignoring path: %s", path) - return nil + return filepath.SkipDir } return err } @@ -185,6 +185,10 @@ func (d *naiveNotify) loop() { //nolint:gocyclo // TODO(dbentley): if there's a delete should we call d.watcher.Remove to prevent leaking? err := filepath.WalkDir(e.Name, func(path string, info fs.DirEntry, err error) error { if err != nil { + if os.IsPermission(err) && d.shouldIgnore(path) { + logrus.Debugf("Ignoring path: %s", path) + return filepath.SkipDir + } return err } @@ -245,11 +249,6 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return false } - // If path is present in the ignore list then we should ignore it - if d.shouldIgnore(path) { - return true - } - // Suppose we're watching // /src/.tiltignore // but the .tiltignore file doesn't exist. @@ -262,15 +261,28 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { // - A child of a directory that's in our notify list, or // - A parent of a directory that's in our notify list // (i.e., to cover the "path doesn't exist" case). + // + // We prioritize "parent of watched path" checks before ignore checks so + // one trigger's ignore rules can't hide another trigger's nested watch root. + isChildOfWatchedDir := false for root := range d.notifyList { - if pathutil.IsChild(root, path) || pathutil.IsChild(path, root) { + if pathutil.IsChild(path, root) { return false } + if pathutil.IsChild(root, path) { + isChildOfWatchedDir = true + } } - return true + + // skip the dir if ignore rules match this full subtree. + if d.shouldIgnoreEntireDir(path) { + return true + } + + return !isChildOfWatchedDir } -func (d *naiveNotify) shouldIgnore(path string) bool { +func (d *naiveNotify) shouldIgnoreEntireDir(path string) bool { if d.ignore == nil { return false } @@ -282,7 +294,14 @@ func (d *naiveNotify) shouldIgnore(path string) bool { if matches { return true } - matches, err = d.ignore.Matches(path) + return false +} + +func (d *naiveNotify) shouldIgnore(path string) bool { + if d.ignore == nil { + return false + } + matches, err := d.ignore.Matches(path) if err != nil { logrus.Debugf("error checking ignored path %q: %v", path, err) return false diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index b188de939..ede4c0d7a 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -157,3 +157,57 @@ func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) { t.Fatalf("watching more than 5 files: %d", n) } } + +func TestShouldSkipDirWithNegatedChildException(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: ignore, + notifyList: map[string]bool{repoRoot: true}, + } + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, !d.shouldSkipDir(bazelBin), "expected bazel-bin to remain traversable for negated child patterns") +} + +func TestShouldIgnorePathStillMatchesDirectoryPattern(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := &naiveNotify{ignore: ignore} + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, d.shouldIgnore(bazelBin), "expected bazel-bin path to match ignore pattern") +} + +func TestShouldSkipDirForIgnoredSubtreeWithoutException(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: ignore, + notifyList: map[string]bool{repoRoot: true}, + } + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, d.shouldSkipDir(bazelBin), "expected fully ignored directory subtree to be skipped") +} + +func TestShouldSkipDirDoesNotSkipAncestorOfWatchedPath(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "parent/\n") + assert.NilError(t, err) + + watchedPath := filepath.Join(repoRoot, "parent", "child", "non-existent") + d := &naiveNotify{ + ignore: ignore, + notifyList: map[string]bool{watchedPath: true}, + } + + parent := filepath.Join(repoRoot, "parent") + assert.Assert(t, !d.shouldSkipDir(parent), "expected parent directory to remain traversable when it contains a watched path") +}