ReadRegular, ReadRegularMax and WriteRegular require a clean, local path and a real directory at every level above the file, so an in-brain directory symlink (or a ".." after one) can no longer redirect brain_update, brain_tunnel, brain_read/get or the other readers elsewhere in the brain. session.Append opens through an os.Root on the brain dir, refuses a non-directory sessions dir, and writes only after confirming the log path is the regular file it opened. Fixes #173. Adversarial review: BLOCKED, PASS. Follow-up: #188 (brain_write create path). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Mathias <mthbqv@gmail.com>
This commit was merged in pull request #187.
This commit is contained in:
@@ -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
|
||||
@@ -90,3 +93,85 @@ 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")
|
||||
}
|
||||
|
||||
// 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")
|
||||
}
|
||||
|
||||
@@ -65,14 +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)
|
||||
}
|
||||
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)
|
||||
// The sessions dir itself must be a real directory, not a symlink that
|
||||
// redirects the log elsewhere (hyperguild#173).
|
||||
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)
|
||||
}
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user