security(parser): X-Forwarded-For resolves to the rightmost untrusted hop (#612)

[evertramos]

What & why

Closes #612 (Phase 2 of #605, invariant C4).

When remote_addr is a trusted proxy, the nginx JSON/custom, Caddy and Traefik parsers took the leftmost non-trusted hop of X-Forwarded-For. Proxies and CDNs append the connecting address to whatever the client sent, so X-Forwarded-For: 198.51.100.7 from an attacker arrives as "198.51.100.7, <attacker>" and the attacker's hits, strikes and ban went to 198.51.100.7 — framing any address, including the operator's, while staying clean. The client is the rightmost hop that is not a trusted proxy: the proxies' appended suffix is the only part of the header the client did not write.

Acceptance criteria restated:

• remote_addr=<trusted>, xff="203.0.113.9, 198.51.100.7" → 198.51.100.7 in all three parsers (TestXFF_RightmostUntrustedHop, table of 5 shapes × 3 parsers; fails on dev for the two forged shapes, commit 1).
• Single hop and all-trusted-suffix shapes still resolve as before (existing multi-hop tests kept, renamed).
• Docs: the WordPress/Docker guide's safety note states the rightmost rule.

Found while fixing: production constructs the three parsers with an empty trusted-proxy list — there is no config key — so header resolution is unreachable today. Filed as #626 (with the CDN-edge attribution consequence, INVARIANTS E4-2).

Changes

• internal/parser/xff.go (new): clientFromXFF(xff, trusted) — rightmost untrusted hop. Tokens may carry a port (ip:port, [v6]:port, as IIS ARR / Azure Application Gateway write); IPv4-mapped forms are unmapped before the trusted check and the result. The walk stops at the first token it cannot read (unknown from Squid, an RFC 7239 obfuscated identifier, garbage) and falls back to remote_addr — walking further left would land in client-controlled text, the framing vector again (adversarial review finding; the earlier "skip unparseable" rule was wrong).
• internal/parser/caddy.go: a repeated X-Forwarded-For header (HAProxy's default adds its own line) is logged by Caddy as a JSON array; the values are joined in order before resolving, so the proxy-added value stays the rightmost suffix.
• internal/parser/nginx.go, caddy.go, traefik.go: use it; comments corrected.
• internal/parser/xff_rightmost_test.go (new): 13 header shapes × 3 parsers (forged/real, ports, brackets, mapped client, mapped trusted proxy, unknown, _hidden, garbage, only-trusted, empty) plus the Caddy repeated-header case; two existing subtests renamed.
• Fuzz: FuzzNginxParser/FuzzCaddyParser/FuzzTraefikParser now construct the parsers with trusted proxies and seed lines that reach the resolver — before, the resolver was never executed under fuzz (adversarial review finding).
• docs/content/en/guides/docker-nginx-wordpress.md, docs/internal/INVARIANTS.md (C4).

Tests

• Unit tests added/updated
• New parser/rule? fixture — N/A (no new parser)
• Parser change? fuzz test present — the three targets now exercise the resolver (trusted proxies configured, seeded headers); 15 s local runs clean each
• make lint test green locally (-race): go test -race ./... ok; gofmt -l . empty; go vet ./... ok; golangci-lint run ./... (v2 image) 0 issues; ip-hygiene and docs-placeholder gates clean.

Blast radius (per docs/internal/INVARIANTS.md)

• Invariant(s): C4 (attribution). Readers of SourceIP downstream (aggregator, counters, decision, allowlist) are unchanged; only which header hop becomes SourceIP when a trusted proxy is configured.
• Other readers/writers re-verified: the remote_addr not trusted → header ignored and no trusted proxies → header ignored branches unchanged (existing tests); combined/vhost nginx formats never consult the header (unchanged, nginx_test); Nextcloud/Keycloak/Vaultwarden parsers use their own remote-address fields (unchanged).
• Harness scenario that failed before the fix: internal/parser.TestXFF_RightmostUntrustedHop (parser level; an end-to-end scenario needs feat(config): trusted_proxies has no config surface — X-Forwarded-For resolution is unreachable in production, attacks behind a CDN are attributed to the edge #626's config surface).

Security review (per docs/internal/SECURITY-REVIEW.md)

• §1 Input handling (hostile logs): OK — the header is split, trimmed and parsed per token (ParseAddrPort then parseIP, both bounded; the 4096-byte line cap applies first); the walk stops at the first unreadable token instead of skipping it, so hostile content can only push the result towards remote_addr, never towards a client-chosen address. Mapped spellings are unmapped before the trusted check, so ::ffff:<trusted> cannot be returned as the client.
• §2 Decision engine (lock-out / false-ban): OK — removes a false-ban/framing vector; the allowlist and CDN-range guards still run on the resolved address as before.
• §3 Privilege separation / enforcer: N/A. §4 Secrets: N/A. §5 AI: N/A. §6 Control surfaces: N/A. §7 Plugins: N/A. §8 Edge: N/A. §9 Dependencies: none.
• §10 Logging / audit / fail-safe: OK — debug logs unchanged and redacted (redactForLog). §10 self-review walked (no dead branches; ok=false fallback to remote_addr preserved).

Self-assessment: closes an evasion + framing vector; no new lock-out, injection, privilege or secret exposure.

Checklist

• Follows AGENTS.md Hard Rules
• No hardening systemd directive removed
• Docs updated
• New dependency justified (none)

🤖 Generated with Claude Code

https://claude.ai/code/session_01KCu1p3UrUGSzWVwhNuJ47k

[strix-security]

Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here.

So far, Strix has reviewed 49 pull requests, surfaced 8 security issues (1 critical/high) and blocked 3 risky merges across this workspace.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[amazon-q-developer]

This PR successfully closes a critical security vulnerability in X-Forwarded-For header processing. The implementation correctly switches from leftmost to rightmost untrusted hop resolution, preventing attackers from framing arbitrary IP addresses while staying undetected.

The changes are well-structured with proper separation of concerns through the new clientFromXFF helper function, comprehensive test coverage including edge cases (forged headers, multi-hop chains, all-trusted suffixes), and clear documentation of the security rationale. The implementation maintains backward compatibility for scenarios without trusted proxies configured and properly handles parsing errors by skipping unparseable tokens.

All three parsers (nginx, Caddy, Traefik) are updated consistently, and the PR includes appropriate documentation updates. No defects identified that would block merge.

---

You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.