From ba0b7d6255a781773a3e1ed8308344dfd12832d8 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 8 Jun 2026 21:04:40 -0400 Subject: [PATCH 1/9] fix: clean up otp code --- README.md | 7 +- docs/oauth.md | 9 +- resources/coverage-badge.svg | 14 +-- src/controllers/magicLinks.ts | 36 +++++++- src/controllers/oauth.ts | 10 ++- src/controllers/otp.ts | 12 ++- src/middleware/rateLimit.ts | 72 +++++++++++++++ src/routes/magicLink.routes.ts | 1 + src/routes/oauth.routes.ts | 3 + src/routes/otp.routes.ts | 5 ++ src/routes/registration.routes.ts | 2 + src/schemas/authEvent.types.ts | 1 + src/schemas/systemConfig.schema.ts | 1 + src/services/messagingService.ts | 4 + src/services/oauthService.ts | 87 ++++++++++++++++++- src/utils/otp.ts | 56 ++++++++++-- tests/integration/magicLink/magicLink.spec.ts | 33 +++++++ tests/integration/oauth/oauth.spec.ts | 16 ++++ tests/integration/otp/otp.security.spec.ts | 5 +- tests/integration/otp/otp.spec.ts | 2 +- tests/setup/mocks.ts | 4 + tests/unit/controllers/otp.spec.ts | 6 +- tests/unit/middleware/rateLimit.spec.ts | 79 ++++++++++++++++- tests/unit/services/messagingService.spec.ts | 29 +++++++ tests/unit/services/oauthService.spec.ts | 47 ++++++++++ tests/unit/utils/otp.spec.ts | 50 +++++++++-- 26 files changed, 552 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 1614f1e..1fba14b 100644 --- a/README.md +++ b/README.md @@ -195,7 +195,8 @@ system config. "nameJsonPath": "name", "allowSignup": true, "accountLinking": "email", - "requireEmailVerified": true + "requireEmailVerified": true, + "pkce": true } ] ``` @@ -235,6 +236,10 @@ curl -X POST http://localhost:5312/oauth/google/callback \ Security notes: - OAuth `state` is signed and expires after a short window. +- OAuth callback state is consumed in-process after first use. Use sticky callback routing or shared + state storage if you need replay detection across multiple API instances. +- PKCE is enabled by default; set `pkce: false` only for providers that cannot accept PKCE + parameters. - `redirectUri` must exactly match a provider `redirectUris` entry when configured. Providers without a redirect allowlist fall back to trusted configured origins. - `returnTo` must match configured `origins`. diff --git a/docs/oauth.md b/docs/oauth.md index 5b3cc5c..b5afb30 100644 --- a/docs/oauth.md +++ b/docs/oauth.md @@ -63,7 +63,14 @@ Use `requireEmailVerified: true` for providers that expose a reliable email veri ## OIDC Notes -When provider scopes include `openid`, Seamless Auth includes a nonce bound into signed state. PKCE support depends on provider and client flow requirements; keep provider callback handling server-side and avoid exposing provider tokens to browsers. +When provider scopes include `openid`, Seamless Auth includes a nonce bound into signed state. OAuth +start also includes PKCE challenge parameters by default, and the callback derives the matching +verifier server-side from the signed state. Set `pkce: false` only for providers that cannot accept +PKCE parameters. + +Callback state is consumed in-process after first use and expires after a short window. In a +multi-instance deployment, keep OAuth callback traffic sticky to the same API instance or add shared +state storage before relying on replay detection across instances. OAuth callback responses return the normal Seamless Auth JSON token payload. Provider tokens remain server-side and must not be forwarded to browser clients. diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index cd72161..e82144e 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 81.4% + + coverage: 82% @@ -7,17 +7,17 @@ - + - - + + coverage coverage - 81.4% - 81.4% + 82% + 82% diff --git a/src/controllers/magicLinks.ts b/src/controllers/magicLinks.ts index 5b155fc..1b8a9b6 100644 --- a/src/controllers/magicLinks.ts +++ b/src/controllers/magicLinks.ts @@ -43,6 +43,15 @@ async function rejectDisabledMagicLink(req: Request, res: Response, userId?: str return true; } +async function logMagicLinkFailure(req: Request, reason: string, userId?: string | null) { + await AuthEventService.log({ + userId: userId ?? null, + type: 'magic_link_failed', + req, + metadata: { reason }, + }); +} + export async function requestMagicLink(req: Request, res: Response) { const authReq = req as AuthenticatedRequest; const preAuthUser = authReq.user; @@ -82,7 +91,7 @@ export async function requestMagicLink(req: Request, res: Response) { }, ); - await MagicLinkToken.create({ + const magicLinkRecord = await MagicLinkToken.create({ user_id: user.id, token_hash: tokenHash, redirect_url, @@ -92,7 +101,23 @@ export async function requestMagicLink(req: Request, res: Response) { }); if (!useExternalDelivery) { - await sendMagicLinkEmail(user.email, rawToken, redirect_url); + try { + await sendMagicLinkEmail(user.email, rawToken, redirect_url); + } catch { + if (magicLinkRecord.id) { + await MagicLinkToken.update( + { expires_at: new Date() }, + { + where: { + id: magicLinkRecord.id, + }, + }, + ); + } + + await logMagicLinkFailure(req, 'Delivery failed', user.id); + return res.status(500).json({ error: 'Failed to deliver magic link' }); + } } await AuthEventService.log({ @@ -135,16 +160,19 @@ export async function verifyMagicLink(req: Request, res: Response) { if (!record) { logger.warn('No magic link found for supplied token'); + await logMagicLinkFailure(req, 'Invalid verification token'); return res.status(400).json({ error: 'Invalid verification token' }); } if (record.used_at) { logger.warn('Magic link token is already used'); + await logMagicLinkFailure(req, 'Token already used', record.user_id); return res.status(400).json({ error: 'Invalid verification token' }); } if (record.expires_at < new Date()) { logger.warn('Magic link token expired'); + await logMagicLinkFailure(req, 'Token expired', record.user_id); return res.status(400).json({ error: 'Invalid verification token' }); } @@ -163,6 +191,7 @@ export async function verifyMagicLink(req: Request, res: Response) { if (!updated) { logger.error('Magic link token was not consumed'); + await logMagicLinkFailure(req, 'Failed to consume token', record.user_id); return res.status(500).json({ error: 'Failed to use token' }); } @@ -209,6 +238,7 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { if (!record) { logger.warn('No magic link token'); + await logMagicLinkFailure(req, 'No active token found while polling', user.id); return res.status(500).json({ error: 'Invalid request' }); } @@ -216,10 +246,12 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { const { ip_hash, user_agent_hash } = hashDeviceFingerprint(req.ip, req.headers['user-agent']); if (record.ip_hash && record.ip_hash !== ip_hash) { + await logMagicLinkFailure(req, 'Polling device IP mismatch', user.id); return res.status(500).json({ error: 'Invalid request' }); } if (record.user_agent_hash && record.user_agent_hash !== user_agent_hash) { + await logMagicLinkFailure(req, 'Polling device user agent mismatch', user.id); return res.status(500).json({ error: 'Invalid request' }); } diff --git a/src/controllers/oauth.ts b/src/controllers/oauth.ts index d5c7a48..5883893 100644 --- a/src/controllers/oauth.ts +++ b/src/controllers/oauth.ts @@ -10,6 +10,9 @@ import { getSystemConfig } from '../config/getSystemConfig.js'; import { AuthEventService } from '../services/authEventService.js'; import { buildOAuthAuthorizationUrl, + consumeOAuthState, + createOAuthPkceCodeChallenge, + createOAuthPkceCodeVerifier, createOAuthState, exchangeOAuthCode, fetchOAuthProfile, @@ -59,6 +62,9 @@ export async function startOAuthLogin(req: Request, res: Response) { ...(returnTo ? { returnTo } : {}), }); const statePayload = verifyOAuthState(state, provider.id); + const codeChallenge = statePayload + ? createOAuthPkceCodeChallenge(provider, statePayload) + : undefined; await AuthEventService.log({ type: 'oauth_login_started', @@ -74,6 +80,7 @@ export async function startOAuthLogin(req: Request, res: Response) { redirectUri, state, ...(statePayload?.nonce ? { nonce: statePayload.nonce } : {}), + ...(codeChallenge ? { codeChallenge } : {}), }), }); } catch { @@ -98,7 +105,7 @@ export async function finishOAuthLogin(req: Request, res: Response) { return res.status(404).json({ error: 'OAuth provider not found' }); } - const statePayload = verifyOAuthState(state, provider.id); + const statePayload = consumeOAuthState(state, provider.id); if (!statePayload) { await AuthEventService.log({ @@ -114,6 +121,7 @@ export async function finishOAuthLogin(req: Request, res: Response) { provider, code, redirectUri: statePayload.redirectUri, + codeVerifier: createOAuthPkceCodeVerifier(provider, statePayload), }); const profile = await fetchOAuthProfile(provider, accessToken); const user = await resolveOAuthUser(provider, profile); diff --git a/src/controllers/otp.ts b/src/controllers/otp.ts index 60dbc3a..4a29f3d 100644 --- a/src/controllers/otp.ts +++ b/src/controllers/otp.ts @@ -380,9 +380,15 @@ export const verifyEmail = async (req: Request, res: Response) => { return res.json({ message: 'Success' }); } else { logger.error(`Verification tokens did not match or expired for email verification`); + await AuthEventService.log({ + userId: user.id, + type: 'verify_otp_failed', + req, + metadata: { reason: 'User verification failed for email' }, + }); + return res.status(401).json({ error: 'Not allowed' }); } - return res.status(500).json({ error: 'Internal server error' }); }; export const verifyLoginPhoneNumber = async (req: Request, res: Response) => { @@ -578,9 +584,9 @@ export const verifyLoginEmail = async (req: Request, res: Response) => { userId: user.id, type: 'verify_otp_failed', req, - metadata: { reason: 'User verification failed for phone' }, + metadata: { reason: 'User verification failed for email' }, }); + return res.status(401).json({ error: 'Not allowed' }); } - return res.status(500).json({ error: 'Internal server error' }); }; diff --git a/src/middleware/rateLimit.ts b/src/middleware/rateLimit.ts index 6ee5c78..67e05dd 100644 --- a/src/middleware/rateLimit.ts +++ b/src/middleware/rateLimit.ts @@ -32,6 +32,32 @@ function getMagicLinkIdentityKey(req: Request) { return `ip:${req.ip ?? req.socket.remoteAddress ?? 'unknown'}`; } +function getOtpIdentityKey(req: Request) { + const authReq = req as AuthenticatedRequest; + const body = req.body as { email?: unknown; phone?: unknown } | undefined; + const email = + authReq.user?.email ?? (typeof body?.email === 'string' ? body.email : undefined); + const phone = + authReq.user?.phone ?? (typeof body?.phone === 'string' ? body.phone : undefined); + + if (email) { + return `email:${email.toLowerCase()}`; + } + + if (phone) { + return `phone:${phone}`; + } + + return `ip:${req.ip ?? req.socket.remoteAddress ?? 'unknown'}`; +} + +function getOAuthFlowKey(req: Request) { + return [ + req.params?.providerId ?? 'unknown-provider', + req.ip ?? req.socket.remoteAddress ?? 'unknown', + ].join(':'); +} + const dynamicLimiter = rateLimit({ windowMs: 1 * 60 * 1000, limit: getConfiguredRateLimit, @@ -55,6 +81,36 @@ const magicLinkIdentityCachedLimiter = rateLimit({ legacyHeaders: false, }); +const otpIpCachedLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + limit: 10, + standardHeaders: true, + legacyHeaders: false, +}); + +const otpIdentityCachedLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + limit: 5, + keyGenerator: getOtpIdentityKey, + standardHeaders: true, + legacyHeaders: false, +}); + +const oauthIpCachedLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + limit: 30, + standardHeaders: true, + legacyHeaders: false, +}); + +const oauthProviderCachedLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + limit: 10, + keyGenerator: getOAuthFlowKey, + standardHeaders: true, + legacyHeaders: false, +}); + export function dynamicRateLimit(req: Request, res: Response, next: NextFunction) { return dynamicLimiter(req, res, next); } @@ -66,3 +122,19 @@ export function magicLinkIpLimiter(req: Request, res: Response, next: NextFuncti export function magicLinkEmailLimiter(req: Request, res: Response, next: NextFunction) { return magicLinkIdentityCachedLimiter(req, res, next); } + +export function otpIpLimiter(req: Request, res: Response, next: NextFunction) { + return otpIpCachedLimiter(req, res, next); +} + +export function otpIdentityLimiter(req: Request, res: Response, next: NextFunction) { + return otpIdentityCachedLimiter(req, res, next); +} + +export function oauthIpLimiter(req: Request, res: Response, next: NextFunction) { + return oauthIpCachedLimiter(req, res, next); +} + +export function oauthProviderLimiter(req: Request, res: Response, next: NextFunction) { + return oauthProviderCachedLimiter(req, res, next); +} diff --git a/src/routes/magicLink.routes.ts b/src/routes/magicLink.routes.ts index bd46a7a..c068e32 100644 --- a/src/routes/magicLink.routes.ts +++ b/src/routes/magicLink.routes.ts @@ -29,6 +29,7 @@ magicLinkRouter.get( response: { 200: MessageSchema, 403: ErrorSchema, + 500: InternalErrorSchema, }, }, }, diff --git a/src/routes/oauth.routes.ts b/src/routes/oauth.routes.ts index 5f0bf0c..a10bb8f 100644 --- a/src/routes/oauth.routes.ts +++ b/src/routes/oauth.routes.ts @@ -6,6 +6,7 @@ import { finishOAuthLogin, listOAuthProviders, startOAuthLogin } from '../controllers/oauth.js'; import { createRouter } from '../lib/createRouter.js'; +import { oauthIpLimiter, oauthProviderLimiter } from '../middleware/rateLimit.js'; import { InternalErrorSchema } from '../schemas/generic.responses.js'; import { FinishOAuthLoginRequestSchema, @@ -39,6 +40,7 @@ oauthRouter.post( { summary: 'Start OAuth login', tags: ['OAuth'], + middleware: [oauthIpLimiter, oauthProviderLimiter], schemas: { params: OAuthProviderParamSchema, body: StartOAuthLoginRequestSchema, @@ -57,6 +59,7 @@ oauthRouter.post( { summary: 'Finish OAuth login', tags: ['OAuth'], + middleware: [oauthIpLimiter, oauthProviderLimiter], schemas: { params: OAuthProviderParamSchema, body: FinishOAuthLoginRequestSchema, diff --git a/src/routes/otp.routes.ts b/src/routes/otp.routes.ts index 60f85ac..b380e3d 100644 --- a/src/routes/otp.routes.ts +++ b/src/routes/otp.routes.ts @@ -15,6 +15,7 @@ import { verifyPhoneNumber, } from '../controllers/otp.js'; import { createRouter } from '../lib/createRouter.js'; +import { otpIdentityLimiter, otpIpLimiter } from '../middleware/rateLimit.js'; import { ErrorSchema, InternalErrorSchema, MessageSchema } from '../schemas/generic.responses.js'; import { VerifyOTPRequestSchema } from '../schemas/otp.requests.js'; import { OTPVerifyTokenSuccessSchema } from '../schemas/otp.responses.js'; @@ -27,6 +28,7 @@ otpRouter.get( auth: 'ephemeral', summary: 'Generate email OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { response: { @@ -45,6 +47,7 @@ otpRouter.get( auth: 'ephemeral', summary: 'Generate phone OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { response: { @@ -63,6 +66,7 @@ otpRouter.get( auth: 'ephemeral', summary: 'Generate login email OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { response: { @@ -80,6 +84,7 @@ otpRouter.get( auth: 'ephemeral', summary: 'Generate login phone OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { response: { diff --git a/src/routes/registration.routes.ts b/src/routes/registration.routes.ts index f523ff3..3964e79 100644 --- a/src/routes/registration.routes.ts +++ b/src/routes/registration.routes.ts @@ -6,6 +6,7 @@ import { register, registerPhone, verifyRegisteredPhone } from '../controllers/registration.js'; import { createRouter } from '../lib/createRouter.js'; +import { otpIdentityLimiter, otpIpLimiter } from '../middleware/rateLimit.js'; import { ErrorSchema } from '../schemas/generic.responses.js'; import { VerifyOTPRequestSchema } from '../schemas/otp.requests.js'; import { OTPVerifyTokenSuccessSchema } from '../schemas/otp.responses.js'; @@ -46,6 +47,7 @@ registrationRouter.post( auth: 'access', summary: 'Register a phone number for the authenticated user', tags: ['Registration'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { body: RegisterPhoneRequestSchema, diff --git a/src/schemas/authEvent.types.ts b/src/schemas/authEvent.types.ts index 4af226f..387e8e2 100644 --- a/src/schemas/authEvent.types.ts +++ b/src/schemas/authEvent.types.ts @@ -30,6 +30,7 @@ export const AUTH_EVENT_TYPES = [ 'logout_success', 'logout_suspicious', 'magic_link_poll_completed_successfully', + 'magic_link_failed', 'magic_link_requested', 'magic_link_success', 'mfa_otp_failed', diff --git a/src/schemas/systemConfig.schema.ts b/src/schemas/systemConfig.schema.ts index b550032..a1baaa7 100644 --- a/src/schemas/systemConfig.schema.ts +++ b/src/schemas/systemConfig.schema.ts @@ -35,6 +35,7 @@ export const OAuthProviderConfigSchema = z.object({ allowSignup: z.boolean().default(true), accountLinking: z.enum(['email', 'disabled']).default('email'), requireEmailVerified: z.boolean().default(false), + pkce: z.boolean().optional(), }); export const LockoutPolicySchema = z.object({ diff --git a/src/services/messagingService.ts b/src/services/messagingService.ts index 2bda6fb..07070bb 100644 --- a/src/services/messagingService.ts +++ b/src/services/messagingService.ts @@ -42,6 +42,7 @@ export const sendOTPEmail = async (to: string, token: string) => { logger.info('Verification email sent'); } catch (error) { logger.error(`Failed to send verification email ${error}`); + throw error; } }; @@ -67,6 +68,7 @@ export const sendOTPSMS = async (to: string, token: number) => { }); } catch (error) { logger.error(`Failed to send verification SMS ${error}`); + throw error; } }; @@ -88,6 +90,7 @@ export const sendMagicLinkEmail = async (to: string, token: string, safeRedirect }); } catch (error) { logger.error(`Failed to send magic link email ${error}`); + throw error; } }; @@ -108,5 +111,6 @@ export const sendBootstrapEmail = async (to: string, url: string) => { }); } catch (error) { logger.error(`Failed to send bootstrap invite email ${error}`); + throw error; } }; diff --git a/src/services/oauthService.ts b/src/services/oauthService.ts index 76079a2..295490d 100644 --- a/src/services/oauthService.ts +++ b/src/services/oauthService.ts @@ -4,7 +4,7 @@ * See LICENSE file in the project root for full license information */ -import { createHmac, randomBytes, timingSafeEqual } from 'crypto'; +import { createHash, createHmac, randomBytes, timingSafeEqual } from 'crypto'; import { getSystemConfig } from '../config/getSystemConfig.js'; import { OAuthIdentity } from '../models/oauthIdentities.js'; @@ -12,6 +12,7 @@ import { User } from '../models/users.js'; import type { OAuthProviderConfig } from '../schemas/systemConfig.schema.js'; const STATE_TTL_MS = 10 * 60 * 1000; +const consumedStateHashes = new Map(); export type PublicOAuthProvider = { id: string; @@ -61,6 +62,26 @@ function signPayload(payload: string) { return createHmac('sha256', stateSecret()).update(payload).digest('base64url'); } +function hashStateForReplayCache(state: string) { + return createHmac('sha256', stateSecret()).update(`oauth-state:${state}`).digest('base64url'); +} + +function purgeConsumedStates(now = Date.now()) { + for (const [stateHash, expiresAt] of consumedStateHashes.entries()) { + if (expiresAt <= now) { + consumedStateHashes.delete(stateHash); + } + } +} + +function pkceEnabled(provider: OAuthProviderConfig) { + return provider.pkce !== false; +} + +function sha256Base64Url(value: string) { + return createHash('sha256').update(value).digest('base64url'); +} + function safeEqual(a: string, b: string) { const left = Buffer.from(a); const right = Buffer.from(b); @@ -199,16 +220,69 @@ export function verifyOAuthState(state: string, providerId: string): OAuthStateP return payload; } +export function consumeOAuthState(state: string, providerId: string): OAuthStatePayload | null { + const payload = verifyOAuthState(state, providerId); + + if (!payload) return null; + + purgeConsumedStates(); + + const stateHash = hashStateForReplayCache(state); + + if (consumedStateHashes.has(stateHash)) { + return null; + } + + consumedStateHashes.set(stateHash, payload.createdAt + STATE_TTL_MS); + + return payload; +} + +export function clearOAuthStateReplayCache() { + consumedStateHashes.clear(); +} + +export function createOAuthPkceCodeVerifier( + provider: OAuthProviderConfig, + payload: OAuthStatePayload, +) { + if (!pkceEnabled(provider)) return undefined; + + return createHmac('sha256', stateSecret()) + .update( + JSON.stringify([ + 'oauth-pkce-v1', + provider.id, + payload.providerId, + payload.redirectUri, + payload.nonce, + payload.createdAt, + ]), + ) + .digest('base64url'); +} + +export function createOAuthPkceCodeChallenge( + provider: OAuthProviderConfig, + payload: OAuthStatePayload, +) { + const verifier = createOAuthPkceCodeVerifier(provider, payload); + + return verifier ? sha256Base64Url(verifier) : undefined; +} + export function buildOAuthAuthorizationUrl({ provider, redirectUri, state, nonce, + codeChallenge, }: { provider: OAuthProviderConfig; redirectUri: string; state: string; nonce?: string; + codeChallenge?: string; }) { const url = new URL(provider.authorizationUrl); @@ -225,6 +299,11 @@ export function buildOAuthAuthorizationUrl({ url.searchParams.set('nonce', nonce); } + if (codeChallenge) { + url.searchParams.set('code_challenge', codeChallenge); + url.searchParams.set('code_challenge_method', 'S256'); + } + return url.toString(); } @@ -232,10 +311,12 @@ export async function exchangeOAuthCode({ provider, code, redirectUri, + codeVerifier, }: { provider: OAuthProviderConfig; code: string; redirectUri: string; + codeVerifier?: string; }) { const clientSecret = process.env[provider.clientSecretEnv]; @@ -251,6 +332,10 @@ export async function exchangeOAuthCode({ client_secret: clientSecret, }); + if (codeVerifier) { + body.set('code_verifier', codeVerifier); + } + const response = await globalThis.fetch(provider.tokenUrl, { method: 'POST', headers: { diff --git a/src/utils/otp.ts b/src/utils/otp.ts index 3124d99..8c12687 100644 --- a/src/utils/otp.ts +++ b/src/utils/otp.ts @@ -4,27 +4,69 @@ * See LICENSE file in the project root for full license information */ +import { randomInt, timingSafeEqual } from 'crypto'; + import { User } from '../models/users.js'; import { sendOTPEmail, sendOTPSMS } from '../services/messagingService.js'; import getLogger from './logger.js'; +import { hashSha256 } from './utils.js'; const logger = getLogger('utils.otp'); +const EMAIL_OTP_ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; +const OTP_HASH_PREFIX = 'sha256:'; export interface GenerateOtpOptions { sendMessage?: boolean; } +function safeStringEqual(left: string, right: string) { + const leftBuffer = Buffer.from(left); + const rightBuffer = Buffer.from(right); + + if (leftBuffer.length !== rightBuffer.length) { + return false; + } + + return timingSafeEqual(leftBuffer, rightBuffer); +} + +function normalizeEmailOtp(token: string) { + return token.trim().toUpperCase(); +} + +function normalizePhoneOtp(token: string) { + return token.trim(); +} + +export function hashOtpToken(token: string) { + return `${OTP_HASH_PREFIX}${hashSha256(token)}`; +} + +function otpMatchesStoredValue( + storedToken: string, + verificationToken: string, + normalize: (token: string) => string, +) { + const normalizedVerificationToken = normalize(verificationToken); + + if (storedToken.startsWith(OTP_HASH_PREFIX)) { + return safeStringEqual(storedToken, hashOtpToken(normalizedVerificationToken)); + } + + // Transitional compatibility for OTPs issued before hashed storage was introduced. + return safeStringEqual(normalize(storedToken), normalizedVerificationToken); +} + export const generateRandomEmailOTP = (): string => { - const characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; let result = ''; for (let i = 0; i < 6; i++) { - result += characters.charAt(Math.floor(Math.random() * characters.length)); + result += EMAIL_OTP_ALPHABET.charAt(randomInt(EMAIL_OTP_ALPHABET.length)); } return result; }; export const generateRandomPhoneOTP = (): number => { - return Math.floor(Math.random() * 900000) + 100000; + return randomInt(100000, 1000000); }; export const generateEmailOTP = async ( @@ -44,7 +86,7 @@ export const generateEmailOTP = async ( const emailVerificationTokenExpiry = now.getTime(); await user.update({ - emailVerificationToken: emailToken, + emailVerificationToken: hashOtpToken(normalizeEmailOtp(emailToken)), emailVerificationTokenExpiry, }); @@ -80,7 +122,7 @@ export const generatePhoneOTP = async ( const phoneVerificationTokenExpiry = now.getTime(); await user.update({ - phoneVerificationToken: String(phoneToken), + phoneVerificationToken: hashOtpToken(String(phoneToken)), phoneVerificationTokenExpiry, }); @@ -104,7 +146,7 @@ export const verifyPhoneOTP = async ( } if ( - user.phoneVerificationToken === verificationToken && + otpMatchesStoredValue(user.phoneVerificationToken, verificationToken, normalizePhoneOtp) && user.phoneVerificationTokenExpiry > new Date().getTime() ) { user.phoneVerified = true; @@ -137,7 +179,7 @@ export const verifyEmailOTP = async ( } if ( - user.emailVerificationToken.toUpperCase() === verificationToken.toUpperCase() && + otpMatchesStoredValue(user.emailVerificationToken, verificationToken, normalizeEmailOtp) && user.emailVerificationTokenExpiry > new Date().getTime() ) { user.emailVerified = true; diff --git a/tests/integration/magicLink/magicLink.spec.ts b/tests/integration/magicLink/magicLink.spec.ts index 85c4359..84b4331 100644 --- a/tests/integration/magicLink/magicLink.spec.ts +++ b/tests/integration/magicLink/magicLink.spec.ts @@ -9,6 +9,8 @@ import { Session } from '../../../src/models/sessions.js'; import { createApp } from '../../../src/app.js'; import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; import { generateRefreshToken, hashRefreshToken, signAccessToken } from '../../../src/lib/token.js'; +import { AuthEventService } from '../../../src/services/authEventService.js'; +import { sendMagicLinkEmail } from '../../../src/services/messagingService.js'; let app: Application; @@ -73,6 +75,24 @@ describe('GET /magic-link', () => { expect(MagicLinkToken.create).toHaveBeenCalled(); }); + it('returns an error when direct magic-link delivery fails', async () => { + (MagicLinkToken.update as any).mockResolvedValue([1]); + (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); + (sendMagicLinkEmail as any).mockRejectedValue(new Error('delivery failed')); + + const res = await request(app).get('/magic-link'); + + expect(res.status).toBe(500); + expect(res.body.error).toBe('Failed to deliver magic link'); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + userId: 'user-1', + type: 'magic_link_failed', + metadata: { reason: 'Delivery failed' }, + }), + ); + }); + it('rejects magic link requests when the method is disabled', async () => { (getSystemConfig as any).mockResolvedValue({ origins: ['http://localhost:5174'], @@ -101,6 +121,12 @@ describe('GET /magic-link/verify/:token', () => { const res = await request(app).get('/magic-link/verify/bad'); expect(res.status).toBe(400); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'magic_link_failed', + metadata: { reason: 'Invalid verification token' }, + }), + ); }); it('rejects used token', async () => { @@ -109,6 +135,13 @@ describe('GET /magic-link/verify/:token', () => { const res = await request(app).get('/magic-link/verify/token'); expect(res.status).toBe(400); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + userId: 'user-1', + type: 'magic_link_failed', + metadata: { reason: 'Token already used' }, + }), + ); }); it('rejects expired token', async () => { diff --git a/tests/integration/oauth/oauth.spec.ts b/tests/integration/oauth/oauth.spec.ts index 4563199..2f54914 100644 --- a/tests/integration/oauth/oauth.spec.ts +++ b/tests/integration/oauth/oauth.spec.ts @@ -13,6 +13,7 @@ import { import { OAuthIdentity } from '../../../src/models/oauthIdentities.js'; import { Session } from '../../../src/models/sessions.js'; import { User } from '../../../src/models/users.js'; +import { clearOAuthStateReplayCache } from '../../../src/services/oauthService.js'; import { buildSystemConfig } from '../../factories/systemConfigFactory.js'; import { buildUser } from '../../factories/userFactory.js'; @@ -45,6 +46,7 @@ beforeAll(async () => { beforeEach(() => { vi.clearAllMocks(); + clearOAuthStateReplayCache(); vi.stubEnv('GOOGLE_CLIENT_SECRET', 'secret'); (getSystemConfig as any).mockResolvedValue( buildSystemConfig({ @@ -81,6 +83,8 @@ describe('OAuth routes', () => { expect(res.body.authorizationUrl).toContain('client_id=client-id'); expect(res.body.authorizationUrl).toContain('state='); expect(res.body.authorizationUrl).toContain('nonce='); + expect(res.body.authorizationUrl).toContain('code_challenge='); + expect(res.body.authorizationUrl).toContain('code_challenge_method=S256'); }); it('rejects redirect URI prefix lookalikes', async () => { @@ -142,6 +146,9 @@ describe('OAuth routes', () => { method: 'POST', }), ); + expect((fetchMock.mock.calls[0][1]?.body as URLSearchParams).get('code_verifier')).toEqual( + expect.any(String), + ); expect(fetchMock).toHaveBeenNthCalledWith( 2, 'https://openidconnect.googleapis.com/v1/userinfo', @@ -158,5 +165,14 @@ describe('OAuth routes', () => { sub: 'user-1', }), ); + + const replay = await request(app).post('/oauth/google/callback').send({ + code: 'oauth-code-replay', + state: start.body.state, + }); + + expect(replay.status).toBe(400); + expect(replay.body.error).toBe('Invalid OAuth state'); + expect(fetchMock).toHaveBeenCalledTimes(2); }); }); diff --git a/tests/integration/otp/otp.security.spec.ts b/tests/integration/otp/otp.security.spec.ts index 359e8ec..17e7387 100644 --- a/tests/integration/otp/otp.security.spec.ts +++ b/tests/integration/otp/otp.security.spec.ts @@ -112,8 +112,7 @@ describe('OTP Security - Email Verification', () => { .set('Authorization', 'Bearer token') .send({ verificationToken: 'bad' }); - // ⚠️ matches your current controller behavior - expect(res.status).toBe(500); + expect(res.status).toBe(401); }); it('rejects expired OTP', async () => { @@ -138,6 +137,6 @@ describe('OTP Security - Email Verification', () => { .set('Authorization', 'Bearer token') .send({ verificationToken: '123456' }); - expect(res.status).toBe(500); + expect(res.status).toBe(401); }); }); diff --git a/tests/integration/otp/otp.spec.ts b/tests/integration/otp/otp.spec.ts index 48a1a60..ba85291 100644 --- a/tests/integration/otp/otp.spec.ts +++ b/tests/integration/otp/otp.spec.ts @@ -109,7 +109,7 @@ describe('OTP - Verify Email', () => { const res = await request(app).post('/otp/verify-email-otp').send({ verificationToken: 'bad' }); - expect(res.status).toBe(500); // matches your controller behavior + expect(res.status).toBe(401); }); it('succeeds when OTP valid', async () => { diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index de70e24..18038b3 100644 --- a/tests/setup/mocks.ts +++ b/tests/setup/mocks.ts @@ -162,6 +162,10 @@ vi.mock('../../src/middleware/requireAdmin.js', () => ({ vi.mock('../../src/middleware/rateLimit.js', () => ({ magicLinkIpLimiter: (_req: any, _res: any, next: any) => next(), magicLinkEmailLimiter: (_req: any, _res: any, next: any) => next(), + oauthIpLimiter: (_req: any, _res: any, next: any) => next(), + oauthProviderLimiter: (_req: any, _res: any, next: any) => next(), + otpIpLimiter: (_req: any, _res: any, next: any) => next(), + otpIdentityLimiter: (_req: any, _res: any, next: any) => next(), dynamicRateLimit: (_req: any, _res: any, next: any) => next(), dynamicSlowDown: (_req: any, _res: any, next: any) => next(), })); diff --git a/tests/unit/controllers/otp.spec.ts b/tests/unit/controllers/otp.spec.ts index bb52bc7..c4e0445 100644 --- a/tests/unit/controllers/otp.spec.ts +++ b/tests/unit/controllers/otp.spec.ts @@ -373,7 +373,7 @@ describe('otp controller', () => { }); }); - it('returns 500 when login email verification fails after lookup succeeds', async () => { + it('returns 401 when login email verification fails after lookup succeeds', async () => { const { verifyLoginEmail } = await loadOtpController(); const failingUser = buildUser(); const req = buildReq(buildUser(), { @@ -394,7 +394,7 @@ describe('otp controller', () => { type: 'verify_otp_failed', }), ); - expect(res.status).toHaveBeenCalledWith(500); - expect(res.json).toHaveBeenCalledWith({ error: 'Internal server error' }); + expect(res.status).toHaveBeenCalledWith(401); + expect(res.json).toHaveBeenCalledWith({ error: 'Not allowed' }); }); }); diff --git a/tests/unit/middleware/rateLimit.spec.ts b/tests/unit/middleware/rateLimit.spec.ts index 11e3b3c..ab7480b 100644 --- a/tests/unit/middleware/rateLimit.spec.ts +++ b/tests/unit/middleware/rateLimit.spec.ts @@ -140,7 +140,7 @@ describe('dynamicRateLimit', () => { await dynamicRateLimit(req, res, next); await dynamicRateLimit(req, res, next); - expect(rateLimit.default).toHaveBeenCalledTimes(3); + expect(rateLimit.default).toHaveBeenCalledTimes(7); }); }); @@ -204,6 +204,77 @@ describe('magicLinkEmailLimiter', () => { }); }); +describe('otpIdentityLimiter', () => { + it('uses authenticated email or phone as key', async () => { + const { getSystemConfig } = await import('../../../src/config/getSystemConfig'); + const rateLimit = await import('express-rate-limit'); + + (getSystemConfig as any).mockResolvedValue({}); + + const { otpIdentityLimiter } = await import('../../../src/middleware/rateLimit'); + + const req: any = { + user: { email: null, phone: '+14155552671' }, + ip: '127.0.0.1', + }; + const next = vi.fn(); + + // @ts-ignore + await otpIdentityLimiter(req, {}, next); + + const options = (rateLimit.default as any).mock.calls + .map(([options]: any[]) => options) + .find((options: any) => options.keyGenerator?.(req) === 'phone:+14155552671'); + + expect(options).toEqual( + expect.objectContaining({ + keyGenerator: expect.any(Function), + legacyHeaders: false, + limit: 5, + standardHeaders: true, + windowMs: 15 * 60 * 1000, + }), + ); + expect(options.keyGenerator({ user: { email: 'Test@Example.com' } })).toBe( + 'email:test@example.com', + ); + }); +}); + +describe('oauthProviderLimiter', () => { + it('keys by provider and ip', async () => { + const { getSystemConfig } = await import('../../../src/config/getSystemConfig'); + const rateLimit = await import('express-rate-limit'); + + (getSystemConfig as any).mockResolvedValue({}); + + const { oauthProviderLimiter } = await import('../../../src/middleware/rateLimit'); + + const req: any = { + params: { providerId: 'google' }, + ip: '127.0.0.1', + }; + const next = vi.fn(); + + // @ts-ignore + await oauthProviderLimiter(req, {}, next); + + const options = (rateLimit.default as any).mock.calls + .map(([options]: any[]) => options) + .find((options: any) => options.keyGenerator?.(req) === 'google:127.0.0.1'); + + expect(options).toEqual( + expect.objectContaining({ + keyGenerator: expect.any(Function), + legacyHeaders: false, + limit: 10, + standardHeaders: true, + windowMs: 15 * 60 * 1000, + }), + ); + }); +}); + describe('dynamicJWKSRateLimit', () => { it('uses config rate_limit and invokes the cached limiter', async () => { const { getSystemConfig } = await import('../../../src/config/getSystemConfig'); @@ -250,7 +321,7 @@ describe('rate limiter caches', () => { // @ts-ignore await magicLinkEmailLimiter({}, {}, next); - expect(rateLimit.default).toHaveBeenCalledTimes(3); + expect(rateLimit.default).toHaveBeenCalledTimes(7); expect((rateLimit.default as any).mock.calls[0][0]).toEqual( expect.objectContaining({ limit: expect.any(Function), @@ -260,6 +331,10 @@ describe('rate limiter caches', () => { expect.any(Function), 20, 5, + 10, + 5, + 30, + 10, ]); }); }); diff --git a/tests/unit/services/messagingService.spec.ts b/tests/unit/services/messagingService.spec.ts index af128fc..59a7107 100644 --- a/tests/unit/services/messagingService.spec.ts +++ b/tests/unit/services/messagingService.spec.ts @@ -89,4 +89,33 @@ describe('messagingService', () => { await expect(sendOTPEmail('test@example.com', '123')).resolves.toBeUndefined(); expect(createDirectAuthMessagingServiceMock).toHaveBeenCalledWith('Seamless Auth Test'); }); + + it('propagates production email delivery failures', async () => { + process.env.NODE_ENV = 'production'; + sendOtpEmailMock.mockRejectedValueOnce(new Error('provider down')); + + const { sendOTPEmail } = await import('../../../src/services/messagingService'); + + await expect(sendOTPEmail('test@example.com', '123')).rejects.toThrow('provider down'); + }); + + it('propagates production SMS delivery failures', async () => { + process.env.NODE_ENV = 'production'; + sendOtpSmsMock.mockRejectedValueOnce(new Error('sms down')); + + const { sendOTPSMS } = await import('../../../src/services/messagingService'); + + await expect(sendOTPSMS('+14155552671', 123456)).rejects.toThrow('sms down'); + }); + + it('propagates production magic-link delivery failures', async () => { + process.env.NODE_ENV = 'production'; + sendMagicLinkEmailMock.mockRejectedValueOnce(new Error('email down')); + + const { sendMagicLinkEmail } = await import('../../../src/services/messagingService'); + + await expect( + sendMagicLinkEmail('test@example.com', 'token', 'https://app.example.com/verify'), + ).rejects.toThrow('email down'); + }); }); diff --git a/tests/unit/services/oauthService.spec.ts b/tests/unit/services/oauthService.spec.ts index 2a77974..d393594 100644 --- a/tests/unit/services/oauthService.spec.ts +++ b/tests/unit/services/oauthService.spec.ts @@ -5,6 +5,10 @@ import { OAuthIdentity } from '../../../src/models/oauthIdentities.js'; import { User } from '../../../src/models/users.js'; import { buildOAuthAuthorizationUrl, + clearOAuthStateReplayCache, + consumeOAuthState, + createOAuthPkceCodeChallenge, + createOAuthPkceCodeVerifier, createOAuthState, exchangeOAuthCode, fetchOAuthProfile, @@ -39,6 +43,7 @@ const provider = { describe('oauthService', () => { beforeEach(() => { vi.clearAllMocks(); + clearOAuthStateReplayCache(); vi.stubEnv('GOOGLE_CLIENT_SECRET', 'secret'); (getSystemConfig as any).mockResolvedValue( buildSystemConfig({ @@ -69,6 +74,20 @@ describe('oauthService', () => { expect(verifyOAuthState(state, 'github')).toBeNull(); }); + it('consumes OAuth state only once per process', () => { + const state = createOAuthState({ + providerId: 'google', + redirectUri: 'https://app.example.com/oauth/callback', + }); + + expect(consumeOAuthState(state, 'google')).toEqual( + expect.objectContaining({ + providerId: 'google', + }), + ); + expect(consumeOAuthState(state, 'google')).toBeNull(); + }); + it('builds provider authorization URLs', () => { const state = 'state'; const url = buildOAuthAuthorizationUrl({ @@ -95,6 +114,30 @@ describe('oauthService', () => { expect(url).toContain('nonce=nonce-value'); }); + it('adds PKCE challenge parameters when supplied', () => { + const state = createOAuthState({ + providerId: 'google', + redirectUri: 'https://app.example.com/oauth/callback', + }); + const payload = verifyOAuthState(state, 'google'); + + expect(payload).not.toBeNull(); + + const codeVerifier = createOAuthPkceCodeVerifier(provider, payload!); + const codeChallenge = createOAuthPkceCodeChallenge(provider, payload!); + const url = buildOAuthAuthorizationUrl({ + provider, + redirectUri: 'https://app.example.com/oauth/callback', + state, + codeChallenge, + }); + + expect(codeVerifier).toEqual(expect.any(String)); + expect(codeChallenge).toEqual(expect.any(String)); + expect(url).toContain('code_challenge='); + expect(url).toContain('code_challenge_method=S256'); + }); + it('rejects redirect URIs outside configured origins', async () => { await expect( resolveOAuthRedirectUri(provider, 'https://evil.example.com/oauth/callback'), @@ -130,10 +173,14 @@ describe('oauthService', () => { provider, code: 'code', redirectUri: 'https://app.example.com/oauth/callback', + codeVerifier: 'pkce-verifier', }); const profile = await fetchOAuthProfile(provider, token); expect(token).toBe('provider-token'); + expect((fetchMock.mock.calls[0][1]?.body as URLSearchParams).get('code_verifier')).toBe( + 'pkce-verifier', + ); expect(profile).toEqual({ subject: 'provider-user', email: 'person@example.com', diff --git a/tests/unit/utils/otp.spec.ts b/tests/unit/utils/otp.spec.ts index cc740fe..a94ee35 100644 --- a/tests/unit/utils/otp.spec.ts +++ b/tests/unit/utils/otp.spec.ts @@ -11,6 +11,7 @@ import { generateRandomPhoneOTP, generateEmailOTP, generatePhoneOTP, + hashOtpToken, verifyPhoneOTP, verifyEmailOTP, } from '../../../src/utils/otp.js'; @@ -68,9 +69,15 @@ describe('OTP utils', () => { it('updates user and sends email', async () => { const user = buildUser(); - await generateEmailOTP(user as any); + const token = await generateEmailOTP(user as any); expect(user.update).toHaveBeenCalled(); + expect(user.update).toHaveBeenCalledWith( + expect.objectContaining({ + emailVerificationToken: hashOtpToken(token), + emailVerificationTokenExpiry: expect.any(Number), + }), + ); expect(sendOTPEmail).toHaveBeenCalled(); }); @@ -91,9 +98,15 @@ describe('OTP utils', () => { it('updates user and sends sms', async () => { const user = buildUser(); - await generatePhoneOTP(user as any); + const token = await generatePhoneOTP(user as any); expect(user.update).toHaveBeenCalled(); + expect(user.update).toHaveBeenCalledWith( + expect.objectContaining({ + phoneVerificationToken: hashOtpToken(String(token)), + phoneVerificationTokenExpiry: expect.any(Number), + }), + ); expect(sendOTPSMS).toHaveBeenCalled(); }); @@ -108,7 +121,7 @@ describe('OTP utils', () => { describe('verifyPhoneOTP', () => { it('verifies valid OTP', async () => { const user = buildUser({ - phoneVerificationToken: '123456', + phoneVerificationToken: hashOtpToken('123456'), phoneVerificationTokenExpiry: Date.now() + 10000, emailVerified: true, }); @@ -123,7 +136,7 @@ describe('OTP utils', () => { it('returns false for invalid token', async () => { const user = buildUser({ - phoneVerificationToken: '123456', + phoneVerificationToken: hashOtpToken('123456'), phoneVerificationTokenExpiry: Date.now() + 10000, }); @@ -134,7 +147,7 @@ describe('OTP utils', () => { it('returns false for expired token', async () => { const user = buildUser({ - phoneVerificationToken: '123456', + phoneVerificationToken: hashOtpToken('123456'), phoneVerificationTokenExpiry: Date.now() - 1000, }); @@ -156,7 +169,7 @@ describe('OTP utils', () => { describe('verifyEmailOTP', () => { it('verifies valid OTP (case insensitive)', async () => { const user = buildUser({ - emailVerificationToken: 'ABCDEF', + emailVerificationToken: hashOtpToken('ABCDEF'), emailVerificationTokenExpiry: Date.now() + 10000, phone: null, }); @@ -171,7 +184,7 @@ describe('OTP utils', () => { it('returns false for invalid token', async () => { const user = buildUser({ - emailVerificationToken: 'ABCDEF', + emailVerificationToken: hashOtpToken('ABCDEF'), emailVerificationTokenExpiry: Date.now() + 10000, }); @@ -187,3 +200,26 @@ describe('OTP utils', () => { }); }); }); + it('accepts legacy plaintext phone OTP values during rollout', async () => { + const user = buildUser({ + phoneVerificationToken: '123456', + phoneVerificationTokenExpiry: Date.now() + 10000, + emailVerified: true, + }); + + const result = await verifyPhoneOTP(user as any, '123456'); + + expect(result.verified).toBe(true); + }); + + it('accepts legacy plaintext email OTP values during rollout', async () => { + const user = buildUser({ + emailVerificationToken: 'ABCDEF', + emailVerificationTokenExpiry: Date.now() + 10000, + phone: null, + }); + + const result = await verifyEmailOTP(user as any, 'abcdef'); + + expect(result.verified).toBe(true); + }); From 3971292a01eab04c92215f3f0087d0c14c5027b2 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 8 Jun 2026 21:05:05 -0400 Subject: [PATCH 2/9] ci: linting --- src/controllers/otp.ts | 2 -- src/middleware/rateLimit.ts | 6 ++---- tests/unit/utils/otp.spec.ts | 36 ++++++++++++++++++------------------ 3 files changed, 20 insertions(+), 24 deletions(-) diff --git a/src/controllers/otp.ts b/src/controllers/otp.ts index 4a29f3d..0811fa3 100644 --- a/src/controllers/otp.ts +++ b/src/controllers/otp.ts @@ -388,7 +388,6 @@ export const verifyEmail = async (req: Request, res: Response) => { }); return res.status(401).json({ error: 'Not allowed' }); } - }; export const verifyLoginPhoneNumber = async (req: Request, res: Response) => { @@ -588,5 +587,4 @@ export const verifyLoginEmail = async (req: Request, res: Response) => { }); return res.status(401).json({ error: 'Not allowed' }); } - }; diff --git a/src/middleware/rateLimit.ts b/src/middleware/rateLimit.ts index 67e05dd..81aed54 100644 --- a/src/middleware/rateLimit.ts +++ b/src/middleware/rateLimit.ts @@ -35,10 +35,8 @@ function getMagicLinkIdentityKey(req: Request) { function getOtpIdentityKey(req: Request) { const authReq = req as AuthenticatedRequest; const body = req.body as { email?: unknown; phone?: unknown } | undefined; - const email = - authReq.user?.email ?? (typeof body?.email === 'string' ? body.email : undefined); - const phone = - authReq.user?.phone ?? (typeof body?.phone === 'string' ? body.phone : undefined); + const email = authReq.user?.email ?? (typeof body?.email === 'string' ? body.email : undefined); + const phone = authReq.user?.phone ?? (typeof body?.phone === 'string' ? body.phone : undefined); if (email) { return `email:${email.toLowerCase()}`; diff --git a/tests/unit/utils/otp.spec.ts b/tests/unit/utils/otp.spec.ts index a94ee35..3744f98 100644 --- a/tests/unit/utils/otp.spec.ts +++ b/tests/unit/utils/otp.spec.ts @@ -200,26 +200,26 @@ describe('OTP utils', () => { }); }); }); - it('accepts legacy plaintext phone OTP values during rollout', async () => { - const user = buildUser({ - phoneVerificationToken: '123456', - phoneVerificationTokenExpiry: Date.now() + 10000, - emailVerified: true, - }); +it('accepts legacy plaintext phone OTP values during rollout', async () => { + const user = buildUser({ + phoneVerificationToken: '123456', + phoneVerificationTokenExpiry: Date.now() + 10000, + emailVerified: true, + }); - const result = await verifyPhoneOTP(user as any, '123456'); + const result = await verifyPhoneOTP(user as any, '123456'); - expect(result.verified).toBe(true); - }); + expect(result.verified).toBe(true); +}); - it('accepts legacy plaintext email OTP values during rollout', async () => { - const user = buildUser({ - emailVerificationToken: 'ABCDEF', - emailVerificationTokenExpiry: Date.now() + 10000, - phone: null, - }); +it('accepts legacy plaintext email OTP values during rollout', async () => { + const user = buildUser({ + emailVerificationToken: 'ABCDEF', + emailVerificationTokenExpiry: Date.now() + 10000, + phone: null, + }); - const result = await verifyEmailOTP(user as any, 'abcdef'); + const result = await verifyEmailOTP(user as any, 'abcdef'); - expect(result.verified).toBe(true); - }); + expect(result.verified).toBe(true); +}); From 150ad7db03e5e8a960f037f8a16e2d47d2df902c Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 8 Jun 2026 21:14:11 -0400 Subject: [PATCH 3/9] fix: allow admins to create users without phone numbers Also folds in follow-up work that previously landed in an unnamed WIP commit: - magic-link polling returns 204/403 instead of 500 on missing/mismatched token - unify OTP verify success response with the refresh response schema - add e2e/integration/unit tests (defineRoute auth, response-schema coverage) - coverage badge refresh + run coverage with --fileParallelism=false --- package.json | 2 +- resources/coverage-badge.svg | 14 ++-- src/controllers/admin.ts | 59 ++++++++++++- src/controllers/magicLinks.ts | 7 +- src/schemas/admin.requests.ts | 4 +- src/schemas/otp.responses.ts | 6 +- tests/e2e/authFlow.spec.ts | 25 +++++- tests/integration/admin/admin.spec.ts | 80 ++++++++++++++++++ tests/integration/magicLink/magicLink.spec.ts | 69 ++++++++++++++- tests/integration/oauth/oauth.spec.ts | 44 ++++++++++ tests/integration/otp/otp.spec.ts | 18 ++++ .../lib/defineRoute.authIntegration.spec.ts | 84 +++++++++++++++++++ .../routes/responseSchemaCoverage.spec.ts | 16 ++++ 13 files changed, 401 insertions(+), 27 deletions(-) create mode 100644 tests/unit/lib/defineRoute.authIntegration.spec.ts diff --git a/package.json b/package.json index 88e7d83..8d2ddde 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,7 @@ "typecheck": "tsc --noEmit", "test": "vitest", "test:run": "vitest run", - "coverage": "vitest run --coverage", + "coverage": "vitest run --coverage --fileParallelism=false", "coverage:badge": "node ./src/scripts/updateCoverageBadge.mjs", "test:ci": "CI=true vitest", "test:e2e": "CI=false vitest", diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index e82144e..cfaca7c 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 82% + + coverage: 82.1% @@ -7,17 +7,17 @@ - + - - + + coverage coverage - 82% - 82% + 82.1% + 82.1% diff --git a/src/controllers/admin.ts b/src/controllers/admin.ts index 62f9931..4cee60f 100644 --- a/src/controllers/admin.ts +++ b/src/controllers/admin.ts @@ -30,6 +30,7 @@ import { hardRevokeSession } from '../services/sessionService.js'; import { ServiceRequest } from '../types/types.js'; import getLogger from '../utils/logger.js'; import { redactMetadata } from '../utils/redaction.js'; +import { isValidPhoneNumber, normalizePhoneNumber } from '../utils/utils.js'; const logger = getLogger('admin'); @@ -84,6 +85,14 @@ export const createUser = async (req: Request, res: Response) => { } const { email, phone, roles } = parsed.data; + const normalizedPhone = phone ? normalizePhoneNumber(phone) : null; + + if (phone && (!normalizedPhone || !isValidPhoneNumber(phone))) { + return res.status(400).json({ + error: 'Invalid payload', + details: { phone: 'Invalid phone number' }, + }); + } try { const existing = await User.findOne({ where: { email } }); @@ -94,7 +103,7 @@ export const createUser = async (req: Request, res: Response) => { const user = await User.create({ email, - phone: phone ?? null, + phone: normalizedPhone, roles: roles ?? [], }); @@ -165,16 +174,60 @@ export const updateUser = async (req: ServiceRequest, res: Response) => { } const before = user.toJSON(); + const updateData: Record = { ...parsed.data }; + const phoneSupplied = Object.prototype.hasOwnProperty.call(parsed.data, 'phone'); + const phoneVerifiedSupplied = Object.prototype.hasOwnProperty.call( + parsed.data, + 'phoneVerified', + ); + + if (phoneSupplied) { + const nextPhone = parsed.data.phone; + + if (nextPhone === null) { + updateData.phone = null; + updateData.phoneVerified = false; + updateData.phoneVerificationToken = null; + updateData.phoneVerificationTokenExpiry = null; + } else if (typeof nextPhone === 'string') { + const normalizedPhone = normalizePhoneNumber(nextPhone); + + if (!normalizedPhone || !isValidPhoneNumber(nextPhone)) { + return res.status(400).json({ + error: 'Invalid update payload', + details: { phone: 'Invalid phone number' }, + }); + } + + updateData.phone = normalizedPhone; + updateData.phoneVerificationToken = null; + updateData.phoneVerificationTokenExpiry = null; + + if (normalizedPhone !== user.phone && !phoneVerifiedSupplied) { + updateData.phoneVerified = false; + } + } + } + + if ( + updateData.phoneVerified === true && + (phoneSupplied ? updateData.phone : user.phone) === null + ) { + return res.status(400).json({ + error: 'Invalid update payload', + details: { phoneVerified: 'Cannot verify a missing phone number' }, + }); + } try { - await user.update(parsed.data); + await user.update(updateData); await AuthEventService.log({ type: 'internal_user_updated_by_owner', req, metadata: { before, - after: parsed.data, + after: updateData, targetUser: userId, }, }); diff --git a/src/controllers/magicLinks.ts b/src/controllers/magicLinks.ts index 1b8a9b6..07f1c21 100644 --- a/src/controllers/magicLinks.ts +++ b/src/controllers/magicLinks.ts @@ -238,8 +238,7 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { if (!record) { logger.warn('No magic link token'); - await logMagicLinkFailure(req, 'No active token found while polling', user.id); - return res.status(500).json({ error: 'Invalid request' }); + return res.status(204).json({ message: 'Success' }); } // Device binding check @@ -247,12 +246,12 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { if (record.ip_hash && record.ip_hash !== ip_hash) { await logMagicLinkFailure(req, 'Polling device IP mismatch', user.id); - return res.status(500).json({ error: 'Invalid request' }); + return res.status(403).json({ error: 'Invalid request' }); } if (record.user_agent_hash && record.user_agent_hash !== user_agent_hash) { await logMagicLinkFailure(req, 'Polling device user agent mismatch', user.id); - return res.status(500).json({ error: 'Invalid request' }); + return res.status(403).json({ error: 'Invalid request' }); } if (record.used_at && record.expires_at > new Date()) { diff --git a/src/schemas/admin.requests.ts b/src/schemas/admin.requests.ts index 0104ed3..efa99ff 100644 --- a/src/schemas/admin.requests.ts +++ b/src/schemas/admin.requests.ts @@ -10,14 +10,14 @@ import { RoleNameSchema } from './roles.schema.js'; export const CreateUserSchema = z.object({ email: z.email(), - phone: z.string().nullish(), + phone: z.string().min(5).nullish(), roles: z.array(RoleNameSchema).min(1), }); export const UpdateUserSchema = z .object({ email: z.email().optional(), - phone: z.string().min(5).optional(), + phone: z.string().min(5).nullable().optional(), emailVerified: z.boolean().optional(), phoneVerified: z.boolean().optional(), roles: z.array(RoleNameSchema).min(1).optional(), diff --git a/src/schemas/otp.responses.ts b/src/schemas/otp.responses.ts index 37f49a8..d474954 100644 --- a/src/schemas/otp.responses.ts +++ b/src/schemas/otp.responses.ts @@ -4,8 +4,8 @@ * See LICENSE file in the project root for full license information */ -import { z } from 'zod'; +import { RefreshSuccessResponseSchema } from './auth.responses.js'; -export const OTPVerifyTokenSuccessSchema = z.object({ - message: z.string(), +export const OTPVerifyTokenSuccessSchema = RefreshSuccessResponseSchema.omit({ + sessionId: true, }); diff --git a/tests/e2e/authFlow.spec.ts b/tests/e2e/authFlow.spec.ts index a7bd07e..139cdbe 100644 --- a/tests/e2e/authFlow.spec.ts +++ b/tests/e2e/authFlow.spec.ts @@ -31,6 +31,7 @@ import { generateEmailOTP, verifyEmailOTP } from '../../src/utils/otp.js'; import { findRefreshSessionByToken, + validateBearerToken, validateAccessToken, } from '../../src/services/sessionService.js'; @@ -80,13 +81,13 @@ describe('E2E Auth Flow', () => { expect(generateEmailOTP).toHaveBeenCalled(); (verifyEmailOTP as any).mockResolvedValue({ - user: { + user: buildUser({ id: 'user-1', emailVerified: true, phoneVerified: false, verified: true, roles: ['user'], - }, + }), verified: true, }); @@ -100,6 +101,17 @@ describe('E2E Auth Flow', () => { .send({ verificationToken: '123456' }); expect(verifyRes.status).toBe(200); + expect(verifyRes.body).toEqual( + expect.objectContaining({ + message: 'Success', + token: 'access-token', + refreshToken: 'refresh-token', + sub: 'user-1', + roles: ['user'], + ttl: 900, + refreshTtl: 3600, + }), + ); (validateAccessToken as any).mockResolvedValue({ sessionId: 'session-1', @@ -113,6 +125,7 @@ describe('E2E Auth Flow', () => { expect(accessRes.status).toBe(200); (validateAccessToken as any).mockResolvedValue(null); + (validateBearerToken as any).mockResolvedValue(null); (findRefreshSessionByToken as any).mockResolvedValue({ session: buildSession(), legacyFallbackCandidates: 0, @@ -121,14 +134,18 @@ describe('E2E Auth Flow', () => { (User.findByPk as any).mockResolvedValue({ id: 'user-1', + email: 'test@example.com', + phone: '+14155552671', + roles: ['user'], }); (Session.create as any).mockResolvedValue(buildSession({ id: 'session-2' })); const refreshRes = await request(app) - .get('/sessions') - .set('Authorization', 'Bearer access-token'); + .post('/refresh') + .set('Authorization', 'Bearer refresh-token'); expect(refreshRes.status).toBe(200); + expect(findRefreshSessionByToken).toHaveBeenCalledWith('refresh-token', expect.any(Date)); }); }); diff --git a/tests/integration/admin/admin.spec.ts b/tests/integration/admin/admin.spec.ts index a1bbe47..4acf65d 100644 --- a/tests/integration/admin/admin.spec.ts +++ b/tests/integration/admin/admin.spec.ts @@ -288,6 +288,59 @@ describe('POST /admin/users', () => { expect(res.body.user).not.toHaveProperty('emailVerificationToken'); }); + it('creates user without a phone number', async () => { + (User.findOne as any).mockResolvedValue(null); + + (User.create as any).mockResolvedValue({ + id: 'user-1', + email: 'test@example.com', + phone: null, + roles: ['user'], + }); + + const res = await request(app) + .post('/admin/users') + .send({ + email: 'test@example.com', + roles: ['user'], + }); + + expect(res.status).toBe(201); + expect(User.create).toHaveBeenCalledWith( + expect.objectContaining({ + email: 'test@example.com', + phone: null, + roles: ['user'], + }), + ); + }); + + it('normalizes optional phone numbers on create', async () => { + (User.findOne as any).mockResolvedValue(null); + + (User.create as any).mockResolvedValue({ + id: 'user-1', + email: 'test@example.com', + phone: '+14155552671', + roles: ['user'], + }); + + const res = await request(app) + .post('/admin/users') + .send({ + email: 'test@example.com', + phone: '+1 (415) 555-2671', + roles: ['user'], + }); + + expect(res.status).toBe(201); + expect(User.create).toHaveBeenCalledWith( + expect.objectContaining({ + phone: '+14155552671', + }), + ); + }); + it('creates user with scoped roles', async () => { (User.findOne as any).mockResolvedValue(null); @@ -376,6 +429,33 @@ describe('PATCH /admin/users/:userId', () => { expect(user.update).toHaveBeenCalledWith({ roles: ['admin:write'] }); }); + it('clears phone state when phone is removed', async () => { + const user = buildUser({ phone: '+14155552671', phoneVerified: true }); + + (User.findByPk as any).mockResolvedValue(user); + + const res = await request(app).patch('/admin/users/user-1').send({ phone: null }); + + expect(res.status).toBe(200); + expect(user.update).toHaveBeenCalledWith({ + phone: null, + phoneVerified: false, + phoneVerificationToken: null, + phoneVerificationTokenExpiry: null, + }); + }); + + it('rejects phoneVerified true without a phone number', async () => { + const user = buildUser({ phone: null, phoneVerified: false }); + + (User.findByPk as any).mockResolvedValue(user); + + const res = await request(app).patch('/admin/users/user-1').send({ phoneVerified: true }); + + expect(res.status).toBe(400); + expect(user.update).not.toHaveBeenCalled(); + }); + it('returns 404 if user not found', async () => { (User.findByPk as any).mockResolvedValue(null); diff --git a/tests/integration/magicLink/magicLink.spec.ts b/tests/integration/magicLink/magicLink.spec.ts index 84b4331..9f625d1 100644 --- a/tests/integration/magicLink/magicLink.spec.ts +++ b/tests/integration/magicLink/magicLink.spec.ts @@ -8,7 +8,12 @@ import { Session } from '../../../src/models/sessions.js'; import { createApp } from '../../../src/app.js'; import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; -import { generateRefreshToken, hashRefreshToken, signAccessToken } from '../../../src/lib/token.js'; +import { + createRefreshTokenLookup, + generateRefreshToken, + hashRefreshToken, + signAccessToken, +} from '../../../src/lib/token.js'; import { AuthEventService } from '../../../src/services/authEventService.js'; import { sendMagicLinkEmail } from '../../../src/services/messagingService.js'; @@ -75,6 +80,25 @@ describe('GET /magic-link', () => { expect(MagicLinkToken.create).toHaveBeenCalled(); }); + 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' }); + + const res = await request(app) + .get('/magic-link') + .set('x-seamless-auth-delivery-mode', 'external'); + + expect(res.status).toBe(200); + expect(sendMagicLinkEmail).not.toHaveBeenCalled(); + expect(res.body.delivery).toEqual({ + kind: 'magic_link_email', + to: 'test@example.com', + token: expect.any(String), + magicLinkUrl: expect.stringContaining('http://localhost:5174/verify-magiclink?token='), + }); + expect(res.body.delivery.magicLinkUrl).toContain(res.body.delivery.token); + }); + it('returns an error when direct magic-link delivery fails', async () => { (MagicLinkToken.update as any).mockResolvedValue([1]); (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); @@ -187,13 +211,19 @@ describe('GET /magic-link/check', () => { expect(res.status).toBe(400); }); - it('returns 500 when no token found', async () => { + it('returns 204 when no active token is found', async () => { (User.findOne as any).mockResolvedValue({ id: 'user-1', email: 'test@example.com' }); (MagicLinkToken.findOne as any).mockResolvedValue(null); const res = await request(app).get('/magic-link/check'); - expect(res.status).toBe(500); + expect(res.status).toBe(204); + expect(AuthEventService.log).not.toHaveBeenCalledWith( + expect.objectContaining({ + type: 'magic_link_failed', + metadata: { reason: 'No active token found while polling' }, + }), + ); }); it('returns 204 when not yet verified', async () => { @@ -205,6 +235,26 @@ describe('GET /magic-link/check', () => { expect(res.status).toBe(204); }); + + it('returns 403 when the polling fingerprint does not match the pending link', async () => { + (User.findOne as any).mockResolvedValue({ id: 'user-1', email: 'test@example.com' }); + (MagicLinkToken.findOne as any).mockResolvedValue( + buildMagicLink({ ip_hash: 'different-ip-hash', used_at: null }), + ); + + const res = await request(app).get('/magic-link/check'); + + expect(res.status).toBe(403); + expect(res.body.error).toBe('Invalid request'); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + userId: 'user-1', + type: 'magic_link_failed', + metadata: { reason: 'Polling device IP mismatch' }, + }), + ); + expect(Session.create).not.toHaveBeenCalled(); + }); }); it('creates session when magic link completed', async () => { @@ -213,6 +263,7 @@ it('creates session when magic link completed', async () => { email: 'test@example.com', roles: ['user'], save: vi.fn(), + update: vi.fn(), }; (User.findOne as any).mockResolvedValue(user); @@ -230,9 +281,21 @@ it('creates session when magic link completed', async () => { (generateRefreshToken as any).mockReturnValue('refresh-token'); (hashRefreshToken as any).mockResolvedValue('hashed-refresh'); + (createRefreshTokenLookup as any).mockReturnValue('refresh-lookup'); (signAccessToken as any).mockResolvedValue('access-token'); const res = await request(app).get('/magic-link/check'); expect(res.status).toBe(200); + expect(res.body).toEqual( + expect.objectContaining({ + message: 'Success', + token: 'access-token', + refreshToken: 'refresh-token', + sub: 'user-1', + roles: ['user'], + ttl: 900, + refreshTtl: 3600, + }), + ); }); diff --git a/tests/integration/oauth/oauth.spec.ts b/tests/integration/oauth/oauth.spec.ts index 2f54914..220dfe5 100644 --- a/tests/integration/oauth/oauth.spec.ts +++ b/tests/integration/oauth/oauth.spec.ts @@ -13,6 +13,7 @@ import { import { OAuthIdentity } from '../../../src/models/oauthIdentities.js'; import { Session } from '../../../src/models/sessions.js'; import { User } from '../../../src/models/users.js'; +import { AuthEventService } from '../../../src/services/authEventService.js'; import { clearOAuthStateReplayCache } from '../../../src/services/oauthService.js'; import { buildSystemConfig } from '../../factories/systemConfigFactory.js'; import { buildUser } from '../../factories/userFactory.js'; @@ -71,6 +72,24 @@ describe('OAuth routes', () => { expect(JSON.stringify(res.body)).not.toContain('GOOGLE_CLIENT_SECRET'); }); + it('hides OAuth providers when OAuth login is disabled', async () => { + (getSystemConfig as any).mockResolvedValue( + buildSystemConfig({ + login_methods: ['passkey'], + oauth_providers: [provider], + }), + ); + + const providersRes = await request(app).get('/oauth/providers'); + const startRes = await request(app).post('/oauth/google/start').send({ + redirectUri: 'http://localhost:5174/oauth/callback', + }); + + expect(providersRes.status).toBe(200); + expect(providersRes.body.providers).toEqual([]); + expect(startRes.status).toBe(404); + }); + it('starts OAuth login with a signed state and authorization URL', async () => { const res = await request(app).post('/oauth/google/start').send({ redirectUri: 'http://localhost:5174/oauth/callback', @@ -175,4 +194,29 @@ describe('OAuth routes', () => { expect(replay.body.error).toBe('Invalid OAuth state'); expect(fetchMock).toHaveBeenCalledTimes(2); }); + + it('fails closed when OAuth callback secrets are missing', async () => { + const start = await request(app).post('/oauth/google/start').send({ + redirectUri: 'http://localhost:5174/oauth/callback', + }); + const fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + vi.stubEnv('GOOGLE_CLIENT_SECRET', ''); + + const res = await request(app).post('/oauth/google/callback').send({ + code: 'oauth-code', + state: start.body.state, + }); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('OAuth login failed'); + expect(fetchMock).not.toHaveBeenCalled(); + expect(Session.create).not.toHaveBeenCalled(); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'oauth_login_failed', + metadata: { providerId: 'google', reason: 'callback_failed' }, + }), + ); + }); }); diff --git a/tests/integration/otp/otp.spec.ts b/tests/integration/otp/otp.spec.ts index ba85291..85cd641 100644 --- a/tests/integration/otp/otp.spec.ts +++ b/tests/integration/otp/otp.spec.ts @@ -1,6 +1,7 @@ import request from 'supertest'; import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest'; import { createApp } from '../../../src/app'; +import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; import { Application } from 'express'; vi.mock('../../../src/models/authEvents.js', () => ({ @@ -41,6 +42,10 @@ beforeAll(async () => { beforeEach(() => { vi.resetModules(); vi.clearAllMocks(); + (getSystemConfig as any).mockResolvedValue({ + login_methods: ['passkey', 'magic_link', 'email_otp', 'phone_otp', 'oauth'], + passkey_login_fallback_enabled: true, + }); }); describe('OTP - Generate', () => { @@ -58,6 +63,19 @@ describe('OTP - Generate', () => { expect(res.status).toBe(200); expect(generateEmailOTP).toHaveBeenCalled(); }); + + it('rejects login phone OTP generation when phone OTP login is disabled', async () => { + (getSystemConfig as any).mockResolvedValue({ + login_methods: ['passkey', 'email_otp'], + passkey_login_fallback_enabled: true, + }); + + const res = await request(app).get('/otp/generate-login-phone-otp'); + + expect(res.status).toBe(403); + expect(res.body.error).toBe('login_method_disabled'); + expect(generatePhoneOTP).not.toHaveBeenCalled(); + }); }); describe('OTP - Verify Phone', () => { diff --git a/tests/unit/lib/defineRoute.authIntegration.spec.ts b/tests/unit/lib/defineRoute.authIntegration.spec.ts new file mode 100644 index 0000000..f16632f --- /dev/null +++ b/tests/unit/lib/defineRoute.authIntegration.spec.ts @@ -0,0 +1,84 @@ +import express, { Router } from 'express'; +import request from 'supertest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod'; + +vi.unmock('../../../src/middleware/attachAuthMiddleware.js'); + +vi.mock('../../../src/openapi/registry.js', () => ({ + registry: { + registerPath: vi.fn(), + }, +})); + +describe('defineRoute auth integration', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('rejects a protected route before handler execution when bearer auth is missing', async () => { + const { defineRoute } = await import('../../../src/lib/defineRoute.js'); + const { validateBearerToken } = await import('../../../src/services/sessionService.js'); + const handler = vi.fn((_req, res) => res.status(200).json({ message: 'ok' })); + const app = express(); + const router = Router(); + + defineRoute(router, { + method: 'get', + path: '/secure', + auth: 'access', + schemas: { + response: { + 200: z.object({ message: z.string() }), + 401: z.object({ error: z.string() }), + }, + }, + handler, + }); + + app.use(router); + + const res = await request(app).get('/secure'); + + expect(res.status).toBe(401); + expect(res.body).toEqual({ error: 'missing bearer token' }); + expect(handler).not.toHaveBeenCalled(); + expect(validateBearerToken).not.toHaveBeenCalled(); + }); + + it('passes the declared auth type to bearer validation before running the handler', async () => { + const { defineRoute } = await import('../../../src/lib/defineRoute.js'); + const { validateBearerToken } = await import('../../../src/services/sessionService.js'); + const app = express(); + const router = Router(); + + (validateBearerToken as any).mockResolvedValue({ + user: { id: 'user-1', roles: ['user'] }, + sessionId: 'session-1', + }); + + defineRoute(router, { + method: 'get', + path: '/secure', + auth: 'access', + schemas: { + response: { + 200: z.object({ message: z.string() }), + }, + }, + handler: (req, res) => { + res.status(200).json({ + message: req.user?.id ?? 'missing', + }); + }, + }); + + app.use(router); + + const res = await request(app).get('/secure').set('Authorization', 'Bearer access-token'); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ message: 'user-1' }); + expect(validateBearerToken).toHaveBeenCalledWith('access-token', 'access'); + }); +}); diff --git a/tests/unit/routes/responseSchemaCoverage.spec.ts b/tests/unit/routes/responseSchemaCoverage.spec.ts index e78744b..14f9ea5 100644 --- a/tests/unit/routes/responseSchemaCoverage.spec.ts +++ b/tests/unit/routes/responseSchemaCoverage.spec.ts @@ -9,6 +9,22 @@ const routeCallPattern = /\w+Router\.(get|post|patch|put|delete)\(\s*(["'])([^"']+)\2([\s\S]*?)\n\);/g; describe('route response schema coverage', () => { + it('keeps protected routes wired through defineRoute auth metadata', () => { + const manualAuthRoutes: string[] = []; + + for (const fileName of readdirSync(routesDir) + .filter((name) => name.endsWith('.ts')) + .sort()) { + const source = readFileSync(join(routesDir, fileName), 'utf8'); + + if (/\battachAuthMiddleware\b/.test(source)) { + manualAuthRoutes.push(fileName); + } + } + + expect(manualAuthRoutes).toEqual([]); + }); + it('documents every route with an explicit response schema', () => { const missingResponses: string[] = []; From 11b7e6d3c5aa40dff4dcd31ca9f288d34a2f72b3 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 27 Jun 2026 13:55:42 +0200 Subject: [PATCH 4/9] chore: add Claude Code project config - CLAUDE.md: imports AGENTS.md and adds a working agreement (verify loop, license-header/commit/changeset conventions, don't-touch list) - .claude/settings.json: allowlist safe project commands to reduce prompts - .gitignore: keep personal .claude/settings.local.json out of version control Co-Authored-By: Claude Opus 4.8 --- .claude/settings.json | 19 +++++++++++++ .gitignore | 3 ++ CLAUDE.md | 66 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 .claude/settings.json create mode 100644 CLAUDE.md diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..7d81553 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,19 @@ +{ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "permissions": { + "allow": [ + "Bash(npm run typecheck:*)", + "Bash(npm run lint:*)", + "Bash(npm run test:run:*)", + "Bash(npm run test:ci:*)", + "Bash(npm run build:*)", + "Bash(npm run coverage:*)", + "Bash(npm run format:*)", + "Bash(git status:*)", + "Bash(git diff:*)", + "Bash(git log:*)", + "Bash(git show:*)", + "Bash(git branch:*)" + ] + } +} diff --git a/.gitignore b/.gitignore index a1a2263..94dfc15 100644 --- a/.gitignore +++ b/.gitignore @@ -29,6 +29,9 @@ docker-data/ .vscode/ *.swp +# Claude Code — share settings.json, keep personal overrides local +.claude/settings.local.json + # Lint / tooling cache .eslintcache docs/* diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..4f91f23 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,66 @@ +# CLAUDE.md + +This file is loaded automatically at the start of every Claude Code session. + +The full architecture guide lives in **[AGENTS.md](AGENTS.md)** — imported below so +there is a single source of truth. Read it for the runtime shape, folder map, token +model, and config layers. + +@AGENTS.md + +## Working agreement + +### Verify before declaring done + +This project has a fast, reliable check loop. Run the relevant ones after changes and +report real output — never claim a change works without running them. + +| Check | Command | Notes | +| -------- | ------------------- | --------------------------------------------------------------- | +| Types | `npm run typecheck` | `tsc --noEmit`, ~2s | +| Lint | `npm run lint` | eslint, ~2s. Enforces license headers + import sort | +| Tests | `npm run test:run` | vitest, ~2s. **Uses a mock DB by default — no Postgres needed** | +| Coverage | `npm run coverage` | thresholds: lines/functions/statements 70%, branches 65% | + +Tests default to `TEST_DB=mock`. Only set `TEST_DB=postgres` (with a running Postgres) +when specifically exercising real DB behavior. + +### Conventions enforced by tooling + +- **License header** — every `src/**/*.ts` file must begin with the AGPL header (eslint + `license-header/header` errors otherwise). Copy it from any existing file, e.g. + [src/utils/otp.ts](src/utils/otp.ts): + + ```ts + /* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + ``` + +- **Commits** — Conventional Commits, enforced by commitlint + husky on commit + (`feat:`, `fix:`, `chore:`, `docs:`, `ci:`, …). Husky also runs lint-staged. +- **Releases** — managed by Changesets. User-facing changes need a changeset + (`npm run changeset`); don't hand-edit `CHANGELOG.md` or the version in `package.json`. +- **No `any`** (`@typescript-eslint/no-explicit-any` is an error) and imports/exports are + auto-sorted (`simple-import-sort`). + +### Codebase shape (where to make changes) + +Routes are auto-discovered: each `src/routes/*.routes.ts` registers handlers via +`defineRoute`, which also wires OpenAPI metadata and validates request/response against +Zod schemas in `src/schemas/`. Trace behavior **route → controller → service → model**. +A new endpoint usually touches: a `*.routes.ts`, a controller, request/response schemas, +and a test under `tests/`. + +### Don't touch + +- `keys/` and `.env*` — local secrets / signing keys. Never read, commit, or modify them. +- `dist/` and `coverage/` — generated output. + +### Security posture + +This is an authentication server. Treat auth, token, OTP, session, and crypto code as +high-stakes: prefer constant-time comparisons, hash secrets at rest, fail closed, and run +`/security-review` on changes to those paths before considering them done. From 320bc1236fa4c7c0e8fa4d9f8b23b42370e01420 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 27 Jun 2026 18:11:00 +0200 Subject: [PATCH 5/9] docs: map ecosystem coupling and add cross-repo ripple protocol - docs/ecosystem.md: deep coupling map of Tier 1/2 sibling repos (seamless-auth-react/-server/-types, seamless-messaging, +consumers): endpoints, schema import sites, token/JWKS touchpoints, blast radius - CLAUDE.md: concise ecosystem section + ripple protocol for contract changes - .gitignore: allowlist docs/ecosystem.md past the docs/* rule Co-Authored-By: Claude Opus 4.8 --- .gitignore | 1 + CLAUDE.md | 35 ++++++++++++++ docs/ecosystem.md | 117 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+) create mode 100644 docs/ecosystem.md diff --git a/.gitignore b/.gitignore index 94dfc15..ca07ec6 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,7 @@ docker-data/ docs/* !docs/admin-operations.md !docs/architecture.md +!docs/ecosystem.md !docs/oauth.md !docs/production-operations.md !docs/webauthn-prf.md diff --git a/CLAUDE.md b/CLAUDE.md index 4f91f23..9d792fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,3 +64,38 @@ and a test under `tests/`. This is an authentication server. Treat auth, token, OTP, session, and crypto code as high-stakes: prefer constant-time comparisons, hash secrets at rest, fail closed, and run `/security-review` on changes to those paths before considering them done. + +## Ecosystem & downstream impact + +This repo is the **contract source** for sibling repos in the parent directory. Changes to +the HTTP/token/schema contract here can break them. Full coupling map (endpoints, schemas, +risks per repo): **[docs/ecosystem.md](docs/ecosystem.md)** — read it before contract changes. + +Topology: `browser → @seamless-auth/react (cookies) → @seamless-auth/server / @seamless-auth/express (bridges to Bearer + JWKS) → THIS API (Bearer/JSON, no cookies)`. +This API also **consumes** `@seamless-auth/types` (shared Zod schemas) and +`@seamless-auth/messaging*` (email/SMS adapter contract), so changes in those repos ripple inward. + +**Direct dependents (Tier 1):** `seamless-auth-server` (server adapter — ~50 routes + JWKS + +token claims), `seamless-auth-react` (client SDK — ~38 hardcoded routes, parses `message`/user +shape), `seamless-auth-types` (shared schemas — bidirectional), `seamless-messaging` (adapter +contract — bidirectional). +**Downstream (Tier 2):** `seamless-auth-admin-dashboard`, `seamless-auth-starter-express`, +`seamless-auth-starter-react`, `seamless-auth-internal-api`, `seamless-auth-docs`, `seamless-cli`. + +All ten are granted in `.claude/settings.local.json` so I can read/edit them. + +### Ripple protocol — when a change here touches the contract + +A "contract change" = adding/renaming/removing a **route or method**, changing a **response/request +schema**, **token** fields/claims or the `/refresh` shape, **JWKS**, a branch-significant **status +code**, or an **auth mode** (ephemeral/access/service). When one happens: + +1. **Flag it** — call out in the change summary that it's contract-affecting and name the + likely-affected repos (use the map). +2. **Survey** the dependents (Explore agents work well in parallel) for concrete break sites — + hardcoded route strings, parsed response fields, expected status codes. +3. **Report blast radius** and propose coordinated edits; apply them in the sibling repos only + with the user's go-ahead. `@seamless-auth/types` changes usually need a version bump + + coordinated release across this API and both SDKs. + +Non-contract changes (internal refactors, tests, logging) don't trigger this. diff --git a/docs/ecosystem.md b/docs/ecosystem.md new file mode 100644 index 0000000..1f0153b --- /dev/null +++ b/docs/ecosystem.md @@ -0,0 +1,117 @@ +# Ecosystem & downstream impact + +`seamless-auth-api` is the contract source for a family of sibling repos under the same +parent directory (`fells-code` org). This document is the **deep coupling map**: +what depends on this API, how, and what changes here ripple outward. `CLAUDE.md` carries +the short version + the ripple protocol; this file is the detail to read before/while +making a contract-affecting change. + +> Last surveyed: 2026-06-27. Versions and line numbers drift — treat specifics as leads to +> re-verify, not gospel. Re-run the survey when the dependency graph changes. + +## Topology + +``` +browser + └─ @seamless-auth/react ............ client SDK (cookie-based session) + └─ @seamless-auth/express ..... server adapter (cookies ⇄ Bearer, JWKS verify) + └─ THIS API (seamless-auth-api) ... Bearer/JSON contract, no cookies + ├─ @seamless-auth/types ..... shared Zod schemas (this API consumes) + └─ @seamless-auth/messaging* . email/SMS adapter contract (this API consumes) +``` + +Note the auth-style boundary: the **React SDK uses cookies** (`credentials: 'include'`), +while **this API is Bearer/JSON with no cookies** (see README "Bearer Token Contract"). +The **server adapter** (`@seamless-auth/express` / `@seamless-auth/core`) is what bridges +them — it holds `seamless-access` / `seamless-refresh` / `seamless-ephemeral` cookies on +the browser side and speaks Bearer + service tokens + JWKS to this API. Direct +browser→API integration is possible ("Direct HTTP APIs (advanced)") but the recommended +path goes through the adapter. + +## Tier 1 — direct contract dependents + +### `seamless-auth-server` — `@seamless-auth/core` + `@seamless-auth/express` (v0.5.x) + +The server-side adapter SDK; a thin stateless proxy + cookie manager. **Highest coupling.** + +- Calls ~50 of this API's routes via `authFetch()` with `Authorization: Bearer`, + `x-seamless-service-token`, `x-seamless-client-ip`. Covers `/login`, `/registration/register`, + all `/otp/*`, `/webAuthn/*`, `/magic-link*`, `/refresh`, `/users/*`, `/organizations/*`, + `/step-up/*`, `/sessions*`, all `/admin/*`, all `/internal/*`, `/system-config/*`. +- **JWKS:** fetches `/.well-known/jwks.json` (`createRemoteJWKSet` + `jwtVerify`, RS256), + validates `iss` against the auth server URL. Reads claims `sub`, `sid`, `aud`, `iss`. +- **Refresh:** POST `/refresh` response unpacked directly into cookies — expects + `sub, token, refreshToken, ttl, refreshTtl, roles?, email?, phone?, organizationId?`. +- **Status-code coupling:** branches on exact codes, e.g. magic-link poll treats `204` as + "not yet verified". +- No shared types package — coupling is 100% string-literal route paths + response shapes. +- **Breaks if this API changes:** any route path/method, JWKS path or key format/alg, token + claim/field names (`sub`/`sid`/...), the `/refresh` response shape, or branch-significant + status codes. + +### `seamless-auth-react` — `@seamless-auth/react` (v0.2.0) + +Drop-in React auth UI (email/phone OTP, magic link, WebAuthn/passkeys, OAuth, step-up, +organizations). Hardcodes ~38 endpoint paths in `src/createSeamlessAuthClient.ts`. + +- Parses response fields for flow control: `message === 'Success'`, plus `token`, + `refreshToken`, `delivery`, and the `/users/me` shape (`user`, `credentials[]`, + `organizations[]`, `activeOrganization`). +- Generic error handling (trusts `response.ok`), so contract breaks often fail **silently**. +- **Breaks if this API changes:** route renames, `message`/user/credential field renames, + switching an endpoint's auth mode (ephemeral ↔ access), or response-shape changes to + `/users/me`, OTP, or organization endpoints. + +### `seamless-auth-types` — `@seamless-auth/types` (v0.1.3) ⇠ this API depends on it + +Shared Zod schemas / TS types — the contract's source of truth. **Reverse coupling:** changes +here propagate _into_ this API and the SDKs. + +- This API imports it at 5 sites: `schemas/internal.responses.ts` (`AuthEventSchema`), + `schemas/credential.responses.ts` (`CredentialApiSchema`, extended), + `schemas/admin.responses.ts` (`AuthEventSchema`, `SessionSchema`), + `schemas/session.responses.ts` (`SessionSchema`), + `routes/users.routes.ts` (`UpdateCredentialRequestSchema`, `DeleteCredentialRequestSchema`). +- **Highest-risk exports:** credential request/response schemas (used in route validation), + `AuthEventSchema` (snake_case, serialized directly), `SessionSchema`, `RoleSchema`. +- ⚠️ **Known drift:** `RoleSchema`'s regex is **duplicated** in this API at + `src/lib/scopedRoles.ts:6` (`ROLE_NAME_PATTERN`) instead of imported — they can silently + diverge. Consider importing from the types package. +- **Breaks if changed here:** rename/remove an exported schema, change/tighten a field or + validator (e.g. `RoleSchema` regex), change `IsoDate` transform → forces coordinated + releases across this API + both SDKs. + +### `seamless-messaging` — `@seamless-auth/messaging` (+ `-aws`, `-twilio`, v0.1.0) ⇠ this API depends on it + +Provider-agnostic email/SMS adapter contract. **Reverse coupling.** + +- This API consumes it at `src/config/directMessaging.ts` (factory + AWS/Twilio transports, + `MessagingConfigurationError`) and `src/services/messagingService.ts` (calls + `sendOtpEmail`, `sendOtpSms`, `sendMagicLinkEmail`, `sendBootstrapInviteEmail` 1:1). +- Core contract: `EmailTransport` / `SmsTransport` (`send()` + `name`), `AuthMessagingService` + (the 4 send methods), and the `Send*Input` payload types (`to`, `token`, `magicLinkUrl`, …). +- **Breaks if changed here:** `AuthMessagingService` method signatures/payloads, transport + `send()` signature, package renames, new required adapter config, or removing + `MessagingConfigurationError`. + +## Tier 2 — downstream consumers (transitively affected) + +| Repo | What it is | Coupling | +| ---------------------------------- | ------------------------------- | ------------------------------------------------------------------------------------------------ | +| `seamless-auth-admin-dashboard` | Admin UI | `@seamless-auth/react` + `@seamless-auth/types`; consumes `/admin/*` + `/internal/*` via the SDK | +| `seamless-auth-starter-express` | Express starter/demo | `@seamless-auth/express` — breaks only if the server adapter's public API changes | +| `seamless-auth-starter-react` | React starter/demo | `@seamless-auth/react` — same, via the client SDK | +| `seamless-auth-internal-api` | Internal/enterprise API variant | Tracks this codebase closely; likely needs parallel changes for shared logic | +| `seamless-auth-docs` | Public docs | Documents the HTTP contract — update when routes/schemas change | +| `seamless-cli` (`create-seamless`) | Scaffolding CLI | Generates projects wired to the SDKs | + +## High-risk contract surfaces (change = survey downstream) + +1. **Route paths / methods** — `src/routes/*.routes.ts`. Hardcoded as strings in both SDKs. +2. **Response schemas** — `src/schemas/*.responses.ts`. Field renames/removals break parsing. +3. **Token contract** — access/refresh/ephemeral/service tokens, claim names (`sub`, `sid`), + the `/refresh` response shape, JWKS at `/.well-known/jwks.json` (RS256). +4. **Status / error codes** — adapters branch on specific codes (e.g. magic-link `204`). +5. **Shared schemas** in `@seamless-auth/types` and the **messaging adapter contract**. +6. **Auth mode** of a route (ephemeral vs access vs service) — see + `src/middleware/attachAuthMiddleware.ts`. From ceb972c8f04496ed4ead4cbca8b95499d91d95e7 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 27 Jun 2026 18:33:19 +0200 Subject: [PATCH 6/9] ci: gate formatting and ratchet coverage thresholds - ci.yml: run `prettier --check` (new format:check script) instead of `--write`, so formatting diffs actually fail the build - vitest.config.ts: raise coverage thresholds to current floor (statements 80 / branches 70 / functions 90 / lines 80) so coverage cannot silently regress; will ratchet up as regression tests land - release.yml: align Node to 20 (matches CI) Co-Authored-By: Claude Opus 4.8 --- .github/workflows/ci.yml | 4 ++-- .github/workflows/release.yml | 2 +- package.json | 1 + vitest.config.ts | 8 ++++---- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86da27e..587e9f1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,8 +24,8 @@ jobs: - name: Lint run: npm run lint - - name: Run formatting - run: npm run format + - name: Check formatting + run: npm run format:check - name: Run Tests with Coverage run: CI=true npm run coverage diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f7741c4..670ca19 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -31,7 +31,7 @@ jobs: - name: Setup Node.js uses: actions/setup-node@v6 with: - node-version: 24.10.0 + node-version: 20 cache: npm - name: Install dependencies diff --git a/package.json b/package.json index 8d2ddde..60b8d3d 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "test:e2e": "CI=false vitest", "lint": "eslint . --ext .ts", "format": "prettier --write .", + "format:check": "prettier --check .", "changeset": "changeset", "version-packages": "changeset version", "db:create": "env-cmd -f .env sequelize-cli db:create || sequelize-cli db:create", diff --git a/vitest.config.ts b/vitest.config.ts index 775026f..b740d90 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -22,10 +22,10 @@ export default defineConfig({ exclude: ['src/**/*.d.ts', 'src/models/index.ts', 'src/server.ts'], thresholds: { - lines: 70, - functions: 70, - branches: 65, - statements: 70, + lines: 80, + functions: 90, + branches: 70, + statements: 80, }, }, }, From 9b0601f542f6f493bbb8d4b7068b3cde6b91815d Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 27 Jun 2026 18:38:32 +0200 Subject: [PATCH 7/9] fix(magic-link): correct verify/poll responses and lock with regression tests - verifyMagicLink: remove dead no-op device-binding branches; device binding is correctly enforced at the poll step (the email may be opened on another device) - pollMagicLinkConfirmation: return 204 with no body (was 204 + JSON body that Express strips); status stays 204 for the server adapter - await the post-session user.update so failures surface - regression tests: full request -> verify -> poll -> session sequence (the path that broke), user-agent mismatch -> 403, 204-empty-body guard (was 500), verify stays device-agnostic Co-Authored-By: Claude Opus 4.8 --- resources/coverage-badge.svg | 8 +- src/controllers/magicLinks.ts | 20 ++-- tests/integration/magicLink/magicLink.spec.ts | 99 +++++++++++++++++++ 3 files changed, 109 insertions(+), 18 deletions(-) diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index cfaca7c..efd658f 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 82.1% + + coverage: 82.2% @@ -17,7 +17,7 @@ coverage coverage - 82.1% - 82.1% + 82.2% + 82.2% diff --git a/src/controllers/magicLinks.ts b/src/controllers/magicLinks.ts index 07f1c21..7b5ed6a 100644 --- a/src/controllers/magicLinks.ts +++ b/src/controllers/magicLinks.ts @@ -202,17 +202,9 @@ export async function verifyMagicLink(req: Request, res: Response) { metadata: { reason: 'Magic link token consumed' }, }); - // Device binding check - const { ip_hash, user_agent_hash } = hashDeviceFingerprint(req.ip, req.headers['user-agent']); - - if (record.ip_hash && record.ip_hash !== ip_hash) { - return res.status(200).json({ message: 'Success' }); - } - - if (record.user_agent_hash && record.user_agent_hash !== user_agent_hash) { - return res.status(200).json({ message: 'Success' }); - } - + // Device binding is enforced at the poll step (pollMagicLinkConfirmation), where the + // session is actually issued. A magic link may legitimately be opened on a different + // device than the one that requested it, so verification must not gate on the device. return res.status(200).json({ message: 'Success' }); } @@ -238,7 +230,7 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { if (!record) { logger.warn('No magic link token'); - return res.status(204).json({ message: 'Success' }); + return res.status(204).end(); } // Device binding check @@ -294,12 +286,12 @@ export async function pollMagicLinkConfirmation(req: Request, res: Response) { res, }); - user.update({ + await user.update({ lastLogin: new Date(), challengeContext: null, }); return; } - return res.status(204).json({ message: 'Success' }); + return res.status(204).end(); } diff --git a/tests/integration/magicLink/magicLink.spec.ts b/tests/integration/magicLink/magicLink.spec.ts index 9f625d1..e8def22 100644 --- a/tests/integration/magicLink/magicLink.spec.ts +++ b/tests/integration/magicLink/magicLink.spec.ts @@ -188,6 +188,21 @@ describe('GET /magic-link/verify/:token', () => { expect(res.status).toBe(200); }); + it('accepts a valid token even when opened on a different device', async () => { + // The email link may legitimately be opened on a different device than the one + // that requested it. Device binding is enforced at the poll step, not here — so + // verify must remain device-agnostic. Guards against re-adding a device gate here. + (MagicLinkToken.findOne as any).mockResolvedValue( + buildMagicLink({ ip_hash: 'a-different-device', user_agent_hash: 'another-ua' }), + ); + (MagicLinkToken.update as any).mockResolvedValue([1]); + + const res = await request(app).get('/magic-link/verify/token'); + + expect(res.status).toBe(200); + expect(MagicLinkToken.update).toHaveBeenCalled(); + }); + it('rejects token verification when magic links are disabled', async () => { (getSystemConfig as any).mockResolvedValue({ login_methods: ['passkey'], @@ -255,6 +270,38 @@ describe('GET /magic-link/check', () => { ); expect(Session.create).not.toHaveBeenCalled(); }); + + it('returns 403 when the polling user-agent does not match the pending link', async () => { + (User.findOne as any).mockResolvedValue({ id: 'user-1', email: 'test@example.com' }); + (MagicLinkToken.findOne as any).mockResolvedValue( + buildMagicLink({ user_agent_hash: 'different-ua-hash', used_at: null }), + ); + + const res = await request(app).get('/magic-link/check'); + + expect(res.status).toBe(403); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + userId: 'user-1', + type: 'magic_link_failed', + metadata: { reason: 'Polling device user agent mismatch' }, + }), + ); + expect(Session.create).not.toHaveBeenCalled(); + }); + + it('polls with 204 and an empty body while waiting (regression: previously 500)', async () => { + // The exact bug that created this branch: polling before confirmation returned 500, + // breaking the CLI starter sign-in. It must return 204 (No Content), never 500. + (User.findOne as any).mockResolvedValue({ id: 'user-1', email: 'test@example.com' }); + (MagicLinkToken.findOne as any).mockResolvedValue(buildMagicLink({ used_at: null })); + + const res = await request(app).get('/magic-link/check'); + + expect(res.status).toBe(204); + expect(res.status).not.toBe(500); + expect(res.text).toBe(''); + }); }); it('creates session when magic link completed', async () => { @@ -299,3 +346,55 @@ it('creates session when magic link completed', async () => { }), ); }); + +describe('magic link full sign-in sequence (regression)', () => { + it('request -> poll(waiting) -> verify -> poll issues a session', async () => { + const user = { + id: 'user-1', + email: 'test@example.com', + roles: ['user'], + save: vi.fn(), + update: vi.fn(), + }; + (User.findOne as any).mockResolvedValue(user); + // clearAllMocks (beforeEach) clears calls but not implementations, so reset the + // delivery mock another test may have left rejecting. + (sendMagicLinkEmail as any).mockResolvedValue(undefined); + + // 1) Request the link + (MagicLinkToken.update as any).mockResolvedValue([1]); + (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); + const requestRes = await request(app).get('/magic-link'); + expect(requestRes.status).toBe(200); + + // 2) Poll before the link is verified -> 204 (still waiting), not 500 + (MagicLinkToken.findOne as any).mockResolvedValue(buildMagicLink({ used_at: null })); + const pendingPoll = await request(app).get('/magic-link/check'); + expect(pendingPoll.status).toBe(204); + + // 3) Verify the token (marks it used) + (MagicLinkToken.findOne as any).mockResolvedValue(buildMagicLink({ used_at: null })); + (MagicLinkToken.update as any).mockResolvedValue([1]); + const verifyRes = await request(app).get('/magic-link/verify/token'); + expect(verifyRes.status).toBe(200); + + // 4) Poll after verification -> session issued + (MagicLinkToken.findOne as any).mockResolvedValue(buildMagicLink({ used_at: new Date() })); + (Session.create as any).mockResolvedValue({ id: 'session-1' }); + (generateRefreshToken as any).mockReturnValue('refresh-token'); + (hashRefreshToken as any).mockResolvedValue('hashed-refresh'); + (createRefreshTokenLookup as any).mockReturnValue('refresh-lookup'); + (signAccessToken as any).mockResolvedValue('access-token'); + + const completedPoll = await request(app).get('/magic-link/check'); + expect(completedPoll.status).toBe(200); + expect(completedPoll.body).toEqual( + expect.objectContaining({ + message: 'Success', + token: 'access-token', + refreshToken: 'refresh-token', + sub: 'user-1', + }), + ); + }); +}); From 9261060f3ed274ce73c01ba41698e2cd42e867d1 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 27 Jun 2026 18:43:16 +0200 Subject: [PATCH 8/9] fix(otp): rate-limit verify, drop plaintext fallback, await session writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - rate-limit the four OTP verify endpoints (otpIpLimiter + otpIdentityLimiter); they were previously unthrottled and brute-forceable once an ephemeral token was held - otpMatchesStoredValue: remove the transitional plaintext-OTP fallback; OTPs are hashed-only (sha256:) now. Any code issued before hashing (5-min TTL) simply expires — re-request to get a hashed one - await the post-session user.update(lastLogin) writes so failures surface - tests: flip legacy-plaintext-acceptance to rejection; add round-trip regression tests (email/phone generate->verify) and a hashed-at-rest format guard Co-Authored-By: Claude Opus 4.8 --- src/controllers/otp.ts | 6 ++-- src/routes/otp.routes.ts | 4 +++ src/utils/otp.ts | 12 +++---- tests/unit/utils/otp.spec.ts | 62 +++++++++++++++++++++++++++++++++--- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/src/controllers/otp.ts b/src/controllers/otp.ts index 0811fa3..5889a89 100644 --- a/src/controllers/otp.ts +++ b/src/controllers/otp.ts @@ -371,7 +371,7 @@ export const verifyEmail = async (req: Request, res: Response) => { res, }); - user.update({ + await user.update({ lastLogin: new Date(), }); @@ -464,7 +464,7 @@ export const verifyLoginPhoneNumber = async (req: Request, res: Response) => { res, }); - user.update({ + await user.update({ lastLogin: new Date(), }); @@ -570,7 +570,7 @@ export const verifyLoginEmail = async (req: Request, res: Response) => { res, }); - user.update({ + await user.update({ lastLogin: new Date(), }); diff --git a/src/routes/otp.routes.ts b/src/routes/otp.routes.ts index b380e3d..21f57cd 100644 --- a/src/routes/otp.routes.ts +++ b/src/routes/otp.routes.ts @@ -102,6 +102,7 @@ otpRouter.post( auth: 'ephemeral', summary: 'Verify email OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { body: VerifyOTPRequestSchema, @@ -123,6 +124,7 @@ otpRouter.post( auth: 'ephemeral', summary: 'Verify phone OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { body: VerifyOTPRequestSchema, @@ -144,6 +146,7 @@ otpRouter.post( auth: 'ephemeral', summary: 'Verify login email OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { body: VerifyOTPRequestSchema, @@ -164,6 +167,7 @@ otpRouter.post( auth: 'ephemeral', summary: 'Verify login phone OTP', tags: ['OTP'], + middleware: [otpIpLimiter, otpIdentityLimiter], schemas: { body: VerifyOTPRequestSchema, diff --git a/src/utils/otp.ts b/src/utils/otp.ts index 8c12687..cdb161c 100644 --- a/src/utils/otp.ts +++ b/src/utils/otp.ts @@ -47,14 +47,10 @@ function otpMatchesStoredValue( verificationToken: string, normalize: (token: string) => string, ) { - const normalizedVerificationToken = normalize(verificationToken); - - if (storedToken.startsWith(OTP_HASH_PREFIX)) { - return safeStringEqual(storedToken, hashOtpToken(normalizedVerificationToken)); - } - - // Transitional compatibility for OTPs issued before hashed storage was introduced. - return safeStringEqual(normalize(storedToken), normalizedVerificationToken); + // OTPs are always stored hashed (`sha256:` prefix). Compare the hash of the + // normalized submission against the stored hash in constant time. Plaintext OTPs + // are no longer accepted; any issued before hashing (5-min TTL) simply expire. + return safeStringEqual(storedToken, hashOtpToken(normalize(verificationToken))); } export const generateRandomEmailOTP = (): string => { diff --git a/tests/unit/utils/otp.spec.ts b/tests/unit/utils/otp.spec.ts index 3744f98..cbe0a2c 100644 --- a/tests/unit/utils/otp.spec.ts +++ b/tests/unit/utils/otp.spec.ts @@ -200,7 +200,7 @@ describe('OTP utils', () => { }); }); }); -it('accepts legacy plaintext phone OTP values during rollout', async () => { +it('rejects legacy plaintext phone OTP values (hashed-only after hardening)', async () => { const user = buildUser({ phoneVerificationToken: '123456', phoneVerificationTokenExpiry: Date.now() + 10000, @@ -209,10 +209,10 @@ it('accepts legacy plaintext phone OTP values during rollout', async () => { const result = await verifyPhoneOTP(user as any, '123456'); - expect(result.verified).toBe(true); + expect(result.verified).toBe(false); }); -it('accepts legacy plaintext email OTP values during rollout', async () => { +it('rejects legacy plaintext email OTP values (hashed-only after hardening)', async () => { const user = buildUser({ emailVerificationToken: 'ABCDEF', emailVerificationTokenExpiry: Date.now() + 10000, @@ -221,5 +221,59 @@ it('accepts legacy plaintext email OTP values during rollout', async () => { const result = await verifyEmailOTP(user as any, 'abcdef'); - expect(result.verified).toBe(true); + expect(result.verified).toBe(false); +}); + +describe('OTP regression — hardened behavior', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('email OTP round-trips: a generated token verifies against the stored hash', async () => { + const issuingUser = buildUser(); + const token = await generateEmailOTP(issuingUser as any, { sendMessage: false }); + const stored = (issuingUser.update as any).mock.calls[0][0].emailVerificationToken; + + const verifyingUser = buildUser({ + emailVerificationToken: stored, + emailVerificationTokenExpiry: Date.now() + 100000, + }); + + expect((await verifyEmailOTP(verifyingUser as any, token)).verified).toBe(true); + }); + + it('email OTP round-trip is case-insensitive', async () => { + const issuingUser = buildUser(); + const token = await generateEmailOTP(issuingUser as any, { sendMessage: false }); + const stored = (issuingUser.update as any).mock.calls[0][0].emailVerificationToken; + + const verifyingUser = buildUser({ + emailVerificationToken: stored, + emailVerificationTokenExpiry: Date.now() + 100000, + }); + + expect((await verifyEmailOTP(verifyingUser as any, token.toLowerCase())).verified).toBe(true); + }); + + it('phone OTP round-trips: a generated token verifies against the stored hash', async () => { + const issuingUser = buildUser(); + const token = await generatePhoneOTP(issuingUser as any, { sendMessage: false }); + const stored = (issuingUser.update as any).mock.calls[0][0].phoneVerificationToken; + + const verifyingUser = buildUser({ + phoneVerificationToken: stored, + phoneVerificationTokenExpiry: Date.now() + 100000, + }); + + expect((await verifyPhoneOTP(verifyingUser as any, String(token))).verified).toBe(true); + }); + + it('stores OTPs hashed at rest (sha256: prefix, never the plaintext code)', async () => { + const user = buildUser(); + const token = await generateEmailOTP(user as any, { sendMessage: false }); + const stored = (user.update as any).mock.calls[0][0].emailVerificationToken; + + expect(stored).toMatch(/^sha256:[a-f0-9]{64}$/); + expect(stored).not.toContain(token); + }); }); From 03651ba31f8b9c2485c970b0a257bc1c5eb57ebb Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 27 Jun 2026 18:44:19 +0200 Subject: [PATCH 9/9] chore: add changeset for magic link & OTP hardening Co-Authored-By: Claude Opus 4.8 --- .changeset/harden-magic-link-otp.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 .changeset/harden-magic-link-otp.md diff --git a/.changeset/harden-magic-link-otp.md b/.changeset/harden-magic-link-otp.md new file mode 100644 index 0000000..d0df703 --- /dev/null +++ b/.changeset/harden-magic-link-otp.md @@ -0,0 +1,14 @@ +--- +'seamless-auth-api': patch +--- + +Harden and regression-test the magic link and OTP sign-in flows. + +- Magic link: polling while waiting now returns `204` (no body) instead of `500`, + fixing the broken starter sign-in; removed dead device-binding code from verify + (binding is enforced at the poll step); the post-session write is awaited. +- OTP: the verify endpoints are now rate-limited; OTPs are stored and compared + hashed-only (the transitional plaintext fallback is removed); post-session writes + are awaited. +- CI: formatting is enforced (`prettier --check`) and coverage thresholds are + ratcheted so these flows cannot silently regress.