diff --git a/ingestion/internal/api/update.go b/ingestion/internal/api/update.go index 9301068..7aee8af 100644 --- a/ingestion/internal/api/update.go +++ b/ingestion/internal/api/update.go @@ -3,15 +3,18 @@ package api import ( "crypto/sha256" "encoding/hex" + "errors" "fmt" - "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" + "io/fs" "os" "path/filepath" "strings" "time" "github.com/mathiasbq/hyperguild/ingestion/internal/brain" + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" "github.com/mathiasbq/hyperguild/ingestion/internal/claim" + "github.com/mathiasbq/hyperguild/ingestion/internal/notes" ) // ContentHash returns the lowercase hex sha256 of b. It is the note's @@ -91,14 +94,23 @@ func UpdateNote(brainDir string, opts UpdateNoteOptions) (relPath, contentHash, rel = filepath.ToSlash(rel) } - rel, abs, err := resolveWithin(brainDir, rel) + // The path is caller-supplied (brain_update): resolve it as brain_edit + // does, knowledge/ and wiki/ notes only, and read and rewrite it through + // the brain root, never through a symlink (hyperguild#169). + t, err := notes.Resolve(brainDir, rel, notes.ScopeNotes) if err != nil { return "", "", "", err } - - prior, err := os.ReadFile(abs) + rel = t.Rel + bfs, err := brainfs.Open(brainDir) if err != nil { - if os.IsNotExist(err) { + return "", "", "", fmt.Errorf("open brain: %w", err) + } + defer func() { _ = bfs.Close() }() + + prior, err := bfs.ReadRegular(rel) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { return "", "", "", fmt.Errorf("note %q does not exist: use brain_write to create", rel) } return "", "", "", fmt.Errorf("read target: %w", err) @@ -164,7 +176,7 @@ func UpdateNote(brainDir string, opts UpdateNoteOptions) (relPath, contentHash, return "", "", "", err } out := []byte(stamped) - if err := os.WriteFile(abs, out, 0o644); err != nil { + if err := bfs.WriteRegular(rel, out); err != nil { return "", "", "", fmt.Errorf("write: %w", err) } return rel, ContentHash(out), priorHash, nil diff --git a/ingestion/internal/api/update_scope_test.go b/ingestion/internal/api/update_scope_test.go new file mode 100644 index 0000000..12c9141 --- /dev/null +++ b/ingestion/internal/api/update_scope_test.go @@ -0,0 +1,112 @@ +package api_test + +import ( + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/api" +) + +// brain_update passed the caller's path through resolveWithin, a lexical +// containment check only: any file in the brain could be superseded +// (classification.yaml, raw/, sessions/), and a final symlink was written +// through. It now resolves like brain_edit: knowledge/ and wiki/ notes only, +// read and rewritten through the brain root. +func TestUpdateNoteRefusesPathsOutsideKnowledgeAndWikiNotes(t *testing.T) { + brain := t.TempDir() + files := map[string]string{ + "classification.yaml": "levels: {}\n", + "raw/r.md": "raw\n", + "sessions/s.jsonl": "{}\n", + "wiki/w/facts/ok.md": "---\ntitle: ok\n---\nold\n", + "knowledge/k.md": "---\ntitle: k\n---\nold\n", + "wiki/w/facts/x.txt": "txt\n", + } + for rel, body := range files { + p := filepath.Join(brain, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, []byte(body), 0o644)) + } + for _, rel := range []string{"classification.yaml", "raw/r.md", "sessions/s.jsonl", "wiki/w/facts/x.txt", "wiki/w/../../raw/r.md"} { + _, _, _, err := api.UpdateNote(brain, api.UpdateNoteOptions{Path: rel, Content: "replaced"}) + assert.Error(t, err, rel) + } + for rel, body := range files { + if rel == "wiki/w/facts/ok.md" || rel == "knowledge/k.md" { + continue + } + got, err := os.ReadFile(filepath.Join(brain, filepath.FromSlash(rel))) + require.NoError(t, err) + assert.Equal(t, body, string(got), rel) + } + for _, rel := range []string{"wiki/w/facts/ok.md", "knowledge/k.md"} { + _, _, _, err := api.UpdateNote(brain, api.UpdateNoteOptions{Path: rel, Content: "new body"}) + require.NoError(t, err, "positive control: %s", rel) + got, _ := os.ReadFile(filepath.Join(brain, filepath.FromSlash(rel))) + assert.Contains(t, string(got), "new body") + } +} + +func TestUpdateNoteDoesNotWriteThroughASymlinkedNote(t *testing.T) { + brain := t.TempDir() + facts := filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + victim := filepath.Join(brain, "raw", "victim.md") + require.NoError(t, os.MkdirAll(filepath.Dir(victim), 0o755)) + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + require.NoError(t, os.Symlink(filepath.Join("..", "..", "..", "raw", "victim.md"), filepath.Join(facts, "l.md"))) + _, _, _, err := api.UpdateNote(brain, api.UpdateNoteOptions{Path: "wiki/w/facts/l.md", Content: "replaced"}) + assert.Error(t, err) + got, _ := os.ReadFile(victim) + assert.Equal(t, "untouched\n", string(got)) +} + +// The note is swapped between a regular file and an in-brain symlink to a +// raw/ file while UpdateNote runs: the read and the write each refuse a +// symlink, so the raw/ file is never rewritten. +func TestUpdateNoteNeverRewritesASymlinkSwappedInForTheNote(t *testing.T) { + brain := t.TempDir() + facts := filepath.Join(brain, "wiki", "w", "facts") + require.NoError(t, os.MkdirAll(facts, 0o755)) + victim := filepath.Join(brain, "raw", "victim.md") + require.NoError(t, os.MkdirAll(filepath.Dir(victim), 0o755)) + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + note := filepath.Join(facts, "n.md") + body := "---\ntitle: n\n---\nold\n" + require.NoError(t, os.WriteFile(note, []byte(body), 0o644)) + + var stop atomic.Bool + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + tmp := note + ".swap" + for i := 0; !stop.Load(); i++ { + _ = os.Remove(tmp) + if i%2 == 0 { + _ = os.WriteFile(tmp, []byte(body), 0o644) + } else { + _ = os.Symlink(filepath.Join("..", "..", "..", "raw", "victim.md"), tmp) + } + _ = os.Rename(tmp, note) + } + }() + ok := 0 + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); { + if _, _, _, err := api.UpdateNote(brain, api.UpdateNoteOptions{Path: "wiki/w/facts/n.md", Content: "new"}); err == nil { + ok++ + } + } + stop.Store(true) + wg.Wait() + got, _ := os.ReadFile(victim) + assert.Equal(t, "untouched\n", string(got)) + assert.Positive(t, ok, "positive control: some updates reach the regular note") +} diff --git a/ingestion/internal/brain/tunnel.go b/ingestion/internal/brain/tunnel.go index e1ed3e6..f445e0f 100644 --- a/ingestion/internal/brain/tunnel.go +++ b/ingestion/internal/brain/tunnel.go @@ -4,9 +4,12 @@ import ( "bufio" "fmt" "os" + "path" "path/filepath" "strings" "time" + + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" ) // seeAlsoHeader is the markdown heading used to group cross-wing links. @@ -197,6 +200,12 @@ func fileExists(p string) bool { // section headers. Rejects same-wing pairs (a tunnel is by definition // cross-wing) and missing notes. func WriteTunnel(brainDir, sourcePath, targetPath string) error { + if err := checkNotePath(sourcePath); err != nil { + return fmt.Errorf("source: %w", err) + } + if err := checkNotePath(targetPath); err != nil { + return fmt.Errorf("target: %w", err) + } srcWing, err := wingOf(sourcePath) if err != nil { return fmt.Errorf("source: %w", err) @@ -209,20 +218,50 @@ func WriteTunnel(brainDir, sourcePath, targetPath string) error { return fmt.Errorf("tunnel must cross wings; got both in %q", srcWing) } - srcFull := filepath.Join(brainDir, filepath.FromSlash(sourcePath)) - tgtFull := filepath.Join(brainDir, filepath.FromSlash(targetPath)) - if _, err := os.Stat(srcFull); err != nil { + // Both notes are read and rewritten through the brain root, regular + // files only: brain_tunnel passes caller-supplied paths, and the brain is + // a Syncthing folder that can hold symlinks. + bfs, err := brainfs.Open(brainDir) + if err != nil { + return fmt.Errorf("open brain: %w", err) + } + defer func() { _ = bfs.Close() }() + srcContent, err := bfs.ReadRegular(sourcePath) + if err != nil { return fmt.Errorf("source note: %w", err) } - if _, err := os.Stat(tgtFull); err != nil { + tgtContent, err := bfs.ReadRegular(targetPath) + if err != nil { return fmt.Errorf("target note: %w", err) } - if err := appendSeeAlso(srcFull, wikilinkOf(targetPath)); err != nil { + if out, changed, err := withSeeAlso(srcContent, wikilinkOf(targetPath)); err != nil { return fmt.Errorf("update source: %w", err) + } else if changed { + if err := bfs.WriteRegular(sourcePath, out); err != nil { + return fmt.Errorf("update source: %w", err) + } } - if err := appendSeeAlso(tgtFull, wikilinkOf(sourcePath)); err != nil { + if out, changed, err := withSeeAlso(tgtContent, wikilinkOf(sourcePath)); err != nil { return fmt.Errorf("update target: %w", err) + } else if changed { + if err := bfs.WriteRegular(targetPath, out); err != nil { + return fmt.Errorf("update target: %w", err) + } + } + return nil +} + +// checkNotePath accepts only a clean, local wiki///.../.md +// path: no "." or ".." segments, no empty segments, no leading slash. +func checkNotePath(p string) error { + if p == "" || path.Clean(p) != p || !filepath.IsLocal(p) || !strings.HasSuffix(p, ".md") { + return fmt.Errorf("not a clean wiki note path: %q", p) + } + for _, part := range strings.Split(p, "/") { + if part == "" || part == "." || part == ".." { + return fmt.Errorf("not a clean wiki note path: %q", p) + } } return nil } @@ -248,16 +287,13 @@ func wingOf(relPath string) (string, error) { return parts[1], nil } -// appendSeeAlso inserts `- [[link]]` under the file's See also section, -// creating the section if absent. No-op when the link is already present. -func appendSeeAlso(filePath, link string) error { - content, err := os.ReadFile(filePath) - if err != nil { - return err - } +// withSeeAlso returns content with `- [[link]]` under its See also section, +// creating the section if absent. changed is false when the link is already +// present. +func withSeeAlso(content []byte, link string) (out []byte, changed bool, err error) { wikilink := "[[" + link + "]]" if strings.Contains(string(content), wikilink) { - return nil + return content, false, nil } bullet := "- " + wikilink @@ -266,8 +302,7 @@ func appendSeeAlso(filePath, link string) error { // No section yet — append a fresh one. Always emit a trailing // newline so subsequent appends don't merge into the previous line. trimmed := strings.TrimRight(string(content), "\n") - out := trimmed + "\n\n" + seeAlsoHeader + "\n\n" + bullet + "\n" - return os.WriteFile(filePath, []byte(out), 0o644) + return []byte(trimmed + "\n\n" + seeAlsoHeader + "\n\n" + bullet + "\n"), true, nil } // Section exists — splice the bullet in just before the next `## ` @@ -293,12 +328,11 @@ func appendSeeAlso(filePath, link string) error { b.WriteByte('\n') } if err := scanner.Err(); err != nil { - return err + return nil, false, err } if !inserted { // section was the last thing in the file — just append bullet - out := strings.TrimRight(b.String(), "\n") + "\n" + bullet + "\n" - return os.WriteFile(filePath, []byte(out), 0o644) + return []byte(strings.TrimRight(b.String(), "\n") + "\n" + bullet + "\n"), true, nil } - return os.WriteFile(filePath, []byte(b.String()), 0o644) + return []byte(b.String()), true, nil } diff --git a/ingestion/internal/brain/tunnel_root_test.go b/ingestion/internal/brain/tunnel_root_test.go new file mode 100644 index 0000000..7fe05e4 --- /dev/null +++ b/ingestion/internal/brain/tunnel_root_test.go @@ -0,0 +1,141 @@ +package brain_test + +import ( + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/brain" +) + +// brain_tunnel passes caller-supplied source and target paths to WriteTunnel, +// which checked only "wiki//..." and then joined them onto the brain +// dir: ".." segments resolved outside the brain and appendSeeAlso rewrote the +// file there. +func TestWriteTunnelRefusesATargetThatTraversesOutOfTheBrain(t *testing.T) { + brain0 := t.TempDir() + src := filepath.Join(brain0, "wiki", "a", "facts", "s.md") + require.NoError(t, os.MkdirAll(filepath.Dir(src), 0o755)) + require.NoError(t, os.WriteFile(src, []byte("---\ntitle: S\n---\nbody\n"), 0o644)) + victim := filepath.Join(t.TempDir(), "victim.md") + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + + up := strings.Repeat("../", strings.Count(filepath.Clean(brain0), "/")+3) + target := "wiki/b/x/" + up + strings.TrimPrefix(victim, "/") + err := brain.WriteTunnel(brain0, "wiki/a/facts/s.md", target) + assert.Error(t, err) + got, rerr := os.ReadFile(victim) + require.NoError(t, rerr) + assert.Equal(t, "untouched\n", string(got), "a file outside the brain was modified") +} + +func TestWriteTunnelRefusesUncleanOrNonWikiPaths(t *testing.T) { + brain0 := t.TempDir() + for _, p := range []string{"wiki/a/facts/s.md", "wiki/b/facts/t.md", "wiki/b/facts/t.txt", "raw/r.md"} { + require.NoError(t, os.MkdirAll(filepath.Dir(filepath.Join(brain0, p)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(brain0, p), []byte("x\n"), 0o644)) + } + for _, target := range []string{ + "wiki/b/facts/../../../raw/r.md", + "wiki/b/./facts/t.md", + "wiki/b//facts/t.md", + "/wiki/b/facts/t.md", + "wiki/b/facts/t.txt", + "wiki/b/facts/", + } { + assert.Error(t, brain.WriteTunnel(brain0, "wiki/a/facts/s.md", target), target) + } + for _, p := range []string{"raw/r.md", "wiki/b/facts/t.txt"} { + got, err := os.ReadFile(filepath.Join(brain0, p)) + require.NoError(t, err) + assert.Equal(t, "x\n", string(got), p) + } + require.NoError(t, brain.WriteTunnel(brain0, "wiki/a/facts/s.md", "wiki/b/facts/t.md"), "positive control") + got, err := os.ReadFile(filepath.Join(brain0, "wiki", "b", "facts", "t.md")) + require.NoError(t, err) + assert.Contains(t, string(got), "[[a/facts/s]]") +} + +func TestWriteTunnelDoesNotWriteThroughASymlinkedNote(t *testing.T) { + brain0 := t.TempDir() + src := filepath.Join(brain0, "wiki", "a", "facts", "s.md") + require.NoError(t, os.MkdirAll(filepath.Dir(src), 0o755)) + require.NoError(t, os.WriteFile(src, []byte("x\n"), 0o644)) + tdir := filepath.Join(brain0, "wiki", "b", "facts") + require.NoError(t, os.MkdirAll(tdir, 0o755)) + victim := filepath.Join(t.TempDir(), "victim.md") + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + require.NoError(t, os.Symlink(victim, filepath.Join(tdir, "t.md"))) + assert.Error(t, brain.WriteTunnel(brain0, "wiki/a/facts/s.md", "wiki/b/facts/t.md")) + got, err := os.ReadFile(victim) + require.NoError(t, err) + assert.Equal(t, "untouched\n", string(got)) + + // The same with a link to a file elsewhere in the brain, which os.Root + // would follow. + inVictim := filepath.Join(brain0, "raw", "v.md") + require.NoError(t, os.MkdirAll(filepath.Dir(inVictim), 0o755)) + require.NoError(t, os.WriteFile(inVictim, []byte("untouched\n"), 0o644)) + require.NoError(t, os.Symlink(filepath.Join("..", "..", "..", "raw", "v.md"), filepath.Join(tdir, "u.md"))) + assert.Error(t, brain.WriteTunnel(brain0, "wiki/a/facts/s.md", "wiki/b/facts/u.md")) + got, err = os.ReadFile(inVictim) + require.NoError(t, err) + assert.Equal(t, "untouched\n", string(got)) + got, err = os.ReadFile(src) + require.NoError(t, err) + assert.Equal(t, "x\n", string(got), "the source is not touched when the target is refused") +} + +// The target is swapped between a regular note and a symlink to an in-brain +// file outside wiki/ while WriteTunnel runs: the read and the write each +// refuse a symlink, so the other file is never rewritten. +func TestWriteTunnelNeverRewritesASymlinkSwappedInForTheTarget(t *testing.T) { + brain0 := t.TempDir() + src := filepath.Join(brain0, "wiki", "a", "facts", "s.md") + require.NoError(t, os.MkdirAll(filepath.Dir(src), 0o755)) + require.NoError(t, os.WriteFile(src, []byte("x\n"), 0o644)) + tdir := filepath.Join(brain0, "wiki", "b", "facts") + require.NoError(t, os.MkdirAll(tdir, 0o755)) + victim := filepath.Join(brain0, "raw", "victim.md") + require.NoError(t, os.MkdirAll(filepath.Dir(victim), 0o755)) + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + tgt := filepath.Join(tdir, "t.md") + require.NoError(t, os.WriteFile(tgt, []byte("t\n"), 0o644)) + + var stop atomic.Bool + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + tmp := tgt + ".swap" + for i := 0; !stop.Load(); i++ { + _ = os.Remove(tmp) + if i%2 == 0 { + _ = os.WriteFile(tmp, []byte("t\n"), 0o644) + } else { + _ = os.Symlink(filepath.Join("..", "..", "..", "raw", "victim.md"), tmp) + } + _ = os.Rename(tmp, tgt) + } + }() + ok := 0 + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); { + if brain.WriteTunnel(brain0, "wiki/a/facts/s.md", "wiki/b/facts/t.md") == nil { + ok++ + } + _ = os.WriteFile(src, []byte("x\n"), 0o644) + } + stop.Store(true) + wg.Wait() + got, err := os.ReadFile(victim) + require.NoError(t, err) + assert.Equal(t, "untouched\n", string(got), "the swapped-in symlink's target was rewritten") + assert.Positive(t, ok, "positive control: some tunnels were written to the regular target") +} diff --git a/ingestion/internal/brainfs/brainfs.go b/ingestion/internal/brainfs/brainfs.go index 4feca7c..7496a74 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -249,3 +249,41 @@ func (b *Brain) WalkRegular(relDir string, fn func(rel string) error, onErr func return fn(p) }) } + +// WriteRegular replaces the contents of an existing regular file at a +// brain-relative path. It never creates a file and never writes through a +// final symlink: os.Root.OpenFile follows an in-root final symlink even with +// 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). +func (b *Brain) WriteRegular(rel string, data []byte) error { + rel = filepath.ToSlash(rel) + lfi, err := b.root.Lstat(rel) + if err != nil { + return err + } + if !lfi.Mode().IsRegular() { + return fmt.Errorf("%s: %w", rel, ErrNotRegular) + } + f, err := b.root.OpenFile(rel, os.O_WRONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + if err != nil { + return err + } + defer func() { _ = f.Close() }() + fi, err := f.Stat() + if err != nil { + return err + } + if !fi.Mode().IsRegular() || !os.SameFile(lfi, fi) { + return fmt.Errorf("%s: changed while opening: %w", rel, ErrNotRegular) + } + if err := f.Truncate(0); err != nil { + return err + } + if _, err := f.Write(data); err != nil { + return err + } + return f.Close() +} diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index 76ea7d1..d964f25 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -263,6 +263,52 @@ func TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlink(t *testing. assert.Positive(t, ok, "positive control: the regular file is read") } +func TestWriteRegularRefusesFinalSymlinksFIFOsEscapesAndMissingFilesAndRewritesRegularFiles(t *testing.T) { + dir := t.TempDir() + outside := filepath.Join(t.TempDir(), "o.md") + require.NoError(t, os.WriteFile(outside, []byte("untouched"), 0o644)) + require.NoError(t, os.Symlink(outside, filepath.Join(dir, "l.md"))) + require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "f.md"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "n.md"), []byte("old content"), 0o644)) + b, err := Open(dir) + require.NoError(t, err) + defer func() { _ = b.Close() }() + + assert.Error(t, b.WriteRegular("l.md", []byte("x"))) + got, _ := os.ReadFile(outside) + assert.Equal(t, "untouched", string(got)) + + // A symlink to another file inside the brain: os.Root would allow it, + // only O_NOFOLLOW refuses it. + require.NoError(t, os.WriteFile(filepath.Join(dir, "in.md"), []byte("inside"), 0o644)) + require.NoError(t, os.Symlink("in.md", filepath.Join(dir, "il.md"))) + assert.Error(t, b.WriteRegular("il.md", []byte("x"))) + got, _ = os.ReadFile(filepath.Join(dir, "in.md")) + assert.Equal(t, "inside", string(got)) + + // A reader held open, so a non-blocking write open would succeed. The + // Lstat type check refuses the FIFO first; the handle check and + // O_NONBLOCK matter only if it is swapped in after the Lstat. + r, err := os.OpenFile(filepath.Join(dir, "f.md"), os.O_RDONLY|syscall.O_NONBLOCK, 0) + require.NoError(t, err) + defer func() { _ = r.Close() }() + done := make(chan error, 1) + go func() { done <- b.WriteRegular("f.md", []byte("x")) }() + select { + case err := <-done: + assert.Error(t, err) + case <-time.After(3 * time.Second): + t.Fatal("WriteRegular hung on a FIFO") + } + + assert.Error(t, b.WriteRegular("../"+filepath.Base(filepath.Dir(outside))+"/o.md", []byte("x"))) + assert.Error(t, b.WriteRegular("missing.md", []byte("x")), "does not create files") + + require.NoError(t, b.WriteRegular("n.md", []byte("new"))) + got, _ = os.ReadFile(filepath.Join(dir, "n.md")) + assert.Equal(t, "new", string(got), "truncated and rewritten") +} + func TestReadDirDirsListsOnlyRealSubdirsAndRefusesSymlinkedComponents(t *testing.T) { dir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(dir, "a", "real"), 0o755)) diff --git a/ingestion/internal/session/id_test.go b/ingestion/internal/session/id_test.go new file mode 100644 index 0000000..fe06f21 --- /dev/null +++ b/ingestion/internal/session/id_test.go @@ -0,0 +1,92 @@ +package session + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// session_log passed the caller's session_id straight into +// filepath.Join(sessionsDir, id+".jsonl") and opened it O_CREATE|O_APPEND: +// path segments in the id created or appended files anywhere the pod could +// write. The id is now a plain name, and the open does not follow a final +// symlink. +func TestAppendRefusesSessionIDsThatAreNotPlainNames(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "sessions") + for _, id := range []string{"../x", "a/b", "/abs", ".", "..", ".hidden", "", "a\\b", "a\x00b"} { + assert.Error(t, Append(dir, id, Entry{SessionID: id}), "%q", id) + } + ents, _ := os.ReadDir(root) + for _, e := range ents { + assert.Equal(t, "sessions", e.Name(), "nothing created beside the sessions dir") + } + for _, id := range []string{"1557b2e1-bf0d-5c4f-bbe3-0754d087e356", "_routing", "2026-05-05-infra.plan", "0"} { + require.NoError(t, Append(dir, id, Entry{SessionID: id}), "positive control: %q", id) + } +} + +func TestAppendDoesNotWriteThroughASymlinkedLog(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "sessions") + require.NoError(t, os.MkdirAll(dir, 0o755)) + victim := filepath.Join(root, "victim.md") + require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644)) + require.NoError(t, os.Symlink(filepath.Join("..", "victim.md"), filepath.Join(dir, "s.jsonl"))) + assert.Error(t, Append(dir, "s", Entry{SessionID: "s"})) + got, _ := os.ReadFile(victim) + assert.Equal(t, "untouched\n", string(got)) +} + +func TestReadRefusesSessionIDsThatAreNotPlainNames(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "x.jsonl"), []byte(`{"session_id":"x"}`+"\n"), 0o644)) + _, err := Read(filepath.Join(root, "sessions"), "../x") + assert.Error(t, err) +} + +// A name that starts plainly and then climbs out with ".." is exactly what the +// ban on "/" in the id's tail stops: filepath.Join cleans "a/../../x" to a +// path beside the sessions dir without needing sessions/a to exist. The +// assertion is on the refusal itself (ErrInvalidID), not on any error. +func TestAppendRefusesAnIDThatClimbsOutAfterAPlainPrefix(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "sessions") + require.NoError(t, os.MkdirAll(dir, 0o755)) + for _, id := range []string{"a/../../escape", "a/../../../escape", "a/../b"} { + err := Append(dir, id, Entry{SessionID: id}) + assert.ErrorIs(t, err, ErrInvalidID, "%q", id) + } + _, err := os.Lstat(filepath.Join(root, "escape.jsonl")) + assert.True(t, errors.Is(err, os.ErrNotExist), "nothing created beside the sessions dir") + _, err = os.Lstat(filepath.Join(dir, "b.jsonl")) + assert.True(t, errors.Is(err, os.ErrNotExist), "no in-dir alias created either") +} + +// Append does no Lstat type check, so O_NONBLOCK is the only thing that keeps +// a FIFO named .jsonl with no reader from blocking session_log forever. +func TestAppendReturnsPromptlyOnAFIFOLogWithNoReader(t *testing.T) { + dir := filepath.Join(t.TempDir(), "sessions") + require.NoError(t, os.MkdirAll(dir, 0o755)) + fifo := filepath.Join(dir, "s.jsonl") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + done := make(chan error, 1) + go func() { done <- Append(dir, "s", Entry{SessionID: "s"}) }() + select { + case err := <-done: + assert.Error(t, err, "a FIFO with no reader is refused, not written") + case <-time.After(3 * time.Second): + // Unblock the stuck open so the goroutine and TempDir cleanup finish. + if r, err := os.OpenFile(fifo, os.O_RDONLY|syscall.O_NONBLOCK, 0); err == nil { + defer r.Close() //nolint:errcheck + } + <-done + t.Fatal("Append blocked on a FIFO log with no reader") + } +} diff --git a/ingestion/internal/session/session.go b/ingestion/internal/session/session.go index d9a7ce5..878d425 100644 --- a/ingestion/internal/session/session.go +++ b/ingestion/internal/session/session.go @@ -9,6 +9,8 @@ import ( "io/fs" "os" "path/filepath" + "regexp" + "syscall" "time" ) @@ -42,13 +44,32 @@ type Attempt struct { RunnerOutput string `json:"runner_output,omitempty"` } +// idPattern is a plain file name: session_log takes the id from the caller, +// and a path segment in it created or appended files anywhere the pod could +// write (hyperguild#169). +var idPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]{0,127}$`) + +// ErrInvalidID is returned when a session id is not a plain file name. +var ErrInvalidID = errors.New("session id is not a plain name") + +func checkID(sessionID string) error { + if !idPattern.MatchString(sessionID) { + return fmt.Errorf("%w: %q", ErrInvalidID, sessionID) + } + return nil +} + // Append writes entry as a single JSON line to sessionsDir/{sessionID}.jsonl. +// The open does not follow a final symlink. func Append(sessionsDir, sessionID string, entry Entry) error { + if err := checkID(sessionID); err != nil { + return err + } if err := os.MkdirAll(sessionsDir, 0o755); err != nil { 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, 0o644) + f, err := os.OpenFile(path, 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) } @@ -70,6 +91,9 @@ func Append(sessionsDir, sessionID string, entry Entry) error { // Read returns all entries for sessionID. Returns empty slice if no log exists. func Read(sessionsDir, sessionID string) ([]Entry, error) { + if err := checkID(sessionID); err != nil { + return nil, err + } path := filepath.Join(sessionsDir, sessionID+".jsonl") f, err := os.Open(path) if errors.Is(err, fs.ErrNotExist) {