feat: add per-IP login rate limit alongside per-account lockout - #184
Open
jakubfilinger-b wants to merge 8 commits into
Open
jakubfilinger-b wants to merge 8 commits into
jakubfilinger-b wants to merge 8 commits into
Conversation
…kout
Adds a coarse per-IP throttle on /identity/login, separate from the
existing per-account lockout and per-email request throttle. Protects
against credential stuffing spread across many accounts from one
source, which the per-email counter can't see since each guess lands
on a different key.
- New RATE_LIMIT_KEYS.LOGIN_IP ('login-ip') key prefix.
- IdentityServiceOptions.loginIpRateLimit lets an operator tune or
disable the threshold; default is 100/15min, well above the 10/5min
per-email budget so a shared network (office NAT) is never blocked.
- Fails closed (onUnavailable: 'deny'), matching the other
credential-guessing keys.
- Skipped when the request has no IP, rather than bucketing under a
shared 'unknown' key - that shared key would let one IP-less
client's traffic exhaust the budget for every other IP-less client.
- Not reset on successful login (unlike the per-email key), so a
successful login on one account can't be used to clear the IP's
failure budget.
Tests cover: per-IP exhaustion across different accounts, per-IP
bucket isolation, the no-IP skip, the enabled:false override, the
fail-closed behavior on backend unavailability, and that locking one
account never blocks a different account signing in from the same IP.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jakubfilinger-b
requested review from
damianrzepka,
klaudia-blazyczek-blurify,
marek-chmielowski-blurify,
mp-blurify,
okapitula and
zaxovaiko
as code owners
September 17, 2026 09:54
damianrzepka
requested changes
Sep 17, 2026
damianrzepka
left a comment
damianrzepka
left a comment
Collaborator
There was a problem hiding this comment.
NO-GO — the per-IP credential-stuffing protection can be bypassed by spoofing the request IP.
…keting test Review feedback (PR #184, @damianrzepka): - IdentityLoginIpRateLimitOptions was defined but never re-exported from the @openora/core/contracts barrel alongside its siblings (IdentityServiceOptions, IdentityLockoutOptions), so a downstream operator had no supported way to import the type. - The 'buckets IPs separately' test asserted only .rejects.not.toMatchObject ({ code: 'TOO_MANY_REQUESTS' }), which also passes on an unrelated TypeError (signInEmail is unconfigured elsewhere in this file) - it never proved the unthrottled IP's login actually succeeded. Stubs a real signInEmail success now and asserts the returned session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-ip throttle Follow-up to review feedback (PR #184, @damianrzepka): the per-IP protection is only meaningful behind a reverse proxy that overwrites X-Real-IP with the real peer address before the request reaches this service - the header is otherwise attacker-controlled (extractClientMeta reads it unconditionally). That assumption already applies to register-ip:/verify-email-ip:/ change-email-ip:/confirm-email-change-ip: elsewhere in this file; this makes it explicit at both the option type and the call site rather than leaving it implicit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
Addressed the barrel-export and test-assertion findings directly (`f1fdbe80`). Replied inline on the IP-spoofing finding with the scope analysis - it's a real characteristic, but pre-existing and shared by 4 other per-IP throttles already in this file (`register-ip:`, `verify-email-ip:`, `change-email-ip:`, `confirm-email-change-ip:`), not something this diff introduces. Made the trust assumption explicit in the code (`9c9c69b3`) rather than leaving it implicit. Over to you on whether that resolves the NO-GO or you'd rather see it tracked as a separate issue against `extractClientMeta` before this merges. |
zaxovaiko
reviewed
Sep 17, 2026
zaxovaiko
reviewed
Sep 17, 2026
…g IP Review feedback (PR #184, @zaxovaiko): - The per-email gate ran before the per-IP one, so a single IP that had already tripped its own throttle could keep spending a targeted email's much narrower 10/5min budget on every further attempt. The IP gate now runs first, so an IP over its own limit is rejected before it ever touches an email's budget. - Skipping the per-IP gate outright when the IP can't be derived let an attacker disable the new protection for free by simply not sending X-Real-IP. Falls back to a shared 'unknown' bucket instead, matching every sibling per-IP key already in this file (register-ip:, verify-email-ip:, change-email-ip:, confirm-email-change-ip:). Tests: new coverage for the ordering fix (an over-limit IP can't touch the targeted email's budget) and the 'unknown' fallback; updated two existing tests whose assertions depended on the old ordering. 100/100 green.
zaxovaiko
previously approved these changes
Sep 17, 2026
zaxovaiko
left a comment
zaxovaiko
left a comment
Member
There was a problem hiding this comment.
More comments than actual code 😄
Every per-IP throttle (login-ip, register-ip, verify-email-ip, change-email-ip, confirm-email-change-ip) and the geo checks key on X-Real-IP, which createApp passed through verbatim from any caller. A direct client could rotate the header per request and never exhaust a bucket. createApp now decides the client address at the ingress: forwarding headers are kept only when the socket peer is a trusted proxy. Any other peer gets its socket address written over X-Real-IP and its X-Forwarded-For dropped. The list comes from `trustedProxies`, then TRUSTED_PROXIES, then loopback plus the private ranges, so an existing deployment behind nginx on the same host or private network keeps working. Entries are validated at boot; a malformed one fails createApp before anything is bound. In-process requests with no socket are left as they are.
…F-only proxies
A trusted proxy entry with an empty suffix (`10.0.0.1/`) parsed as /0, because
`Number('')` is 0, and trusted every IPv4 peer. Entries are now matched against an
anchored pattern, so an empty or second suffix is refused at boot.
A trusted proxy that sends only X-Forwarded-For left X-Real-IP empty, so every
login behind it shared the `login-ip:unknown` bucket. Behind a trusted proxy,
X-Real-IP now always holds a valid address. That is the proxy's own X-Real-IP
when it sent a usable one. Otherwise it is the client from X-Forwarded-For,
read right to left and stopping at the first untrusted hop. If neither gives
one, the proxy's socket address is used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a coarse per-IP throttle on
/identity/login, separate from the existing per-accountlockout (
user.failedLoginAttempts/lockoutUntil) and the existing per-email requestthrottle (
login:<email>, 10/5min).Why
The per-email throttle and per-account lockout both key on the email/account being attacked,
so they see repeated guessing against one account but are blind to credential stuffing
spread thin across many accounts from the same source (a botnet, a leaked-credential list).
A coarse per-IP counter closes that gap without touching the per-account behavior at all.
How
RATE_LIMIT_KEYS.LOGIN_IP('login-ip') added to the rate-limit key registry.IdentityServiceOptions.loginIpRateLimit(enabled?,limit?,windowMs?) lets adownstream operator tune or disable the threshold without a core change. Default is
100 requests / 15 min - well above the 10/5min per-email budget so a shared network
(office NAT) is never blocked.
onUnavailable: 'deny'), matching the other credential-guessing keys(
login:,pwreset:,verify2fa:, ...) - an unthrottled window during a limiter outageis worse than a transient 429 on this surface.
'unknown'key - that shared key would let one IP-less client's traffic exhaust thebudget for every other IP-less client, a self-inflicted DoS.
account can't be used to clear the IP's failure budget.
Tests
identity.rate-limit.int.test.ts(real Redis): per-IP exhaustion across different accounts,bucket isolation between IPs, the no-IP skip, the
enabled: falseoverride, fail-closedbehavior on backend unavailability.
identity.service.int.test.ts(real Postgres + Redis): locking one account never blocks adifferent account signing in from the same IP.
99/99 tests green (
pnpm -F @openora/core test:integration).check:types,check:lint,check:format,check:shapeclean.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com