From 86c3553d7e19eafc64e81e0aff435b5802838cfe Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:01:24 +0300 Subject: [PATCH 01/29] =?UTF-8?q?fix(oauth2):=20cap=20granted=20scope=20at?= =?UTF-8?q?=20client=20=E2=88=A9=20server=20scopes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit authorize and client_credentials used the client's registered scope instead of the server's advertised list whenever the client declared one, so a client registering scope "admin" could be granted it. The device authorization endpoint did not validate the requested scope at all. Every grant now goes through one scopeCeiling() helper: the intersection of client.scope and config.scopes (either may be absent). --- .../src/server/grants/client-credentials.js | 27 ++++----- .../oauth2/src/server/handlers/authorize.js | 25 +++----- packages/oauth2/src/server/handlers/device.js | 4 +- packages/oauth2/src/server/security/scope.js | 60 +++++++++++++++++++ .../oauth2/tests/server-hardening.test.js | 58 ++++++++++++++++++ packages/oauth2/tests/server-tierc.test.js | 7 +++ 6 files changed, 145 insertions(+), 36 deletions(-) create mode 100644 packages/oauth2/src/server/security/scope.js create mode 100644 packages/oauth2/tests/server-hardening.test.js diff --git a/packages/oauth2/src/server/grants/client-credentials.js b/packages/oauth2/src/server/grants/client-credentials.js index b6dd3500..9cc05f0b 100644 --- a/packages/oauth2/src/server/grants/client-credentials.js +++ b/packages/oauth2/src/server/grants/client-credentials.js @@ -8,6 +8,7 @@ import { isArray, isNonEmptyString } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; import { resolveTokenAudience } from '../security/resource.js'; +import { assertWithinCeiling, parseScope, scopeCeiling } from '../security/scope.js'; import { buildAccessResponse } from './_issue.js'; /** @@ -40,10 +41,11 @@ export async function clientCredentialsGrant(req, ctx) { } /** - * A requested scope is validated against the client's registered `scope` (or - * the server's advertised `scopes` when the client declares none). With no - * `scope` requested, the client's registered scope is granted — or an empty - * scope when it registers none; never the full server scope list by default. + * A requested scope is validated against the client's scope ceiling (its + * registered `scope` ∩ the server's advertised `scopes`). With no `scope` + * requested, the client's registered scope (within that ceiling) is granted — + * or an empty scope when it registers none; never the full server scope list + * by default. * * @param {string | undefined} requested * @param {import('../clients.js').Client} client @@ -51,20 +53,11 @@ export async function clientCredentialsGrant(req, ctx) { * @returns {string[]} */ function resolveScope(requested, client, config) { + const ceiling = scopeCeiling(client, config); if (!isNonEmptyString(requested)) { - return isArray(client.scope) ? client.scope : []; - } - const asked = requested.split(/\s+/).filter(Boolean); - const allowed = isArray(client.scope) ? client.scope : config.scopes; - if (isArray(allowed)) { - for (const s of asked) { - if (!allowed.includes(s)) { - throw new ServerError( - ProtocolError.INVALID_SCOPE, - `scope ${JSON.stringify(s)} is not permitted for this client`, - ); - } - } + return isArray(client.scope) ? (ceiling ?? []) : []; } + const asked = parseScope(requested); + assertWithinCeiling(asked, ceiling); return asked; } diff --git a/packages/oauth2/src/server/handlers/authorize.js b/packages/oauth2/src/server/handlers/authorize.js index 1706fa41..61d3ca42 100644 --- a/packages/oauth2/src/server/handlers/authorize.js +++ b/packages/oauth2/src/server/handlers/authorize.js @@ -11,7 +11,7 @@ * handler only once the user has authenticated and approved the request. * A `null` result is treated as the user declining (`access_denied`). */ -import { isArray, isNonEmptyString } from '@exortek/shared/predicates'; +import { isNonEmptyString } from '@exortek/shared/predicates'; import { randomState } from '../../internal/state.js'; import { ProtocolError, ServerError } from '../errors.js'; @@ -21,6 +21,7 @@ import { resolvePkceChallenge } from '../security/pkce.js'; import { validateResource } from '../security/resource.js'; import { validateAuthorizationDetails } from '../security/rar.js'; import { verifyRequestObject, signJarmResponse } from '../security/jar-jarm.js'; +import { assertWithinCeiling, parseScope, scopeCeiling } from '../security/scope.js'; /** * @param {import('../index.js').ResolvedServerConfig & Record} config @@ -202,9 +203,9 @@ function requireRegisteredRedirectUri(redirectUri, client) { } /** - * Requested scope must be a subset of the server's advertised scopes and, - * when the client registers its own `scope`, of that too (RFC 6749 - * §4.1.2.1). An out-of-bounds scope is `invalid_scope`. + * Requested scope must be a subset of both the server's advertised scopes + * and the client's registered `scope` (RFC 6749 §4.1.2.1). An + * out-of-bounds scope is `invalid_scope`. * * @param {string | undefined} raw * @param {import('../clients.js').Client} client @@ -213,20 +214,8 @@ function requireRegisteredRedirectUri(redirectUri, client) { * @returns {string[]} */ function resolveScope(raw, client, config, state) { - const requested = isNonEmptyString(raw) ? raw.split(/\s+/).filter(Boolean) : []; - // The client's registered scope is the tighter ceiling when present; - // otherwise the server-wide list gates. - const allowed = isArray(client.scope) ? client.scope : config.scopes; - if (isArray(allowed)) { - for (const s of requested) { - if (!allowed.includes(s)) { - throw new ServerError(ProtocolError.INVALID_SCOPE, `scope ${JSON.stringify(s)} is not permitted`, { - redirectable: true, - state, - }); - } - } - } + const requested = parseScope(raw); + assertWithinCeiling(requested, scopeCeiling(client, config), { redirectable: true, state }); return requested; } diff --git a/packages/oauth2/src/server/handlers/device.js b/packages/oauth2/src/server/handlers/device.js index a9cca167..b4dde023 100644 --- a/packages/oauth2/src/server/handlers/device.js +++ b/packages/oauth2/src/server/handlers/device.js @@ -20,6 +20,7 @@ import { randomState } from '../../internal/state.js'; import { normalizeRequest } from '../request.js'; import { json, jsonError } from '../response.js'; import { authenticateClient } from '../security/client-auth.js'; +import { assertWithinCeiling, parseScope, scopeCeiling } from '../security/scope.js'; const DEFAULT_TTL_MS = 600_000; // 10 minutes const DEFAULT_INTERVAL_SEC = 5; @@ -51,7 +52,8 @@ export function createDeviceHandler(config) { throw new ServerError(ProtocolError.UNAUTHORIZED_CLIENT, 'client may not use the device grant'); } - const scope = isNonEmptyString(req.form.scope) ? req.form.scope.split(/\s+/).filter(Boolean) : []; + const scope = parseScope(req.form.scope); + assertWithinCeiling(scope, scopeCeiling(client, config)); const deviceCode = randomState(); const userCode = generateUserCode(); diff --git a/packages/oauth2/src/server/security/scope.js b/packages/oauth2/src/server/security/scope.js new file mode 100644 index 00000000..e19f9907 --- /dev/null +++ b/packages/oauth2/src/server/security/scope.js @@ -0,0 +1,60 @@ +/** + * Scope ceiling — the single rule every grant applies to a requested scope. + * A client may only ever be granted scopes that BOTH the server advertises + * (`config.scopes`) and its own registration lists (`client.scope`). Taking + * either one alone is an escalation lever: a dynamically-registered client + * picks its own `scope`, so trusting it instead of the server list would let + * a registrant grant itself anything. + */ +import { isArray, isNonEmptyString } from '@exortek/shared/predicates'; + +import { ProtocolError, ServerError } from '../errors.js'; + +/** + * The scopes `client` may be granted, or `undefined` when neither the server + * nor the client declares a list (no ceiling configured). + * + * @param {import('../clients.js').Client} client + * @param {Record} config + * @returns {string[] | undefined} + */ +export function scopeCeiling(client, config) { + const own = isArray(client.scope) ? client.scope : undefined; + const server = isArray(config.scopes) ? config.scopes : undefined; + if (own && server) { + return own.filter(s => server.includes(s)); + } + return own ?? server; +} + +/** + * Split a space-delimited `scope` parameter (RFC 6749 §3.3). + * + * @param {unknown} raw + * @returns {string[]} + */ +export function parseScope(raw) { + return isNonEmptyString(raw) ? raw.split(/\s+/).filter(Boolean) : []; +} + +/** + * Throw `invalid_scope` for the first requested scope outside the ceiling. + * + * @param {string[]} requested + * @param {string[] | undefined} ceiling + * @param {{ redirectable?: boolean, state?: string }} [errorOptions] + */ +export function assertWithinCeiling(requested, ceiling, errorOptions) { + if (!isArray(ceiling)) { + return; + } + for (const s of requested) { + if (!ceiling.includes(s)) { + throw new ServerError( + ProtocolError.INVALID_SCOPE, + `scope ${JSON.stringify(s)} is not permitted for this client`, + errorOptions, + ); + } + } +} diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js new file mode 100644 index 00000000..6398027b --- /dev/null +++ b/packages/oauth2/tests/server-hardening.test.js @@ -0,0 +1,58 @@ +/** + * Authorization-server hardening: the scope ceiling, dynamic-registration + * limits, PAR integrity, mTLS chain checks, refresh / device lifetimes and + * token-exchange subject checks. Each test reproduces an escalation or + * bypass that used to succeed. + */ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; + +import { buildServer, getAuthorizationCode, post, body } from './helpers/server.js'; + +// SCOPE CEILING + +const WIDE_CLIENT = { + clientId: 'wide', + clientSecret: 's3cret-value-strong', + tokenEndpointAuthMethod: 'client_secret_post', + redirectUris: ['https://wide.example.com/cb'], + grantTypes: ['authorization_code', 'client_credentials', 'urn:ietf:params:oauth:grant-type:device_code'], + // Registers a scope the server never advertises. + scope: ['read', 'admin'], +}; + +test('scope ceiling: authorize refuses a client scope the server does not advertise', async () => { + const { server } = buildServer({ clients: [WIDE_CLIENT], scopes: ['read', 'write'] }); + const { res, code } = await getAuthorizationCode(server, { + clientId: 'wide', + redirectUri: 'https://wide.example.com/cb', + scope: 'read admin', + }); + assert.equal(code, null); + assert.equal(new URL(res.headers.location).searchParams.get('error'), 'invalid_scope'); +}); + +test('scope ceiling: client_credentials refuses, and never defaults to, an unadvertised scope', async () => { + const { server } = buildServer({ clients: [WIDE_CLIENT], scopes: ['read', 'write'] }); + const creds = { client_id: 'wide', client_secret: 's3cret-value-strong' }; + + const asked = await server.token(post({ grant_type: 'client_credentials', scope: 'admin', ...creds })); + assert.equal(asked.status, 400); + assert.equal(body(asked).error, 'invalid_scope'); + + const implicit = body(await server.token(post({ grant_type: 'client_credentials', ...creds }))); + assert.equal(implicit.scope, 'read'); +}); + +test('scope ceiling: device authorization validates the requested scope', async () => { + const { server } = buildServer({ + clients: [WIDE_CLIENT], + scopes: ['read', 'write'], + grants: ['urn:ietf:params:oauth:grant-type:device_code'], + }); + const res = await server.deviceAuthorization( + post({ client_id: 'wide', client_secret: 's3cret-value-strong', scope: 'admin' }), + ); + assert.equal(res.status, 400); + assert.equal(body(res).error, 'invalid_scope'); +}); diff --git a/packages/oauth2/tests/server-tierc.test.js b/packages/oauth2/tests/server-tierc.test.js index 970d007c..7dcbc64f 100644 --- a/packages/oauth2/tests/server-tierc.test.js +++ b/packages/oauth2/tests/server-tierc.test.js @@ -26,6 +26,7 @@ test('client_credentials issues a token with no refresh (RFC 6749 §4.4)', async }, ], grants: [CC], + scopes: ['api'], }); const tok = body( await server.token( @@ -40,6 +41,7 @@ test('a public client cannot use client_credentials', async () => { const { server } = buildServer({ clients: [{ clientId: 'pub', tokenEndpointAuthMethod: 'none', redirectUris: ['https://x/cb'], grantTypes: [CC] }], grants: [CC], + scopes: ['api'], }); const res = body(await server.token(post({ grant_type: CC, client_id: 'pub' }))); assert.equal(res.error, 'unauthorized_client'); @@ -59,6 +61,7 @@ test('private_key_jwt client auth with assertion replay defense (RFC 7523, threa }, ], grants: [CC], + scopes: ['api'], }); const ok = await server.token( @@ -112,6 +115,7 @@ test('client_secret_jwt client auth (HS256)', async () => { }, ], grants: [CC], + scopes: ['api'], }); const now = Math.floor(Date.now() / 1000); const assertion = await jwtSign( @@ -140,6 +144,7 @@ test('mTLS self_signed client auth binds a certificate-bound token (RFC 8705)', }, ], grants: [CC], + scopes: ['api'], }); const req = { @@ -218,6 +223,7 @@ test('token exchange delegates a subject token to a new audience (RFC 8693)', as }, ], grants: [CC, EXCHANGE], + scopes: ['api'], }); const subjectTok = body( @@ -309,6 +315,7 @@ test('a confidential client cannot authenticate with the wrong method', async () }, ], grants: [CC], + scopes: ['api'], }); // Registered basic, presenting post → refused. const res = body( From bf7f38e88ebcefeb9b24fa0ba8f0a61659df3fee Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:02:15 +0300 Subject: [PATCH 02/29] fix(oauth2): restrict what a dynamic registration may request A registrant could ask for any grant_types and any scope: the server stored them as-is, so an initial-access-token holder could register a client_credentials client with scope "admin superuser" and mint tokens. Registrant-supplied values are now checked: - grant_types against registration.allowedGrants (default authorization_code + refresh_token) - scope against the server's advertised scopes - token_endpoint_auth_method against registration.allowedAuthMethods (default every non-mTLS method) Anything else is invalid_client_metadata. The deployment's own registration.defaults stay trusted. --- .../oauth2/src/server/handlers/register.js | 64 +++++++++++++++++-- packages/oauth2/src/server/index.js | 2 + packages/oauth2/tests/registration.test.js | 50 +++++++++++++++ 3 files changed, 111 insertions(+), 5 deletions(-) diff --git a/packages/oauth2/src/server/handlers/register.js b/packages/oauth2/src/server/handlers/register.js index d30c6aab..bce6e5a5 100644 --- a/packages/oauth2/src/server/handlers/register.js +++ b/packages/oauth2/src/server/handlers/register.js @@ -22,6 +22,21 @@ import { json, jsonError } from '../response.js'; const SECRET_BASED = new Set(['client_secret_basic', 'client_secret_post', 'client_secret_jwt']); +// What a registrant may ask for unless the deployment widens it. The +// client_credentials / device / token-exchange grants act without a user +// in the loop, so a self-registered client gets them only when the +// deployment lists them in `registration.allowedGrants`. +const DEFAULT_ALLOWED_GRANTS = ['authorization_code', 'refresh_token']; +// mTLS methods need key material (subject DN / thumbprint) that RFC 7591 +// metadata here does not carry, so they are registered out of band. +const DEFAULT_ALLOWED_AUTH_METHODS = [ + 'none', + 'client_secret_basic', + 'client_secret_post', + 'client_secret_jwt', + 'private_key_jwt', +]; + /** * @param {Record} config * @returns {(raw: import('../request.js').RawRequest) => Promise} @@ -50,7 +65,7 @@ export function createRegisterHandler(config) { throw new ServerError(ProtocolError.INVALID_CLIENT_METADATA, 'client metadata is required'); } - const clientConfig = buildClientConfig(meta, reg); + const clientConfig = buildClientConfig(meta, reg, config); let client; try { client = config.registry.register(clientConfig); @@ -106,18 +121,46 @@ async function authorizeRegistration(req, reg) { * the deployment's `registration.defaults` under the request. The server owns * the generated `client_id` / `client_secret`. * + * Registrant-supplied `grant_types`, `scope` and `token_endpoint_auth_method` + * are checked against what the deployment allows; the deployment's own + * `defaults` are trusted as configured. + * * @param {Record} meta * @param {Record} reg + * @param {Record} config * @returns {import('../clients.js').ClientConfig} */ -function buildClientConfig(meta, reg) { +function buildClientConfig(meta, reg, config) { const defaults = isObject(reg.defaults) ? reg.defaults : {}; const redirectUris = meta.redirect_uris ?? defaults.redirectUris; - const authMethod = meta.token_endpoint_auth_method ?? defaults.tokenEndpointAuthMethod ?? 'client_secret_basic'; + const requestedAuthMethod = meta.token_endpoint_auth_method; + if (requestedAuthMethod !== undefined) { + const allowed = isArray(reg.allowedAuthMethods) ? reg.allowedAuthMethods : DEFAULT_ALLOWED_AUTH_METHODS; + if (!allowed.includes(requestedAuthMethod)) { + throw new ServerError( + ProtocolError.INVALID_CLIENT_METADATA, + `token_endpoint_auth_method ${JSON.stringify(requestedAuthMethod)} is not permitted for registration`, + ); + } + } + const authMethod = requestedAuthMethod ?? defaults.tokenEndpointAuthMethod ?? 'client_secret_basic'; + + const requestedGrants = toArray(meta.grant_types); + if (requestedGrants) { + const allowed = isArray(reg.allowedGrants) ? reg.allowedGrants : DEFAULT_ALLOWED_GRANTS; + for (const grant of requestedGrants) { + if (!allowed.includes(grant)) { + throw new ServerError( + ProtocolError.INVALID_CLIENT_METADATA, + `grant_type ${JSON.stringify(grant)} is not permitted for registration`, + ); + } + } + } // RFC 7591 §2 / §3.2.2: a client using a redirect-based grant must supply // at least one redirect URI. - const grantTypes = toArray(meta.grant_types) ?? defaults.grantTypes ?? ['authorization_code']; + const grantTypes = requestedGrants ?? defaults.grantTypes ?? ['authorization_code']; const usesRedirect = grantTypes.includes('authorization_code'); if (usesRedirect && (!isArray(redirectUris) || redirectUris.length === 0)) { throw new ServerError(ProtocolError.INVALID_REDIRECT_URI, 'redirect_uris is required for this grant'); @@ -137,7 +180,18 @@ function buildClientConfig(meta, reg) { if (responseTypes) { cfg.responseTypes = responseTypes; } - const scope = isNonEmptyString(meta.scope) ? meta.scope.split(/\s+/).filter(Boolean) : defaults.scope; + const requestedScope = isNonEmptyString(meta.scope) ? meta.scope.split(/\s+/).filter(Boolean) : undefined; + if (requestedScope && isArray(config.scopes)) { + for (const s of requestedScope) { + if (!config.scopes.includes(s)) { + throw new ServerError( + ProtocolError.INVALID_CLIENT_METADATA, + `scope ${JSON.stringify(s)} is not supported by this server`, + ); + } + } + } + const scope = requestedScope ?? defaults.scope; if (isArray(scope)) { cfg.scope = scope; } diff --git a/packages/oauth2/src/server/index.js b/packages/oauth2/src/server/index.js index 50cca383..fcf752e1 100644 --- a/packages/oauth2/src/server/index.js +++ b/packages/oauth2/src/server/index.js @@ -78,6 +78,8 @@ const DEFAULT_REFRESH_TTL = '30d'; * @property {boolean} [open] allow unauthenticated registration (NOT recommended) * @property {(req: import('./request.js').ServerRequest) => boolean | Promise} [authorize] custom gate * @property {Partial} [defaults] defaults layered under each registration + * @property {string[]} [allowedGrants] grant types a registrant may request (default `['authorization_code','refresh_token']`) + * @property {string[]} [allowedAuthMethods] token-endpoint auth methods a registrant may request (default: every non-mTLS method) * * @typedef {Object} OidcConfig * @property {import('./issuers/id-token.js').IdTokenSignerConfig} idToken id_token JWS signer (signingKey + alg) diff --git a/packages/oauth2/tests/registration.test.js b/packages/oauth2/tests/registration.test.js index 0a57487e..cd72ee8d 100644 --- a/packages/oauth2/tests/registration.test.js +++ b/packages/oauth2/tests/registration.test.js @@ -108,3 +108,53 @@ test('open registration is allowed only when explicitly opened', async () => { const reg = body(await server.register(jsonPost({ redirect_uris: ['https://o/cb'] }))); assert.ok(reg.client_id); }); + +test('a registrant cannot pick client_credentials or an unadvertised scope', async () => { + const { server } = buildRegistrationServer({ initialAccessToken: IAT }); + const auth = { authorization: `Bearer ${IAT}` }; + + const grant = await server.register( + jsonPost({ grant_types: ['client_credentials'], token_endpoint_auth_method: 'client_secret_post' }, auth), + ); + assert.equal(grant.status, 400); + assert.equal(body(grant).error, 'invalid_client_metadata'); + + const scope = await server.register( + jsonPost({ redirect_uris: ['https://c.example.com/cb'], scope: 'admin superuser' }, auth), + ); + assert.equal(scope.status, 400); + assert.equal(body(scope).error, 'invalid_client_metadata'); + + const method = await server.register( + jsonPost({ redirect_uris: ['https://c.example.com/cb'], token_endpoint_auth_method: 'tls_client_auth' }, auth), + ); + assert.equal(method.status, 400); + assert.equal(body(method).error, 'invalid_client_metadata'); +}); + +test('registration.allowedGrants opts a deployment into more grants', async () => { + const { server } = buildRegistrationServer({ + initialAccessToken: IAT, + allowedGrants: ['client_credentials'], + }); + const reg = body( + await server.register( + jsonPost( + { + redirect_uris: ['https://c.example.com/cb'], + grant_types: ['client_credentials'], + token_endpoint_auth_method: 'client_secret_post', + scope: 'read', + }, + { authorization: `Bearer ${IAT}` }, + ), + ), + ); + assert.ok(reg.client_id); + const tok = body( + await server.token( + post({ grant_type: 'client_credentials', client_id: reg.client_id, client_secret: reg.client_secret }), + ), + ); + assert.equal(tok.scope, 'read'); +}); From 6d16ff35384bbb9af559f633a3f5e9f6870c122a Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:02:42 +0300 Subject: [PATCH 03/29] fix(oauth2): validate client redirect URIs at definition time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit defineClient only checked that each redirect URI was a non-empty string, so a dynamic registration could register javascript:, plain http or a fragment-bearing URI. Because authorize redirects its errors to a registered URI, that made the error path an open redirect. A redirect URI must now be absolute and fragment-free, and use https, http on a loopback host, or a reverse-domain private-use scheme (RFC 8252 §7.1). --- packages/oauth2/src/server/clients.js | 52 ++++++++++++++++++++-- packages/oauth2/tests/registration.test.js | 14 ++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/packages/oauth2/src/server/clients.js b/packages/oauth2/src/server/clients.js index 8aab6764..25779880 100644 --- a/packages/oauth2/src/server/clients.js +++ b/packages/oauth2/src/server/clients.js @@ -36,7 +36,7 @@ export const AUTH_METHODS = Object.freeze([ * @typedef {Object} ClientConfig * @property {string} clientId * @property {string} [clientSecret] confidential clients only - * @property {string[]} redirectUris exact-match allowlist (RFC 6749 §3.1.2) + * @property {string[]} redirectUris exact-match allowlist (RFC 6749 §3.1.2); https, loopback http, or a reverse-domain private-use scheme * @property {string[]} [grantTypes] default `['authorization_code','refresh_token']` * @property {string[]} [responseTypes] default `['code']` * @property {string} [tokenEndpointAuthMethod] one of {@link AUTH_METHODS} @@ -76,9 +76,7 @@ export function defineClient(config) { ); } for (const uri of config.redirectUris) { - if (!isNonEmptyString(uri)) { - throw new OAuth2Error(ErrorCode.INVALID_ARGUMENT, `client ${config.clientId} has an invalid redirectUri`); - } + assertRedirectUri(uri, config.clientId); } const authMethod = @@ -132,6 +130,52 @@ export function defineClient(config) { }); } +const LOOPBACK_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]']); + +/** + * A redirect URI must be absolute with no fragment (RFC 6749 §3.1.2) and use + * a transport that cannot be intercepted (OAuth 2.1 §2.3.1 / RFC 8252): + * `https`, `http` on a loopback host, or a reverse-domain private-use scheme + * such as `com.example.app:/cb`. Anything else — `javascript:`, `data:`, + * plain `http`, a fragment — is refused at definition time, so neither a + * static client nor a dynamic registration can turn the authorize error + * redirect into an open redirect. + * + * @param {unknown} uri + * @param {string} clientId + */ +function assertRedirectUri(uri, clientId) { + const invalid = reason => + new OAuth2Error(ErrorCode.INVALID_ARGUMENT, `client ${clientId} has an invalid redirectUri: ${reason}`); + if (!isNonEmptyString(uri)) { + throw invalid('must be a non-empty string'); + } + let url; + try { + url = new URL(uri); + } catch { + throw invalid(`${JSON.stringify(uri)} is not an absolute URI`); + } + if (uri.includes('#')) { + throw invalid('must not contain a fragment'); + } + const scheme = url.protocol.slice(0, -1); + if (scheme === 'https') { + return; + } + if (scheme === 'http') { + if (LOOPBACK_HOSTS.has(url.hostname)) { + return; + } + throw invalid('http is only allowed for a loopback host'); + } + // RFC 8252 §7.1: a private-use scheme is a reverse domain name. + if (scheme.includes('.')) { + return; + } + throw invalid(`scheme ${JSON.stringify(scheme)} is not allowed`); +} + /** * @typedef {Object} ClientRegistry * @property {(clientId: string) => (Client | undefined) | Promise} getClient diff --git a/packages/oauth2/tests/registration.test.js b/packages/oauth2/tests/registration.test.js index cd72ee8d..a3cdc58a 100644 --- a/packages/oauth2/tests/registration.test.js +++ b/packages/oauth2/tests/registration.test.js @@ -158,3 +158,17 @@ test('registration.allowedGrants opts a deployment into more grants', async () = ); assert.equal(tok.scope, 'read'); }); + +test('a registered redirect_uri must be https, loopback http, or a private-use scheme', async () => { + const { server } = buildRegistrationServer({ initialAccessToken: IAT }); + const auth = { authorization: `Bearer ${IAT}` }; + for (const uri of ['javascript:alert(1)', 'http://evil.example/cb', 'https://c.example.com/cb#frag', 'myapp:/cb']) { + const res = await server.register(jsonPost({ redirect_uris: [uri] }, auth)); + assert.equal(res.status, 400, uri); + assert.equal(body(res).error, 'invalid_client_metadata', uri); + } + for (const uri of ['http://127.0.0.1:8080/cb', 'com.example.app:/oauth/cb']) { + const res = await server.register(jsonPost({ redirect_uris: [uri] }, auth)); + assert.equal(res.status, 201, uri); + } +}); From 7473a4dd68a68e8a9e2669bf121593ef7273b3e3 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:03:31 +0300 Subject: [PATCH 04/29] fix(oauth2): carry the PAR flag out of band MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit authorize decided whether a request came through PAR by reading params._fromPar, but the same object held the raw query string, form body and request-object claims. Sending ?_fromPar=1 skipped a PAR requirement (security.par.required, requirePushedAuthorizationRequests, FAPI 2.0) and returned a code. resolveRequestParams now returns { params, fromPar } and the flag never lives in the parameters. Also: - request_uri must be redeemed with the client_id that pushed it (RFC 9126 §4); a missing or different client_id is invalid_request - a request object pushed to PAR is verified at push time and its claims are stored, instead of storing the unverified JWT --- .../oauth2/src/server/handlers/authorize.js | 25 ++++++-- packages/oauth2/src/server/handlers/par.js | 13 +++- packages/oauth2/tests/audit.test.js | 2 +- .../oauth2/tests/server-hardening.test.js | 60 ++++++++++++++++++- packages/oauth2/tests/server.test.js | 4 +- 5 files changed, 91 insertions(+), 13 deletions(-) diff --git a/packages/oauth2/src/server/handlers/authorize.js b/packages/oauth2/src/server/handlers/authorize.js index 61d3ca42..3206fddf 100644 --- a/packages/oauth2/src/server/handlers/authorize.js +++ b/packages/oauth2/src/server/handlers/authorize.js @@ -33,7 +33,7 @@ export function createAuthorizeHandler(config) { // Untrusted until the client + redirect URI are validated. let redirectUri; try { - const params = await resolveRequestParams(req, config); + const { params, fromPar } = await resolveRequestParams(req, config); const client = await requireClient(params.client_id, config); redirectUri = requireRegisteredRedirectUri(params.redirect_uri, client); @@ -69,7 +69,7 @@ export function createAuthorizeHandler(config) { } } // PAR is required by the server OR by this client's registration. - if ((config.requirePar || client.requirePushedAuthorizationRequests === true) && !params._fromPar) { + if ((config.requirePar || client.requirePushedAuthorizationRequests === true) && !fromPar) { throw new ServerError(ProtocolError.INVALID_REQUEST, 'this client requires a pushed authorization request', { redirectable: true, state, @@ -143,9 +143,14 @@ export function createAuthorizeHandler(config) { * (RFC 9126); the JAR signed-request-object path (RFC 9101) is layered in * `security/jar-jarm.js`. Plain front-channel query params are the base. * + * Whether the params came from PAR is returned out of band (`fromPar`), + * never as a parameter: anything inside `params` is attacker-reachable + * (query string, form body, request object), so a flag carried there could + * be forged to skip a PAR requirement. + * * @param {import('../request.js').ServerRequest} req * @param {Record} config - * @returns {Promise>} + * @returns {Promise<{ params: Record, fromPar: boolean }>} */ async function resolveRequestParams(req, config) { // RFC 6749 §3.1 allows the authorization request over POST too; Apple's @@ -155,17 +160,25 @@ async function resolveRequestParams(req, config) { const requestUri = src.request_uri; if (isNonEmptyString(requestUri)) { + // RFC 9126 §4: client_id is still sent on the front channel and must + // name the client that pushed the request. + if (!isNonEmptyString(src.client_id)) { + throw new ServerError(ProtocolError.INVALID_REQUEST, 'client_id is required alongside request_uri'); + } const pushed = await config.stores.par.consume(requestUri); if (!pushed) { throw new ServerError(ProtocolError.INVALID_REQUEST, 'request_uri is unknown, expired, or already used'); } - return { ...pushed, _fromPar: true }; + if (pushed.client_id !== src.client_id) { + throw new ServerError(ProtocolError.INVALID_REQUEST, 'request_uri was pushed by a different client'); + } + return { params: { ...pushed }, fromPar: true }; } // JAR (RFC 9101): a signed request object supersedes the query params. if (isNonEmptyString(src.request)) { - return verifyRequestObject(src.request, src.client_id, config); + return { params: await verifyRequestObject(src.request, src.client_id, config), fromPar: false }; } - return { ...src }; + return { params: { ...src }, fromPar: false }; } /** diff --git a/packages/oauth2/src/server/handlers/par.js b/packages/oauth2/src/server/handlers/par.js index ebf855f2..42b35a5c 100644 --- a/packages/oauth2/src/server/handlers/par.js +++ b/packages/oauth2/src/server/handlers/par.js @@ -19,6 +19,7 @@ import { normalizeRequest } from '../request.js'; import { json, jsonError } from '../response.js'; import { authenticateClient } from '../security/client-auth.js'; import { resolveDpopBinding } from '../security/dpop.js'; +import { verifyRequestObject } from '../security/jar-jarm.js'; const REQUEST_URI_PREFIX = 'urn:ietf:params:oauth:request_uri:'; @@ -56,7 +57,13 @@ export function createParHandler(config) { // (RFC 9449 §10.1) via `dpop_jkt`. const { dpopJkt } = await resolveDpopBinding(req, config, client, { htu: config.endpoints.par }); - const params = collectParams(req.form, client.clientId, dpopJkt); + // RFC 9126 §3: a pushed request may itself be a JAR request object. + // Verify it now, while the client is authenticated, and store the + // parameters it carries — never the unverified JWT. + const source = isNonEmptyString(req.form.request) + ? await verifyRequestObject(req.form.request, client.clientId, config) + : req.form; + const params = collectParams(source, client.clientId, dpopJkt); const requestUri = REQUEST_URI_PREFIX + randomState(); await config.stores.par.save(requestUri, params, config.parTtlMs); @@ -74,7 +81,7 @@ export function createParHandler(config) { } /** - * @param {Record} form + * @param {Record} form * @param {string} clientId * @param {string | undefined} dpopJkt * @returns {Record} @@ -83,7 +90,7 @@ function collectParams(form, clientId, dpopJkt) { /** @type {Record} */ const params = {}; for (const [key, value] of Object.entries(form)) { - if (!CLIENT_AUTH_PARAMS.has(key)) { + if (!CLIENT_AUTH_PARAMS.has(key) && key !== 'request') { params[key] = value; } } diff --git a/packages/oauth2/tests/audit.test.js b/packages/oauth2/tests/audit.test.js index 1195b22c..8d8a3352 100644 --- a/packages/oauth2/tests/audit.test.js +++ b/packages/oauth2/tests/audit.test.js @@ -373,7 +373,7 @@ test('D3: async (Promise-returning) PAR / auth-code stores drive a full flow', a ), ); assert.ok(pushed.request_uri.startsWith('urn:ietf:params:oauth:request_uri:')); - const authed = await server.authorize(get('/authorize', { request_uri: pushed.request_uri })); + const authed = await server.authorize(get('/authorize', { client_id: 'app', request_uri: pushed.request_uri })); assert.ok(new URL(authed.headers.location).searchParams.get('code')); }); diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 6398027b..1dff770e 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -7,7 +7,7 @@ import assert from 'node:assert/strict'; import { test } from 'node:test'; -import { buildServer, getAuthorizationCode, post, body } from './helpers/server.js'; +import { buildServer, getAuthorizationCode, get, post, body } from './helpers/server.js'; // SCOPE CEILING @@ -56,3 +56,61 @@ test('scope ceiling: device authorization validates the requested scope', async assert.equal(res.status, 400); assert.equal(body(res).error, 'invalid_scope'); }); + +// PAR + +const PKCE_PARAMS = { + redirect_uri: 'https://app.example.com/cb', + response_type: 'code', + code_challenge: 'a'.repeat(43), + code_challenge_method: 'S256', +}; + +test('PAR: a forged _fromPar parameter does not satisfy a PAR requirement', async () => { + const { server } = buildServer({ security: { par: { required: true } } }); + const res = await server.authorize(get('/authorize', { client_id: 'app', _fromPar: '1', ...PKCE_PARAMS })); + const location = new URL(res.headers.location); + assert.equal(location.searchParams.get('code'), null); + assert.equal(location.searchParams.get('error'), 'invalid_request'); +}); + +test('PAR: request_uri must be redeemed with the client_id that pushed it', async () => { + const other = { + clientId: 'other', + clientSecret: 'other-secret-value-strong', + tokenEndpointAuthMethod: 'client_secret_post', + redirectUris: ['https://app.example.com/cb'], + }; + const { server } = buildServer({ + clients: [ + { + clientId: 'app', + clientSecret: 's3cret-value-strong', + tokenEndpointAuthMethod: 'client_secret_post', + redirectUris: ['https://app.example.com/cb'], + }, + other, + ], + }); + const pushed = body( + await server.par(post({ client_id: 'app', client_secret: 's3cret-value-strong', ...PKCE_PARAMS })), + ); + + const missing = await server.authorize(get('/authorize', { request_uri: pushed.request_uri })); + assert.equal(body(missing).error, 'invalid_request'); + + const pushedAgain = body( + await server.par(post({ client_id: 'app', client_secret: 's3cret-value-strong', ...PKCE_PARAMS })), + ); + const mismatch = await server.authorize( + get('/authorize', { client_id: 'other', request_uri: pushedAgain.request_uri }), + ); + assert.equal(body(mismatch).error, 'invalid_request'); +}); + +test('PAR: a pushed request object is verified at push time', async () => { + const { server } = buildServer(); + const res = await server.par(post({ client_id: 'app', client_secret: 's3cret-value-strong', request: 'not.a.jwt' })); + assert.equal(res.status, 400); + assert.equal(body(res).error, 'invalid_request'); +}); diff --git a/packages/oauth2/tests/server.test.js b/packages/oauth2/tests/server.test.js index 196a9108..c2f34a34 100644 --- a/packages/oauth2/tests/server.test.js +++ b/packages/oauth2/tests/server.test.js @@ -271,10 +271,10 @@ test('PAR pushes params and the request_uri is single-use', async () => { ); assert.ok(pushed.request_uri.startsWith('urn:ietf:params:oauth:request_uri:')); - const ok = await server.authorize(get('/authorize', { request_uri: pushed.request_uri })); + const ok = await server.authorize(get('/authorize', { client_id: 'app', request_uri: pushed.request_uri })); assert.ok(new URL(ok.headers.location).searchParams.get('code')); - const reuse = await server.authorize(get('/authorize', { request_uri: pushed.request_uri })); + const reuse = await server.authorize(get('/authorize', { client_id: 'app', request_uri: pushed.request_uri })); assert.equal(body(reuse).error, 'invalid_request'); }); From e1e0dcfba604a784bd7e06d258675660508b022a Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:04:55 +0300 Subject: [PATCH 05/29] fix(oauth2): require a verified chain for tls_client_auth The adapters read getPeerCertificate() but never socket.authorized, and client auth trusted the subject DN string alone. A listener run with rejectUnauthorized:false (which self_signed_tls_client_auth needs) therefore accepted any self-signed certificate carrying the registered subject. The DN was also built by joining unescaped values, so CN=foo,O=Bank and CN="foo,O=Bank" compared equal. - normalizeRequest exposes clientCertificateAuthorized; the express and fastify adapters set it from socket.authorized, and tls_client_auth refuses a certificate whose chain the TLS layer did not verify - the registered DN is parsed as RFC 4514 and compared attribute by attribute with the certificate subject (order-insensitive) - self_signed_tls_client_auth compares thumbprints in constant time --- .../oauth2/src/server/middleware/express.js | 15 ++ .../oauth2/src/server/middleware/fastify.js | 15 ++ packages/oauth2/src/server/request.js | 3 + .../oauth2/src/server/security/client-auth.js | 50 ++++--- packages/oauth2/src/server/security/dn.js | 135 ++++++++++++++++++ .../oauth2/tests/server-hardening.test.js | 75 ++++++++++ 6 files changed, 267 insertions(+), 26 deletions(-) create mode 100644 packages/oauth2/src/server/security/dn.js diff --git a/packages/oauth2/src/server/middleware/express.js b/packages/oauth2/src/server/middleware/express.js index 43aa8f0d..bca2833f 100644 --- a/packages/oauth2/src/server/middleware/express.js +++ b/packages/oauth2/src/server/middleware/express.js @@ -148,6 +148,7 @@ function toRawRequest(req) { query: req.query, body: req.body, clientCertificate: readPeerCertificate(req), + clientCertificateAuthorized: peerCertificateAuthorized(req), }; } @@ -166,3 +167,17 @@ function readPeerCertificate(req) { } return undefined; } + +/** + * Whether the TLS layer verified the client certificate against the + * server's trusted CAs. A listener run with `rejectUnauthorized: false` + * (needed for `self_signed_tls_client_auth`) still accepts unverified + * certificates, so `tls_client_auth` must not trust the subject DN alone. + * + * @param {any} req + * @returns {boolean} + */ +function peerCertificateAuthorized(req) { + const socket = req.socket ?? req.connection; + return socket?.authorized === true; +} diff --git a/packages/oauth2/src/server/middleware/fastify.js b/packages/oauth2/src/server/middleware/fastify.js index f383f2bf..773961da 100644 --- a/packages/oauth2/src/server/middleware/fastify.js +++ b/packages/oauth2/src/server/middleware/fastify.js @@ -138,6 +138,7 @@ function adapt(handler) { query: request.query, body: request.body, clientCertificate: readPeerCertificate(request), + clientCertificateAuthorized: peerCertificateAuthorized(request), }); } catch (err) { out = errorResponse(err); @@ -165,6 +166,20 @@ function readPeerCertificate(request) { return undefined; } +/** + * Whether the TLS layer verified the client certificate against the + * server's trusted CAs. A listener run with `rejectUnauthorized: false` + * (needed for `self_signed_tls_client_auth`) still accepts unverified + * certificates, so `tls_client_auth` must not trust the subject DN alone. + * + * @param {any} request + * @returns {boolean} + */ +function peerCertificateAuthorized(request) { + const socket = request.socket ?? request.raw?.socket; + return socket?.authorized === true; +} + export const oauth2ServerPlugin = fastifyPlugin(oauth2ServerPluginFn, { fastify: '>=4', name: '@exortek/oauth2-server', diff --git a/packages/oauth2/src/server/request.js b/packages/oauth2/src/server/request.js index d4bc1442..0ae718e3 100644 --- a/packages/oauth2/src/server/request.js +++ b/packages/oauth2/src/server/request.js @@ -24,6 +24,7 @@ import { ProtocolError, ServerError } from './errors.js'; * @property {Record} [query] pre-parsed query, else derived from `url` * @property {string | Record} [body] raw string or a framework-parsed object * @property {import('node:tls').PeerCertificate | Record} [clientCertificate] mTLS client cert (RFC 8705) + * @property {boolean} [clientCertificateAuthorized] true only when the TLS layer verified the cert chain (`socket.authorized`) * * @typedef {Object} ServerRequest * @property {string} method @@ -33,6 +34,7 @@ import { ProtocolError, ServerError } from './errors.js'; * @property {(name: string) => string | undefined} header * @property {(name: string) => string | undefined} param query ∪ form (form wins) * @property {object | undefined} clientCertificate + * @property {boolean} clientCertificateAuthorized chain verified by the TLS layer (required for `tls_client_auth`) */ /** @@ -57,6 +59,7 @@ export function normalizeRequest(raw) { query, form, clientCertificate: isObject(raw.clientCertificate) ? raw.clientCertificate : undefined, + clientCertificateAuthorized: raw.clientCertificateAuthorized === true, header(name) { return headers[String(name).toLowerCase()]; }, diff --git a/packages/oauth2/src/server/security/client-auth.js b/packages/oauth2/src/server/security/client-auth.js index 2bace8b0..f3835411 100644 --- a/packages/oauth2/src/server/security/client-auth.js +++ b/packages/oauth2/src/server/security/client-auth.js @@ -25,6 +25,7 @@ import { isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates import { ProtocolError, ServerError } from '../errors.js'; import { readClientCredentials } from '../request.js'; import { resolveClientKeys } from './client-keys.js'; +import { parseDn, sameDn, subjectAttributes } from './dn.js'; import { createReplayCache } from './replay-cache.js'; const JWT_BEARER_ASSERTION = 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer'; @@ -245,10 +246,17 @@ async function authenticateByCertificate(req, client) { } if (client.tokenEndpointAuthMethod === 'tls_client_auth') { - // PKI mTLS: the certificate's subject DN must match the registered - // expected value (RFC 8705 §2.1). - const subject = certSubjectString(cert); - if (!isNonEmptyString(client.tlsClientAuthSubjectDn) || client.tlsClientAuthSubjectDn !== subject) { + // PKI mTLS (RFC 8705 §2.1): the chain must have been verified by the TLS + // layer — otherwise any self-signed certificate carrying the right + // subject would pass — and the subject DN must match the registered one. + if (req.clientCertificateAuthorized !== true) { + throw new ServerError(ProtocolError.INVALID_CLIENT, 'client certificate chain was not verified'); + } + const presented = subjectAttributes(/** @type {any} */ (cert).subject); + const registered = isNonEmptyString(client.tlsClientAuthSubjectDn) + ? parseDn(client.tlsClientAuthSubjectDn) + : undefined; + if (!presented || !registered || !sameDn(presented, registered)) { throw new ServerError( ProtocolError.INVALID_CLIENT, 'client certificate subject does not match the registered value', @@ -257,10 +265,10 @@ async function authenticateByCertificate(req, client) { return client; } if (client.tokenEndpointAuthMethod === 'self_signed_tls_client_auth') { - // Self-signed mTLS: the certificate thumbprint must be present in the - // client's registered JWKS (RFC 8705 §2.2). + // Self-signed mTLS: the certificate itself is the credential, so its + // thumbprint must equal the registered one (RFC 8705 §2.2). const thumb = certThumbprint(cert); - if (!certInJwks(client, thumb)) { + if (!thumbprintMatches(client, thumb)) { throw new ServerError(ProtocolError.INVALID_CLIENT, 'client certificate is not registered for this client'); } return client; @@ -283,29 +291,19 @@ function certThumbprint(cert) { return base64urlEncode(createHash('sha256').update(der).digest()); } -/** @param {object} cert @returns {string} */ -function certSubjectString(cert) { - const subject = /** @type {any} */ (cert).subject; - if (isNonEmptyString(subject)) { - return subject; - } - if (isObject(subject)) { - // Node parses the subject into an object; render a stable DN string. - return Object.entries(subject) - .map(([k, v]) => `${k}=${v}`) - .join(','); - } - throw new ServerError(ProtocolError.INVALID_CLIENT, 'client certificate has no subject'); -} - /** + * Compare the presented certificate's thumbprint with the one registered + * for a `self_signed_tls_client_auth` client (RFC 8705 §2.2). + * * @param {import('../clients.js').Client} client * @param {string} thumb * @returns {boolean} */ -function certInJwks(client, thumb) { - if (isNonEmptyString(client.certificateThumbprint)) { - return client.certificateThumbprint === thumb; +function thumbprintMatches(client, thumb) { + if (!isNonEmptyString(client.certificateThumbprint)) { + return false; } - return false; + const a = Buffer.from(client.certificateThumbprint, 'utf8'); + const b = Buffer.from(thumb, 'utf8'); + return a.length === b.length && timingSafeEqual(a, b); } diff --git a/packages/oauth2/src/server/security/dn.js b/packages/oauth2/src/server/security/dn.js new file mode 100644 index 00000000..b88a32c6 --- /dev/null +++ b/packages/oauth2/src/server/security/dn.js @@ -0,0 +1,135 @@ +/** + * Distinguished-name comparison for `tls_client_auth` (RFC 8705 §2.1.2). + * The registered `tlsClientAuthSubjectDn` is an RFC 4514 string; Node hands + * the certificate subject over as an object of attribute values. Both are + * reduced to a set of `TYPE=value` attributes and compared structurally, so + * an escaped comma inside a value (`CN=foo\,O=Bank`) can never collide with + * an extra attribute (`CN=foo,O=Bank`), and attribute order does not matter. + */ +import { isArray, isNonEmptyString, isObject, isString } from '@exortek/shared/predicates'; + +/** + * Parse an RFC 4514 DN string into attributes. + * + * @param {string} dn + * @returns {{ type: string, value: string }[] | undefined} undefined when malformed + */ +export function parseDn(dn) { + /** @type {{ type: string, value: string }[]} */ + const out = []; + let type = ''; + /** @type {number[] | null} value bytes while reading a value, null while reading a type */ + let value = null; + let trailingSpaces = 0; + + const flush = () => { + if (value === null) { + return false; + } + const t = type.trim(); + if (t.length === 0) { + return false; + } + const bytes = trailingSpaces > 0 ? value.slice(0, value.length - trailingSpaces) : value; + out.push({ type: t.toUpperCase(), value: Buffer.from(bytes).toString('utf8') }); + type = ''; + value = null; + trailingSpaces = 0; + return true; + }; + + for (let i = 0; i < dn.length; i++) { + const ch = dn[i]; + if (value === null) { + if (ch === '=') { + value = []; + } else if (ch === ',' || ch === '+' || ch === '\\') { + return undefined; + } else { + type += ch; + } + continue; + } + if (ch === '\\') { + const next = dn[i + 1]; + if (next === undefined) { + return undefined; + } + if (/[0-9a-fA-F]/.test(next) && /[0-9a-fA-F]/.test(dn[i + 2] ?? '')) { + value.push(Number.parseInt(dn.slice(i + 1, i + 3), 16)); + i += 2; + } else { + value.push(...Buffer.from(next, 'utf8')); + i += 1; + } + trailingSpaces = 0; + continue; + } + if (ch === ',' || ch === '+') { + if (!flush()) { + return undefined; + } + continue; + } + if (ch === ' ' && value.length === 0) { + // Unescaped leading space after `=` is insignificant. + continue; + } + if (ch === '#' && value.length === 0) { + // Hex-encoded BER values are not supported for matching. + return undefined; + } + value.push(...Buffer.from(ch, 'utf8')); + trailingSpaces = ch === ' ' ? trailingSpaces + 1 : 0; + } + if (!flush()) { + return undefined; + } + return out; +} + +/** + * Attributes of a certificate subject — Node's parsed object (multi-valued + * attributes arrive as arrays) or an RFC 4514 string. + * + * @param {unknown} subject + * @returns {{ type: string, value: string }[] | undefined} + */ +export function subjectAttributes(subject) { + if (isNonEmptyString(subject)) { + return parseDn(subject); + } + if (!isObject(subject)) { + return undefined; + } + /** @type {{ type: string, value: string }[]} */ + const out = []; + for (const [type, raw] of Object.entries(subject)) { + for (const value of isArray(raw) ? raw : [raw]) { + if (!isString(value)) { + return undefined; + } + out.push({ type: type.toUpperCase(), value }); + } + } + return out; +} + +/** + * Whether two attribute lists name the same DN (order-insensitive). + * + * @param {{ type: string, value: string }[]} a + * @param {{ type: string, value: string }[]} b + * @returns {boolean} + */ +export function sameDn(a, b) { + if (a.length === 0 || a.length !== b.length) { + return false; + } + const key = attrs => + attrs + .map(({ type, value }) => JSON.stringify([type, value])) + .sort() + .join('\n'); + return key(a) === key(b); +} diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 1dff770e..44051bbd 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -114,3 +114,78 @@ test('PAR: a pushed request object is verified at push time', async () => { assert.equal(res.status, 400); assert.equal(body(res).error, 'invalid_request'); }); + +// mTLS (tls_client_auth) + +const PKI_CLIENT = { + clientId: 'pki', + tokenEndpointAuthMethod: 'tls_client_auth', + tlsClientAuthSubjectDn: 'CN=pki-client,O=Bank', + redirectUris: ['https://x.example.com/cb'], + grantTypes: ['client_credentials'], + scope: ['read'], +}; + +function mtlsRequest(subject, authorized) { + return { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ grant_type: 'client_credentials', client_id: 'pki' }).toString(), + clientCertificate: { raw: Buffer.from('der'), subject }, + clientCertificateAuthorized: authorized, + }; +} + +test('tls_client_auth: an unverified certificate with a matching DN is refused', async () => { + const { server } = buildServer({ clients: [PKI_CLIENT], grants: ['client_credentials'] }); + const res = await server.token(mtlsRequest({ CN: 'pki-client', O: 'Bank' }, false)); + assert.equal(res.status, 401); + assert.equal(body(res).error, 'invalid_client'); + + const ok = await server.token(mtlsRequest({ O: 'Bank', CN: 'pki-client' }, true)); + assert.equal(ok.status, 200); +}); + +test('tls_client_auth: DN values are compared structurally, not as joined strings', async () => { + const { server } = buildServer({ + clients: [{ ...PKI_CLIENT, tlsClientAuthSubjectDn: 'CN=foo\\,O=Bank' }], + grants: ['client_credentials'], + }); + // Joined naively this subject renders as "CN=foo,O=Bank" too. + const forged = await server.token(mtlsRequest({ CN: 'foo', O: 'Bank' }, true)); + assert.equal(forged.status, 401); + + const real = await server.token(mtlsRequest({ CN: 'foo,O=Bank' }, true)); + assert.equal(real.status, 200); +}); + +test('express adapter passes socket.authorized through', async () => { + const { server } = buildServer({ clients: [PKI_CLIENT], grants: ['client_credentials'] }); + const { expressHandler } = await import('../src/server/middleware/express.js'); + const handle = expressHandler(server.token); + const statuses = []; + for (const authorized of [false, true]) { + const socket = { + authorized, + getPeerCertificate: () => ({ raw: Buffer.from('der'), subject: { CN: 'pki-client', O: 'Bank' } }), + }; + const req = { + method: 'POST', + url: '/token', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: { grant_type: 'client_credentials', client_id: 'pki' }, + query: {}, + socket, + }; + const res = { + status(code) { + statuses.push(code); + return this; + }, + setHeader() {}, + send() {}, + }; + await handle(req, res); + } + assert.deepEqual(statuses, [401, 200]); +}); From 03e91020d8b8574b93e548ff0b95f28f99c99cf4 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:06:02 +0300 Subject: [PATCH 06/29] fix(shared): keep the TTL when a record-store update has none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Redis record store rewrote an updated record with a plain SET when no ttlMs was passed, which clears the key's expiry. oauth2's refresh rotation calls update(old, { used: true }) that way, so every rotated refresh token (and its reuse-detection marker) became immortal. A TTL store now rewrites with SET … KEEPTTL (dispatched per driver dialect) unless the caller passes a new TTL. --- packages/shared/src/record-store.js | 14 +++++++++++-- packages/shared/src/redis-helpers.js | 12 +++++++++++ packages/shared/tests/record-store.test.js | 23 ++++++++++++++++++++-- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/packages/shared/src/record-store.js b/packages/shared/src/record-store.js index 5edc0779..627de493 100644 --- a/packages/shared/src/record-store.js +++ b/packages/shared/src/record-store.js @@ -149,7 +149,7 @@ export function createMemoryRecordStore(config) { * @returns {{ * put: (record: object, ttlMs?: number) => Promise, * getById: (id: string) => Promise, - * update: (id: string, patch: object, ttlMs?: number) => Promise, + * update: (id: string, patch: object, ttlMs?: number) => Promise, without `ttlMs` a TTL store keeps the key's remaining expiry * listByIndex: (indexValue: string) => Promise, * readRecord: (id: string) => Promise, * writeRecord: (record: object, ttlMs?: number) => Promise, @@ -214,6 +214,16 @@ export function createRedisRecordStore(client, config) { } } + // An update with no explicit TTL keeps the key's remaining expiry + // instead of clearing it. + async function rewriteRecord(record, ttlMs) { + if (ttl && ttlMs === undefined) { + await helpers.setKeepTTL(rk(record[idField]), JSON.stringify(record)); + return; + } + await writeRecord(record, ttlMs); + } + async function writeTombstone(id, data, ttlMs) { const value = JSON.stringify(data); if (ttl && ttlMs !== undefined) { @@ -279,7 +289,7 @@ export function createRedisRecordStore(client, config) { } const oldIndex = existing[indexField]; const next = { ...existing, ...patch }; - await writeRecord(next, ttlMs); + await rewriteRecord(next, ttlMs); if (!isUndefined(patch[indexField]) && patch[indexField] !== oldIndex) { if (isString(oldIndex)) { await indexRemove(oldIndex, id); diff --git a/packages/shared/src/redis-helpers.js b/packages/shared/src/redis-helpers.js index cb22317d..40b4d05e 100644 --- a/packages/shared/src/redis-helpers.js +++ b/packages/shared/src/redis-helpers.js @@ -74,6 +74,7 @@ export function evalScript(client, script, keys, args, dialect = detectDialect(c * smembers: (key: string) => Promise, * setWithTTL: (key: string, value: string, ttlMs: number) => Promise, * setPlain: (key: string, value: string) => Promise, + * setKeepTTL: (key: string, value: string) => Promise, * parseRecord: (raw: string|object|null) => object|null, * }} */ @@ -143,6 +144,17 @@ export function createRedisHelpers(client) { await client.set(key, value); }, + // Overwrite a value but keep whatever expiry the key already has + // (Redis ≥ 6.0 `SET … KEEPTTL`). A plain SET clears the TTL, so an + // update to an expiring record would otherwise make it immortal. + async setKeepTTL(key, value) { + if (dialect === 'ioredis') { + await client.set(key, value, 'KEEPTTL'); + } else { + await client.set(key, value, { KEEPTTL: true }); + } + }, + parseRecord(raw) { if (!raw) { return null; diff --git a/packages/shared/tests/record-store.test.js b/packages/shared/tests/record-store.test.js index 351a1460..6a9a2e59 100644 --- a/packages/shared/tests/record-store.test.js +++ b/packages/shared/tests/record-store.test.js @@ -101,15 +101,17 @@ describe('createMemoryRecordStore', () => { // Redis -function createFakeRedisClient() { +function createFakeRedisClient(extra = {}) { const data = new Map(); const sets = new Map(); + const calls = []; return { async get(key) { return data.get(key) ?? null; }, - async set(key, value) { + async set(key, value, ...opts) { data.set(key, value); + calls.push(opts); }, async del(key) { data.delete(key); @@ -130,6 +132,8 @@ function createFakeRedisClient() { }, _data: data, _sets: sets, + _setOptions: calls, + ...extra, }; } @@ -246,4 +250,19 @@ describe('createRedisRecordStore', () => { await store.writeTombstone('k1', { at: 1 }); assert.ok(client._data.has('ml:t:k1')); }); + + test('update without ttlMs keeps the key expiry on a TTL store (both dialects)', async () => { + for (const [extra, expected] of [ + [{ status: 'ready' }, ['KEEPTTL']], + [{}, [{ KEEPTTL: true }]], + ]) { + const client = createFakeRedisClient(extra); + const store = createRedisRecordStore(client, { idField: 'id', indexField: 'userId', ttl: true }); + await store.put({ id: 'k1', userId: 'u1' }, 60_000); + await store.update('k1', { used: true }); + assert.deepEqual(client._setOptions.at(-1), expected); + await store.update('k1', { used: true }, 5_000); + assert.notDeepEqual(client._setOptions.at(-1), expected); + } + }); }); From b39b67fe5c7e3b1e89294b0eef6a516ce41932e3 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:07:13 +0300 Subject: [PATCH 07/29] fix(oauth2): rotated refresh tokens keep the family expiry The refresh grant built the successor record without expiresAt, so the first rotation produced a token that never expired: the memory store treated it as valid forever and the Redis store saved it with no TTL. The successor now inherits the family's absolute expiry (a record written before expiries were carried gets now + refreshTokenTtl). Covered for the grant, the fake Redis client (which now honours PX / KEEPTTL like real Redis) and the live Redis suite on both drivers. --- .../oauth2/src/server/grants/refresh-token.js | 8 +++- .../oauth2/tests/server-hardening.test.js | 43 +++++++++++++++++++ .../tests/stores/redis.integration.test.js | 13 ++++++ packages/oauth2/tests/stores/redis.test.js | 16 +++++++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/packages/oauth2/src/server/grants/refresh-token.js b/packages/oauth2/src/server/grants/refresh-token.js index 7eb6557a..7d5a1642 100644 --- a/packages/oauth2/src/server/grants/refresh-token.js +++ b/packages/oauth2/src/server/grants/refresh-token.js @@ -5,7 +5,7 @@ * revokes the whole family. Scope may be narrowed but never widened, and * a DPoP-bound token stays bound to the same key. */ -import { isNonEmptyString } from '@exortek/shared/predicates'; +import { isNonEmptyString, isNumber } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; import { randomState } from '../../internal/state.js'; @@ -72,6 +72,11 @@ export async function refreshTokenGrant(req, ctx) { // reuse check before either marks it used. The security guarantee that // still holds unconditionally is the important one — any later replay of // the now-rotated token trips reuse detection and burns the family. + // + // The successor inherits the family's absolute expiry: rotation renews the + // token, not the grant, so a refresh family still dies `refreshTokenTtl` + // after the user authorized it. (A record written before expiries were + // carried gets a fresh one rather than none.) const nextToken = randomState(); await config.stores.refresh.rotate(presented, { token: nextToken, @@ -81,6 +86,7 @@ export async function refreshTokenGrant(req, ctx) { subject: record.subject, resource: record.resource, dpopJkt: record.dpopJkt, + expiresAt: isNumber(record.expiresAt) ? record.expiresAt : Date.now() + config.refreshTokenTtlMs, }); body.refresh_token = nextToken; diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 44051bbd..d9438966 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -189,3 +189,46 @@ test('express adapter passes socket.authorized through', async () => { } assert.deepEqual(statuses, [401, 200]); }); + +// REFRESH ROTATION + +async function issueRefresh(server) { + const { code, pkce, redirectUri } = await getAuthorizationCode(server, { scope: 'read write' }); + return body( + await server.token( + post({ + grant_type: 'authorization_code', + code, + redirect_uri: redirectUri, + client_id: 'app', + client_secret: 's3cret-value-strong', + code_verifier: pkce.codeVerifier, + }), + ), + ); +} + +function refresh(server, token, extra = {}) { + return server.token( + post({ + grant_type: 'refresh_token', + refresh_token: token, + client_id: 'app', + client_secret: 's3cret-value-strong', + ...extra, + }), + ); +} + +test('refresh: a rotated token keeps the family expiry', async () => { + const { server } = buildServer({ security: { refreshTokenTtl: 80 } }); + const first = await issueRefresh(server); + const rotated = body(await refresh(server, first.refresh_token)); + assert.ok(rotated.refresh_token); + + const record = await server._config.stores.refresh.get(rotated.refresh_token); + assert.equal(typeof record.expiresAt, 'number'); + + await new Promise(r => setTimeout(r, 100)); + assert.equal(body(await refresh(server, rotated.refresh_token)).error, 'invalid_grant'); +}); diff --git a/packages/oauth2/tests/stores/redis.integration.test.js b/packages/oauth2/tests/stores/redis.integration.test.js index 311d4d20..dd54b7fa 100644 --- a/packages/oauth2/tests/stores/redis.integration.test.js +++ b/packages/oauth2/tests/stores/redis.integration.test.js @@ -44,6 +44,19 @@ forEachRedisDriver('oauth2 server stores', ({ test, client, ns, raw }) => { assert.equal((await store.get('t2')).revoked, true); }); + test('refresh rotation keeps both tokens on a TTL', async () => { + const keyPrefix = ns('c2'); + const store = createRedisRefreshStore(client(), { keyPrefix }); + const base = { familyId: 'f1', clientId: 'app', scope: [], expiresAt: Date.now() + 60_000 }; + + await store.save({ ...base, token: 't1' }); + await store.rotate('t1', { ...base, token: 't2' }); + for (const token of ['t1', 't2']) { + const pttl = await raw.pttl(`${keyPrefix}refresh:${token}`); + assert.ok(pttl > 0 && pttl <= 60_000, `${token} must keep an expiry; got PTTL ${pttl}`); + } + }); + test('an expired refresh record reads as absent', async () => { const store = createRedisRefreshStore(client(), { keyPrefix: ns('d') }); await store.save({ token: 't1', familyId: 'f1', clientId: 'app', scope: [], expiresAt: Date.now() - 1 }); diff --git a/packages/oauth2/tests/stores/redis.test.js b/packages/oauth2/tests/stores/redis.test.js index 7690bfc2..a163a5e4 100644 --- a/packages/oauth2/tests/stores/redis.test.js +++ b/packages/oauth2/tests/stores/redis.test.js @@ -27,11 +27,17 @@ function fakeClient() { async get(k) { return live(k) ? kv.get(k) : null; }, + // Mirrors real SET semantics for both driver call forms: PX sets an + // expiry, KEEPTTL keeps it, anything else clears it. async set(k, v, ...rest) { kv.set(k, v); + const opts = typeof rest[0] === 'object' && rest[0] !== null ? rest[0] : null; const ix = rest.indexOf('PX'); if (ix !== -1) ttl.set(k, Date.now() + Number(rest[ix + 1])); + else if (opts?.PX !== undefined) ttl.set(k, Date.now() + Number(opts.PX)); + else if (!rest.includes('KEEPTTL') && !opts?.KEEPTTL) ttl.delete(k); }, + _ttl: ttl, async del(k) { kv.delete(k); ttl.delete(k); @@ -80,6 +86,16 @@ test('redis refresh store: rotate marks used and revokeFamily burns the family', assert.equal((await store.get('t2')).revoked, true); }); +test('redis refresh store: rotation keeps both tokens on a TTL', async () => { + const client = fakeClient(); + const store = createRedisRefreshStore(client, { keyPrefix: 't:' }); + const expiresAt = Date.now() + 60_000; + await store.save({ token: 'r1', familyId: 'f', clientId: 'app', scope: [], expiresAt }); + await store.rotate('r1', { token: 'r2', familyId: 'f', clientId: 'app', scope: [], expiresAt }); + assert.ok(client._ttl.has('t:refresh:r1'), 'rotated-away token lost its TTL'); + assert.ok(client._ttl.has('t:refresh:r2'), 'successor stored without a TTL'); +}); + test('redis refresh store: an expired record reads as absent', async () => { const store = createRedisRefreshStore(fakeClient()); await store.save({ token: 't1', familyId: 'f1', clientId: 'app', scope: [], expiresAt: Date.now() - 1 }); From 9e7a308dd6bdc8639fbb4171e2323aa95a18ddfa Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:07:37 +0300 Subject: [PATCH 08/29] feat(shared): setIfAbsent redis helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SET … PX … NX dispatched per driver dialect, resolving true only for the caller that created the key — a cross-process compare-and-set for single-winner claims. Adds unit coverage for the dialect dispatch of setIfAbsent and setKeepTTL. --- packages/shared/src/redis-helpers.js | 13 ++++++ packages/shared/tests/redis-helpers.test.js | 48 +++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 packages/shared/tests/redis-helpers.test.js diff --git a/packages/shared/src/redis-helpers.js b/packages/shared/src/redis-helpers.js index 40b4d05e..660395b9 100644 --- a/packages/shared/src/redis-helpers.js +++ b/packages/shared/src/redis-helpers.js @@ -75,6 +75,7 @@ export function evalScript(client, script, keys, args, dialect = detectDialect(c * setWithTTL: (key: string, value: string, ttlMs: number) => Promise, * setPlain: (key: string, value: string) => Promise, * setKeepTTL: (key: string, value: string) => Promise, + * setIfAbsent: (key: string, value: string, ttlMs: number) => Promise, * parseRecord: (raw: string|object|null) => object|null, * }} */ @@ -155,6 +156,18 @@ export function createRedisHelpers(client) { } }, + // Atomic claim: `SET key value PX ttl NX`. Resolves true when this call + // created the key, false when it already existed — a cross-process + // compare-and-set for "only the first caller wins". + async setIfAbsent(key, value, ttlMs) { + const px = Math.max(1, Math.ceil(ttlMs)); + const reply = + dialect === 'ioredis' + ? await client.set(key, value, 'PX', px, 'NX') + : await client.set(key, value, { PX: px, NX: true }); + return reply === 'OK'; + }, + parseRecord(raw) { if (!raw) { return null; diff --git a/packages/shared/tests/redis-helpers.test.js b/packages/shared/tests/redis-helpers.test.js new file mode 100644 index 00000000..cc0309cf --- /dev/null +++ b/packages/shared/tests/redis-helpers.test.js @@ -0,0 +1,48 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { createRedisHelpers } from '../src/redis-helpers.js'; + +/** Records SET calls; `existing` keys make an NX set fail like Redis does. */ +function recorder(extra = {}, existing = new Set()) { + const calls = []; + return { + calls, + client: { + async set(key, value, ...opts) { + calls.push(opts); + const nx = opts.includes('NX') || opts[0]?.NX === true; + if (nx && existing.has(key)) { + return null; + } + existing.add(key); + return 'OK'; + }, + ...extra, + }, + }; +} + +test('setIfAbsent dispatches NX per dialect and reports who won', async () => { + const io = recorder({ status: 'ready' }); + const ioHelpers = createRedisHelpers(io.client); + assert.equal(await ioHelpers.setIfAbsent('k', '1', 1000), true); + assert.equal(await ioHelpers.setIfAbsent('k', '1', 1000), false); + assert.deepEqual(io.calls[0], ['PX', 1000, 'NX']); + + const nr = recorder(); + const nrHelpers = createRedisHelpers(nr.client); + assert.equal(await nrHelpers.setIfAbsent('k', '1', 1000), true); + assert.equal(await nrHelpers.setIfAbsent('k', '1', 1000), false); + assert.deepEqual(nr.calls[0], [{ PX: 1000, NX: true }]); +}); + +test('setKeepTTL dispatches KEEPTTL per dialect', async () => { + const io = recorder({ status: 'ready' }); + await createRedisHelpers(io.client).setKeepTTL('k', 'v'); + assert.deepEqual(io.calls[0], ['KEEPTTL']); + + const nr = recorder(); + await createRedisHelpers(nr.client).setKeepTTL('k', 'v'); + assert.deepEqual(nr.calls[0], [{ KEEPTTL: true }]); +}); From a82e2fda3d39ad11f644ba730927b72a78d6c763 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:08:23 +0300 Subject: [PATCH 09/29] fix(oauth2): make refresh rotation a compare-and-set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refresh grant ran get → reuse check → rotate as separate steps, so two simultaneous redemptions of the same token both passed the check and forked the family into two live branches. rotate() is now a compare-and-set that resolves false when the token was already claimed: the memory store flips used in one synchronous step, the Redis store claims a per-token key with SET NX that lives as long as the family. The grant rotates before issuing anything and treats a lost race as reuse (the family is revoked). Custom stores that resolve nothing keep working as before. The memory refresh store also sweeps expired records in the background instead of keeping them forever. --- .../oauth2/src/server/grants/refresh-token.js | 38 +++++++------- packages/oauth2/src/server/stores.js | 51 +++++++++++++++---- packages/oauth2/src/server/stores/redis.js | 12 +++++ .../oauth2/tests/server-hardening.test.js | 23 +++++++++ .../tests/stores/redis.integration.test.js | 12 +++++ packages/oauth2/tests/stores/redis.test.js | 10 +++- 6 files changed, 115 insertions(+), 31 deletions(-) diff --git a/packages/oauth2/src/server/grants/refresh-token.js b/packages/oauth2/src/server/grants/refresh-token.js index 7d5a1642..edbc484c 100644 --- a/packages/oauth2/src/server/grants/refresh-token.js +++ b/packages/oauth2/src/server/grants/refresh-token.js @@ -55,30 +55,17 @@ export async function refreshTokenGrant(req, ctx) { const scope = narrowScope(req.form.scope, record.scope); const audience = resolveTokenAudience(req.form.resource, record.resource, config); - const body = await buildAccessResponse(config, { - subject: record.subject, - clientId: client.clientId, - scope, - audience, - dpopJkt: record.dpopJkt, - }); - - // Rotate: mint the successor in the same family and mark the presented - // token used so a later presentation trips the reuse check above. - // - // The get→check→rotate sequence assumes a single in-flight redemption per - // token (the normal client behaviour). It is not a compare-and-set: two - // truly-simultaneous redemptions of the SAME token could both pass the - // reuse check before either marks it used. The security guarantee that - // still holds unconditionally is the important one — any later replay of - // the now-rotated token trips reuse detection and burns the family. + // Rotate before issuing anything: mint the successor in the same family + // and claim the presented token. The store's rotate is a compare-and-set, + // so of two simultaneous redemptions of the SAME token exactly one wins; + // the loser is handled exactly like a replay and burns the family. // // The successor inherits the family's absolute expiry: rotation renews the // token, not the grant, so a refresh family still dies `refreshTokenTtl` // after the user authorized it. (A record written before expiries were // carried gets a fresh one rather than none.) const nextToken = randomState(); - await config.stores.refresh.rotate(presented, { + const rotated = await config.stores.refresh.rotate(presented, { token: nextToken, familyId: record.familyId, clientId: client.clientId, @@ -88,6 +75,21 @@ export async function refreshTokenGrant(req, ctx) { dpopJkt: record.dpopJkt, expiresAt: isNumber(record.expiresAt) ? record.expiresAt : Date.now() + config.refreshTokenTtlMs, }); + if (rotated === false) { + await config.stores.refresh.revokeFamily(record.familyId); + throw new ServerError( + ProtocolError.INVALID_GRANT, + 'refresh token reuse detected; the token family has been revoked', + ); + } + + const body = await buildAccessResponse(config, { + subject: record.subject, + clientId: client.clientId, + scope, + audience, + dpopJkt: record.dpopJkt, + }); body.refresh_token = nextToken; return body; diff --git a/packages/oauth2/src/server/stores.js b/packages/oauth2/src/server/stores.js index 976a581a..6a847ad6 100644 --- a/packages/oauth2/src/server/stores.js +++ b/packages/oauth2/src/server/stores.js @@ -143,34 +143,63 @@ export function createAuthCodeStore() { * @typedef {Object} RefreshStore * @property {(record: RefreshRecord) => Promise} save * @property {(token: string) => Promise} get - * @property {(oldToken: string, next: RefreshRecord) => Promise} rotate + * @property {(oldToken: string, next: RefreshRecord) => Promise} rotate + * Atomically claim `oldToken` and persist `next`. Resolve `false` when the + * token was already rotated or revoked (a concurrent redemption won), so the + * grant treats it as reuse. A store that resolves nothing is treated as a + * successful rotation (pre-CAS custom stores). * @property {(familyId: string) => Promise} revokeFamily */ -/** @returns {RefreshStore} */ -export function createRefreshStore() { +/** + * @param {{ sweepIntervalMs?: number }} [options] + * @returns {RefreshStore} + */ +export function createRefreshStore({ sweepIntervalMs = 60_000 } = {}) { const store = createMemoryRecordStore({ idField: 'token', indexField: 'familyId' }); + + // The record store has no TTL of its own: evict expired tokens in the + // background so a family that is never presented again cannot leak. + const sweep = setInterval(() => { + const now = Date.now(); + for (const [token, record] of store.byId) { + const { expiresAt, familyId } = /** @type {RefreshRecord} */ (record); + if (isNumber(expiresAt) && expiresAt <= now) { + store.byId.delete(token); + const members = store.byIndex.get(familyId); + members?.delete(token); + if (members?.size === 0) { + store.byIndex.delete(familyId); + } + } + } + }, sweepIntervalMs); + if (isFunction(sweep.unref)) { + sweep.unref(); + } + return { async save(record) { await store.put(record); }, async get(token) { const record = /** @type {RefreshRecord | null} */ (await store.getById(token)); - // An expired refresh token is treated as absent (the memory record - // store has no TTL of its own); a Redis-backed store would let the - // key expire instead. + // An expired refresh token is treated as absent. if (record && isNumber(record.expiresAt) && record.expiresAt <= Date.now()) { return null; } return record; }, - // Mark the presented token used and persist its successor. If the - // presented token was ALREADY used, the caller detects reuse from - // `get` first and calls `revokeFamily` instead — this only runs on a - // legitimate rotation. + // Compare-and-set: the check and the `used` flip happen in one + // synchronous step, so of two concurrent redemptions exactly one wins. async rotate(oldToken, next) { - await store.update(oldToken, { used: true }); + const existing = /** @type {RefreshRecord | undefined} */ (store.byId.get(oldToken)); + if (!existing || existing.used === true || existing.revoked === true) { + return false; + } + existing.used = true; await store.put(next); + return true; }, async revokeFamily(familyId) { const members = await store.listByIndex(familyId); diff --git a/packages/oauth2/src/server/stores/redis.js b/packages/oauth2/src/server/stores/redis.js index 1457cea3..189dc70a 100644 --- a/packages/oauth2/src/server/stores/redis.js +++ b/packages/oauth2/src/server/stores/redis.js @@ -27,6 +27,8 @@ import { createRedisRecordStore } from '@exortek/shared/record-store'; import { isFunction } from '@exortek/shared/predicates'; const DEFAULT_PREFIX = 'oauth2:'; +// Claim-key lifetime for a family that carries no expiry (legacy records). +const CLAIM_FALLBACK_TTL_MS = 30 * 24 * 60 * 60 * 1000; /** * GET-then-DEL a key, atomically when the client supports `GETDEL` @@ -117,6 +119,7 @@ export function createRedisRefreshStore(client, options = {}) { /** @param {{ expiresAt?: number }} record */ const ttlOf = record => typeof record.expiresAt === 'number' ? Math.max(1, record.expiresAt - Date.now()) : undefined; + const claimKey = token => `${options.keyPrefix ?? DEFAULT_PREFIX}refresh:claim:${token}`; return { async save(record) { @@ -129,9 +132,18 @@ export function createRedisRefreshStore(client, options = {}) { } return record; }, + // Compare-and-set across nodes: the first redemption creates the claim + // key (SET NX) and wins; a concurrent one finds it and resolves false, + // which the grant treats as reuse. The claim lives as long as the + // family, so a late replay is still caught after the record is gone. async rotate(oldToken, next) { + const won = await store.helpers.setIfAbsent(claimKey(oldToken), '1', ttlOf(next) ?? CLAIM_FALLBACK_TTL_MS); + if (!won) { + return false; + } await store.update(oldToken, { used: true }); await store.put(next, ttlOf(next)); + return true; }, async revokeFamily(familyId) { const members = await store.listByIndex(familyId); diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index d9438966..fa3c2459 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -232,3 +232,26 @@ test('refresh: a rotated token keeps the family expiry', async () => { await new Promise(r => setTimeout(r, 100)); assert.equal(body(await refresh(server, rotated.refresh_token)).error, 'invalid_grant'); }); + +test('refresh: two simultaneous redemptions cannot fork the family', async () => { + const { server } = buildServer(); + const first = await issueRefresh(server); + const results = (await Promise.all([refresh(server, first.refresh_token), refresh(server, first.refresh_token)])).map( + body, + ); + const winners = results.filter(r => r.refresh_token); + assert.equal(winners.length, 1); + assert.equal(results.find(r => !r.refresh_token).error, 'invalid_grant'); + // The race is treated as reuse: the winner's successor is burned too. + assert.equal(body(await refresh(server, winners[0].refresh_token)).error, 'invalid_grant'); +}); + +test('refresh: the memory store evicts expired tokens in the background', async () => { + const { createRefreshStore } = await import('../src/server/stores.js'); + const store = createRefreshStore({ sweepIntervalMs: 10 }); + await store.save({ token: 'r1', familyId: 'f1', clientId: 'app', scope: [], expiresAt: Date.now() + 5 }); + await new Promise(r => setTimeout(r, 40)); + assert.equal(await store.get('r1'), null); + await store.revokeFamily('f1'); + assert.equal(await store.get('r1'), null); +}); diff --git a/packages/oauth2/tests/stores/redis.integration.test.js b/packages/oauth2/tests/stores/redis.integration.test.js index dd54b7fa..65b301d0 100644 --- a/packages/oauth2/tests/stores/redis.integration.test.js +++ b/packages/oauth2/tests/stores/redis.integration.test.js @@ -57,6 +57,18 @@ forEachRedisDriver('oauth2 server stores', ({ test, client, ns, raw }) => { } }); + test('refresh rotate is a compare-and-set across clients', async () => { + const store = createRedisRefreshStore(client(), { keyPrefix: ns('c3') }); + const base = { familyId: 'f1', clientId: 'app', scope: [], expiresAt: Date.now() + 60_000 }; + await store.save({ ...base, token: 't1' }); + + const outcomes = await Promise.all([ + store.rotate('t1', { ...base, token: 't2a' }), + store.rotate('t1', { ...base, token: 't2b' }), + ]); + assert.deepEqual(outcomes.sort(), [false, true]); + }); + test('an expired refresh record reads as absent', async () => { const store = createRedisRefreshStore(client(), { keyPrefix: ns('d') }); await store.save({ token: 't1', familyId: 'f1', clientId: 'app', scope: [], expiresAt: Date.now() - 1 }); diff --git a/packages/oauth2/tests/stores/redis.test.js b/packages/oauth2/tests/stores/redis.test.js index a163a5e4..67b9a01f 100644 --- a/packages/oauth2/tests/stores/redis.test.js +++ b/packages/oauth2/tests/stores/redis.test.js @@ -27,15 +27,21 @@ function fakeClient() { async get(k) { return live(k) ? kv.get(k) : null; }, - // Mirrors real SET semantics for both driver call forms: PX sets an - // expiry, KEEPTTL keeps it, anything else clears it. + // Mirrors real SET semantics for both driver call forms: NX refuses an + // existing key, PX sets an expiry, KEEPTTL keeps it, anything else + // clears it. async set(k, v, ...rest) { + const nx = rest.includes('NX') || rest[0]?.NX === true; + if (nx && live(k)) { + return null; + } kv.set(k, v); const opts = typeof rest[0] === 'object' && rest[0] !== null ? rest[0] : null; const ix = rest.indexOf('PX'); if (ix !== -1) ttl.set(k, Date.now() + Number(rest[ix + 1])); else if (opts?.PX !== undefined) ttl.set(k, Date.now() + Number(opts.PX)); else if (!rest.includes('KEEPTTL') && !opts?.KEEPTTL) ttl.delete(k); + return 'OK'; }, _ttl: ttl, async del(k) { From d37626eb2f9e64d2b6090eb1c7c1a87b722fbb99 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:08:33 +0300 Subject: [PATCH 10/29] fix(oauth2): keep the granted scope on a rotated refresh token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refresh request with a narrower scope also narrowed the successor refresh token, permanently losing the rest of the grant. RFC 6749 §6 requires the new refresh token's scope to equal the presented one's; the narrowing now applies to the issued access token only. --- packages/oauth2/src/server/grants/refresh-token.js | 4 +++- packages/oauth2/tests/server-hardening.test.js | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/oauth2/src/server/grants/refresh-token.js b/packages/oauth2/src/server/grants/refresh-token.js index edbc484c..0f26e79d 100644 --- a/packages/oauth2/src/server/grants/refresh-token.js +++ b/packages/oauth2/src/server/grants/refresh-token.js @@ -69,7 +69,9 @@ export async function refreshTokenGrant(req, ctx) { token: nextToken, familyId: record.familyId, clientId: client.clientId, - scope, + // RFC 6749 §6: the new refresh token's scope MUST be identical to the + // presented one's — a narrowed request narrows this access token only. + scope: record.scope, subject: record.subject, resource: record.resource, dpopJkt: record.dpopJkt, diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index fa3c2459..6dc4f3ab 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -255,3 +255,12 @@ test('refresh: the memory store evicts expired tokens in the background', async await store.revokeFamily('f1'); assert.equal(await store.get('r1'), null); }); + +test('refresh: a narrowed request does not narrow the successor refresh token', async () => { + const { server } = buildServer(); + const first = await issueRefresh(server); + const narrowed = body(await refresh(server, first.refresh_token, { scope: 'read' })); + assert.equal(narrowed.scope, 'read'); + const next = body(await refresh(server, narrowed.refresh_token, { scope: 'write' })); + assert.equal(next.scope, 'write'); +}); From 7ae9c539305349ec7b85480306365cb4ca3425e0 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:09:29 +0300 Subject: [PATCH 11/29] fix(oauth2): token exchange checks who may exchange the subject token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token-exchange grant accepted any active access token: no check that it was issued to (or for) the exchanging client, any audience was accepted, and a DPoP- or certificate-bound subject token came back bound to the exchanger's key instead. Any client allowed the grant could launder a token it had picked up into a fresh one for any audience. - the subject token's client_id or aud must name the exchanger, or its may_act claim must permit it (RFC 8693 §4.4) - a sender-constrained subject token (cnf.jkt / cnf.x5t#S256) needs a request proving the same key or certificate - resource / audience must be listed in tokenExchange.audiences (default: none) - scope also stays within the exchanger's own scope ceiling - certificate-bound exchangers get certificate-bound tokens, and an existing act chain nests under the new actor (RFC 8693 §4.1) --- .../src/server/grants/token-exchange.js | 102 +++++++++++++++--- packages/oauth2/src/server/index.js | 2 + .../oauth2/tests/server-hardening.test.js | 88 ++++++++++++++- packages/oauth2/tests/server-tierc.test.js | 1 + 4 files changed, 178 insertions(+), 15 deletions(-) diff --git a/packages/oauth2/src/server/grants/token-exchange.js b/packages/oauth2/src/server/grants/token-exchange.js index 66e1a6ef..b4223f08 100644 --- a/packages/oauth2/src/server/grants/token-exchange.js +++ b/packages/oauth2/src/server/grants/token-exchange.js @@ -4,13 +4,20 @@ * and exchanges it for a new access token, optionally scoped down and * re-audienced for a downstream `resource`/`audience`. The subject token * is validated through the issuer strategy; a valid one yields a token - * for the same subject, never a privilege escalation (scope can only - * narrow, audience must be an authorized target). + * for the same subject, never a privilege escalation: + * - the subject token must have been issued to the exchanging client, + * name it in `aud`, or permit it through `may_act` (RFC 8693 §4.4) — + * a token picked up elsewhere cannot be laundered into a fresh one; + * - a sender-constrained subject token (`cnf`) is exchanged only by the + * holder of the bound key / certificate; + * - scope only narrows, and stays within the exchanger's own ceiling; + * - the target audience must be on `tokenExchange.audiences`. */ -import { isNonEmptyString } from '@exortek/shared/predicates'; +import { isArray, isNonEmptyString, isObject } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; import { validateResource } from '../security/resource.js'; +import { assertWithinCeiling, parseScope, scopeCeiling } from '../security/scope.js'; import { buildAccessResponse } from './_issue.js'; const ACCESS_TOKEN_TYPE = 'urn:ietf:params:oauth:token-type:access_token'; @@ -46,9 +53,19 @@ export async function tokenExchangeGrant(req, ctx) { throw new ServerError(ProtocolError.INVALID_GRANT, 'subject_token is not valid'); } - const grantedScope = isNonEmptyString(claims.scope) ? claims.scope.split(/\s+/).filter(Boolean) : []; - const scope = narrowScope(req.form.scope, grantedScope); - const audience = resolveTarget(req.form.resource, req.form.audience); + assertMayExchange(claims, client); + assertHolderOfKey(claims, ctx); + + const grantedScope = parseScope(claims.scope); + const ceiling = scopeCeiling(client, config); + let scope; + if (isNonEmptyString(req.form.scope)) { + scope = narrowScope(req.form.scope, grantedScope); + assertWithinCeiling(scope, ceiling); + } else { + scope = isArray(ceiling) ? grantedScope.filter(s => ceiling.includes(s)) : grantedScope; + } + const audience = resolveTarget(req.form.resource, req.form.audience, config); const body = await buildAccessResponse(config, { subject: claims.sub, @@ -56,8 +73,10 @@ export async function tokenExchangeGrant(req, ctx) { scope, audience, dpopJkt: ctx.dpopJkt, - // RFC 8693 §4.1: record the acting party in the issued token. - extra: { act: { sub: client.clientId } }, + confirmation: ctx.confirmation, + // RFC 8693 §4.1: the current actor is the top-level `act`; any prior + // actor chain from the subject token nests beneath it. + extra: { act: isObject(claims.act) ? { sub: client.clientId, act: claims.act } : { sub: client.clientId } }, }); // RFC 8693 §2.2.1 requires issued_token_type on the response. body.issued_token_type = ACCESS_TOKEN_TYPE; @@ -85,20 +104,75 @@ function narrowScope(requested, granted) { return asked; } +/** + * The exchanging client must be the subject token's own client, one of its + * audiences, or the actor its `may_act` claim authorizes. + * + * @param {Record} claims + * @param {import('../clients.js').Client} client + */ +function assertMayExchange(claims, client) { + const id = client.clientId; + const audiences = isArray(claims.aud) ? claims.aud : [claims.aud]; + const mayAct = isObject(claims.may_act) ? claims.may_act : undefined; + if (claims.client_id === id || audiences.includes(id) || mayAct?.client_id === id || mayAct?.sub === id) { + return; + } + throw new ServerError(ProtocolError.INVALID_GRANT, 'subject_token was not issued to or for this client'); +} + +/** + * A sender-constrained subject token can only be exchanged by a request + * that proves the same key (DPoP `cnf.jkt`, RFC 9449) or certificate + * (`cnf.x5t#S256`, RFC 8705) — otherwise exchanging would strip the + * binding off a stolen token. + * + * @param {Record} claims + * @param {{ dpopJkt?: string, confirmation?: Record }} ctx + */ +function assertHolderOfKey(claims, ctx) { + if (!isObject(claims.cnf)) { + return; + } + const { jkt } = claims.cnf; + const x5t = claims.cnf['x5t#S256']; + if (isNonEmptyString(jkt) && ctx.dpopJkt !== jkt) { + throw new ServerError( + ProtocolError.INVALID_GRANT, + 'subject_token is DPoP-bound to a key this request did not prove', + ); + } + if (isNonEmptyString(x5t) && ctx.confirmation?.['x5t#S256'] !== x5t) { + throw new ServerError( + ProtocolError.INVALID_GRANT, + 'subject_token is bound to a client certificate this request did not present', + ); + } +} + /** * The exchange target — a `resource` URI (RFC 8707) or an `audience` - * identifier (RFC 8693). A malformed `resource` is `invalid_target`. + * identifier (RFC 8693). A malformed `resource` is `invalid_target`, and so + * is any target missing from `tokenExchange.audiences`: without an + * allowlist, a token can only be exchanged for one addressed to the client. * * @param {string | undefined} resource * @param {string | undefined} audience + * @param {Record} config * @returns {string | undefined} */ -function resolveTarget(resource, audience) { +function resolveTarget(resource, audience, config) { + let target; if (isNonEmptyString(resource)) { - return validateResource(resource); + target = validateResource(resource); + } else if (isNonEmptyString(audience)) { + target = audience; + } else { + return undefined; } - if (isNonEmptyString(audience)) { - return audience; + const allowed = config.tokenExchange?.audiences; + if (!isArray(allowed) || !allowed.includes(target)) { + throw new ServerError(ProtocolError.INVALID_TARGET, `exchange target ${JSON.stringify(target)} is not permitted`); } - return undefined; + return target; } diff --git a/packages/oauth2/src/server/index.js b/packages/oauth2/src/server/index.js index fcf752e1..89830939 100644 --- a/packages/oauth2/src/server/index.js +++ b/packages/oauth2/src/server/index.js @@ -72,6 +72,7 @@ const DEFAULT_REFRESH_TTL = '30d'; * @property {ServerSecurity} [security] * @property {OidcConfig} [oidc] make this an OpenID Provider (issue id_token on the `openid` scope) * @property {RegistrationConfig} [registration] enable RFC 7591 dynamic client registration (opt-in) + * @property {{ audiences?: string[] }} [tokenExchange] RFC 8693: the `resource` / `audience` values a token may be exchanged for (default: none) * * @typedef {Object} RegistrationConfig * @property {string} [initialAccessToken] bearer required to register (secure default; omit only with `open`) @@ -197,6 +198,7 @@ export function createServer(config) { refreshTokenTtlMs: parseDuration(security.refreshTokenTtl ?? DEFAULT_REFRESH_TTL), authorizationDetailsTypes: config.authorizationDetailsTypes, introspection: isObject(config.introspection) ? config.introspection : {}, + tokenExchange: isObject(config.tokenExchange) ? config.tokenExchange : {}, // JARM is enabled only when fully configured: without a signing key // `signJarmResponse` can't sign, so advertising / accepting jwt mode would // fail at runtime. An under-configured `jarm` is treated as absent. diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 6dc4f3ab..5487c23a 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -7,7 +7,7 @@ import assert from 'node:assert/strict'; import { test } from 'node:test'; -import { buildServer, getAuthorizationCode, get, post, body } from './helpers/server.js'; +import { buildServer, getAuthorizationCode, dpopClient, get, post, body, ISSUER } from './helpers/server.js'; // SCOPE CEILING @@ -264,3 +264,89 @@ test('refresh: a narrowed request does not narrow the successor refresh token', const next = body(await refresh(server, narrowed.refresh_token, { scope: 'write' })); assert.equal(next.scope, 'write'); }); + +// TOKEN EXCHANGE + +const EXCHANGE = 'urn:ietf:params:oauth:grant-type:token-exchange'; +const ACCESS_TOKEN_TYPE = 'urn:ietf:params:oauth:token-type:access_token'; + +function exchangeServer(extra = {}) { + const svc = id => ({ + clientId: id, + clientSecret: `${id}-secret-value-strong`, + tokenEndpointAuthMethod: 'client_secret_post', + redirectUris: ['https://x.example.com/cb'], + grantTypes: ['client_credentials', EXCHANGE], + scope: ['api'], + }); + return buildServer({ + clients: [svc('alice'), svc('mallory')], + grants: ['client_credentials', EXCHANGE], + scopes: ['api'], + tokenExchange: { audiences: ['https://downstream.example.com'] }, + ...extra, + }); +} + +const creds = id => ({ client_id: id, client_secret: `${id}-secret-value-strong` }); + +async function ccToken(server, id, headers = {}) { + return body(await server.token(post({ grant_type: 'client_credentials', scope: 'api', ...creds(id) }, headers))); +} + +function exchange(server, id, subjectToken, extra = {}, headers = {}) { + return server.token( + post( + { + grant_type: EXCHANGE, + subject_token: subjectToken, + subject_token_type: ACCESS_TOKEN_TYPE, + ...creds(id), + ...extra, + }, + headers, + ), + ); +} + +test('token exchange: a token issued to another client cannot be exchanged', async () => { + const { server } = exchangeServer(); + const alice = await ccToken(server, 'alice'); + const res = await exchange(server, 'mallory', alice.access_token); + assert.equal(res.status, 400); + assert.equal(body(res).error, 'invalid_grant'); + + assert.equal((await exchange(server, 'alice', alice.access_token)).status, 200); +}); + +test('token exchange: a DPoP-bound subject token needs the same key', async () => { + const { server } = exchangeServer(); + const dpop = await dpopClient(); + const htu = `${ISSUER}/token`; + const bound = await ccToken(server, 'alice', { dpop: await dpop.proof('POST', htu) }); + assert.equal(bound.token_type, 'DPoP'); + + const stripped = await exchange(server, 'alice', bound.access_token); + assert.equal(body(stripped).error, 'invalid_grant'); + + const proven = await exchange(server, 'alice', bound.access_token, {}, { dpop: await dpop.proof('POST', htu) }); + assert.equal(proven.status, 200); +}); + +test('token exchange: the target audience must be allowlisted', async () => { + const { server } = exchangeServer(); + const alice = await ccToken(server, 'alice'); + const res = await exchange(server, 'alice', alice.access_token, { audience: 'https://evil.example.com' }); + assert.equal(body(res).error, 'invalid_target'); + const ok = await exchange(server, 'alice', alice.access_token, { resource: 'https://downstream.example.com' }); + assert.equal(ok.status, 200); +}); + +test('token exchange: a prior actor chain nests under the new act', async () => { + const { server } = exchangeServer(); + const alice = await ccToken(server, 'alice'); + const once = body(await exchange(server, 'alice', alice.access_token)); + const twice = body(await exchange(server, 'alice', once.access_token)); + const claims = JSON.parse(Buffer.from(twice.access_token.split('.')[1], 'base64url').toString()); + assert.deepEqual(claims.act, { sub: 'alice', act: { sub: 'alice' } }); +}); diff --git a/packages/oauth2/tests/server-tierc.test.js b/packages/oauth2/tests/server-tierc.test.js index 7dcbc64f..e4757ab7 100644 --- a/packages/oauth2/tests/server-tierc.test.js +++ b/packages/oauth2/tests/server-tierc.test.js @@ -224,6 +224,7 @@ test('token exchange delegates a subject token to a new audience (RFC 8693)', as ], grants: [CC, EXCHANGE], scopes: ['api'], + tokenExchange: { audiences: ['https://downstream.example.com'] }, }); const subjectTok = body( From 9685edb4b21862efe1ab251da570e2cc13d88420 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:09:47 +0300 Subject: [PATCH 12/29] fix(oauth2): keep certificate binding on device and refresh grants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token endpoint computes cnf.x5t#S256 for a certificate-authenticated client, but only the code and client_credentials grants passed it on. Access tokens from the device-code and refresh grants came out as plain bearer tokens for mTLS clients (RFC 8705 §3). --- .../oauth2/src/server/grants/device-code.js | 1 + .../oauth2/src/server/grants/refresh-token.js | 1 + .../oauth2/tests/server-hardening.test.js | 38 +++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/packages/oauth2/src/server/grants/device-code.js b/packages/oauth2/src/server/grants/device-code.js index 38be9c19..da5af2ba 100644 --- a/packages/oauth2/src/server/grants/device-code.js +++ b/packages/oauth2/src/server/grants/device-code.js @@ -73,6 +73,7 @@ export async function deviceCodeGrant(req, ctx) { clientId: client.clientId, scope: record.scope, dpopJkt: ctx.dpopJkt, + confirmation: ctx.confirmation, }); if (refreshAllowed(config, client)) { diff --git a/packages/oauth2/src/server/grants/refresh-token.js b/packages/oauth2/src/server/grants/refresh-token.js index 0f26e79d..0defb0bc 100644 --- a/packages/oauth2/src/server/grants/refresh-token.js +++ b/packages/oauth2/src/server/grants/refresh-token.js @@ -91,6 +91,7 @@ export async function refreshTokenGrant(req, ctx) { scope, audience, dpopJkt: record.dpopJkt, + confirmation: ctx.confirmation, }); body.refresh_token = nextToken; diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 5487c23a..59ac5f97 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -350,3 +350,41 @@ test('token exchange: a prior actor chain nests under the new act', async () => const claims = JSON.parse(Buffer.from(twice.access_token.split('.')[1], 'base64url').toString()); assert.deepEqual(claims.act, { sub: 'alice', act: { sub: 'alice' } }); }); + +// CERTIFICATE BINDING + +test('refresh and device grants keep mTLS certificate binding', async () => { + const der = Buffer.from('device-cert-der'); + const { createHash } = await import('node:crypto'); + const thumb = createHash('sha256').update(der).digest('base64url'); + const DEVICE = 'urn:ietf:params:oauth:grant-type:device_code'; + const { server } = buildServer({ + clients: [ + { + clientId: 'mtls', + tokenEndpointAuthMethod: 'self_signed_tls_client_auth', + certificateThumbprint: thumb, + redirectUris: ['https://x.example.com/cb'], + grantTypes: [DEVICE, 'refresh_token'], + scope: ['read'], + }, + ], + grants: [DEVICE, 'refresh_token'], + }); + const call = (handler, params) => + handler({ + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ client_id: 'mtls', ...params }).toString(), + clientCertificate: { raw: der }, + }); + const cnfOf = tok => JSON.parse(Buffer.from(tok.split('.')[1], 'base64url').toString()).cnf; + + const da = body(await call(server.deviceAuthorization, { scope: 'read' })); + await server.device.approve(da.user_code, { subject: 'user-1' }); + const tok = body(await call(server.token, { grant_type: DEVICE, device_code: da.device_code })); + assert.equal(cnfOf(tok.access_token)['x5t#S256'], thumb); + + const refreshed = body(await call(server.token, { grant_type: 'refresh_token', refresh_token: tok.refresh_token })); + assert.equal(cnfOf(refreshed.access_token)['x5t#S256'], thumb); +}); From 2e174b2787c163a21b7b932d11ae8d8acc380f85 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:10:57 +0300 Subject: [PATCH 13/29] fix(oauth2): device codes keep their original expiry and redeem once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every poll (including slow_down) rewrote the device record with a fresh 10-minute TTL, so a device that kept polling kept its code alive indefinitely. The in-memory user_code → device_code index had no TTL at all, and two concurrent polls of an approved code could both get tokens. - the stores stamp expiresAt on save and update without extending it (memory: remaining TTL, Redis: SET … KEEPTTL); the reverse index lives on the same TTL - new optional DeviceStore.redeem() flips approved → redeemed atomically (memory: synchronous check-and-flip, Redis: SET NX claim); the grant uses it when present and falls back to update() for custom stores --- .../oauth2/src/server/grants/device-code.js | 12 ++++- packages/oauth2/src/server/stores.js | 49 ++++++++++--------- packages/oauth2/src/server/stores/redis.js | 24 +++++++-- .../oauth2/tests/server-hardening.test.js | 40 +++++++++++++++ .../tests/stores/redis.integration.test.js | 16 ++++++ 5 files changed, 110 insertions(+), 31 deletions(-) diff --git a/packages/oauth2/src/server/grants/device-code.js b/packages/oauth2/src/server/grants/device-code.js index da5af2ba..40b08ae4 100644 --- a/packages/oauth2/src/server/grants/device-code.js +++ b/packages/oauth2/src/server/grants/device-code.js @@ -6,7 +6,7 @@ * the code is redeemed exactly once for tokens; a denied or expired * request returns the matching error. */ -import { isNonEmptyString, isNumber } from '@exortek/shared/predicates'; +import { isFunction, isNonEmptyString, isNumber } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; import { buildAccessResponse, refreshAllowed, mintRefreshToken } from './_issue.js'; @@ -66,7 +66,15 @@ export async function deviceCodeGrant(req, ctx) { } // Redeem once: flip to a terminal state so a repeat poll can't re-issue. - await config.stores.device.update(deviceCode, { status: 'redeemed' }); + // The built-in stores do it atomically, so two concurrent polls of an + // approved code cannot both get tokens. + if (isFunction(config.stores.device.redeem)) { + if (!(await config.stores.device.redeem(deviceCode))) { + throw new ServerError(ProtocolError.INVALID_GRANT, 'the device_code has already been used'); + } + } else { + await config.stores.device.update(deviceCode, { status: 'redeemed' }); + } const body = await buildAccessResponse(config, { subject: record.subject, diff --git a/packages/oauth2/src/server/stores.js b/packages/oauth2/src/server/stores.js index 6a847ad6..67e18204 100644 --- a/packages/oauth2/src/server/stores.js +++ b/packages/oauth2/src/server/stores.js @@ -242,51 +242,47 @@ export function createParStore() { * @property {'pending' | 'approved' | 'denied' | 'redeemed'} status * @property {string} [subject] set on approval * @property {number} [lastPolledAt] for slow_down enforcement + * @property {number} [expiresAt] epoch ms; set by the store on save and never extended * * @typedef {Object} DeviceStore * @property {(record: DeviceRecord, ttlMs: number) => void | Promise} save * @property {(deviceCode: string) => (DeviceRecord | undefined) | Promise} getByDeviceCode * @property {(userCode: string) => (DeviceRecord | undefined) | Promise} getByUserCode * @property {(deviceCode: string, patch: Partial) => void | Promise} update + * Patch a record without extending its lifetime. + * @property {(deviceCode: string) => boolean | Promise} [redeem] + * Atomically flip an `approved` record to `redeemed`; resolve whether this + * call did it. Optional for custom stores (the grant then falls back to a + * non-atomic `update`), implemented by the built-in stores. */ /** @returns {DeviceStore} */ export function createDeviceStore() { const byDeviceCode = createTtlMap(); - /** @type {Map} user_code → device_code */ - const userToDevice = new Map(); + // user_code → device_code, on the same TTL as the record it points at. + const userToDevice = createTtlMap(); return { save(record, ttlMs) { - byDeviceCode.set(record.deviceCode, record, ttlMs); - userToDevice.set(record.userCode, record.deviceCode); + const expiresAt = Date.now() + ttlMs; + byDeviceCode.set(record.deviceCode, { ...record, expiresAt }, ttlMs); + userToDevice.set(record.userCode, record.deviceCode, ttlMs); }, getByDeviceCode(deviceCode) { return /** @type {DeviceRecord | undefined} */ (byDeviceCode.get(deviceCode)); }, getByUserCode(userCode) { - const deviceCode = userToDevice.get(userCode); - if (!deviceCode) { - return undefined; - } - const record = this.getByDeviceCode(deviceCode); - // The device record expired out from under the reverse index — drop - // the dangling `user_code` mapping so it cannot leak. - if (!record) { - userToDevice.delete(userCode); - return undefined; - } - return record; + const deviceCode = /** @type {string | undefined} */ (userToDevice.get(userCode)); + return deviceCode ? this.getByDeviceCode(deviceCode) : undefined; }, update(deviceCode, patch) { const existing = /** @type {DeviceRecord | undefined} */ (byDeviceCode.get(deviceCode)); if (!existing) { return; } - // Preserve the remaining TTL by not resetting it — re-save with the - // original expiry window is unnecessary here since getByDeviceCode - // already evicts past expiry; a short re-arm is acceptable. - byDeviceCode.set(deviceCode, { ...existing, ...patch }, DEVICE_REEARM_MS); + // Keep the original expiry: polling must never extend a device code. + const remaining = /** @type {number} */ (existing.expiresAt) - Date.now(); + byDeviceCode.set(deviceCode, { ...existing, ...patch }, remaining); // Once the code is spent (redeemed/denied), the one-time `user_code` // is done — free the reverse index eagerly rather than waiting for // TTL, so a verified device flow leaves nothing behind. @@ -294,13 +290,18 @@ export function createDeviceStore() { userToDevice.delete(existing.userCode); } }, + // Check-and-flip in one synchronous step: exactly one poll redeems. + redeem(deviceCode) { + const existing = /** @type {DeviceRecord | undefined} */ (byDeviceCode.get(deviceCode)); + if (!existing || existing.status !== 'approved') { + return false; + } + this.update(deviceCode, { status: 'redeemed' }); + return true; + }, }; } -// A device flow is minutes-long; re-arming an update for 10 min keeps a -// pending record alive across polls without a separate expiry field. -const DEVICE_REEARM_MS = 600_000; - /** * @param {unknown} store * @param {string[]} methods diff --git a/packages/oauth2/src/server/stores/redis.js b/packages/oauth2/src/server/stores/redis.js index 189dc70a..3c84e021 100644 --- a/packages/oauth2/src/server/stores/redis.js +++ b/packages/oauth2/src/server/stores/redis.js @@ -168,13 +168,11 @@ export function createRedisDeviceStore(client, options = {}) { const dk = deviceCode => `${prefix}d:${deviceCode}`; const uk = userCode => `${prefix}u:${userCode}`; const helpers = createRedisHelpers(client); - // Re-arm window for an update that carries no explicit TTL (mirrors the - // in-memory store): a device flow is minutes-long. - const REEARM_MS = 600_000; const self = { async save(record, ttlMs) { - await helpers.setWithTTL(dk(record.deviceCode), JSON.stringify(record), ttlMs); + const expiresAt = Date.now() + ttlMs; + await helpers.setWithTTL(dk(record.deviceCode), JSON.stringify({ ...record, expiresAt }), ttlMs); await helpers.setWithTTL(uk(record.userCode), record.deviceCode, ttlMs); }, async getByDeviceCode(deviceCode) { @@ -187,16 +185,32 @@ export function createRedisDeviceStore(client, options = {}) { } return self.getByDeviceCode(deviceCode); }, + // KEEPTTL: an update (every poll writes lastPolledAt) never extends the + // code's life. async update(deviceCode, patch) { const existing = await self.getByDeviceCode(deviceCode); if (!existing) { return; } - await helpers.setWithTTL(dk(deviceCode), JSON.stringify({ ...existing, ...patch }), REEARM_MS); + await helpers.setKeepTTL(dk(deviceCode), JSON.stringify({ ...existing, ...patch })); if (patch.status === 'redeemed' || patch.status === 'denied') { await client.del(uk(existing.userCode)); } }, + // Single redemption across nodes: the first poll to create the redeem + // claim (SET NX) wins; any concurrent poll resolves false. + async redeem(deviceCode) { + const existing = await self.getByDeviceCode(deviceCode); + if (!existing || existing.status !== 'approved') { + return false; + } + const ttlMs = Math.max(1, (existing.expiresAt ?? Date.now() + 1000) - Date.now()); + if (!(await helpers.setIfAbsent(`${dk(deviceCode)}:redeemed`, '1', ttlMs))) { + return false; + } + await self.update(deviceCode, { status: 'redeemed' }); + return true; + }, }; return self; } diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 59ac5f97..9c030e52 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -388,3 +388,43 @@ test('refresh and device grants keep mTLS certificate binding', async () => { const refreshed = body(await call(server.token, { grant_type: 'refresh_token', refresh_token: tok.refresh_token })); assert.equal(cnfOf(refreshed.access_token)['x5t#S256'], thumb); }); + +// DEVICE CODE LIFETIME + +function deviceServer(device) { + const DEVICE = 'urn:ietf:params:oauth:grant-type:device_code'; + const { server } = buildServer({ + clients: [ + { + clientId: 'tv', + tokenEndpointAuthMethod: 'none', + redirectUris: ['https://x.example.com/cb'], + grantTypes: [DEVICE], + }, + ], + grants: [DEVICE], + device, + }); + const poll = deviceCode => server.token(post({ grant_type: DEVICE, device_code: deviceCode, client_id: 'tv' })); + return { server, poll }; +} + +test('device: polling never extends the device code past its lifetime', async () => { + const { server, poll } = deviceServer({ ttl: 80, interval: 0 }); + const da = body(await server.deviceAuthorization(post({ client_id: 'tv' }))); + assert.equal(body(await poll(da.device_code)).error, 'authorization_pending'); + await new Promise(r => setTimeout(r, 40)); + assert.equal(body(await poll(da.device_code)).error, 'authorization_pending'); + await new Promise(r => setTimeout(r, 60)); + assert.equal(body(await poll(da.device_code)).error, 'expired_token'); + assert.equal(await server.device.getByUserCode(da.user_code), undefined); +}); + +test('device: two concurrent polls of an approved code redeem it once', async () => { + const { server, poll } = deviceServer({ interval: 0 }); + const da = body(await server.deviceAuthorization(post({ client_id: 'tv' }))); + await server.device.approve(da.user_code, { subject: 'user-1' }); + const results = (await Promise.all([poll(da.device_code), poll(da.device_code)])).map(body); + assert.equal(results.filter(r => r.access_token).length, 1); + assert.equal(results.find(r => !r.access_token).error, 'invalid_grant'); +}); diff --git a/packages/oauth2/tests/stores/redis.integration.test.js b/packages/oauth2/tests/stores/redis.integration.test.js index 65b301d0..e3672c31 100644 --- a/packages/oauth2/tests/stores/redis.integration.test.js +++ b/packages/oauth2/tests/stores/redis.integration.test.js @@ -75,6 +75,22 @@ forEachRedisDriver('oauth2 server stores', ({ test, client, ns, raw }) => { assert.equal(await store.get('t1'), null); }); + test('device updates keep the original expiry and redeem once', async () => { + const keyPrefix = ns('e2'); + const store = createRedisDeviceStore(client(), { keyPrefix }); + await store.save( + { deviceCode: 'd2', userCode: 'BCDF-2345', clientId: 'app', scope: [], status: 'pending' }, + 60_000, + ); + await store.update('d2', { status: 'approved', subject: 'u' }); + const pttl = await raw.pttl(`${keyPrefix}device:d:d2`); + assert.ok(pttl > 0 && pttl <= 60_000, `update must keep the expiry; got PTTL ${pttl}`); + assert.deepEqual((await store.getByDeviceCode('d2')).scope, []); + + const outcomes = await Promise.all([store.redeem('d2'), store.redeem('d2')]); + assert.deepEqual(outcomes.sort(), [false, true]); + }); + test('device lookup + reverse-index cleanup', async () => { const store = createRedisDeviceStore(client(), { keyPrefix: ns('e') }); await store.save({ deviceCode: 'd1', userCode: 'WXYZ-2345', clientId: 'app', scope: [], status: 'pending' }, 5_000); From f0bf23db7e93e52446c806fb43687fec179ecdb6 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:12:12 +0300 Subject: [PATCH 14/29] perf(oauth2): cache client JWKS resolvers; refuse private jwks_uri hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveClientKeys built a fresh remote JWKS resolver on every call, so the JWKS response cache never survived: each private_key_jwt token request and each unauthenticated /authorize?request= triggered a fetch of a client-chosen URL. Resolvers are now cached per server — remote sets by jwks_uri (bounded LRU), inline sets by client descriptor. When dynamic registration is enabled and no security.allowJwksHost gate is configured, jwks_uri hosts that are loopback, private, link-local or metadata addresses are refused by default. --- packages/oauth2/src/server/index.js | 8 +- .../oauth2/src/server/security/client-auth.js | 3 +- .../oauth2/src/server/security/client-keys.js | 171 +++++++++++++----- .../oauth2/src/server/security/jar-jarm.js | 3 +- .../oauth2/tests/server-hardening.test.js | 87 +++++++++ 5 files changed, 225 insertions(+), 47 deletions(-) diff --git a/packages/oauth2/src/server/index.js b/packages/oauth2/src/server/index.js index 89830939..c8bee0ef 100644 --- a/packages/oauth2/src/server/index.js +++ b/packages/oauth2/src/server/index.js @@ -28,6 +28,7 @@ import { createDeviceHandler, createDeviceApproval } from './handlers/device.js' import { createRegisterHandler } from './handlers/register.js'; import { applyFapiProfile } from './security/fapi.js'; import { createIdTokenSigner } from './issuers/id-token.js'; +import { createClientKeyResolver } from './security/client-keys.js'; export { defineClient, createClientRegistry, AUTH_METHODS } from './clients.js'; export { jwtIssuer } from './issuers/jwt.js'; @@ -57,7 +58,7 @@ const DEFAULT_REFRESH_TTL = '30d'; * @property {{ required?: boolean, algs?: string[], nonce?: boolean }} [dpop] RFC 9449 * @property {{ required?: boolean, ttl?: string|number }} [par] RFC 9126 * @property {boolean} [fapi] FAPI 2.0 profile (bundles PAR + PKCE + DPoP/mTLS + iss) - * @property {(hostname: string, url: URL) => boolean} [allowJwksHost] gate the hosts the server will fetch a client's `jwks_uri` from; return false to refuse. Recommended wherever dynamic client registration is enabled, since the URI is then chosen by the registrant. + * @property {(hostname: string, url: URL) => boolean} [allowJwksHost] gate the hosts the server will fetch a client's `jwks_uri` from; return false to refuse. With dynamic registration enabled and no gate set, loopback / private / link-local hosts are refused by default, since the URI is then chosen by the registrant. * * @typedef {Object} ServerConfig * @property {string} issuer AS issuer identifier (https, no trailing slash) @@ -210,6 +211,11 @@ export function createServer(config) { // `registration` config is present AND the registry can persist. registration: isObject(config.registration) ? config.registration : undefined, registrationEnabled: isObject(config.registration) && isFunction(registry.register), + // Per-server client JWKS resolver cache (private_key_jwt, JAR). + clientKeys: createClientKeyResolver({ + allowJwksHost: security.allowJwksHost, + registrationEnabled: isObject(config.registration) && isFunction(registry.register), + }), }; return { diff --git a/packages/oauth2/src/server/security/client-auth.js b/packages/oauth2/src/server/security/client-auth.js index f3835411..fbe3cec4 100644 --- a/packages/oauth2/src/server/security/client-auth.js +++ b/packages/oauth2/src/server/security/client-auth.js @@ -24,7 +24,6 @@ import { isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates import { ProtocolError, ServerError } from '../errors.js'; import { readClientCredentials } from '../request.js'; -import { resolveClientKeys } from './client-keys.js'; import { parseDn, sameDn, subjectAttributes } from './dn.js'; import { createReplayCache } from './replay-cache.js'; @@ -189,7 +188,7 @@ async function authenticateAssertion(req, config, assertionType) { `private_key_jwt alg ${JSON.stringify(alg)} is not an accepted asymmetric algorithm`, ); } - keyish = await resolveClientKeys(client, { allowJwksHost: config.security?.allowJwksHost }); + keyish = await config.clientKeys(client); } else { throw new ServerError( ProtocolError.INVALID_CLIENT, diff --git a/packages/oauth2/src/server/security/client-keys.js b/packages/oauth2/src/server/security/client-keys.js index eccd1412..d0672310 100644 --- a/packages/oauth2/src/server/security/client-keys.js +++ b/packages/oauth2/src/server/security/client-keys.js @@ -7,59 +7,146 @@ * inline set becomes a `kid`-aware resolver so a signed token's `kid` * selects the right key (importing to bare KeyObjects would drop the * `kid` and break selection). + * + * Resolvers are cached per server: a remote set by its `jwks_uri` (so the + * JWKS response cache actually survives between requests instead of every + * token request and every `/authorize?request=` triggering a fetch) and an + * inline set by the client descriptor. */ +import { isIP } from 'node:net'; + import { createRemoteJWKS } from '@exortek/jwks'; import { importJWK } from '@exortek/jwk'; -import { isArray, isNonEmptyString, isObject } from '@exortek/shared/predicates'; +import { isArray, isFunction, isNonEmptyString, isObject } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; +const MAX_REMOTE_RESOLVERS = 1000; + /** - * @param {import('../clients.js').Client} client - * @param {{ allowJwksHost?: (hostname: string, url: URL) => boolean }} [options] + * @typedef {(client: import('../clients.js').Client) => Promise} ClientKeyResolver + */ + +/** + * @param {{ allowJwksHost?: (hostname: string, url: URL) => boolean, registrationEnabled?: boolean }} [options] * `allowJwksHost` gates the destination of a `jwksUri` fetch. A client's - * `jwks_uri` is client-supplied — under open or token-gated dynamic - * registration it is chosen by whoever registered — so a deployment can - * constrain which hosts the server will reach out to. - * @returns {Promise} a `keyish` resolver for `jwt.verify` + * `jwks_uri` is client-supplied — under dynamic registration it is chosen + * by whoever registered — so when registration is enabled and no gate is + * configured, loopback, private and link-local hosts are refused by + * default (an SSRF lever into the server's own network). + * @returns {ClientKeyResolver} */ -export async function resolveClientKeys(client, options = {}) { - if (isNonEmptyString(client.jwksUri)) { - try { - return createRemoteJWKS(client.jwksUri, { - ...(options.allowJwksHost ? { allowHost: options.allowJwksHost } : {}), - }); - } catch (err) { - throw new ServerError( - ProtocolError.INVALID_CLIENT, - `client jwks_uri was refused: ${err instanceof Error ? err.message : String(err)}`, - ); +export function createClientKeyResolver(options = {}) { + const allowHost = isFunction(options.allowJwksHost) + ? options.allowJwksHost + : options.registrationEnabled === true + ? isPublicHost + : undefined; + + /** @type {Map} jwks_uri → remote resolver (insertion-ordered LRU) */ + const remote = new Map(); + /** @type {WeakMap>} client descriptor → inline resolver */ + const inline = new WeakMap(); + + return async function resolveClientKeys(client) { + if (isNonEmptyString(client.jwksUri)) { + const cached = remote.get(client.jwksUri); + if (cached) { + remote.delete(client.jwksUri); + remote.set(client.jwksUri, cached); + return cached; + } + let resolver; + try { + resolver = createRemoteJWKS(client.jwksUri, allowHost ? { allowHost } : {}); + } catch (err) { + throw new ServerError( + ProtocolError.INVALID_CLIENT, + `client jwks_uri was refused: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if (remote.size >= MAX_REMOTE_RESOLVERS) { + remote.delete(remote.keys().next().value); + } + remote.set(client.jwksUri, resolver); + return resolver; } - } - if (isObject(client.jwks) && isArray(client.jwks.keys) && client.jwks.keys.length > 0) { - const entries = await Promise.all( - client.jwks.keys.map(async jwk => [isNonEmptyString(jwk.kid) ? jwk.kid : undefined, await importJWK(jwk)]), - ); - const byKid = new Map(entries.filter(([kid]) => kid !== undefined)); - const all = entries.map(([, key]) => key); - return header => { - const kid = header?.kid; - if (isNonEmptyString(kid)) { - const match = byKid.get(kid); - if (match) { - return match; - } - // A named kid that isn't in the set is a hard miss. - throw new ServerError(ProtocolError.INVALID_CLIENT, `no client key matches kid ${JSON.stringify(kid)}`); + if (isObject(client.jwks) && isArray(client.jwks.keys) && client.jwks.keys.length > 0) { + let pending = inline.get(client); + if (!pending) { + pending = inlineResolver(client.jwks.keys); + inline.set(client, pending); + // A key that fails to import must not poison the cache. + pending.catch(() => inline.delete(client)); } - if (all.length === 1) { - return all[0]; + return pending; + } + throw new ServerError(ProtocolError.INVALID_CLIENT, `client ${JSON.stringify(client.clientId)} has no JWKS`); + }; +} + +/** + * @param {object[]} keys + * @returns {Promise} + */ +async function inlineResolver(keys) { + const entries = await Promise.all( + keys.map(async jwk => [isNonEmptyString(jwk.kid) ? jwk.kid : undefined, await importJWK(jwk)]), + ); + const byKid = new Map(entries.filter(([kid]) => kid !== undefined)); + const all = entries.map(([, key]) => key); + return header => { + const kid = header?.kid; + if (isNonEmptyString(kid)) { + const match = byKid.get(kid); + if (match) { + return match; } - throw new ServerError( - ProtocolError.INVALID_CLIENT, - 'the signed token has no kid but the client has multiple keys', - ); - }; + // A named kid that isn't in the set is a hard miss. + throw new ServerError(ProtocolError.INVALID_CLIENT, `no client key matches kid ${JSON.stringify(kid)}`); + } + if (all.length === 1) { + return all[0]; + } + throw new ServerError(ProtocolError.INVALID_CLIENT, 'the signed token has no kid but the client has multiple keys'); + }; +} + +/** + * Default `jwks_uri` host gate under dynamic registration: refuse names and + * IP literals that point back into the server's own or private networks. + * This is a literal check — a public name that resolves to a private + * address is only stopped by a deployment-supplied `allowJwksHost` (or an + * egress proxy). + * + * @param {string} hostname + * @returns {boolean} + */ +export function isPublicHost(hostname) { + const host = hostname.replace(/^\[|\]$/g, '').toLowerCase(); + if (host === 'localhost' || host.endsWith('.localhost') || host.endsWith('.local') || host.endsWith('.internal')) { + return false; + } + const family = isIP(host); + if (family === 4) { + const [a, b] = host.split('.').map(Number); + return !( + a === 0 || + a === 10 || + a === 127 || + (a === 100 && b >= 64 && b <= 127) || + (a === 169 && b === 254) || + (a === 172 && b >= 16 && b <= 31) || + (a === 192 && b === 168) || + a >= 224 + ); + } + if (family === 6) { + const mapped = host.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/); + if (mapped) { + return isPublicHost(mapped[1]); + } + return !(host === '::' || host === '::1' || /^f[cd]/.test(host) || /^fe[89ab]/.test(host) || /^ff/.test(host)); } - throw new ServerError(ProtocolError.INVALID_CLIENT, `client ${JSON.stringify(client.clientId)} has no JWKS`); + return true; } diff --git a/packages/oauth2/src/server/security/jar-jarm.js b/packages/oauth2/src/server/security/jar-jarm.js index 11c2a419..d0c66edb 100644 --- a/packages/oauth2/src/server/security/jar-jarm.js +++ b/packages/oauth2/src/server/security/jar-jarm.js @@ -15,7 +15,6 @@ import { sign as jwtSign, verify as jwtVerify, decode as jwtDecode, JwtError } f import { isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; -import { resolveClientKeys } from './client-keys.js'; import { createReplayCache } from './replay-cache.js'; // A JAR request object is short-lived; bound its accepted lifetime so a @@ -74,7 +73,7 @@ export async function verifyRequestObject(requestJwt, outerClientId, config) { let payload; try { - const keys = await resolveClientKeys(client, { allowJwksHost: config.security?.allowJwksHost }); + const keys = await config.clientKeys(client); payload = (await jwtVerify(requestJwt, keys, { alg: [alg], audience: config.issuer })).payload; } catch (err) { if (err instanceof JwtError) { diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 9c030e52..f926df4c 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -428,3 +428,90 @@ test('device: two concurrent polls of an approved code redeem it once', async () assert.equal(results.filter(r => r.access_token).length, 1); assert.equal(results.find(r => !r.access_token).error, 'invalid_grant'); }); + +// CLIENT JWKS + +test('client jwks_uri: the resolver is cached across token requests', async () => { + const { assertionClient } = await import('./helpers/server.js'); + const pk = await assertionClient('pk'); + const { server } = buildServer({ + clients: [ + { + clientId: 'pk', + tokenEndpointAuthMethod: 'private_key_jwt', + jwksUri: 'https://keys.example.com/jwks.json', + redirectUris: ['https://x.example.com/cb'], + grantTypes: ['client_credentials'], + scope: ['read'], + }, + ], + grants: ['client_credentials'], + }); + const realFetch = globalThis.fetch; + let fetches = 0; + globalThis.fetch = async () => { + fetches += 1; + return new Response(JSON.stringify({ keys: [pk.jwk] }), { headers: { 'content-type': 'application/json' } }); + }; + try { + for (const jti of ['a1', 'a2', 'a3']) { + const res = await server.token( + post({ + grant_type: 'client_credentials', + client_assertion_type: 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer', + client_assertion: await pk.assertion(jti), + }), + ); + assert.equal(res.status, 200); + } + } finally { + globalThis.fetch = realFetch; + } + assert.equal(fetches, 1); +}); + +test('client jwks_uri: private hosts are refused when registration is enabled', async () => { + const { isPublicHost } = await import('../src/server/security/client-keys.js'); + for (const host of [ + 'localhost', + '127.0.0.1', + '10.1.2.3', + '172.20.0.1', + '192.168.1.1', + '169.254.169.254', + '[::1]', + '[fd00::1]', + '[::ffff:127.0.0.1]', + ]) { + assert.equal(isPublicHost(host), false, host); + } + for (const host of ['keys.example.com', '8.8.8.8', '[2606:4700::1111]']) { + assert.equal(isPublicHost(host), true, host); + } + + const { createClientRegistry } = await import('../src/server/index.js'); + const { server } = buildServer({ + clients: createClientRegistry([ + { + clientId: 'ssrf', + tokenEndpointAuthMethod: 'private_key_jwt', + jwksUri: 'https://169.254.169.254/latest/meta-data', + redirectUris: ['https://x.example.com/cb'], + grantTypes: ['client_credentials'], + }, + ]), + grants: ['client_credentials'], + registration: { initialAccessToken: 'iat-value-strong-enough' }, + }); + const { assertionClient } = await import('./helpers/server.js'); + const attacker = await assertionClient('ssrf'); + const res = await server.token( + post({ + grant_type: 'client_credentials', + client_assertion_type: 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer', + client_assertion: await attacker.assertion('x1'), + }), + ); + assert.equal(res.status, 401); + assert.equal(body(res).error, 'invalid_client'); +}); From dab742005c40b700fcd388ae8c408517a26838da Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:13:47 +0300 Subject: [PATCH 15/29] feat(oauth2): injectable replay store and stateless DPoP nonces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DPoP proof, client-assertion and request-object jti caches and the DPoP nonce map were module-level: shared by every createServer in the process, invisible to other nodes, and not injectable. Behind a load balancer a replayed proof or assertion succeeded on another node and dpop.nonce:true broke (a nonce issued by one node was unknown to the next). The nonce map also swept itself O(n) on every issue past 1024. - stores.replay accepts any IncrStore-shaped store (incr(key, ttlMs)); without one each server gets its own in-memory guard - verifyDpopForResource takes an optional replayStore for multi-node resource servers - DPoP nonces are HMAC time-window tokens: every node configured with the same security.dpop.nonceSecret (≥ 32 bytes) issues and accepts the same nonces, with nothing stored or swept --- packages/oauth2/src/server/index.js | 13 +- .../oauth2/src/server/security/client-auth.js | 23 ++- packages/oauth2/src/server/security/dpop.js | 142 ++++++++++-------- .../oauth2/src/server/security/jar-jarm.js | 10 +- packages/oauth2/src/server/security/replay.js | 60 ++++++++ .../oauth2/tests/server-hardening.test.js | 46 ++++++ 6 files changed, 216 insertions(+), 78 deletions(-) create mode 100644 packages/oauth2/src/server/security/replay.js diff --git a/packages/oauth2/src/server/index.js b/packages/oauth2/src/server/index.js index c8bee0ef..86013ed0 100644 --- a/packages/oauth2/src/server/index.js +++ b/packages/oauth2/src/server/index.js @@ -29,6 +29,8 @@ import { createRegisterHandler } from './handlers/register.js'; import { applyFapiProfile } from './security/fapi.js'; import { createIdTokenSigner } from './issuers/id-token.js'; import { createClientKeyResolver } from './security/client-keys.js'; +import { createDpopNonces } from './security/dpop.js'; +import { createReplayGuard, isReplayStore } from './security/replay.js'; export { defineClient, createClientRegistry, AUTH_METHODS } from './clients.js'; export { jwtIssuer } from './issuers/jwt.js'; @@ -55,7 +57,7 @@ const DEFAULT_REFRESH_TTL = '30d'; * @typedef {Object} ServerSecurity * @property {string|number} [authorizationCodeTtl='1m'] * @property {string|number} [refreshTokenTtl='30d'] lifetime of an issued refresh token - * @property {{ required?: boolean, algs?: string[], nonce?: boolean }} [dpop] RFC 9449 + * @property {{ required?: boolean, algs?: string[], nonce?: boolean, nonceSecret?: string | Uint8Array }} [dpop] RFC 9449; give every node the same ≥32-byte `nonceSecret` when nonces are on behind a load balancer * @property {{ required?: boolean, ttl?: string|number }} [par] RFC 9126 * @property {boolean} [fapi] FAPI 2.0 profile (bundles PAR + PKCE + DPoP/mTLS + iss) * @property {(hostname: string, url: URL) => boolean} [allowJwksHost] gate the hosts the server will fetch a client's `jwks_uri` from; return false to refuse. With dynamic registration enabled and no gate set, loopback / private / link-local hosts are refused by default, since the URI is then chosen by the registrant. @@ -106,6 +108,7 @@ const DEFAULT_REFRESH_TTL = '30d'; * @property {ReturnType} refresh * @property {ReturnType} par * @property {ReturnType} device + * @property {import('./security/replay.js').ReplayStore} [replay] IncrStore-shaped (`incr(key, ttlMs)`) — shares DPoP / client-assertion / request-object jti replay checks across nodes * * @typedef {Object} ResolvedServerConfig * @property {string} issuer @@ -160,6 +163,11 @@ export function createServer(config) { ]), }; + const replayStore = config.stores?.replay; + if (replayStore !== undefined && !isReplayStore(replayStore)) { + throw new OAuth2Error(ErrorCode.INVALID_ARGUMENT, 'stores.replay must implement incr(key, ttlMs)'); + } + // FAPI 2.0 tightens several defaults at once (PLAN Tier C). const { fapi, requirePkce, requirePar, dpopRequired } = applyFapiProfile(security); @@ -192,6 +200,9 @@ export function createServer(config) { requirePkce, dpopRequired, dpopNonceRequired: security.dpop?.nonce === true, + dpopNonces: createDpopNonces(security.dpop?.nonceSecret), + // Per-server single-use jti guard (DPoP, client assertions, JAR). + replay: createReplayGuard(replayStore), fapi, authenticateUser: config.authenticateUser, authorizationCodeTtlMs: parseDuration(security.authorizationCodeTtl ?? DEFAULT_AUTH_CODE_TTL), diff --git a/packages/oauth2/src/server/security/client-auth.js b/packages/oauth2/src/server/security/client-auth.js index fbe3cec4..d3ca2483 100644 --- a/packages/oauth2/src/server/security/client-auth.js +++ b/packages/oauth2/src/server/security/client-auth.js @@ -25,12 +25,10 @@ import { isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates import { ProtocolError, ServerError } from '../errors.js'; import { readClientCredentials } from '../request.js'; import { parseDn, sameDn, subjectAttributes } from './dn.js'; -import { createReplayCache } from './replay-cache.js'; const JWT_BEARER_ASSERTION = 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer'; -// RFC 7523 §3 requires `exp`; bound its reach so the (process-local, -// opportunistically-evicted) jti replay cache actually covers the token's -// whole life. +// RFC 7523 §3 requires `exp`; bound its reach so the jti replay guard +// actually covers the token's whole life. const MAX_ASSERTION_LIFETIME_MS = 300_000; // 5 minutes const ASSERTION_ALGS = new Set([ 'ES256', @@ -45,10 +43,6 @@ const ASSERTION_ALGS = new Set([ 'EdDSA', ]); -// Assertion replay cache — a `jti` is single-use within its lifetime. The -// 5-minute retention matches the accepted assertion lifetime bound above. -const seenAssertionJti = createReplayCache(MAX_ASSERTION_LIFETIME_MS); - /** * Resolve and authenticate the client behind a request. * @@ -217,16 +211,21 @@ async function authenticateAssertion(req, config, assertionType) { throw new ServerError(ProtocolError.INVALID_CLIENT, 'client_assertion lifetime exceeds the accepted maximum'); } - assertSingleUse(payload.jti); + await assertSingleUse(payload.jti, config.replay); return client; } -/** @param {unknown} jti */ -function assertSingleUse(jti) { +/** + * A `jti` is single-use within the accepted assertion lifetime. + * + * @param {unknown} jti + * @param {import('./replay.js').ReplayGuard} replay + */ +async function assertSingleUse(jti, replay) { if (!isNonEmptyString(jti)) { throw new ServerError(ProtocolError.INVALID_CLIENT, 'client_assertion is missing jti'); } - if (seenAssertionJti.seen(jti)) { + if (await replay.seen('assertion', jti, MAX_ASSERTION_LIFETIME_MS)) { throw new ServerError(ProtocolError.INVALID_CLIENT, 'client_assertion jti has already been used (replay)'); } } diff --git a/packages/oauth2/src/server/security/dpop.js b/packages/oauth2/src/server/security/dpop.js index 4188bfdf..290b4479 100644 --- a/packages/oauth2/src/server/security/dpop.js +++ b/packages/oauth2/src/server/security/dpop.js @@ -12,15 +12,17 @@ * proof is allowed only when neither the client nor the server requires * DPoP — otherwise it is `invalid_dpop_proof`. */ -import { createHash, randomBytes } from 'node:crypto'; +import { createHash, createHmac, randomBytes } from 'node:crypto'; import { importJWK, thumbprint } from '@exortek/jwk'; import { verify as jwsVerify, decodeProtectedHeader } from '@exortek/jws'; -import { encode as base64urlEncode } from '@exortek/shared/base64url'; +import { encode as base64urlEncode, decode as base64urlDecode } from '@exortek/shared/base64url'; import { isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates'; +import { timingSafeEqual } from '@exortek/shared/timing-safe'; +import { ErrorCode, OAuth2Error } from '../../internal/errors.js'; import { ProtocolError, ServerError } from '../errors.js'; -import { createReplayCache } from './replay-cache.js'; +import { createReplayGuard } from './replay.js'; // RFC 9449 §4.2 — the proof `alg` must be an asymmetric JWS alg; symmetric // and `none` are rejected so the embedded JWK is a real public key. @@ -39,52 +41,71 @@ const ALLOWED_ALGS = new Set([ // Proofs are single-use and short-lived; §4.3 recommends a small window. const MAX_PROOF_AGE_MS = 60_000; -// Replay cache: a proof `jti` may be used once. A deployment fronting -// multiple nodes overrides this with a shared store if it needs cross-node -// replay defense. Retention covers the full freshness window (a proof's -// iat is accepted within ±MAX_PROOF_AGE_MS). -const seenJti = createReplayCache(2 * MAX_PROOF_AGE_MS); +// A proof `jti` may be used once; retention covers the full freshness +// window (a proof's iat is accepted within ±MAX_PROOF_AGE_MS). The server +// passes its own replay guard (shareable across nodes via `stores.replay`); +// `verifyDpopForResource` without one falls back to this process-local guard. +const REPLAY_TTL_MS = 2 * MAX_PROOF_AGE_MS; +let defaultReplay = createReplayGuard(); // Server-provided DPoP nonce (RFC 9449 §8-9). When the server requires a // nonce, the client must echo the most recent `DPoP-Nonce` value as the // proof's `nonce` claim; a missing/stale nonce is answered with // `use_dpop_nonce` and a fresh `DPoP-Nonce` header so the client retries. -// Issued nonces are accepted for a short window (module-level, single- -// process — a multi-node deployment supplies a shared nonce service). -const NONCE_TTL_MS = 300_000; -/** @type {Map} nonce → expiry epoch ms */ -const issuedNonces = new Map(); +// +// Nonces are stateless: `base64url(window ‖ HMAC(secret, window)[0..16])` +// for a 150 s time window, accepted for the current and the previous window. +// Every node holding the same `security.dpop.nonceSecret` issues and accepts +// the same nonces, so nonce mode works behind a load balancer with no shared +// state and nothing to sweep. +const NONCE_WINDOW_MS = 150_000; +const NONCE_MAC_BYTES = 16; /** - * Mint a fresh DPoP nonce, remember it for the acceptance window, and - * return the value to hand back in the `DPoP-Nonce` header. - * - * @returns {string} + * @param {string | Uint8Array | undefined} secret ≥ 32 bytes; random per server when omitted */ -function issueNonce() { - const now = Date.now(); - if (issuedNonces.size > 1024) { - for (const [value, exp] of issuedNonces) { - if (exp <= now) { - issuedNonces.delete(value); - } +export function createDpopNonces(secret) { + let key; + if (secret === undefined) { + key = randomBytes(32); + } else { + key = Buffer.from(/** @type {any} */ (secret)); + if (key.length < 32) { + throw new OAuth2Error(ErrorCode.INVALID_ARGUMENT, 'security.dpop.nonceSecret must be at least 32 bytes'); } } - const nonce = base64urlEncode(randomBytes(16)); - issuedNonces.set(nonce, now + NONCE_TTL_MS); - return nonce; -} - -/** - * @param {unknown} nonce - * @returns {boolean} - */ -function isValidNonce(nonce) { - if (!isNonEmptyString(nonce)) { - return false; - } - const exp = issuedNonces.get(nonce); - return exp !== undefined && exp > Date.now(); + const mac = window => { + const w = Buffer.alloc(4); + w.writeUInt32BE(window); + return Buffer.concat([w, createHmac('sha256', key).update(w).digest().subarray(0, NONCE_MAC_BYTES)]); + }; + const currentWindow = () => Math.floor(Date.now() / NONCE_WINDOW_MS); + return { + issue() { + return base64urlEncode(mac(currentWindow())); + }, + /** @param {unknown} nonce */ + isValid(nonce) { + if (!isNonEmptyString(nonce)) { + return false; + } + let raw; + try { + raw = base64urlDecode(nonce); + } catch { + return false; + } + if (raw.length !== 4 + NONCE_MAC_BYTES) { + return false; + } + const window = raw.readUInt32BE(0); + const now = currentWindow(); + if (window !== now && window !== now - 1) { + return false; + } + return timingSafeEqual(raw, mac(window)); + }, + }; } /** @@ -113,24 +134,26 @@ export async function resolveDpopBinding(req, config, client, options = {}) { // The proof binds to the endpoint it was sent to — the token endpoint, // or the PAR endpoint for a `dpop_jkt` pre-binding (RFC 9449 §10.1). htu: isNonEmptyString(options.htu) ? options.htu : config.endpoints.token, + replay: config.replay, }); // RFC 9449 §8-9: when the server requires a nonce, a proof that omits it // or carries a stale one is answered with `use_dpop_nonce` plus a fresh // `DPoP-Nonce` header, and the client retries with the new value. - if (nonceRequired && !isValidNonce(nonce)) { + if (nonceRequired && !config.dpopNonces.isValid(nonce)) { throw new ServerError( ProtocolError.USE_DPOP_NONCE, 'a DPoP proof nonce is required; retry with the supplied nonce', { - headers: { 'DPoP-Nonce': issueNonce() }, + headers: { 'DPoP-Nonce': config.dpopNonces.issue() }, }, ); } - // On success, hand the client a fresh nonce to use on its next request - // (kept rotating so a captured nonce ages out). Only when nonces are on. - return { dpopJkt: jkt, nonceHeaders: nonceRequired ? { 'DPoP-Nonce': issueNonce() } : {} }; + // On success, hand the client the current nonce for its next request + // (it rotates every window, so a captured nonce ages out). Only when + // nonces are on. + return { dpopJkt: jkt, nonceHeaders: nonceRequired ? { 'DPoP-Nonce': config.dpopNonces.issue() } : {} }; } /** @@ -138,7 +161,7 @@ export async function resolveDpopBinding(req, config, client, options = {}) { * `nonce` claim (if any) for the caller's nonce-challenge check. * * @param {string} proof - * @param {{ htm: string, htu: string, ath?: string }} binding + * @param {{ htm: string, htu: string, ath?: string, replay?: import('./replay.js').ReplayGuard }} binding * @returns {Promise<{ jkt: string, nonce: string | undefined }>} */ export async function verifyDpopProof(proof, binding) { @@ -193,7 +216,7 @@ export async function verifyDpopProof(proof, binding) { throw new ServerError(ProtocolError.INVALID_DPOP_PROOF, 'DPoP proof ath does not match the access token'); } assertFresh(payload.iat); - assertUnique(payload.jti); + await assertUnique(payload.jti, binding.replay ?? defaultReplay); return { jkt, nonce: isNonEmptyString(payload.nonce) ? payload.nonce : undefined }; } @@ -206,8 +229,11 @@ export async function verifyDpopProof(proof, binding) { * key is the one the token was issued to — the missing high-level helper * for the resource-server half of DPoP. * + * A multi-node resource server passes `replayStore` (IncrStore-shaped, + * e.g. Redis-backed) so a proof replayed against a different node is caught. + * * @param {string} proof the `DPoP` request header - * @param {{ htm: string, htu: string, accessToken: string, cnfJkt: string }} binding + * @param {{ htm: string, htu: string, accessToken: string, cnfJkt: string, replayStore?: import('./replay.js').ReplayStore }} binding * @returns {Promise<{ jkt: string }>} */ export async function verifyDpopForResource(proof, binding) { @@ -218,7 +244,8 @@ export async function verifyDpopForResource(proof, binding) { throw new ServerError(ProtocolError.INVALID_DPOP_PROOF, 'verifyDpopForResource requires the access token'); } const ath = base64urlEncode(createHash('sha256').update(binding.accessToken).digest()); - const { jkt } = await verifyDpopProof(proof, { htm: binding.htm, htu: binding.htu, ath }); + const replay = binding.replayStore !== undefined ? createReplayGuard(binding.replayStore) : undefined; + const { jkt } = await verifyDpopProof(proof, { htm: binding.htm, htu: binding.htu, ath, replay }); // The proving key must be the key the token is bound to. if (!isNonEmptyString(binding.cnfJkt) || jkt !== binding.cnfJkt) { throw new ServerError(ProtocolError.INVALID_DPOP_PROOF, 'DPoP proof key does not match the token cnf.jkt'); @@ -255,23 +282,20 @@ function assertFresh(iat) { } } -/** @param {unknown} jti */ -function assertUnique(jti) { +/** + * @param {unknown} jti + * @param {import('./replay.js').ReplayGuard} replay + */ +async function assertUnique(jti, replay) { if (!isNonEmptyString(jti)) { throw new ServerError(ProtocolError.INVALID_DPOP_PROOF, 'DPoP proof is missing jti'); } - if (seenJti.seen(jti)) { + if (await replay.seen('dpop', jti, REPLAY_TTL_MS)) { throw new ServerError(ProtocolError.INVALID_DPOP_PROOF, 'DPoP proof jti has already been used (replay)'); } } -/** Test hook — clear the replay + nonce caches between cases. */ +/** Test hook — reset the process-local fallback replay guard. */ export function _clearDpopReplayCache() { - seenJti.clear(); - issuedNonces.clear(); -} - -/** Test hook — mint a nonce as the server would (to seed a proof). */ -export function _issueDpopNonce() { - return issueNonce(); + defaultReplay = createReplayGuard(); } diff --git a/packages/oauth2/src/server/security/jar-jarm.js b/packages/oauth2/src/server/security/jar-jarm.js index d0c66edb..89d0896c 100644 --- a/packages/oauth2/src/server/security/jar-jarm.js +++ b/packages/oauth2/src/server/security/jar-jarm.js @@ -15,15 +15,13 @@ import { sign as jwtSign, verify as jwtVerify, decode as jwtDecode, JwtError } f import { isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates'; import { ProtocolError, ServerError } from '../errors.js'; -import { createReplayCache } from './replay-cache.js'; // A JAR request object is short-lived; bound its accepted lifetime so a // captured one cannot be replayed indefinitely, and remember its `jti` -// (when present) for that window so a single-use request object cannot be -// replayed at all. Module-level / single-process, like the DPoP and -// client-assertion caches; a multi-node deployment fronts it with PAR. +// (when present) for that window — through the server's replay guard, +// shareable across nodes via `stores.replay` — so a single-use request +// object cannot be replayed at all. const MAX_REQUEST_OBJECT_LIFETIME_MS = 300_000; // 5 minutes -const seenRequestJti = createReplayCache(MAX_REQUEST_OBJECT_LIFETIME_MS); const REQUEST_OBJECT_ALGS = new Set([ 'ES256', @@ -91,7 +89,7 @@ export async function verifyRequestObject(requestJwt, outerClientId, config) { if (payload.exp * 1000 - Date.now() > MAX_REQUEST_OBJECT_LIFETIME_MS) { throw new ServerError(ProtocolError.INVALID_REQUEST, 'request object lifetime exceeds the accepted maximum'); } - if (isNonEmptyString(payload.jti) && seenRequestJti.seen(payload.jti)) { + if (isNonEmptyString(payload.jti) && (await config.replay.seen('jar', payload.jti, MAX_REQUEST_OBJECT_LIFETIME_MS))) { throw new ServerError(ProtocolError.INVALID_REQUEST, 'request object jti has already been used (replay)'); } diff --git a/packages/oauth2/src/server/security/replay.js b/packages/oauth2/src/server/security/replay.js new file mode 100644 index 00000000..64ad98fa --- /dev/null +++ b/packages/oauth2/src/server/security/replay.js @@ -0,0 +1,60 @@ +/** + * Replay guard — one place that decides whether a single-use identifier + * (a DPoP proof `jti`, a client-assertion `jti`, a request-object `jti`) has + * been seen before. Each server gets its own guard: + * + * - with `stores.replay` — any IncrStore-shaped store (`incr(key, ttlMs)` + * → `{ count }` or a number, e.g. `@exortek/shared/incr-store` over + * Redis) — the check is shared by every node behind a load balancer; + * - without it, per-server in-memory two-bucket caches (single process). + * + * The first `incr` of a key returns 1; anything higher is a replay. + */ +import { isFunction, isNumber, isObject } from '@exortek/shared/predicates'; + +import { createReplayCache } from './replay-cache.js'; + +/** + * @typedef {Object} ReplayStore + * @property {(key: string, ttlMs: number) => Promise<{ count: number } | number> | { count: number } | number} incr + * + * @typedef {Object} ReplayGuard + * @property {(namespace: string, id: string, ttlMs: number) => Promise} seen + * record `id` under `namespace` for `ttlMs`; resolve true when it was already present + */ + +/** + * @param {unknown} value + * @returns {value is ReplayStore} + */ +export function isReplayStore(value) { + return isObject(value) && isFunction(/** @type {Record} */ (value).incr); +} + +/** + * @param {ReplayStore} [store] + * @returns {ReplayGuard} + */ +export function createReplayGuard(store) { + if (store !== undefined) { + return { + async seen(namespace, id, ttlMs) { + const res = await store.incr(`${namespace}:${id}`, ttlMs); + const count = isNumber(res) ? res : res?.count; + return !isNumber(count) || count > 1; + }, + }; + } + /** @type {Map>} */ + const caches = new Map(); + return { + async seen(namespace, id, ttlMs) { + let cache = caches.get(namespace); + if (!cache) { + cache = createReplayCache(ttlMs); + caches.set(namespace, cache); + } + return cache.seen(id); + }, + }; +} diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index f926df4c..8ce08c03 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -515,3 +515,49 @@ test('client jwks_uri: private hosts are refused when registration is enabled', assert.equal(res.status, 401); assert.equal(body(res).error, 'invalid_client'); }); + +// MULTI-NODE REPLAY + NONCES + +test('stores.replay shares client-assertion replay checks across servers', async () => { + const { createMemoryIncrStore } = await import('@exortek/shared/incr-store'); + const { assertionClient } = await import('./helpers/server.js'); + const pk = await assertionClient('pk'); + const replay = createMemoryIncrStore(); + const node = () => + buildServer({ + clients: [ + { + clientId: 'pk', + tokenEndpointAuthMethod: 'private_key_jwt', + jwks: { keys: [pk.jwk] }, + redirectUris: ['https://x.example.com/cb'], + grantTypes: ['client_credentials'], + scope: ['read'], + }, + ], + grants: ['client_credentials'], + stores: { replay }, + }).server; + const assertion = await pk.assertion('only-once'); + const req = () => + post({ + grant_type: 'client_credentials', + client_assertion_type: 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer', + client_assertion: assertion, + }); + assert.equal((await node().token(req())).status, 200); + const replayed = await node().token(req()); + assert.equal(replayed.status, 401); + assert.match(body(replayed).error_description, /replay/); +}); + +test('DPoP nonces issued by one node are accepted by another with the same secret', async () => { + const nonceSecret = 'n'.repeat(32); + const a = buildServer({ security: { dpop: { nonce: true, nonceSecret } } }).server; + const b = buildServer({ security: { dpop: { nonce: true, nonceSecret } } }).server; + const nonce = a._config.dpopNonces.issue(); + assert.equal(b._config.dpopNonces.isValid(nonce), true); + assert.equal(buildServer({ security: { dpop: { nonce: true } } }).server._config.dpopNonces.isValid(nonce), false); + assert.equal(b._config.dpopNonces.isValid(`${nonce.slice(0, -2)}AA`), false); + assert.throws(() => buildServer({ security: { dpop: { nonce: true, nonceSecret: 'short' } } }), /32 bytes/); +}); From 68c2f9f03ef6de41a9c26b3c33bf6ceebbe549cb Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:14:28 +0300 Subject: [PATCH 16/29] fix(oauth2): a replayed authorization code revokes its refresh family MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second redemption of an authorization code was rejected, but the tokens already issued from it stayed valid. RFC 6749 §4.1.2 says they SHOULD be revoked, since a reused code means it was intercepted. After a successful redemption the grant leaves a short-lived marker under the same code key (plain save, so any auth-code store works). A replay finds the marker and revokes the refresh family that redemption started. Consume-first ordering is kept: it is what makes redemption atomic. --- .../src/server/grants/authorization-code.js | 19 +++++++++++++++- packages/oauth2/src/server/stores.js | 2 ++ .../oauth2/tests/server-hardening.test.js | 22 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/packages/oauth2/src/server/grants/authorization-code.js b/packages/oauth2/src/server/grants/authorization-code.js index 557c85de..e1b13ae2 100644 --- a/packages/oauth2/src/server/grants/authorization-code.js +++ b/packages/oauth2/src/server/grants/authorization-code.js @@ -7,6 +7,7 @@ */ import { isNonEmptyString } from '@exortek/shared/predicates'; +import { randomState } from '../../internal/state.js'; import { ProtocolError, ServerError } from '../errors.js'; import { verifyCodeVerifier } from '../security/pkce.js'; import { resolveTokenAudience } from '../security/resource.js'; @@ -29,8 +30,19 @@ export async function authorizationCodeGrant(req, ctx) { throw new ServerError(ProtocolError.INVALID_REQUEST, 'missing code'); } - // Single-use: consuming the code here means a replay finds nothing. + // Single-use: consuming the code here is what makes redemption atomic. + // A redeemed code leaves a short-lived marker under the same key, so a + // replay finds it and revokes the refresh family the code produced + // (RFC 6749 §4.1.2: tokens issued for a reused code SHOULD be revoked). + // Refresh tokens are revocable; self-contained access tokens simply run + // out their (short) lifetime. const record = await config.stores.authCode.consume(code); + if (record?.redeemed === true) { + if (isNonEmptyString(record.familyId)) { + await config.stores.refresh.revokeFamily(record.familyId); + } + throw new ServerError(ProtocolError.INVALID_GRANT, 'authorization code has already been used'); + } if (!record) { throw new ServerError(ProtocolError.INVALID_GRANT, 'authorization code is invalid, expired, or already used'); } @@ -81,8 +93,12 @@ export async function authorizationCodeGrant(req, ctx) { authTime: record.authTime, }); + /** @type {string | undefined} */ + let familyId; if (refreshAllowed(config, client)) { + familyId = randomState(); body.refresh_token = await mintRefreshToken(config, { + familyId, subject: record.subject, clientId: client.clientId, scope: record.scope, @@ -90,6 +106,7 @@ export async function authorizationCodeGrant(req, ctx) { dpopJkt, }); } + await config.stores.authCode.save(code, { redeemed: true, familyId }, config.authorizationCodeTtlMs); return body; } diff --git a/packages/oauth2/src/server/stores.js b/packages/oauth2/src/server/stores.js index 67e18204..db6a4d2a 100644 --- a/packages/oauth2/src/server/stores.js +++ b/packages/oauth2/src/server/stores.js @@ -106,6 +106,8 @@ function createTtlMap({ sweepIntervalMs = 60_000 } = {}) { * @property {string} [resource] * @property {unknown} [authorizationDetails] * @property {string} [dpopJkt] DPoP `jkt` bound at the authorize step (RFC 9449 §10) + * @property {boolean} [redeemed] marker left after redemption so a replay can revoke the family + * @property {string} [familyId] the refresh family a redeemed code produced * * @typedef {Object} AuthCodeStore * @property {(code: string, record: AuthCodeRecord, ttlMs: number) => void | Promise} save diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 8ce08c03..629842df 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -561,3 +561,25 @@ test('DPoP nonces issued by one node are accepted by another with the same secre assert.equal(b._config.dpopNonces.isValid(`${nonce.slice(0, -2)}AA`), false); assert.throws(() => buildServer({ security: { dpop: { nonce: true, nonceSecret: 'short' } } }), /32 bytes/); }); + +// AUTHORIZATION CODE REUSE + INTROSPECTION + +test('a replayed authorization code revokes the refresh family it produced', async () => { + const { server } = buildServer(); + const { code, pkce, redirectUri } = await getAuthorizationCode(server, { scope: 'read' }); + const redeem = () => + server.token( + post({ + grant_type: 'authorization_code', + code, + redirect_uri: redirectUri, + client_id: 'app', + client_secret: 's3cret-value-strong', + code_verifier: pkce.codeVerifier, + }), + ); + const first = body(await redeem()); + assert.ok(first.refresh_token); + assert.equal(body(await redeem()).error, 'invalid_grant'); + assert.equal(body(await refresh(server, first.refresh_token)).error, 'invalid_grant'); +}); From 3a827ba382c0b0f4ee7b0a592d579ac9dda356f9 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:14:28 +0300 Subject: [PATCH 17/29] fix(oauth2): introspection requires a confidential client A public client authenticates by client_id alone, so anyone who knew a public client's id could use the introspection endpoint to query that client's tokens. Public clients are now refused unless the deployment sets introspection.allowPublicClients. --- packages/oauth2/src/server/handlers/introspect.js | 7 +++++++ packages/oauth2/tests/server-hardening.test.js | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/packages/oauth2/src/server/handlers/introspect.js b/packages/oauth2/src/server/handlers/introspect.js index 1765b79f..c9eca32b 100644 --- a/packages/oauth2/src/server/handlers/introspect.js +++ b/packages/oauth2/src/server/handlers/introspect.js @@ -13,6 +13,10 @@ * user identities). Deployments where trusted resource servers must * introspect any client's token opt in with * `introspection: { allowCrossClient: true }`. + * + * A public client authenticates by `client_id` alone, so it is refused + * unless `introspection.allowPublicClients` is set — otherwise anyone who + * knows a public client's id could query its tokens. */ import { isArray, isNonEmptyString, isObject } from '@exortek/shared/predicates'; @@ -36,6 +40,9 @@ export function createIntrospectHandler(config) { } const client = await authenticateClient(req, config); + if (client.isPublic && config.introspection?.allowPublicClients !== true) { + throw new ServerError(ProtocolError.INVALID_CLIENT, 'introspection requires a confidential client'); + } const token = req.form.token; if (!isNonEmptyString(token)) { diff --git a/packages/oauth2/tests/server-hardening.test.js b/packages/oauth2/tests/server-hardening.test.js index 629842df..606c844c 100644 --- a/packages/oauth2/tests/server-hardening.test.js +++ b/packages/oauth2/tests/server-hardening.test.js @@ -583,3 +583,12 @@ test('a replayed authorization code revokes the refresh family it produced', asy assert.equal(body(await redeem()).error, 'invalid_grant'); assert.equal(body(await refresh(server, first.refresh_token)).error, 'invalid_grant'); }); + +test('introspection refuses a public client unless allowed', async () => { + const pub = { clientId: 'spa', tokenEndpointAuthMethod: 'none', redirectUris: ['https://spa.example.com/cb'] }; + const refused = buildServer({ clients: [pub] }).server; + const res = await refused.introspect(post({ token: 'x', client_id: 'spa' })); + assert.equal(res.status, 401); + const allowed = buildServer({ clients: [pub], introspection: { allowPublicClients: true } }).server; + assert.equal((await allowed.introspect(post({ token: 'x', client_id: 'spa' }))).status, 200); +}); From ef6e4ba5e8533806c601cf43e6dbda335f07f51c Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:15:43 +0300 Subject: [PATCH 18/29] fix(oidc): verify the id_token_hint before trusting its subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit endSessionHandler decoded id_token_hint without checking its signature and handed the hint's sub to onLogout. Anyone could mint an unsigned token with the OP's iss and any sub, send a victim's browser (or call the endpoint directly) and log that user out everywhere. - the hint is verified against the published JWKS (or the signing key) with the configured alg and issuer; an expired hint is still accepted for logout.idTokenHintMaxAge (default 30d) past exp - an unverifiable hint names no subject; onLogout also gets clientId - a client_id parameter must match the hint's audience - postLogoutRedirectUris may be a client_id → URIs map, checked against the verified hint's client --- packages/oidc/src/internal/logout.js | 37 +++++++++--------- packages/oidc/src/provider/index.js | 44 +++++++++++++++++++--- packages/oidc/tests/logout.test.js | 56 ++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 22 deletions(-) diff --git a/packages/oidc/src/internal/logout.js b/packages/oidc/src/internal/logout.js index 5b7b10aa..66570b33 100644 --- a/packages/oidc/src/internal/logout.js +++ b/packages/oidc/src/internal/logout.js @@ -7,7 +7,7 @@ * redirects back. These are the pure builders/validators shared by the * client's `endSessionUrl` and the provider's `endSessionHandler`. */ -import { decode } from '@exortek/jwt'; +import { verify } from '@exortek/jwt'; import { isArray, isNonEmptyString } from '@exortek/shared/predicates'; /** @@ -40,31 +40,34 @@ export function buildEndSessionUrl(endpoint, params) { } /** - * Read the (unverified) `sub` / `aud` from an `id_token_hint`. A logout hint - * is frequently expired, so the OP validates issuer/audience rather than the - * full signature+exp; the return lets the handler match the session to clear. + * Verify an `id_token_hint` and read its `sub` / `aud`. The hint is the only + * thing that tells the OP whose session to end, so it must carry the OP's + * own signature — a decoded-but-unverified hint lets anyone name any `sub` + * and force that user's logout. A logout hint is frequently expired, so + * `exp` is tolerated up to `maxAge` past expiry; signature, `iss` and `alg` + * are always enforced. * - * @param {string} idTokenHint - * @param {string} issuer the OP's own issuer — the hint's `iss` must match - * @returns {{ sub?: string, aud?: string | string[] } | null} null when the hint is unusable / not ours + * @param {string | undefined} idTokenHint + * @param {{ issuer: string, keys: unknown, alg: string, maxAge: string | number }} options + * @returns {Promise<{ sub?: string, aud?: string | string[] } | null>} null when the hint is absent or not genuinely ours */ -export function readIdTokenHint(idTokenHint, issuer) { +export async function verifyIdTokenHint(idTokenHint, { issuer, keys, alg, maxAge }) { if (!isNonEmptyString(idTokenHint)) { return null; } - let payload; try { - payload = decode(idTokenHint).payload; + const { payload } = await verify(idTokenHint, /** @type {any} */ (keys), { + alg: [alg], + issuer, + clockTolerance: maxAge, + }); + return { + sub: /** @type {string|undefined} */ (payload.sub), + aud: /** @type {string|string[]|undefined} */ (payload.aud), + }; } catch { return null; } - if (payload.iss !== issuer) { - return null; - } - return { - sub: /** @type {string|undefined} */ (payload.sub), - aud: /** @type {string|string[]|undefined} */ (payload.aud), - }; } /** diff --git a/packages/oidc/src/provider/index.js b/packages/oidc/src/provider/index.js index 1b67d31a..c1b4a395 100644 --- a/packages/oidc/src/provider/index.js +++ b/packages/oidc/src/provider/index.js @@ -21,12 +21,13 @@ import { isArray, isNonEmptyString, isObject } from '@exortek/shared/predicates' import { invalidArgument } from '../internal/errors.js'; import { buildDiscoveryDocument } from '../internal/discovery-doc.js'; import { buildUserInfo } from '../internal/userinfo.js'; -import { isRegisteredPostLogoutUri, readIdTokenHint } from '../internal/logout.js'; +import { isRegisteredPostLogoutUri, verifyIdTokenHint } from '../internal/logout.js'; import { checkSessionIframeHtml, computeSessionState } from '../internal/session.js'; import { htmlResponse, jsonResponse, normalizeRequest, redirectResponse } from '../internal/http-io.js'; const DEFAULT_SCOPES = ['openid', 'profile', 'email']; const DEFAULT_CLAIMS_SUPPORTED = ['sub', 'iss', 'aud', 'exp', 'iat', 'auth_time', 'nonce']; +const DEFAULT_HINT_MAX_AGE = '30d'; /** * @typedef {object} OidcProviderConfig @@ -38,7 +39,11 @@ const DEFAULT_CLAIMS_SUPPORTED = ['sub', 'iss', 'aud', 'exp', 'iat', 'auth_time' * @property {string[]} [scopes] advertised scopes (default openid/profile/email). * @property {string[]} [authMethods] token_endpoint_auth_methods_supported. * @property {{ resolve: (accessToken: string) => (Promise<{ sub: string, scope?: string|string[], claims?: Record } | null> | { sub: string, scope?: string|string[], claims?: Record } | null) }} [userinfo] access-token resolver for the UserInfo endpoint. - * @property {{ postLogoutRedirectUris?: string[], onLogout?: (ctx: { sub?: string, idTokenHint?: string }) => unknown }} [logout] RP-Initiated Logout policy. + * @property {{ postLogoutRedirectUris?: string[] | Record, idTokenHintMaxAge?: string | number, onLogout?: (ctx: { sub?: string, clientId?: string, idTokenHint?: string }) => unknown }} [logout] + * RP-Initiated Logout policy. `postLogoutRedirectUris` is one list for every client, or a map + * client_id → URIs (the client is identified by the verified hint's `aud`). `onLogout` receives a + * `sub` only from a hint that verifies against the OP's keys; expired hints are accepted for + * `idTokenHintMaxAge` (default `'30d'`) past `exp`. * @property {{ cookieName?: string }} [session] enable Session Management; `cookieName` names the OP browser-state cookie. */ @@ -179,24 +184,53 @@ export function createProvider(config) { */ endSessionHandler() { const logout = isObject(config.logout) ? config.logout : {}; - const registered = isArray(logout.postLogoutRedirectUris) ? logout.postLogoutRedirectUris : []; + const uris = logout.postLogoutRedirectUris; const onLogout = typeof logout.onLogout === 'function' ? logout.onLogout : undefined; + const hintOptions = { + issuer, + // Published keys verify hints signed with a rotated-out key too. + keys: publicJwks.length > 0 ? publicJwks : signing.key, + alg: signing.alg, + maxAge: logout.idTokenHintMaxAge ?? DEFAULT_HINT_MAX_AGE, + }; return async raw => { const req = normalizeRequest(raw); const idTokenHint = req.param('id_token_hint'); const postLogout = req.param('post_logout_redirect_uri'); const state = req.param('state'); - const hint = readIdTokenHint(idTokenHint, issuer); + const requestedClient = req.param('client_id'); + const hint = await verifyIdTokenHint(idTokenHint, hintOptions); + + // The client is whoever the verified hint was issued to; a + // `client_id` parameter must agree with it (RP-Initiated Logout §2). + const audiences = hint ? (isArray(hint.aud) ? hint.aud : [hint.aud]).filter(isNonEmptyString) : []; + if (isNonEmptyString(requestedClient) && hint && !audiences.includes(requestedClient)) { + return jsonResponse( + 400, + { error: 'invalid_request', error_description: 'client_id does not match the id_token_hint audience' }, + { 'cache-control': 'no-store' }, + ); + } + const clientId = isNonEmptyString(requestedClient) + ? requestedClient + : audiences.length === 1 + ? audiences[0] + : undefined; if (onLogout) { try { - await onLogout({ sub: hint ? hint.sub : undefined, idTokenHint }); + await onLogout({ sub: hint ? hint.sub : undefined, clientId, idTokenHint }); } catch { // Session teardown is best-effort — never block the logout redirect. } } if (isNonEmptyString(postLogout)) { + const registered = isArray(uris) + ? uris + : isObject(uris) && isNonEmptyString(clientId) && hint + ? (uris[clientId] ?? []) + : []; if (!isRegisteredPostLogoutUri(postLogout, registered)) { return jsonResponse( 400, diff --git a/packages/oidc/tests/logout.test.js b/packages/oidc/tests/logout.test.js index f9803759..1141ddcc 100644 --- a/packages/oidc/tests/logout.test.js +++ b/packages/oidc/tests/logout.test.js @@ -109,3 +109,59 @@ describe('provider.endSessionHandler', () => { assert.equal(JSON.parse(res.body).logged_out, true); }); }); + +describe('provider.endSessionHandler — id_token_hint verification', () => { + async function providerWith(logout) { + const signer = await makeSigner(); + const seen = []; + const provider = createProvider({ + issuer: ISSUER, + signing: { key: signer.privateJwk, alg: signer.alg, kid: signer.kid }, + jwks: [signer.publicJwk], + endpoints: { authorization: '/authorize', token: '/token' }, + logout: { onLogout: ctx => seen.push(ctx), ...logout }, + }); + return { provider, signer, seen }; + } + + it('a hint signed by anyone else names no subject', async () => { + const { provider, seen } = await providerWith({}); + const attacker = await makeSigner({ kid: 'test-key-1' }); + const forged = await attacker.mint({ iss: ISSUER, sub: 'victim', aud: CLIENT_ID }); + await provider.endSessionHandler()({ query: { id_token_hint: forged } }); + assert.equal(seen[0].sub, undefined); + }); + + it('an expired but genuine hint still identifies the user', async () => { + const { provider, signer, seen } = await providerWith({}); + const now = Math.floor(Date.now() / 1000); + const hint = await signer.mint({ iss: ISSUER, sub: 'user-1', aud: CLIENT_ID, iat: now - 7200, exp: now - 3600 }); + await provider.endSessionHandler()({ query: { id_token_hint: hint } }); + assert.equal(seen[0].sub, 'user-1'); + assert.equal(seen[0].clientId, CLIENT_ID); + }); + + it('client_id must match the hint audience', async () => { + const { provider, signer } = await providerWith({}); + const hint = await signer.mint({ iss: ISSUER, sub: 'user-1', aud: CLIENT_ID }); + const res = await provider.endSessionHandler()({ query: { id_token_hint: hint, client_id: 'other-client' } }); + assert.equal(res.status, 400); + }); + + it('per-client post_logout_redirect_uris follow the verified audience', async () => { + const { provider, signer } = await providerWith({ + postLogoutRedirectUris: { [CLIENT_ID]: [POST_LOGOUT], other: ['https://other.example.com/out'] }, + }); + const hint = await signer.mint({ iss: ISSUER, sub: 'user-1', aud: CLIENT_ID }); + const ok = await provider.endSessionHandler()({ + query: { id_token_hint: hint, post_logout_redirect_uri: POST_LOGOUT }, + }); + assert.equal(ok.status, 302); + const cross = await provider.endSessionHandler()({ + query: { id_token_hint: hint, post_logout_redirect_uri: 'https://other.example.com/out' }, + }); + assert.equal(cross.status, 400); + const noHint = await provider.endSessionHandler()({ query: { post_logout_redirect_uri: POST_LOGOUT } }); + assert.equal(noHint.status, 400); + }); +}); From c0800000c7bf4ef394e0ffd137ba999ae7ea8722 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:16:40 +0300 Subject: [PATCH 19/29] fix(oidc): enforce the requested max_age and acr_values authorize() forwarded maxAge / acrValues to the OP, but handleCallback never looked at auth_time or acr. An OP that ignored the request (or an attacker replaying an older login through it) passed a stale or weaker authentication as if it met the requirement. handleCallback now requires, when they were requested (per call, or via the new client-level maxAge / acrValues defaults that authorize() also sends): - an auth_time no older than max_age (+ clockTolerance) - an acr from the requested acr_values list Failures throw OidcError AUTHENTICATION_REQUIREMENTS_NOT_MET (401). --- packages/oidc/src/client/index.js | 51 ++++++++++++++++++++++++---- packages/oidc/src/internal/errors.js | 14 ++++++++ packages/oidc/tests/client.test.js | 47 ++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/packages/oidc/src/client/index.js b/packages/oidc/src/client/index.js index a1dcd3c9..296ce5c0 100644 --- a/packages/oidc/src/client/index.js +++ b/packages/oidc/src/client/index.js @@ -16,9 +16,10 @@ */ import { createOAuth, defineProvider } from '@exortek/oauth2'; import { decode } from '@exortek/jwt'; -import { isArray, isNonEmptyString, isObject } from '@exortek/shared/predicates'; +import { parseDuration } from '@exortek/shared/duration'; +import { isArray, isNonEmptyString, isNumber, isObject } from '@exortek/shared/predicates'; -import { invalidArgument } from '../internal/errors.js'; +import { authenticationRequirementsNotMet, invalidArgument } from '../internal/errors.js'; import { buildEndSessionUrl } from '../internal/logout.js'; import { resolveIssuerMetadata } from '../internal/issuer-discovery.js'; @@ -57,7 +58,9 @@ const AUTH_PARAM_NAMES = { * @property {string} redirectUri Registered redirect URI for the callback. * @property {string[]} [scope] Requested scopes; `openid` is enforced. * @property {string[]} [idTokenAlgs] Signature alg allowlist for the id_token. - * @property {string|number} [clockTolerance] Leeway for `exp`/`nbf`/`iat`. + * @property {string|number} [clockTolerance] Leeway for `exp`/`nbf`/`iat` (and `auth_time` against `maxAge`). + * @property {number} [maxAge] Default `max_age` (seconds) for every authorization request; enforced on `auth_time` at the callback. + * @property {string} [acrValues] Default `acr_values` for every authorization request; the returned `acr` must be one of them. * @property {import('@exortek/jwks').RemoteJWKSOptions} [jwksOptions] Forwarded to the id_token JWKS resolver. * @property {string} [endSessionEndpoint] RP-Initiated Logout endpoint; auto-discovered from OP metadata when omitted. * @property {typeof fetch} [fetch] Override used only for the logout-endpoint metadata fetch (tests / proxies). @@ -132,8 +135,9 @@ export function createClient(config) { async authorize(options = {}) { /** @type {Record} */ const params = { ...options.params }; + const withDefaults = { maxAge: config.maxAge, acrValues: config.acrValues, ...options }; for (const [key, wire] of Object.entries(AUTH_PARAM_NAMES)) { - const value = /** @type {Record} */ (options)[key]; + const value = /** @type {Record} */ (withDefaults)[key]; if (value !== undefined) { params[wire] = String(value); } @@ -148,16 +152,28 @@ export function createClient(config) { * are already checked inside the oauth2 hub; `decode` here only reads the * now-trusted payload (no second network hop). * + * When the request asked for `max_age` / `acr_values` (pass the same + * values here, or set them once on the client config), the id_token must + * show an `auth_time` within `max_age` and an `acr` from the list (OIDC + * Core §3.1.2.1, §3.1.3.7) — otherwise the OP silently ignoring them + * would let a stale or weaker login through. + * * @param {Record} query the callback query params - * @param {{ session?: string }} [options] + * @param {{ session?: string, maxAge?: number, acrValues?: string }} [options] * @returns {Promise<{ idToken: string, claims: Record, userinfo: Record, user: object, tokens: Record, warnings: object[] }>} */ async handleCallback(query, options = {}) { const { tokens, user, warnings } = await oauth.callback('oidc', query, { session: options.session }); const idToken = /** @type {string} */ (tokens.id_token); + const claims = decode(idToken).payload; + assertAuthenticationRequirements(claims, { + maxAge: options.maxAge ?? config.maxAge, + acrValues: options.acrValues ?? config.acrValues, + clockToleranceMs: config.clockTolerance === undefined ? 0 : parseDuration(config.clockTolerance), + }); return { idToken, - claims: decode(idToken).payload, + claims, userinfo: /** @type {Record} */ (user).raw, user, tokens, @@ -215,3 +231,26 @@ export function createClient(config) { }, }; } + +/** + * @param {Record} claims + * @param {{ maxAge?: number, acrValues?: string, clockToleranceMs: number }} required + */ +function assertAuthenticationRequirements(claims, { maxAge, acrValues, clockToleranceMs }) { + if (maxAge !== undefined) { + const authTime = claims.auth_time; + if (!isNumber(authTime)) { + authenticationRequirementsNotMet('max_age was requested but the id_token has no auth_time'); + } + const ageMs = Date.now() - authTime * 1000; + if (ageMs > Number(maxAge) * 1000 + clockToleranceMs) { + authenticationRequirementsNotMet(`the end user authenticated more than max_age (${maxAge}s) ago`); + } + } + if (isNonEmptyString(acrValues)) { + const allowed = acrValues.split(/\s+/).filter(Boolean); + if (!isNonEmptyString(claims.acr) || !allowed.includes(claims.acr)) { + authenticationRequirementsNotMet(`the id_token acr is not one of the requested acr_values (${acrValues})`); + } + } +} diff --git a/packages/oidc/src/internal/errors.js b/packages/oidc/src/internal/errors.js index 57ed9165..96fa2fa1 100644 --- a/packages/oidc/src/internal/errors.js +++ b/packages/oidc/src/internal/errors.js @@ -18,6 +18,11 @@ export const ErrorCode = Object.freeze({ // `end_session_endpoint` for RP-Initiated Logout — the main login flow's // discovery is delegated to @exortek/oauth2 and surfaces as OAuth2Error). DISCOVERY_FAILED: 'DISCOVERY_FAILED', + + // The id_token verified, but the authentication it describes does not meet + // what the client asked for: `auth_time` older than the requested + // `max_age`, or an `acr` outside the requested `acr_values`. + AUTHENTICATION_REQUIREMENTS_NOT_MET: 'AUTHENTICATION_REQUIREMENTS_NOT_MET', }); /** @@ -31,6 +36,7 @@ export class OidcError extends BaseError { static statuses = { INVALID_ARGUMENT: 400, DISCOVERY_FAILED: 502, + AUTHENTICATION_REQUIREMENTS_NOT_MET: 401, }; static defaultStatus = 500; @@ -45,3 +51,11 @@ export class OidcError extends BaseError { export function invalidArgument(message) { throw new OidcError(ErrorCode.INVALID_ARGUMENT, message); } + +/** + * @param {string} message + * @returns {never} + */ +export function authenticationRequirementsNotMet(message) { + throw new OidcError(ErrorCode.AUTHENTICATION_REQUIREMENTS_NOT_MET, message); +} diff --git a/packages/oidc/tests/client.test.js b/packages/oidc/tests/client.test.js index 48a43511..3238c8a0 100644 --- a/packages/oidc/tests/client.test.js +++ b/packages/oidc/tests/client.test.js @@ -21,7 +21,7 @@ afterEach(async () => { * exchange (the sync stub handler can't await, but the id_token is known * once `authorize` has produced the nonce). */ -async function rig({ userinfo } = {}) { +async function rig({ userinfo, clientConfig } = {}) { const signer = await makeSigner(); const holder = { idToken: '' }; const as = await startStubAS({ @@ -41,6 +41,7 @@ async function rig({ userinfo } = {}) { redirectUri: REDIRECT_URI, // The stub AS serves its JWKS over http on loopback. jwksOptions: { allowInsecure: true }, + ...clientConfig, }); return { as, client, signer, holder }; } @@ -121,3 +122,47 @@ describe('createClient.handleCallback', () => { await assert.rejects(client.handleCallback({ code: 'auth-code', state: params.get('state') }, { session })); }); }); + +describe('createClient.handleCallback — max_age / acr_values', () => { + async function flow(clientConfig, claims, callbackOptions = {}) { + const { as, client, signer, holder } = await rig({ userinfo: () => ({ sub: 'user-1' }), clientConfig }); + const { url, session } = await client.authorize(); + const params = new URL(url).searchParams; + holder.idToken = await signer.mint({ + iss: as.issuer, + sub: 'user-1', + aud: CLIENT_ID, + nonce: params.get('nonce'), + ...claims, + }); + return { + params, + run: () => client.handleCallback({ code: 'c', state: params.get('state') }, { session, ...callbackOptions }), + }; + } + const now = () => Math.floor(Date.now() / 1000); + const notMet = { code: ErrorCode.AUTHENTICATION_REQUIREMENTS_NOT_MET }; + + it('sends the configured max_age and rejects a stale auth_time', async () => { + const { params, run } = await flow({ maxAge: 60 }, { auth_time: now() - 3600 }); + assert.equal(params.get('max_age'), '60'); + await assert.rejects(run(), notMet); + }); + + it('rejects a missing auth_time when max_age was requested', async () => { + const { run } = await flow({}, {}, { maxAge: 60 }); + await assert.rejects(run(), notMet); + }); + + it('accepts a fresh auth_time', async () => { + const { run } = await flow({ maxAge: 60 }, { auth_time: now() - 5 }); + assert.equal((await run()).claims.sub, 'user-1'); + }); + + it('rejects an acr outside the requested acr_values', async () => { + const { run } = await flow({ acrValues: 'urn:mfa urn:phr' }, { acr: 'urn:pwd' }); + await assert.rejects(run(), notMet); + const ok = await flow({ acrValues: 'urn:mfa urn:phr' }, { acr: 'urn:phr' }); + assert.equal((await ok.run()).claims.acr, 'urn:phr'); + }); +}); From 0891622d7e96a5d6670475a267bfe8d264b2d9f5 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:17:14 +0300 Subject: [PATCH 20/29] fix(oidc): session cookie name must be an RFC 6265 token checkSessionIframeHtml placed session.cookieName into an inline script with JSON.stringify (which does not escape ) and into new RegExp unescaped. A configured name containing broke out of the script; one containing regex metacharacters matched other cookies. The name must now be an RFC 6265 cookie-name token (INVALID_ARGUMENT otherwise) and is regex-escaped before it reaches the RegExp. --- packages/oidc/src/internal/session.js | 19 ++++++++++++++----- packages/oidc/tests/session.test.js | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/packages/oidc/src/internal/session.js b/packages/oidc/src/internal/session.js index 6dbc3bd8..0e5fb685 100644 --- a/packages/oidc/src/internal/session.js +++ b/packages/oidc/src/internal/session.js @@ -16,6 +16,11 @@ import { createHash, randomBytes } from 'node:crypto'; import { encode as base64urlEncode } from '@exortek/shared/base64url'; +import { invalidArgument } from './errors.js'; + +// RFC 6265 §4.1.1 cookie-name = token (RFC 2616 §2.2). +const COOKIE_NAME_TOKEN = /^[!#$%&'*+\-.^_`|~0-9A-Za-z]+$/; + /** * Compute a `session_state` value (OIDC Session Management §4.2): * `base64url(sha256(client_id + " " + origin + " " + op_browser_state + " " + @@ -42,19 +47,23 @@ export function computeSessionState(input) { */ export function checkSessionIframeHtml(options = {}) { const cookieName = options.cookieName ?? 'op_browser_state'; - // The cookie name is the only injected value; JSON.stringify keeps it a safe - // string literal inside the script. - const cookieLiteral = JSON.stringify(cookieName); + // The cookie name is the only injected value. Restricting it to an + // RFC 6265 token keeps `<`, quotes and whitespace out of the script + // entirely, and it is regex-escaped before it reaches `new RegExp`. + if (!COOKIE_NAME_TOKEN.test(cookieName)) { + invalidArgument('session.cookieName must be an RFC 6265 cookie-name token.'); + } + const cookieLiteral = JSON.stringify(cookieName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); return ` check_session_iframe