diff --git a/ingestion/cmd/server/main.go b/ingestion/cmd/server/main.go index 81b28c1..f0fc95d 100644 --- a/ingestion/cmd/server/main.go +++ b/ingestion/cmd/server/main.go @@ -174,7 +174,8 @@ func alertingWarnings(getenv func(string) string) []string { var out []string if getenv("BRAIN_NTFY_URL") == "" { out = append(out, "alerting DISABLED: BRAIN_NTFY_URL unset — "+ - "brain_edit_root governance rewrites and capture audit degradation will not notify") + "brain_edit_root governance rewrites, capture audit degradation and the "+ + "brain symlink tripwire (hyperguild#157) will not notify") } return out } @@ -366,7 +367,7 @@ func main() { ctx := context.Background() // hyperguild#157: report a symlink that appears in the brain while running. - go symlinkguard.Watch(ctx, brainDir, time.Duration(envInt("BRAIN_SYMLINK_SCAN_INTERVAL", 300))*time.Second, buildNotifier()) + go symlinkguard.Watch(ctx, brainDir, symlinkScanInterval(os.Getenv), buildNotifier()) // Built before the watcher, the graph backfill and session ingestion, and // retried rather than fatal (#83). Two separate reasons: diff --git a/ingestion/cmd/server/symlink_check.go b/ingestion/cmd/server/symlink_check.go index 4439e1a..f1b987e 100644 --- a/ingestion/cmd/server/symlink_check.go +++ b/ingestion/cmd/server/symlink_check.go @@ -1,24 +1,39 @@ package main import ( + "errors" "fmt" - "strings" + "os" + "strconv" + "time" "github.com/mathiasbq/hyperguild/ingestion/internal/symlinkguard" ) // startupSymlinkCheck is the hyperguild#157 tripwire at boot: a symlink in the // brain dir would let brain readers serve a file from outside the brain, so -// the server does not start until it is removed. A scan that cannot run (e.g. -// a missing brain dir on first boot) is not treated as an incident. +// the server does not start until it is removed. A directory the scan cannot +// read could hide one, so it refuses on that too. A missing brain dir (first +// boot) is the only case it lets pass. func startupSymlinkCheck(brainDir string) error { - found, err := symlinkguard.Scan(brainDir) + r, err := symlinkguard.Scan(brainDir) if err != nil { - return nil + if errors.Is(err, os.ErrNotExist) { + return nil + } + return fmt.Errorf("refusing to start: cannot scan the brain dir %s (hyperguild#157): %w", brainDir, err) } - if len(found) > 0 { - return fmt.Errorf("refusing to start: %d symlink(s) in the brain dir %s (hyperguild#157); remove them: %s", - len(found), brainDir, strings.Join(found, ", ")) + if !r.Clean() { + return fmt.Errorf("refusing to start: brain dir %s has %s (hyperguild#157); fix them first", brainDir, r) } return nil } + +// symlinkScanInterval reads BRAIN_SYMLINK_SCAN_INTERVAL (seconds), falling +// back to the default on an unset, non-numeric or non-positive value. +func symlinkScanInterval(getenv func(string) string) time.Duration { + if s, err := strconv.Atoi(getenv("BRAIN_SYMLINK_SCAN_INTERVAL")); err == nil && s > 0 { + return time.Duration(s) * time.Second + } + return symlinkguard.DefaultInterval +} diff --git a/ingestion/cmd/server/symlink_check_test.go b/ingestion/cmd/server/symlink_check_test.go index e8efc03..0efcb5e 100644 --- a/ingestion/cmd/server/symlink_check_test.go +++ b/ingestion/cmd/server/symlink_check_test.go @@ -3,6 +3,7 @@ package main import ( "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -10,7 +11,8 @@ import ( ) // hyperguild#157: the server refuses to start while a symlink exists in the -// brain dir (Mathias's chosen tripwire), naming it; a clean brain starts. +// brain dir, or while any part of it cannot be scanned (which could hide +// one); a missing brain dir is the only thing it lets pass. func TestStartupSymlinkCheckRefusesABrainWithASymlink(t *testing.T) { brain := t.TempDir() require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brain, "planted"))) @@ -19,8 +21,35 @@ func TestStartupSymlinkCheckRefusesABrainWithASymlink(t *testing.T) { assert.Contains(t, err.Error(), "planted") } -func TestStartupSymlinkCheckPassesACleanBrain(t *testing.T) { +func TestStartupSymlinkCheckRefusesABrainWithAnUnreadableDir(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads mode-000 directories") + } + brain := t.TempDir() + locked := filepath.Join(brain, "aaa") + require.NoError(t, os.MkdirAll(locked, 0o755)) + require.NoError(t, os.Chmod(locked, 0o000)) + t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) + err := startupSymlinkCheck(brain) + require.Error(t, err) + assert.Contains(t, err.Error(), "aaa") +} + +func TestStartupSymlinkCheckPassesACleanOrMissingBrain(t *testing.T) { brain := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(brain, "n.md"), []byte("x"), 0o644)) assert.NoError(t, startupSymlinkCheck(brain)) + assert.NoError(t, startupSymlinkCheck(filepath.Join(brain, "missing"))) +} + +func TestSymlinkScanIntervalFallsBackOnNonPositive(t *testing.T) { + for _, v := range []string{"0", "-5", "", "junk"} { + got := symlinkScanInterval(func(string) string { return v }) + assert.Positive(t, got, "env %q", v) + } +} + +func TestAlertingWarningsNamesTheSymlinkTripwire(t *testing.T) { + w := strings.Join(alertingWarnings(func(string) string { return "" }), " ") + assert.Contains(t, w, "symlink tripwire") } diff --git a/ingestion/internal/symlinkguard/symlinkguard.go b/ingestion/internal/symlinkguard/symlinkguard.go index ecf68ab..e3fa3b2 100644 --- a/ingestion/internal/symlinkguard/symlinkguard.go +++ b/ingestion/internal/symlinkguard/symlinkguard.go @@ -2,9 +2,9 @@ // a Syncthing sendreceive folder and Syncthing syncs symlinks; several brain // readers (search, vectorstore, context, graph) follow a symlink out of the // brain and would index or return whatever it points to. Until every reader -// reads through an os.Root, any symlink inside the brain is treated as an -// incident: the server refuses to start on one (main), and Watch reports one -// that appears while it runs. +// reads through an os.Root, a symlink inside the brain, or a directory the +// scan cannot read (which could hide one), is an incident: the server refuses +// to start on one (main), and Watch reports one that appears while it runs. package symlinkguard import ( @@ -15,77 +15,131 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" "time" ) +// DefaultInterval is used when Watch is given a non-positive interval. +const DefaultInterval = 5 * time.Minute + // Notifier is the out-of-band alert sink (audit.Notifier satisfies it). type Notifier interface { Notify(ctx context.Context, msg string) error } -// Scan returns the brain-relative paths of every symlink inside brainDir, -// skipping .git. brainDir itself may be a symlink (a volume mount); it is -// resolved first, and only entries inside it are checked. WalkDir never -// descends a symlinked directory, so each link is reported, not followed. -func Scan(brainDir string) ([]string, error) { +// Result is what a scan found: symlinks, and directories it could not read +// (each of which could hide a symlink). Paths are brain-relative. +type Result struct { + Symlinks []string + Unreadable []string +} + +// Clean reports whether the scan found nothing to act on. +func (r Result) Clean() bool { return len(r.Symlinks) == 0 && len(r.Unreadable) == 0 } + +// String describes the findings for a log line or alert. Paths are quoted, so +// a name containing a newline cannot blur two findings together. +func (r Result) String() string { + q := func(xs []string) string { + out := make([]string, len(xs)) + for i, x := range xs { + out[i] = strconv.Quote(x) + } + return strings.Join(out, ", ") + } + var parts []string + if len(r.Symlinks) > 0 { + parts = append(parts, fmt.Sprintf("%d symlink(s): %s", len(r.Symlinks), q(r.Symlinks))) + } + if len(r.Unreadable) > 0 { + parts = append(parts, fmt.Sprintf("%d unreadable dir(s): %s", len(r.Unreadable), q(r.Unreadable))) + } + return strings.Join(parts, "; ") +} + +// Scan walks the brain dir and reports every symlink in it and every +// directory it could not read, continuing past both. Only the ROOT .git is +// skipped (it is brain-sync's, and no reader walks it); a nested .git is +// scanned like any other directory. brainDir itself may be a symlink (a +// volume mount); it is resolved first. The error is non-nil only when the +// root cannot be resolved, wrapping os.ErrNotExist when it is missing. +func Scan(brainDir string) (Result, error) { root, err := filepath.EvalSymlinks(brainDir) if err != nil { - return nil, fmt.Errorf("resolve brain dir: %w", err) + return Result{}, fmt.Errorf("resolve brain dir: %w", err) } - var found []string - err = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + var r Result + rootGit := filepath.Join(root, ".git") + _ = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + rel, _ := filepath.Rel(root, p) + rel = filepath.ToSlash(rel) if err != nil { - return err + if p == root { + return err + } + r.Unreadable = append(r.Unreadable, rel) + if d != nil && d.IsDir() { + return filepath.SkipDir + } + return nil } - if d.IsDir() && d.Name() == ".git" { + if d.IsDir() && p == rootGit { return filepath.SkipDir } if d.Type()&os.ModeSymlink != 0 { - rel, _ := filepath.Rel(root, p) - found = append(found, filepath.ToSlash(rel)) + r.Symlinks = append(r.Symlinks, rel) } return nil }) - if err != nil { - return nil, fmt.Errorf("scan brain dir: %w", err) - } - sort.Strings(found) - return found, nil + sort.Strings(r.Symlinks) + sort.Strings(r.Unreadable) + return r, nil } -// Watch rescans every interval and notifies once per distinct set of -// symlinks found, until ctx ends. A scan error is logged, not fatal. +// Watch rescans every interval (DefaultInterval if non-positive) until ctx +// ends, and notifies once per distinct non-clean result. A result counts as +// reported only after Notify succeeds, so an alert lost to a notifier outage +// is retried on the next tick. func Watch(ctx context.Context, brainDir string, interval time.Duration, n Notifier) { + if interval <= 0 { + interval = DefaultInterval + } t := time.NewTicker(interval) defer t.Stop() - last := "" + reported := "" for { select { case <-ctx.Done(): return case <-t.C: } - found, err := Scan(brainDir) - if err != nil { - slog.Warn("symlink tripwire scan failed", "err", err) + r, err := Scan(brainDir) + var key, msg string + switch { + case err != nil: + key = "error: " + err.Error() + msg = "brain symlink tripwire (hyperguild#157): cannot scan the brain dir: " + err.Error() + case r.Clean(): + reported = "" + continue + default: + key = r.String() + msg = "brain symlink tripwire (hyperguild#157): " + key + + ". Readers may follow symlinks out of the brain; remove them." + } + if key == reported { continue } - key := strings.Join(found, "\n") - if key == last { + slog.Error("brain symlink tripwire", "finding", key) + if n == nil { + reported = key continue } - last = key - if len(found) == 0 { + if err := n.Notify(ctx, msg); err != nil { + slog.Warn("symlink tripwire notify failed; will retry", "err", err) continue } - msg := fmt.Sprintf("brain symlink tripwire (hyperguild#157): %d symlink(s) in the brain dir; "+ - "readers may follow them out of the brain. Remove them: %s", len(found), strings.Join(found, ", ")) - slog.Error("brain symlink tripwire", "count", len(found), "paths", found) - if n != nil { - if err := n.Notify(ctx, msg); err != nil { - slog.Warn("symlink tripwire notify failed", "err", err) - } - } + reported = key } } diff --git a/ingestion/internal/symlinkguard/symlinkguard_test.go b/ingestion/internal/symlinkguard/symlinkguard_test.go index d9405b4..4dbbf67 100644 --- a/ingestion/internal/symlinkguard/symlinkguard_test.go +++ b/ingestion/internal/symlinkguard/symlinkguard_test.go @@ -2,6 +2,7 @@ package symlinkguard import ( "context" + "errors" "os" "path/filepath" "sync" @@ -13,63 +14,84 @@ import ( ) // hyperguild#157: the brain dir is a Syncthing sendreceive folder and -// Syncthing syncs symlinks; search, vectorstore, context and graph readers -// follow them out of the brain. Until every reader goes through an os.Root, -// a symlink anywhere in the brain is a tripwire: the server refuses to start -// on one, and reports one that appears while it runs. +// Syncthing syncs symlinks; several readers follow them out of the brain. +// Until every reader goes through an os.Root, a symlink anywhere in the brain, +// or a directory the scan cannot read (which could hide one), is an incident. -func TestScanFindsSymlinksAtAnyDepthAndSkipsGit(t *testing.T) { +func TestScanReportsSymlinksAtEveryDepthIncludingNestedGit(t *testing.T) { brain := t.TempDir() outside := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w", "facts"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w", "facts", ".git"), 0o755)) require.NoError(t, os.MkdirAll(filepath.Join(brain, ".git", "objects"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(brain, "wiki", "w", "facts", "ok.md"), []byte("x"), 0o644)) require.NoError(t, os.Symlink(filepath.Join(outside, "f"), filepath.Join(brain, "wiki", "w", "facts", "leak.md"))) require.NoError(t, os.Symlink(outside, filepath.Join(brain, "wiki", "dirlink"))) + // PR #159 review: only the ROOT .git is skipped; a nested .git is not + // special to the brain readers, which walk into it. + require.NoError(t, os.Symlink(filepath.Join(outside, "g"), filepath.Join(brain, "wiki", "w", "facts", ".git", "leak.md"))) require.NoError(t, os.Symlink(outside, filepath.Join(brain, ".git", "objects", "alt"))) - found, err := Scan(brain) + r, err := Scan(brain) require.NoError(t, err) - assert.ElementsMatch(t, []string{"wiki/w/facts/leak.md", "wiki/dirlink"}, found) + assert.ElementsMatch(t, []string{"wiki/w/facts/leak.md", "wiki/dirlink", "wiki/w/facts/.git/leak.md"}, r.Symlinks) + assert.Empty(t, r.Unreadable) } -func TestScanOfACleanBrainFindsNothing(t *testing.T) { +// PR #159 review: WalkDir stopped at the first unreadable directory and the +// startup check passed, while search (which keeps walking) served a planted +// symlink. Scan now records the unreadable dir and keeps going. +func TestScanReportsUnreadableDirsAndKeepsWalkingPastThem(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads mode-000 directories") + } brain := t.TempDir() - require.NoError(t, os.MkdirAll(filepath.Join(brain, "raw"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(brain, "raw", "n.md"), []byte("x"), 0o644)) - found, err := Scan(brain) + locked := filepath.Join(brain, "aaa") + require.NoError(t, os.MkdirAll(locked, 0o755)) + require.NoError(t, os.Chmod(locked, 0o000)) + t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brain, "zzz-planted"))) + + r, err := Scan(brain) require.NoError(t, err) - assert.Empty(t, found) + assert.Equal(t, []string{"aaa"}, r.Unreadable) + assert.Equal(t, []string{"zzz-planted"}, r.Symlinks, "the walk continues past the unreadable dir") } -// The brain dir itself being a symlink is fine (the volume mount may be one); -// only entries inside it are checked. -func TestScanFollowsTheRootButNotEntries(t *testing.T) { +func TestScanResolvesASymlinkedRootAndReportsSymlinkedEntriesInsideIt(t *testing.T) { real := t.TempDir() link := filepath.Join(t.TempDir(), "brain") require.NoError(t, os.Symlink(real, link)) require.NoError(t, os.WriteFile(filepath.Join(real, "n.md"), []byte("x"), 0o644)) - found, err := Scan(link) + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(real, "inner"))) + r, err := Scan(link) require.NoError(t, err) - assert.Empty(t, found) + assert.Equal(t, []string{"inner"}, r.Symlinks) +} + +func TestScanOfAMissingRootIsErrNotExist(t *testing.T) { + _, err := Scan(filepath.Join(t.TempDir(), "nope")) + assert.True(t, errors.Is(err, os.ErrNotExist), "got %v", err) } type recNotifier struct { mu sync.Mutex msgs []string + fail int // fail the first N calls } func (r *recNotifier) Notify(_ context.Context, m string) error { r.mu.Lock() defer r.mu.Unlock() r.msgs = append(r.msgs, m) + if r.fail > 0 { + r.fail-- + return errors.New("ntfy down") + } return nil } func (r *recNotifier) count() int { r.mu.Lock(); defer r.mu.Unlock(); return len(r.msgs) } -// Watch reports a symlink that appears while running, once per distinct set. -func TestWatchNotifiesOncePerNewSymlinkSet(t *testing.T) { +func TestWatchNotifiesOncePerDistinctSetAndAgainForANewSet(t *testing.T) { brain := t.TempDir() n := &recNotifier{} ctx, cancel := context.WithCancel(context.Background()) @@ -78,10 +100,56 @@ func TestWatchNotifiesOncePerNewSymlinkSet(t *testing.T) { time.Sleep(50 * time.Millisecond) assert.Zero(t, n.count(), "a clean brain raises nothing") - - require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brain, "planted"))) + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brain, "first"))) require.Eventually(t, func() bool { return n.count() == 1 }, 2*time.Second, 10*time.Millisecond) time.Sleep(60 * time.Millisecond) - assert.Equal(t, 1, n.count(), "the same symlink set is reported once, not every tick") - assert.Contains(t, n.msgs[0], "planted") + assert.Equal(t, 1, n.count(), "the same set is reported once, not every tick") + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brain, "second"))) + require.Eventually(t, func() bool { return n.count() == 2 }, 2*time.Second, 10*time.Millisecond) +} + +// PR #159 review: last was recorded before Notify, so an alert lost to an +// ntfy outage was never retried while the symlink stayed. +func TestWatchRetriesAnAlertThatFailedToSend(t *testing.T) { + brain := t.TempDir() + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(brain, "planted"))) + n := &recNotifier{fail: 2} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go Watch(ctx, brain, 10*time.Millisecond, n) + require.Eventually(t, func() bool { return n.count() == 3 }, 2*time.Second, 10*time.Millisecond) + time.Sleep(60 * time.Millisecond) + assert.Equal(t, 3, n.count(), "after one successful send, no more") +} + +func TestWatchAlertsOnAnUnreadableDir(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads mode-000 directories") + } + brain := t.TempDir() + locked := filepath.Join(brain, "locked") + require.NoError(t, os.MkdirAll(locked, 0o755)) + require.NoError(t, os.Chmod(locked, 0o000)) + t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) + n := &recNotifier{} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go Watch(ctx, brain, 10*time.Millisecond, n) + require.Eventually(t, func() bool { return n.count() == 1 }, 2*time.Second, 10*time.Millisecond) + assert.Contains(t, n.msgs[0], "locked") +} + +// PR #159 review: a non-positive interval panicked time.NewTicker inside the +// goroutine and crashlooped the server. +func TestWatchToleratesANonPositiveInterval(t *testing.T) { + brain := t.TempDir() + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { Watch(ctx, brain, 0, nil); close(done) }() + cancel() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Watch did not return on cancel") + } }