From e548b97ba4a3268395923b8e9d179364e0321e4f Mon Sep 17 00:00:00 2001 From: Mathias Date: Sun, 4 Oct 2026 22:23:45 +0200 Subject: [PATCH] fix(tunnel): brain_tunnel cannot write outside the wiki or through symlinks brain_tunnel passes caller-supplied source and target paths to WriteTunnel, which checked only the first segments ("wiki//...") and joined them onto the brain dir. "wiki/b/x/../../../../tmp/f.md" resolved outside the brain, and appendSeeAlso read and rewrote that file (verified by execution): any MCP token holder could append to any existing file the pod user can write, inside the brain (raw/, sessions/, classification.yaml) or outside it. - checkNotePath: only a clean, local wiki/.../.md path, with no ".", ".." or empty segments. - Both notes are read and rewritten through brainfs. New brainfs.WriteRegular: never creates; Lstat through the root, open with O_NOFOLLOW|O_NONBLOCK, and require the opened handle to be the same regular file (os.SameFile). os.Root.OpenFile follows an in-root final symlink even with O_NOFOLLOW (verified on go1.27), so the flag alone does not refuse it. - The source is no longer rewritten when the target is refused. Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/brain/tunnel.go | 74 ++++++++--- ingestion/internal/brain/tunnel_root_test.go | 128 +++++++++++++++++++ ingestion/internal/brainfs/brainfs.go | 38 ++++++ ingestion/internal/brainfs/brainfs_test.go | 45 +++++++ 4 files changed, 265 insertions(+), 20 deletions(-) create mode 100644 ingestion/internal/brain/tunnel_root_test.go 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..3ab909d --- /dev/null +++ b/ingestion/internal/brain/tunnel_root_test.go @@ -0,0 +1,128 @@ +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", "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) + } + got, err := os.ReadFile(filepath.Join(brain0, "raw", "r.md")) + require.NoError(t, err) + assert.Equal(t, "x\n", string(got)) + 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)) + 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 fb1b76c..7fe1180 100644 --- a/ingestion/internal/brainfs/brainfs.go +++ b/ingestion/internal/brainfs/brainfs.go @@ -228,3 +228,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 7146af8..fc7e126 100644 --- a/ingestion/internal/brainfs/brainfs_test.go +++ b/ingestion/internal/brainfs/brainfs_test.go @@ -262,3 +262,48 @@ func TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlink(t *testing. assert.Zero(t, leaks) 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 and + // only the type check on the handle refuses it. + 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") +}