Two audit agents (code-reviewer + spec-adherence) both returned PASS but
flagged substantive findings. Per fix-in-phase / no-backlog-rot, clearing
every MEDIUM + LOW in-phase.
Findings addressed:
MEDIUM — realIP RemoteAddr fallback returned "IP:port" verbatim, where
the spec wants the bare IP (geoip + downstream parsing assume host-only).
Now uses net.SplitHostPort with raw-string fallback if not host:port.
Adds 4 RemoteAddr cases: IPv4 strips port, IPv6 bracket form strips
brackets+port, loopback IPv6, and the port-less raw fallback.
MEDIUM — realIP XFF branch accepted whatever rightmost-comma-split
produced, so headers like "198.51.100.1, " (trailing comma) yielded
"" and skipped XRI/RemoteAddr entirely. Extracted into lastNonEmptyXFF
which walks right-to-left and falls through cleanly. Adds two cases:
trailing-comma falls through, all-empty entries fall through.
MEDIUM — pixel.TransparentGIF was an exported mutable []byte; any caller
could clobber it process-wide (mutating one response's body would affect
every subsequent webbug trigger). Renamed to unexported transparentGIF
+ exposed pixel.Clone() and pixel.Len(). Webbug now calls pixel.Clone()
per trigger; new test TestTrigger_ResponseBodyIsIndependentCopyPerCall
verifies two triggers return independent slices.
LOW (reviewer-escalated to HIGH-for-Phase-3) — Trigger on nil token
returned a non-nil event with empty TokenID. Since events.token_id is a
NOT NULL FK to tokens.id, the Phase 3 handler could not have persisted
that event anyway, and the contract was implicit (would have required
an inline comment, which the no-comments rule forbids). Now: nil token
in → nil event, non-nil response out. Contract is explicit in the
return shape. Test asserts evt is nil for nil-token path.
LOW — no IPv6 coverage in IP-precedence tests. Added IPv6 cases for
XFF rightmost and three RemoteAddr forms (bracketed, loopback, no-port).
NIT — gracefulShutdown swallowed every shutdown error, returning nil
unconditionally. Now collects with errors.Join so callers can detect
partial-shutdown for telemetry/exit-code purposes.
Audits accepted as-is (NIT, not blocking):
- Artifact discriminated-union shape (Phase 3+ concern when other Kinds
are produced)
- Registry returning bare map (read-only post-Build; concurrent reads
are safe by Go's memory model)
- Build(_ Config) ignoring its arg (signature reserved for future
stateful generators)
Quality verified post-fix: build/vet/lint clean (0 issues),
14 webbug tests + 6 pixel tests + 4 registry tests PASS under -race,
integration tests unchanged.
1a240952c7