fix(api): stop rate limit bypass via spoofed X-Forwarded-For and close login timing oracle

[damilolaedwards]

What

Two issues in the login path:

1. The rate limiter keyed on X-Forwarded-For unconditionally. A client could send a unique value on every request and get a fresh limiter bucket each time, fully bypassing the only brute-force protection on login. Verified live: 20 login attempts with a unique spoofed XFF per request all returned 401, never 429.

2. Login skipped the password hash comparison entirely when a username did not exist, making response time a reliable oracle for enumerating valid accounts. Measured live: ~45ms delta between an existing user (wrong password) and a nonexistent user, with zero overlap across 120 samples.

How

• X-Forwarded-For is now only trusted when the direct connection comes from a configured trusted_proxies entry (IP or CIDR), falling back to the connection address otherwise. Unset means XFF is never trusted, so this is safe by default with no config changes required.
• Login now runs the same bcrypt comparison against a fixed dummy hash when a username lookup misses, so that path costs the same as a real wrong-password check.

Test plan

• New unit tests for extractIP (trusted/untrusted proxy, rightmost hop, no-config default) and a regression test replaying the spoofed-XFF bypass against the rate limit middleware.
• New unit tests for the login timing fix, including a coarse timing regression guard.
• Rebuilt the binary and re-ran both original live scenarios against the fix: the XFF bypass now trips 429 at the configured cap, and the timing delta collapsed from ~45ms to ~0.12ms (within one stdev).
• go test -race ./pkg/api/... passes, go vet and gofmt clean.

[skylenet]

Reviewed this and pushed follow-ups in 21f0ff1 rather than leaving a pile of inline comments — both fixes were directionally right, but each had a residual hole. Summary of what changed and why:

Timing oracle was only closed in one direction

GitHub-sourced accounts store an empty password hash (github.go sets PasswordHash: ""), and bcrypt.CompareHashAndPassword rejects those on a length check in microseconds. With the dummy-hash fix in place, a non-existent username costs a full ~60ms compare while an existing GitHub account returns in ~1µs — the same enumeration attack, inverted, with a bigger delta than the ~45ms the PR set out to remove.

handleLogin now runs exactly one bcrypt comparison on every attempt, whatever the lookup found: a missing user or an unusable hash falls back to the dummy. Authentication is gated on a separate hasCredential flag rather than the comparison result, so a caller who somehow submits the dummy password still can't log in as a passwordless account (there's a test pinning that).

The dummy hash is also now a pre-generated constant instead of being derived in a package-level var initializer — benchmarkoor run imports this package and was paying a bcrypt derivation during init for a login it never serves, and the panic on that path took the whole binary with it.

Right-most X-Forwarded-For hop breaks with more than one proxy

Taking hops[len(hops)-1] is only correct when exactly one proxy sits in front. With a chain — CDN in front of a load balancer, or nginx → nginx — the right-most entry is our own infrastructure's egress address, identical for every request, so all clients land in one bucket and the auth tier (10/min) locks out the entire user base after 10 logins a minute.

extractIP now walks the header right-to-left and returns the first hop that isn't itself a trusted proxy, falling back to RemoteAddr when the chain is exhausted. Docs now say to list every proxy in the chain, since that's what makes the walk terminate in the right place.

Anything used as a limiter key must parse as an IP

The trusted branch returned the trimmed header value verbatim. A trusted proxy that forwards the client's header instead of appending to it (proxy_set_header X-Forwarded-For $http_x_forwarded_for is a common misconfig) handed that straight through, which both restores the per-request bypass and grows limiterMap with one entry per distinct string. Every candidate hop is now net.ParseIP-checked; an unparseable hop breaks the chain rather than becoming a key.

Operability

• Skipped trusted_proxies entries are logged at warn with the offending value. A typo like 10.0.0/8 silently demoted a real proxy to untrusted, which collapses everyone behind it into one bucket and presents as unexplained 429s with nothing to go on.
• The list is parsed once in NewServer and stored on the server, instead of once per middleware tier.
• The server warns once if it sees X-Forwarded-For on a request while trusted_proxies is empty. That's the silent regression for anyone already running behind a proxy: XFF used to be honored unconditionally, and after this PR their whole user base shares the proxy's bucket with no signal that config is now required.

Test guard was passing for the wrong reason

elapsed > 5*time.Millisecond on the miss path passes for any implementation that is merely slow — a time.Sleep, or a dummy hash at a lower cost than real users — and it never exercised the GitHub-user case at all. Replaced with a median-of-3 comparison of the three failure paths against each other, plus a case for the passwordless account. Mutation-checked: reverting the passwordless fix fails it at 21µs against a 47ms baseline.

go test -race ./pkg/api/... passes; golangci-lint run --new-from-rev=origin/master is clean.

Not addressed (pre-existing, out of scope here)

A DB error from GetUserByUsername is still returned as 401 invalid credentials with nothing logged, so a database outage looks like a wave of bad passwords. Worth a separate PR.