diff --git a/ingestion/internal/api/readnote_root_test.go b/ingestion/internal/api/readnote_root_test.go new file mode 100644 index 0000000..0a7353d --- /dev/null +++ b/ingestion/internal/api/readnote_root_test.go @@ -0,0 +1,38 @@ +package api_test + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/api" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs/brainfstest" +) + +// hyperguild#157 slice 2: ReadNote (brain_get, read-after-write) read by +// path; it now reads through brainfs. +func TestReadNoteIsConfinedToRegularFilesInTheBrain(t *testing.T) { + brain := t.TempDir() + facts := filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + require.NoError(t, os.Symlink(brainfstest.Outside(t), filepath.Join(facts, "leak.md"))) + _, body, _, err := api.ReadNote(brain, "wiki/w/facts/leak.md") + assert.Error(t, err) + assert.NotContains(t, body, brainfstest.Sentinel) + + p := filepath.Join(facts, "r.md") + read := func() string { + fm, body, _, _ := api.ReadNote(brain, "wiki/w/facts/r.md") + s := body + for k, v := range fm { + s += k + v + } + return s + } + brainfstest.AssertNoLeak(t, p, "---\nx: y\n---\ninside", 1500*time.Millisecond, read) + brainfstest.AssertNoHang(t, p, "---\nx: y\n---\ninside", 1500*time.Millisecond, 6*time.Second, func() { _ = read() }) +} diff --git a/ingestion/internal/api/update.go b/ingestion/internal/api/update.go index 442a0a8..9301068 100644 --- a/ingestion/internal/api/update.go +++ b/ingestion/internal/api/update.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" "os" "path/filepath" "strings" @@ -174,11 +175,17 @@ func UpdateNote(brainDir string, opts UpdateNoteOptions) (relPath, contentHash, // write primitive behind brain_get: the hash it returns equals the hash // brain_write / brain_update returned for the same bytes. func ReadNote(brainDir, relPath string) (fm map[string]string, body, contentHash string, err error) { - _, abs, err := resolveWithin(brainDir, relPath) + rel, _, err := resolveWithin(brainDir, relPath) if err != nil { return nil, "", "", err } - raw, err := os.ReadFile(abs) + // Through the brain root, regular files only (hyperguild#157). + bfs, err := brainfs.Open(brainDir) + if err != nil { + return nil, "", "", fmt.Errorf("read note: %w", err) + } + defer func() { _ = bfs.Close() }() + raw, err := bfs.ReadRegular(rel) if err != nil { if os.IsNotExist(err) { return nil, "", "", fmt.Errorf("note %q does not exist", relPath) diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index c370d7b..a182238 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -20,6 +20,9 @@ import ( // ErrNotRegular is returned for an entry that is not a regular file. var ErrNotRegular = errors.New("not a regular file") +// ErrTooLarge is returned by ReadRegularMax for a file over the limit. +var ErrTooLarge = errors.New("file too large") + // Brain is an open brain directory. type Brain struct { root *os.Root @@ -74,7 +77,22 @@ 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) { +func (b *Brain) readOpened(rel string) ([]byte, error) { return b.readOpenedMax(rel, -1) } + +// ReadRegularMax is ReadRegular with a size limit, checked on the opened +// handle before reading (max < 0 means no limit). +func (b *Brain) ReadRegularMax(rel string, max int64) ([]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) + } + return b.readOpenedMax(rel, max) +} + +func (b *Brain) readOpenedMax(rel string, max int64) ([]byte, error) { f, err := b.root.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0) if err != nil { return nil, err @@ -87,6 +105,9 @@ func (b *Brain) readOpened(rel string) ([]byte, error) { if !hfi.Mode().IsRegular() { return nil, fmt.Errorf("%s: %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) + } return io.ReadAll(f) } diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index 4d4b2dc..3c48a00 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -90,3 +90,16 @@ func TestWalkRegularYieldsOnlyRegularFiles(t *testing.T) { require.NoError(t, b.WalkRegular(".", func(rel string) error { got = append(got, rel); return nil }, nil)) assert.Equal(t, []string{"a/n.md"}, got) } + +func TestReadRegularMaxRefusesAFileOverTheLimit(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "big.md"), make([]byte, 100), 0o644)) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + _, err = b.ReadRegularMax("big.md", 99) + assert.True(t, errors.Is(err, ErrTooLarge), "got %v", err) + got, err := b.ReadRegularMax("big.md", 100) + require.NoError(t, err) + assert.Len(t, got, 100) +} diff --git a/ingestion/internal/brainfs/brainfstest/brainfstest.go b/ingestion/internal/brainfs/brainfstest/brainfstest.go new file mode 100644 index 0000000..56f6bc8 --- /dev/null +++ b/ingestion/internal/brainfs/brainfstest/brainfstest.go @@ -0,0 +1,102 @@ +// Package brainfstest is the shared harness for hyperguild#157's behavioural +// confinement tests: it keeps swapping a regular note, a symlink to an +// outside file, or a FIFO (with a writer held open) into a path while a +// reader under test loops. A reader is confined if no sentinel byte ever +// reaches its output and it never hangs. +package brainfstest + +import ( + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" +) + +// Sentinel marks the outside file's contents. +const Sentinel = "zqbrainfssentinelonefiftyseven" + +// Outside writes a file outside any brain dir containing Sentinel. +func Outside(t *testing.T) string { + t.Helper() + p := filepath.Join(t.TempDir(), "secret.md") + if err := os.WriteFile(p, []byte("# secret\n\n"+Sentinel), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// 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 +// handle check parks forever instead of reading EOF. The swapper is stopped +// and awaited before t.TempDir cleanup. +func StartSwap(t *testing.T, path, inside, outside string, fifo bool) { + t.Helper() + var stop atomic.Bool + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + tmp := path + ".swap" + 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(inside), 0o644) + case fifo: + 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) + } + _ = os.Rename(tmp, path) + } + }() + t.Cleanup(func() { stop.Store(true); wg.Wait() }) +} + +// AssertNoLeak loops read for d while a symlink swapper runs on path, and +// fails if any output contains Sentinel. +func AssertNoLeak(t *testing.T, path, inside string, d time.Duration, read func() string) { + t.Helper() + StartSwap(t, path, inside, Outside(t), false) + leaks, iters := 0, 0 + for deadline := time.Now().Add(d); time.Now().Before(deadline); iters++ { + leaks += strings.Count(read(), Sentinel) + } + if leaks > 0 { + t.Fatalf("outside file leaked %d times in %d iterations", leaks, iters) + } +} + +// AssertNoHang loops read for d while a FIFO swapper (held writers) runs on +// path, and fails if a read does not return within timeout. +func AssertNoHang(t *testing.T, path, inside string, d, timeout time.Duration, read func()) { + t.Helper() + StartSwap(t, path, inside, Outside(t), true) + done := make(chan struct{}) + go func() { + for deadline := time.Now().Add(d); time.Now().Before(deadline); { + read() + } + close(done) + }() + select { + case <-done: + case <-time.After(timeout): + t.Fatal("reader hung on a swapped-in FIFO with a writer held open") + } +} diff --git a/ingestion/internal/mcp/readexcerpt_root_test.go b/ingestion/internal/mcp/readexcerpt_root_test.go new file mode 100644 index 0000000..bb26d40 --- /dev/null +++ b/ingestion/internal/mcp/readexcerpt_root_test.go @@ -0,0 +1,34 @@ +package mcp + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs/brainfstest" +) + +// hyperguild#157 slice 2: readExcerpt (brain_context) read by path, so a +// symlink in the Syncthing-shared brain put an outside file in context, and +// a FIFO hung it. It now reads through brainfs. +func TestReadExcerptIsConfinedToRegularFilesInTheBrain(t *testing.T) { + brain := t.TempDir() + facts := filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + require.NoError(t, os.Symlink(brainfstest.Outside(t), filepath.Join(facts, "leak.md"))) + assert.NotContains(t, readExcerpt(brain, "wiki/w/facts/leak.md", 500), brainfstest.Sentinel) + + p := filepath.Join(facts, "r.md") + require.NoError(t, os.WriteFile(p, []byte("---\nx: y\n---\ninside"), 0o644)) + assert.Equal(t, "inside", readExcerpt(brain, "wiki/w/facts/r.md", 500)) + brainfstest.AssertNoLeak(t, p, "---\nx: y\n---\ninside", 1500*time.Millisecond, func() string { + return readExcerpt(brain, "wiki/w/facts/r.md", 500) + }) + brainfstest.AssertNoHang(t, p, "---\nx: y\n---\ninside", 1500*time.Millisecond, 6*time.Second, func() { + _ = readExcerpt(brain, "wiki/w/facts/r.md", 500) + }) +} diff --git a/ingestion/internal/mcp/tools_context.go b/ingestion/internal/mcp/tools_context.go index d906f20..7d8bf70 100644 --- a/ingestion/internal/mcp/tools_context.go +++ b/ingestion/internal/mcp/tools_context.go @@ -4,7 +4,7 @@ import ( "context" "encoding/json" "fmt" - "os" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" "path/filepath" "sort" "strings" @@ -181,8 +181,13 @@ func readExcerpt(brainDir, relPath string, maxLen int) string { if relPath == "" { return "" } - 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 "" + } + defer func() { _ = bfs.Close() }() + content, err := bfs.ReadRegular(filepath.ToSlash(relPath)) if err != nil { return "" } diff --git a/ingestion/internal/notes/edit.go b/ingestion/internal/notes/edit.go index 99e86b1..1bf4f9b 100644 --- a/ingestion/internal/notes/edit.go +++ b/ingestion/internal/notes/edit.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "errors" "fmt" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" "os" "path/filepath" "strings" @@ -71,14 +72,19 @@ func ReadScoped(brainDir, path string, scope Scope) ([]byte, error) { } func readContained(t Target) ([]byte, error) { - fi, err := os.Stat(t.Abs) + // Through the brain root, regular files only, size checked on the opened + // handle (hyperguild#157): Resolve's Lstat could be raced by a swap, and + // os.ReadFile blocked on a FIFO. + brainDir := strings.TrimSuffix(t.Abs, string(filepath.Separator)+filepath.FromSlash(t.Rel)) + bfs, err := brainfs.Open(brainDir) if err != nil { - return nil, fmt.Errorf("notes: stat %s: %w", t.Rel, err) + return nil, fmt.Errorf("notes: %w", err) } - if fi.Size() > maxNoteBytes { - return nil, fmt.Errorf("%w: %s is %d bytes (limit %d)", ErrTooLarge, t.Rel, fi.Size(), maxNoteBytes) + defer func() { _ = bfs.Close() }() + b, err := bfs.ReadRegularMax(t.Rel, maxNoteBytes) + if errors.Is(err, brainfs.ErrTooLarge) { + return nil, fmt.Errorf("%w: %s (limit %d)", ErrTooLarge, t.Rel, maxNoteBytes) } - b, err := os.ReadFile(t.Abs) if err != nil { return nil, fmt.Errorf("notes: read %s: %w", t.Rel, err) } diff --git a/ingestion/internal/notes/read_root_test.go b/ingestion/internal/notes/read_root_test.go new file mode 100644 index 0000000..8e87a38 --- /dev/null +++ b/ingestion/internal/notes/read_root_test.go @@ -0,0 +1,31 @@ +package notes + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs/brainfstest" +) + +// hyperguild#157 slice 2: ReadScoped (brain_read) Lstat-checked the final +// component, then reopened by name with os.ReadFile: a swap after the check +// was followed, and a FIFO blocked the read. It now reads through brainfs. +func TestReadScopedIsConfinedToRegularFilesInTheBrain(t *testing.T) { + brain := t.TempDir() + facts := filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + p := filepath.Join(facts, "r.md") + read := func() string { + b, _ := ReadScoped(brain, "wiki/w/facts/r.md", ScopeNotes) + return string(b) + } + require.NoError(t, os.WriteFile(p, []byte("inside"), 0o644)) + assert.Equal(t, "inside", read()) + brainfstest.AssertNoLeak(t, p, "inside", 1500*time.Millisecond, read) + brainfstest.AssertNoHang(t, p, "inside", 1500*time.Millisecond, 6*time.Second, func() { _ = read() }) +}