From 687afa0145fe39416fbee3c4692b3c12d58ebb52 Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 3 Oct 2026 15:30:49 +0000 Subject: [PATCH] fix(ingestion): brain/raw/ readers go through an os.Root; symlinks are inert (#153) (#156) Co-authored-by: Mathias --- ingestion/internal/api/handler.go | 5 +- ingestion/internal/api/ingestpath_root.go | 29 ++++ ingestion/internal/api/promote.go | 57 +++--- ingestion/internal/api/raw_race_test.go | 172 +++++++++++++++++++ ingestion/internal/api/raw_symlink_test.go | 80 +++++++++ ingestion/internal/api/rawroot.go | 67 ++++++++ ingestion/internal/extract/callsites_test.go | 130 ++++++++++++++ ingestion/internal/extract/extract.go | 69 ++++++++ ingestion/internal/extract/root_test.go | 64 +++++++ ingestion/internal/mcp/handlers.go | 3 +- ingestion/internal/watcher/watcher.go | 84 ++++++--- ingestion/internal/watcher/watcher_test.go | 102 +++++++++++ 12 files changed, 811 insertions(+), 51 deletions(-) create mode 100644 ingestion/internal/api/raw_race_test.go create mode 100644 ingestion/internal/api/raw_symlink_test.go create mode 100644 ingestion/internal/api/rawroot.go create mode 100644 ingestion/internal/extract/callsites_test.go create mode 100644 ingestion/internal/extract/root_test.go diff --git a/ingestion/internal/api/handler.go b/ingestion/internal/api/handler.go index ad49182..3921e89 100644 --- a/ingestion/internal/api/handler.go +++ b/ingestion/internal/api/handler.go @@ -13,7 +13,6 @@ import ( "github.com/mathiasbq/hyperguild/ingestion/internal/brain" "github.com/mathiasbq/hyperguild/ingestion/internal/claim" - "github.com/mathiasbq/hyperguild/ingestion/internal/extract" "github.com/mathiasbq/hyperguild/ingestion/internal/pipeline" "github.com/mathiasbq/hyperguild/ingestion/internal/search" "github.com/mathiasbq/hyperguild/ingestion/internal/vectorstore" @@ -481,7 +480,7 @@ func (h *Handler) IngestPath(w http.ResponseWriter, r *http.Request) { allWarnings = append(allWarnings, fmt.Sprintf("skipped %s: %v", path, rerr)) return nil } - content, readErr := extract.Text(path) + content, readErr := ReadIngestFile(h.brainDir, path) if readErr != nil { allWarnings = append(allWarnings, fmt.Sprintf("extract %s: %v", path, readErr)) return nil @@ -510,7 +509,7 @@ func (h *Handler) IngestPath(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, fmt.Sprintf("unsupported file extension: %s", ext)) return } - content, readErr := extract.Text(req.Path) + content, readErr := ReadIngestFile(h.brainDir, req.Path) if readErr != nil { writeError(w, http.StatusInternalServerError, fmt.Sprintf("extract text: %v", readErr)) return diff --git a/ingestion/internal/api/ingestpath_root.go b/ingestion/internal/api/ingestpath_root.go index fe36a90..2de2b92 100644 --- a/ingestion/internal/api/ingestpath_root.go +++ b/ingestion/internal/api/ingestpath_root.go @@ -2,6 +2,9 @@ package api import ( "errors" + "os" + + "github.com/mathiasbq/hyperguild/ingestion/internal/extract" "path/filepath" "strings" ) @@ -19,6 +22,11 @@ func ResolveIngestPath(brainDir, p string) (string, error) { if strings.TrimSpace(p) == "" { return "", ErrOutsideIngestRoot } + // raw/ itself must be a real directory: if it were a symlink, containment + // inside it would mean nothing (hyperguild#153 item 4). + if fi, lerr := os.Lstat(filepath.Join(brainDir, "raw")); lerr != nil || fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() { + return "", ErrOutsideIngestRoot + } root, err := filepath.EvalSymlinks(filepath.Join(brainDir, "raw")) if err != nil { return "", ErrOutsideIngestRoot @@ -36,3 +44,24 @@ func ResolveIngestPath(brainDir, p string) (string, error) { } return real, nil } + +// ReadIngestFile extracts text from an ingest path already confined by +// ResolveIngestPath, reading it through the raw/ root rather than reopening +// the path by name (hyperguild#153 item 3): if the file is swapped for a +// symlink after the check, the read still cannot leave raw/. +func ReadIngestFile(brainDir, resolved string) (string, error) { + root, rawPath, err := OpenRawRoot(brainDir) + if err != nil { + return "", ErrOutsideIngestRoot + } + defer func() { _ = root.Close() }() + realRoot, err := filepath.EvalSymlinks(rawPath) + if err != nil { + return "", ErrOutsideIngestRoot + } + rel, err := filepath.Rel(realRoot, resolved) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return "", ErrOutsideIngestRoot + } + return extract.TextIn(root, rel) +} diff --git a/ingestion/internal/api/promote.go b/ingestion/internal/api/promote.go index 7b96b53..c6ec563 100644 --- a/ingestion/internal/api/promote.go +++ b/ingestion/internal/api/promote.go @@ -1,7 +1,9 @@ package api import ( + "errors" "fmt" + "io/fs" "os" "path/filepath" "regexp" @@ -28,25 +30,25 @@ var datePrefix = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}-`) // (natural review order). An absent raw/ dir yields an empty slice, not an // error. Only .md files are listed; tunnel-candidate files are skipped. func ListPending(brainDir string) ([]PendingNote, error) { - dir := filepath.Join(brainDir, "raw") - entries, err := os.ReadDir(dir) + root, _, err := OpenRawRoot(brainDir) if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, os.ErrNotExist) { return []PendingNote{}, nil } return nil, fmt.Errorf("read raw dir: %w", err) } + defer func() { _ = root.Close() }() + entries, err := fs.ReadDir(root.FS(), ".") + if err != nil { + return nil, fmt.Errorf("read raw dir: %w", err) + } out := make([]PendingNote, 0, len(entries)) for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".md") || strings.HasPrefix(e.Name(), "tunnel-candidates-") { - continue + if !e.Type().IsRegular() || !strings.HasSuffix(e.Name(), ".md") || strings.HasPrefix(e.Name(), "tunnel-candidates-") { + continue // directories, and symlinks whose target may be anywhere (#153) } - info, statErr := e.Info() - if statErr != nil { - continue - } - raw, readErr := os.ReadFile(filepath.Join(dir, e.Name())) + raw, info, readErr := readRawNote(root, e.Name()) if readErr != nil { continue } @@ -105,12 +107,9 @@ func PromoteNote(brainDir string, opts PromoteOptions) (string, error) { } src := filepath.Join(brainDir, "raw", base) - raw, err := os.ReadFile(src) + raw, err := readRawSource(brainDir, base) if err != nil { - if os.IsNotExist(err) { - return "", fmt.Errorf("pending note %q does not exist in raw/", base) - } - return "", fmt.Errorf("read source: %w", err) + return "", err } // Collision: never silently overwrite an existing promoted note. @@ -180,12 +179,9 @@ func RejectNote(brainDir string, opts RejectOptions) (string, error) { } src := filepath.Join(brainDir, "raw", base) - raw, err := os.ReadFile(src) + raw, err := readRawSource(brainDir, base) if err != nil { - if os.IsNotExist(err) { - return "", fmt.Errorf("pending note %q does not exist in raw/", base) - } - return "", fmt.Errorf("read source: %w", err) + return "", err } dest := filepath.Join(brainDir, "raw", "rejected", base) @@ -224,3 +220,24 @@ func excerpt(s string, n int) string { } return strings.TrimSpace(string(r)) } + +// readRawSource reads a pending note for promote/reject through the raw root, +// refusing symlinks and a symlinked raw/ (hyperguild#153). +func readRawSource(brainDir, base string) ([]byte, error) { + root, _, err := OpenRawRoot(brainDir) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("pending note %q does not exist in raw/", base) + } + return nil, fmt.Errorf("read source: %w", err) + } + defer func() { _ = root.Close() }() + raw, _, err := readRawNote(root, base) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("pending note %q does not exist in raw/", base) + } + return nil, fmt.Errorf("read source: %w", err) + } + return raw, nil +} diff --git a/ingestion/internal/api/raw_race_test.go b/ingestion/internal/api/raw_race_test.go new file mode 100644 index 0000000..1c7b2bd --- /dev/null +++ b/ingestion/internal/api/raw_race_test.go @@ -0,0 +1,172 @@ +package api_test + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/mathiasbq/hyperguild/ingestion/internal/api" +) + +// hyperguild#153 / PR #156: behavioural race tests, so the control does not +// rest on lints that list forbidden APIs (round 3: an os.ReadFile or +// Root.ReadFile rewrite passed the lint and leaked 19k times in 4s). A +// swapper keeps renaming a regular note, a symlink to an outside file, or a +// FIFO into raw/r.md while the public readers loop. Any leaked byte fails; +// any hang fails. + +const raceSentinel = "SENTINEL-153-RACE" + +func swap(raw, outside string, fifo bool, stop *atomic.Bool) { + tgt := filepath.Join(raw, "r.md") + for i := 0; !stop.Load(); i++ { + tmp := filepath.Join(raw, ".swap") + _ = os.Remove(tmp) + switch { + case i%2 == 0: + _ = os.WriteFile(tmp, []byte("# r\n\ninside"), 0o644) + case fifo: + _ = syscall.Mkfifo(tmp, 0o644) + default: + _ = os.Symlink(outside, tmp) + } + _ = os.Rename(tmp, tgt) + } +} + +func raceBrain(t *testing.T) (brain, raw, outside string) { + t.Helper() + brain = t.TempDir() + raw = filepath.Join(brain, "raw") + if err := os.MkdirAll(raw, 0o755); err != nil { + t.Fatal(err) + } + outside = filepath.Join(t.TempDir(), "secret.md") + if err := os.WriteFile(outside, []byte("# s\n\n"+raceSentinel), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(raw, "r.md"), []byte("# r\n\ninside"), 0o644); err != nil { + t.Fatal(err) + } + return brain, raw, outside +} + +// readers runs every public raw/ reader once and returns all text they +// produced (excerpts, extracted text). +func readers(brain, raw string) string { + var b strings.Builder + if ns, err := api.ListPending(brain); err == nil { + for _, n := range ns { + b.WriteString(n.Excerpt) + } + } + if s, err := api.ReadIngestFile(brain, filepath.Join(raw, "r.md")); err == nil { + b.WriteString(s) + } + return b.String() +} + +func TestRawReadersLeakNothingUnderASymlinkSwapRace(t *testing.T) { + brain, raw, outside := raceBrain(t) + var stop atomic.Bool + go swap(raw, outside, false, &stop) + defer stop.Store(true) + leaks, iters := 0, 0 + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); iters++ { + leaks += strings.Count(readers(brain, raw), raceSentinel) + } + if leaks > 0 { + t.Fatalf("outside file leaked %d times in %d iterations", leaks, iters) + } + t.Logf("%d iterations, 0 leaks", iters) +} + +func TestRawReadersDoNotHangUnderAFIFOSwapRace(t *testing.T) { + brain, raw, outside := raceBrain(t) + var stop atomic.Bool + go swap(raw, outside, true, &stop) + var iters atomic.Int64 + done := make(chan struct{}) + go func() { + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); { + _ = readers(brain, raw) + iters.Add(1) + } + close(done) + }() + select { + case <-done: + stop.Store(true) + case <-time.After(6 * time.Second): + stop.Store(true) + // Unblock the stuck open so the test process can exit. + if f, err := os.OpenFile(filepath.Join(raw, "r.md"), os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { + _ = f.Close() + } + t.Fatalf("a raw/ reader hung on a swapped-in FIFO after %d iterations", iters.Load()) + } +} + +// PR #156 round 4: the pdf/docmark branch of TextIn copies the file through +// the root to a temp file; pointing the extractor back at the raw/ path by +// name stayed green because the race tests only used r.md (a stub docmark +// then received the outside file's bytes 34k times in 4s). This races a +// symlink into raw/r.docx and has a fake docmark count the sentinel in every +// document it is sent. +func TestDocmarkExtractionLeaksNothingUnderASymlinkSwapRace(t *testing.T) { + var seen atomic.Int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req struct { + Params struct { + Arguments struct { + ContentBase64 string `json:"content_base64"` + } `json:"arguments"` + } `json:"params"` + } + _ = json.NewDecoder(r.Body).Decode(&req) + if b, err := base64.StdEncoding.DecodeString(req.Params.Arguments.ContentBase64); err == nil && + strings.Contains(string(b), raceSentinel) { + seen.Add(1) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"converted"}]}}`)) + })) + defer srv.Close() + t.Setenv("DOCMARK_URL", srv.URL) + + brain, raw, outside := raceBrain(t) + tgt := filepath.Join(raw, "r.docx") + if err := os.WriteFile(tgt, []byte("inside docx bytes"), 0o644); err != nil { + t.Fatal(err) + } + var stop atomic.Bool + go func() { + for i := 0; !stop.Load(); i++ { + tmp := filepath.Join(raw, ".swapdocx") + _ = os.Remove(tmp) + if i%2 == 0 { + _ = os.WriteFile(tmp, []byte("inside docx bytes"), 0o644) + } else { + _ = os.Symlink(outside, tmp) + } + _ = os.Rename(tmp, tgt) + } + }() + defer stop.Store(true) + iters := 0 + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); iters++ { + _, _ = api.ReadIngestFile(brain, tgt) + } + if n := seen.Load(); n > 0 { + t.Fatalf("docmark received the outside file %d times in %d iterations", n, iters) + } + t.Logf("%d iterations, 0 leaks to docmark", iters) +} diff --git a/ingestion/internal/api/raw_symlink_test.go b/ingestion/internal/api/raw_symlink_test.go new file mode 100644 index 0000000..fcdea12 --- /dev/null +++ b/ingestion/internal/api/raw_symlink_test.go @@ -0,0 +1,80 @@ +package api_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mathiasbq/hyperguild/ingestion/internal/api" +) + +// hyperguild#153: readers of brain/raw/ followed symlinks. /pending showed the +// target's contents and promote would copy it into wiki/ (and brain-sync to +// Gitea). raw/ is writable via Syncthing, so a symlink there is reachable +// input. Every raw/ reader now treats a symlink as not-a-note. +const sentinel153 = "SENTINEL-153-OUTSIDE" + +func rawWithSymlink(t *testing.T) (brain string) { + t.Helper() + brain = t.TempDir() + raw := filepath.Join(brain, "raw") + require.NoError(t, os.MkdirAll(raw, 0o755)) + outside := filepath.Join(t.TempDir(), "secret.md") + require.NoError(t, os.WriteFile(outside, []byte("---\ncreated_at: 2026-01-01T00:00:00Z\n---\n# s\n\n"+sentinel153), 0o644)) + require.NoError(t, os.Symlink(outside, filepath.Join(raw, "2026-01-01-leak.md"))) + require.NoError(t, os.WriteFile(filepath.Join(raw, "2026-01-02-real.md"), []byte("# real\n\nreal body"), 0o644)) + return brain +} + +func TestListPendingSkipsSymlinkedNotes(t *testing.T) { + notes, err := api.ListPending(rawWithSymlink(t)) + require.NoError(t, err) + var names []string + for _, n := range notes { + names = append(names, n.Filename) + assert.NotContains(t, n.Excerpt, sentinel153) + } + assert.Equal(t, []string{"2026-01-02-real.md"}, names) +} + +func TestPromoteNoteRefusesASymlinkedSource(t *testing.T) { + brain := rawWithSymlink(t) + _, err := api.PromoteNote(brain, api.PromoteOptions{Filename: "2026-01-01-leak.md", Wing: "testing", Hall: "facts"}) + require.Error(t, err) + _, statErr := os.Stat(filepath.Join(brain, "wiki", "testing", "facts", "leak.md")) + assert.True(t, os.IsNotExist(statErr), "nothing may be promoted from a symlink") +} + +func TestRejectNoteRefusesASymlinkedSource(t *testing.T) { + brain := rawWithSymlink(t) + _, err := api.RejectNote(brain, api.RejectOptions{Filename: "2026-01-01-leak.md"}) + require.Error(t, err) + b, _ := os.ReadFile(filepath.Join(brain, "raw", "rejected", "2026-01-01-leak.md")) + assert.False(t, strings.Contains(string(b), sentinel153)) +} + +// Item 4: if raw/ itself is a symlink, containment inside it means nothing. +func TestResolvePendingPromoteRejectAndReadIngestRefuseASymlinkedRawRoot(t *testing.T) { + brain := t.TempDir() + elsewhere := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(elsewhere, "x.md"), []byte("# x\n\n"+sentinel153), 0o644)) + require.NoError(t, os.Symlink(elsewhere, filepath.Join(brain, "raw"))) + + _, err := api.ResolveIngestPath(brain, "x.md") + assert.Error(t, err, "ingest must refuse a symlinked raw/") + notes, err := api.ListPending(brain) + assert.Error(t, err, "pending must refuse a symlinked raw/") + assert.Empty(t, notes) + _, err = api.PromoteNote(brain, api.PromoteOptions{Filename: "x.md", Wing: "testing", Hall: "facts"}) + assert.Error(t, err, "promote must refuse a symlinked raw/") + _, err = api.RejectNote(brain, api.RejectOptions{Filename: "x.md"}) + assert.Error(t, err, "reject must refuse a symlinked raw/") + text, err := api.ReadIngestFile(brain, filepath.Join(elsewhere, "x.md")) + assert.Error(t, err, "ReadIngestFile must refuse a symlinked raw/") + assert.NotContains(t, text, sentinel153) + // The watcher's refusal is tested in internal/watcher. +} diff --git a/ingestion/internal/api/rawroot.go b/ingestion/internal/api/rawroot.go new file mode 100644 index 0000000..203e9be --- /dev/null +++ b/ingestion/internal/api/rawroot.go @@ -0,0 +1,67 @@ +package api + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + + "github.com/mathiasbq/hyperguild/ingestion/internal/extract" +) + +// errNotANote is returned for a raw/ entry that is not a regular file — in +// particular a symlink, whose target may be anywhere (hyperguild#153). +var errNotANote = errors.New("not a regular note file in raw/ (symlinks are refused)") + +// errRawNotDir means brain/raw is not a real directory (e.g. a symlink), so +// containment inside it would mean nothing (hyperguild#153 item 4). +var errRawNotDir = errors.New("brain/raw is not a real directory") + +// OpenRawRoot opens brain/raw as an os.Root, refusing a raw/ that is itself a +// symlink. Reads through the root cannot leave it, even if an entry is +// swapped for a symlink between a check and the read. It returns the root +// and raw/'s path. A missing raw/ returns an error wrapping os.ErrNotExist. +func OpenRawRoot(brainDir string) (*os.Root, string, error) { + p := filepath.Join(brainDir, "raw") + fi, err := os.Lstat(p) + if err != nil { + return nil, "", err + } + if fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() { + return nil, "", errRawNotDir + } + r, err := os.OpenRoot(p) + if err != nil { + return nil, "", fmt.Errorf("open raw root: %w", err) + } + return r, p, nil +} + +// readRawNote reads one raw/ entry by basename through the root, refusing +// anything that is not a regular file. +// +// Accepted residual: a HARD link to an outside file is a regular file and is +// read. Syncthing syncs contents, not links, so a peer cannot create one; +// fs.protected_hardlinks=1 and the pod's secrets live on a different +// filesystem from the brain volume, so a local caller cannot link them in. +// Refusing Nlink > 1 is the next step if either assumption changes. +func readRawNote(r *os.Root, base string) ([]byte, os.FileInfo, error) { + fi, err := r.Lstat(base) + if err != nil { + return nil, nil, err + } + if !fi.Mode().IsRegular() { + return nil, nil, errNotANote + } + f, _, err := extract.OpenRegular(r, base) + if err != nil { + return nil, nil, err + } + defer func() { _ = f.Close() }() + b, err := io.ReadAll(f) + if err != nil { + return nil, nil, err + } + return b, fi, nil +} diff --git a/ingestion/internal/extract/callsites_test.go b/ingestion/internal/extract/callsites_test.go new file mode 100644 index 0000000..c544e2e --- /dev/null +++ b/ingestion/internal/extract/callsites_test.go @@ -0,0 +1,130 @@ +package extract + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +const extractPkg = "github.com/mathiasbq/hyperguild/ingestion/internal/extract" + +// parseNonTest parses every non-test Go file under the module. +func parseNonTest(t *testing.T) map[string]*ast.File { + t.Helper() + fset := token.NewFileSet() + out := map[string]*ast.File{} + err := filepath.WalkDir(filepath.Join("..", ".."), func(p string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() && (d.Name() == "vendor" || d.Name() == ".git") { + return filepath.SkipDir + } + if d.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + f, perr := parser.ParseFile(fset, p, nil, 0) + if perr != nil { + return perr + } + out[filepath.ToSlash(p)] = f + return nil + }) + if err != nil { + t.Fatal(err) + } + return out +} + +// PR #156 review: reading a caller-chosen path by name reopens it after the +// containment check (hyperguild#153 item 3). Outside this package, nothing +// may reference extract.Text — called OR taken as a value — under whatever +// name the package is imported (round 2: an alias and a func value slipped +// past a plain text search). +func TestNoPackageReferencesExtractTextOutsideExtract(t *testing.T) { + for path, f := range parseNonTest(t) { + if strings.Contains(path, "internal/extract/") { + continue + } + names := map[string]bool{} + for _, imp := range f.Imports { + if p, _ := strconv.Unquote(imp.Path.Value); p == extractPkg { + name := "extract" + if imp.Name != nil { + name = imp.Name.Name + } + if name == "." { + t.Errorf("%s dot-imports extract; import it by name so its use stays checkable", path) + } + names[name] = true + } + } + if len(names) == 0 { + continue + } + ast.Inspect(f, func(n ast.Node) bool { + if sel, ok := n.(*ast.SelectorExpr); ok { + if id, ok := sel.X.(*ast.Ident); ok && names[id.Name] && sel.Sel.Name == "Text" { + t.Errorf("%s references %s.Text; use extract.TextIn through an os.Root", path, id.Name) + } + } + return true + }) + } +} + +// PR #156 review round 2: the FIFO fix (open non-blocking, check the handle) +// lives in OpenRegular, and reverting any caller to a plain Root.Open stayed +// green. In the three raw/ reader FILES parsed below (and only those: a +// wrapper in another file is not seen), no Open/OpenFile/ReadFile selector +// may appear outside OpenRegular, called or taken as a method value. This is +// a second layer; the behavioural race tests in internal/api and +// internal/watcher are what catch a rewrite through any API (round 3). +// osOpenAllowed names the functions in guarded files that may call os.Open* +// on a path that is not a raw/ read. +var osOpenAllowed = map[string]bool{ + "internal/watcher/watcher.go:appendWatcherLog": true, // ingest log append, not a raw/ read + // Text reads by path; outside this package only TextIn's temp copy reaches + // it (TestNoPackageReferencesExtractTextOutsideExtract enforces that). + "internal/extract/extract.go:Text": true, +} + +func TestRawReaderFilesUseNoOpenOrReadFileOutsideOpenRegular(t *testing.T) { + guarded := []string{"internal/api/rawroot.go", "internal/extract/extract.go", "internal/watcher/watcher.go"} + files := parseNonTest(t) + for _, g := range guarded { + var f *ast.File + for p, af := range files { + if strings.HasSuffix(p, g) { + f = af + } + } + if f == nil { + t.Fatalf("guarded file %s not found", g) + } + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name == "OpenRegular" { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + if sel.Sel.Name == "Open" || sel.Sel.Name == "OpenFile" || sel.Sel.Name == "ReadFile" { + if id, ok := sel.X.(*ast.Ident); ok && id.Name == "os" && osOpenAllowed[g+":"+fn.Name.Name] { + return true // a named non-raw write (the ingest log append) + } + t.Errorf("%s: %s calls .%s; raw/ reads must go through OpenRegular", g, fn.Name.Name, sel.Sel.Name) + } + return true + }) + } + } +} diff --git a/ingestion/internal/extract/extract.go b/ingestion/internal/extract/extract.go index f7512ec..eac7cea 100644 --- a/ingestion/internal/extract/extract.go +++ b/ingestion/internal/extract/extract.go @@ -3,8 +3,10 @@ package extract import ( "fmt" + "io" "os" "strings" + "syscall" ) // Text reads the file at path and returns its plain-text content. @@ -41,3 +43,70 @@ func fileExt(path string) string { } return "" } + +// TextIn is Text for a file read through an os.Root (hyperguild#153): the +// read cannot leave the root, so a path checked earlier cannot be swapped +// for a symlink pointing elsewhere before it is read. Formats that need an +// external tool (pdf, docmark) are copied to a temp file from the handle +// opened through the root, then extracted from the copy. +func TextIn(r *os.Root, rel string) (string, error) { + fi, err := r.Lstat(rel) + if err != nil { + return "", fmt.Errorf("stat %s: %w", rel, err) + } + if !fi.Mode().IsRegular() { + return "", fmt.Errorf("%s: not a regular file", rel) + } + f, _, err := OpenRegular(r, rel) + if err != nil { + return "", fmt.Errorf("open %s: %w", rel, err) + } + defer func() { _ = f.Close() }() + ext := strings.ToLower(fileExt(rel)) + switch ext { + case ".md", ".txt": + b, err := io.ReadAll(f) + if err != nil { + return "", fmt.Errorf("read %s: %w", rel, err) + } + return string(b), nil + case ".pdf", ".docx", ".xlsx", ".pptx", ".png", ".jpg", ".jpeg": + tmp, err := os.CreateTemp("", "ingest-*"+ext) + if err != nil { + return "", fmt.Errorf("temp copy: %w", err) + } + defer func() { _ = os.Remove(tmp.Name()) }() + if _, err := io.Copy(tmp, f); err != nil { + _ = tmp.Close() + return "", fmt.Errorf("copy %s: %w", rel, err) + } + if err := tmp.Close(); err != nil { + return "", fmt.Errorf("copy %s: %w", rel, err) + } + return Text(tmp.Name()) + default: + return "", fmt.Errorf("unsupported file extension: %s", ext) + } +} + +// OpenRegular opens rel inside r and returns it only if the OPENED handle is +// a regular file. The open is non-blocking, so a FIFO swapped in after an +// Lstat (PR #156 review: it hung open(2) forever) is refused promptly rather +// than stalling the reader. Callers keep their Lstat check, which refuses +// symlinks outright; this closes the race between that check and the open. +func OpenRegular(r *os.Root, rel string) (*os.File, os.FileInfo, error) { + f, err := r.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, nil, err + } + fi, err := f.Stat() + if err != nil { + _ = f.Close() + return nil, nil, err + } + if !fi.Mode().IsRegular() { + _ = f.Close() + return nil, nil, fmt.Errorf("%s: not a regular file", rel) + } + return f, fi, nil +} diff --git a/ingestion/internal/extract/root_test.go b/ingestion/internal/extract/root_test.go new file mode 100644 index 0000000..be7ef3d --- /dev/null +++ b/ingestion/internal/extract/root_test.go @@ -0,0 +1,64 @@ +package extract + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// hyperguild#153 item 3: ingest checked a path, then extract.Text reopened it +// by name, so a file swapped for a symlink in between was followed out of +// raw/. TextIn reads through an os.Root, which refuses to leave the root. +func TestTextInReadsInsideTheRootAndRefusesEscapes(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "ok.md"), []byte("inside"), 0o644)) + outside := filepath.Join(t.TempDir(), "secret.md") + require.NoError(t, os.WriteFile(outside, []byte("SENTINEL-153-TEXTIN"), 0o644)) + require.NoError(t, os.Symlink(outside, filepath.Join(root, "leak.md"))) + + r, err := os.OpenRoot(root) + require.NoError(t, err) + defer func() { _ = r.Close() }() + + got, err := TextIn(r, "ok.md") + require.NoError(t, err) + assert.Equal(t, "inside", got) + + got, err = TextIn(r, "leak.md") + assert.Error(t, err) + assert.NotContains(t, got, "SENTINEL-153-TEXTIN") + + _, err = TextIn(r, "../"+filepath.Base(outside)) + assert.Error(t, err) +} + +// PR #156 review: a regular file swapped for a FIFO between Lstat and Open +// blocked open(2) forever. OpenRegular opens non-blocking and checks the type +// on the opened handle, so a FIFO is refused promptly instead of hanging. +func TestOpenRegularRefusesAFIFOWithoutBlocking(t *testing.T) { + dir := t.TempDir() + require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "pipe.md"), 0o644)) + r, err := os.OpenRoot(dir) + require.NoError(t, err) + defer func() { _ = r.Close() }() + + done := make(chan error, 1) + go func() { + f, _, err := OpenRegular(r, "pipe.md") + if f != nil { + _ = f.Close() + } + done <- err + }() + select { + case err := <-done: + assert.Error(t, err) + case <-time.After(2 * time.Second): + t.Fatal("OpenRegular blocked on a FIFO") + } +} diff --git a/ingestion/internal/mcp/handlers.go b/ingestion/internal/mcp/handlers.go index e4eeb59..62872e1 100644 --- a/ingestion/internal/mcp/handlers.go +++ b/ingestion/internal/mcp/handlers.go @@ -12,7 +12,6 @@ import ( "github.com/mathiasbq/hyperguild/ingestion/internal/api" "github.com/mathiasbq/hyperguild/ingestion/internal/brain" "github.com/mathiasbq/hyperguild/ingestion/internal/capture" - "github.com/mathiasbq/hyperguild/ingestion/internal/extract" "github.com/mathiasbq/hyperguild/ingestion/internal/graphsync" "github.com/mathiasbq/hyperguild/ingestion/internal/notes" "github.com/mathiasbq/hyperguild/ingestion/internal/pipeline" @@ -549,7 +548,7 @@ func (s *Server) brainIngest(ctx context.Context, args json.RawMessage) (json.Ra return nil, err } a.Path = resolved - text, err := extract.Text(a.Path) + text, err := api.ReadIngestFile(s.brainDir, a.Path) if err != nil { return nil, fmt.Errorf("extract: %w", err) } diff --git a/ingestion/internal/watcher/watcher.go b/ingestion/internal/watcher/watcher.go index dea92f9..b5c41b3 100644 --- a/ingestion/internal/watcher/watcher.go +++ b/ingestion/internal/watcher/watcher.go @@ -48,8 +48,25 @@ func Start(ctx context.Context, cfg Config) { func processDir(ctx context.Context, cfg Config, date string) []error { rawDir := filepath.Join(cfg.BrainDir, "raw") + // Every read goes through an os.Root on raw/ (hyperguild#153): raw/ is + // writable via Syncthing, and a file swapped for a symlink between the walk + // and the read must not lead out of it. A symlinked raw/ is refused. + if fi, lerr := os.Lstat(rawDir); lerr != nil { + if os.IsNotExist(lerr) { + return nil + } + return []error{fmt.Errorf("stat raw dir: %w", lerr)} + } else if fi.Mode()&os.ModeSymlink != 0 || !fi.IsDir() { + return []error{fmt.Errorf("raw dir %s is not a real directory; refusing to ingest from it", rawDir)} + } + root, err := os.OpenRoot(rawDir) + if err != nil { + return []error{fmt.Errorf("open raw root: %w", err)} + } + defer func() { _ = root.Close() }() + var errs []error - err := filepath.WalkDir(rawDir, func(path string, d os.DirEntry, err error) error { + err = filepath.WalkDir(rawDir, func(path string, d os.DirEntry, err error) error { if err != nil { return err } @@ -86,6 +103,10 @@ func processDir(ctx context.Context, cfg Config, date string) []error { if strings.HasPrefix(d.Name(), "tunnel-candidates-") { return nil } + // Only regular files are notes; a symlink's target may be anywhere. + if !d.Type().IsRegular() { + return nil + } // Skip files that have already been processed or permanently failed. if _, err := os.Stat(path + ".processed"); err == nil { @@ -95,7 +116,7 @@ func processDir(ctx context.Context, cfg Config, date string) []error { return nil } - if err := processFile(ctx, cfg, path, date); err != nil { + if err := processFile(ctx, cfg, root, rawDir, path, date); err != nil { errs = append(errs, fmt.Errorf("process %s: %w", filepath.Base(path), err)) } return nil @@ -109,11 +130,15 @@ func processDir(ctx context.Context, cfg Config, date string) []error { // processFile reads a file, calls pipeline.Run, copies it to processed/ or failed/, // and writes a marker file next to the original so the watcher skips it next poll. // The original file is never deleted, keeping Syncthing-connected vaults (e.g. Obsidian) intact. -func processFile(ctx context.Context, cfg Config, path, date string) error { +func processFile(ctx context.Context, cfg Config, root *os.Root, rawDir, path, date string) error { filename := filepath.Base(path) source := deriveSource(filename) + rel, err := filepath.Rel(rawDir, path) + if err != nil { + return fmt.Errorf("relative path: %w", err) + } - content, err := extract.Text(path) + content, err := extract.TextIn(root, rel) if err != nil { return fmt.Errorf("extract text: %w", err) } @@ -126,7 +151,7 @@ func processFile(ctx context.Context, cfg Config, path, date string) error { return fmt.Errorf("mkdir failed dir: %w", mkErr) } dest := filepath.Join(failedDir, filename) - if cpErr := copyFile(path, dest); cpErr != nil { + if cpErr := copyFromRoot(root, rel, dest); cpErr != nil { return fmt.Errorf("copy to failed: %w", cpErr) } if mkErr := os.WriteFile(path+".failed", []byte(runErr.Error()), 0o644); mkErr != nil { @@ -154,7 +179,7 @@ func processFile(ctx context.Context, cfg Config, path, date string) error { base := strings.TrimSuffix(filename, ext) dest = filepath.Join(processedDir, base+"-"+time.Now().UTC().Format("150405")+ext) } - if err := copyFile(path, dest); err != nil { + if err := copyFromRoot(root, rel, dest); err != nil { return fmt.Errorf("copy to processed: %w", err) } if err := os.WriteFile(path+".processed", []byte(date), 0o644); err != nil { @@ -165,26 +190,6 @@ func processFile(ctx context.Context, cfg Config, path, date string) error { return nil } -// copyFile copies src to dst, creating dst if it doesn't exist. -func copyFile(src, dst string) error { - in, err := os.Open(src) - if err != nil { - return fmt.Errorf("open src: %w", err) - } - defer in.Close() //nolint:errcheck - - out, err := os.Create(dst) - if err != nil { - return fmt.Errorf("create dst: %w", err) - } - - if _, err := io.Copy(out, in); err != nil { - out.Close() //nolint:errcheck - return fmt.Errorf("copy: %w", err) - } - return out.Close() -} - // deriveSource turns a filename into a human-readable source name. // "shape-up-book.md" → "Shape Up Book" func deriveSource(filename string) string { @@ -221,3 +226,30 @@ func appendWatcherLog(brainDir, filename string, runErr error, date string) erro } return f.Close() } + +// copyFromRoot archives a raw/ file read through the root, refusing anything +// that is not a regular file, so a swapped-in symlink cannot copy an outside +// file into raw/processed/ or raw/failed/ (hyperguild#153). +func copyFromRoot(root *os.Root, rel, dst string) error { + fi, err := root.Lstat(rel) + if err != nil { + return err + } + if !fi.Mode().IsRegular() { + return fmt.Errorf("%s: not a regular file", rel) + } + in, _, err := extract.OpenRegular(root, rel) + if err != nil { + return err + } + defer func() { _ = in.Close() }() + out, err := os.Create(dst) + if err != nil { + return err + } + if _, err := io.Copy(out, in); err != nil { + _ = out.Close() + return err + } + return out.Close() +} diff --git a/ingestion/internal/watcher/watcher_test.go b/ingestion/internal/watcher/watcher_test.go index 7581194..eaf1dba 100644 --- a/ingestion/internal/watcher/watcher_test.go +++ b/ingestion/internal/watcher/watcher_test.go @@ -7,6 +7,9 @@ import ( "fmt" "os" "path/filepath" + "strings" + "sync/atomic" + "syscall" "testing" "time" @@ -282,3 +285,102 @@ func TestProcessDir_SkipsTunnelCandidateFiles(t *testing.T) { _, err = os.Stat(filepath.Join(brainDir, "raw", "processed", date, "tunnel-candidates-2026-07-19.md")) assert.True(t, os.IsNotExist(err), "tunnel-candidates file should not be copied to processed/") } + +// hyperguild#153: raw/ is writable via Syncthing, and the watcher ingests +// whatever lands there with no API call at all. WalkDir yields symlinked +// files, and extract.Text followed them, so a symlink to any pod-readable +// file would be extracted into the brain. The watcher now ingests only +// regular files, read through an os.Root on raw/. +func TestProcessDir_IngestsTheRealNoteAndSkipsTheSymlinkBesideIt(t *testing.T) { + brainDir := setupBrainDir(t) + outside := filepath.Join(t.TempDir(), "secret.md") + require.NoError(t, os.WriteFile(outside, []byte("SENTINEL-153-WATCHER"), 0o644)) + // "a-" sorts before "real": a walk that stopped at the first symlink + // (filepath.SkipAll — one synced link halting all ingestion) would fail + // the call count below (PR #156 review). + require.NoError(t, os.Symlink(outside, filepath.Join(brainDir, "raw", "a-leak.md"))) + require.NoError(t, os.WriteFile(filepath.Join(brainDir, "raw", "real.md"), []byte("real note"), 0o644)) + + var sawSentinel bool + calls := 0 + cfg := Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: pipeline.Config{ + Complete: func(_ context.Context, _, user string) (string, error) { + calls++ + if strings.Contains(user, "SENTINEL-153-WATCHER") { + sawSentinel = true + } + b, _ := json.Marshal([]pipeline.RawPage{{Title: "X", Type: "source", Subtype: "article", Content: "## S\n\nx\n"}}) + return string(b), nil + }, + Schema: "# Schema", + }} + errs := processDir(context.Background(), cfg, time.Now().UTC().Format("2006-01-02")) + assert.Empty(t, errs) + assert.False(t, sawSentinel, "the symlink target must never reach the pipeline") + assert.Equal(t, 1, calls, "exactly the real note is ingested; the symlink is not a note") +} + +// A symlinked raw/ is refused outright: os.OpenRoot would otherwise follow +// it and confine reads to wherever it points. +func TestProcessDir_RefusesASymlinkedRawDir(t *testing.T) { + brainDir := t.TempDir() + elsewhere := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(elsewhere, "x.md"), []byte("SENTINEL-153-RAWLINK"), 0o644)) + require.NoError(t, os.Symlink(elsewhere, filepath.Join(brainDir, "raw"))) + calls := 0 + cfg := Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: pipeline.Config{ + Complete: func(context.Context, string, string) (string, error) { calls++; return "[]", nil }, + Schema: "# Schema", + }} + errs := processDir(context.Background(), cfg, time.Now().UTC().Format("2006-01-02")) + assert.NotEmpty(t, errs, "a symlinked raw/ must be reported, not silently ingested") + assert.Zero(t, calls) +} + +// PR #156: a FIFO swapped into raw/ between the watcher's checks and its +// read or archive copy must not hang the watcher, which runs unattended. +func TestProcessDir_DoesNotHangUnderAFIFOSwapRace(t *testing.T) { + brainDir := setupBrainDir(t) + raw := filepath.Join(brainDir, "raw") + var stop atomic.Bool + go func() { + for i := 0; !stop.Load(); i++ { + tmp := filepath.Join(raw, ".swap") + _ = os.Remove(tmp) + if i%2 == 0 { + _ = os.WriteFile(tmp, []byte("x"), 0o644) + } else { + _ = syscall.Mkfifo(tmp, 0o644) + } + _ = os.Rename(tmp, filepath.Join(raw, "r.md")) + } + }() + cfg := Config{BrainDir: brainDir, Interval: time.Hour, Pipeline: pipeline.Config{ + Complete: func(context.Context, string, string) (string, error) { + b, _ := json.Marshal([]pipeline.RawPage{{Title: "R", Type: "source", Subtype: "article", Content: "## S\n\nx\n"}}) + return string(b), nil + }, + Schema: "# Schema", + }} + var iters atomic.Int64 + done := make(chan struct{}) + go func() { + for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); { + _ = os.Remove(filepath.Join(raw, "r.md.processed")) + _ = os.Remove(filepath.Join(raw, "r.md.failed")) + _ = processDir(context.Background(), cfg, "2026-10-03") + iters.Add(1) + } + close(done) + }() + select { + case <-done: + stop.Store(true) + case <-time.After(8 * time.Second): + stop.Store(true) + if f, err := os.OpenFile(filepath.Join(raw, "r.md"), os.O_WRONLY|syscall.O_NONBLOCK, 0); err == nil { + _ = f.Close() + } + t.Fatalf("the watcher hung on a swapped-in FIFO after %d iterations", iters.Load()) + } +}