Skip to content

feat(relay): wire per-IP rate-limit middleware on /v1/server + /v1/client (#47)#89

Merged
ilmoniemi merged 3 commits into
mainfrom
feature/47
May 13, 2026
Merged

feat(relay): wire per-IP rate-limit middleware on /v1/server + /v1/client (#47)#89
ilmoniemi merged 3 commits into
mainfrom
feature/47

Conversation

@ilmoniemi
Copy link
Copy Markdown
Contributor

What

Wires the #50 IPRateLimiter and #51 ClientIP primitives into the upgrade pipeline as middleware. Excess attempts from a single source IP are rejected with HTTP 429 Too Many Requests before websocket.Accept runs.

  • internal/relay/ratelimit_middleware.go — new exported NewRateLimitMiddleware(limiter, logger, trustForwardedFor). Empty-IP requests deny before limiter.Allow is called (loud-failure rule from docs/PROJECT-MEMORY.md). Both deny paths emit one slog.Warn("rate_limited", "remote", strconv.Quote(ip)) line — "remote" is already in log_allowlist.go, no edit needed.
  • cmd/pyrycode-relay/main.go — new --trust-x-forwarded-for bool flag (default false, Usage string carries the spoofing-without-a-proxy warning). Policy constants rateLimitRefillEvery=6s, rateLimitBurst=20, rateLimitEvictionInterval=5min (~10 attempts/IP/minute steady-state, burst 20). One limiter shared across /v1/server and /v1/client; /healthz is intentionally NOT wrapped. defer limiter.Close() is placed before the listeners block (best-effort — pre-existing os.Exit-on-error path skips defers; graceful shutdown is out of scope per AC).
  • docs/threat-model.md § DoS resistance — v1 mitigation updated to reference the wiring; the "token-bucket rate limit on /v1/server and /v1/client" line removed from Future hardening. Residual-risk paragraph now names the concurrent-connection-count gap and the single-instance limitation.
  • docs/knowledge/codebase/47.md — per-ticket implementation summary + design choices + trust model + out-of-scope items.

Issue

Closes #47. Depends on #50 (limiter primitive, landed) and #51 (IP helper, landed).

Testing

internal/relay/ratelimit_middleware_test.go — 9 tests:

  1. BurstThenDeny — burst exhausts, sentinel handler counter pins "handler not invoked on deny".
  2. PerIPIsolation — denial of IP A does not affect IP B.
  3. TrustXFF_BucketsByForgedIP — with trustForwardedFor=true, two requests with same RemoteAddr but identical XFF share a bucket; different XFF lands in a fresh bucket.
  4. TrustXFFOff_IgnoresHeader — default mode keys by RemoteAddr even when XFF is set.
  5. EmptyIPDeniesWithoutTouchingLimiterRemoteAddr="" returns 429 and len(limiter.buckets)==0 afterwards (asserts Allow was not called).
  6. DenyEmitsOneLogLine — exactly one level=WARN msg=rate_limited remote="\"1.2.3.4\"" line per deny; forbidden fields (server_id, binary_version, conn_id, device_name, user_agent) explicitly absent.
  7. EmptyIPDenyLogsQuotedEmptyString — empty-IP path renders remote="\"\"" so operators can disambiguate.
  8. AllowEmitsNoLogLine — allow path is silent.
  9. RegistryNotTouchedOnDeny — wrapped over a real ServerHandler, reg.Counts() stays 0,0 after a denied attempt.

TestLogKeysAreAllowlisted continues to pass — the middleware uses literal-string keys only.

  • go test -race ./... clean
  • go vet ./... clean
  • go build ./cmd/pyrycode-relay clean

Architecture compliance

  • Middleware seam at the composition root, not constructor injection — keeps ServerHandler / ClientHandler single-purpose; /healthz's exemption falls out for free. Matches the existing EnforceHost(*domain, mux) shape.
  • Empty-IP guard lives in the middleware (Allow("") is a normal map key per the limiter's contract). Asserted structurally by EmptyIPDeniesWithoutTouchingLimiter.
  • Log shape: Warn, single remote key, strconv.Quote-wrapped value — conforms to docs/threat-model.md § Log hygiene. Mobile/binary token never read by this code path.
  • 429 body empty (http.Error(w, "", 429)) — no internal state leaked per § Error response leakage.
  • Architect's security review verdict (PASS, no findings) — no MUST FIX / SHOULD FIX items.

🤖 Generated with Claude Code

ilmoniemi and others added 2 commits May 13, 2026 10:42
…ient (#47)

Composes the #50 IPRateLimiter and #51 ClientIP primitives as middleware
around the two WS upgrade endpoints. Excess attempts from a single source
IP return 429 before websocket.Accept runs; /healthz is intentionally
unwrapped. A new --trust-x-forwarded-for boolean flag (default false)
controls whether XFF is honoured; the flag's Usage string carries the
spoofing-without-a-proxy warning. Empty-IP requests deny before the
limiter is consulted, per the loud-failure rule. Each deny emits one
slog Warn line with the strconv.Quote-wrapped remote only.

Closes #47.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ilmoniemi
Copy link
Copy Markdown
Contributor Author

Code Review: #47

Decision: PASS

Findings

None.

Summary

Implementation matches the spec exactly. The empty-IP guard correctly precedes limiter.Allow (verified structurally by TestRateLimitMiddleware_EmptyIPDeniesWithoutTouchingLimiter asserting len(l.buckets) == 0 after a denied empty-IP request). The deny log line uses an allowlisted key with strconv.Quote per client_ip.go's log-injection note. /healthz is correctly left unwrapped. A single *IPRateLimiter is shared across both endpoints. No new dependencies.

Verified locally:

  • gofmt -l clean.
  • go vet ./... clean.
  • go test -race -count=1 ./internal/relay/ -run "RateLimitMiddleware|LogKeys" passes — the LogKeys AST-walker also accepts the new Warn call (literal-string keys, no With/LogAttrs/Log).

Test coverage maps 1:1 onto the AC bullets: burst-then-deny + sentinel-counter for "handler never invoked on deny"; per-IP isolation; XFF trust on/off semantics; empty-IP deny without touching the limiter; log-line shape (level=WARN, msg=rate_limited, single remote key, no forbidden fields); allow path emits no log; registry pristine on deny when wrapped over a real ServerHandler.

Threat-model doc update correctly converts the future-hardening line to v1 mitigation and trims residual risk + future hardening of items now delivered. Knowledge doc docs/knowledge/codebase/47.md added per the 2026-05-10 convention; INDEX.md correctly untouched (documentation phase appends).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ilmoniemi ilmoniemi merged commit eddeb68 into main May 13, 2026
4 checks passed
@ilmoniemi ilmoniemi deleted the feature/47 branch May 13, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

relay: wire per-IP rate-limit middleware on /v1/server + /v1/client

1 participant