From a7e444c55e600551bb538a083ffaffdee558c389 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 29 Aug 2026 01:04:22 -0400 Subject: [PATCH] fix(webauthn): ask for exactly the user verification that will be enforced Registration advertised userVerification 'preferred', telling the authenticator verification was optional, then rejected a response that skipped it, because SimpleWebAuthn's requireUserVerification defaults to true and this server never set it. A user on an authenticator that skips verification completed the whole ceremony and failed at the last step, having never been asked to verify. Authentication separately asked for 'required', so the two halves of the same product disagreed. authenticator_policy.userVerification now drives both the ask and the enforcement, on registration and authentication, from one value. Defaults to required, which does not change what is accepted, since that was already enforced. It changes what is asked for. Step-up deliberately keeps requiring verification regardless of the policy. It exists to re-verify the human, and without verification it is a second signature from a key the session already proved it holds. Its verification is now pinned explicitly rather than relying on the same library default. Closes #167 --- .changeset/lazy-pears-repeat.md | 28 +++++++ .env.example | 4 +- docs/configuration.md | 2 +- openapi.json | 12 ++- package-lock.json | 8 +- package.json | 2 +- src/config/systemConfig.defaults.ts | 1 + src/controllers/stepUp.ts | 8 ++ src/controllers/webauthn.ts | 16 ++-- src/generated/api.ts | 13 +++- tests/integration/webauthn/webauthn.spec.ts | 82 +++++++++++++++++++-- 11 files changed, 156 insertions(+), 20 deletions(-) create mode 100644 .changeset/lazy-pears-repeat.md diff --git a/.changeset/lazy-pears-repeat.md b/.changeset/lazy-pears-repeat.md new file mode 100644 index 0000000..adf6f19 --- /dev/null +++ b/.changeset/lazy-pears-repeat.md @@ -0,0 +1,28 @@ +--- +'seamless-auth-api': minor +--- + +Ask for exactly the user verification that will be enforced. + +Registration advertised `userVerification: 'preferred'`, telling the +authenticator verification was optional, and then rejected a response that +skipped it, because SimpleWebAuthn's `requireUserVerification` defaults to true +and this server never set it. A user on an authenticator that skips verification +completed the whole ceremony and failed at the last step, having never been asked +to verify. Authentication separately asked for `required`, so the two halves +disagreed. + +`authenticator_policy.userVerification` now drives both, for registration and +authentication, so what the browser is asked for and what the server accepts come +from one value and cannot drift. It accepts `required`, `preferred` or +`discouraged` and defaults to `required`. + +The default does not change what is accepted, since that was already enforced. It +changes what is asked for, so an authenticator is told to verify rather than +being allowed to skip and be rejected afterwards. + +Step-up deliberately still requires verification regardless of the policy. It +exists to re-verify the human, and without verification it is a second signature +from a key the session already proved it holds. + +Requires `@seamless-auth/types` 0.13.0. diff --git a/.env.example b/.env.example index 8155621..eea6b9b 100644 --- a/.env.example +++ b/.env.example @@ -86,7 +86,9 @@ LOCKOUT_POLICY={"enabled":true,"maxFailures":10,"windowSeconds":900,"lockoutSeco # JSON. Which authenticators this deployment will enrol. attachment is any, platform or # cross-platform. "any" offers both built-in authenticators and roaming security keys; # naming one narrows the browser picker and rejects a request asking for the other. -AUTHENTICATOR_POLICY={"attachment":"any"} +# userVerification is required, preferred or discouraged. It drives both what the +# browser is asked for and what the server enforces, so the two cannot disagree. +AUTHENTICATOR_POLICY={"attachment":"any","userVerification":"required"} # SERVICE TOKENS # Required for trusted server adapters and internal bearer validation. diff --git a/docs/configuration.md b/docs/configuration.md index 6994df7..b021f84 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -269,7 +269,7 @@ Validation is enforced by [`systemConfig.schema.ts`](../src/schemas/systemConfig | `passkey_login_fallback_enabled` | boolean | `PASSKEY_LOGIN_FALLBACK_ENABLED` | `true` | | `oauth_providers` | provider[] | `OAUTH_PROVIDERS` | `[]` | | `lockout_policy` | object | `LOCKOUT_POLICY` | `{enabled,maxFailures:10,windowSeconds:900,lockoutSeconds:900}` | -| `authenticator_policy` | object | `AUTHENTICATOR_POLICY` | `{attachment:"any"}` | +| `authenticator_policy` | object | `AUTHENTICATOR_POLICY` | `{attachment:"any",userVerification:"required"}` | | `session_idle_ttl` | string (`\d+[smhd]`) | `SESSION_IDLE_TTL` | `8h` | | `access_token_ttl` | string (`\d+[smhd]`) | `ACCESS_TOKEN_TTL` | - | | `refresh_token_ttl` | string (`\d+[smhd]`) | `REFRESH_TOKEN_TTL` | - | diff --git a/openapi.json b/openapi.json index 0a0cbf0..8a23a6a 100644 --- a/openapi.json +++ b/openapi.json @@ -6333,9 +6333,14 @@ "type": "string", "enum": ["any", "platform", "cross-platform"], "default": "any" + }, + "userVerification": { + "type": "string", + "enum": ["required", "preferred", "discouraged"], + "default": "required" } }, - "default": { "attachment": "any" } + "default": { "attachment": "any", "userVerification": "required" } }, "access_token_ttl": { "type": "string", "pattern": "^\\d+[smhd]$" }, "session_idle_ttl": { @@ -6536,6 +6541,11 @@ "type": "string", "enum": ["any", "platform", "cross-platform"], "default": "any" + }, + "userVerification": { + "type": "string", + "enum": ["required", "preferred", "discouraged"], + "default": "required" } } }, diff --git a/package-lock.json b/package-lock.json index a31dce6..727a67d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,7 +13,7 @@ "@seamless-auth/messaging": "^0.1.0", "@seamless-auth/messaging-aws": "^0.1.0", "@seamless-auth/messaging-twilio": "^0.1.0", - "@seamless-auth/types": "^0.12.0", + "@seamless-auth/types": "^0.13.0", "@simplewebauthn/server": "^13.1.1", "base64url": "^3.0.1", "bcrypt-ts": "^7.1.0", @@ -3393,9 +3393,9 @@ } }, "node_modules/@seamless-auth/types": { - "version": "0.12.0", - "resolved": "https://registry.npmjs.org/@seamless-auth/types/-/types-0.12.0.tgz", - "integrity": "sha512-FetF+KYZ7HQ1X5TNczRCVRPL3GQ8aV/CpgtwAGntse9LMaLUHe1ttxd/dFjTYkPq0c+NdcCGYkWWykWFWQnD/Q==", + "version": "0.13.0", + "resolved": "https://registry.npmjs.org/@seamless-auth/types/-/types-0.13.0.tgz", + "integrity": "sha512-dkgTljGcMur/U93pGz9aU3I3xMNtSLE5ZECx/AGqVl8Kj3V/2MDV6FLspTb5qN81foilMJUBVMkPSzsE28vt/A==", "license": "AGPL-3.0-only", "dependencies": { "zod": "^4.3.6" diff --git a/package.json b/package.json index da5a388..2327da8 100644 --- a/package.json +++ b/package.json @@ -50,7 +50,7 @@ "@seamless-auth/messaging": "^0.1.0", "@seamless-auth/messaging-aws": "^0.1.0", "@seamless-auth/messaging-twilio": "^0.1.0", - "@seamless-auth/types": "^0.12.0", + "@seamless-auth/types": "^0.13.0", "@simplewebauthn/server": "^13.1.1", "base64url": "^3.0.1", "bcrypt-ts": "^7.1.0", diff --git a/src/config/systemConfig.defaults.ts b/src/config/systemConfig.defaults.ts index aa7c685..1de1e85 100644 --- a/src/config/systemConfig.defaults.ts +++ b/src/config/systemConfig.defaults.ts @@ -17,6 +17,7 @@ export const SYSTEM_CONFIG_DEFAULTS: Partial = { }, authenticator_policy: { attachment: 'any', + userVerification: 'required', }, session_idle_ttl: '8h', passkey_login_fallback_enabled: true, diff --git a/src/controllers/stepUp.ts b/src/controllers/stepUp.ts index 10ce6d1..7eef357 100644 --- a/src/controllers/stepUp.ts +++ b/src/controllers/stepUp.ts @@ -106,6 +106,11 @@ export const startWebAuthnStepUp = async (req: Request, res: Response) => { id: credential.id, transports: credential.transports, })), + // Deliberately not the deployment's authenticator_policy. Step-up exists to + // re-verify the human; without user verification it is a second signature + // from a key the session already proved it holds, which proves nothing + // extra. A deployment that relaxes verification generally should still get + // a real check when elevating. userVerification: 'required', timeout: 60000, rpID: rpid, @@ -199,6 +204,9 @@ export const finishWebAuthnStepUp = async (req: Request, res: Response) => { const { origins, rpid } = await getSystemConfig(); const verification = await verifyAuthenticationResponse({ response: assertionResponse, + // Matches the 'required' asked for above rather than relying on a library + // default, so the ask and the enforcement cannot drift apart. + requireUserVerification: true, expectedChallenge, expectedOrigin: origins, expectedRPID: rpid, diff --git a/src/controllers/webauthn.ts b/src/controllers/webauthn.ts index 90549a2..af0e0f0 100644 --- a/src/controllers/webauthn.ts +++ b/src/controllers/webauthn.ts @@ -126,6 +126,7 @@ const registerWebAuthn = async (req: Request, res: Response) => { }); const { app_name, rpid, authenticator_policy } = await getSystemConfig(); + const userVerification = authenticator_policy.userVerification; const pinnedAttachment = authenticator_policy.attachment === 'any' ? null : authenticator_policy.attachment; @@ -164,7 +165,9 @@ const registerWebAuthn = async (req: Request, res: Response) => { // this to 'platform' hides roaming authenticators from the browser picker // entirely, which makes issued security keys impossible to enrol. authenticatorSelection: { - userVerification: 'preferred', + // The same value is enforced at verification below, so the browser is + // never asked for less than the server will accept. + userVerification, residentKey: 'preferred', ...(effectiveAttachment ? { authenticatorAttachment: effectiveAttachment } : {}), }, @@ -264,13 +267,14 @@ const verifyWebAuthnRegistration = async (req: Request, res: Response) => { let verification; try { - const { origins, rpid } = await getSystemConfig(); + const { origins, rpid, authenticator_policy } = await getSystemConfig(); verification = await verifyRegistrationResponse({ response: attestationResponse, expectedChallenge, expectedOrigin: origins, expectedRPID: rpid, + requireUserVerification: authenticator_policy.userVerification === 'required', // Pinned to the advertised set. The library default here is every // algorithm it knows, which would accept a credential using something // this server never offered. @@ -413,7 +417,8 @@ const generateWebAuthn = async (req: Request, res: Response) => { return res.status(401).send('Credentials not found'); } - const { rpid } = await getSystemConfig(); + const { rpid, authenticator_policy } = await getSystemConfig(); + const userVerification = authenticator_policy.userVerification; const options: PublicKeyCredentialRequestOptionsJSON = await generateAuthenticationOptions({ allowCredentials: assertionCredentials.map((cred) => { @@ -422,7 +427,7 @@ const generateWebAuthn = async (req: Request, res: Response) => { transports: cred.transports, }; }), - userVerification: 'required', + userVerification, timeout: 60000, rpID: rpid, extensions: buildPrfAuthenticationExtensions(prf), @@ -532,12 +537,13 @@ const verifyWebAuthn = async (req: Request, res: Response) => { let verification; try { - const { origins, rpid } = await getSystemConfig(); + const { origins, rpid, authenticator_policy } = await getSystemConfig(); verification = await verifyAuthenticationResponse({ response: assertionResponse, expectedChallenge, expectedOrigin: origins, expectedRPID: rpid, + requireUserVerification: authenticator_policy.userVerification === 'required', credential: { id: cred.id, // @ts-expect-error Needed to work. diff --git a/src/generated/api.ts b/src/generated/api.ts index 409efef..9dc97c3 100644 --- a/src/generated/api.ts +++ b/src/generated/api.ts @@ -6850,7 +6850,8 @@ export interface paths { }; /** * @default { - * "attachment": "any" + * "attachment": "any", + * "userVerification": "required" * } */ authenticator_policy: { @@ -6859,6 +6860,11 @@ export interface paths { * @enum {string} */ attachment: 'any' | 'platform' | 'cross-platform'; + /** + * @default required + * @enum {string} + */ + userVerification: 'required' | 'preferred' | 'discouraged'; }; access_token_ttl: string; /** @default 8h */ @@ -6985,6 +6991,11 @@ export interface paths { * @enum {string} */ attachment?: 'any' | 'platform' | 'cross-platform'; + /** + * @default required + * @enum {string} + */ + userVerification?: 'required' | 'preferred' | 'discouraged'; }; access_token_ttl?: string; session_idle_ttl?: string; diff --git a/tests/integration/webauthn/webauthn.spec.ts b/tests/integration/webauthn/webauthn.spec.ts index 9e06441..63beaac 100644 --- a/tests/integration/webauthn/webauthn.spec.ts +++ b/tests/integration/webauthn/webauthn.spec.ts @@ -76,7 +76,7 @@ beforeEach(() => { access_token_ttl: '15m', refresh_token_ttl: '1h', session_idle_ttl: '8h', - authenticator_policy: { attachment: 'any' }, + authenticator_policy: { attachment: 'any', userVerification: 'required' }, }); (Credential.findAll as any).mockResolvedValue([]); (Credential.findOne as any).mockResolvedValue(null); @@ -88,7 +88,7 @@ describe('GET /webauthn/register/start', () => { (getSystemConfig as any).mockResolvedValue({ app_name: 'SeamlessAuth', rpid: 'localhost', - authenticator_policy: { attachment: 'any' }, + authenticator_policy: { attachment: 'any', userVerification: 'required' }, }); const { generateRegistrationOptions } = await import('@simplewebauthn/server'); @@ -108,7 +108,7 @@ describe('GET /webauthn/register/start', () => { (getSystemConfig as any).mockResolvedValue({ app_name: 'SeamlessAuth', rpid: 'localhost', - authenticator_policy: { attachment: 'any' }, + authenticator_policy: { attachment: 'any', userVerification: 'required' }, }); const { generateRegistrationOptions } = await import('@simplewebauthn/server'); @@ -203,7 +203,7 @@ describe('GET /webauthn/register/start', () => { (getSystemConfig as any).mockResolvedValue({ app_name: 'SeamlessAuth', rpid: 'localhost', - authenticator_policy: { attachment: 'cross-platform' }, + authenticator_policy: { attachment: 'cross-platform', userVerification: 'required' }, }); const { generateRegistrationOptions } = await import('@simplewebauthn/server'); (generateRegistrationOptions as any).mockResolvedValue({ challenge: 'challenge' }); @@ -224,7 +224,7 @@ describe('GET /webauthn/register/start', () => { (getSystemConfig as any).mockResolvedValue({ app_name: 'SeamlessAuth', rpid: 'localhost', - authenticator_policy: { attachment: 'cross-platform' }, + authenticator_policy: { attachment: 'cross-platform', userVerification: 'required' }, }); const { generateRegistrationOptions } = await import('@simplewebauthn/server'); (generateRegistrationOptions as any).mockResolvedValue({ challenge: 'challenge' }); @@ -240,7 +240,7 @@ describe('GET /webauthn/register/start', () => { (getSystemConfig as any).mockResolvedValue({ app_name: 'SeamlessAuth', rpid: 'localhost', - authenticator_policy: { attachment: 'cross-platform' }, + authenticator_policy: { attachment: 'cross-platform', userVerification: 'required' }, }); const { generateRegistrationOptions } = await import('@simplewebauthn/server'); (generateRegistrationOptions as any).mockResolvedValue({ challenge: 'challenge' }); @@ -303,6 +303,38 @@ describe('GET /webauthn/register/start', () => { expect(options.supportedAlgorithmIDs).toEqual([-8, -7, -257, -65535]); }); + // Registration used to ask for 'preferred' while the library default enforced + // 'required', so a user on an authenticator that skips verification completed + // the whole ceremony and was rejected at the last step. + it('asks for exactly the verification it will enforce', async () => { + const { generateRegistrationOptions } = await import('@simplewebauthn/server'); + (generateRegistrationOptions as any).mockResolvedValue({ challenge: 'challenge' }); + + const res = await request(app).get('/webauthn/register/start'); + + expect(res.status).toBe(200); + + const [options] = (generateRegistrationOptions as any).mock.calls.at(-1); + + expect(options.authenticatorSelection.userVerification).toBe('required'); + }); + + it('follows a deployment that relaxes verification', async () => { + (getSystemConfig as any).mockResolvedValue({ + app_name: 'SeamlessAuth', + rpid: 'localhost', + authenticator_policy: { attachment: 'any', userVerification: 'discouraged' }, + }); + const { generateRegistrationOptions } = await import('@simplewebauthn/server'); + (generateRegistrationOptions as any).mockResolvedValue({ challenge: 'challenge' }); + + await request(app).get('/webauthn/register/start'); + + const [options] = (generateRegistrationOptions as any).mock.calls.at(-1); + + expect(options.authenticatorSelection.userVerification).toBe('discouraged'); + }); + it('returns 500 when credential lookup fails', async () => { (Credential.findAll as any).mockRejectedValue(new Error('db down')); @@ -384,6 +416,42 @@ describe('challenge is spent on every login outcome', () => { }); }); +describe('verification policy reaches both verifiers', () => { + it('enforces user verification at registration when the policy requires it', async () => { + (User.findOne as any).mockResolvedValue(buildUser()); + const { verifyRegistrationResponse } = await import('@simplewebauthn/server'); + (verifyRegistrationResponse as any).mockResolvedValue({ verified: false }); + + await request(app) + .post('/webauthn/register/finish') + .send({ attestationResponse: {}, metadata: {} }); + + const [options] = (verifyRegistrationResponse as any).mock.calls.at(-1); + + expect(options.requireUserVerification).toBe(true); + }); + + it('stops enforcing it when the deployment relaxes the policy', async () => { + (getSystemConfig as any).mockResolvedValue({ + app_name: 'SeamlessAuth', + rpid: 'localhost', + origins: ['http://localhost:5137'], + authenticator_policy: { attachment: 'any', userVerification: 'discouraged' }, + }); + (User.findOne as any).mockResolvedValue(buildUser()); + const { verifyRegistrationResponse } = await import('@simplewebauthn/server'); + (verifyRegistrationResponse as any).mockResolvedValue({ verified: false }); + + await request(app) + .post('/webauthn/register/finish') + .send({ attestationResponse: {}, metadata: {} }); + + const [options] = (verifyRegistrationResponse as any).mock.calls.at(-1); + + expect(options.requireUserVerification).toBe(false); + }); +}); + describe('POST /webauthn/register/finish', () => { it('creates credential and session', async () => { const user = buildUser(); @@ -895,6 +963,7 @@ describe('POST /webauthn/login/finish', () => { (getSystemConfig as any).mockResolvedValue({ origins: ['http://localhost:5137'], rpid: 'localhost', + authenticator_policy: { attachment: 'any', userVerification: 'required' }, }); const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockRejectedValue(new Error('bad assertion')); @@ -953,6 +1022,7 @@ describe('POST /webauthn/login/finish', () => { (getSystemConfig as any).mockResolvedValue({ origins: ['http://localhost:5137'], rpid: 'localhost', + authenticator_policy: { attachment: 'any', userVerification: 'required' }, }); const { verifyAuthenticationResponse } = await import('@simplewebauthn/server'); (verifyAuthenticationResponse as any).mockResolvedValue({