fix(tunnel): DetectTunnels walks and reads through brainfs (#174, #157 slice 5b) #186

Merged

mathias merged 2 commits from fix/tunnel-detect-174 into main 2026-10-05 20:12:45 +00:00

Conversation 2 Commits 2 Files Changed 3

+155 -32

mathias commented 2026-10-05 18:51:40 +00:00

Owner

Copy Link

Copy Source

Fixes #174. Refs #157 (slice 5b, the last reader slice). risk/medium per #174; control code, so it gets an adversarial review.

Problem

DetectTunnels runs inside every brain_write and brain_update (wikiUpkeep → AutoTunnel). It walked wiki/ with filepath.WalkDir and opened each note with a blocking os.Open:

• FIFO: a FIFO named *.md anywhere under wiki/ hung the write with no deadline.
• Symlinked note: its title became a tunnel candidate (171k leaks in the swap race).

Change

• Walks with brainfs.WalkRegular (regular files only) and reads titles with readTitleAndCreatedIn (ReadRegular: O_NONBLOCK, plus the handle and SameFile checks).
• A wiki/ that is not a real directory yields no candidates, as filepath.WalkDir did; other walk errors still abort (the same open question as #178).
• The path-based readTitleAndCreated, now unused, is removed.

Tests

• TestDetectTunnelsReturnsPromptlyWithAFIFOUnderWiki: red on main (hung for 3 s), with a positive control.
• TestDetectTunnelsRefusesSymlinkedNotesAndSurvivesSwapRaces: outside and in-brain targets, static and racing; red on main.
• TestDetectTunnelsSkipsAWikiThatIsNotARealDirectory: a regression guard, already green on main.

Flips

• Red:
  • title read via os.ReadFile (2 tests)
  • skip turned off
• Green, as expected:
  • Skip any error: below-root errors are unpinned, as in #178.
  • FIFO case under a read flip: WalkRegular never lists the FIFO. Main's whole walk was red.

🤖 Generated with Claude Code

---

Created via git-mcp on behalf of @mathias

Fixes #174. Refs #157 (slice 5b, the last reader slice). risk/medium per #174; control code, so it gets an adversarial review. ## Problem `DetectTunnels` runs inside every brain_write and brain_update (`wikiUpkeep` → `AutoTunnel`). It walked `wiki/` with `filepath.WalkDir` and opened each note with a blocking `os.Open`: - **FIFO:** a FIFO named `*.md` anywhere under `wiki/` hung the write with no deadline. - **Symlinked note:** its title became a tunnel candidate (171k leaks in the swap race). ## Change - Walks with `brainfs.WalkRegular` (regular files only) and reads titles with `readTitleAndCreatedIn` (`ReadRegular`: O_NONBLOCK, plus the handle and SameFile checks). - A `wiki/` that is not a real directory yields no candidates, as `filepath.WalkDir` did; other walk errors still abort (the same open question as #178). - The path-based `readTitleAndCreated`, now unused, is removed. ## Tests - `TestDetectTunnelsReturnsPromptlyWithAFIFOUnderWiki`: red on main (hung for 3 s), with a positive control. - `TestDetectTunnelsRefusesSymlinkedNotesAndSurvivesSwapRaces`: outside and in-brain targets, static and racing; red on main. - `TestDetectTunnelsSkipsAWikiThatIsNotARealDirectory`: a regression guard, already green on main. ## Flips - **Red:** - title read via `os.ReadFile` (2 tests) - skip turned off - **Green, as expected:** - **Skip any error:** below-root errors are unpinned, as in #178. - **FIFO case under a read flip:** `WalkRegular` never lists the FIFO. Main's whole walk was red. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Created via git-mcp on behalf of @mathias_

mathias added 1 commit 2026-10-05 18:51:40 +00:00

fix(tunnel): DetectTunnels walks and reads through brainfs (#174, #157 slice 5b) ... da020d1e6c

DetectTunnels runs inside every brain_write and brain_update
(wikiUpkeep -> AutoTunnel). It walked wiki/ with filepath.WalkDir and
opened each note with a blocking os.Open: a FIFO named *.md anywhere
under wiki/ hung the write with no deadline, and a symlinked note's
title became a tunnel candidate (171k leaks in the swap race).

It now walks with brainfs.WalkRegular (regular files only) and reads
titles with readTitleAndCreatedIn (ReadRegular: O_NONBLOCK, handle and
SameFile checks). A wiki/ that is not a real directory yields no
candidates, as filepath.WalkDir did; other walk errors still abort. The
path-based readTitleAndCreated, now unused, is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mathias added the risk/medium label 2026-10-05 18:51:54 +00:00

mathias referenced this pull request 2026-10-05 18:58:23 +00:00

SECURITY: symlinks anywhere in the Syncthing-shared brain dir are followed by search, vectorstore, context and graph readers #157

mathias commented 2026-10-05 19:01:28 +00:00

Author

Owner

Copy Link

Copy Source

Gatekeeper review: BLOCKED (head da020d1)

The fix holds up when run. The tests don't prove it. I tried to break the production code and couldn't (see "what survived" below). This is blocked because two things the PR adds would not be noticed if they were removed.

Blocking

1. The race test's "inside read" floor is satisfied by the wrong file. detect_root_test.go, TestDetectTunnelsRefusesSymlinkedNotesAndSurvivesSwapRaces. AssertNoLeak/AssertNoLeakFrom require at least one read to return InsideMarker ("inside"), so that a reader which never reads the raced path cannot pass (#177). Here the static note ok.md has the same title (Zqinside Phrase) as the raced note r.md, so every call meets the floor whether or not r.md was read. Flips, both green:

   • DetectTunnels skips r.md entirely: the test passes.
   • readTitleAndCreatedIn fails every read of r.md: the test passes.

   So this is the vacuous positive control from #177 again. Fix: give ok.md a title without "inside" (or drop it from the race half), so the only way to meet the floor is to read the raced path.

2. The new walk-error path is untested, and swallowing it goes unnoticed. tunnel.go, the walkErr accumulator and if err == nil { err = walkErr }. The path can be reached: an unreadable subdirectory under wiki/ makes DetectTunnels return wiki/<x>: ... permission denied (I ran it). Flip err = walkErr to _ = walkErr and every test in brain, brainstore, mcp, api and watcher stays green. This is new code standing in for the old WalkDir abort, and nothing notices if it is removed. Add a test that pins whichever behaviour you intend (abort with an error, as now, or skip and continue).

Non-blocking

• TestDetectTunnelsReturnsPromptlyWithAFIFOUnderWiki uses a FIFO with no writer, which reads EOF straight away under O_NONBLOCK. With the walk type filter, the ReadRegular Lstat check and the handle check all removed, it still passes. It only goes red when the open also blocks. FIFO-with-writer coverage comes from brainfs and the BuildWingIndex race test, not from this one. A writer-held FIFO here, or a name that says "no writer", would make it honest under rule 9.
• _index.md skip in DetectTunnels: no test fails when it is removed, here or on main. This gap predates the PR, but the PR rewrote the line.
• gofumpt -l flags internal/brain/index.go (a double blank line left where readTitleAndCreated was removed). CI's golangci-lint does not run gofumpt.
• Known residual, by execution: a hall directory swapped for an in-brain directory symlink mid-walk lets notes from elsewhere in the same brain become candidates. Detection saw thousands of such candidates in 3s, and in one of two end-to-end runs one reached the fuzzy tunnel-candidates log. The data stays in this brain (os.Root), and this is the residual WalkRegular already documents (#157/#167). No outside or other-brain content was reached.

What survived (all by execution)

• End-to-end brainstore.Write returns promptly and leaks nothing with each of these under wiki/: a FIFO with a writer, with a reader, with neither; a FIFO in place of a wing dir; a unix socket; links to /dev/zero (absolute and relative); 400-deep tree (positive control found at the bottom); a self-loop dir link. I could not create a device node (mknod EPERM).
• Static file and directory symlinks to outside, other-brain and in-brain targets (relative and absolute; a wing linked to raw/): no sentinel in any candidate, and the legit cross-wing tunnel was still written in every case.
• Swap races during detection and end-to-end writes, about 3s each: a FIFO with a writer held open, and an in-brain file symlink. No hangs, no leaks, and the raced path was read thousands of times.
• Parity: slug fallback, single-word case-exact match, and TargetPath format (wiki/..., forward slashes) each go red when flipped. The PR's FIFO and swap tests go red against main's DetectTunnels. Removing the non-dir wiki/ skip or the missing-wiki early return turns a test red.
• Removing SameFile turns the swap test red. That check is what stops in-brain symlink swaps here.

CI

Run 9450 ("CI / Lint / Test / Vet (pull_request)") succeeded. CI=1 task check exits 0 locally. -race -count=5 passes on brain, brainstore and brainfs. Green, and green covers neither blocking finding.

Not checked

Hard links (accepted residual), device nodes (no CAP_MKNOD), the api/promote.go caller of AutoTunnel end-to-end, and behaviour on the real Syncthing volume or in the pod's filesystem.

## Gatekeeper review: BLOCKED (head da020d1) **The fix holds up when run. The tests don't prove it.** I tried to break the production code and couldn't (see "what survived" below). This is blocked because two things the PR adds would not be noticed if they were removed. ### Blocking 1. **The race test's "inside read" floor is satisfied by the wrong file.** `detect_root_test.go`, `TestDetectTunnelsRefusesSymlinkedNotesAndSurvivesSwapRaces`. `AssertNoLeak`/`AssertNoLeakFrom` require at least one read to return `InsideMarker` ("inside"), so that a reader which never reads the raced path cannot pass (#177). Here the static note `ok.md` has the same title (`Zqinside Phrase`) as the raced note `r.md`, so every call meets the floor whether or not `r.md` was read. Flips, both green: - DetectTunnels skips `r.md` entirely: the test passes. - `readTitleAndCreatedIn` fails every read of `r.md`: the test passes. So this is the vacuous positive control from #177 again. Fix: give `ok.md` a title without "inside" (or drop it from the race half), so the only way to meet the floor is to read the raced path. 2. **The new walk-error path is untested, and swallowing it goes unnoticed.** `tunnel.go`, the `walkErr` accumulator and `if err == nil { err = walkErr }`. The path can be reached: an unreadable subdirectory under `wiki/` makes DetectTunnels return `wiki/<x>: ... permission denied` (I ran it). Flip `err = walkErr` to `_ = walkErr` and every test in brain, brainstore, mcp, api and watcher stays green. This is new code standing in for the old WalkDir abort, and nothing notices if it is removed. Add a test that pins whichever behaviour you intend (abort with an error, as now, or skip and continue). ### Non-blocking - `TestDetectTunnelsReturnsPromptlyWithAFIFOUnderWiki` uses a FIFO with no writer, which reads EOF straight away under O_NONBLOCK. With the walk type filter, the ReadRegular Lstat check and the handle check all removed, it still passes. It only goes red when the open also blocks. FIFO-with-writer coverage comes from brainfs and the BuildWingIndex race test, not from this one. A writer-held FIFO here, or a name that says "no writer", would make it honest under rule 9. - `_index.md` skip in DetectTunnels: no test fails when it is removed, here or on main. This gap predates the PR, but the PR rewrote the line. - `gofumpt -l` flags `internal/brain/index.go` (a double blank line left where `readTitleAndCreated` was removed). CI's golangci-lint does not run gofumpt. - Known residual, by execution: a hall directory swapped for an in-brain directory symlink mid-walk lets notes from elsewhere in the same brain become candidates. Detection saw thousands of such candidates in 3s, and in one of two end-to-end runs one reached the fuzzy tunnel-candidates log. The data stays in this brain (os.Root), and this is the residual WalkRegular already documents (#157/#167). No outside or other-brain content was reached. ### What survived (all by execution) - End-to-end `brainstore.Write` returns promptly and leaks nothing with each of these under `wiki/`: a FIFO with a writer, with a reader, with neither; a FIFO in place of a wing dir; a unix socket; links to /dev/zero (absolute and relative); 400-deep tree (positive control found at the bottom); a self-loop dir link. I could not create a device node (mknod EPERM). - Static file and directory symlinks to outside, other-brain and in-brain targets (relative and absolute; a wing linked to raw/): no sentinel in any candidate, and the legit cross-wing tunnel was still written in every case. - Swap races during detection and end-to-end writes, about 3s each: a FIFO with a writer held open, and an in-brain file symlink. No hangs, no leaks, and the raced path was read thousands of times. - Parity: slug fallback, single-word case-exact match, and TargetPath format (`wiki/...`, forward slashes) each go red when flipped. The PR's FIFO and swap tests go red against main's DetectTunnels. Removing the non-dir `wiki/` skip or the missing-wiki early return turns a test red. - Removing SameFile turns the swap test red. That check is what stops in-brain symlink swaps here. ### CI Run 9450 ("CI / Lint / Test / Vet (pull_request)") succeeded. `CI=1 task check` exits 0 locally. `-race -count=5` passes on brain, brainstore and brainfs. **Green, and green covers neither blocking finding.** ### Not checked Hard links (accepted residual), device nodes (no CAP_MKNOD), the `api/promote.go` caller of AutoTunnel end-to-end, and behaviour on the real Syncthing volume or in the pod's filesystem.

mathias referenced this issue from a commit 2026-10-05 19:03:32 +00:00

test(tunnel): pin the raced read, the walk error and the _index.md skip (#174)

mathias added 1 commit 2026-10-05 19:03:32 +00:00

test(tunnel): pin the raced read, the walk error and the _index.md skip (#174) ... 1c9c218dbf

Review round 1 on PR #186:

- The swap test's static note had the same title as the raced note, so
  the harness's "inside was read" floor passed without reading r.md
  (skipping r.md stayed green). The static note now has its own title.
- An unreadable directory under wiki/ reaches the walk-error path, and
  swallowing that error stayed green. New test asserts the error.
- The FIFO test now runs with and without a writer held open.
- _index.md skip had no test; new test.
- index.go: the double blank line left by removing readTitleAndCreated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mathias commented 2026-10-05 19:06:51 +00:00

Author

Owner

Copy Link

Copy Source

Gatekeeper review, round 2: PASS (head 1c9c218)

Both round-1 blockers are fixed, and I checked each by execution. The round-1 non-blocking items are fixed too. Since da020d1 the only production change is the whitespace fix in index.go; everything else is tests. I am not merging: Mathias approves merges because merging deploys ingestion.

Flip results at 1c9c218 (each one is a deliberate break, run against the tests)

Flip	Result
DetectTunnels skips the raced r.md	red (swap test)
every read of r.md fails	red (swap test)
walk error swallowed (_ = walkErr)	red (ReportsAnUnreadableDirectoryUnderWiki)
walk error wrapped with %v instead of %w	red
every walk error filtered as if it were the non-dir wiki/ case	red
_index.md skip removed	red (SkipsIndexFiles)
SameFile check removed	red (swap test)
walk type filter + Lstat check + handle check removed	red (FIFO test, writer case; it was green in round 1)
O_NONBLOCK + walk filter + Lstat check removed	red
walk filter + Lstat check removed (handle check kept)	green: the handle check still refuses the FIFO, which is correct

The round-1 flips that also stayed green and are not repeated above (the type-check layers removed one at a time) stay green because the remaining layers still refuse the file. They are not gaps.

Re-run end to end (brainstore.Write, unchanged since round 1)

• FIFOs (with writer, with reader, with neither), a socket, a FIFO in place of a wing directory, device-file links, a 400-deep tree, and static file and directory symlinks to outside, other-brain and in-brain targets: no hang, no sentinel in any candidate, and the legit cross-wing tunnel was written every time.
• Swap races with a writer-held FIFO and with an in-brain file symlink, during detection and end-to-end writes: no hangs, no leaks.

Non-blocking

• ReportsAnUnreadableDirectoryUnderWiki skips when running as root. CI runs on a self-hosted act runner, and the log (no -v) does not show whether it ran there. It runs and goes red under the flip locally (uid 1000) and in CI=1 task check. If the runner is root, CI does not cover the walk-error path.
• Known residual, unchanged and already documented (#157/#167): a hall directory swapped mid-walk for a link to another directory in the same brain still lets in-brain notes become candidates. In one end-to-end run, one such row reached the fuzzy tunnel-candidates log. No outside or other-brain content was reached.
• Rule 9, minor: the "SurvivesSwapRaces" part of RefusesSymlinkedNotesAndSurvivesSwapRaces covers symlink swaps only. FIFO swaps are covered elsewhere (brainfs, BuildWingIndex race test).

CI

Run 9458 ("CI / Lint / Test / Vet (pull_request)") succeeded. CI=1 task check at 1c9c218 exits 0 with no FAIL lines. -race -count=5 passes on brain, brainstore, brainfs and brainfstest. gofumpt -l internal/brain/ is clean.

Not checked

Hard links (accepted residual), real device nodes (no CAP_MKNOD), the api/promote.go caller end to end, the real Syncthing volume and the pod, and whether CI runs as root.

## Gatekeeper review, round 2: PASS (head 1c9c218) Both round-1 blockers are fixed, and I checked each by execution. The round-1 non-blocking items are fixed too. Since da020d1 the only production change is the whitespace fix in `index.go`; everything else is tests. **I am not merging: Mathias approves merges because merging deploys ingestion.** ### Flip results at 1c9c218 (each one is a deliberate break, run against the tests) | Flip | Result | |---|---| | DetectTunnels skips the raced `r.md` | red (swap test) | | every read of `r.md` fails | red (swap test) | | walk error swallowed (`_ = walkErr`) | red (`ReportsAnUnreadableDirectoryUnderWiki`) | | walk error wrapped with `%v` instead of `%w` | red | | every walk error filtered as if it were the non-dir `wiki/` case | red | | `_index.md` skip removed | red (`SkipsIndexFiles`) | | SameFile check removed | red (swap test) | | walk type filter + Lstat check + handle check removed | red (FIFO test, writer case; it was green in round 1) | | O_NONBLOCK + walk filter + Lstat check removed | red | | walk filter + Lstat check removed (handle check kept) | green: the handle check still refuses the FIFO, which is correct | The round-1 flips that also stayed green and are not repeated above (the type-check layers removed one at a time) stay green because the remaining layers still refuse the file. They are not gaps. ### Re-run end to end (brainstore.Write, unchanged since round 1) - FIFOs (with writer, with reader, with neither), a socket, a FIFO in place of a wing directory, device-file links, a 400-deep tree, and static file and directory symlinks to outside, other-brain and in-brain targets: no hang, no sentinel in any candidate, and the legit cross-wing tunnel was written every time. - Swap races with a writer-held FIFO and with an in-brain file symlink, during detection and end-to-end writes: no hangs, no leaks. ### Non-blocking - `ReportsAnUnreadableDirectoryUnderWiki` skips when running as root. CI runs on a self-hosted act runner, and the log (no `-v`) does not show whether it ran there. It runs and goes red under the flip locally (uid 1000) and in `CI=1 task check`. If the runner is root, CI does not cover the walk-error path. - Known residual, unchanged and already documented (#157/#167): a hall directory swapped mid-walk for a link to another directory in the same brain still lets in-brain notes become candidates. In one end-to-end run, one such row reached the fuzzy tunnel-candidates log. No outside or other-brain content was reached. - Rule 9, minor: the "SurvivesSwapRaces" part of `RefusesSymlinkedNotesAndSurvivesSwapRaces` covers symlink swaps only. FIFO swaps are covered elsewhere (brainfs, BuildWingIndex race test). ### CI Run 9458 ("CI / Lint / Test / Vet (pull_request)") succeeded. `CI=1 task check` at 1c9c218 exits 0 with no FAIL lines. `-race -count=5` passes on brain, brainstore, brainfs and brainfstest. `gofumpt -l internal/brain/` is clean. ### Not checked Hard links (accepted residual), real device nodes (no CAP_MKNOD), the `api/promote.go` caller end to end, the real Syncthing volume and the pod, and whether CI runs as root.

mathias merged commit 1fff54769d into main 2026-10-05 20:12:45 +00:00

mathias referenced this pull request 2026-10-05 20:17:47 +00:00

SECURITY: symlinks anywhere in the Syncthing-shared brain dir are followed by search, vectorstore, context and graph readers #157

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

agent/ready

Meets the agent-ready contract; authorizes unattended execution

blocked/client

Waiting on the client

blocked/supplier

Waiting on a supplier

blocked/upstream

Waiting on an upstream project or vendor

risk/high

Irreversible, security-relevant, or control plane

risk/low

Reversible, local, test-covered

risk/medium

Touches shared state, deploys or data

size/l

Too big - split it

size/m

Under two days

size/s

Under half a day

size/xs

Under an hour

status:dispatch

status:dispatched

status:review

status:reviewing

type/chore

Maintenance, no behaviour change

type/decision

Human decision required; never agent/ready

type/docs

Documentation only

type/feat

New capability

type/fix

Corrects broken behaviour

type/ops

Infrastructure, deploy, runtime

type/research

Investigate; output is a finding, not code

type/test

Test coverage only

No labels risk/medium

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate-bot mathias

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mathias/hyperguild#186