fix(brainfs): refuse an in-brain file swapped in behind a symlink; in-brain test targets (#157)

Review round 1 on PR #168:

- ReadRegular/ReadRegularMax Lstat the path, then open it again by path;
  os.Root follows an in-root final symlink even with O_NOFOLLOW, so a
  symlink to another brain file swapped in between was read (8k-109k
  raw/ leaks in RebuildIndex, LoadInventory, IndexDoc). The opened handle
  must now be the same file as the Lstat (os.SameFile). This covers every
  ReadRegular caller, slices 1-3 included.
- Every symlink test pointed outside the brain, which os.Root refuses on
  its own. Each now has an in-brain case too: a static in-brain link per
  reader, AssertNoLeakFrom with an in-brain target, and brainfs unit tests
  for ReadDirRegular and the in-brain swap.
- brainfstest: swap links are relative (os.Root refuses every absolute
  link, so an absolute in-brain target tested nothing), and each swapper
  has its own temp name (two swappers on one path deadlocked on each
  other's FIFO).
- IndexDoc indexes only a clean wiki/ or knowledge/ path (callers pass
  caller-supplied paths).
- Rule 9: TestLoadInventoryReturnsErrNotRegularForASymlinkedTypeDir now
  asserts the error; the backfill test says it aborts on a symlinked root.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ingestion/internal/brainfs/brainfs.go

@@ -114,7 +114,7 @@ func (b *Brain) ReadRegular(rel string) ([]byte, error) {
 	if !fi.Mode().IsRegular() {
 		return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular)
 	}
-	return b.readOpened(rel)
+	return b.readOpenedMax(rel, -1, fi)
 }
 
 // readOpened opens rel non-blocking and reads it only if the OPENED handle is
@@ -128,7 +128,7 @@ func (b *Brain) ReadRegular(rel string) ([]byte, error) {
 // cannot create one; fs.protected_hardlinks=1 and the pod's secrets are on a
 // different filesystem from the brain volume, so a local caller cannot link
 // them in. Refusing Nlink > 1 is the next step if either changes.
-func (b *Brain) readOpened(rel string) ([]byte, error) { return b.readOpenedMax(rel, -1) }
+func (b *Brain) readOpened(rel string) ([]byte, error) { return b.readOpenedMax(rel, -1, nil) }
 
 // ReadRegularMax is ReadRegular with a size limit, checked on the opened
 // handle before reading (max < 0 means no limit).
@@ -140,10 +140,14 @@ func (b *Brain) ReadRegularMax(rel string, max int64) ([]byte, error) {
 	if !fi.Mode().IsRegular() {
 		return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular)
 	}
-	return b.readOpenedMax(rel, max)
+	return b.readOpenedMax(rel, max, fi)
 }
 
-func (b *Brain) readOpenedMax(rel string, max int64) ([]byte, error) {
+// readOpenedMax opens rel and reads it if the opened handle is a regular file
+// and, when lfi is set, the same file the caller Lstat-ed: os.Root follows an
+// in-root final symlink even with O_NOFOLLOW, so a symlink to another brain
+// file swapped in between the Lstat and the open is refused only here.
+func (b *Brain) readOpenedMax(rel string, max int64, lfi fs.FileInfo) ([]byte, error) {
 	f, err := b.root.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0)
 	if err != nil {
 		return nil, err
@@ -156,6 +160,9 @@ func (b *Brain) readOpenedMax(rel string, max int64) ([]byte, error) {
 	if !hfi.Mode().IsRegular() {
 		return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular)
 	}
+	if lfi != nil && !os.SameFile(lfi, hfi) {
+		return nil, fmt.Errorf("%s: changed while opening: %w", rel, ErrNotRegular)
+	}
 	if max >= 0 && hfi.Size() > max {
 		return nil, fmt.Errorf("%s is %d bytes (limit %d): %w", rel, hfi.Size(), max, ErrTooLarge)
 	}

ingestion/internal/brainfs/brainfs_test.go

@@ -6,6 +6,8 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"sync"
+	"sync/atomic"
 	"syscall"
 	"testing"
 	"time"
@@ -189,3 +191,74 @@ func TestWalkRegularReportsASymlinkedIntermediateComponentOfTheWalkRoot(t *testi
 	require.Len(t, errs, 1)
 	assert.ErrorIs(t, errs[0], ErrNotRegular)
 }
+
+func TestReadDirRegularListsOnlyRegularFilesAndRefusesSymlinkedComponents(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "a", "b", "sub"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "a", "b", "n.md"), []byte("n"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "other.md"), []byte("o"), 0o644))
+	require.NoError(t, os.Symlink(filepath.Join("..", "..", "other.md"), filepath.Join(dir, "a", "b", "l.md")))
+	require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "a", "b", "f.md"), 0o644))
+	require.NoError(t, os.Symlink("a", filepath.Join(dir, "alias")))
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "x"), 0o755))
+	require.NoError(t, os.Symlink(filepath.Join("..", "a"), filepath.Join(dir, "x", "mid")))
+	b, err := Open(dir)
+	require.NoError(t, err)
+	defer func() { _ = b.Close() }()
+
+	names, err := b.ReadDirRegular("a/b")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"n.md"}, names)
+	for _, rel := range []string{"alias/b", "x/mid/b"} {
+		_, err := b.ReadDirRegular(rel)
+		assert.ErrorIs(t, err, ErrNotRegular, rel)
+	}
+}
+
+// A final component swapped for a symlink to another regular file in the
+// brain between the Lstat and the open: os.Root follows in-root links, so
+// only comparing the two (os.SameFile) refuses it.
+func TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlink(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "s.md"), []byte("zqsamefilesentinel"), 0o644))
+	p := filepath.Join(dir, "r.md")
+	require.NoError(t, os.WriteFile(p, []byte("inside"), 0o644))
+	b, err := Open(dir)
+	require.NoError(t, err)
+	defer func() { _ = b.Close() }()
+
+	var stop atomic.Bool
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		tmp := p + ".swap"
+		for i := 0; !stop.Load(); i++ {
+			_ = os.Remove(tmp)
+			if i%2 == 0 {
+				_ = os.WriteFile(tmp, []byte("inside"), 0o644)
+			} else {
+				_ = os.Symlink("s.md", tmp)
+			}
+			_ = os.Rename(tmp, p)
+		}
+	}()
+	leaks, ok := 0, 0
+	for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); {
+		got, err := b.ReadRegular("r.md")
+		if err == nil {
+			ok++
+		}
+		if string(got) == "zqsamefilesentinel" {
+			leaks++
+		}
+		got, _ = b.ReadRegularMax("r.md", 1<<20)
+		if string(got) == "zqsamefilesentinel" {
+			leaks++
+		}
+	}
+	stop.Store(true)
+	wg.Wait()
+	assert.Zero(t, leaks)
+	assert.Positive(t, ok, "positive control: the regular file is read")
+}

ingestion/internal/brainfs/brainfstest/brainfstest.go

@@ -7,6 +7,7 @@
 package brainfstest
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -35,6 +36,8 @@ func Outside(t *testing.T) string {
 	return p
 }
 
+var swapSeq atomic.Int64
+
 // StartSwap renames, in a loop, a regular note (content inside) and either
 // a symlink to outside or a FIFO into path, until the test ends. FIFOs get a
 // writer held open until the swapper exits, so a reader that skips the
@@ -47,7 +50,15 @@ func StartSwap(t *testing.T, path, inside, outside string, fifo bool) {
 	wg.Add(1)
 	go func() {
 		defer wg.Done()
-		tmp := path + ".swap"
+		// A name of its own: swappers on one path share a test, and one
+		// opening another's FIFO for write would block forever.
+		tmp := fmt.Sprintf("%s.swap%d", path, swapSeq.Add(1))
+		// A relative link: os.Root refuses every absolute link on its own, so
+		// an absolute in-brain target would test nothing.
+		link := outside
+		if rel, err := filepath.Rel(filepath.Dir(path), outside); err == nil {
+			link = rel
+		}
 		var writers []*os.File
 		defer func() {
 			for _, w := range writers {
@@ -66,7 +77,7 @@ func StartSwap(t *testing.T, path, inside, outside string, fifo bool) {
 					}
 				}
 			default:
-				_ = os.Symlink(outside, tmp)
+				_ = os.Symlink(link, tmp)
 			}
 			_ = os.Rename(tmp, path)
 		}
@@ -74,11 +85,33 @@ func StartSwap(t *testing.T, path, inside, outside string, fifo bool) {
 	t.Cleanup(func() { stop.Store(true); wg.Wait() })
 }
 
+// WriteSentinelAt writes a file carrying Sentinel at p (creating its
+// directory), for an in-brain symlink target.
+func WriteSentinelAt(t *testing.T, p string) string {
+	t.Helper()
+	if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(p, []byte(outsideContent), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	return p
+}
+
 // AssertNoLeak loops read for d while a symlink swapper runs on path, and
-// fails if any output contains Sentinel.
+// fails if any output contains Sentinel. The symlink points outside the
+// brain; os.Root refuses that on its own, so pair it with AssertNoLeakFrom
+// and an in-brain target.
 func AssertNoLeak(t *testing.T, path, inside string, d time.Duration, read func() string) {
 	t.Helper()
-	StartSwap(t, path, inside, Outside(t), false)
+	AssertNoLeakFrom(t, path, inside, Outside(t), d, read)
+}
+
+// AssertNoLeakFrom is AssertNoLeak with the symlink pointing at target, e.g.
+// a file elsewhere in the brain written with WriteSentinelAt.
+func AssertNoLeakFrom(t *testing.T, path, inside, target string, d time.Duration, read func() string) {
+	t.Helper()
+	StartSwap(t, path, inside, target, false)
 	leaks, iters := 0, 0
 	for deadline := time.Now().Add(d); time.Now().Before(deadline); iters++ {
 		leaks += strings.Count(read(), Sentinel)

ingestion/internal/graphsync/graphsync.go

@@ -16,6 +16,8 @@ import (
 	"fmt"
 	"io/fs"
 	"path"
+	"path/filepath"
+	"strings"
 
 	"github.com/mathiasbq/hyperguild/ingestion/internal/brainfs"
 	"github.com/mathiasbq/hyperguild/ingestion/internal/graph"
@@ -48,6 +50,12 @@ func IndexDoc(ctx context.Context, store Store, brainDir, relPath string) error
 	if relPath == "" {
 		return nil
 	}
+	// Callers pass caller-supplied paths (brain_tunnel, brain_update): index
+	// only a clean path under wiki/ or knowledge/.
+	if path.Clean(relPath) != relPath || !filepath.IsLocal(relPath) ||
+		(!strings.HasPrefix(relPath, "wiki/") && !strings.HasPrefix(relPath, "knowledge/")) {
+		return fmt.Errorf("index %q: not a clean wiki/ or knowledge/ path", relPath)
+	}
 	bfs, err := brainfs.Open(brainDir)
 	if err != nil {
 		return fmt.Errorf("open brain: %w", err)

ingestion/internal/graphsync/graphsync_root_test.go

@@ -34,6 +34,11 @@ func TestIndexDocRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) {
 	require.NoError(t, IndexDoc(context.Background(), store, brain, "wiki/w/facts/ok.md"))
 	assert.Contains(t, store.dump(), "zqinside", "positive control: a regular note is indexed")
 	assert.Error(t, IndexDoc(context.Background(), store, brain, "wiki/w/facts/leak.md"))
+	// A link to a file elsewhere in the brain: os.Root allows it, only the
+	// regular-file check refuses it.
+	inBrain := brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "s.md"))
+	require.NoError(t, os.Symlink(filepath.Join("..", "..", "..", "raw", "s.md"), filepath.Join(facts, "inlink.md")))
+	assert.Error(t, IndexDoc(context.Background(), store, brain, "wiki/w/facts/inlink.md"))
 	assert.NotContains(t, store.dump(), brainfstest.Sentinel)
 
 	p := filepath.Join(facts, "r.md")
@@ -43,10 +48,11 @@ func TestIndexDocRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) {
 		return s.dump()
 	}
 	brainfstest.AssertNoLeak(t, p, "---\ntitle: zqinside\n---\n", 1500*time.Millisecond, read)
+	brainfstest.AssertNoLeakFrom(t, p, "---\ntitle: zqinside\n---\n", inBrain, 1500*time.Millisecond, read)
 	brainfstest.AssertNoHang(t, p, "---\ntitle: zqinside\n---\n", 1500*time.Millisecond, 6*time.Second, func() { _ = read() })
 }
 
-func TestBackfillSkipsSymlinkedNotesAndASymlinkedRoot(t *testing.T) {
+func TestBackfillSkipsSymlinkedNotesAndAbortsOnASymlinkedRoot(t *testing.T) {
 	brain := t.TempDir()
 	facts := filepath.Join(brain, "wiki", "w", "facts")
 	require.NoError(t, os.MkdirAll(facts, 0o755))
@@ -54,6 +60,7 @@ func TestBackfillSkipsSymlinkedNotesAndASymlinkedRoot(t *testing.T) {
 	require.NoError(t, os.Symlink(brainfstest.Outside(t), filepath.Join(facts, "leak.md")))
 	require.NoError(t, os.MkdirAll(filepath.Join(brain, "raw"), 0o755))
 	require.NoError(t, os.WriteFile(filepath.Join(brain, "raw", "r.md"), []byte("---\ntitle: zqrawonly\n---\n"), 0o644))
+	require.NoError(t, os.Symlink(filepath.Join("..", "..", "..", "raw", "r.md"), filepath.Join(facts, "inlink.md")))
 	require.NoError(t, os.Symlink(".", filepath.Join(brain, "knowledge")))
 
 	store := newFakeStore()
@@ -70,3 +77,16 @@ func TestBackfillSkipsSymlinkedNotesAndASymlinkedRoot(t *testing.T) {
 	assert.Contains(t, store.dump(), "zqinside")
 	assert.NotContains(t, store.dump(), brainfstest.Sentinel)
 }
+
+// IndexDoc gets caller-supplied paths (brain_tunnel, brain_update): only a
+// clean path under wiki/ or knowledge/ is indexed.
+func TestIndexDocRefusesPathsOutsideWikiAndKnowledgeAndUncleanPaths(t *testing.T) {
+	brain := t.TempDir()
+	brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "secret.md"))
+	require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w"), 0o755))
+	for _, p := range []string{"raw/secret.md", "wiki/w/../../raw/secret.md", "./raw/secret.md", "/raw/secret.md"} {
+		store := newFakeStore()
+		assert.Error(t, IndexDoc(context.Background(), store, brain, p), p)
+		assert.NotContains(t, store.dump(), brainfstest.Sentinel, p)
+	}
+}

ingestion/internal/wiki/root_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/mathiasbq/hyperguild/ingestion/internal/brainfs"
 	"github.com/mathiasbq/hyperguild/ingestion/internal/brainfs/brainfstest"
 )
 
@@ -23,6 +24,8 @@ func TestLoadInventoryRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) {
 	require.NoError(t, os.MkdirAll(dir, 0o755))
 	require.NoError(t, os.WriteFile(filepath.Join(dir, "ok.md"), []byte("---\ntitle: zqinside\n---\n"), 0o644))
 	require.NoError(t, os.Symlink(brainfstest.Outside(t), filepath.Join(dir, "leak.md")))
+	inBrain := brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "s.md"))
+	require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "s.md"), filepath.Join(dir, "inlink.md")))
 
 	inv, err := LoadInventory(brain)
 	require.NoError(t, err)
@@ -30,6 +33,7 @@ func TestLoadInventoryRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) {
 	assert.Contains(t, got, "zqinside", "positive control")
 	assert.NotContains(t, got, brainfstest.Sentinel)
 	assert.NotContains(t, got, "leak", "a symlinked page is not listed")
+	assert.NotContains(t, got, "inlink", "a page linked to another brain file is not listed")
 
 	p := filepath.Join(dir, "r.md")
 	read := func() string {
@@ -37,16 +41,18 @@ func TestLoadInventoryRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) {
 		return fmt.Sprintf("%+v", inv)
 	}
 	brainfstest.AssertNoLeak(t, p, "---\ntitle: zqinside\n---\n", 1500*time.Millisecond, read)
+	brainfstest.AssertNoLeakFrom(t, p, "---\ntitle: zqinside\n---\n", inBrain, 1500*time.Millisecond, read)
 	brainfstest.AssertNoHang(t, p, "---\ntitle: zqinside\n---\n", 1500*time.Millisecond, 6*time.Second, func() { _ = read() })
 }
 
-func TestLoadInventorySkipsASymlinkedTypeDir(t *testing.T) {
+func TestLoadInventoryReturnsErrNotRegularForASymlinkedTypeDir(t *testing.T) {
 	brain := t.TempDir()
 	require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki"), 0o755))
 	require.NoError(t, os.MkdirAll(filepath.Join(brain, "raw"), 0o755))
 	require.NoError(t, os.WriteFile(filepath.Join(brain, "raw", "r.md"), []byte("---\ntitle: zqrawonly\n---\n"), 0o644))
 	require.NoError(t, os.Symlink(filepath.Join("..", "raw"), filepath.Join(brain, "wiki", "concepts")))
-	inv, _ := LoadInventory(brain)
+	inv, err := LoadInventory(brain)
+	require.ErrorIs(t, err, brainfs.ErrNotRegular)
 	assert.NotContains(t, fmt.Sprintf("%+v", inv), "zqrawonly")
 }
 
@@ -67,5 +73,7 @@ func TestRebuildIndexDoesNotReadAPageSwappedForASymlink(t *testing.T) {
 		return string(b)
 	}
 	brainfstest.AssertNoLeak(t, p, "---\ntitle: R\n---\nzqinside sentence.\n", 1500*time.Millisecond, read)
+	inBrain := brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "s.md"))
+	brainfstest.AssertNoLeakFrom(t, p, "---\ntitle: R\n---\nzqinside sentence.\n", inBrain, 1500*time.Millisecond, read)
 	brainfstest.AssertNoHang(t, p, "---\ntitle: R\n---\nzqinside sentence.\n", 1500*time.Millisecond, 6*time.Second, func() { _ = read() })
 }