From 87c80e4e82694d780d2d1f174e5ef753a8f66510 Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 3 Oct 2026 16:59:08 +0200 Subject: [PATCH] fix(ingestion): close the PR #156 review findings on raw/ reads - Reverting the ingest call sites to extract.Text(path) stayed green; a test now fails if anything outside internal/extract calls extract.Text( (only TextIn, through an os.Root, may be used). - The watcher symlink test passed for a watcher that ingested nothing, or stopped at the first symlink (SkipAll: one synced link halting all ingestion). It now plants a real note after the symlink and asserts exactly one ingest, and the symlinked-raw refusal has its own test. - A FIFO swapped in between Lstat and Open hung open(2) forever. extract.OpenRegular opens non-blocking and checks the type on the handle; readRawNote, TextIn and the watcher's archive copy use it. - The symlinked-root test covers pending, promote, reject, resolve and ReadIngestFile, and is named for exactly those. - The hard-link residual is documented where it is accepted. Out of scope, filed: #157 (symlinks elsewhere in the Syncthing-shared brain dir reach search and other readers), #158 (writes under raw/ follow symlinks). Refs #153 Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/api/raw_symlink_test.go | 8 +++- ingestion/internal/api/rawroot.go | 10 ++++- ingestion/internal/extract/callsites_test.go | 43 ++++++++++++++++++++ ingestion/internal/extract/extract.go | 25 +++++++++++- ingestion/internal/extract/root_test.go | 28 +++++++++++++ ingestion/internal/watcher/watcher.go | 2 +- ingestion/internal/watcher/watcher_test.go | 30 ++++++++++++-- 7 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 ingestion/internal/extract/callsites_test.go diff --git a/ingestion/internal/api/raw_symlink_test.go b/ingestion/internal/api/raw_symlink_test.go index 3842e29..fcdea12 100644 --- a/ingestion/internal/api/raw_symlink_test.go +++ b/ingestion/internal/api/raw_symlink_test.go @@ -58,7 +58,7 @@ func TestRejectNoteRefusesASymlinkedSource(t *testing.T) { } // Item 4: if raw/ itself is a symlink, containment inside it means nothing. -func TestRawReadersRefuseASymlinkedRawRoot(t *testing.T) { +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)) @@ -71,4 +71,10 @@ func TestRawReadersRefuseASymlinkedRawRoot(t *testing.T) { 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 index e41d993..203e9be 100644 --- a/ingestion/internal/api/rawroot.go +++ b/ingestion/internal/api/rawroot.go @@ -6,6 +6,8 @@ import ( "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 @@ -38,6 +40,12 @@ func OpenRawRoot(brainDir string) (*os.Root, string, error) { // 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 { @@ -46,7 +54,7 @@ func readRawNote(r *os.Root, base string) ([]byte, os.FileInfo, error) { if !fi.Mode().IsRegular() { return nil, nil, errNotANote } - f, err := r.Open(base) + f, _, err := extract.OpenRegular(r, base) if err != nil { return nil, nil, err } diff --git a/ingestion/internal/extract/callsites_test.go b/ingestion/internal/extract/callsites_test.go new file mode 100644 index 0000000..10be812 --- /dev/null +++ b/ingestion/internal/extract/callsites_test.go @@ -0,0 +1,43 @@ +package extract + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// PR #156 review: putting extract.Text(path) back at the ingest call sites +// stayed green, because only TextIn itself was tested. Reading a caller-chosen +// path by name reopens it after the containment check (hyperguild#153 item 3), +// so outside this package only TextIn may be used. +func TestNothingOutsideExtractCallsTextByPath(t *testing.T) { + root := filepath.Join("..", "..") + var offenders []string + err := filepath.WalkDir(root, 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") || + strings.Contains(filepath.ToSlash(p), "internal/extract/") { + return nil + } + b, err := os.ReadFile(p) + if err != nil { + return err + } + if strings.Contains(string(b), "extract.Text(") { + offenders = append(offenders, p) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + if len(offenders) > 0 { + t.Fatalf("extract.Text( called outside internal/extract (use TextIn through an os.Root): %v", offenders) + } +} diff --git a/ingestion/internal/extract/extract.go b/ingestion/internal/extract/extract.go index 6c8ce5e..eac7cea 100644 --- a/ingestion/internal/extract/extract.go +++ b/ingestion/internal/extract/extract.go @@ -6,6 +6,7 @@ import ( "io" "os" "strings" + "syscall" ) // Text reads the file at path and returns its plain-text content. @@ -56,7 +57,7 @@ func TextIn(r *os.Root, rel string) (string, error) { if !fi.Mode().IsRegular() { return "", fmt.Errorf("%s: not a regular file", rel) } - f, err := r.Open(rel) + f, _, err := OpenRegular(r, rel) if err != nil { return "", fmt.Errorf("open %s: %w", rel, err) } @@ -87,3 +88,25 @@ func TextIn(r *os.Root, rel string) (string, error) { 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 index a55ccd2..be7ef3d 100644 --- a/ingestion/internal/extract/root_test.go +++ b/ingestion/internal/extract/root_test.go @@ -3,7 +3,9 @@ package extract import ( "os" "path/filepath" + "syscall" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,3 +36,29 @@ func TestTextInReadsInsideTheRootAndRefusesEscapes(t *testing.T) { _, 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/watcher/watcher.go b/ingestion/internal/watcher/watcher.go index 64c8d87..b5c41b3 100644 --- a/ingestion/internal/watcher/watcher.go +++ b/ingestion/internal/watcher/watcher.go @@ -238,7 +238,7 @@ func copyFromRoot(root *os.Root, rel, dst string) error { if !fi.Mode().IsRegular() { return fmt.Errorf("%s: not a regular file", rel) } - in, err := root.Open(rel) + in, _, err := extract.OpenRegular(root, rel) if err != nil { return err } diff --git a/ingestion/internal/watcher/watcher_test.go b/ingestion/internal/watcher/watcher_test.go index eeccbe8..4ba49d9 100644 --- a/ingestion/internal/watcher/watcher_test.go +++ b/ingestion/internal/watcher/watcher_test.go @@ -289,11 +289,15 @@ func TestProcessDir_SkipsTunnelCandidateFiles(t *testing.T) { // 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_DoesNotIngestSymlinks(t *testing.T) { +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)) - require.NoError(t, os.Symlink(outside, filepath.Join(brainDir, "raw", "leak.md"))) + // "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 @@ -308,7 +312,25 @@ func TestProcessDir_DoesNotIngestSymlinks(t *testing.T) { }, Schema: "# Schema", }} - _ = processDir(context.Background(), cfg, time.Now().UTC().Format("2006-01-02")) + 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.Zero(t, calls, "a symlink is not a note") + 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) }