relay: log-key allowlist test (#36)

[ilmoniemi]

What

Adds a deterministic fourth defence layer for docs/threat-model.md § Log hygiene:

• internal/relay/log_allowlist.go — allowedLogKeys map enumerating the seven structured-log keys the relay currently emits (server_id, conn_id, binary_conn_id, device_name, remote, binary_version, err). Canonical machine-checkable form of the threat-model rule; adding a key here is the commit-time gate paired with the doc.
• internal/relay/log_keys_test.go — TestLogKeysAreAllowlisted. AST-walks every non-test .go file in the package using go/parser + go/ast (no go/types), matches *ast.CallExpr whose selector is Info/Warn/Error/Debug, narrows to calls whose first arg is a string literal (filters http.Error(w, "", code)), and flags:
  • keys not in allowedLogKeys
  • non-literal key arguments (dynamic keys defeat the allowlist)
  • logger.With/LogAttrs/Log selectors (shapes the walker does not parse)
  • odd-length key/value lists
• docs/threat-model.md — one-paragraph cross-reference under § Log hygiene pointing at the allowlist source as the canonical enforcement point.

Issue

Closes #36.

Testing

• go test -race ./... — all tests pass, including the new one against current main content.
• go vet ./... — clean.
• go build ./cmd/pyrycode-relay — builds.
• Negative-path manual check (documented in the test docstring, not committed): temporarily added "forbidden_key", "x" to a logger.Info call; test failed with server_endpoint.go:93:58: log key "forbidden_key" is not in allowedLogKeys — file path, line, and offending key all named without -v. Reverted.

Architecture compliance

Follows the spec's three-artefact design exactly: allowlist as a normal .go source (not _test.go) so the doc can point at a stable path; AST walker local to the test file with no shared visitor types; violations accumulated and reported at end of walk with a single trailing t.Fatalf count. Walker matches by Sel.Name only (no type resolution), as the spec specifies.

Deviation from spec, documented in the test docstring: the walker additionally requires the matched call's first argument to be a string literal. The spec said false positives from same-named non-logger methods were acceptable, but http.Error(w, "", http.StatusBadRequest) is present in both client_endpoint.go and server_endpoint.go and would have failed the test on main (it has Sel.Name == "Error"). The first-arg-must-be-string-literal narrowing matches the slog convention at every existing call site, excludes http.Error cleanly, and stays inside the spec's "deterministic structural check" principle.

🤖 Generated with Claude Code

[ilmoniemi]

Code Review: #36

Decision: PASS

Findings

• [NIT] internal/relay/log_keys_test.go:3-46 — the long doc comment sits between the package clause and the import block, so godoc will not associate it with TestLogKeysAreAllowlisted. Moving it to immediately above the func would make it discoverable via go doc. Comment content itself is excellent — clearly documents matched shape, narrowing rationale, rejected bypasses, out-of-scope cases, and the negative-path manual check. Not a merge blocker.

• [NIT] internal/relay/log_keys_test.go:108 vs :137,147,154,162 — the unsupported-method violation reports position via fset.Position(call.Lparen), while key violations use fset.Position(keyArg.Pos()). Both work; consistent use of keyArg.Pos() (or for the unsupported case, sel.Sel.Pos()) would make all messages point at the offending symbol rather than the (. Cosmetic.

Verification

• go test -race ./internal/relay/ — pass (the new test included).
• go vet ./... — clean.
• All seven log call-site keys (server_id, conn_id, binary_conn_id, device_name, remote, binary_version, err) reconcile against the allowlist; cross-checked the 14 logger.{Info,Warn} sites in client_endpoint.go, server_endpoint.go, forward.go.
• Verified the unsupportedMethods check (With/LogAttrs/Log) does not currently trip — grep finds no .With(, .LogAttrs(, or .Log( selectors in non-test files. The check is dormant-but-armed as the spec intended.
• Verified the first-arg-string-literal narrowing successfully excludes the two http.Error(w, "", http.StatusBadRequest) call sites in client_endpoint.go:36 and server_endpoint.go:44 (first arg is an *ast.Ident, not a *ast.BasicLit). Without this narrowing, the test would fail on main — good defensive call by the developer; deviation is documented in both the PR description and the test's leading comment.

Security review

Ticket carries security-sensitive. Spec contains a ## Security review section with a PASS verdict and a documented threat list (10 threats considered, with mitigations and explicit out-of-scope decisions for value-side leaks, tampering, off-package logs). No new attack surface introduced — pure test-time gate plus a static data file. No tokens/secrets in diff, no file/subprocess/crypto/network changes. Implementation matches the spec's mitigation design (rejects non-literal keys, flags unsupported methods, reports file:line:key on violation).

Summary

Faithful execution of the spec with one well-justified deviation (first-arg-string-literal narrowing to exclude http.Error), documented in both the PR body and the test docstring. Test passes against current main content; failure modes (dynamic key, alternate method, malformed pair) all carry distinct, actionable error messages. The four-layer defence the threat model now references (spec → code review → structural absence → test-time gate) is in place.