feat(relay): refuse to boot if listener ports exceed expected set (#81)#86
Merged
Conversation
Adds a boot-time set-comparison: ports the process is about to bind (http.Server.Addr values) vs ports flag-parsing permits. Any actual port absent from the expected set fails the deploy's health check before traffic flows — catching stray pprof / metrics / debug listeners. - internal/relay/listeners.go: ErrUnexpectedListener sentinel, ListenerPort parser (rejects :0), CheckListenerPorts (asymmetric: surplus is an error, missing is not). - internal/relay/listeners_test.go: AC matrix (cases a–d), sorted- surplus determinism, multiple-surplus reporting, errors.Is branch. - cmd/pyrycode-relay/main.go: build expected/actual sets from parsed flags + http.Server.Addr values; check before each ListenAndServe; exit 2 (boot misconfig) with structured log line carrying err, unexpected_ports, expected_ports. - cmd/pyrycode-relay/deps_test.go: build-graph assertion that net/http/pprof is not in the binary's transitive imports — second defence (compile-time) against the highest-frequency leak shape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ilmoniemi
commented
May 13, 2026
Contributor
Author
ilmoniemi
left a comment
There was a problem hiding this comment.
Code Review: #81
Decision: PASS
Findings
- [NIT]
internal/relay/listeners.go:23-32— the explicitif addr == ""pre-check is redundant:net.SplitHostPort("")already returns an error. The custom message ("listener address is empty") is slightly cleaner than the wrapped stdlib one, so this is purely cosmetic — keep or drop as you prefer. - [NIT]
cmd/pyrycode-relay/main.go:123-138— in insecure mode,expectedandactualare both derived from the sameportvariable, soCheckListenerPortsis structurally tautological in this branch (can never fail). This matches the spec verbatim, and the value is forward-looking — if a future change adds a secondhttp.Serverand updatesactualbut forgetsexpected(or vice versa), the check catches the drift. Worth a one-line comment explaining "the check is here so future drift between expected/actual is caught" if you want to make the intent obvious to a future reader; not required.
Verification
- Spec is
security-sensitiveand the architect's## Security reviewsection is present with verdict PASS — required precondition satisfied. - Security-goggles walk: no tokens/secrets in diff; no file ops; one subprocess (
go list -deps -json) with hardcoded args — no injection surface; no crypto changes; no new network listeners (the whole point is to prevent one); error messages contain only port numbers and operator flag values (acceptable per the spec's security review). The SHOULD-FIX from the security review (silence-on-success) is honoured — port sets are only logged in the failure branch. - All AC items covered: sentinel branchable via
errors.Is; asymmetric check (missing → nil, surplus → wrapped sentinel naming offending ports); wiring in both insecure and autocert branches before anyListen*call; structured log fieldserr/unexpected_ports/expected_ports; build-graphTestBinaryDoesNotImportPprof; unit-test matrix covers cases (a)–(d) plus the sorted-determinism and multiple-surplus lock-ins. go test -race ./...passes;go vet ./...clean.- Exit code 2 (spec override of the AC's literal
1) is consistent with sibling boot-checks #77/#78/#79/#80 — distinguishes boot-time configuration refusal from runtime listener failures.
Summary
Spec-faithful, well-tested, no leaks introduced. The asymmetric port-set check + the net/http/pprof build-graph assertion together encode the "belt-and-suspenders means different fabric" guidance. Ship it.
- features/listener-port-allowlist.md — contract, API, wiring, threat-model alignment, build-graph defence - codebase/81.md — implementation summary, AC verification map, patterns, lessons learned - INDEX.md — new feature entry pointer Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a boot-time set-comparison so the relay refuses to start when it is about to bind any TCP port outside the expected set derived from parsed flags. Catches the surplus-listener leak shapes called out in the spec: stray
net/http/pprofon:6060, env-flipped debug ports, accidentally-enabled metrics exporters.Pairs the runtime guard with a build-graph test that fails if
net/http/pprofis in the binary's transitive imports — second defence on a different layer (compile-time vs runtime) against the highest-frequency leak shape, per the spec's "belt-and-suspenders means different fabric" guidance.Issue
Closes #81. Split from #42. Sibling of #77 / #78 / #79.
Files
internal/relay/listeners.go(new) —ErrUnexpectedListenersentinel (branchable viaerrors.Is),ListenerPortparser (net.SplitHostPort+ParseUint(_, 10, 16); rejects:0ephemeral-port placeholder),CheckListenerPorts(asymmetric: surplus → wrapped sentinel naming offending ports in ascending order + expected set; missing → nil).internal/relay/listeners_test.go(new) — AC matrix (cases a–d in both autocert and insecure shapes), sorted-surplus determinism, multiple-surplus reporting (all listed, not just the first),errors.Isbranchability, fullListenerPortvalue matrix including:0/:99999/:notaport/empty/no-colon/IPv6-bracket forms.cmd/pyrycode-relay/main.go— builds expected and actual port sets in both branches, invokesCheckListenerPortsbefore the firstListenAndServe, emitsos.Exit(2)(boot-time refusal, distinct fromos.Exit(1)runtime listener failures) with a structured log line carryingerr,unexpected_ports,expected_ports. Parse failures on--insecure-listenget their own log line +os.Exit(2). Silent on success per the spec's SHOULD-FIX.cmd/pyrycode-relay/deps_test.go(new) — shells out togo list -deps -jsonand asserts no transitive import hasImportPath == "net/http/pprof". Skips with a clear message ifgois not in$PATH.Testing
go test -race ./...andgo vet ./...pass.TestListenerPort_Matrix— 12-row value matrix; the:0row locks in the explicit ephemeral-port reject.TestCheckListenerPorts_ACMatrix— verbatim AC cases (a)–(d) in both autocert ({443, 80}) and insecure ({8080}) shapes.TestCheckListenerPorts_SurplusListedSorted— ascending order in the error message (locks in deterministic logging acrossmapiteration randomisation).TestCheckListenerPorts_MultipleSurplus— "report all surplus, not just the first" contract.TestErrUnexpectedListener_IsBranchable—errors.Iscontract.TestBinaryDoesNotImportPprof—go list -deps -jsonbuild-graph assertion.Test plan:
go test -race ./...passesgo vet ./...cleango build -o pyry ./cmd/pyrycode-relaysucceeds:6060listener fails the health check withunexpected_ports=[6060]in the boot log.Architecture compliance
Follows the spec at
docs/specs/architecture/81-refuse-extra-listeners.mdexactly:tls.go(ErrCacheDirInsecure) andproduction.go(ErrInsecureListenInProduction).1; spec overrode to keep boot-misconfig distinct from runtime listener failures).EADDRINUSE-style runtime errors fromListenAndServe, which has its own exit path.go list -deps -json(not build-tag-gated compile check) — catches transitive imports too, fails on the offending CI run unconditionally.🤖 Generated with Claude Code