diff --git a/ingestion/internal/symlinkguard/special_test.go b/ingestion/internal/symlinkguard/special_test.go index e16c61a..f592ff4 100644 --- a/ingestion/internal/symlinkguard/special_test.go +++ b/ingestion/internal/symlinkguard/special_test.go @@ -34,9 +34,9 @@ func TestScanReportsFIFOsAndSocketsAsSpecialFiles(t *testing.T) { assert.Contains(t, r.String(), "special file") } -// Watch removes special files as it removes symlinks, and leaves regular -// files alone. -func TestWatchRemovesSpecialFilesButNotRegularFiles(t *testing.T) { +// Watch removes a FIFO as it removes symlinks, and leaves a regular file +// (which Scan never reports) alone. +func TestWatchRemovesAFIFOAndLeavesAnUnreportedRegularFile(t *testing.T) { brain := t.TempDir() fifo := filepath.Join(brain, "f.md") require.NoError(t, syscall.Mkfifo(fifo, 0o644)) @@ -59,8 +59,10 @@ func TestWatchRemovesSpecialFilesButNotRegularFiles(t *testing.T) { assert.Contains(t, n.msgs[0], "f.md") } -// removeSpecial re-checks the type: a path that is now a regular file or a -// directory is not removed. +// removeSpecial re-checks the type: a path that is a regular file or a +// directory at the re-check is not removed. (A swap between the re-check and +// the unlink can still remove one; Linux has no conditional unlink. The same +// window exists in removeSymlinks.) func TestRemoveSpecialRefusesRegularFilesAndDirectories(t *testing.T) { brain := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(brain, "n.md"), []byte("n"), 0o644)) @@ -72,3 +74,28 @@ func TestRemoveSpecialRefusesRegularFilesAndDirectories(t *testing.T) { _, err := os.Stat(filepath.Join(brain, "n.md")) assert.NoError(t, err) } + +// A special file Watch cannot remove (its directory is read-only) stays +// reported: the persistent-finding alert names it. +func TestWatchReportsASpecialFileItCannotRemove(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions") + } + brain := t.TempDir() + ro := filepath.Join(brain, "ro") + require.NoError(t, os.MkdirAll(ro, 0o755)) + require.NoError(t, syscall.Mkfifo(filepath.Join(ro, "f.md"), 0o644)) + require.NoError(t, os.Chmod(ro, 0o555)) + t.Cleanup(func() { _ = os.Chmod(ro, 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) + _, err := os.Lstat(filepath.Join(ro, "f.md")) + require.NoError(t, err, "the FIFO could not be removed") + n.mu.Lock() + defer n.mu.Unlock() + assert.Contains(t, n.msgs[0], "ro/f.md") + assert.Contains(t, n.msgs[0], "special file") +} diff --git a/ingestion/internal/symlinkguard/symlinkguard.go b/ingestion/internal/symlinkguard/symlinkguard.go index f848264..5086b8a 100644 --- a/ingestion/internal/symlinkguard/symlinkguard.go +++ b/ingestion/internal/symlinkguard/symlinkguard.go @@ -66,7 +66,7 @@ func (r Result) String() string { parts = append(parts, fmt.Sprintf("%d symlink(s): %s", len(r.Symlinks), q(r.Symlinks))) } if len(r.Special) > 0 { - parts = append(parts, fmt.Sprintf("%d special file(s): %s", len(r.Special), q(r.Special))) + parts = append(parts, fmt.Sprintf("%d special file(s) (FIFO, socket or device; delete them): %s", len(r.Special), q(r.Special))) } if len(r.Unreadable) > 0 { parts = append(parts, fmt.Sprintf("%d unreadable dir(s): %s", len(r.Unreadable), q(r.Unreadable))) @@ -207,8 +207,8 @@ func Watch(ctx context.Context, brainDir string, interval time.Duration, n Notif reported = key if key != "" { slog.Error("brain symlink tripwire", "finding", key) - q.push("brain symlink tripwire (hyperguild#157): " + key + - ". Readers may follow symlinks out of the brain; fix them.") + q.push("brain symlink tripwire (hyperguild#157, #179): " + key + + ". Symlinks can lead readers out of the brain and special files can hang them; remove them.") } } } @@ -343,8 +343,10 @@ func permanent(err error) bool { // removeSpecial unlinks each brain-relative FIFO, socket or device through // an os.Root on the brain dir, after re-checking that it is still one: a -// path that is now a regular file, a directory or a symlink is left alone -// and reported as failed. +// path that is a regular file, a directory or a symlink at the re-check is +// left alone and reported as failed. The re-check narrows, but cannot close, +// the window before the unlink (Linux has no conditional unlink); the same +// window exists in removeSymlinks, and only a local writer can use it. func removeSpecial(brainDir string, rels []string) (removed, failed []string) { if len(rels) == 0 { return nil, nil