fix(symlinkguard): report and remove FIFOs, sockets and devices too (#179) #190

Merged
mathias merged 3 commits from fix/guard-special-179 into main 2026-10-05 21:15:57 +00:00
Owner

Fixes #179. risk/medium: the tripwire is a control, so it gets an adversarial review.

Problem

The tripwire only looked for symlinks, so a FIFO in the brain passed it:

  • At classification.yaml: startup exited, because Load refuses a non-regular file since #176. That's a crash loop.
  • At a note path: it could hang a reader that skips the handle check.

Change

  • Scan: reports special files (anything that isn't a regular file, directory or symlink) in Result.Special. Clean() and String() include them, so the startup check refuses with a message naming the path.
  • Watch: removes them the way it removes symlinks, through an os.Root, after re-checking each is still special. A regular file, directory or symlink swapped in is left alone and reported.
  • Live brain: it has no special files (checked with find), so this deploy does not newly refuse startup.

Tests

  • TestScanReportsFIFOsAndSocketsAsSpecialFiles
  • TestWatchRemovesSpecialFilesButNotRegularFiles
  • TestRemoveSpecialRefusesRegularFilesAndDirectories
  • TestStartupSymlinkCheckRefusesABrainWithAFIFO

Flips (red)

  • Scan without the special case (3 tests)
  • Watch not removing special files
  • removeSpecial removing anything

🤖 Generated with Claude Code


Created via git-mcp on behalf of @mathias

Fixes #179. risk/medium: the tripwire is a control, so it gets an adversarial review. ## Problem The tripwire only looked for symlinks, so a FIFO in the brain passed it: - **At `classification.yaml`:** startup exited, because Load refuses a non-regular file since #176. That's a crash loop. - **At a note path:** it could hang a reader that skips the handle check. ## Change - **Scan:** reports special files (anything that isn't a regular file, directory or symlink) in `Result.Special`. `Clean()` and `String()` include them, so the **startup check refuses with a message naming the path**. - **Watch:** removes them the way it removes symlinks, through an `os.Root`, after re-checking each is still special. A regular file, directory or symlink swapped in is left alone and reported. - **Live brain:** it has **no special files** (checked with `find`), so this deploy does not newly refuse startup. ## Tests - `TestScanReportsFIFOsAndSocketsAsSpecialFiles` - `TestWatchRemovesSpecialFilesButNotRegularFiles` - `TestRemoveSpecialRefusesRegularFilesAndDirectories` - `TestStartupSymlinkCheckRefusesABrainWithAFIFO` ## Flips (red) - Scan without the special case (3 tests) - Watch not removing special files - `removeSpecial` removing anything 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Created via git-mcp on behalf of @mathias_
mathias added 1 commit 2026-10-05 20:42:53 +00:00
fix(symlinkguard): report and remove FIFOs, sockets and devices too (#179)
CI / Lint / Test / Vet (pull_request) Successful in 1m18s
CI / Mirror to GitHub (pull_request) Skipped
f1f975d281
The tripwire looked only for symlinks. A FIFO in the brain passed it: at
classification.yaml it made startup exit (classification.Load refuses a
non-regular file, #176), and at a note path it could hang a reader that
skips the handle check.

- Scan reports special files (anything not a regular file, directory or
  symlink) in Result.Special; Clean() and String() include them, so the
  startup check refuses with a clear message naming the path.
- Watch removes them as it removes symlinks, through an os.Root, after
  re-checking each is still special (a regular file, directory or
  symlink swapped in is left alone and reported).

The live brain has no special files (checked before this change), so the
startup check does not newly refuse.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mathias added the risk/medium label 2026-10-05 20:43:06 +00:00
mathias reviewed 2026-10-05 20:48:17 +00:00
mathias left a comment
Author
Owner

Adversarial review (pr-gatekeeper) — head f1f975d — BLOCKED

Verified by execution in a scratch copy, not by reading. CI run 9491 green, CI=1 task check exit 0, go test -race -count=5 on symlinkguard and cmd/server green. Green covers none of the blocking finding below.

Blocking

B1. The persistent-failure report for a special file Watch cannot remove has no assertion. symlinkguard.go Watch: key = Result{..., Special: sFailed}.String(). Flip: drop Special: sFailed from the key. All tests stay green. Executed repro: put a FIFO in a 0555 subdirectory of the brain, so unlink fails with EACCES, and run Watch at 10ms. Real code: FIFO stays, one alert naming it as a special file. Flipped code: FIFO stays, zero alerts. So the special file sits in the brain with nothing reported until the next restart. The PR body says a type that is swapped in is "left alone and reported". Nothing tests the "reported" half. Fix: add a Watch test that puts an unremovable FIFO in a read-only dir and asserts an alert that names it, then flip-check it.

Non-blocking

  • N1. Lstat→Remove window (same as removeSymlinks). In a stress loop, a goroutine kept swapping a FIFO and then a regular file onto one path while removeSpecial ran. Regular files deleted: 742 of 61k swaps in 3s. The same harness against removeSymlinks (symlink then regular) deleted 821 of 61k. So this matches the existing window and adds no new class of bug. It needs local write access to the brain dir, and that access can already delete the file. The doc comment ("a path that is now a regular file … is left alone") and the test comment ("re-checks the type: … is not removed") overstate this. The re-check narrows the window but does not close it, and Linux has no conditional unlink. Please say so in both comments. One consequence is worth recording: in a Syncthing sendreceive folder, a regular file deleted in this window is propagated to peers as a deletion. An empty directory swapped in can also be rmdir'd (root.Remove falls back to rmdir), the same as removeSymlinks.
  • N2. Untested guards, all low impact. Each was flipped and nothing went red: (a) the symlink check in removeSpecial (Scan never puts symlinks in Special, and removing a link is harmless); (b) EvalSymlinks/OpenRoot failure in removeSpecial returning rels as failed (flipped to nil, so specials would go unreported if the root vanished between Scan and remove); (c) sort.Strings(r.Special) (WalkDir is already lexical).
  • N3. Messages. The startup refusal reads ... has 1 special file(s): "classification.yaml" (hyperguild#157); fix them first. It does not say what a special file is (FIFO/socket/device) or that it must be deleted. The persistent Watch alert for a special file ends with "Readers may follow symlinks out of the brain", which is wrong for a FIFO.
  • N4. Startup refuses, it does not remove. This matches symlinks. The issue text says startup removes symlinks, but it refuses them. So the pod still crash-loops on a planted FIFO classification.yaml, now with a clear tripwire message instead of a Load error. I think refusing is right: Syncthing doesn't sync FIFOs, so one means someone has local write access, and that needs a human. But the title symptom ("crash-loops the pod") remains by design. Note that startup refusal sends no ntfy alert (this was already true).
  • N5. Rule 9. TestWatchRemovesSpecialFilesButNotRegularFiles uses only a FIFO (no socket), and its regular file was never reported, so "ButNotRegularFiles" proves only that Watch doesn't touch unreported files. TestStartupSymlinkCheckRefusesABrainWithAFIFO's comment says "with a clear message" but it asserts only that the path is present.
  • N6. d_type trust. Scan classifies from the dirent type. Something bind-mounted over a regular-file path (e.g. a device node) scans as regular. That needs mount privilege, so it is outside the Syncthing/local-write threat model. Recorded for completeness.

Verified working (executed)

FIFO, socket → Special. Symlink to a FIFO → reported once, as a symlink. FIFO in a nested .git → Special (only the root .git is skipped). FIFO in an unreadable dir → reported as an unreadable dir. Scan over /dev (read-only) → 274 special, matching find -type b,c,p,s exactly. Real brains, scanned read-only: ~/dev/AGENTS/brain and /var/lib/supervisor/brain → 0 symlinks / 0 special / 0 unreadable, and find agrees on 0 special. So deploying this does not newly refuse startup.

Flip results

Red: drop the Scan special case (3 tests); Clean ignores Special (2); String omits Special (2); removeSpecial without the regular-file check (1); without the dir check (1); Watch without removeSpecial (1); Watch without the removed-special alert (1). Green (survivors): key drops sFailed (B1); removeSpecial symlink check; root-failure path; Special sort.

Not checked

A real char/block device under Watch (mknod is EPERM unprivileged, even in a userns; classification was verified via /dev instead). Behaviour on a real Syncthing peer. The ingestion image/deploy.

## Adversarial review (pr-gatekeeper) — head f1f975d — **BLOCKED** Verified by execution in a scratch copy, not by reading. CI run 9491 green, `CI=1 task check` exit 0, `go test -race -count=5` on symlinkguard and cmd/server green. **Green covers none of the blocking finding below.** ### Blocking **B1. The persistent-failure report for a special file Watch cannot remove has no assertion.** `symlinkguard.go` Watch: `key = Result{..., Special: sFailed}.String()`. Flip: drop `Special: sFailed` from the key. All tests stay green. Executed repro: put a FIFO in a 0555 subdirectory of the brain, so `unlink` fails with EACCES, and run Watch at 10ms. Real code: FIFO stays, one alert naming it as a special file. Flipped code: FIFO stays, **zero alerts**. So the special file sits in the brain with nothing reported until the next restart. The PR body says a type that is swapped in is "left alone and reported". Nothing tests the "reported" half. Fix: add a Watch test that puts an unremovable FIFO in a read-only dir and asserts an alert that names it, then flip-check it. ### Non-blocking - **N1. Lstat→Remove window (same as removeSymlinks).** In a stress loop, a goroutine kept swapping a FIFO and then a regular file onto one path while `removeSpecial` ran. Regular files deleted: 742 of 61k swaps in 3s. The same harness against `removeSymlinks` (symlink then regular) deleted 821 of 61k. So this matches the existing window and adds no new class of bug. It needs local write access to the brain dir, and that access can already delete the file. The doc comment ("a path that is now a regular file … is left alone") and the test comment ("re-checks the type: … is not removed") overstate this. The re-check narrows the window but does not close it, and Linux has no conditional unlink. Please say so in both comments. One consequence is worth recording: in a Syncthing sendreceive folder, a regular file deleted in this window is propagated to peers as a deletion. An empty directory swapped in can also be rmdir'd (root.Remove falls back to rmdir), the same as removeSymlinks. - **N2. Untested guards, all low impact.** Each was flipped and nothing went red: (a) the symlink check in `removeSpecial` (Scan never puts symlinks in Special, and removing a link is harmless); (b) EvalSymlinks/OpenRoot failure in `removeSpecial` returning `rels` as failed (flipped to nil, so specials would go unreported if the root vanished between Scan and remove); (c) `sort.Strings(r.Special)` (WalkDir is already lexical). - **N3. Messages.** The startup refusal reads `... has 1 special file(s): "classification.yaml" (hyperguild#157); fix them first`. It does not say what a special file is (FIFO/socket/device) or that it must be deleted. The persistent Watch alert for a special file ends with "Readers may follow symlinks out of the brain", which is wrong for a FIFO. - **N4. Startup refuses, it does not remove.** This matches symlinks. The issue text says startup *removes* symlinks, but it refuses them. So the pod still crash-loops on a planted FIFO `classification.yaml`, now with a clear tripwire message instead of a Load error. I think refusing is right: Syncthing doesn't sync FIFOs, so one means someone has local write access, and that needs a human. But the title symptom ("crash-loops the pod") remains by design. Note that startup refusal sends no ntfy alert (this was already true). - **N5. Rule 9.** `TestWatchRemovesSpecialFilesButNotRegularFiles` uses only a FIFO (no socket), and its regular file was never reported, so "ButNotRegularFiles" proves only that Watch doesn't touch unreported files. `TestStartupSymlinkCheckRefusesABrainWithAFIFO`'s comment says "with a clear message" but it asserts only that the path is present. - **N6. d_type trust.** Scan classifies from the dirent type. Something bind-mounted over a regular-file path (e.g. a device node) scans as regular. That needs mount privilege, so it is outside the Syncthing/local-write threat model. Recorded for completeness. ### Verified working (executed) FIFO, socket → Special. Symlink to a FIFO → reported once, as a symlink. FIFO in a nested `.git` → Special (only the root `.git` is skipped). FIFO in an unreadable dir → reported as an unreadable dir. Scan over `/dev` (read-only) → 274 special, matching `find -type b,c,p,s` exactly. Real brains, scanned read-only: `~/dev/AGENTS/brain` and `/var/lib/supervisor/brain` → 0 symlinks / 0 special / 0 unreadable, and `find` agrees on 0 special. So deploying this does not newly refuse startup. ### Flip results Red: drop the Scan special case (3 tests); Clean ignores Special (2); String omits Special (2); removeSpecial without the regular-file check (1); without the dir check (1); Watch without removeSpecial (1); Watch without the removed-special alert (1). **Green (survivors):** key drops sFailed (**B1**); removeSpecial symlink check; root-failure path; Special sort. ### Not checked A real char/block device under Watch (mknod is EPERM unprivileged, even in a userns; classification was verified via /dev instead). Behaviour on a real Syncthing peer. The ingestion image/deploy.
mathias added 1 commit 2026-10-05 20:51:20 +00:00
test(symlinkguard): pin the alert for a special file Watch cannot remove (#179)
CI / Lint / Test / Vet (pull_request) Successful in 1m14s
CI / Mirror to GitHub (pull_request) Skipped
edcfe9e9bc
Review round 1 on PR #190:
- B1: dropping the unremovable special files from Watch's persistent
  finding left every test green. New test: a FIFO in a read-only dir
  stays and the alert names it; red under that flip.
- The removeSpecial doc and test comment no longer claim a regular file
  swapped in is always left alone: the re-check narrows, but cannot close,
  the window before the unlink (as in removeSymlinks).
- Messages: the special-file finding says what one is and to delete it;
  the persistent alert no longer talks only about symlinks.
- Rule 9: the Watch removal test is named for what it observes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mathias reviewed 2026-10-05 20:53:44 +00:00
mathias left a comment
Author
Owner

Adversarial review round 2 (pr-gatekeeper) — head edcfe9e — BLOCKED

CI run 9495 green. CI=1 task check at the worktree exits 0. go test -race -count=5 on symlinkguard and cmd/server green. Green does not cover B2.

Round-1 items

  • B1 fixed. TestWatchReportsASpecialFileItCannotRemove goes red under the round-1 flip (the key drops sFailed).
  • N1, N3, N5 addressed as described. N2, N4 and N6 are accepted as recorded.

Blocking

B2. The new test cannot tell "reported as not removable" from "falsely reported as removed". In removeSpecial, flip if err := root.Remove(rel); err != nil { failed...; continue } to ignore the error. Every test stays green, including TestWatchReportsASpecialFileItCannotRemove.

Executed: with the flip, the FIFO in the 0555 dir stays. Watch then sends a "removed 1 special file(s) ... ro/f.md" alert on every scan: 30 alerts in 300ms at a 10ms interval, every 30s in prod. It never sends the persistent-finding alert. The test's two asserts (ro/f.md, special file) both match that false alert. The Lstat still succeeds because the removal really failed. So the name claims something the test does not check (rule 9), and the swallowed-error path has no assertion.

Fix: assert that the alert is the persistent-finding one, not a removal. For example, check that no message contains "removed", or check for the persistent prefix/suffix. Then flip-check it against the swallowed-Remove mutation.

Flip results (head edcfe9e)

  • Red: the key drops sFailed; String omits Special; the Scan special case is removed.
  • Green (survivors): the Remove error is swallowed (B2); the symlink re-check in removeSpecial (N2, accepted).

Not checked

The same as round 1: a real device node under Watch (mknod EPERM), a live Syncthing peer, and the deploy.

## Adversarial review round 2 (pr-gatekeeper) — head edcfe9e — **BLOCKED** CI run 9495 green. `CI=1 task check` at the worktree exits 0. `go test -race -count=5` on symlinkguard and cmd/server green. **Green does not cover B2.** ### Round-1 items - **B1 fixed.** `TestWatchReportsASpecialFileItCannotRemove` goes red under the round-1 flip (the key drops `sFailed`). - **N1, N3, N5 addressed** as described. N2, N4 and N6 are accepted as recorded. ### Blocking **B2. The new test cannot tell "reported as not removable" from "falsely reported as removed".** In `removeSpecial`, flip `if err := root.Remove(rel); err != nil { failed...; continue }` to ignore the error. Every test stays green, including `TestWatchReportsASpecialFileItCannotRemove`. Executed: with the flip, the FIFO in the 0555 dir stays. Watch then sends a **"removed 1 special file(s) ... ro/f.md"** alert on *every* scan: 30 alerts in 300ms at a 10ms interval, every 30s in prod. It never sends the persistent-finding alert. The test's two asserts (`ro/f.md`, `special file`) both match that false alert. The `Lstat` still succeeds because the removal really failed. So the name claims something the test does not check (rule 9), and the swallowed-error path has no assertion. Fix: assert that the alert is the persistent-finding one, not a removal. For example, check that no message contains "removed", or check for the persistent prefix/suffix. Then flip-check it against the swallowed-Remove mutation. ### Flip results (head edcfe9e) - Red: the key drops sFailed; String omits Special; the Scan special case is removed. - **Green (survivors):** the Remove error is swallowed (**B2**); the symlink re-check in removeSpecial (N2, accepted). ### Not checked The same as round 1: a real device node under Watch (mknod EPERM), a live Syncthing peer, and the deploy.
mathias added 1 commit 2026-10-05 20:55:38 +00:00
test(symlinkguard): an unremovable special file must not be reported as removed (#179)
CI / Lint / Test / Vet (pull_request) Successful in 1m15s
CI / Mirror to GitHub (pull_request) Skipped
e614d39e06
Review round 2 on PR #190: swallowing root.Remove's error in
removeSpecial left every test green (the false 'removed' alert also named
the file). The test now asserts no message says 'removed'; red under that
flip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mathias reviewed 2026-10-05 21:01:19 +00:00
mathias left a comment
Author
Owner

Adversarial review round 3 (pr-gatekeeper) — head e614d39 — PASS (merge is Mathias's call)

The only change since edcfe9e is three lines in special_test.go: assert that no alert contains "removed".

Flip results on e614d39 (scratch copy, the worktree was not touched)

  • Remove error swallowed in removeSpecial (B2) → red: TestWatchReportsASpecialFileItCannotRemove.
  • The key drops sFailed (B1) → red: the same test.
  • The Scan special case is removed → red: 4 tests.
  • removeSpecial without the regular-file check → red: TestRemoveSpecialRefusesRegularFilesAndDirectories.
  • Watch without the removed-special alert → red.
  • Still green, accepted as N2: the symlink re-check in removeSpecial, the root-failure path, and the Special sort.

I checked the new assertion for a vacuous pass. The persistent alert text says "remove them", not "removed", and the messages hold only brain-relative paths. So the assertion fails only on a real removal claim.

Gates

  • CI run 9496 on e614d39 is green.
  • CI=1 task check at the worktree (HEAD e614d39) exits 0.

Flake (pre-existing, not introduced here)

One sequential go test -race -count=5 run of symlinkguard failed once, and 3 sequential reruns passed. Under parallel stress (8 parallel x -test.count=10), the older Watch/alert-queue tests flake at about the same rate on main (3/8) as on this head (2/8). The new #179 tests never failed. Filed as #193.

Accepted, unchanged

  • N1: the unlink window, now documented.
  • N2: the low-impact survivors listed above.
  • N4: startup refuses rather than removing, kept on purpose.
  • N6: bind mounts, outside the threat model.

Not checked

A real device node under Watch (mknod EPERM unprivileged; classification was verified via a read-only scan of /dev), a live Syncthing peer, and the deploy.

## Adversarial review round 3 (pr-gatekeeper) — head e614d39 — **PASS** (merge is Mathias's call) The only change since edcfe9e is three lines in `special_test.go`: assert that no alert contains "removed". ### Flip results on e614d39 (scratch copy, the worktree was not touched) - Remove error swallowed in `removeSpecial` (**B2**) → **red**: `TestWatchReportsASpecialFileItCannotRemove`. - The key drops `sFailed` (**B1**) → **red**: the same test. - The Scan special case is removed → **red**: 4 tests. - `removeSpecial` without the regular-file check → **red**: `TestRemoveSpecialRefusesRegularFilesAndDirectories`. - Watch without the removed-special alert → **red**. - Still green, accepted as N2: the symlink re-check in removeSpecial, the root-failure path, and the Special sort. I checked the new assertion for a vacuous pass. The persistent alert text says "remove them", not "removed", and the messages hold only brain-relative paths. So the assertion fails only on a real removal claim. ### Gates - CI run 9496 on e614d39 is green. - `CI=1 task check` at the worktree (HEAD e614d39) exits 0. ### Flake (pre-existing, not introduced here) One sequential `go test -race -count=5` run of symlinkguard failed once, and 3 sequential reruns passed. Under parallel stress (8 parallel x `-test.count=10`), the older Watch/alert-queue tests flake at about the same rate on **main** (3/8) as on this head (2/8). The new #179 tests never failed. Filed as #193. ### Accepted, unchanged - N1: the unlink window, now documented. - N2: the low-impact survivors listed above. - N4: startup refuses rather than removing, kept on purpose. - N6: bind mounts, outside the threat model. ### Not checked A real device node under Watch (mknod EPERM unprivileged; classification was verified via a read-only scan of /dev), a live Syncthing peer, and the deploy.
mathias merged commit 4770f97ffd into main 2026-10-05 21:15:57 +00:00
Sign in to join this conversation.