fix(brainfs,session): clean paths only; session logs open through the brain root (#173)

Review round 1 on PR #187:

- realDir checked the cleaned parent while Lstat/OpenFile used the raw
  path; os.Root resolves ".." after following a symlink, so a ".." placed
  after a symlinked directory passed the check and redirected the read or
  write. ReadRegular, ReadRegularMax and WriteRegular now refuse a path
  that is not clean and local (ErrNotRegular). No current entry point
  reached it (callers clean first); the primitive no longer relies on that.
- session.Append checked the sessions dir with Lstat, then opened by plain
  path: a symlink swapped in between created logs outside the brain
  (29k in the reviewer's RENAME_EXCHANGE race). It now opens through an
  os.Root on the sessions dir's parent, so a swap can only redirect inside
  the brain (#167), and, since os.Root follows an in-root final symlink
  even with O_NOFOLLOW, writes only after confirming the path is the
  regular file it opened (Lstat + os.SameFile).
- Endpoint tests assert ErrorIs(brainfs.ErrNotRegular) and have positive
  controls.

golang.org/x/sys moves from indirect to direct (unix.Renameat2 in the
session race test); it was already in the module graph.

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

ingestion/go.mod

@@ -68,7 +68,7 @@ require (
 	github.com/segmentio/asm v1.2.0 // indirect
 	golang.org/x/crypto v0.51.0 // indirect
 	golang.org/x/sync v0.21.0 // indirect
-	golang.org/x/sys v0.45.0 // indirect
+	golang.org/x/sys v0.45.0
 	golang.org/x/text v0.39.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1
 )

ingestion/internal/api/update_redirect_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/mathiasbq/hyperguild/ingestion/internal/api"
+	"github.com/mathiasbq/hyperguild/ingestion/internal/brainfs"
 )
 
 // hyperguild#173: a directory symlink inside wiki/ that points elsewhere in
@@ -23,7 +24,13 @@ func TestUpdateNoteRefusesANoteBehindASymlinkedDirectory(t *testing.T) {
 	require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "facts"), filepath.Join(brain, "wiki", "w", "alias")))
 
 	_, _, _, err := api.UpdateNote(brain, api.UpdateNoteOptions{Path: "wiki/w/alias/v.md", Content: "replaced"})
-	assert.Error(t, err)
+	assert.ErrorIs(t, err, brainfs.ErrNotRegular)
 	got, _ := os.ReadFile(victim)
 	assert.Contains(t, string(got), "untouched")
+
+	real := filepath.Join(brain, "wiki", "w", "facts", "r.md")
+	require.NoError(t, os.MkdirAll(filepath.Dir(real), 0o755))
+	require.NoError(t, os.WriteFile(real, []byte("---\ntitle: r\n---\nold\n"), 0o644))
+	_, _, _, err = api.UpdateNote(brain, api.UpdateNoteOptions{Path: "wiki/w/facts/r.md", Content: "new"})
+	require.NoError(t, err, "positive control: a note in a real directory is updated")
 }

ingestion/internal/brain/tunnel_redirect_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/mathiasbq/hyperguild/ingestion/internal/brain"
+	"github.com/mathiasbq/hyperguild/ingestion/internal/brainfs"
 )
 
 // hyperguild#173: brain_tunnel's write followed a directory symlink inside
@@ -24,9 +25,14 @@ func TestWriteTunnelRefusesATargetBehindASymlinkedDirectory(t *testing.T) {
 	require.NoError(t, os.MkdirAll(filepath.Join(b, "wiki", "b"), 0o755))
 	require.NoError(t, os.Symlink(filepath.Join("..", "..", "raw", "facts"), filepath.Join(b, "wiki", "b", "facts")))
 
-	assert.Error(t, brain.WriteTunnel(b, "wiki/a/facts/s.md", "wiki/b/facts/v.md"))
+	assert.ErrorIs(t, brain.WriteTunnel(b, "wiki/a/facts/s.md", "wiki/b/facts/v.md"), brainfs.ErrNotRegular)
 	got, _ := os.ReadFile(victim)
 	assert.Equal(t, "untouched\n", string(got))
 	got, _ = os.ReadFile(src)
 	assert.Equal(t, "x\n", string(got), "the source is not rewritten either")
+
+	real := filepath.Join(b, "wiki", "c", "facts", "t.md")
+	require.NoError(t, os.MkdirAll(filepath.Dir(real), 0o755))
+	require.NoError(t, os.WriteFile(real, []byte("t\n"), 0o644))
+	require.NoError(t, brain.WriteTunnel(b, "wiki/a/facts/s.md", "wiki/c/facts/t.md"), "positive control: a tunnel between real directories is written")
 }

ingestion/internal/brainfs/brainfs.go

@@ -104,6 +104,17 @@ func (b *Brain) ReadDirRegular(relDir string) ([]string, error) {
 	return names, nil
 }
 
+// cleanRel refuses a path that is not clean and local. realDir checks the
+// cleaned parent, so the I/O must use that same path: os.Root resolves ".."
+// after following a symlink, and a ".." after a symlinked directory would
+// otherwise pass the check and redirect the read or write.
+func cleanRel(rel string) error {
+	if rel == "" || path.Clean(rel) != rel || !filepath.IsLocal(rel) || strings.Contains(rel, "\\") {
+		return fmt.Errorf("%q: not a clean brain-relative path: %w", rel, ErrNotRegular)
+	}
+	return nil
+}
+
 // ReadDirDirs lists the real subdirectories directly in a brain-relative
 // directory, by name. Every component of relDir must be a real directory
 // (ErrNotRegular otherwise); symlinks to directories are left out.
@@ -128,7 +139,10 @@ func (b *Brain) ReadDirDirs(relDir string) ([]string, error) {
 // 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) {
-	if err := b.realDir(path.Dir(filepath.ToSlash(rel))); err != nil {
+	if err := cleanRel(rel); err != nil {
+		return nil, err
+	}
+	if err := b.realDir(path.Dir(rel)); err != nil {
 		return nil, err
 	}
 	fi, err := b.root.Lstat(rel)
@@ -157,7 +171,10 @@ func (b *Brain) readOpened(rel string) ([]byte, error) { return b.readOpenedMax(
 // 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) {
-	if err := b.realDir(path.Dir(filepath.ToSlash(rel))); err != nil {
+	if err := cleanRel(rel); err != nil {
+		return nil, err
+	}
+	if err := b.realDir(path.Dir(rel)); err != nil {
 		return nil, err
 	}
 	fi, err := b.root.Lstat(rel)
@@ -266,7 +283,9 @@ func (b *Brain) WalkRegular(relDir string, fn func(rel string) error, onErr func
 // (hyperguild#173); a directory swapped in after that check is the #167
 // residual.
 func (b *Brain) WriteRegular(rel string, data []byte) error {
-	rel = filepath.ToSlash(rel)
+	if err := cleanRel(rel); err != nil {
+		return err
+	}
 	// Every directory above the file must be real: os.Root follows an
 	// in-brain intermediate symlink, which would redirect the write
 	// elsewhere in the brain (hyperguild#173). A swap after this check is

ingestion/internal/brainfs/brainfs_test.go

@@ -351,3 +351,28 @@ func TestReadAndWriteRegularRefuseAFileBehindASymlinkedDirectory(t *testing.T) {
 	require.NoError(t, err, "positive control: the real path still reads")
 	assert.Equal(t, "untouched", string(got))
 }
+
+// realDir checks the cleaned parent, so the I/O must use that same clean
+// path: os.Root resolves ".." after following a symlink, and a ".." placed
+// after a symlinked directory would otherwise pass the check and redirect.
+func TestReadAndWriteRegularRefuseAPathThatIsNotClean(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw", "facts"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "raw", "v.md"), []byte("untouched"), 0o644))
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "wiki"), 0o755))
+	require.NoError(t, os.Symlink(filepath.Join("..", "raw", "facts"), filepath.Join(dir, "wiki", "alias")))
+	b, err := Open(dir)
+	require.NoError(t, err)
+	defer func() { _ = b.Close() }()
+
+	for _, rel := range []string{"wiki/alias/../v.md", "./raw/v.md", "raw//v.md", "/raw/v.md"} {
+		got, err := b.ReadRegular(rel)
+		assert.ErrorIs(t, err, ErrNotRegular, rel)
+		assert.NotContains(t, string(got), "untouched", rel)
+		_, err = b.ReadRegularMax(rel, 100)
+		assert.ErrorIs(t, err, ErrNotRegular, rel)
+		assert.ErrorIs(t, b.WriteRegular(rel, []byte("OVERWRITTEN")), ErrNotRegular, rel)
+	}
+	got, _ := os.ReadFile(filepath.Join(dir, "raw", "v.md"))
+	assert.Equal(t, "untouched", string(got))
+}

ingestion/internal/session/id_test.go

@@ -2,14 +2,17 @@ package session
 
 import (
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
+	"sync/atomic"
 	"syscall"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
 )
 
 // session_log passed the caller's session_id straight into
@@ -102,3 +105,73 @@ func TestAppendRefusesASymlinkedSessionsDir(t *testing.T) {
 	ents, _ := os.ReadDir(elsewhere)
 	assert.Empty(t, ents, "a log was created through the symlinked dir")
 }
+
+// The sessions dir is swapped (RENAME_EXCHANGE) with a symlink to a
+// directory outside the brain while Append runs. Append opens through an
+// os.Root on the sessions dir's parent, so a swap can redirect it only
+// inside that parent (the #167 residual), never outside it.
+func TestAppendNeverCreatesALogOutsideTheParentUnderADirSwapRace(t *testing.T) {
+	brain := t.TempDir()
+	outside := t.TempDir()
+	dir := filepath.Join(brain, "sessions")
+	other := filepath.Join(brain, "sessions.link")
+	require.NoError(t, os.MkdirAll(dir, 0o755))
+	require.NoError(t, os.Symlink(outside, other))
+	var stop atomic.Bool
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		for !stop.Load() {
+			_ = unix.Renameat2(unix.AT_FDCWD, dir, unix.AT_FDCWD, other, unix.RENAME_EXCHANGE)
+		}
+	}()
+	ok := 0
+	for i := 0; i < 20000; i++ {
+		if Append(dir, fmt.Sprintf("s%d", i), Entry{SessionID: "s"}) == nil {
+			ok++
+		}
+	}
+	stop.Store(true)
+	<-done
+	ents, _ := os.ReadDir(outside)
+	assert.Empty(t, ents, "logs were created outside the brain")
+	assert.Positive(t, ok, "positive control: appends to the real dir succeed")
+}
+
+// The log file is swapped between a regular file and a symlink to another
+// file in the brain while Append runs: Append writes only after confirming
+// the path is the regular file it opened, so the other file never grows.
+func TestAppendNeverWritesThroughALogSwappedForASymlink(t *testing.T) {
+	brain := t.TempDir()
+	dir := filepath.Join(brain, "sessions")
+	require.NoError(t, os.MkdirAll(dir, 0o755))
+	victim := filepath.Join(brain, "victim.md")
+	require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644))
+	log := filepath.Join(dir, "s.jsonl")
+	var stop atomic.Bool
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		tmp := log + ".swap"
+		for i := 0; !stop.Load(); i++ {
+			_ = os.Remove(tmp)
+			if i%2 == 0 {
+				_ = os.WriteFile(tmp, nil, 0o644)
+			} else {
+				_ = os.Symlink(filepath.Join("..", "victim.md"), tmp)
+			}
+			_ = os.Rename(tmp, log)
+		}
+	}()
+	ok := 0
+	for i := 0; i < 20000; i++ {
+		if Append(dir, "s", Entry{SessionID: "s"}) == nil {
+			ok++
+		}
+	}
+	stop.Store(true)
+	<-done
+	got, _ := os.ReadFile(victim)
+	assert.Equal(t, "untouched\n", string(got), "a log line was written through the swapped-in symlink")
+	assert.Positive(t, ok, "positive control: appends to the regular log succeed")
+}

ingestion/internal/session/session.go

@@ -65,21 +65,42 @@ func Append(sessionsDir, sessionID string, entry Entry) error {
 	if err := checkID(sessionID); err != nil {
 		return err
 	}
-	if err := os.MkdirAll(sessionsDir, 0o755); err != nil {
+	// Opened through an os.Root on the sessions dir's parent (the brain dir),
+	// so a sessions dir swapped for a symlink after the check below can
+	// redirect the log only inside that parent, never outside it (#173; the
+	// in-brain swap is the #167 residual).
+	parentDir, name := filepath.Dir(sessionsDir), filepath.Base(sessionsDir)
+	if err := os.MkdirAll(parentDir, 0o755); err != nil {
+		return fmt.Errorf("create sessions parent: %w", err)
+	}
+	root, err := os.OpenRoot(parentDir)
+	if err != nil {
+		return fmt.Errorf("open sessions parent: %w", err)
+	}
+	defer func() { _ = root.Close() }()
+	if err := root.Mkdir(name, 0o755); err != nil && !errors.Is(err, fs.ErrExist) {
 		return fmt.Errorf("create sessions dir: %w", err)
 	}
 	// The sessions dir itself must be a real directory, not a symlink that
 	// redirects the log elsewhere (hyperguild#173).
-	if fi, err := os.Lstat(sessionsDir); err != nil {
+	if fi, err := root.Lstat(name); err != nil {
 		return fmt.Errorf("stat sessions dir: %w", err)
 	} else if !fi.IsDir() {
 		return fmt.Errorf("sessions dir %s is not a real directory", sessionsDir)
 	}
-	path := filepath.Join(sessionsDir, sessionID+".jsonl")
+	logRel := name + "/" + sessionID + ".jsonl"
-	f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644)
+	f, err := root.OpenFile(logRel, os.O_CREATE|os.O_APPEND|os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644)
 	if err != nil {
 		return fmt.Errorf("open session log: %w", err)
 	}
+	// os.Root follows an in-root final symlink even with O_NOFOLLOW, so
+	// confirm, before writing, that the path is the regular file we opened.
+	hfi, herr := f.Stat()
+	lfi, lerr := root.Lstat(logRel)
+	if herr != nil || lerr != nil || !lfi.Mode().IsRegular() || !os.SameFile(lfi, hfi) {
+		_ = f.Close()
+		return fmt.Errorf("session log %s is not a regular file", logRel)
+	}
 
 	line, err := json.Marshal(entry)
 	if err != nil {