security: failed-login tracking keys on spoofable X-Forwarded-For, and its cleanup never runs

[krisarmstrong]

Found while refactoring the version global in internal/logging/audit.go (#797).
Filing separately because the fix needs a design decision, not a rename.

Two defects, one cause

logging.GetClientIP (internal/logging/redact.go:199) returns the
client-supplied X-Forwarded-For value verbatim, with no check on the immediate
peer and no validation that it is even an IP. Its own comments say so:

// Check X-Forwarded-For header (UNTRUSTED - can be spoofed by clients).
"security_note", "XFF can be spoofed - only use for logging, not security decisions"

That is correct guidance, and ~18 of its ~20 call sites honour it — they put the
value in a log field. Two do not. AuditLoginFailure uses it as the
FailedLoginTracker map key, and AuditLoginSuccess uses it to clear entries:

// internal/logging/audit.go:312
ipAddress := GetClientIP(r)
return getFailedLoginTrackerInternal().RecordFailedAttempt(ctx, r, ipAddress, username)

// internal/logging/audit.go:306
getFailedLoginTrackerInternal().ClearAttempts(GetClientIP(r))

1. Suspicious-activity detection is evadable

The tracker alerts at FailedLoginThreshold (5) failures inside
FailedLoginWindow (15m) per key. Since the key is an attacker-supplied
header, sending a different X-Forwarded-For on each attempt puts every
failure under a distinct key, so no key ever reaches 5 and
SuspiciousMultipleFailedLogins never fires.

To be accurate about blast radius: rate limiting is not bypassed.
ratelimit.ClientIP gets this right and still throttles auth at 5/min against
the real peer. What is defeated is the alerting, so a slow credential-stuffing
run stays invisible to operators.

2. The attempts map grows without bound

RecordFailedAttempt prunes expired timestamps within the touched key's
slice, but never deletes the key. Since it appends the current attempt, a key
always ends non-empty, so nothing removes it.

CleanupOldAttempts is the function that would — and its own comment says
// This should be called periodically to prevent memory growth. It has zero
production callers. Only audit_internal_test.go:522 calls it.

So every distinct key value ever seen is retained for the process lifetime. The
rate limiter bounds the rate of new keys, not the total, and the key is an
arbitrary attacker-chosen string rather than a validated IP.

The correct implementation already exists in this repo

ratelimit.ClientIP (internal/api/ratelimit/limiter.go:323) gates the
forwarding headers on the immediate peer being loopback, and its doc comment
describes this exact attack:

Without this gate, an attacker can defeat per-IP rate limiting and pollute
audit logs by sending X-Forwarded-For: 1.2.3.4 on every request.

It cannot simply be reused: internal/api/ratelimit already imports
internal/logging, so logging importing it back is an import cycle. Hence
this is a design decision, not a one-line swap.

Options

1. Move the peer-gated logic into logging and have ratelimit.ClientIP
   delegate to it, leaving one implementation. Preferred — it deletes the
   duplicate rather than adding a third.
2. Add logging.TrustedClientIP alongside the existing function, and change
   only the two security call sites. Smaller, but keeps two implementations of
   the same rule.

Either way the cleanup lifecycle needs an owner. #797 replaces the package
global with a FailedLoginTracker owned by the Server, which is the natural
place to run periodic cleanup and stop it on shutdown; that half is being fixed
there.

Note the same trusted-proxy gap is already acknowledged for non-loopback proxies
in docs/security/AUTH_AUDIT_2026-05-19.md.

Acceptance

• The two security call sites key on a peer-gated client IP, not raw XFF.
• A rotating-X-Forwarded-For test proves the threshold still fires.
• CleanupOldAttempts has a production caller, with a test proving keys are
  reclaimed.