From 49286ba8b8d43f54c5a828db5aa80adb0c426456 Mon Sep 17 00:00:00 2001 From: Mathias Date: Tue, 20 Oct 2026 20:49:10 +0200 Subject: [PATCH] fix(api,watcher,server): reject moves non-markdown untouched; read caps; BOM; strict routing flag (review of #211, pass 2) - brain_reject on a parked pdf/txt moves the bytes; it no longer prepends YAML - raw/ notes are read through a cap (readCapped); an endless reader test fails instead of exhausting memory - a byte-order mark no longer hides a note's wing: key - INGEST_WING_ROUTING is parsed strictly; a typo stops the server rather than leaving routing silently off - tests for the reserved-wing check in every disguise (Sources, sources., ...) Mutation-checked. Co-Authored-By: Claude Sonnet 5.5 --- docs/raw-inbox.md | 4 +- ingestion/cmd/server/main.go | 22 ++++++- ingestion/cmd/server/main_test.go | 29 ++++++++ ingestion/internal/api/promote.go | 16 +++-- ingestion/internal/api/promote_queue_test.go | 66 +++++++++++++++++++ ingestion/internal/api/rawroot.go | 21 +++++- ingestion/internal/watcher/watcher.go | 1 + .../internal/watcher/watcher_wing_test.go | 4 ++ 8 files changed, 152 insertions(+), 11 deletions(-) diff --git a/docs/raw-inbox.md b/docs/raw-inbox.md index bb2e374..2814ff0 100644 --- a/docs/raw-inbox.md +++ b/docs/raw-inbox.md @@ -30,9 +30,9 @@ deployment: | Variable | Meaning | |---|---| -| `INGEST_WING_ROUTING=1` | enable the rules above | +| `INGEST_WING_ROUTING` | `1`/`true` enables the rules above; unset or `0`/`false` is off; any other value stops the server at startup | | `INGEST_DEFAULT_WING` | wing for unsignalled notes (default `web-clip`; must be a plain name, not `sources`/`concepts`/`entities`/`wiki`) | -| `INGEST_CLIENT_BLOCK` | case-insensitive regexp over note text; a match parks an unsignalled note. Falls back to `CLAUDE_INGEST_CLIENT_BLOCK`. Matched after folding fullwidth letters, removing zero-width characters, normalising dashes and removing whitespace. It catches what it names and nothing else | +| `INGEST_CLIENT_BLOCK` | case-insensitive regexp over note text; a match parks an unsignalled note. Falls back to `CLAUDE_INGEST_CLIENT_BLOCK`. Matched after folding fullwidth letters, removing zero-width characters, normalising dashes and removing whitespace. Patterns with spaces, `\\s` or `\\b` work (whitespace is collapsed in one view and removed in another). It catches what it names and nothing else: leetspeak, punctuation splits (`a.c.m.e`), encodings and homoglyphs outside a small Cyrillic/Greek table are not caught. Large notes are checked in 1 MiB chunks | A bad default or pattern stops the server at startup. A wing created while the server runs is usable without a restart. diff --git a/ingestion/cmd/server/main.go b/ingestion/cmd/server/main.go index 07d26e7..d33af07 100644 --- a/ingestion/cmd/server/main.go +++ b/ingestion/cmd/server/main.go @@ -150,6 +150,21 @@ func envInt(key string, fallback int) int { return fallback } +// envFlag reads a boolean switch. Unset is off; a value strconv.ParseBool does +// not understand is an error, so a typo ("ture") cannot silently leave a +// safety-relevant switch off. +func envFlag(key string) (bool, error) { + v := os.Getenv(key) + if v == "" { + return false, nil + } + b, err := strconv.ParseBool(v) + if err != nil { + return false, fmt.Errorf("%s=%q: want 1, 0, true or false", key, v) + } + return b, nil +} + // buildNotifier returns the ntfy notifier, or a nil interface when // BRAIN_NTFY_URL is unset. A nil *interface* (not a typed-nil) is required so // DegradingSink/Reconcile and the governance observer skip it cleanly. @@ -405,7 +420,12 @@ func main() { // the layout this flag exists to leave. The interface stays nil when off // (a nil *Router inside it would not be nil). var placer wing.Placer - if os.Getenv("INGEST_WING_ROUTING") == "1" { + routing, ferr := envFlag("INGEST_WING_ROUTING") + if ferr != nil { + logger.Error("INGEST_WING_ROUTING", "err", ferr) + os.Exit(1) + } + if routing { pattern := os.Getenv("INGEST_CLIENT_BLOCK") if pattern == "" { pattern = os.Getenv("CLAUDE_INGEST_CLIENT_BLOCK") diff --git a/ingestion/cmd/server/main_test.go b/ingestion/cmd/server/main_test.go index b52f0c7..24f7bb2 100644 --- a/ingestion/cmd/server/main_test.go +++ b/ingestion/cmd/server/main_test.go @@ -100,3 +100,32 @@ func TestClaudeSinkDoesNotWriteThroughASymlinkedArchiveDir(t *testing.T) { }) } } + +func TestEnvFlag(t *testing.T) { + tests := []struct { + name, set string + want bool + wantErr bool + }{ + {"unset is off", "", false, false}, + {"1", "1", true, false}, + {"true", "true", true, false}, + {"TRUE", "TRUE", true, false}, + {"0", "0", false, false}, + {"false", "false", false, false}, + {"a typo is an error, not silently off", "ture", false, true}, + {"yes is not a bool here", "yes", false, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("TEST_ENV_FLAG", tt.set) + got, err := envFlag("TEST_ENV_FLAG") + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/ingestion/internal/api/promote.go b/ingestion/internal/api/promote.go index a0a86cb..f056398 100644 --- a/ingestion/internal/api/promote.go +++ b/ingestion/internal/api/promote.go @@ -261,15 +261,19 @@ func RejectNote(brainDir string, opts RejectOptions) (string, error) { return "", fmt.Errorf("a rejected note named %s already exists", filepath.ToSlash(rel)) } - fm, body := parseFrontmatter(string(raw)) - fm.set("rejected_at", time.Now().UTC().Format(time.RFC3339)) - if opts.Reason != "" { - fm.set("reject_reason", opts.Reason) - } + content := string(raw) + if strings.HasSuffix(base, ".md") { + fm, body := parseFrontmatter(string(raw)) + fm.set("rejected_at", time.Now().UTC().Format(time.RFC3339)) + if opts.Reason != "" { + fm.set("reject_reason", opts.Reason) + } + content = fm.render() + body + } // a pdf or txt is moved byte for byte: prepending frontmatter would alter it // Write-then-delete: the source survives any write failure. Both go // through the brain root (hyperguild#188). - if err := createBrainFile(brainDir, dest, []byte(fm.render()+body)); err != nil { + if err := createBrainFile(brainDir, dest, []byte(content)); err != nil { return "", fmt.Errorf("write rejected note: %w", err) } if err := removeBrainFile(brainDir, src); err != nil { diff --git a/ingestion/internal/api/promote_queue_test.go b/ingestion/internal/api/promote_queue_test.go index 7e58e95..32a1b74 100644 --- a/ingestion/internal/api/promote_queue_test.go +++ b/ingestion/internal/api/promote_queue_test.go @@ -1,8 +1,10 @@ package api import ( + "errors" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -124,3 +126,67 @@ func TestPromoteNote_OnlyMarkdownCanBePromoted(t *testing.T) { _, statErr := os.Stat(filepath.Join(dir, "raw", "needs-review", "paper.pdf")) assert.NoError(t, statErr, "refused, untouched") } + +// brain.Sanitise normalises case, trailing dots and separators; the reserved-name +// check must look at the normalised name or "Sources" and "sources." walk past it. +func TestPromoteNote_ReservedWingNamesAreRefusedInEveryDisguise(t *testing.T) { + for _, w := range []string{"Sources", "sources.", "SOURCES ", "Entities/", "concepts\\", " wiki "} { + t.Run(w, func(t *testing.T) { + dir := t.TempDir() + writeRaw(t, dir, "a.md", "top") + _, err := PromoteNote(dir, PromoteOptions{Filename: "a.md", Wing: w, Hall: "facts"}) + assert.Error(t, err) + _, statErr := os.Stat(filepath.Join(dir, "raw", "a.md")) + assert.NoError(t, statErr, "refused, untouched") + }) + } +} + +// Reject moves the bytes; it must not prepend frontmatter to a pdf or txt. +func TestRejectNote_AParkedNonMarkdownFileIsMovedUntouched(t *testing.T) { + dir := t.TempDir() + pdf := "%PDF-1.4\r\n\x00\xff binary \r\n" + require.NoError(t, os.MkdirAll(filepath.Join(dir, "raw", "needs-review"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "raw", "needs-review", "paper.pdf"), []byte(pdf), 0o644)) + + rel, err := RejectNote(dir, RejectOptions{Filename: "paper.pdf", Reason: "not relevant"}) + require.NoError(t, err) + got, err := os.ReadFile(filepath.Join(dir, rel)) + require.NoError(t, err) + assert.Equal(t, pdf, string(got), "byte for byte") + _, statErr := os.Stat(filepath.Join(dir, "raw", "needs-review", "paper.pdf")) + assert.ErrorIs(t, statErr, os.ErrNotExist) +} + +func TestReadRawNote_IsBoundedByTheCap(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "big.md"), []byte(strings.Repeat("x", 200)), 0o644)) + root, err := os.OpenRoot(dir) + require.NoError(t, err) + defer func() { _ = root.Close() }() + prev := maxRawNoteBytes + maxRawNoteBytes = 100 + t.Cleanup(func() { maxRawNoteBytes = prev }) + + _, _, err = readRawNote(root, "big.md") + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds") +} + +type endless struct{ n int64 } + +func (e *endless) Read(p []byte) (int, error) { + if e.n > 8<<20 { // a regression fails the test instead of exhausting memory + return 0, errors.New("endless reader drained") + } + e.n += int64(len(p)) + return len(p), nil +} + +func TestReadCapped_StopsAtTheCap(t *testing.T) { + e := &endless{} + _, err := readCapped(e, 1000) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds") + assert.LessOrEqual(t, e.n, int64(1000+1+32*1024), "pulled far more than the cap: %d", e.n) +} diff --git a/ingestion/internal/api/rawroot.go b/ingestion/internal/api/rawroot.go index 203e9be..6beb5d0 100644 --- a/ingestion/internal/api/rawroot.go +++ b/ingestion/internal/api/rawroot.go @@ -38,6 +38,10 @@ func OpenRawRoot(brainDir string) (*os.Root, string, error) { return r, p, nil } +// maxRawNoteBytes bounds what promote, reject and the pending listing read from +// one raw/ entry. A variable so a test can lower it. +var maxRawNoteBytes int64 = 64 << 20 + // readRawNote reads one raw/ entry by basename through the root, refusing // anything that is not a regular file. // @@ -59,9 +63,22 @@ func readRawNote(r *os.Root, base string) ([]byte, os.FileInfo, error) { return nil, nil, err } defer func() { _ = f.Close() }() - b, err := io.ReadAll(f) + b, err := readCapped(f, maxRawNoteBytes) if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("%s: %w", base, err) } return b, fi, nil } + +// readCapped reads r but never more than max+1 bytes, so an oversize or growing +// file costs at most that much memory. +func readCapped(r io.Reader, max int64) ([]byte, error) { + b, err := io.ReadAll(io.LimitReader(r, max+1)) + if err != nil { + return nil, err + } + if int64(len(b)) > max { + return nil, fmt.Errorf("note exceeds %d bytes", max) + } + return b, nil +} diff --git a/ingestion/internal/watcher/watcher.go b/ingestion/internal/watcher/watcher.go index 94b941c..8e7e3b2 100644 --- a/ingestion/internal/watcher/watcher.go +++ b/ingestion/internal/watcher/watcher.go @@ -397,6 +397,7 @@ func topDir(rel string) string { // line that is exactly "---"), and an indented `wing:` belongs to some other // mapping, not to the note. func frontmatterWing(content string) string { + content = strings.TrimPrefix(content, "\ufeff") // a byte-order mark must not hide the block content = strings.ReplaceAll(content, "\r\n", "\n") if !strings.HasPrefix(content, "---\n") { return "" diff --git a/ingestion/internal/watcher/watcher_wing_test.go b/ingestion/internal/watcher/watcher_wing_test.go index ff7a68b..16da56d 100644 --- a/ingestion/internal/watcher/watcher_wing_test.go +++ b/ingestion/internal/watcher/watcher_wing_test.go @@ -335,3 +335,7 @@ func TestFrontmatterWing(t *testing.T) { }) } } + +func TestFrontmatterWing_ABOMDoesNotHideTheKey(t *testing.T) { + assert.Equal(t, "homelab", frontmatterWing("\ufeff---\nwing: homelab\n---\nbody")) +}