diff --git a/.changeset/attempt-scoped-flow-limits.md b/.changeset/attempt-scoped-flow-limits.md new file mode 100644 index 00000000..5309b5a1 --- /dev/null +++ b/.changeset/attempt-scoped-flow-limits.md @@ -0,0 +1,10 @@ +--- +'seamless-auth-api': patch +--- + +OTP and magic link limits and links are scoped to the sign-in attempt, so a stranger who knows an address can no longer use up or void the owner's codes and links (#468). + +- The OTP and magic link identity limiters key `flow_rate_limits.*.perIdentity` on the identifier plus the ephemeral token's attempt (`jti`), or the requester's IP when there is none. Another attempt for the same address keeps its own allowance. +- Every attempt for one identifier together is capped at four times `perIdentity` (20 per window by default), so minting fresh attempts still cannot message an address without bound. +- `POST /magic-link` expires only earlier links from the same attempt instead of every link for the account. +- `FLOW_RATE_LIMITS` / `flow_rate_limits` keep their shape and meaning. No new config key. diff --git a/.changeset/registration-otp-login-methods.md b/.changeset/registration-otp-login-methods.md new file mode 100644 index 00000000..05becba2 --- /dev/null +++ b/.changeset/registration-otp-login-methods.md @@ -0,0 +1,11 @@ +--- +'seamless-auth-api': patch +--- + +The registration email OTP path honours `login_methods` for accounts that are already verified (#470). + +- `POST /otp/verify-email-otp` signs an already verified account in only when `email_otp` is a login method (and the passkey rules allow it). Otherwise it answers `403 login_method_disabled` before checking the code, the same answer as `/otp/verify-login-email-otp`. Before, a deployment that removed `email_otp` from `login_methods` could still be signed in to with an email code through this endpoint. +- `POST /otp/generate-email-otp` (and its deprecated `GET`) refuses an already verified account the same way, and its OpenAPI responses now list `403`. +- An unverified account still receives and verifies its registration code whatever `login_methods` says. +- The session is marked as an account verification only when the code actually moved the account from unverified to verified, so the phishing-resistant-only backstop in session issuance stays in force for an existing account. +- Decoy continuations answer both endpoints the same way a verified account is answered. diff --git a/docs/architecture.md b/docs/architecture.md index 0f878412..a16c314c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -91,6 +91,11 @@ Supported login methods are controlled by `login_methods` system config: - `phone_otp` - `oauth` +Email OTP is also how registration proves an address, so `/otp/generate-email-otp` and +`/otp/verify-email-otp` serve an unverified account whatever `login_methods` says. For an account +that is already verified those two endpoints are a sign-in, and they answer +`403 login_method_disabled` unless `email_otp` is listed (and the passkey rules below allow it). + Accounts holding a passkey can be restricted to passkey-only continuation by disabling `passkey_login_fallback_enabled`. `phishing_resistant_only` goes further and restricts every account to passkeys, apart from the one session that verifies a new account's address. Both diff --git a/docs/configuration.md b/docs/configuration.md index ad978387..8d6cb944 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -430,9 +430,13 @@ Validation is enforced by [`systemConfig.schema.ts`](../src/schemas/systemConfig carry a second, tighter set of limits, `flow_rate_limits`, applied per 15-minute window (the `windowSeconds` default) and split two ways per flow: -- `perIdentity` bounds how often one address or phone can be messaged (`otp`, `magicLink`) and - how often one provider can be started from one address (`oauth.perProvider`). This is the - abuse a sender cares about, and it rarely needs changing. +- `perIdentity` bounds how often one sign-in attempt can message one address or phone (`otp`, + `magicLink`), keyed on the identifier plus the ephemeral token's attempt (`jti`, or the + requester's IP without one), and how often one provider can be started from one address + (`oauth.perProvider`). All attempts for one identifier together are capped at four times + `perIdentity`, so a stranger starting attempts for someone else's address cannot use up the + owner's allowance ([security posture](./security-posture.md#code-and-link-sends-per-address)). + This is the abuse a sender cares about, and it rarely needs changing. - `perIp` bounds how many distinct flows one network location can drive. This is the enumeration and spam guard, and it is the one a mobile audience runs into: carriers put thousands of subscribers behind one IPv4 address, so `otp.perIp: 10` refuses a phone diff --git a/docs/security-posture.md b/docs/security-posture.md index f4393303..9f9f5cb3 100644 --- a/docs/security-posture.md +++ b/docs/security-posture.md @@ -204,8 +204,9 @@ Ephemeral tokens ([`src/lib/token.ts`](../src/lib/token.ts)) are validated by si but are not single-use, so a leaked one can be replayed until it expires. The window is 5 minutes. Every continuation endpoint that accepts an ephemeral token is behind the -OTP, magic-link, or registration rate limiters, which are keyed per IP and per identity, so replay -cannot be used to generate OTPs in bulk. An ephemeral token also cannot be exchanged for anything +OTP, magic-link, or registration rate limiters, which are keyed per IP, per identity and attempt, +and per identity across attempts (see [Code and link sends per address](#code-and-link-sends-per-address)), +so replay cannot be used to generate OTPs in bulk. An ephemeral token also cannot be exchanged for anything on its own: it authorizes a continuation step that still requires the OTP code, the magic-link token, or a WebAuthn assertion. @@ -485,6 +486,31 @@ factor. Enrolling a passkey keeps a user able to sign in through a lock someone The counter lives in Postgres, so every instance of a deployment sees the same count. When it cannot be read, the account is treated as locked (see [An unknown count means locked](#an-unknown-count-means-locked)). +## Code and link sends per address + +**Posture: one attempt cannot spend or void another attempt's codes or links.** + +The same thing that makes lockout a denial-of-service lever applies to message sends: `/login` +mints an ephemeral token from an address alone, so anyone can start attempts for someone else's +address. Previously the OTP and magic link identity limiters keyed on the address only, and a +magic link request expired every earlier link for the account, so a stranger could keep those +methods unavailable to the owner. + +- **Per attempt.** `flow_rate_limits.otp.perIdentity` and `magicLink.perIdentity` (default 5 per + 15 minutes) bound one attempt for one address: the address plus the ephemeral token's `jti`, + or the requester's IP for a request without one. Another attempt for the same address has its + own allowance. +- **Per address.** Every attempt for one address together is capped at four times `perIdentity` + (20 by default), so minting fresh attempts still cannot message an address without bound. With + the default limits a single network location cannot reach the OTP cap, because `otp.perIp` is + 10; the magic link cap equals `magicLink.perIp`, so one location can just reach it. +- **Links.** Requesting a magic link expires only earlier links from the same attempt. A link + another attempt is waiting on stays usable until it expires or is used. + +What remains: a stranger with several network locations can still fill the per-address cap for the +window, and the owner then waits it out, as with lockout. Wrong codes from every attempt still +count toward lockout together, because lockout is per account (see [Account lockout](#account-lockout)). + ## Audit write failure **Posture: the trail may be lost, no security control may be.** diff --git a/openapi.json b/openapi.json index b5e8bc7f..194355fe 100644 --- a/openapi.json +++ b/openapi.json @@ -9973,6 +9973,19 @@ } } }, + "403": { + "description": "HTTP 403", + "content": { + "application/json": { + "example": { "message": "string", "error": "string" }, + "schema": { + "type": "object", + "properties": { "message": { "type": "string" }, "error": { "type": "string" } }, + "required": ["error"] + } + } + } + }, "429": { "description": "HTTP 429", "content": { @@ -10078,6 +10091,19 @@ } } }, + "403": { + "description": "HTTP 403", + "content": { + "application/json": { + "example": { "message": "string", "error": "string" }, + "schema": { + "type": "object", + "properties": { "message": { "type": "string" }, "error": { "type": "string" } }, + "required": ["error"] + } + } + } + }, "429": { "description": "HTTP 429", "content": { diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index c931b02b..322cf29d 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 99.2% + + coverage: 99.1% @@ -17,7 +17,7 @@ coverage coverage - 99.2% - 99.2% + 99.1% + 99.1% diff --git a/src/controllers/decoyResponders.ts b/src/controllers/decoyResponders.ts index 30f0d741..67e3847d 100644 --- a/src/controllers/decoyResponders.ts +++ b/src/controllers/decoyResponders.ts @@ -123,8 +123,13 @@ async function respondOtpSent(req: Request, res: Response, kind: 'otp_email' | ' return res.status(200).json({ message: 'success', token }); } -export const decoySendEmailOtp = (req: Request, res: Response) => - respondOtpSent(req, res, 'otp_email'); +export const decoySendEmailOtp = async (req: Request, res: Response) => { + if (await rejectDisabledMethod('email_otp', req, res)) { + return; + } + + return respondOtpSent(req, res, 'otp_email'); +}; export const decoySendPhoneOtp = (req: Request, res: Response) => respondOtpSent(req, res, 'otp_sms'); @@ -157,8 +162,8 @@ async function respondOtpVerifyFailed(req: Request, res: Response) { } /** - * Mirrors the passkey rule on the paths a real verified account cannot use once a passkey - * is required: email verification, which signs such an account in, and TOTP login. + * Mirrors the passkey rule on TOTP login, which a real verified account cannot use once a + * passkey is required. */ async function rejectPasskeyRequired(method: string, req: Request, res: Response) { const policy = await getLoginPolicy(); @@ -179,7 +184,7 @@ async function rejectPasskeyRequired(method: string, req: Request, res: Response } export const decoyVerifyEmailOtp = async (req: Request, res: Response) => { - if (await rejectPasskeyRequired('email_otp', req, res)) { + if (await rejectDisabledMethod('email_otp', req, res)) { return; } diff --git a/src/controllers/magicLinks.ts b/src/controllers/magicLinks.ts index 30fedc90..8f139029 100644 --- a/src/controllers/magicLinks.ts +++ b/src/controllers/magicLinks.ts @@ -123,12 +123,14 @@ export async function requestMagicLink(req: MagicLinkRequest, res: Response) { await logMagicLinkFailure(req, 'Ephemeral token has no attempt id', user.id); return res.status(400).json({ error: 'Invalid request' }); } - // Expire all previous links + // Only this attempt's earlier links. Anyone who knows the address can start an attempt, + // so expiring every link for the user would let them void the one the owner is opening. await MagicLinkToken.update( { expires_at: new Date() }, { where: { user_id: user.id, + attempt_id: authReq.attemptId, }, }, ); diff --git a/src/controllers/otp.ts b/src/controllers/otp.ts index 47908d64..d04c9a67 100644 --- a/src/controllers/otp.ts +++ b/src/controllers/otp.ts @@ -224,6 +224,16 @@ export const sendLoginPhoneOTP = async (req: Request, res: Response) => { return sendPhoneOTP(req, res); }; +export const sendRegistrationEmailOTP = async (req: Request, res: Response) => { + const user = (req as AuthenticatedRequest).user; + + if (user.verified && (await rejectDisabledLoginMethod('email_otp', req, res))) { + return; + } + + return sendEmailOTP(req, res); +}; + export const sendLoginEmailOTP = async (req: Request, res: Response) => { if (await rejectDisabledLoginMethod('email_otp', req, res)) { return; @@ -312,25 +322,19 @@ export const verifyEmail = async (req: Request, res: Response) => { let user = authReq.user; const email = user.email; - // This endpoint issues a session for an already-verified account, so it is a login - // whatever its name says, and the lockout policy has to bind here too. Without it, - // an account locked out of /otp/verify-login-email-otp could still authenticate - // through this one. The login-method list deliberately does not gate it: email OTP - // is how registration proves an address, whether or not the deployment offers it as - // a way to sign in. A rule that requires a passkey does, once the account is already - // verified, or this endpoint would be the way around it. + // For an account that was already verified this endpoint is a login whatever its name + // says, so the lockout policy and the login method list bind here as they do on + // /otp/verify-login-email-otp. Only an unverified account skips the method list, since + // email OTP is how registration proves an address whether or not the deployment + // offers it as a way to sign in. if (await rejectIfUserLocked({ userId: user.id, req, res })) { return; } - if (user.verified && (await isPasskeyRequiredForUser(user.id))) { - await AuthEventService.log({ - userId: user.id, - type: 'login_failed', - req, - metadata: { reason: 'Passkey required', method: 'email_otp' }, - }); - return res.status(403).json({ error: 'login_method_disabled' }); + const wasVerified = Boolean(user.verified); + + if (wasVerified && (await rejectDisabledLoginMethod('email_otp', req, res))) { + return; } logger.info('Verifying email'); @@ -402,7 +406,7 @@ export const verifyEmail = async (req: Request, res: Response) => { await issueSessionAndRespond({ method: 'email_otp', - accountVerification: true, + accountVerification: !wasVerified, user: { id: user.id, email: user.email, diff --git a/src/generated/api.ts b/src/generated/api.ts index 085dfd7e..e2a1afd7 100644 --- a/src/generated/api.ts +++ b/src/generated/api.ts @@ -9864,6 +9864,24 @@ export interface paths { }; }; }; + /** @description HTTP 403 */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + /** + * @example { + * "message": "string", + * "error": "string" + * } + */ + 'application/json': { + message?: string; + error: string; + }; + }; + }; /** @description HTTP 429 */ 429: { headers: { @@ -9976,6 +9994,24 @@ export interface paths { }; }; }; + /** @description HTTP 403 */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + /** + * @example { + * "message": "string", + * "error": "string" + * } + */ + 'application/json': { + message?: string; + error: string; + }; + }; + }; /** @description HTTP 429 */ 429: { headers: { diff --git a/src/middleware/rateLimit.ts b/src/middleware/rateLimit.ts index c82f3e95..02797014 100644 --- a/src/middleware/rateLimit.ts +++ b/src/middleware/rateLimit.ts @@ -61,6 +61,19 @@ function getOtpIdentityKey(req: Request) { return `ip:${clientAddressKey(req)}`; } +// Scopes an identifier's key to the attempt carrying the request, so one attempt cannot +// spend another's allowance. The attempt is the ephemeral token's jti, which bearer auth +// has already set by the time route middleware runs. A request without one falls back to +// the requester's address. +function withAttempt(identityKey: (req: Request) => string) { + return (req: Request) => { + const { attemptId } = req as AuthenticatedRequest; + const scope = attemptId ? `attempt:${attemptId}` : `ip:${clientAddressKey(req)}`; + + return `${identityKey(req)}:${scope}`; + }; +} + function getOAuthFlowKey(req: Request) { return [req.params?.providerId ?? 'unknown-provider', clientAddressKey(req)].join(':'); } @@ -119,13 +132,39 @@ function createFlowLimiter( }; } +/** + * `perIdentity` is enforced per attempt. Across every attempt for one identifier the cap + * is this many times `perIdentity`, so minting fresh attempts still cannot message an + * address without bound, while a stranger has to drive several attempts' worth of + * requests before the owner is refused. A multiple rather than its own config value, + * because `flow_rate_limits` is a shared schema and a new key there needs a types release. + */ +export const IDENTIFIER_ATTEMPT_MULTIPLIER = 4; + +function createIdentityLimiter( + pick: (limits: FlowRateLimits) => number, + identityKey: (req: Request) => string, +) { + const perAttempt = createFlowLimiter(pick, withAttempt(identityKey)); + const perIdentifier = createFlowLimiter( + (limits) => pick(limits) * IDENTIFIER_ATTEMPT_MULTIPLIER, + identityKey, + ); + + return function identityLimiter(req: Request, res: Response, next: NextFunction) { + return perAttempt(req, res, (error?: unknown) => + error ? next(error) : perIdentifier(req, res, next), + ); + }; +} + const magicLinkIpCachedLimiter = createFlowLimiter((limits) => limits.magicLink.perIp); -const magicLinkIdentityCachedLimiter = createFlowLimiter( +const magicLinkIdentityCachedLimiter = createIdentityLimiter( (limits) => limits.magicLink.perIdentity, getMagicLinkIdentityKey, ); const otpIpCachedLimiter = createFlowLimiter((limits) => limits.otp.perIp); -const otpIdentityCachedLimiter = createFlowLimiter( +const otpIdentityCachedLimiter = createIdentityLimiter( (limits) => limits.otp.perIdentity, getOtpIdentityKey, ); diff --git a/src/routes/otp.routes.ts b/src/routes/otp.routes.ts index 2f748e57..bd31f281 100644 --- a/src/routes/otp.routes.ts +++ b/src/routes/otp.routes.ts @@ -15,10 +15,10 @@ import { decoyVerifyPhoneOtp, } from '../controllers/decoyResponders.js'; import { - sendEmailOTP, sendLoginEmailOTP, sendLoginPhoneOTP, sendPhoneOTP, + sendRegistrationEmailOTP, verifyEmail, verifyLoginEmail, verifyLoginPhoneNumber, @@ -46,11 +46,12 @@ otpRouter.post( response: { 200: MessageSchema, 400: ErrorSchema, + 403: ErrorSchema, 500: InternalErrorSchema, }, }, }, - sendEmailOTP, + sendRegistrationEmailOTP, ); otpRouter.get( @@ -70,11 +71,12 @@ otpRouter.get( response: { 200: MessageSchema, 400: ErrorSchema, + 403: ErrorSchema, 500: InternalErrorSchema, }, }, }, - sendEmailOTP, + sendRegistrationEmailOTP, ); otpRouter.post( diff --git a/tests/e2e/authFlow.spec.ts b/tests/e2e/authFlow.spec.ts index 8e381895..ba74d495 100644 --- a/tests/e2e/authFlow.spec.ts +++ b/tests/e2e/authFlow.spec.ts @@ -45,8 +45,11 @@ beforeAll(async () => { beforeEach(() => { vi.clearAllMocks(); + // The shared auth mock injects an already verified account, which only signs in + // through email verification while email OTP is a login method. (getSystemConfig as any).mockResolvedValue({ default_roles: ['user'], + login_methods: ['passkey', 'email_otp'], }); (signEphemeralToken as any).mockResolvedValue('ephemeral-token'); diff --git a/tests/integration/authentication/decoyContinuation.spec.ts b/tests/integration/authentication/decoyContinuation.spec.ts index 74d9bd15..1332c9b8 100644 --- a/tests/integration/authentication/decoyContinuation.spec.ts +++ b/tests/integration/authentication/decoyContinuation.spec.ts @@ -166,6 +166,26 @@ describe('decoy continuation: OTP', () => { expect(res.status).toBe(403); expect(res.body).toEqual({ error: 'login_method_disabled' }); }); + + // A real verified account is refused the registration code path too once email OTP + // is off, and a decoy stands in for a verified account. + it.each([ + ['/otp/generate-email-otp', 'get'], + ['/otp/verify-email-otp', 'post'], + ] as const)('refuses %s when email OTP is not a login method', async (path, method) => { + (getSystemConfig as any).mockResolvedValue({ + login_methods: ['passkey', 'magic_link'], + passkey_login_fallback_enabled: true, + }); + + const res = + method === 'get' + ? await request(app).get(path) + : await request(app).post(path).send({ verificationToken: '123456' }); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'login_method_disabled' }); + }); }); describe('decoy continuation: magic link', () => { @@ -301,6 +321,7 @@ describe('decoy continuation: TOTP', () => { describe('decoy continuation: passkey required', () => { const PASSKEY_ONLY_PATHS: [string, 'get' | 'post'][] = [ ['/otp/generate-login-email-otp', 'get'], + ['/otp/generate-email-otp', 'get'], ['/otp/verify-login-email-otp', 'post'], ['/otp/verify-email-otp', 'post'], ['/magic-link', 'get'], diff --git a/tests/integration/magicLink/magicLink.spec.ts b/tests/integration/magicLink/magicLink.spec.ts index 6ee2ef09..40744078 100644 --- a/tests/integration/magicLink/magicLink.spec.ts +++ b/tests/integration/magicLink/magicLink.spec.ts @@ -120,6 +120,21 @@ describe('GET /magic-link', () => { expect(MagicLinkToken.create).toHaveBeenCalled(); }); + // Anyone who knows the address can start an attempt, so expiring every link for the + // user let them void the one the owner was about to open. + it("expires only the requesting attempt's earlier links", async () => { + (MagicLinkToken.update as any).mockResolvedValue([1]); + (MagicLinkToken.create as any).mockResolvedValue({}); + + const res = await request(app).post('/magic-link'); + + expect(res.status).toBe(200); + expect(MagicLinkToken.update).toHaveBeenCalledWith( + { expires_at: expect.any(Date) }, + { where: { user_id: 'user-1', attempt_id: 'attempt-1' } }, + ); + }); + it('returns external magic-link delivery payload without direct email delivery', async () => { (MagicLinkToken.update as any).mockResolvedValue([1]); (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); diff --git a/tests/integration/otp/otp.security.spec.ts b/tests/integration/otp/otp.security.spec.ts index 17e73878..5cdc77ca 100644 --- a/tests/integration/otp/otp.security.spec.ts +++ b/tests/integration/otp/otp.security.spec.ts @@ -3,6 +3,7 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { Application } from 'express'; import { createApp } from '../../../src/app'; +import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; import { verifyPhoneOTP, verifyEmailOTP } from '../../../src/utils/otp.js'; let app: Application; @@ -14,6 +15,9 @@ beforeAll(async () => { beforeEach(() => { vi.resetModules(); vi.clearAllMocks(); + // The shared auth mock injects an already verified account, which email verification + // refuses unless email OTP is a login method. + (getSystemConfig as any).mockResolvedValue({ login_methods: ['passkey', 'email_otp'] }); }); describe('OTP Security - Phone Verification', () => { diff --git a/tests/integration/otp/otp.spec.ts b/tests/integration/otp/otp.spec.ts index 85cd641e..eeeeacfd 100644 --- a/tests/integration/otp/otp.spec.ts +++ b/tests/integration/otp/otp.spec.ts @@ -78,6 +78,34 @@ describe('OTP - Generate', () => { }); }); +describe('OTP - Registration path for a verified account', () => { + beforeEach(() => { + (getSystemConfig as any).mockResolvedValue({ + login_methods: ['passkey', 'magic_link'], + passkey_login_fallback_enabled: true, + }); + }); + + it('refuses to send a registration code when email OTP is not a login method', async () => { + const res = await request(app).post('/otp/generate-email-otp'); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'login_method_disabled' }); + expect(generateEmailOTP).not.toHaveBeenCalled(); + }); + + it('refuses to sign in through email verification when email OTP is not a login method', async () => { + const res = await request(app) + .post('/otp/verify-email-otp') + .send({ verificationToken: '123456' }); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'login_method_disabled' }); + expect(verifyEmailOTP).not.toHaveBeenCalled(); + expect(issueSessionAndRespond).not.toHaveBeenCalled(); + }); +}); + describe('OTP - Verify Phone', () => { it('fails when token missing', async () => { const res = await request(app).post('/otp/verify-phone-otp').send({}); diff --git a/tests/integration/rateLimit/identityFlowLimits.spec.ts b/tests/integration/rateLimit/identityFlowLimits.spec.ts new file mode 100644 index 00000000..c7fb9417 --- /dev/null +++ b/tests/integration/rateLimit/identityFlowLimits.spec.ts @@ -0,0 +1,116 @@ +import express, { Application, NextFunction, Request, Response } from 'express'; +import request from 'supertest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// tests/setup/mocks.ts replaces the limiters with pass-throughs for the whole suite. +vi.unmock('../../../src/middleware/rateLimit'); + +import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; + +type IdentityLimiter = (req: Request, res: Response, next: NextFunction) => unknown; + +/** + * Mounts one identity limiter behind a stand-in for bearer auth, which is what sets the + * user and the attempt on a real ephemeral route before route middleware runs. + */ +async function appWith(name: 'otpIdentityLimiter' | 'magicLinkEmailLimiter') { + vi.resetModules(); + const limiters = await import('../../../src/middleware/rateLimit.js'); + const limiter = limiters[name] as IdentityLimiter; + const app = express(); + + app.post( + '/flow', + (req, _res, next) => { + Object.assign(req, { user: { email: req.get('x-test-email'), phone: null } }); + const attempt = req.get('x-test-attempt'); + if (attempt) Object.assign(req, { attemptId: attempt }); + next(); + }, + limiter, + (_req, res) => { + res.status(200).json({ ok: true }); + }, + ); + + return app as Application; +} + +function send(app: Application, email: string, attempt?: string) { + const call = request(app).post('/flow').set('x-test-email', email); + return attempt ? call.set('x-test-attempt', attempt) : call; +} + +async function status(app: Application, email: string, attempt?: string) { + return (await send(app, email, attempt)).status; +} + +beforeEach(() => { + vi.clearAllMocks(); + (getSystemConfig as ReturnType).mockResolvedValue({}); +}); + +describe.each(['otpIdentityLimiter', 'magicLinkEmailLimiter'] as const)('%s', (name) => { + it('refuses an attempt past perIdentity', async () => { + const app = await appWith(name); + + for (let i = 0; i < 5; i++) { + expect(await status(app, 'owner@example.com', 'attempt-a')).toBe(200); + } + + expect(await status(app, 'owner@example.com', 'attempt-a')).toBe(429); + }); + + it('leaves another attempt for the same address its own allowance', async () => { + const app = await appWith(name); + + for (let i = 0; i < 6; i++) { + await send(app, 'owner@example.com', 'stranger'); + } + + expect(await status(app, 'owner@example.com', 'owner')).toBe(200); + }); + + it('caps every attempt for one address together at four times perIdentity', async () => { + const app = await appWith(name); + + for (let attempt = 0; attempt < 4; attempt++) { + for (let i = 0; i < 5; i++) { + expect(await status(app, 'owner@example.com', `attempt-${attempt}`)).toBe(200); + } + } + + expect(await status(app, 'owner@example.com', 'attempt-new')).toBe(429); + expect(await status(app, 'other@example.com', 'attempt-new')).toBe(200); + }); + + it('reads perIdentity from flow_rate_limits', async () => { + (getSystemConfig as ReturnType).mockResolvedValue({ + flow_rate_limits: { + windowSeconds: 900, + otp: { perIp: 10, perIdentity: 1 }, + magicLink: { perIp: 20, perIdentity: 1 }, + oauth: { perIp: 30, perProvider: 10 }, + }, + }); + const app = await appWith(name); + + expect(await status(app, 'owner@example.com', 'a')).toBe(200); + expect(await status(app, 'owner@example.com', 'a')).toBe(429); + for (const attempt of ['b', 'c', 'd']) { + expect(await status(app, 'owner@example.com', attempt)).toBe(200); + } + expect(await status(app, 'owner@example.com', 'e')).toBe(429); + }); + + it('scopes a request without an attempt to the requester address', async () => { + const app = await appWith(name); + + for (let i = 0; i < 5; i++) { + expect(await status(app, 'owner@example.com')).toBe(200); + } + + expect(await status(app, 'owner@example.com')).toBe(429); + expect(await status(app, 'owner@example.com', 'owner')).toBe(200); + }); +}); diff --git a/tests/unit/controllers/otp.spec.ts b/tests/unit/controllers/otp.spec.ts index 2b0afd77..61c81cad 100644 --- a/tests/unit/controllers/otp.spec.ts +++ b/tests/unit/controllers/otp.spec.ts @@ -366,7 +366,7 @@ describe('otp controller', () => { it('issues a session when email verification fully verifies the user', async () => { const { verifyEmail } = await loadOtpController(); const verifiedUser = buildUser(); - const req = buildReq(buildUser(), { + const req = buildReq(buildUser({ verified: false, emailVerified: false }), { body: { verificationToken: 'EMAILOTP' }, }); const res = buildRes(); @@ -1109,6 +1109,83 @@ describe('otp controller', () => { }); }); + describe('when email OTP is not a login method', () => { + const emailOtpOff = { + login_methods: ['passkey', 'magic_link'], + passkey_login_fallback_enabled: true, + }; + + // The bypass this closes: the registration path signed a verified account in with an + // email code whatever login_methods said. + it('refuses to send a registration code to an already verified account', async () => { + const { sendRegistrationEmailOTP } = await loadOtpController(); + getSystemConfigMock.mockResolvedValue(emailOtpOff); + const res = buildRes(); + + await sendRegistrationEmailOTP(buildReq(buildUser()), res); + + expect(generateEmailOTPMock).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith({ error: 'login_method_disabled' }); + }); + + it('refuses to sign an already verified account in through email verification', async () => { + const { verifyEmail } = await loadOtpController(); + getSystemConfigMock.mockResolvedValue(emailOtpOff); + const res = buildRes(); + + await verifyEmail(buildReq(buildUser(), { body: { verificationToken: 'EMAILOTP' } }), res); + + expect(verifyEmailOTPMock).not.toHaveBeenCalled(); + expect(issueSessionAndRespondMock).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith({ error: 'login_method_disabled' }); + expect(authEventLogMock).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'login_failed', + metadata: { reason: 'Login method disabled', method: 'email_otp' }, + }), + ); + }); + + it('still sends a code to and verifies an unverified account', async () => { + const { sendRegistrationEmailOTP, verifyEmail } = await loadOtpController(); + getSystemConfigMock.mockResolvedValue(emailOtpOff); + const newcomer = () => buildUser({ verified: false, emailVerified: false }); + verifyEmailOTPMock.mockResolvedValue({ user: buildUser(), verified: true }); + + const sendRes = buildRes(); + await sendRegistrationEmailOTP(buildReq(newcomer()), sendRes); + await verifyEmail( + buildReq(newcomer(), { body: { verificationToken: 'EMAILOTP' } }), + buildRes(), + ); + + expect(generateEmailOTPMock).toHaveBeenCalled(); + expect(sendRes.status).toHaveBeenCalledWith(200); + expect(issueSessionAndRespondMock).toHaveBeenCalledWith( + expect.objectContaining({ method: 'email_otp', accountVerification: true }), + ); + }); + }); + + it('signs a verified account in as a login rather than an account verification', async () => { + const { sendRegistrationEmailOTP, verifyEmail } = await loadOtpController(); + verifyEmailOTPMock.mockResolvedValue({ user: buildUser(), verified: true }); + + const sendRes = buildRes(); + await sendRegistrationEmailOTP(buildReq(buildUser()), sendRes); + await verifyEmail( + buildReq(buildUser(), { body: { verificationToken: 'EMAILOTP' } }), + buildRes(), + ); + + expect(sendRes.status).toHaveBeenCalledWith(200); + expect(issueSessionAndRespondMock).toHaveBeenCalledWith( + expect.objectContaining({ method: 'email_otp', accountVerification: false }), + ); + }); + describe('review accounts', () => { const flagged = (metadata: Record) => expect.objectContaining({ metadata: { ...metadata, reviewAccount: true } }); diff --git a/tests/unit/middleware/rateLimit.spec.ts b/tests/unit/middleware/rateLimit.spec.ts index ce843aeb..8529e843 100644 --- a/tests/unit/middleware/rateLimit.spec.ts +++ b/tests/unit/middleware/rateLimit.spec.ts @@ -202,11 +202,13 @@ function lastOptions(rateLimit: any) { } describe('flow limiters', () => { + // The identity limiters build two: per attempt, then the looser identifier-wide cap, + // which is the last one built. it.each([ ['magicLinkIpLimiter', 20], - ['magicLinkEmailLimiter', 5], + ['magicLinkEmailLimiter', 20], ['otpIpLimiter', 10], - ['otpIdentityLimiter', 5], + ['otpIdentityLimiter', 20], ['oauthIpLimiter', 30], ['oauthProviderLimiter', 10], ] as const)( @@ -242,10 +244,13 @@ describe('flow limiters', () => { const otpIp = lastOptions(rateLimit); // @ts-ignore await limiters.otpIdentityLimiter({ body: { email: 'a@b.c' } }, {}, vi.fn()); + const calls = rateLimit.default.mock.calls; + const otpAttempt = calls[calls.length - 2][0]; const otpIdentity = lastOptions(rateLimit); await expect(otpIp.limit()).resolves.toBe(500); - await expect(otpIdentity.limit()).resolves.toBe(5); + await expect(otpAttempt.limit()).resolves.toBe(5); + await expect(otpIdentity.limit()).resolves.toBe(20); }); it('takes a changed limit on the next hit without rebuilding the limiter', async () => { @@ -326,6 +331,58 @@ describe('otpIdentityLimiter', () => { }); }); +describe('identity limiters', () => { + async function attemptAndIdentifierKeys(name: 'otpIdentityLimiter' | 'magicLinkEmailLimiter') { + const { rateLimit, limiters } = await loadFlowLimiters(); + // @ts-ignore + await limiters[name]({ params: {} }, {}, vi.fn()); + const calls = rateLimit.default.mock.calls; + + return { + attempt: calls[calls.length - 2][0].keyGenerator, + identifier: calls[calls.length - 1][0].keyGenerator, + }; + } + + it.each(['otpIdentityLimiter', 'magicLinkEmailLimiter'] as const)( + '%s keys the strict bucket on identifier and attempt', + async (name) => { + const { attempt, identifier } = await attemptAndIdentifierKeys(name); + const owner = { user: { email: 'Owner@x.com' }, attemptId: 'owner-attempt' }; + const stranger = { user: { email: 'owner@x.com' }, attemptId: 'stranger-attempt' }; + + expect(attempt(owner)).toBe('email:owner@x.com:attempt:owner-attempt'); + expect(attempt(owner)).not.toBe(attempt(stranger)); + expect(identifier(owner)).toBe(identifier(stranger)); + }, + ); + + it('falls back to the requester address when there is no attempt', async () => { + const { attempt } = await attemptAndIdentifierKeys('otpIdentityLimiter'); + + expect(attempt({ user: { email: 'a@x.com' }, ip: '1.2.3.4' })).toBe('email:a@x.com:ip:1.2.3.4'); + }); + + it('passes an attempt bucket error on without consulting the identifier cap', async () => { + const { rateLimit, limiters } = await loadFlowLimiters(); + const next = vi.fn(); + // @ts-ignore + await limiters.otpIdentityLimiter({ params: {} }, {}, vi.fn()); + const calls = rateLimit.default.mock.calls; + const attemptLimiter = rateLimit.default.mock.results[calls.length - 2].value; + const identifierLimiter = rateLimit.default.mock.results[calls.length - 1].value; + const failure = new Error('store unavailable'); + attemptLimiter.mockImplementationOnce((_req: any, _res: any, done: any) => done(failure)); + identifierLimiter.mockClear(); + + // @ts-ignore + await limiters.otpIdentityLimiter({ params: {} }, {}, next); + + expect(next).toHaveBeenCalledWith(failure); + expect(identifierLimiter).not.toHaveBeenCalled(); + }); +}); + describe('rate limiter key generators', () => { async function keyGenerators() { const { rateLimit, limiters } = await loadFlowLimiters(); @@ -418,7 +475,7 @@ describe('refusal body', () => { const messages = (rateLimit.default as any).mock.calls.map( ([options]: any[]) => options.message, ); - expect(messages).toHaveLength(8); + expect(messages).toHaveLength(10); for (const message of messages) { expect(message).toEqual({ error: 'Too many requests, please try again later' }); } @@ -486,12 +543,12 @@ describe('rate limiter caches', () => { // @ts-ignore await magicLinkIpLimiter({}, {}, next); - // One general limiter at load, then one per flow limiter on first use, and a - // repeat hit reuses the instance it built. - expect(rateLimit.default).toHaveBeenCalledTimes(3); + // One general limiter at load, then one per flow limiter on first use (two for the + // identity limiter), and a repeat hit reuses the instance it built. + expect(rateLimit.default).toHaveBeenCalledTimes(4); const limits = await Promise.all( (rateLimit.default as any).mock.calls.map(([options]: any[]) => options.limit()), ); - expect(limits).toEqual([100, 20, 5]); + expect(limits).toEqual([100, 20, 5, 20]); }); });