From 9c9ba31770ceef2925dd617cc59431febb22b9b3 Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 3 Oct 2026 18:18:54 +0200 Subject: [PATCH 1/3] fix(search): walk and read the brain through an os.Root (#157, slice 1) Option 1 of #157, first slice. Search walked and read the brain by path, so a symlink planted in the Syncthing-shared brain dir put an outside file's contents in brain_query results, and a FIFO swapped into the wiki hung the query forever (reproduced: the FIFO race test blocked for the full 6s timeout before this change). New package internal/brainfs: Open (os.Root on the brain dir), ReadRegular (Lstat + non-blocking open + type check on the handle) and WalkRegular (walks the root's FS, yields regular files only, never follows a link). search.Query and hydrate use it. Tests (red first): a wiki symlink is not followed, a symlinked dir is not descended, hydrate refuses a symlink, and race tests swap a symlink or a FIFO into a wiki note while Query loops; any leaked byte or a hang fails. Refs #157 Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/brainfs/brainfs.go | 98 ++++++++++++++++ ingestion/internal/search/brainroot_test.go | 124 ++++++++++++++++++++ ingestion/internal/search/search.go | 38 ++++-- 3 files changed, 250 insertions(+), 10 deletions(-) create mode 100644 ingestion/internal/brainfs/brainfs.go create mode 100644 ingestion/internal/search/brainroot_test.go diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go new file mode 100644 index 0000000..8cbb477 --- /dev/null +++ b/ingestion/internal/brainfs/brainfs.go @@ -0,0 +1,98 @@ +// Package brainfs reads the brain directory through an os.Root (hyperguild#157). +// The brain dir is a Syncthing sendreceive folder and Syncthing syncs +// symlinks; a reader that walks and reads by path follows a planted symlink +// out of the brain, and blocks forever on a planted FIFO. Readers that go +// through a Brain only ever read regular files inside the brain: os.Root +// refuses to leave the root even if an entry is swapped mid-read, and every +// open is non-blocking with the type checked on the opened handle. +package brainfs + +import ( + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "syscall" +) + +// ErrNotRegular is returned for an entry that is not a regular file. +var ErrNotRegular = errors.New("not a regular file") + +// Brain is an open brain directory. +type Brain struct { + root *os.Root + dir string +} + +// Open opens brainDir (which may itself be a mount symlink) as a root. +func Open(brainDir string) (*Brain, error) { + r, err := os.OpenRoot(brainDir) + if err != nil { + return nil, fmt.Errorf("open brain root: %w", err) + } + return &Brain{root: r, dir: brainDir}, nil +} + +// Close releases the root. +func (b *Brain) Close() error { return b.root.Close() } + +// Rel converts a path under the brain dir to a root-relative path. +func (b *Brain) Rel(path string) (string, error) { + rel, err := filepath.Rel(b.dir, path) + if err != nil { + return "", err + } + if rel == ".." || len(rel) > 2 && rel[:3] == ".."+string(filepath.Separator) { + return "", fmt.Errorf("%s is outside the brain dir", path) + } + return filepath.ToSlash(rel), nil +} + +// ReadRegular reads a brain-relative path, refusing anything that is not a +// regular file, both before the open and on the opened handle. +func (b *Brain) ReadRegular(rel string) ([]byte, error) { + fi, err := b.root.Lstat(rel) + if err != nil { + return nil, err + } + if !fi.Mode().IsRegular() { + return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular) + } + f, err := b.root.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + hfi, err := f.Stat() + if err != nil { + return nil, err + } + if !hfi.Mode().IsRegular() { + return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular) + } + return io.ReadAll(f) +} + +// WalkRegular walks a brain-relative directory through the root and calls fn +// for each regular file (directories are descended; symlinks, FIFOs and +// devices are skipped, never followed). An entry error is passed to onErr and +// the walk continues; a nil onErr ignores it. +func (b *Brain) WalkRegular(relDir string, fn func(rel string) error, onErr func(rel string, err error)) error { + return fs.WalkDir(b.root.FS(), filepath.ToSlash(relDir), func(p string, d fs.DirEntry, err error) error { + if err != nil { + if onErr != nil { + onErr(p, err) + } + if d != nil && d.IsDir() { + return fs.SkipDir + } + return nil + } + if d.IsDir() || !d.Type().IsRegular() { + return nil + } + return fn(p) + }) +} diff --git a/ingestion/internal/search/brainroot_test.go b/ingestion/internal/search/brainroot_test.go new file mode 100644 index 0000000..77715d4 --- /dev/null +++ b/ingestion/internal/search/brainroot_test.go @@ -0,0 +1,124 @@ +package search + +import ( + "os" + "path/filepath" + "strings" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// hyperguild#157 option 1: search walked and read the brain by path, so a +// symlink anywhere in the Syncthing-shared brain dir (wiki///leak.md -> +// /any/file) put the target's contents in brain_query results (reproduced in +// the PR #156 review). Search now walks and reads through an os.Root on the +// brain dir and only reads regular files. + +const sentinel157 = "zqsentinelonefiftyseven" + +func searchBrain(t *testing.T) (brain, facts, outside string) { + t.Helper() + brain = t.TempDir() + facts = filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(brain, "knowledge"), 0o755)) + outside = filepath.Join(t.TempDir(), "secret.md") + require.NoError(t, os.WriteFile(outside, []byte("# secret\n\n"+sentinel157), 0o644)) + return brain, facts, outside +} + +func leaked(rs []Result) int { + n := 0 + for _, r := range rs { + n += strings.Count(r.Excerpt+r.Title, sentinel157) + } + return n +} + +func TestQueryDoesNotFollowASymlinkInTheWiki(t *testing.T) { + brain, facts, outside := searchBrain(t) + require.NoError(t, os.Symlink(outside, filepath.Join(facts, "leak.md"))) + require.NoError(t, os.WriteFile(filepath.Join(facts, "real.md"), []byte("# real\n\nzqrealnote"), 0o644)) + rs, err := Query(brain, QueryOptions{Query: sentinel157, Limit: 10}) + require.NoError(t, err) + assert.Zero(t, leaked(rs)) + rs, err = Query(brain, QueryOptions{Query: "zqrealnote", Limit: 10}) + require.NoError(t, err) + assert.Len(t, rs, 1, "regular notes are still found") +} + +func TestQueryDoesNotFollowASymlinkedDirectory(t *testing.T) { + brain, _, outside := searchBrain(t) + require.NoError(t, os.Symlink(filepath.Dir(outside), filepath.Join(brain, "wiki", "linked"))) + rs, err := Query(brain, QueryOptions{Query: sentinel157, Limit: 10}) + require.NoError(t, err) + assert.Zero(t, leaked(rs)) +} + +func swapInto(path, outside string, fifo bool, stop *atomic.Bool) { + tmp := path + ".swap" + for i := 0; !stop.Load(); i++ { + _ = os.Remove(tmp) + switch { + case i%2 == 0: + _ = os.WriteFile(tmp, []byte("# r\n\nzqinside "+"zqracenote"), 0o644) + case fifo: + _ = syscall.Mkfifo(tmp, 0o644) + default: + _ = os.Symlink(outside, tmp) + } + _ = os.Rename(tmp, path) + } +} + +func TestQueryLeaksNothingUnderASymlinkSwapRace(t *testing.T) { + brain, facts, outside := searchBrain(t) + var stop atomic.Bool + go swapInto(filepath.Join(facts, "r.md"), outside, false, &stop) + defer stop.Store(true) + leaks, iters := 0, 0 + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); iters++ { + rs, _ := Query(brain, QueryOptions{Query: sentinel157, Limit: 10}) + leaks += leaked(rs) + } + if leaks > 0 { + t.Fatalf("outside file leaked %d times in %d iterations", leaks, iters) + } +} + +func TestQueryDoesNotHangUnderAFIFOSwapRace(t *testing.T) { + brain, facts, outside := searchBrain(t) + var stop atomic.Bool + go swapInto(filepath.Join(facts, "r.md"), outside, true, &stop) + done := make(chan struct{}) + go func() { + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); { + _, _ = Query(brain, QueryOptions{Query: "zqracenote", Limit: 10}) + } + close(done) + }() + select { + case <-done: + stop.Store(true) + case <-time.After(6 * time.Second): + stop.Store(true) + if f, err := os.OpenFile(filepath.Join(facts, "r.md"), os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { + _ = f.Close() + } + t.Fatal("search hung on a swapped-in FIFO") + } +} + +// hydrate (vector-only hits) reads by relative path; it must refuse a symlink. +func TestHydrateRefusesASymlink(t *testing.T) { + brain, facts, outside := searchBrain(t) + require.NoError(t, os.Symlink(outside, filepath.Join(facts, "leak.md"))) + r, err := hydrate(brain, "wiki/w/facts/leak.md") + assert.Error(t, err) + assert.NotContains(t, r.Excerpt, sentinel157) +} diff --git a/ingestion/internal/search/search.go b/ingestion/internal/search/search.go index c272a61..842fd7c 100644 --- a/ingestion/internal/search/search.go +++ b/ingestion/internal/search/search.go @@ -5,6 +5,7 @@ import ( "bufio" "context" "fmt" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" sharedfm "github.com/mathiasbq/hyperguild/ingestion/internal/frontmatter" "log/slog" "os" @@ -119,20 +120,30 @@ func QueryContext(ctx context.Context, brainDir string, opts QueryOptions) ([]Re return nil, err } + // Walk and read through an os.Root on the brain dir, regular files only + // (hyperguild#157): a symlink planted in the Syncthing-shared brain must + // not put an outside file in results, nor a FIFO hang the query. + bfs, err := brainfs.Open(brainDir) + if err != nil { + return nil, fmt.Errorf("search: %w", err) + } + defer func() { _ = bfs.Close() }() + var results []Result for _, dir := range roots { if _, statErr := os.Stat(dir); os.IsNotExist(statErr) { continue } - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { - if err != nil { - slog.Warn("search: skipping path", "path", path, "err", err) + relDir, relErr := bfs.Rel(dir) + if relErr != nil { + return nil, fmt.Errorf("search: %w", relErr) + } + err := bfs.WalkRegular(relDir, func(relPath string) error { + if !strings.HasSuffix(relPath, ".md") { return nil } - if d.IsDir() || !strings.HasSuffix(path, ".md") { - return nil - } - content, err := os.ReadFile(path) + path := filepath.Join(brainDir, filepath.FromSlash(relPath)) + content, err := bfs.ReadRegular(relPath) if err != nil { slog.Warn("search: skipping unreadable file", "path", path, "err", err) return nil @@ -155,7 +166,7 @@ func QueryContext(ctx context.Context, brainDir string, opts QueryOptions) ([]Re origin, writtenBy := extractProvenance(string(content)) results = append(results, Result{ Path: rel, - Title: extractTitle(string(content), d.Name()), + Title: extractTitle(string(content), filepath.Base(relPath)), Excerpt: excerpt(string(content), 300), Score: score, Wing: wing, @@ -166,6 +177,8 @@ func QueryContext(ctx context.Context, brainDir string, opts QueryOptions) ([]Re WrittenBy: writtenBy, }) return nil + }, func(p string, err error) { + slog.Warn("search: skipping path", "path", p, "err", err) }) if err != nil { return nil, err @@ -275,8 +288,13 @@ func pathInScope(relPath, wing, hall string) bool { // excerpt, wing, and hall populated. Used for paths that surface only // via vector search. func hydrate(brainDir, relPath string) (Result, error) { - full := filepath.Join(brainDir, filepath.FromSlash(relPath)) - content, err := os.ReadFile(full) + // Through the brain root, regular files only (hyperguild#157). + bfs, err := brainfs.Open(brainDir) + if err != nil { + return Result{}, err + } + defer func() { _ = bfs.Close() }() + content, err := bfs.ReadRegular(filepath.ToSlash(relPath)) if err != nil { return Result{}, err } -- 2.54.0 From ffe46c7077507dec8ad1a23b8b752d5f1edd0e76 Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 3 Oct 2026 18:26:27 +0200 Subject: [PATCH 2/3] test(brainfs): pin the handle check with a FIFO held open by a writer PR #161 review: removing ReadRegular's type check on the opened handle stayed green, because the race test's FIFOs had no writer and read EOF at once; with a writer holding the FIFO open, search hung after 16 iterations. The open-and-check step is split into readOpened, and TestReadOpenedRefusesAFIFOHeldOpenByAWriter reaches it past the Lstat with a writer open: it must return ErrNotRegular promptly (the flip is red). Also unit tests for ReadRegular (symlink, escape), Rel (inside incl. ..foo, escapes) and WalkRegular (regular files only), and the hard-link residual documented where it is accepted, as in api/rawroot.go. Refs #157 Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/brainfs/brainfs.go | 15 ++++ ingestion/internal/brainfs/brainfs_test.go | 92 ++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 ingestion/internal/brainfs/brainfs_test.go diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index 8cbb477..c370d7b 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -60,6 +60,21 @@ func (b *Brain) ReadRegular(rel string) ([]byte, error) { if !fi.Mode().IsRegular() { return nil, fmt.Errorf("%s: %w", rel, ErrNotRegular) } + return b.readOpened(rel) +} + +// readOpened opens rel non-blocking and reads it only if the OPENED handle is +// a regular file. This is the check that matters under a race: an entry +// swapped for a FIFO after the Lstat, with a writer holding it open, would +// otherwise park io.ReadAll forever (PR #161 review: removing it hung search +// after 16 iterations). +// +// Accepted residual, as in api/rawroot.go: a HARD link to an outside file is +// a regular file and is read. Syncthing syncs contents, not links, so a peer +// 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) { f, err := b.root.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0) if err != nil { return nil, err diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go new file mode 100644 index 0000000..4d4b2dc --- /dev/null +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -0,0 +1,92 @@ +package brainfs + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The handle check is what stops a FIFO swapped in after the Lstat. A FIFO +// with no writer reads EOF at once, so a race test could not see the check +// missing; here a writer holds the FIFO open, as any local process can, and +// readOpened must refuse it promptly (PR #161 review). +func TestReadOpenedRefusesAFIFOHeldOpenByAWriter(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "p.md") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + w, err := os.OpenFile(fifo, os.O_RDWR, 0) + require.NoError(t, err) + defer func() { _ = w.Close() }() + + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + + done := make(chan error, 1) + go func() { _, err := b.readOpened("p.md"); done <- err }() + select { + case err := <-done: + assert.True(t, errors.Is(err, ErrNotRegular), "got %v", err) + case <-time.After(2 * time.Second): + t.Fatal("readOpened blocked on a FIFO with a writer") + } +} + +func TestReadRegularRefusesSymlinksAndReadsRegularFiles(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "ok.md"), []byte("ok"), 0o644)) + outside := filepath.Join(t.TempDir(), "s.md") + require.NoError(t, os.WriteFile(outside, []byte("secret"), 0o644)) + require.NoError(t, os.Symlink(outside, filepath.Join(dir, "link.md"))) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + got, err := b.ReadRegular("ok.md") + require.NoError(t, err) + assert.Equal(t, "ok", string(got)) + _, err = b.ReadRegular("link.md") + assert.True(t, errors.Is(err, ErrNotRegular), "got %v", err) + _, err = b.ReadRegular("../" + filepath.Base(outside)) + assert.Error(t, err) +} + +func TestRelAcceptsPathsInsideAndRefusesEscapes(t *testing.T) { + dir := t.TempDir() + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + for in, want := range map[string]string{ + filepath.Join(dir, "wiki", "x.md"): "wiki/x.md", + filepath.Join(dir, "..foo", "x"): "..foo/x", + dir: ".", + } { + got, err := b.Rel(in) + require.NoError(t, err, in) + assert.Equal(t, want, got, in) + } + for _, bad := range []string{filepath.Join(dir, "..", "x"), dir + "-evil/x", "/etc/passwd", filepath.Join(dir, "wiki", "..", "..", "x")} { + _, err := b.Rel(bad) + assert.Error(t, err, bad) + } +} + +func TestWalkRegularYieldsOnlyRegularFiles(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "a"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "a", "n.md"), []byte("n"), 0o644)) + require.NoError(t, os.Symlink(t.TempDir(), filepath.Join(dir, "a", "dirlink"))) + require.NoError(t, os.Symlink(filepath.Join(dir, "a", "n.md"), filepath.Join(dir, "a", "filelink.md"))) + require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "a", "p.md"), 0o644)) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + var got []string + require.NoError(t, b.WalkRegular(".", func(rel string) error { got = append(got, rel); return nil }, nil)) + assert.Equal(t, []string{"a/n.md"}, got) +} -- 2.54.0 From fd33e044675fa25b22b6b1ab69ee852632f9350f Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 3 Oct 2026 18:32:40 +0200 Subject: [PATCH 3/3] test(search): hold a writer on swapped-in FIFOs; await the swapper PR #161 round 2: ReadRegular bypassing readOpened (non-blocking open, no handle check) stayed green, because the search FIFO race used FIFOs with no writer, which read EOF at once. The swapper now opens each FIFO O_RDWR and keeps every writer open until it exits, as any local process could, so a reader that skips the handle check parks forever; the bypass flip is red. The swapper is stopped and awaited in a t.Cleanup before t.TempDir removal (hyperguild#162). Refs #157 Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/search/brainroot_test.go | 41 +++++++++++++++++---- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/ingestion/internal/search/brainroot_test.go b/ingestion/internal/search/brainroot_test.go index 77715d4..cfca022 100644 --- a/ingestion/internal/search/brainroot_test.go +++ b/ingestion/internal/search/brainroot_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "strings" + "sync" "sync/atomic" "syscall" "testing" @@ -60,15 +61,44 @@ func TestQueryDoesNotFollowASymlinkedDirectory(t *testing.T) { assert.Zero(t, leaked(rs)) } +// swapInto keeps renaming a regular note, and either a symlink to outside or +// a FIFO, into path. Each FIFO has a writer held open (as any local process +// can): a FIFO with no writer reads EOF at once, which would let a reader +// that skips the handle check pass (PR #161 round 2: with a held writer, a +// bypass of readOpened hung Query after 5 iterations). +// startSwap runs swapInto until the test ends and waits for it before +// t.TempDir's cleanup (hyperguild#162). +func startSwap(t *testing.T, path, outside string, fifo bool) { + t.Helper() + var stop atomic.Bool + var wg sync.WaitGroup + wg.Add(1) + go func() { defer wg.Done(); swapInto(path, outside, fifo, &stop) }() + t.Cleanup(func() { stop.Store(true); wg.Wait() }) +} + func swapInto(path, outside string, fifo bool, stop *atomic.Bool) { tmp := path + ".swap" + // Writers stay open until the swapper exits, so a reader parked on a + // FIFO stays parked (closing one would hand the reader EOF and hide the + // hang). Past the fd limit, new FIFOs simply have no writer. + var writers []*os.File + defer func() { + for _, w := range writers { + _ = w.Close() + } + }() for i := 0; !stop.Load(); i++ { _ = os.Remove(tmp) switch { case i%2 == 0: _ = os.WriteFile(tmp, []byte("# r\n\nzqinside "+"zqracenote"), 0o644) case fifo: - _ = syscall.Mkfifo(tmp, 0o644) + if err := syscall.Mkfifo(tmp, 0o644); err == nil { + if w, err := os.OpenFile(tmp, os.O_RDWR, 0); err == nil { + writers = append(writers, w) + } + } default: _ = os.Symlink(outside, tmp) } @@ -78,9 +108,7 @@ func swapInto(path, outside string, fifo bool, stop *atomic.Bool) { func TestQueryLeaksNothingUnderASymlinkSwapRace(t *testing.T) { brain, facts, outside := searchBrain(t) - var stop atomic.Bool - go swapInto(filepath.Join(facts, "r.md"), outside, false, &stop) - defer stop.Store(true) + startSwap(t, filepath.Join(facts, "r.md"), outside, false) leaks, iters := 0, 0 for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); iters++ { rs, _ := Query(brain, QueryOptions{Query: sentinel157, Limit: 10}) @@ -93,8 +121,7 @@ func TestQueryLeaksNothingUnderASymlinkSwapRace(t *testing.T) { func TestQueryDoesNotHangUnderAFIFOSwapRace(t *testing.T) { brain, facts, outside := searchBrain(t) - var stop atomic.Bool - go swapInto(filepath.Join(facts, "r.md"), outside, true, &stop) + startSwap(t, filepath.Join(facts, "r.md"), outside, true) done := make(chan struct{}) go func() { for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); { @@ -104,9 +131,7 @@ func TestQueryDoesNotHangUnderAFIFOSwapRace(t *testing.T) { }() select { case <-done: - stop.Store(true) case <-time.After(6 * time.Second): - stop.Store(true) if f, err := os.OpenFile(filepath.Join(facts, "r.md"), os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { _ = f.Close() } -- 2.54.0