From 88f6d5d9071e0246c4e282a9c12c787d5300b517 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:27:20 -0500 Subject: [PATCH 01/13] test(webauthn): reproduce passkey assurance taken from the assertion flag Refs #552 --- .../webauthn/passkeyAssurance.spec.ts | 184 ++++++++++++++++++ 1 file changed, 184 insertions(+) create mode 100644 tests/integration/webauthn/passkeyAssurance.spec.ts diff --git a/tests/integration/webauthn/passkeyAssurance.spec.ts b/tests/integration/webauthn/passkeyAssurance.spec.ts new file mode 100644 index 00000000..b6154d2f --- /dev/null +++ b/tests/integration/webauthn/passkeyAssurance.spec.ts @@ -0,0 +1,184 @@ +import request from 'supertest'; +import { Application } from 'express'; +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createApp } from '../../../src/app'; +import { getSystemConfig } from '../../../src/config/getSystemConfig'; +import { finishWebAuthnStepUp } from '../../../src/controllers/stepUp'; +import { generateRefreshToken, signAccessToken } from '../../../src/lib/token'; +import { AuthEvent } from '../../../src/models/authEvents'; +import { Credential } from '../../../src/models/credentials'; +import { Session } from '../../../src/models/sessions'; +import { WebAuthnChallenge } from '../../../src/models/webauthnChallenges'; +import { buildCredential } from '../../factories/credentialFactory'; +import { buildSession } from '../../factories/sessionFactory'; +import { buildUser } from '../../factories/userFactory'; +import { buildWebAuthnChallenge } from '../../factories/webauthnChallengeFactory'; + +// The backup eligibility flag in an assertion is reported by the authenticator. WebAuthn +// Level 3 fixes it at registration, so the stored value is what decides hardware assurance, +// and only a registration whose attestation was verified can claim it. + +let app: Application; + +vi.mock('../../../src/middleware/attachAuthMiddleware.js', async (importOriginal) => { + const actual = + await importOriginal(); + + return { + ...actual, + attachAuthMiddleware: () => (req: any, _res: any, next: any) => { + req.user = buildUser(); + req.sessionId = 'session-1'; + req.attemptId = 'attempt-1'; + next(); + }, + }; +}); + +beforeAll(async () => { + app = await createApp(); +}); + +beforeEach(() => { + (WebAuthnChallenge.findOne as any).mockResolvedValue(buildWebAuthnChallenge()); + (AuthEvent.count as any).mockResolvedValue(0); + (getSystemConfig as any).mockResolvedValue({ + app_name: 'SeamlessAuth', + rpid: 'localhost', + origins: ['http://localhost:5137'], + access_token_ttl: '15m', + refresh_token_ttl: '1h', + session_idle_ttl: '8h', + authenticator_policy: { + attachment: 'any', + userVerification: 'required', + attestation: 'none', + requireKnownAuthenticator: false, + syncedPasskeys: 'allow', + aaguidAllowList: [], + aaguidDenyList: [], + }, + }); +}); + +async function assertWith(reported: 'singleDevice' | 'multiDevice') { + const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); + (verifyAuthenticationResponse as any).mockResolvedValue({ + verified: true, + authenticationInfo: { newCounter: 1, credentialDeviceType: reported, userVerified: true }, + }); +} + +async function signIn(stored: Record, reported: 'singleDevice' | 'multiDevice') { + (Credential.findOne as any).mockResolvedValue( + buildCredential({ id: 'cred-1', update: vi.fn(), ...stored }), + ); + (Session.create as any).mockResolvedValue({ id: 'session-1' }); + (signAccessToken as any).mockResolvedValue('access-token'); + (generateRefreshToken as any).mockReturnValue('refresh-token'); + await assertWith(reported); + + return request(app) + .post('/webauthn/login/finish') + .send({ assertionResponse: { id: 'cred-1' } }); +} + +function signedInPasskey() { + const created = (Session.create as any).mock.calls[0]?.[0]; + return created?.authMethods?.find((m: any) => m.method === 'passkey'); +} + +async function stepUp(stored: Record, reported: 'singleDevice' | 'multiDevice') { + const user = buildUser(); + const session = buildSession({ authMethods: [] }); + (WebAuthnChallenge.findOne as any).mockResolvedValue( + buildWebAuthnChallenge({ purpose: 'step_up' }), + ); + (Credential.findOne as any).mockResolvedValue( + buildCredential({ id: 'cred-1', userId: user.id, ...stored }), + ); + (Session.findOne as any).mockResolvedValue(session); + await assertWith(reported); + + const res: any = {}; + res.status = vi.fn().mockReturnValue(res); + res.json = vi.fn().mockReturnValue(res); + await finishWebAuthnStepUp( + { + body: { assertionResponse: { id: 'cred-1' } }, + headers: {}, + ip: '127.0.0.1', + user, + sessionId: 'session-1', + } as any, + res, + ); + return { res, session }; +} + +describe('passkey sign-in assurance', () => { + it('refuses an assertion whose backup eligibility differs from the registered credential', async () => { + const res = await signIn( + { deviceType: 'multiDevice', attestationVerified: true }, + 'singleDevice', + ); + + expect(res.status).toBe(401); + expect(Session.create).not.toHaveBeenCalled(); + }); + + it('does not call an unattested credential hardware bound', async () => { + const res = await signIn( + { deviceType: 'singleDevice', attestationType: 'none', attestationVerified: false }, + 'singleDevice', + ); + + expect(res.status).toBe(200); + expect(signedInPasskey()).toEqual(expect.objectContaining({ deviceBound: false })); + }); + + it('calls a device-bound credential with verified attestation hardware bound', async () => { + const res = await signIn( + { deviceType: 'singleDevice', attestationType: 'basic', attestationVerified: true }, + 'singleDevice', + ); + + expect(res.status).toBe(200); + expect(signedInPasskey()).toEqual(expect.objectContaining({ deviceBound: true })); + }); +}); + +describe('passkey step-up assurance', () => { + it('refuses an assertion whose backup eligibility differs from the registered credential', async () => { + const { res, session } = await stepUp( + { deviceType: 'multiDevice', attestationVerified: true }, + 'singleDevice', + ); + + expect(res.status).toHaveBeenCalledWith(401); + expect(session.authMethods).toEqual([]); + }); + + it('does not call an unattested credential hardware bound', async () => { + const { session } = await stepUp( + { deviceType: 'singleDevice', attestationType: 'none', attestationVerified: false }, + 'singleDevice', + ); + + expect(session.authMethods).toEqual([ + expect.objectContaining({ method: 'passkey', deviceBound: false }), + ]); + }); + + it('calls a device-bound credential with verified attestation hardware bound', async () => { + const { session } = await stepUp( + { deviceType: 'singleDevice', attestationType: 'basic', attestationVerified: true }, + 'singleDevice', + ); + + expect(session.authMethods).toEqual([ + expect.objectContaining({ method: 'passkey', deviceBound: true }), + ]); + }); +}); From 985f7ed6a52949d88c9119680895e0041e1e5da6 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:28:57 -0500 Subject: [PATCH 02/13] fix(webauthn): derive passkey hardware assurance from the registered credential Device binding now comes from the deviceType stored at registration, and counts only when that registration's attestation was verified. An assertion whose backup eligibility flag differs from the stored value is refused at sign-in and step-up. Refs #552 --- .../passkey-assurance-from-registration.md | 5 +++ docs/adr/0001-openid-provider.md | 18 ++++---- src/controllers/stepUp.ts | 18 +++++++- src/controllers/webauthn.ts | 20 ++++++++- src/lib/passkeyAssurance.ts | 38 ++++++++++++++++ .../integration/breakGlass/breakGlass.spec.ts | 2 +- tests/integration/webauthn/webauthn.spec.ts | 8 ++-- tests/unit/controllers/stepUp.spec.ts | 12 +++-- tests/unit/lib/passkeyAssurance.spec.ts | 44 +++++++++++++++++++ 9 files changed, 145 insertions(+), 20 deletions(-) create mode 100644 .changeset/passkey-assurance-from-registration.md create mode 100644 src/lib/passkeyAssurance.ts create mode 100644 tests/unit/lib/passkeyAssurance.spec.ts diff --git a/.changeset/passkey-assurance-from-registration.md b/.changeset/passkey-assurance-from-registration.md new file mode 100644 index 00000000..4be32fdb --- /dev/null +++ b/.changeset/passkey-assurance-from-registration.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +Passkey hardware assurance now comes from the registered credential rather than the assertion (#552). Sign-in and step-up refuse an assertion whose backup eligibility flag differs from the one stored at registration (`401`, audited as `webauthn_login_failed` or `step_up_failed` with reason `Backup eligibility differs from registration`). A passkey counts as device-bound, and earns `acr=phrh` and `amr=hwk`, only when it was registered device-bound with attestation verified against the FIDO Metadata Service. Deployments that use the default `none` attestation, and credentials registered before attestation was recorded, now get `phr` and `swk` instead of `phrh` and `hwk`. diff --git a/docs/adr/0001-openid-provider.md b/docs/adr/0001-openid-provider.md index 33198c79..39b95fff 100644 --- a/docs/adr/0001-openid-provider.md +++ b/docs/adr/0001-openid-provider.md @@ -215,14 +215,14 @@ The API derives both from the session's `auth_methods`. The edge only passes the `amr` uses the RFC 8176 registry: -| Sign-in method | `amr` | -| --------------------------------------- | -------------------------------------------------- | -| Passkey, device-bound (BE flag not set) | `hwk`, plus `user` and `mfa` when UV was performed | -| Passkey, synced or unknown | `swk`, plus `user` and `mfa` when UV was performed | -| Email code or magic link | `otp` | -| SMS code | `sms`, `otp` | -| TOTP | `otp` | -| Two distinct factors in one session | `mfa` added | +| Sign-in method | `amr` | +| ----------------------------------- | -------------------------------------------------- | +| Passkey, device-bound (see below) | `hwk`, plus `user` and `mfa` when UV was performed | +| Passkey, synced or unknown | `swk`, plus `user` and `mfa` when UV was performed | +| Email code or magic link | `otp` | +| SMS code | `sms`, `otp` | +| TOTP | `otp` | +| Two distinct factors in one session | `mfa` added | `acr` uses the OpenID Enhanced Authentication Profile ACR values: @@ -230,6 +230,8 @@ The API derives both from the session's `auth_methods`. The edge only passes the - `phr` for any passkey with user verification. - Omitted otherwise. +**Amendment, 2026-11-02: device binding comes from registration (#552).** A passkey counts as device-bound only when its registration stored the BE flag as not set and its attestation was verified against the FIDO Metadata Service. The flag in each assertion is no longer trusted on its own: WebAuthn Level 3 fixes it at registration, so an assertion whose flag differs from the stored value is refused, and under `none` attestation the flag is self-reported. A deployment that does not request attestation therefore never issues `phrh` or `hwk`. + `acr_values_supported` is `["phr", "phrh"]`. Seamless-specific names were rejected, because generic RPs would not recognise them. #510's `webauthn` example is read with this mapping. ## D10. Consent phishing limits diff --git a/src/controllers/stepUp.ts b/src/controllers/stepUp.ts index 0c6153ec..a81b6e41 100644 --- a/src/controllers/stepUp.ts +++ b/src/controllers/stepUp.ts @@ -13,6 +13,7 @@ import base64url from 'base64url'; import { Request, Response } from 'express'; import { getSystemConfig } from '../config/getSystemConfig.js'; +import { passkeyDeviceBinding } from '../lib/passkeyAssurance.js'; import { buildPrfAuthenticationExtensions, containsPrfOutput } from '../lib/webauthnPrf.js'; import { Credential } from '../models/credentials.js'; import { AuthEventService } from '../services/authEventService.js'; @@ -228,6 +229,21 @@ export const finishWebAuthnStepUp = async (req: Request, res: Response) => { return res.status(401).json({ error: 'step_up_failed' }); } + const binding = passkeyDeviceBinding( + credential, + verification.authenticationInfo.credentialDeviceType, + ); + + if (!binding) { + await AuthEventService.log({ + userId: user.id, + type: 'step_up_failed', + req, + metadata: { reason: 'Backup eligibility differs from registration' }, + }); + return res.status(401).json({ error: 'step_up_failed' }); + } + await credential.update({ lastUsedAt: new Date(), counter: verification.authenticationInfo.newCounter, @@ -238,7 +254,7 @@ export const finishWebAuthnStepUp = async (req: Request, res: Response) => { userId: user.id, method: 'webauthn', passkey: { - deviceBound: verification.authenticationInfo.credentialDeviceType === 'singleDevice', + deviceBound: binding.deviceBound, userVerified: verification.authenticationInfo.userVerified, }, }); diff --git a/src/controllers/webauthn.ts b/src/controllers/webauthn.ts index 8afa80f5..0a93e7d7 100644 --- a/src/controllers/webauthn.ts +++ b/src/controllers/webauthn.ts @@ -17,6 +17,7 @@ import { Request, Response } from 'express'; import { isBreakGlassAdmin } from '../config/breakGlass.js'; import { getSystemConfig } from '../config/getSystemConfig.js'; import { classifyAttestation } from '../lib/attestationType.js'; +import { passkeyDeviceBinding } from '../lib/passkeyAssurance.js'; import { SUPPORTED_ALGORITHM_IDS } from '../lib/webauthnAlgorithms.js'; import { buildPrfAuthenticationExtensions, @@ -626,7 +627,22 @@ const verifyWebAuthn = async (req: Request, res: Response) => { return res.status(500).json({ error: 'Internal server error' }); } - if (verification.verified) { + const binding = verification.verified + ? passkeyDeviceBinding(cred, verification.authenticationInfo.credentialDeviceType) + : null; + + if (verification.verified && !binding) { + await AuthEventService.log({ + userId: user.id, + type: 'webauthn_login_failed', + req, + metadata: { reason: 'Backup eligibility differs from registration' }, + }); + + return res.status(401).json({ error: 'Authentication failed.' }); + } + + if (verification.verified && binding) { await cred.update({ lastUsedAt: new Date(), counter: verification.authenticationInfo.newCounter, @@ -644,7 +660,7 @@ const verifyWebAuthn = async (req: Request, res: Response) => { await issueSessionAndRespond({ method: 'passkey', passkey: { - deviceBound: verification.authenticationInfo.credentialDeviceType === 'singleDevice', + deviceBound: binding.deviceBound, userVerified: verification.authenticationInfo.userVerified, }, user: { diff --git a/src/lib/passkeyAssurance.ts b/src/lib/passkeyAssurance.ts new file mode 100644 index 00000000..462dceda --- /dev/null +++ b/src/lib/passkeyAssurance.ts @@ -0,0 +1,38 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the Apache License, Version 2.0 + * See LICENSE file in the project root for full license information + */ + +import type { CredentialDeviceType } from '@simplewebauthn/server'; + +interface RegisteredCredential { + deviceType: CredentialDeviceType | null; + attestationVerified: boolean | null; +} + +/** + * The hardware assurance a passkey assertion earns, or null when the assertion's backup + * eligibility differs from the one registered. + * + * WebAuthn Level 3 fixes the backup eligibility flag at registration, so an assertion that + * reports another value did not come from the authenticator that registered. The flag is + * self-reported under `none` attestation, so only a registration whose attestation was + * verified against metadata can claim a device-bound key. + * + * A credential registered before the device type was stored has nothing to compare with, + * and earns no hardware assurance. + */ +export function passkeyDeviceBinding( + credential: RegisteredCredential, + reported: CredentialDeviceType, +): { deviceBound: boolean } | null { + if (credential.deviceType && credential.deviceType !== reported) { + return null; + } + + return { + deviceBound: + credential.deviceType === 'singleDevice' && credential.attestationVerified === true, + }; +} diff --git a/tests/integration/breakGlass/breakGlass.spec.ts b/tests/integration/breakGlass/breakGlass.spec.ts index 2b515c95..17b34761 100644 --- a/tests/integration/breakGlass/breakGlass.spec.ts +++ b/tests/integration/breakGlass/breakGlass.spec.ts @@ -122,7 +122,7 @@ async function passkeyAssertionVerifies() { (verifyAuthenticationResponse as any).mockResolvedValue({ verified: true, - authenticationInfo: { newCounter: 1 }, + authenticationInfo: { newCounter: 1, credentialDeviceType: 'singleDevice' }, }); } diff --git a/tests/integration/webauthn/webauthn.spec.ts b/tests/integration/webauthn/webauthn.spec.ts index 7882fdca..5c1af625 100644 --- a/tests/integration/webauthn/webauthn.spec.ts +++ b/tests/integration/webauthn/webauthn.spec.ts @@ -585,7 +585,7 @@ describe('challenge is spent on every login outcome', () => { const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({ verified: true, - authenticationInfo: { newCounter: 1 }, + authenticationInfo: { newCounter: 1, credentialDeviceType: 'singleDevice' }, }); await request(app) @@ -1260,7 +1260,7 @@ describe('POST /webauthn/login/finish', () => { (verifyAuthenticationResponse as any).mockResolvedValue({ verified: true, - authenticationInfo: { newCounter: 1 }, + authenticationInfo: { newCounter: 1, credentialDeviceType: 'singleDevice' }, id: 'cred-1', }); @@ -1405,7 +1405,7 @@ describe('POST /webauthn/login/finish', () => { const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({ verified: false, - authenticationInfo: { newCounter: 1 }, + authenticationInfo: { newCounter: 1, credentialDeviceType: 'singleDevice' }, }); const res = buildRes(); @@ -1440,7 +1440,7 @@ describe('POST /webauthn/login/finish', () => { const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({ verified: true, - authenticationInfo: { newCounter: 3 }, + authenticationInfo: { newCounter: 3, credentialDeviceType: 'singleDevice' }, }); const res = buildRes(); diff --git a/tests/unit/controllers/stepUp.spec.ts b/tests/unit/controllers/stepUp.spec.ts index 46a756c7..203a6cf0 100644 --- a/tests/unit/controllers/stepUp.spec.ts +++ b/tests/unit/controllers/stepUp.spec.ts @@ -121,7 +121,7 @@ describe('step-up controller', () => { const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({ verified: true, - authenticationInfo: { newCounter: 2 }, + authenticationInfo: { newCounter: 2, credentialDeviceType: 'singleDevice' }, }); const req = buildReq({ @@ -155,7 +155,11 @@ describe('step-up controller', () => { it('records the passkey step-up in the session auth context with its device binding', async () => { const user = buildUser(); - const credential = buildCredential({ id: 'cred-1', userId: user.id }); + const credential = buildCredential({ + id: 'cred-1', + userId: user.id, + attestationVerified: true, + }); const session = buildSession({ authMethods: [] }); (Credential.findOne as any).mockResolvedValue(credential); @@ -199,7 +203,7 @@ describe('step-up controller', () => { const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({ verified: false, - authenticationInfo: { newCounter: 2 }, + authenticationInfo: { newCounter: 2, credentialDeviceType: 'singleDevice' }, }); const req = buildReq({ @@ -405,7 +409,7 @@ describe('step-up controller', () => { const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({ verified: true, - authenticationInfo: { newCounter: 2 }, + authenticationInfo: { newCounter: 2, credentialDeviceType: 'singleDevice' }, }); const req = buildReq({ user, body: { assertionResponse: { id: 'cred-1' } } }); diff --git a/tests/unit/lib/passkeyAssurance.spec.ts b/tests/unit/lib/passkeyAssurance.spec.ts new file mode 100644 index 00000000..a8ff32f6 --- /dev/null +++ b/tests/unit/lib/passkeyAssurance.spec.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from 'vitest'; + +import { passkeyDeviceBinding } from '../../../src/lib/passkeyAssurance.js'; + +describe('passkeyDeviceBinding', () => { + it('awards hardware assurance only to an attested device-bound registration', () => { + expect( + passkeyDeviceBinding( + { deviceType: 'singleDevice', attestationVerified: true }, + 'singleDevice', + ), + ).toEqual({ deviceBound: true }); + expect( + passkeyDeviceBinding( + { deviceType: 'singleDevice', attestationVerified: null }, + 'singleDevice', + ), + ).toEqual({ deviceBound: false }); + expect( + passkeyDeviceBinding({ deviceType: 'multiDevice', attestationVerified: true }, 'multiDevice'), + ).toEqual({ deviceBound: false }); + }); + + it('refuses a flag that differs from registration in either direction', () => { + expect( + passkeyDeviceBinding( + { deviceType: 'multiDevice', attestationVerified: true }, + 'singleDevice', + ), + ).toBeNull(); + expect( + passkeyDeviceBinding( + { deviceType: 'singleDevice', attestationVerified: true }, + 'multiDevice', + ), + ).toBeNull(); + }); + + it('gives a credential registered before the device type was stored no hardware assurance', () => { + expect( + passkeyDeviceBinding({ deviceType: null, attestationVerified: null }, 'singleDevice'), + ).toEqual({ deviceBound: false }); + }); +}); From 917116d6f2abce254df12e8694343209079afc01 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:29:49 -0500 Subject: [PATCH 03/13] test(notices): reproduce case variants of a user id escaping the per-user limit Refs #553 --- .../integration/applications/accountNotices.spec.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/integration/applications/accountNotices.spec.ts b/tests/integration/applications/accountNotices.spec.ts index 40761f0b..44cad4ff 100644 --- a/tests/integration/applications/accountNotices.spec.ts +++ b/tests/integration/applications/accountNotices.spec.ts @@ -355,6 +355,19 @@ describe('POST /applications/account-notices', () => { expect(limited.body).toEqual({ error: 'notice_rate_limited' }); }); + it('counts case variants of one user id against the same allowance', async () => { + const userId = 'a1b2c3d4-e5f6-4a7b-8c9d-0e1f2a3b4c5d'; + const bearer = await token(); + + for (let attempt = 0; attempt < 10; attempt += 1) { + await send({ ...notice, userId }, bearer); + } + + const variant = await send({ ...notice, userId: userId.toUpperCase() }, bearer); + expect(variant.status).toBe(429); + expect(variant.body).toEqual({ error: 'notice_rate_limited' }); + }); + it('limits every notice from one client', async () => { vi.mocked(OidcClient.findByPk).mockResolvedValue(client({ id: 'busy' }) as never); const bearer = await token({ client_id: 'busy', sub: 'busy' }); From eef6591310b54739324ecf2bcd1822878c658078 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:35:02 -0500 Subject: [PATCH 04/13] fix(notices): key the per-user notice limit on the lower-cased user id The schema accepts upper-case UUIDs and Postgres matches them case-insensitively, so case variants of one id each got a bucket of their own. The multi-instance note in production operations now names the notice limits, which stay per instance until a shared store exists. Refs #553 --- .changeset/notice-limit-canonical-user.md | 5 +++++ docs/production-operations.md | 10 +++++----- src/middleware/applicationNoticeRateLimit.ts | 10 +++++++++- 3 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 .changeset/notice-limit-canonical-user.md diff --git a/.changeset/notice-limit-canonical-user.md b/.changeset/notice-limit-canonical-user.md new file mode 100644 index 00000000..d54cefd0 --- /dev/null +++ b/.changeset/notice-limit-canonical-user.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +The per-user limit on `POST /applications/account-notices` now folds the `userId` to lower case before counting (#553). Upper-case variants of one UUID no longer each get their own allowance of 10 notices an hour. diff --git a/docs/production-operations.md b/docs/production-operations.md index 715bdf6d..74d102c3 100644 --- a/docs/production-operations.md +++ b/docs/production-operations.md @@ -220,11 +220,11 @@ Several instances behind a load balancer share all of it. Some state is held in each process, and with more than one instance it behaves differently: -| State | Where | With N instances | -| --------------------------------- | ----------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Rate limit and slow-down counters | `express-rate-limit` and `express-slow-down` in-memory stores ([`src/middleware`](../src/middleware)) | Each instance counts on its own, so a client whose requests are spread across instances gets up to N times the configured budget. Route by client address at the load balancer, or enforce a limit at the proxy as well. | -| `system_config` cache | Five minutes per process ([`getSystemConfig.ts`](../src/config/getSystemConfig.ts)) | A config write clears the cache only on the instance that served it. The others keep the old values (roles, origins, login methods, lockout policy, token lifetimes) for up to five minutes. Restart the instances when a change must apply at once. | -| Audit degraded flag | Five minutes after a failed audit write ([`auditHealth.ts`](../src/services/auditHealth.ts)) | `/health/status` reports `degraded` only on the instance whose write failed. Monitor every instance, not one. | +| State | Where | With N instances | +| --------------------------------- | ----------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Rate limit and slow-down counters | `express-rate-limit` and `express-slow-down` in-memory stores ([`src/middleware`](../src/middleware)) | Each instance counts on its own, so a client whose requests are spread across instances gets up to N times the configured budget. Route by client address at the load balancer, or enforce a limit at the proxy as well. The account notice limits on `POST /applications/account-notices` are keyed by application and user rather than address, so an application can send up to N times its notice allowance however requests are routed. | +| `system_config` cache | Five minutes per process ([`getSystemConfig.ts`](../src/config/getSystemConfig.ts)) | A config write clears the cache only on the instance that served it. The others keep the old values (roles, origins, login methods, lockout policy, token lifetimes) for up to five minutes. Restart the instances when a change must apply at once. | +| Audit degraded flag | Five minutes after a failed audit write ([`auditHealth.ts`](../src/services/auditHealth.ts)) | `/health/status` reports `degraded` only on the instance whose write failed. Monitor every instance, not one. | ## Redaction diff --git a/src/middleware/applicationNoticeRateLimit.ts b/src/middleware/applicationNoticeRateLimit.ts index 6417f8c2..c8c2a9b9 100644 --- a/src/middleware/applicationNoticeRateLimit.ts +++ b/src/middleware/applicationNoticeRateLimit.ts @@ -34,9 +34,17 @@ function limiter(limit: number, key: (req: Request) => string) { export const applicationNoticeClientLimiter = limiter(NOTICES_PER_CLIENT_PER_HOUR, clientKey); +// The schema accepts an upper-case UUID and Postgres matches it case-insensitively, so the +// id is folded here or each case variant would get an allowance of its own. +function userKey(req: Request) { + const userId = (req.body as { userId?: unknown })?.userId; + + return typeof userId === 'string' ? userId.toLowerCase() : String(userId); +} + // Per user within one client, so one application cannot spend another's allowance for the // same user. export const applicationNoticeUserLimiter = limiter( NOTICES_PER_USER_PER_CLIENT_PER_HOUR, - (req) => `${clientKey(req)}:${String((req.body as { userId?: unknown })?.userId)}`, + (req) => `${clientKey(req)}:${userKey(req)}`, ); From 9e276480057ec5392c81d3a73c2b648b8f3dda44 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:36:14 -0500 Subject: [PATCH 05/13] test(notices): reproduce links and forged attribution in application notice text Refs #554 --- .../applications/accountNotices.spec.ts | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/integration/applications/accountNotices.spec.ts b/tests/integration/applications/accountNotices.spec.ts index 44cad4ff..052e7607 100644 --- a/tests/integration/applications/accountNotices.spec.ts +++ b/tests/integration/applications/accountNotices.spec.ts @@ -210,6 +210,32 @@ describe('POST /applications/account-notices', () => { ); }); + it.each([ + 'Sign in again at https://evil.example/login to keep your account.', + 'Sign in again at http://evil.example to keep your account.', + 'Sign in again at www.evil.example to keep your account.', + 'Sign in again at evil.example/login to keep your account.', + 'Sign in again at login.evil-example.com to keep your account.', + 'Sign in again at HTTPS://EVIL.EXAMPLE to keep your account.', + ])('refuses a link in the notice text, which only actionUrl may carry (%s)', async (text) => { + const res = await send({ ...notice, text }); + + expect(res.status).toBe(400); + expect(sendNoticeEmail).not.toHaveBeenCalled(); + expect(sendNoticeSms).not.toHaveBeenCalled(); + }); + + it('puts the attribution first, where the text cannot precede it', async () => { + const res = await send({ + ...notice, + text: 'A recovery opened.\n\n--\nSent by Acme ID Security through Acme ID.', + }); + + expect(res.status).toBe(200); + const [, , text] = vi.mocked(sendNoticeEmail).mock.calls[0]; + expect(text.split('\n')[0]).toBe('Sent by Locksley through Acme ID.'); + }); + it.each([ 'https://evil.example/devices', 'https://app.locksley.example:8443/devices', From 51fbc97ddc85573174e3c1538ebffb749ba352ad Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:37:05 -0500 Subject: [PATCH 06/13] fix(notices): keep application notice text from carrying links and attribution Notice text that contains a URL, a www address, a domain name or an email address is refused, so actionUrl stays the only link. The attribution moves to the first line, where the text cannot precede it with an imitation. Refs #554 --- .changeset/notice-text-without-links.md | 5 +++++ docs/adr/0001-openid-provider.md | 4 ++-- resources/coverage-badge.svg | 8 ++++---- src/schemas/applicationNotices.requests.ts | 17 +++++++++++++++++ src/services/applicationNoticeService.ts | 3 ++- .../applications/accountNotices.spec.ts | 13 +++++++++++-- 6 files changed, 41 insertions(+), 9 deletions(-) create mode 100644 .changeset/notice-text-without-links.md diff --git a/.changeset/notice-text-without-links.md b/.changeset/notice-text-without-links.md new file mode 100644 index 00000000..c3ceab01 --- /dev/null +++ b/.changeset/notice-text-without-links.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +Application notice text can no longer carry its own links (#554). `POST /applications/account-notices` refuses with `400` a `text` that contains a URL, a `www.` address, a domain name or an email address, since mail and SMS clients turn all of them into links; `actionUrl`, checked against the application's origins, is the only link a notice carries. The `Sent by through .` attribution is now the first line of the message, ahead of the text, rather than a footer the text could imitate. Adapters that render notices from external delivery receive the new layout in `text`. diff --git a/docs/adr/0001-openid-provider.md b/docs/adr/0001-openid-provider.md index 39b95fff..7cd077cc 100644 --- a/docs/adr/0001-openid-provider.md +++ b/docs/adr/0001-openid-provider.md @@ -315,8 +315,8 @@ The notices endpoint, `POST /applications/account-notices`, stays on the private - It goes only to a user who already holds a grant with that application, or with an application it is linked to (amended 2026-11-02, below). - The application must have notices enabled by an admin (`oidc_clients`). -- It is plain text only, inside a fixed template the application cannot change: "Sent by through ". It cannot pose as a Seamless security notice. -- An optional action link must be on one of the application's `https` redirect origins or its `noticeActionOrigins`. +- It is plain text only, inside a fixed template the application cannot change: "Sent by through " as the first line, ahead of the text, so the text cannot imitate it (amended 2026-11-02, #554). It cannot pose as a Seamless security notice. +- An optional action link must be on one of the application's `https` redirect origins or its `noticeActionOrigins`. It is the only link: text that contains a URL, a domain name or an email address is refused with `400`, since mail and SMS clients link those too (#554). - Rate limits apply per client and per user, and every notice writes an auth event. - Delivery uses `src/services/accountNotices.ts`, including external delivery (`account_notice_email`, `account_notice_sms`). - types#120 defines the request and response. The response lists channels, never addresses. diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index c931b02b..fdefd25b 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 99.2% + + coverage: 99.3% @@ -17,7 +17,7 @@ coverage coverage - 99.2% - 99.2% + 99.3% + 99.3% diff --git a/src/schemas/applicationNotices.requests.ts b/src/schemas/applicationNotices.requests.ts index 3d8730dd..29439969 100644 --- a/src/schemas/applicationNotices.requests.ts +++ b/src/schemas/applicationNotices.requests.ts @@ -7,8 +7,25 @@ import { SendApplicationNoticeRequestSchema as BaseRequestSchema } from '@seamless-auth/types'; import { z } from 'zod'; +// Mail and SMS clients turn a bare domain or an email address into a link as readily as a +// URL, so any of them is refused and `actionUrl`, checked against the client's origins, +// stays the only link a notice carries. Ideographic and fullwidth full stops count as dots +// because some clients link them too. +const SCHEME = /[\p{L}][\p{L}\p{N}+.-]*:\/\//iu; +const WWW = /(? !noticeTextHasLink(text), + 'Notice text must not contain links, domain names or email addresses. Put the one link in actionUrl.', + ), /** A client the caller is linked to, to send for. The caller itself when omitted. */ clientId: z.string().min(1).max(255).optional(), }); diff --git a/src/services/applicationNoticeService.ts b/src/services/applicationNoticeService.ts index b6a9a270..a5bd9fda 100644 --- a/src/services/applicationNoticeService.ts +++ b/src/services/applicationNoticeService.ts @@ -59,6 +59,7 @@ export function actionUrlAllowed(client: OidcClient, actionUrl: string) { /** * The fixed template (ADR 0001, D14). The application supplies only the subject line and * the body; the attribution is the API's, so a notice cannot read as one from Seamless. + * The attribution is the first line, since the text could imitate one placed after it. */ export function applicationNotice( client: OidcClient, @@ -76,7 +77,7 @@ export function applicationNotice( return { name: 'application_notice', subject: `[${application}] ${singleLine(subject)}`, - text: `${body}\n\n--\nSent by ${application} through ${instance}.`, + text: `Sent by ${application} through ${instance}.\n\n${body}`, application: { clientId: client.id, name: client.name }, }; } diff --git a/tests/integration/applications/accountNotices.spec.ts b/tests/integration/applications/accountNotices.spec.ts index 052e7607..b1348151 100644 --- a/tests/integration/applications/accountNotices.spec.ts +++ b/tests/integration/applications/accountNotices.spec.ts @@ -114,7 +114,7 @@ describe('POST /applications/account-notices', () => { expect(sendNoticeEmail).toHaveBeenCalledWith( 'test@example.com', '[Locksley] Device recovery started', - 'A recovery opened.\n\nhttps://app.locksley.example/devices\n\n--\nSent by Locksley through Acme ID.', + 'Sent by Locksley through Acme ID.\n\nA recovery opened.\n\nhttps://app.locksley.example/devices', ); expect(sendNoticeSms).toHaveBeenCalledWith('+14155552671', expect.stringContaining('Locksley')); expect(JSON.stringify(res.body)).not.toContain('test@example.com'); @@ -217,6 +217,8 @@ describe('POST /applications/account-notices', () => { 'Sign in again at evil.example/login to keep your account.', 'Sign in again at login.evil-example.com to keep your account.', 'Sign in again at HTTPS://EVIL.EXAMPLE to keep your account.', + 'Write to help@evil.example to keep your account.', + 'Sign in again at evil\uff0eexample to keep your account.', ])('refuses a link in the notice text, which only actionUrl may carry (%s)', async (text) => { const res = await send({ ...notice, text }); @@ -225,6 +227,13 @@ describe('POST /applications/account-notices', () => { expect(sendNoticeSms).not.toHaveBeenCalled(); }); + it.each([ + 'Version 2.4.1 is ready, e.g. on your desktop.', + 'Your order 3.5 shipped. Reply STOP to opt out.', + ])('sends plain text that names no address (%s)', async (text) => { + expect((await send({ ...notice, text })).status).toBe(200); + }); + it('puts the attribution first, where the text cannot precede it', async () => { const res = await send({ ...notice, @@ -468,7 +477,7 @@ describe('POST /applications/account-notices for a linked client', () => { expect(sendNoticeEmail).toHaveBeenCalledWith( 'test@example.com', '[Locksley Desktop] Device recovery started', - 'A recovery opened.\n\nhttps://store.locksley.example/recoveries/cancel\n\n--\nSent by Locksley Desktop through Acme ID.', + 'Sent by Locksley Desktop through Acme ID.\n\nA recovery opened.\n\nhttps://store.locksley.example/recoveries/cancel', ); expect(OidcPayload.findOne).toHaveBeenCalledWith({ where: expect.objectContaining({ From eda1e13c9891b5221c7fa91fc28f694deb0e9cc5 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:38:43 -0500 Subject: [PATCH 07/13] test(oidc): reproduce third-party clients served with third_party_clients_enabled off Refs #555 --- .../unit/services/oidcThirdPartyGate.spec.ts | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 tests/unit/services/oidcThirdPartyGate.spec.ts diff --git a/tests/unit/services/oidcThirdPartyGate.spec.ts b/tests/unit/services/oidcThirdPartyGate.spec.ts new file mode 100644 index 00000000..fe6dc898 --- /dev/null +++ b/tests/unit/services/oidcThirdPartyGate.spec.ts @@ -0,0 +1,136 @@ +import { generateKeyPairSync } from 'crypto'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../../src/models/oidcClients', () => ({ + OidcClient: { findByPk: vi.fn() }, +})); + +vi.mock('../../../src/models/oidcClientResources', () => ({ + OidcClientResource: { findAll: vi.fn(async () => []) }, +})); + +vi.mock('../../../src/models/oidcResources', () => ({ + OidcResource: { findAll: vi.fn(async () => []) }, +})); + +vi.mock('../../../src/services/oidcProviderKeys', async (importOriginal) => ({ + ...(await importOriginal()), + getActiveOidcSigningKey: vi.fn(), +})); + +import { getSystemConfig } from '../../../src/config/getSystemConfig'; +import { OidcClient } from '../../../src/models/oidcClients'; +import { + createOidcClientSecretLookup, + findActiveOidcClient, + getOidcClientMetadata, + verifyOidcClientSecret, +} from '../../../src/services/oidcClientRegistry'; +import { getActiveOidcSigningKey } from '../../../src/services/oidcProviderKeys'; +import { signForOidcEdge } from '../../../src/services/oidcSigning'; + +// ADR 0001, D10: third_party_clients_enabled gates third-party clients entirely, so turning +// it off must stop the API serving and signing for the ones that already exist. + +const ISSUER = 'https://login.example.com'; +const { privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 }); + +function client(overrides: Record = {}) { + return { + id: 'svc', + name: 'Partner', + firstParty: false, + clientType: 'confidential', + tokenEndpointAuthMethod: 'client_secret_basic', + grantTypes: ['authorization_code', 'client_credentials'], + redirectUris: ['https://partner.example/cb'], + postLogoutRedirectUris: [], + jwks: null, + jwksUri: null, + backchannelLogoutUri: null, + secretLookup: createOidcClientSecretLookup('s3cret'), + noticesEnabled: true, + disabledAt: null, + ...overrides, + }; +} + +function config(thirdParty: boolean) { + vi.mocked(getSystemConfig).mockResolvedValue({ + oidc_issuer: ISSUER, + allow_client_secrets: true, + third_party_clients_enabled: thirdParty, + } as never); +} + +function b64(value: unknown) { + return Buffer.from(JSON.stringify(value)).toString('base64url'); +} + +const clientCredentials = () => ({ + kind: 'client_credentials' as const, + kid: 'k1', + clientId: 'svc', + signingInput: `${b64({ alg: 'RS256', kid: 'k1', typ: 'at+jwt' })}.${b64({ + iss: ISSUER, + sub: 'svc', + client_id: 'svc', + aud: `${ISSUER}/seamless`, + scope: 'notices:send', + exp: Math.floor(Date.now() / 1000) + 600, + })}`, +}); + +beforeEach(() => { + vi.mocked(getActiveOidcSigningKey).mockReturnValue(privateKey); +}); + +describe('third_party_clients_enabled off', () => { + beforeEach(() => { + config(false); + vi.mocked(OidcClient.findByPk).mockResolvedValue(client() as never); + }); + + it('treats an existing third-party client as inactive', async () => { + expect(await findActiveOidcClient('svc')).toBeNull(); + }); + + it('does not serve its metadata to the edge', async () => { + expect(await getOidcClientMetadata('svc')).toBeNull(); + }); + + it('does not verify its secret', async () => { + expect(await verifyOidcClientSecret('svc', 's3cret')).toBe(false); + }); + + it('does not sign for it', async () => { + expect(await signForOidcEdge(clientCredentials())).toEqual({ + ok: false, + code: 'client_inactive', + }); + }); + + it('still serves a first-party client', async () => { + vi.mocked(OidcClient.findByPk).mockResolvedValue(client({ firstParty: true }) as never); + + expect(await findActiveOidcClient('svc')).not.toBeNull(); + expect(await getOidcClientMetadata('svc')).toEqual( + expect.objectContaining({ client_id: 'svc' }), + ); + }); +}); + +describe('third_party_clients_enabled on', () => { + beforeEach(() => { + config(true); + vi.mocked(OidcClient.findByPk).mockResolvedValue(client() as never); + }); + + it('serves and signs for a third-party client', async () => { + expect(await findActiveOidcClient('svc')).not.toBeNull(); + expect(await verifyOidcClientSecret('svc', 's3cret')).toBe(true); + expect(await signForOidcEdge(clientCredentials())).toEqual( + expect.objectContaining({ ok: true }), + ); + }); +}); From c4fadf74a25dc5822c4493e546436e21a5af4769 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:39:58 -0500 Subject: [PATCH 08/13] fix(oidc): enforce third_party_clients_enabled for existing clients findActiveOidcClient now treats a client that is not first-party as inactive while third_party_clients_enabled is off, so client metadata, secret checks, /oidc/sign, notice tokens and back-channel logout all fail closed, the same pattern verifyOidcClientSecret uses for allow_client_secrets. Refs #555 --- .changeset/third-party-clients-gate.md | 5 +++++ docs/adr/0001-openid-provider.md | 2 +- docs/configuration.md | 20 +++++++++---------- src/services/oidcApplicationService.ts | 4 ++-- src/services/oidcClientRegistry.ts | 10 +++++++++- .../applications/accountNotices.spec.ts | 9 +++++++++ .../unit/services/oidcClientRegistry.spec.ts | 1 + 7 files changed, 37 insertions(+), 14 deletions(-) create mode 100644 .changeset/third-party-clients-gate.md diff --git a/.changeset/third-party-clients-gate.md b/.changeset/third-party-clients-gate.md new file mode 100644 index 00000000..d76e1daf --- /dev/null +++ b/.changeset/third-party-clients-gate.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +`third_party_clients_enabled` now gates existing third-party OpenID Provider clients, not only creating and updating them (#555). While it is off, a client that is not first-party is treated like a disabled one: `/oidc/clients/:id` answers `404`, its shared secret does not verify, `/oidc/sign` refuses with `client_inactive`, its notice tokens are refused and back-channel logout skips it. An instance that turned the flag off after registering third-party clients stops serving them on upgrade; turn the flag back on, or mark the applications first-party, to keep them working. diff --git a/docs/adr/0001-openid-provider.md b/docs/adr/0001-openid-provider.md index 7cd077cc..969e75e7 100644 --- a/docs/adr/0001-openid-provider.md +++ b/docs/adr/0001-openid-provider.md @@ -249,7 +249,7 @@ A passkey resists phishing because it is bound to an origin. Two flows can still - A client is first-party only if an admin marks it so. - Third-party clients always show the consent page, with their redirect domain. -- An instance-wide `third_party_clients_enabled` flag, off by default, gates them entirely. +- An instance-wide `third_party_clients_enabled` flag, off by default, gates them entirely. Turning it off makes existing third-party clients inactive in the API, as a disabled client is (#555). ## D11. Compatibility and token separation diff --git a/docs/configuration.md b/docs/configuration.md index d88b1ece..8a424286 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -364,16 +364,16 @@ See [docs/production-operations.md](./production-operations.md) for key rotation The API's side of the OpenID Provider service ([ADR 0001](./adr/0001-openid-provider.md)). All of it is off until a deployment runs the provider service and turns it on. -| Variable | Required | Default | Notes | -| ---------------------------------- | ------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `OIDC_PROVIDER_ENABLED` | No | `false` | Seeds `oidc_provider_enabled`. While off, every `/oidc/*` route answers 404. The server refuses to start with it on unless the checks below pass. | -| `OIDC_ISSUER` | When enabled | - | Seeds `oidc_issuer`, the provider service's public URL. Must be https, must differ from `ISSUER`, and its host must be `rpid` or a subdomain of it. | -| `OIDC_ALLOW_CLIENT_SECRETS` | No | `false` | Seeds `allow_client_secrets`. Lets an admin opt individual provider clients in to shared secrets. `private_key_jwt` is the default without it. | -| `OIDC_THIRD_PARTY_CLIENTS_ENABLED` | No | `false` | Seeds `third_party_clients_enabled`. Third-party clients always show a consent page. | -| `SEAMLESS_OIDC_SERVICE_TOKEN` | When enabled | - | The provider service's own service token secret. Must differ from `SEAMLESS_SERVICE_TOKEN`, or the server refuses to start. `/oidc/*` accepts only this, and external delivery refuses it. | -| `OIDC_JWKS_ACTIVE_KID` | When enabled | - | Key ID of the provider signing key. Only this key signs. | -| `OIDC_JWKS_KEYS` | When enabled | - | JSON JWK set of private RS256 keys, each with a unique `kid`. Separate from the `SEAMLESS_JWKS_*` keys. Every key in it is published at `/oidc/jwks`. | -| `OIDC_HOSTED_UI` | No | - | Seeds `hosted_ui` as JSON: branding for the hosted pages (`appName`, an https `logoUrl`, hex `colors`, `fontFamily`). Admins change it through `PATCH /system-config/admin`. | +| Variable | Required | Default | Notes | +| ---------------------------------- | ------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `OIDC_PROVIDER_ENABLED` | No | `false` | Seeds `oidc_provider_enabled`. While off, every `/oidc/*` route answers 404. The server refuses to start with it on unless the checks below pass. | +| `OIDC_ISSUER` | When enabled | - | Seeds `oidc_issuer`, the provider service's public URL. Must be https, must differ from `ISSUER`, and its host must be `rpid` or a subdomain of it. | +| `OIDC_ALLOW_CLIENT_SECRETS` | No | `false` | Seeds `allow_client_secrets`. Lets an admin opt individual provider clients in to shared secrets. `private_key_jwt` is the default without it. | +| `OIDC_THIRD_PARTY_CLIENTS_ENABLED` | No | `false` | Seeds `third_party_clients_enabled`. Third-party clients always show a consent page. While it is off, existing third-party clients are inactive: the API does not serve them to the edge, verify their secrets, sign for them or accept their notice tokens. | +| `SEAMLESS_OIDC_SERVICE_TOKEN` | When enabled | - | The provider service's own service token secret. Must differ from `SEAMLESS_SERVICE_TOKEN`, or the server refuses to start. `/oidc/*` accepts only this, and external delivery refuses it. | +| `OIDC_JWKS_ACTIVE_KID` | When enabled | - | Key ID of the provider signing key. Only this key signs. | +| `OIDC_JWKS_KEYS` | When enabled | - | JSON JWK set of private RS256 keys, each with a unique `kid`. Separate from the `SEAMLESS_JWKS_*` keys. Every key in it is published at `/oidc/jwks`. | +| `OIDC_HOSTED_UI` | No | - | Seeds `hosted_ui` as JSON: branding for the hosted pages (`appName`, an https `logoUrl`, hex `colors`, `fontFamily`). Admins change it through `PATCH /system-config/admin`. | The issuer has to sit under `rpid` because the hosted sign-in pages run on it and passkeys are bound to `rpid`. Changing `rpid` to fit a new hostname orphans every existing passkey, so pick diff --git a/src/services/oidcApplicationService.ts b/src/services/oidcApplicationService.ts index 11f9fcde..c4bb608a 100644 --- a/src/services/oidcApplicationService.ts +++ b/src/services/oidcApplicationService.ts @@ -115,8 +115,8 @@ function storedGrantTypes(grants: string[], deviceFlowEnabled: boolean) { /** * ADR 0001, D6 and D10. The schema cannot see instance config, so these are checked * here, and only for settings the request names: an instance that later turns a flag off - * does not lock every existing client out of being renamed. The edge enforces both flags - * at sign-in either way. + * does not lock every existing client out of being renamed. Serving, secret checks and + * signing apply both flags to existing clients either way (`oidcClientRegistry.ts`). */ async function assertInstanceAllows(settings: { firstParty?: boolean | undefined; diff --git a/src/services/oidcClientRegistry.ts b/src/services/oidcClientRegistry.ts index a4c4f13b..e9f3db29 100644 --- a/src/services/oidcClientRegistry.ts +++ b/src/services/oidcClientRegistry.ts @@ -37,10 +37,18 @@ export function createOidcClientSecretLookup(secret: string) { .digest('hex'); } +/** + * A disabled client, or a third-party one while the instance has third-party clients off + * (ADR 0001, D10), is inactive everywhere the edge or an application can reach it: serving, + * secret checks, signing, notices and back-channel logout. + */ export async function findActiveOidcClient(id: string) { const client = await OidcClient.findByPk(id); + if (!client || client.disabledAt) return null; + if (client.firstParty) return client; - return client && !client.disabledAt ? client : null; + const { third_party_clients_enabled } = await getSystemConfig(); + return third_party_clients_enabled ? client : null; } /** A loopback or private-use redirect (RFC 8252) makes the client a native one. */ diff --git a/tests/integration/applications/accountNotices.spec.ts b/tests/integration/applications/accountNotices.spec.ts index b1348151..72b17194 100644 --- a/tests/integration/applications/accountNotices.spec.ts +++ b/tests/integration/applications/accountNotices.spec.ts @@ -40,6 +40,7 @@ function client(overrides: Record = {}) { return { id: 'locksley', name: 'Locksley', + firstParty: true, grantTypes: ['authorization_code', 'client_credentials'], redirectUris: ['https://app.locksley.example/callback', 'http://127.0.0.1/callback'], noticesEnabled: true, @@ -539,6 +540,14 @@ describe('POST /applications/account-notices for a linked client', () => { ['moved across the first-party boundary', { firstParty: false }], ['became confidential', { clientType: 'confidential' }], ])('answers 404 once the linked client %s', async (_label, change) => { + // Third-party clients on, so the boundary check rather than the instance gate refuses. + vi.mocked(getSystemConfig).mockResolvedValue( + buildSystemConfig({ + oidc_issuer: ISSUER, + app_name: 'Acme ID', + third_party_clients_enabled: true, + }) as never, + ); registry(backend, { ...desktop, ...change }); const res = await send({ ...notice, clientId: 'locksley-desktop' }); diff --git a/tests/unit/services/oidcClientRegistry.spec.ts b/tests/unit/services/oidcClientRegistry.spec.ts index 0d6fef08..03e6a78c 100644 --- a/tests/unit/services/oidcClientRegistry.spec.ts +++ b/tests/unit/services/oidcClientRegistry.spec.ts @@ -143,6 +143,7 @@ describe('getOidcClientMetadata', () => { ); expect((await getOidcClientMetadata('app'))?.grant_types).toEqual(['refresh_token']); + vi.mocked(getSystemConfig).mockResolvedValue({ third_party_clients_enabled: true } as never); vi.mocked(OidcClient.findByPk).mockResolvedValue( client({ grantTypes, deviceFlowEnabled: true, firstParty: false }) as never, ); From 65ba6c11d7b2d270ca7a05c90f027c120b839f81 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:43:11 -0500 Subject: [PATCH 09/13] test(security): reproduce cacheable secrets, any-subject delivery and unguarded staging fetches Refs #556 --- .../authentication/authentication.spec.ts | 2 ++ .../integration/oidc/oidcApplications.spec.ts | 2 ++ tests/unit/lib/externalDelivery.spec.ts | 17 +++++++++++ tests/unit/lib/oauthProviderPolicy.spec.ts | 29 +++++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/tests/integration/authentication/authentication.spec.ts b/tests/integration/authentication/authentication.spec.ts index 81140445..8cf6420c 100644 --- a/tests/integration/authentication/authentication.spec.ts +++ b/tests/integration/authentication/authentication.spec.ts @@ -665,6 +665,8 @@ describe('POST /refresh', () => { const res = await request(app).post('/refresh').set('Authorization', 'Bearer refresh-token'); expect(res.status).toBe(200); + // A token response must never be stored by a cache between the adapter and the API. + expect(res.headers['cache-control']).toBe('no-store'); expect(signAccessToken).toHaveBeenCalledWith( 'new-session', expect.any(String), diff --git a/tests/integration/oidc/oidcApplications.spec.ts b/tests/integration/oidc/oidcApplications.spec.ts index 4a6e606f..abdaa25b 100644 --- a/tests/integration/oidc/oidcApplications.spec.ts +++ b/tests/integration/oidc/oidcApplications.spec.ts @@ -308,6 +308,7 @@ describe('POST /admin/applications', () => { }); expect(res.status).toBe(201); + expect(res.headers['cache-control']).toBe('no-store'); expect(res.body.clientSecret).toMatch(/^[A-Za-z0-9_-]{43}$/); expect(res.body.application.hasSecret).toBe(true); const stored = vi.mocked(OidcClient.create).mock.calls[0][0].secretLookup; @@ -735,6 +736,7 @@ describe('POST /admin/applications/:id/secret', () => { const res = await request(app).post('/admin/applications/app-1/secret'); expect(res.status).toBe(200); + expect(res.headers['cache-control']).toBe('no-store'); expect(existing.update).toHaveBeenCalledWith({ secretLookup: createOidcClientSecretLookup(res.body.clientSecret), }); diff --git a/tests/unit/lib/externalDelivery.spec.ts b/tests/unit/lib/externalDelivery.spec.ts index b793ed6f..e92f2d09 100644 --- a/tests/unit/lib/externalDelivery.spec.ts +++ b/tests/unit/lib/externalDelivery.spec.ts @@ -53,6 +53,23 @@ describe('external delivery gates', () => { ).resolves.toBe(false); }); + it('refuses a valid service token minted for something other than external delivery', async () => { + (validateInternalServiceToken as any).mockResolvedValue({ + sub: 'seamless-auth-express-adapter', + iss: 'seamless-portal-api', + aud: 'seamless-auth', + }); + + await expect( + canReturnExternalDelivery( + req({ + 'x-seamless-auth-delivery-mode': 'external', + 'x-seamless-service-token': 'Bearer proxy-token', + }), + ), + ).resolves.toBe(false); + }); + it('allows external delivery with a trusted service token outside production', async () => { (validateInternalServiceToken as any).mockResolvedValue({ sub: 'service', diff --git a/tests/unit/lib/oauthProviderPolicy.spec.ts b/tests/unit/lib/oauthProviderPolicy.spec.ts index 868d4716..a8310449 100644 --- a/tests/unit/lib/oauthProviderPolicy.spec.ts +++ b/tests/unit/lib/oauthProviderPolicy.spec.ts @@ -314,6 +314,35 @@ describe('providerFetch', () => { }); }); +describe('providerFetch outside production', () => { + const staging = { NODE_ENV: 'staging' }; + + it.each([ + 'http://169.254.169.254/latest/meta-data/', + 'http://[::ffff:169.254.169.254]/latest/meta-data/', + 'http://0.0.0.0:9000/token', + ])('never reaches a link-local or unspecified address (%s)', async (target) => { + const fetch = vi.fn().mockResolvedValue(new Response('{}')); + vi.stubGlobal('fetch', fetch); + + await expect(providerFetch(target, {}, staging)).rejects.toBeInstanceOf( + OAuthProviderPolicyError, + ); + expect(fetch).not.toHaveBeenCalled(); + }); + + it('never reaches a host that resolves to the metadata address', async () => { + lookup.mockResolvedValue([{ address: '169.254.169.254', family: 4 }]); + const fetch = vi.fn().mockResolvedValue(new Response('{}')); + vi.stubGlobal('fetch', fetch); + + await expect( + providerFetch('http://idp.staging.example/.well-known/openid-configuration', {}, staging), + ).rejects.toBeInstanceOf(OAuthProviderPolicyError); + expect(fetch).not.toHaveBeenCalled(); + }); +}); + describe('guardedRequest', () => { const allowLocal = { ...production, OAUTH_PROVIDER_PRIVATE_HOSTS: 'idp.corp.example' }; let port = 0; From b064241aa7f195d6842c3c12bd6a23af4e71b94c Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:53:09 -0500 Subject: [PATCH 10/13] fix(security): stop caching secret responses and guard provider fetches in every mode Every response now starts with Cache-Control: no-store, which routes that publish public material (JWKS, hashed dashboard assets) override. Provider requests use the guarded socket in every mode; outside production any host may be private, but link-local and unspecified addresses stay refused, whether named or resolved. The socket leg moves to providerTransport so the suite can keep answering provider calls from a stubbed fetch. Requiring the external delivery subject is left for a coordinated change with the seamless-cli conformance harness; its reproducing test is marked as an expected failure. Refs #556 --- ...n-secret-responses-and-provider-fetches.md | 5 +++ docs/oauth.md | 5 ++- docs/security-posture.md | 10 +++++ src/app.ts | 8 ++++ src/lib/oauthProviderPolicy.ts | 31 ++++++++------- src/lib/providerTransport.ts | 19 ++++++++++ tests/setup/mocks.ts | 9 +++++ tests/unit/lib/externalDelivery.spec.ts | 38 +++++++++++-------- tests/unit/lib/oauthProviderPolicy.spec.ts | 35 +++++++---------- 9 files changed, 108 insertions(+), 52 deletions(-) create mode 100644 .changeset/harden-secret-responses-and-provider-fetches.md create mode 100644 src/lib/providerTransport.ts diff --git a/.changeset/harden-secret-responses-and-provider-fetches.md b/.changeset/harden-secret-responses-and-provider-fetches.md new file mode 100644 index 00000000..78e2f65a --- /dev/null +++ b/.changeset/harden-secret-responses-and-provider-fetches.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +Two hardening fixes from #556. Every API response now carries `Cache-Control: no-store` unless its route sets its own caching (the JWKS and the admin dashboard's hashed assets do), so tokens, client secrets from create and rotate, recovery codes and delivery payloads are never stored by a cache. Provider requests (OAuth token, userinfo and discovery, and back-channel logout) now go through the address guard in every mode: outside production, `http` and private or loopback addresses are still allowed, but link-local `169.254.0.0/16` and unspecified `0.0.0.0/8` addresses are refused whether named directly or reached through DNS. diff --git a/docs/oauth.md b/docs/oauth.md index 2910aa44..4bc9e3cc 100644 --- a/docs/oauth.md +++ b/docs/oauth.md @@ -13,7 +13,10 @@ Provider access tokens are used only during callback handling to fetch profile d other secret the server holds. 4. In production, provider URLs must use `https` and resolve to public addresses, and the server does not follow redirects from them. For an on-premises IdP, list its host in `OAUTH_PROVIDER_PRIVATE_HOSTS`. - Link-local addresses are refused even when listed. + Link-local addresses are refused even when listed. Outside production, `http` and private or loopback + addresses are allowed so a local stack can reach its IdP, but link-local (`169.254.0.0/16`, which carries + cloud instance metadata) and unspecified (`0.0.0.0/8`) addresses are refused in every mode, whether + named directly or reached through DNS. 5. Set `OAUTH_STATE_SECRET` in production. Adding a provider answers `409 secret_required` until it is set, and the server refuses to start with a provider configured and no value. diff --git a/docs/security-posture.md b/docs/security-posture.md index 6bf15ade..5b20cd23 100644 --- a/docs/security-posture.md +++ b/docs/security-posture.md @@ -328,6 +328,16 @@ that were never looked up at all. `basic`. The format alone cannot separate the last two: a manufacturer-signed statement and one a credential signed for itself are both `packed`. +## Response caching + +**Posture: no API response is cacheable unless its route says so.** + +Responses carry access, refresh and ephemeral tokens, client secrets, recovery codes and +external delivery payloads, so every response starts with `Cache-Control: no-store`, and +a cache between the adapter and the API cannot keep one. Only routes that publish +public material opt back in: the JWKS (`public, max-age=300`) and the dashboard's +content-hashed assets. + ## Token audience **Posture: `aud` is the deployment's own issuer URL.** diff --git a/src/app.ts b/src/app.ts index 1f2b0189..b460c7db 100644 --- a/src/app.ts +++ b/src/app.ts @@ -122,6 +122,14 @@ if (process.env.NODE_ENV !== 'test') { app.use(dynamicRateLimit); } +// Responses carry tokens, client secrets, recovery codes and delivery payloads, so none is +// stored by a cache between the caller and the API unless its route says otherwise (JWKS, +// hashed dashboard assets). +app.use((_req, res, next) => { + res.setHeader('Cache-Control', 'no-store'); + next(); +}); + app.use(express.json()); app.use(cors(corsOptionsDelegate)); diff --git a/src/lib/oauthProviderPolicy.ts b/src/lib/oauthProviderPolicy.ts index fa09b5cc..1f1048c2 100644 --- a/src/lib/oauthProviderPolicy.ts +++ b/src/lib/oauthProviderPolicy.ts @@ -13,6 +13,7 @@ import { BlockList, isIP, type LookupFunction } from 'node:net'; import { missingOAuthProviderEndpoints } from '@seamless-auth/types'; import type { OAuthProviderConfig } from '../schemas/systemConfig.schema.js'; +import { sendProviderRequest } from './providerTransport.js'; type EnvSource = Record; @@ -123,6 +124,15 @@ function isProduction(env: EnvSource) { return env.NODE_ENV === 'production'; } +/** + * Outside production any host may be private, so a local stack can reach a provider on a + * private address. The never-reachable ranges stay closed in every mode, since a staging + * deployment runs on the same cloud metadata network as a production one. + */ +function hostMayBePrivate(hostname: string, env: EnvSource) { + return !isProduction(env) || privateHostsAllowed(env).has(hostname.toLowerCase()); +} + function privateHostsAllowed(env: EnvSource) { return new Set( (env[PRIVATE_PROVIDER_HOSTS_ENV] ?? '') @@ -204,14 +214,12 @@ export function assertProviderDestination(target: string | URL, env: EnvSource = throw new OAuthProviderPolicyError('OAuth provider URL must use https'); } - if (!isProduction(env)) return url; - - if (url.protocol !== 'https:') { + if (isProduction(env) && url.protocol !== 'https:') { throw new OAuthProviderPolicyError('OAuth provider URL must use https'); } const host = url.hostname.replace(/^\[|\]$/g, '').toLowerCase(); - if (isIP(unmapIPv4(host)) && !addressAllowed(host, privateHostsAllowed(env).has(host))) { + if (isIP(unmapIPv4(host)) && !addressAllowed(host, hostMayBePrivate(host, env))) { throw new OAuthProviderPolicyError(`OAuth provider host ${host} is a non-public address`); } @@ -233,8 +241,6 @@ type LookupCallback = ( * cannot answer one way for the check and another for the connection. */ export function guardedLookup(env: EnvSource = process.env): LookupFunction { - const allowlisted = privateHostsAllowed(env); - const guarded = ( hostname: string, options: { family?: number | string; all?: boolean }, @@ -249,7 +255,7 @@ export function guardedLookup(env: EnvSource = process.env): LookupFunction { lookup(hostname, { all: true, verbatim: true, family }).then( (addresses) => { - const allowed = allowlisted.has(hostname.toLowerCase()); + const allowed = hostMayBePrivate(hostname, env); if ( addresses.length === 0 || addresses.some((entry) => !addressAllowed(entry.address, allowed)) @@ -339,9 +345,10 @@ export function guardedRequest(url: URL, init: RequestInit, env: EnvSource): Pro /** * fetch for provider endpoints. Redirects are not followed, since a redirect would - * carry the request (and its credentials) to a host that was never checked. In - * production the request goes through `guardedLookup`; elsewhere it is plain fetch, so - * local stacks can reach a provider on a private address. + * carry the request (and its credentials) to a host that was never checked. Every + * request goes through `guardedLookup`, which outside production admits private + * addresses, so local stacks can reach a provider on one, but never link-local or + * unspecified ones. */ export async function providerFetch( target: string | URL, @@ -350,9 +357,7 @@ export async function providerFetch( ): Promise { const url = assertProviderDestination(target, env); - const response = isProduction(env) - ? await guardedRequest(url, init, env) - : await globalThis.fetch(target, { ...init, redirect: 'manual' }); + const response = await sendProviderRequest(url, init, env); if (response.status >= 300 && response.status < 400) { throw new OAuthProviderPolicyError( diff --git a/src/lib/providerTransport.ts b/src/lib/providerTransport.ts new file mode 100644 index 00000000..e9a53d5c --- /dev/null +++ b/src/lib/providerTransport.ts @@ -0,0 +1,19 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the Apache License, Version 2.0 + * See LICENSE file in the project root for full license information + */ + +import { guardedRequest } from './oauthProviderPolicy.js'; + +/** + * The network leg of `providerFetch`, in a module of its own so the test suite can stand + * in for the socket without replacing the destination checks around it. + */ +export function sendProviderRequest( + url: URL, + init: RequestInit, + env: Record, +): Promise { + return guardedRequest(url, init, env); +} diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index cbfdfb45..8901f795 100644 --- a/tests/setup/mocks.ts +++ b/tests/setup/mocks.ts @@ -293,6 +293,15 @@ vi.mock('../../src/utils/otp.js', async (importOriginal) => ({ verifyEmailOTP: vi.fn(), })); +// Provider requests go to the network through node:http(s) with an address guard. The +// suite answers them from a stubbed global fetch instead, the way the specs stub it, and +// tests/unit/lib/oauthProviderPolicy.spec.ts unmocks this to exercise the real socket. +vi.mock('../../src/lib/providerTransport.js', () => ({ + sendProviderRequest: vi.fn((url: URL, init: RequestInit) => + globalThis.fetch(url.href, { ...init, redirect: 'manual' }), + ), +})); + vi.mock('../../src/lib/token.js', () => ({ signEphemeralToken: vi.fn(), signAccessToken: vi.fn(), diff --git a/tests/unit/lib/externalDelivery.spec.ts b/tests/unit/lib/externalDelivery.spec.ts index e92f2d09..3bb97ea6 100644 --- a/tests/unit/lib/externalDelivery.spec.ts +++ b/tests/unit/lib/externalDelivery.spec.ts @@ -53,22 +53,28 @@ describe('external delivery gates', () => { ).resolves.toBe(false); }); - it('refuses a valid service token minted for something other than external delivery', async () => { - (validateInternalServiceToken as any).mockResolvedValue({ - sub: 'seamless-auth-express-adapter', - iss: 'seamless-portal-api', - aud: 'seamless-auth', - }); - - await expect( - canReturnExternalDelivery( - req({ - 'x-seamless-auth-delivery-mode': 'external', - 'x-seamless-service-token': 'Bearer proxy-token', - }), - ), - ).resolves.toBe(false); - }); + // TODO(#556): requiring the `seamless-auth-external-delivery` subject waits on the + // seamless-cli conformance harness, which mints `seamless-verify` for delivery. This + // reproduces the finding and starts failing, so it must become `it`, once it is fixed. + it.fails( + 'refuses a valid service token minted for something other than external delivery', + async () => { + (validateInternalServiceToken as any).mockResolvedValue({ + sub: 'seamless-auth-express-adapter', + iss: 'seamless-portal-api', + aud: 'seamless-auth', + }); + + await expect( + canReturnExternalDelivery( + req({ + 'x-seamless-auth-delivery-mode': 'external', + 'x-seamless-service-token': 'Bearer proxy-token', + }), + ), + ).resolves.toBe(false); + }, + ); it('allows external delivery with a trusted service token outside production', async () => { (validateInternalServiceToken as any).mockResolvedValue({ diff --git a/tests/unit/lib/oauthProviderPolicy.spec.ts b/tests/unit/lib/oauthProviderPolicy.spec.ts index a8310449..6af3077f 100644 --- a/tests/unit/lib/oauthProviderPolicy.spec.ts +++ b/tests/unit/lib/oauthProviderPolicy.spec.ts @@ -3,6 +3,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const lookup = vi.fn(); vi.mock('node:dns/promises', () => ({ lookup, default: { lookup } })); +// The real socket and address guard, not the suite's fetch-backed stand-in. +vi.unmock('../../../src/lib/providerTransport.js'); const { createServer, get: httpGet } = await import('node:http'); type AddressInfo = import('node:net').AddressInfo; @@ -292,28 +294,6 @@ describe('guardedLookup', () => { }); }); -describe('providerFetch', () => { - it('does not follow redirects', async () => { - const fetch = vi.fn().mockResolvedValue(new Response(null, { status: 302 })); - vi.stubGlobal('fetch', fetch); - - await expect( - providerFetch('https://idp.example.com/token', { method: 'POST' }, {}), - ).rejects.toThrow(/redirect \(302\)/); - expect(fetch).toHaveBeenCalledWith('https://idp.example.com/token', { - method: 'POST', - redirect: 'manual', - }); - }); - - it('returns other responses', async () => { - const answer = new Response('{}', { status: 400 }); - vi.stubGlobal('fetch', vi.fn().mockResolvedValue(answer)); - - await expect(providerFetch('https://idp.example.com/token', {}, {})).resolves.toBe(answer); - }); -}); - describe('providerFetch outside production', () => { const staging = { NODE_ENV: 'staging' }; @@ -388,6 +368,17 @@ describe('guardedRequest', () => { const url = (path: string) => new URL(`http://idp.corp.example:${port}${path}`); + it('lets providerFetch reach a private provider outside production', async () => { + const response = await providerFetch(url('/token'), { method: 'POST' }, {}); + + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ method: 'POST' }); + }); + + it('does not let providerFetch follow a redirect', async () => { + await expect(providerFetch(url('/redirect'), {}, {})).rejects.toThrow(/redirect \(302\)/); + }); + it('sends the method, headers and body, and returns the answer', async () => { const response = await guardedRequest( url('/token'), From a8723b521ac0fa0c4a00f3d7bffe9b751319e31b Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:54:22 -0500 Subject: [PATCH 11/13] test(recovery): reproduce owner notices dropped for kinds the adapter does not deliver Refs #557 --- .../recovery/accountRecovery.spec.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/integration/recovery/accountRecovery.spec.ts b/tests/integration/recovery/accountRecovery.spec.ts index ee13c347..7b39551f 100644 --- a/tests/integration/recovery/accountRecovery.spec.ts +++ b/tests/integration/recovery/accountRecovery.spec.ts @@ -350,6 +350,37 @@ describe('POST /recovery/verify', () => { expect(JSON.stringify(logged.metadata)).not.toContain('/recovery/cancel#'); }); + it('sends directly the notice kinds a credentialed caller does not deliver', async () => { + const token = await mintInternalServiceToken(); + const started = await request(app) + .post('/recovery/start') + .set('x-seamless-auth-delivery-mode', 'external') + .set('x-seamless-service-token', token) + .send({ channel: 'email' }); + + const res = await request(app) + .post('/recovery/verify') + .set('x-seamless-auth-delivery-mode', 'external') + .set('x-seamless-service-token', token) + .set('x-seamless-auth-delivery-kinds', 'account_notice_email') + .send({ code: started.body.delivery.token }); + + expect(res.status).toBe(200); + expect(res.body.deliveries).toEqual([ + expect.objectContaining({ kind: 'account_notice_email', to: 'owner@example.com' }), + ]); + expect(sendNoticeEmail).not.toHaveBeenCalled(); + expect(sendNoticeSms).toHaveBeenCalledWith('+14155552671', expect.any(String)); + + const logged = (AuthEventService.log as any).mock.calls.find( + ([event]: [{ type: string }]) => event.type === 'account_recovery_verified', + )[0]; + expect(logged.metadata.notices).toEqual([ + { channel: 'email', status: 'returned_for_external_delivery' }, + { channel: 'phone', status: 'sent' }, + ]); + }); + it('still voids the request for a credentialed caller that lists no notice kind', async () => { const token = await mintInternalServiceToken(); const started = await request(app) From 3d64d8c4798e9698a502a167126f26973e016879 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:56:52 -0500 Subject: [PATCH 12/13] fix(notices): send owner notice kinds the adapter does not deliver Under external delivery, a notice whose kind the caller does not list in x-seamless-auth-delivery-kinds is now sent through the API's own transport for its channel when one is configured, instead of being recorded as not_sent_external_delivery and dropped. Refs #557 --- .changeset/send-unadvertised-notice-kinds.md | 5 +++ docs/account-recovery.md | 7 +++-- docs/extending.md | 5 +-- src/services/accountNotices.ts | 16 ++++++---- src/services/messagingService.ts | 23 ++++++++++++++ .../applications/accountNotices.spec.ts | 6 +++- .../recovery/accountRecovery.spec.ts | 7 +++-- tests/setup/mocks.ts | 1 + tests/unit/services/accountNotices.spec.ts | 31 ++++++++++++++++--- tests/unit/services/messagingService.spec.ts | 22 +++++++++++++ 10 files changed, 104 insertions(+), 19 deletions(-) create mode 100644 .changeset/send-unadvertised-notice-kinds.md diff --git a/.changeset/send-unadvertised-notice-kinds.md b/.changeset/send-unadvertised-notice-kinds.md new file mode 100644 index 00000000..f6295e2e --- /dev/null +++ b/.changeset/send-unadvertised-notice-kinds.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +Owner notices that a delivery-owning adapter does not advertise are now sent directly instead of dropped (#557). Under external delivery with a service token, a notice whose kind is missing from `x-seamless-auth-delivery-kinds` (for example the SMS notice on `/recovery/verify` when the adapter lists only `account_notice_email`) goes out through the API's own email or SMS transport when one is configured, and is recorded as `sent` or `failed` rather than `not_sent_external_delivery`. The same applies to the recovery completed and recovery code used notices and to `POST /applications/account-notices`. A recovery that previously answered `503 recovery_notice_failed` for an adapter listing no notice kinds now proceeds when the API can notify the owner itself. Admin identity-change notices are unchanged. diff --git a/docs/account-recovery.md b/docs/account-recovery.md index 55922c9e..32ad5ecd 100644 --- a/docs/account-recovery.md +++ b/docs/account-recovery.md @@ -144,9 +144,10 @@ off, and a request id from another account answers `404`. `x-seamless-auth-delivery-kinds` (`account_notice_email`, `account_notice_sms`): they arrive as `deliveries` on the `/recovery/verify` response, each with `notice: "account_recovery_opened"` and the `cancelUrl`, and count as notifying the owner. A - caller that owns delivery but lists neither kind gets the `503`, with each notice - recorded as `not_sent_external_delivery`, because an adapter that predates the kinds - would drop them. The adapter manifest marks the route `notices: true`. The delivery + kind the caller does not list goes out directly through the API's own transport for + that channel, because an adapter that predates the kinds would drop it. Only when the + API has no transport for it either is it recorded as `not_sent_external_delivery`, and + if no notice reached the owner the caller gets the `503`. The adapter manifest marks the route `notices: true`. The delivery header alone does not suppress notices, so a stranger cannot recover an account silently, and the cancel link is never written to the audit event. - Audit events: `account_recovery_started`, `account_recovery_verified` (with each notice's diff --git a/docs/extending.md b/docs/extending.md index 1f026bf5..5172ac5b 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -41,8 +41,9 @@ Kinds that older adapters do not know are only returned to a caller that names t `x-seamless-auth-delivery-kinds` (a comma separated list of `kind` values). Today that covers the owner notices for account recovery (`account_notice_email`, `account_notice_sms`), which `POST /recovery/verify` returns as a `deliveries` array; routes that can do this are marked -`notices: true` in the adapter manifest. A caller that does not name them gets direct -delivery, or a refusal where a notice is required. +`notices: true` in the adapter manifest. A kind the caller does not name is sent directly +through the API's own transport for its channel; when the API has none, the notice is not sent, and a +route that requires it refuses. This is how a server adapter integrates its own email/SMS stack without the API baking in a provider. See the delivery payload shapes in diff --git a/src/services/accountNotices.ts b/src/services/accountNotices.ts index 0a467fb6..a6531d0b 100644 --- a/src/services/accountNotices.ts +++ b/src/services/accountNotices.ts @@ -13,7 +13,7 @@ import type { import getLogger from '../utils/logger.js'; import { DeliveryError } from './deliveryError.js'; import type { IdentityNoticeStatus } from './identityChangeService.js'; -import { sendNoticeEmail, sendNoticeSms } from './messagingService.js'; +import { canSendNoticeDirectly, sendNoticeEmail, sendNoticeSms } from './messagingService.js'; const logger = getLogger('accountNotices'); @@ -85,9 +85,10 @@ export function verifiedAddresses(user: NoticeRecipient) { * account. A notice that fails to send does not fail the request it reports on; the * outcome belongs in that request's audit event. * - * `external` is for a credentialed caller that owns delivery, so the API does not send - * for it. A route that can return notices passes the kinds that caller accepts as - * `returnable`; a notice of any other kind is not sent at all. + * `external` is for a credentialed caller that owns delivery. A route that can return + * notices passes the kinds that caller accepts as `returnable`, and those are handed back + * rather than sent. A notice of any other kind is sent directly when the API has a + * transport for its channel, since the caller would drop it, and is otherwise not sent. */ export async function sendAccountNotice( user: NoticeRecipient, @@ -111,10 +112,13 @@ export async function sendAccountNotice( if (returnable?.has(delivery.kind)) { deliveries.push(delivery); results.push({ channel, status: 'returned_for_external_delivery' }); - } else { + continue; + } + + if (!canSendNoticeDirectly(channel)) { results.push({ channel, status: 'not_sent_external_delivery' }); + continue; } - continue; } try { diff --git a/src/services/messagingService.ts b/src/services/messagingService.ts index e9b83140..becec688 100644 --- a/src/services/messagingService.ts +++ b/src/services/messagingService.ts @@ -141,6 +141,29 @@ export const sendEnrollmentInviteEmail = async (to: string, signInUrl: string) = let noticeEmailTransport: EmailTransport | null = null; let noticeSmsTransport: SmsTransport | null = null; +/** + * Whether the API can send an account notice on a channel itself. Outside production a + * send is skipped and reported as sent, so this answers true there too; otherwise the + * channel's transport has to be configured. + */ +export function canSendNoticeDirectly(channel: 'email' | 'phone') { + if (shouldBypassDirectMessaging()) { + return true; + } + + try { + if (channel === 'email') { + noticeEmailTransport ??= createDirectEmailTransport(); + } else { + noticeSmsTransport ??= createDirectSmsTransport(); + } + return true; + } catch (error) { + logger.debug(`No direct transport for account notices on ${channel}: ${error}`); + return false; + } +} + export const sendNoticeEmail = async (to: string, subject: string, text: string) => { logger.debug('Sending account notice email'); diff --git a/tests/integration/applications/accountNotices.spec.ts b/tests/integration/applications/accountNotices.spec.ts index 72b17194..1d729239 100644 --- a/tests/integration/applications/accountNotices.spec.ts +++ b/tests/integration/applications/accountNotices.spec.ts @@ -292,7 +292,7 @@ describe('POST /applications/account-notices', () => { expect(res.status).toBe(200); expect(res.body.channels).toEqual([ { channel: 'email', status: 'returned_for_external_delivery' }, - { channel: 'sms', status: 'not_sent_external_delivery' }, + { channel: 'sms', status: 'sent' }, ]); expect(res.body.deliveries).toEqual([ expect.objectContaining({ @@ -303,6 +303,10 @@ describe('POST /applications/account-notices', () => { }), ]); expect(sendNoticeEmail).not.toHaveBeenCalled(); + expect(sendNoticeSms).toHaveBeenCalledWith( + '+14155552671', + expect.stringContaining('Locksley'), + ); }); it('sends directly when the caller asks for external delivery without the service token', async () => { diff --git a/tests/integration/recovery/accountRecovery.spec.ts b/tests/integration/recovery/accountRecovery.spec.ts index 7b39551f..2db36161 100644 --- a/tests/integration/recovery/accountRecovery.spec.ts +++ b/tests/integration/recovery/accountRecovery.spec.ts @@ -35,6 +35,7 @@ import { Session } from '../../../src/models/sessions.js'; import { AuthEventService } from '../../../src/services/authEventService.js'; import { DeliveryError } from '../../../src/services/deliveryError.js'; import { + canSendNoticeDirectly, sendNoticeEmail, sendNoticeSms, sendOTPEmail, @@ -276,7 +277,8 @@ describe('POST /recovery/verify', () => { ); }); - it('voids the request when a credentialed caller owns delivery, since no notice is sent', async () => { + it('voids the request when a credentialed caller owns delivery and the API cannot send', async () => { + vi.mocked(canSendNoticeDirectly).mockReturnValue(false); const token = await mintInternalServiceToken(); const started = await request(app) .post('/recovery/start') @@ -381,7 +383,8 @@ describe('POST /recovery/verify', () => { ]); }); - it('still voids the request for a credentialed caller that lists no notice kind', async () => { + it('still voids the request for a caller that lists no notice kind when the API cannot send', async () => { + vi.mocked(canSendNoticeDirectly).mockReturnValue(false); const token = await mintInternalServiceToken(); const started = await request(app) .post('/recovery/start') diff --git a/tests/setup/mocks.ts b/tests/setup/mocks.ts index 8901f795..5137ae7f 100644 --- a/tests/setup/mocks.ts +++ b/tests/setup/mocks.ts @@ -343,6 +343,7 @@ vi.mock('../../src/services/messagingService.js', () => ({ sendOTPSMS: vi.fn(), sendNoticeEmail: vi.fn(), sendNoticeSms: vi.fn(), + canSendNoticeDirectly: vi.fn(() => true), })); vi.mock('crypto', async () => { diff --git a/tests/unit/services/accountNotices.spec.ts b/tests/unit/services/accountNotices.spec.ts index a6f85370..0489b2a8 100644 --- a/tests/unit/services/accountNotices.spec.ts +++ b/tests/unit/services/accountNotices.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { ownerWasNotified, @@ -6,7 +6,11 @@ import { verifiedAddresses, } from '../../../src/services/accountNotices.js'; import { DeliveryError } from '../../../src/services/deliveryError.js'; -import { sendNoticeEmail, sendNoticeSms } from '../../../src/services/messagingService.js'; +import { + canSendNoticeDirectly, + sendNoticeEmail, + sendNoticeSms, +} from '../../../src/services/messagingService.js'; const notice = { name: 'account_recovery_opened' as const, @@ -60,7 +64,23 @@ describe('sendAccountNotice', () => { await expect(sendAccountNotice(owner, notice, { external: false })).rejects.toThrow('bug'); }); - it('sends nothing to a caller that owns delivery, and says so', async () => { + it('sends directly what a caller that owns delivery does not take', async () => { + const outcome = await sendAccountNotice(owner, notice, { external: true }); + + expect(sendNoticeEmail).toHaveBeenCalledWith('owner@example.com', 'Subject', 'Body'); + expect(sendNoticeSms).toHaveBeenCalledWith('+14155552671', 'Body'); + expect(outcome).toEqual({ + results: [ + { channel: 'email', status: 'sent' }, + { channel: 'phone', status: 'sent' }, + ], + deliveries: [], + }); + }); + + it('sends nothing for a caller that owns delivery when the API has no transport, and says so', async () => { + vi.mocked(canSendNoticeDirectly).mockReturnValue(false); + const outcome = await sendAccountNotice(owner, notice, { external: true }); expect(sendNoticeEmail).not.toHaveBeenCalled(); @@ -74,16 +94,17 @@ describe('sendAccountNotice', () => { expect(ownerWasNotified(outcome)).toBe(false); }); - it('hands a notice back only in the kinds the caller accepts', async () => { + it('hands a notice back only in the kinds the caller accepts, and sends the rest', async () => { const outcome = await sendAccountNotice(owner, notice, { external: true, returnable: new Set(['account_notice_email']), }); expect(sendNoticeEmail).not.toHaveBeenCalled(); + expect(sendNoticeSms).toHaveBeenCalledWith('+14155552671', 'Body'); expect(outcome.results).toEqual([ { channel: 'email', status: 'returned_for_external_delivery' }, - { channel: 'phone', status: 'not_sent_external_delivery' }, + { channel: 'phone', status: 'sent' }, ]); expect(outcome.deliveries).toEqual([ { diff --git a/tests/unit/services/messagingService.spec.ts b/tests/unit/services/messagingService.spec.ts index b976a92b..3ea9a62a 100644 --- a/tests/unit/services/messagingService.spec.ts +++ b/tests/unit/services/messagingService.spec.ts @@ -228,4 +228,26 @@ describe('messagingService', () => { await expect(sendNoticeSms('+14155552671', 'B')).rejects.toBeInstanceOf(DeliveryError); await expect(sendNoticeSms('not-a-phone', 'B')).rejects.toBeInstanceOf(DeliveryError); }); + + it('can always send a notice outside production, where sends are skipped', async () => { + process.env.NODE_ENV = 'development'; + + const { canSendNoticeDirectly } = await import('../../../src/services/messagingService'); + + expect(canSendNoticeDirectly('email')).toBe(true); + expect(canSendNoticeDirectly('phone')).toBe(true); + expect(createDirectSmsTransportMock).not.toHaveBeenCalled(); + }); + + it('can send a notice in production only on a channel with a configured transport', async () => { + process.env.NODE_ENV = 'production'; + createDirectSmsTransportMock.mockImplementationOnce(() => { + throw new Error('MESSAGING_TWILIO_ACCOUNT_SID is required'); + }); + + const { canSendNoticeDirectly } = await import('../../../src/services/messagingService'); + + expect(canSendNoticeDirectly('email')).toBe(true); + expect(canSendNoticeDirectly('phone')).toBe(false); + }); }); From 41ce6f64a89209c524580bd7a5ee21423c9651ce Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 2 Nov 2026 21:58:59 -0500 Subject: [PATCH 13/13] chore: mark behavior-changing security fixes as minor --- .changeset/notice-text-without-links.md | 2 +- .changeset/passkey-assurance-from-registration.md | 2 +- .changeset/send-unadvertised-notice-kinds.md | 2 +- .changeset/third-party-clients-gate.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changeset/notice-text-without-links.md b/.changeset/notice-text-without-links.md index c3ceab01..684f065a 100644 --- a/.changeset/notice-text-without-links.md +++ b/.changeset/notice-text-without-links.md @@ -1,5 +1,5 @@ --- -'seamless-auth-api': patch +'seamless-auth-api': minor --- Application notice text can no longer carry its own links (#554). `POST /applications/account-notices` refuses with `400` a `text` that contains a URL, a `www.` address, a domain name or an email address, since mail and SMS clients turn all of them into links; `actionUrl`, checked against the application's origins, is the only link a notice carries. The `Sent by through .` attribution is now the first line of the message, ahead of the text, rather than a footer the text could imitate. Adapters that render notices from external delivery receive the new layout in `text`. diff --git a/.changeset/passkey-assurance-from-registration.md b/.changeset/passkey-assurance-from-registration.md index 4be32fdb..04ac0705 100644 --- a/.changeset/passkey-assurance-from-registration.md +++ b/.changeset/passkey-assurance-from-registration.md @@ -1,5 +1,5 @@ --- -'seamless-auth-api': patch +'seamless-auth-api': minor --- Passkey hardware assurance now comes from the registered credential rather than the assertion (#552). Sign-in and step-up refuse an assertion whose backup eligibility flag differs from the one stored at registration (`401`, audited as `webauthn_login_failed` or `step_up_failed` with reason `Backup eligibility differs from registration`). A passkey counts as device-bound, and earns `acr=phrh` and `amr=hwk`, only when it was registered device-bound with attestation verified against the FIDO Metadata Service. Deployments that use the default `none` attestation, and credentials registered before attestation was recorded, now get `phr` and `swk` instead of `phrh` and `hwk`. diff --git a/.changeset/send-unadvertised-notice-kinds.md b/.changeset/send-unadvertised-notice-kinds.md index f6295e2e..59bb9e7b 100644 --- a/.changeset/send-unadvertised-notice-kinds.md +++ b/.changeset/send-unadvertised-notice-kinds.md @@ -1,5 +1,5 @@ --- -'seamless-auth-api': patch +'seamless-auth-api': minor --- Owner notices that a delivery-owning adapter does not advertise are now sent directly instead of dropped (#557). Under external delivery with a service token, a notice whose kind is missing from `x-seamless-auth-delivery-kinds` (for example the SMS notice on `/recovery/verify` when the adapter lists only `account_notice_email`) goes out through the API's own email or SMS transport when one is configured, and is recorded as `sent` or `failed` rather than `not_sent_external_delivery`. The same applies to the recovery completed and recovery code used notices and to `POST /applications/account-notices`. A recovery that previously answered `503 recovery_notice_failed` for an adapter listing no notice kinds now proceeds when the API can notify the owner itself. Admin identity-change notices are unchanged. diff --git a/.changeset/third-party-clients-gate.md b/.changeset/third-party-clients-gate.md index d76e1daf..f58b58aa 100644 --- a/.changeset/third-party-clients-gate.md +++ b/.changeset/third-party-clients-gate.md @@ -1,5 +1,5 @@ --- -'seamless-auth-api': patch +'seamless-auth-api': minor --- `third_party_clients_enabled` now gates existing third-party OpenID Provider clients, not only creating and updating them (#555). While it is off, a client that is not first-party is treated like a disabled one: `/oidc/clients/:id` answers `404`, its shared secret does not verify, `/oidc/sign` refuses with `client_inactive`, its notice tokens are refused and back-channel logout skips it. An instance that turned the flag off after registering third-party clients stops serving them on upgrade; turn the flag back on, or mark the applications first-party, to keep them working.