fix(security): make the client IP trustworthy and scope credential lockout to it

[JOY (JOY)]

Two coupled defects. The second could not be fixed safely before the first, which is why this is one pull request rather than two.

1. The client IP was forgeable with a single header

Nothing in the repository calls SetTrustedProxies or sets TrustedPlatform - grep for SetTrustedProxies|ForwardedByClientIP|TrustedPlatform|RemoteIPHeaders|X-Forwarded-For|CF-Connecting-IP across the whole tree returns zero hits. So Gin ran on its own default, and the chain is:

Where	What it does
gin@v1.12.0 gin.go:39-47	defaultTrustedCIDRs = [0.0.0.0/0, ::/0] - every peer is a trusted proxy
gin.go:482-501	validateHeader walks X-Forwarded-For right to left and stops at the first address that is not a trusted proxy
context.go:975-1024	ClientIP() finds TrustedPlatform empty, the peer trusted, ForwardedByClientIP true, and calls validateHeader

With everything trusted, the walk never stops and returns the leftmost value - the one the caller supplied. X-Real-IP is in Gin's default RemoteIPHeaders too, so it was a second route to the same result.

Consequence today: t_login_credential_log.client_ip (written at four points in the login flow) and t_user.last_login_ip (three writers) record an attacker-chosen value, so the forensic value of both is nil. Consequence for anything built on top: an IP-keyed control is bypassable by editing a header.

Fix. ServerConfig gains trustedProxies and trustedPlatform, applied in NewServer before any middleware is registered.

• Unset, trustedProxies falls back to loopback / RFC1918 / IPv6 unique-local / link-local rather than to Gin's trust-everything default. That covers a sidecar tunnel, a compose network and a local nginx, and a client on the public internet cannot present one of those addresses as its direct peer. Direct exposure still resolves to the real peer, because a public peer is not in the list.
• trustedPlatform maps cloudflare, fly.io and google-app-engine onto Gin's own constants and passes anything else through as a literal header name. It defaults to empty on purpose: an edge that appends rather than overwrites would turn the header back into a client-controlled value. .env.example sets it to cloudflare because this deployment sits behind a Cloudflare Tunnel, where CF-Connecting-IP is overwritten and therefore not forgeable.
• An unparseable CIDR fails startup instead of quietly dropping the trust boundary.

2. Credential lockout was a denial of service

isCredentialLocked keyed on the username alone. Anyone who knew a username - admin is not a secret - could lock the real account out for the full credentialLockMinute window, from anywhere, and renew it indefinitely by repeating. That is a DoS wearing a protection's clothes.

• The per-account window is now keyed on principal and client address, so one source grinding on one account is still stopped.
• A second window counts failures from one address across all principals, which is what credential stuffing looks like. auth.maxFailedAttemptsPerIP, defaulting to four times maxFailedAttempts; disabling maxFailedAttempts disables both, matching the existing convention.
• An address the server cannot determine normalises to one unknown bucket and is excluded from the per-address window, so a missing IP cannot pool every such caller into a single lockout.
• createLoginCredentialLog normalises on write, so the read and write sides cannot drift apart.

Behaviour change operators need to know

A user who previously got locked out because somebody else failed passwords on their username will no longer be locked out. Conversely, an attacker rotating source addresses is no longer stopped by the per-account window - that is what the per-address window and, separately, rate limiting are for. Nothing else about the login contract changes.

Tests

TestGinDefaultTrustsEveryProxy builds a bare engine and asserts that a forged X-Forwarded-For is honoured. It exists so the fix stays load-bearing: if a future Gin changes its default, that test fails and says so, rather than leaving the configuration silently decorative.

Three more cover an untrusted peer's forged header being ignored, a trusted proxy's appended address beating client-prepended junk, and the platform header taking precedence over X-Forwarded-For. A fourth asserts startup fails on a bad CIDR.

Two lockout tests cover the denial of service being closed - the attacking address is locked while the legitimate owner arriving from elsewhere gets a session - and the per-address window catching a four-username spray that no single account trips.

Four existing lockout tests seeded credential logs without a client address, which is exactly the assumption this removes; they failed once the key changed and now seed one. That failure is the point: the old tests encoded the vulnerable semantics.

Config helpers are covered too, including that TRUSTED_PROXIES="10.0.0.0/8,172.16.0.0/12" arrives as a two-element slice rather than one string - Gin rejects an unparseable CIDR at startup, so that conversion failing would take the process down rather than degrade quietly.

Verification run locally

go build -tags dev ./..., go vet -tags dev ./... clean, and go test -count=1 -tags dev across services, repositories, pkg, migration, bootstrap, builders, handlers all ok.

Not in this pull request

The upstream port is deliberately deferred. This is upstream code, so it belongs in huabeitech/agent-desk too, but it cannot be produced the way the earlier upload fix was. That one touched eight files that were byte-identical between dev and upstream/main, so the commit could be replayed directly. These files are not: internal/pkg/config/config.go carries fork-only configuration on dev, and internal/bootstrap/server.go on dev already contains the storage hardening that is still sitting in the open upstream PR huabeitech#40. Copying either file across would ship fork configuration to upstream and duplicate an open pull request. Porting it needs the hunks applied to upstream's versions by hand, which is a separate piece of work.

Rate limiting (SEC-08) is also not here. It is the third step in this chain and depends on the trustworthy address this establishes.

---

Note

High Risk
Changes authentication abuse controls and how client IPs are derived for login logs, last-login IP, and lockouts—misconfigured trusted proxies could still mis-identify clients or weaken protections.

Overview
Closes two linked security issues: forgeable client IPs and username-only lockout that anyone could trigger as a DoS.

Client IP. NewServer now applies server.trustedProxies and server.trustedPlatform before middleware runs, replacing Gin’s default of trusting every peer (so X-Forwarded-For could be chosen by the caller). Empty proxy config falls back to private/loopback CIDRs; Cloudflare and similar platforms map to edge headers like CF-Connecting-IP. Bad CIDRs fail startup. Example env/YAML documents TRUSTED_PROXIES and TRUSTED_PLATFORM.

Credential lockout. Failed attempts are counted per username + client IP, plus a new auth.maxFailedAttemptsPerIP window (default 4× per-account limit) for stuffing from one address across many usernames. IPs are normalized on write; unknown IPs skip the per-IP bucket. Lockout checks now take clientIP, which flows from ctx.ClientIP() on login.

Tests pin Gin’s vulnerable default, verify trusted/untrusted/platform behavior in NewServer, and regression-test address-scoped lockout vs cross-IP login success.

^{Reviewed by Cursor Bugbot for commit b748036. Configure here.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a783634c-6e31-44bd-802a-074578b96e23

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_42da26f6-ab6e-49c4-8ced-a6d978eb2c1d)

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.