security: resolve client IP through a trusted-proxy allowlist (rate-limit bypass)

[IbraTech04]

The problem

private String getClientIp(HttpServletRequest request) {
    String xfHeader = request.getHeader("X-Forwarded-For");
    if (xfHeader != null && !xfHeader.isEmpty()) {
        return xfHeader.split(",")[0].trim();   // FIRST entry -- attacker-controlled
    }
    return request.getRemoteAddr();
}

docker/nginx/nginx.conf:96 used proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for, which appends the real peer to whatever the client already sent. Taking split(",")[0] therefore read the entry the caller supplied.

Consequence: rotate a random X-Forwarded-For on each request and every request gets a fresh Caffeine bucket. The 100 req/min limiter becomes a no-op — including in front of POST /api/agent/enroll, the unauthenticated device-enrollment endpoint where the rate limiter is the only abuse guard on token redemption.

AgentEnrollmentController.resolveClientIp duplicated the same helper, so enrollment lastSeenIp values and audit records could be stamped with arbitrary attacker-chosen addresses.

The fix

A single shared ClientIpResolver:

• Honours X-Forwarded-For only when the peer is inside a configured trusted-proxy CIDR set.
• Reads hop size - trustedProxyCount — the entry our own proxy contributed, not the first one. This is correct under both the old appending config and the new overwriting one.
• Fails closed: with no trusted proxies configured it ignores the header entirely and uses the peer address. An unconfigured deployment is safe by default rather than exploitable by default.
• Validates the extracted value is a real IP literal and strips ports; falls back to the peer address on anything malformed.
• Both call sites now use it.

At the edge, nginx overwrites instead of appending (proxy_set_header X-Forwarded-For $remote_addr), so a client's own header can never reach the app at all. docker-compose.yml sets the bridge network as the trusted range, since only nginx can reach the app container (it is not port-published).

A note on the test suite

clientIpIsTakenFromXForwardedForFirstHop asserted the vulnerable behaviour — that two requests from different peers sharing a forged first hop share a bucket. It is replaced by three tests:

• forwardedHeaderFromATrustedProxyIdentifiesTheClient — the header is honoured behind our proxy.
• spoofedForwardedHeaderFromAnUntrustedPeerCannotEvadeTheBucket — the actual fix: a direct caller rotating a forged header still gets 429.
• trustedProxyWithoutAForwardedHeaderFallsBackToThePeerAddress.

MethodSecurityTestConfig gains the resolver bean: @WebMvcTest slices pick up RateLimitingFilter (it is a servlet filter) but not the resolver it depends on, so without it five test classes fail to start their context. No trusted proxies are configured there, which is both the safe default and the right model for a slice with no proxy in front of it.

Verification

RateLimitingFilterTest    tests=10  failures=0  errors=0
---
Tests run: 577, Failures: 0, Errors: 0, Skipped: 0     BUILD SUCCESS

Merge note

The sibling account-lockout PR keys lockout on (email, clientIp) with its own temporary IP resolution and a TODO to adopt this resolver. Once both land, that TODO is a one-line swap to inject this bean.

🤖 Generated with Claude Code

https://claude.ai/code/session_011xQusSt39vP3c82gAUXRCm

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[IbraTech04]

Not sure how necessary this is considering we use Cloudflare to protect our backend. Will research more and work based off of that.