From 02cecb25b10c2cfb1f857bd222e5d457d5ef7d8d Mon Sep 17 00:00:00 2001 From: Mathias Date: Sun, 4 Oct 2026 00:33:36 +0200 Subject: [PATCH] fix(brainfs): bound the read in ReadRegularMax; honest test names (#157) Review round 1 on PR #165: - ReadRegularMax checked the size on the opened handle, then read with an unbounded io.ReadAll, so a file growing after the check returned past the limit. The read now goes through io.LimitReader(max+1). A test seam grows the file between the check and the read and counts bytes consumed. - New: ReadScoped returns ErrTooLarge for a note over maxNoteBytes; red if readContained drops the limit. - notes.Target carries the brain root instead of deriving it by TrimSuffix. - Rule 9: reader tests renamed to RefusesSymlinksAndSurvivesSwapRaces; ReadNote test has a positive control; brainfstest comment states what the harness observes and what it does not. Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/api/readnote_root_test.go | 7 +++- ingestion/internal/brainfs/brainfs.go | 21 ++++++++++- ingestion/internal/brainfs/brainfs_test.go | 36 +++++++++++++++++++ .../brainfs/brainfstest/brainfstest.go | 11 +++--- .../internal/mcp/readexcerpt_root_test.go | 2 +- ingestion/internal/notes/edit.go | 3 +- ingestion/internal/notes/path.go | 7 ++-- ingestion/internal/notes/read_root_test.go | 11 +++++- 8 files changed, 84 insertions(+), 14 deletions(-) diff --git a/ingestion/internal/api/readnote_root_test.go b/ingestion/internal/api/readnote_root_test.go index 0a7353d..94fedff 100644 --- a/ingestion/internal/api/readnote_root_test.go +++ b/ingestion/internal/api/readnote_root_test.go @@ -15,7 +15,7 @@ import ( // hyperguild#157 slice 2: ReadNote (brain_get, read-after-write) read by // path; it now reads through brainfs. -func TestReadNoteIsConfinedToRegularFilesInTheBrain(t *testing.T) { +func TestReadNoteRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) { brain := t.TempDir() facts := filepath.Join(brain, "wiki", "w", "facts") require.NoError(t, os.MkdirAll(facts, 0o755)) @@ -25,6 +25,11 @@ func TestReadNoteIsConfinedToRegularFilesInTheBrain(t *testing.T) { assert.NotContains(t, body, brainfstest.Sentinel) p := filepath.Join(facts, "r.md") + require.NoError(t, os.WriteFile(p, []byte("---\nx: y\n---\ninside"), 0o644)) + fm, body, _, err := api.ReadNote(brain, "wiki/w/facts/r.md") + require.NoError(t, err) + assert.Equal(t, "y", fm["x"]) + assert.Contains(t, body, "inside") read := func() string { fm, body, _, _ := api.ReadNote(brain, "wiki/w/facts/r.md") s := body diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index a182238..250244f 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -108,9 +108,28 @@ func (b *Brain) readOpenedMax(rel string, max int64) ([]byte, error) { 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) + var r io.Reader = f + if wrapAfterStat != nil { + r = wrapAfterStat(r) + } + if max < 0 { + return io.ReadAll(r) + } + // The handle's size can grow after the check; bound the read itself. + data, err := io.ReadAll(io.LimitReader(r, max+1)) + if err != nil { + return nil, err + } + if int64(len(data)) > max { + return nil, fmt.Errorf("%s grew past %d bytes while read: %w", rel, max, ErrTooLarge) + } + return data, nil } +// wrapAfterStat is a test seam: it runs between the size check and the read, +// and wraps the reader the read consumes. +var wrapAfterStat func(io.Reader) io.Reader + // 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 diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index 3c48a00..8a09e01 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -2,6 +2,7 @@ package brainfs import ( "errors" + "io" "os" "path/filepath" "syscall" @@ -103,3 +104,38 @@ func TestReadRegularMaxRefusesAFileOverTheLimit(t *testing.T) { require.NoError(t, err) assert.Len(t, got, 100) } + +func TestReadRegularMaxRefusesAndStopsReadingAFileThatGrowsPastTheLimit(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "grow.md") + require.NoError(t, os.WriteFile(p, make([]byte, 10), 0o644)) + consumed := &countingReader{} + wrapAfterStat = func(r io.Reader) io.Reader { + f, err := os.OpenFile(p, os.O_WRONLY|os.O_APPEND, 0) + require.NoError(t, err) + _, err = f.Write(make([]byte, 1000)) + require.NoError(t, err) + require.NoError(t, f.Close()) + consumed.r = r + return consumed + } + t.Cleanup(func() { wrapAfterStat = nil }) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + got, err := b.ReadRegularMax("grow.md", 100) + assert.True(t, errors.Is(err, ErrTooLarge), "got %v", err) + assert.Nil(t, got) + assert.LessOrEqual(t, consumed.n, 101, "read past max+1 bytes") +} + +type countingReader struct { + r io.Reader + n int +} + +func (c *countingReader) Read(p []byte) (int, error) { + n, err := c.r.Read(p) + c.n += n + return n, err +} diff --git a/ingestion/internal/brainfs/brainfstest/brainfstest.go b/ingestion/internal/brainfs/brainfstest/brainfstest.go index 56f6bc8..3cf4e14 100644 --- a/ingestion/internal/brainfs/brainfstest/brainfstest.go +++ b/ingestion/internal/brainfs/brainfstest/brainfstest.go @@ -1,8 +1,9 @@ -// 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 is the shared harness for hyperguild#157's swap-race +// 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. +// It observes two things: no sentinel byte from the outside file reaches the +// reader's output, and the reader returns within a timeout. It does not cover +// hard links, other paths into the reader, or races it does not generate. package brainfstest import ( diff --git a/ingestion/internal/mcp/readexcerpt_root_test.go b/ingestion/internal/mcp/readexcerpt_root_test.go index bb26d40..9ba1748 100644 --- a/ingestion/internal/mcp/readexcerpt_root_test.go +++ b/ingestion/internal/mcp/readexcerpt_root_test.go @@ -15,7 +15,7 @@ import ( // 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) { +func TestReadExcerptRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) { brain := t.TempDir() facts := filepath.Join(brain, "wiki", "w", "facts") require.NoError(t, os.MkdirAll(facts, 0o755)) diff --git a/ingestion/internal/notes/edit.go b/ingestion/internal/notes/edit.go index 1bf4f9b..c289db8 100644 --- a/ingestion/internal/notes/edit.go +++ b/ingestion/internal/notes/edit.go @@ -75,8 +75,7 @@ func readContained(t Target) ([]byte, error) { // 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) + bfs, err := brainfs.Open(t.Brain) if err != nil { return nil, fmt.Errorf("notes: %w", err) } diff --git a/ingestion/internal/notes/path.go b/ingestion/internal/notes/path.go index e54a026..42ac5a0 100644 --- a/ingestion/internal/notes/path.go +++ b/ingestion/internal/notes/path.go @@ -58,8 +58,9 @@ var ( // Target is a resolved, contained path. type Target struct { - Rel string // brain-relative, cleaned - Abs string // absolute, symlink-checked + Rel string // brain-relative, cleaned + Abs string // absolute, symlink-checked + Brain string // the brain root Rel is relative to } // Resolve canonicalises path, then checks it against the scope's allowlist, @@ -109,7 +110,7 @@ func Resolve(brainDir, path string, scope Scope) (Target, error) { return Target{}, err } - return Target{Rel: clean, Abs: abs}, nil + return Target{Rel: clean, Abs: abs, Brain: brainDir}, nil } // allow enforces the scope's allowlist. The two scopes are DISJOINT, not diff --git a/ingestion/internal/notes/read_root_test.go b/ingestion/internal/notes/read_root_test.go index 8e87a38..a699fd4 100644 --- a/ingestion/internal/notes/read_root_test.go +++ b/ingestion/internal/notes/read_root_test.go @@ -15,7 +15,7 @@ import ( // 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) { +func TestReadScopedRefusesSymlinksAndSurvivesSwapRaces(t *testing.T) { brain := t.TempDir() facts := filepath.Join(brain, "wiki", "w", "facts") require.NoError(t, os.MkdirAll(facts, 0o755)) @@ -29,3 +29,12 @@ func TestReadScopedIsConfinedToRegularFilesInTheBrain(t *testing.T) { brainfstest.AssertNoLeak(t, p, "inside", 1500*time.Millisecond, read) brainfstest.AssertNoHang(t, p, "inside", 1500*time.Millisecond, 6*time.Second, func() { _ = read() }) } + +func TestReadScopedRefusesANoteOverMaxNoteBytes(t *testing.T) { + brain := t.TempDir() + facts := filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(facts, "big.md"), make([]byte, maxNoteBytes+1), 0o644)) + _, err := ReadScoped(brain, "wiki/w/facts/big.md", ScopeNotes) + assert.ErrorIs(t, err, ErrTooLarge) +}