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_root_test.go b/ingestion/internal/brain/tunnel_root_test.go index 3ab909d..7fe05e4 100644 --- a/ingestion/internal/brain/tunnel_root_test.go +++ b/ingestion/internal/brain/tunnel_root_test.go @@ -38,7 +38,7 @@ func TestWriteTunnelRefusesATargetThatTraversesOutOfTheBrain(t *testing.T) { func TestWriteTunnelRefusesUncleanOrNonWikiPaths(t *testing.T) { brain0 := t.TempDir() - for _, p := range []string{"wiki/a/facts/s.md", "wiki/b/facts/t.md", "raw/r.md"} { + 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)) } @@ -52,11 +52,13 @@ func TestWriteTunnelRefusesUncleanOrNonWikiPaths(t *testing.T) { } { assert.Error(t, brain.WriteTunnel(brain0, "wiki/a/facts/s.md", target), target) } - got, err := os.ReadFile(filepath.Join(brain0, "raw", "r.md")) - require.NoError(t, err) - assert.Equal(t, "x\n", string(got)) + 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")) + got, err := os.ReadFile(filepath.Join(brain0, "wiki", "b", "facts", "t.md")) require.NoError(t, err) assert.Contains(t, string(got), "[[a/facts/s]]") } @@ -75,6 +77,17 @@ func TestWriteTunnelDoesNotWriteThroughASymlinkedNote(t *testing.T) { 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") diff --git a/ingestion/internal/brainfs/brainfs_test.go b/ingestion/internal/brainfs/brainfs_test.go index 646da5f..1ebb901 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -213,8 +213,9 @@ func TestWriteRegularRefusesFinalSymlinksFIFOsEscapesAndMissingFilesAndRewritesR 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 and - // only the type check on the handle refuses it. + // 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() }() diff --git a/ingestion/internal/session/id_test.go b/ingestion/internal/session/id_test.go new file mode 100644 index 0000000..d40c605 --- /dev/null +++ b/ingestion/internal/session/id_test.go @@ -0,0 +1,49 @@ +package session + +import ( + "os" + "path/filepath" + "testing" + + "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) +} diff --git a/ingestion/internal/session/session.go b/ingestion/internal/session/session.go index d9a7ce5..095441b 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,29 @@ 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}$`) + +func checkID(sessionID string) error { + if !idPattern.MatchString(sessionID) { + return fmt.Errorf("session id %q is not a plain name", 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 +88,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) {