From 87dace37b19b6489209ac8cc7a474cde312cdd1a Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 20:54:00 +0200 Subject: [PATCH 1/2] fix(brainfs): refuse a file behind a symlinked directory for reads and writes (#173) os.Root follows an intermediate directory symlink that stays inside the brain. A dir symlink inside wiki/ pointing elsewhere in the brain let brain_update and brain_tunnel rewrite an existing .md there (e.g. under raw/), and session_log followed a symlinked sessions dir. - WriteRegular, ReadRegular and ReadRegularMax require every directory above the file to be a real directory (realDir). Reads too: brain_tunnel read the redirected target, then rewrote the source before the target's write was refused; and brain_read/brain_get could read another part of the brain through such a path. - session.Append refuses a sessions dir that is not a real directory. - A directory swapped in after the check remains the #167 residual. Co-Authored-By: Claude Opus 5.5 --- .../internal/api/update_redirect_test.go | 29 +++++++++++++++++ .../internal/brain/tunnel_redirect_test.go | 32 +++++++++++++++++++ ingestion/internal/brainfs/brainfs.go | 18 +++++++++-- ingestion/internal/brainfs/brainfs_test.go | 25 +++++++++++++++ ingestion/internal/session/id_test.go | 12 +++++++ ingestion/internal/session/session.go | 7 ++++ 6 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 ingestion/internal/api/update_redirect_test.go create mode 100644 ingestion/internal/brain/tunnel_redirect_test.go diff --git a/ingestion/internal/api/update_redirect_test.go b/ingestion/internal/api/update_redirect_test.go new file mode 100644 index 0000000..65241ba --- /dev/null +++ b/ingestion/internal/api/update_redirect_test.go @@ -0,0 +1,29 @@ +package api_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/api" +) + +// hyperguild#173: a directory symlink inside wiki/ that points elsewhere in +// the brain redirected brain_update's write (os.Root follows in-root +// intermediate symlinks). Every directory above the note must be real. +func TestUpdateNoteRefusesANoteBehindASymlinkedDirectory(t *testing.T) { + brain := t.TempDir() + victim := filepath.Join(brain, "raw", "facts", "v.md") + require.NoError(t, os.MkdirAll(filepath.Dir(victim), 0o755)) + require.NoError(t, os.WriteFile(victim, []byte("---\ntitle: v\n---\nuntouched\n"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w"), 0o755)) + 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) + got, _ := os.ReadFile(victim) + assert.Contains(t, string(got), "untouched") +} diff --git a/ingestion/internal/brain/tunnel_redirect_test.go b/ingestion/internal/brain/tunnel_redirect_test.go new file mode 100644 index 0000000..fe57d5b --- /dev/null +++ b/ingestion/internal/brain/tunnel_redirect_test.go @@ -0,0 +1,32 @@ +package brain_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/brain" +) + +// hyperguild#173: brain_tunnel's write followed a directory symlink inside +// wiki/ that points elsewhere in the brain. +func TestWriteTunnelRefusesATargetBehindASymlinkedDirectory(t *testing.T) { + b := t.TempDir() + src := filepath.Join(b, "wiki", "a", "facts", "s.md") + require.NoError(t, os.MkdirAll(filepath.Dir(src), 0o755)) + require.NoError(t, os.WriteFile(src, []byte("x\n"), 0o644)) + victim := filepath.Join(b, "raw", "facts", "v.md") + require.NoError(t, os.MkdirAll(filepath.Dir(victim), 0o755)) + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + 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")) + 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") +} diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index 7496a74..92ea8e3 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -128,6 +128,9 @@ 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 { + return nil, err + } fi, err := b.root.Lstat(rel) if err != nil { return nil, err @@ -154,6 +157,9 @@ 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 { + return nil, err + } fi, err := b.root.Lstat(rel) if err != nil { return nil, err @@ -256,10 +262,18 @@ func (b *Brain) WalkRegular(relDir string, fn func(rel string) error, onErr func // O_NOFOLLOW (verified on go1.27), so the path is Lstat-ed through the root // first and the opened handle must be that same file (os.SameFile), which // also catches a swap between the two. O_NONBLOCK keeps a FIFO from blocking -// the open. Intermediate in-brain symlinks are still followed (os.Root keeps -// the write inside the brain; hyperguild#167). +// the open. Every directory above the file must be a real directory +// (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) + // 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 + // the #167 residual. + if err := b.realDir(path.Dir(rel)); err != nil { + return err + } lfi, err := b.root.Lstat(rel) if err != nil { return err diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index d964f25..b8565b7 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -326,3 +326,28 @@ func TestReadDirDirsListsOnlyRealSubdirsAndRefusesSymlinkedComponents(t *testing _, err = b.ReadDirDirs("alias") assert.ErrorIs(t, err, ErrNotRegular) } + +// hyperguild#173: a file reached through a symlinked directory inside the +// brain is refused for reads and writes, so a caller-supplied path cannot be +// redirected to another part of the brain. +func TestReadAndWriteRegularRefuseAFileBehindASymlinkedDirectory(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw"), 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"), filepath.Join(dir, "wiki", "alias"))) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + + _, err = b.ReadRegular("wiki/alias/v.md") + assert.ErrorIs(t, err, ErrNotRegular) + _, err = b.ReadRegularMax("wiki/alias/v.md", 100) + assert.ErrorIs(t, err, ErrNotRegular) + assert.ErrorIs(t, b.WriteRegular("wiki/alias/v.md", []byte("x")), ErrNotRegular) + got, _ := os.ReadFile(filepath.Join(dir, "raw", "v.md")) + assert.Equal(t, "untouched", string(got)) + got, err = b.ReadRegular("raw/v.md") + require.NoError(t, err, "positive control: the real path still reads") + assert.Equal(t, "untouched", string(got)) +} diff --git a/ingestion/internal/session/id_test.go b/ingestion/internal/session/id_test.go index fe06f21..ee86db3 100644 --- a/ingestion/internal/session/id_test.go +++ b/ingestion/internal/session/id_test.go @@ -90,3 +90,15 @@ func TestAppendReturnsPromptlyOnAFIFOLogWithNoReader(t *testing.T) { t.Fatal("Append blocked on a FIFO log with no reader") } } + +// hyperguild#173: session_log followed a symlinked sessions dir. +func TestAppendRefusesASymlinkedSessionsDir(t *testing.T) { + root := t.TempDir() + elsewhere := filepath.Join(root, "elsewhere") + require.NoError(t, os.MkdirAll(elsewhere, 0o755)) + dir := filepath.Join(root, "sessions") + require.NoError(t, os.Symlink("elsewhere", dir)) + assert.Error(t, Append(dir, "s", Entry{SessionID: "s"})) + ents, _ := os.ReadDir(elsewhere) + assert.Empty(t, ents, "a log was created through the symlinked dir") +} diff --git a/ingestion/internal/session/session.go b/ingestion/internal/session/session.go index 878d425..656da92 100644 --- a/ingestion/internal/session/session.go +++ b/ingestion/internal/session/session.go @@ -68,6 +68,13 @@ func Append(sessionsDir, sessionID string, entry Entry) error { if err := os.MkdirAll(sessionsDir, 0o755); err != nil { 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 { + 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") f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644) if err != nil { -- 2.54.0 From 9a288bea6fea52d3b310c3bc95e3e04996eed8e6 Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 21:08:47 +0200 Subject: [PATCH 2/2] 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 --- ingestion/go.mod | 2 +- .../internal/api/update_redirect_test.go | 9 ++- .../internal/brain/tunnel_redirect_test.go | 8 +- ingestion/internal/brainfs/brainfs.go | 25 ++++++- ingestion/internal/brainfs/brainfs_test.go | 25 +++++++ ingestion/internal/session/id_test.go | 73 +++++++++++++++++++ ingestion/internal/session/session.go | 29 +++++++- 7 files changed, 161 insertions(+), 10 deletions(-) diff --git a/ingestion/go.mod b/ingestion/go.mod index 5e9f7b2..ac4eea7 100644 --- a/ingestion/go.mod +++ b/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 ) diff --git a/ingestion/internal/api/update_redirect_test.go b/ingestion/internal/api/update_redirect_test.go index 65241ba..6d798bd 100644 --- a/ingestion/internal/api/update_redirect_test.go +++ b/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") } diff --git a/ingestion/internal/brain/tunnel_redirect_test.go b/ingestion/internal/brain/tunnel_redirect_test.go index fe57d5b..caf5cdd 100644 --- a/ingestion/internal/brain/tunnel_redirect_test.go +++ b/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") } diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index 92ea8e3..97bf8aa 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/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 diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index b8565b7..8935356 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/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)) +} diff --git a/ingestion/internal/session/id_test.go b/ingestion/internal/session/id_test.go index ee86db3..e7aeae8 100644 --- a/ingestion/internal/session/id_test.go +++ b/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") +} diff --git a/ingestion/internal/session/session.go b/ingestion/internal/session/session.go index 656da92..901d555 100644 --- a/ingestion/internal/session/session.go +++ b/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") - f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0o644) + logRel := name + "/" + sessionID + ".jsonl" + 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 { -- 2.54.0