From 2cda71ded869cbc19b3c6902dd7a1344bb776bd2 Mon Sep 17 00:00:00 2001 From: Deepak Prabhakara Date: Fri, 26 Jun 2026 10:18:18 +0100 Subject: [PATCH 1/3] fixed checks for inresponseto and validateExpiration --- lib/response.ts | 47 +++- lib/saml20.ts | 67 +++++- lib/typings.ts | 14 ++ test/assets/certificates/testIdpCert.crt | 19 ++ test/assets/certificates/testIdpKey.pem | 28 +++ test/lib/saml20.inResponseTo.attacks.spec.ts | 214 +++++++++++++++++++ test/lib/saml20.spec.ts | 23 +- 7 files changed, 392 insertions(+), 20 deletions(-) create mode 100644 test/assets/certificates/testIdpCert.crt create mode 100644 test/assets/certificates/testIdpKey.pem create mode 100644 test/lib/saml20.inResponseTo.attacks.spec.ts diff --git a/lib/response.ts b/lib/response.ts index 852f1b48..121d8475 100644 --- a/lib/response.ts +++ b/lib/response.ts @@ -167,7 +167,7 @@ const validateInternal = async (rawAssertion, options, cb) => { // Save the js object derived from xml and check status code const assertionObj = await xmlToJs(decryptedSignedXml, cb); // this is our assertion object - parseResponseAndVersion(assertionObj, responseObj, function onParse(err, assertion, version) { + parseResponseAndVersion(assertionObj, responseObj, async function onParse(err, assertion, version) { if (err) { cb(err); return; @@ -188,11 +188,41 @@ const validateInternal = async (rawAssertion, options, cb) => { return; } - if (options.inResponseTo && assertion.inResponseTo && assertion.inResponseTo !== options.inResponseTo) { + // Fail closed: when the caller supplies an expected InResponseTo (an + // SP-initiated flow), the assertion must carry a matching, signed + // InResponseTo. A missing value is a mismatch, not a reason to skip the + // check, otherwise a captured assertion can be replayed into a different + // login by stripping InResponseTo from the unsigned wrapper. + if (options.inResponseTo && assertion.inResponseTo !== options.inResponseTo) { cb(new Error('Invalid InResponseTo.')); return; } + // Optional one-time replay protection. The library is stateless, so the + // caller supplies the store. The callback receives the signed assertion + // identifiers and must return true when the assertion has already been + // seen (a replay), keying entries by assertion ID until NotOnOrAfter. + if (typeof options.assertionReplayValidator === 'function') { + let alreadyUsed: boolean; + try { + alreadyUsed = await options.assertionReplayValidator({ + assertionId: tokenHandler.getAssertionId(assertion), + sessionIndex: assertion.AuthnStatement?.['@']?.SessionIndex, + notOnOrAfter: assertion.Conditions?.['@']?.NotOnOrAfter, + inResponseTo: assertion.inResponseTo, + }); + } catch (e) { + const error = new WrapError('An error occurred during assertion replay validation.'); + error.inner = e; + cb(error); + return; + } + if (alreadyUsed) { + cb(new Error('Assertion has already been used (replay detected).')); + return; + } + } + parseAttributes(assertion, tokenHandler, cb); }); }; @@ -266,7 +296,18 @@ function parseResponseAndVersion(assertionObj, responseObj, cb) { } const tokenHandler = tokenHandlers[version]; - assertion.inResponseTo = tokenHandler.getInResponseTo(responseObj); + + // Derive InResponseTo only from signed content so it cannot be forged by + // tampering with the unsigned wrapper. When the whole Response is + // signed, `assertionObj` is the Response subtree and Response/@InResponseTo is + // trustworthy. In the common assertion-only-signed case, fall back to the + // bearer SubjectConfirmationData/@InResponseTo, which is inside the signed + // assertion. The outer `responseObj` wrapper is never trusted here. + const signedResponseInResponseTo = assertionObj.Response + ? tokenHandler.getInResponseTo(assertionObj) + : undefined; + assertion.inResponseTo = + signedResponseInResponseTo || tokenHandler.getSubjectConfirmationInResponseTo(assertion); cb(null, assertion, version, response); } diff --git a/lib/saml20.ts b/lib/saml20.ts index 59e8701b..8f4f8a46 100644 --- a/lib/saml20.ts +++ b/lib/saml20.ts @@ -101,6 +101,8 @@ const parse = (assertion) => { claims: claims, issuer: getProp(assertion, 'Issuer'), sessionIndex: getProp(assertion, 'AuthnStatement.@.SessionIndex'), + assertionId: getAssertionId(assertion), + notOnOrAfter: getAttribute(assertion, 'Conditions.@.NotOnOrAfter'), }; }; @@ -134,23 +136,72 @@ const validateAudience = (assertion, realm, strictValidation = false) => { } }; +const clockSkewMs = 10 * 60 * 1000; // 10 minutes clock skew. + const validateExpiration = (assertion) => { - const dteNotBefore = getProp(assertion, 'Conditions.@.NotBefore'); - let notBefore: any = new Date(dteNotBefore); - notBefore = notBefore.setMinutes(notBefore.getMinutes() - 10); // 10 minutes clock skew + const dteNotBefore = getAttribute(assertion, 'Conditions.@.NotBefore'); + const dteNotOnOrAfter = getAttribute(assertion, 'Conditions.@.NotOnOrAfter'); + + // A bounded validity window is required. A missing or unparseable + // NotBefore/NotOnOrAfter must be treated as invalid (expired) rather than + // "never expires": new Date(undefined) yields NaN, and NaN comparisons are + // always false, so the original `!(now < NaN || now > NaN)` evaluated to true. + if (!dteNotBefore || !dteNotOnOrAfter) { + return false; + } - const dteNotOnOrAfter = getProp(assertion, 'Conditions.@.NotOnOrAfter'); - let notOnOrAfter: any = new Date(dteNotOnOrAfter); - notOnOrAfter = notOnOrAfter.setMinutes(notOnOrAfter.getMinutes() + 10); // 10 minutes clock skew + const notBeforeMs = new Date(dteNotBefore).getTime(); + const notOnOrAfterMs = new Date(dteNotOnOrAfter).getTime(); + if (Number.isNaN(notBeforeMs) || Number.isNaN(notOnOrAfterMs)) { + return false; + } - const now = new Date(); + const notBefore = notBeforeMs - clockSkewMs; + const notOnOrAfter = notOnOrAfterMs + clockSkewMs; + const now = Date.now(); return !(now < notBefore || now > notOnOrAfter); }; +// InResponseTo read from the outer wrapper. Only trust this when the +// whole Response is signed; the wrapper is unsigned in the common +// assertion-only-signed case. const getInResponseTo = (xml) => { return getProp(xml, 'Response.@.InResponseTo'); }; -const saml20 = { getInResponseTo, validateExpiration, validateAudience, parse }; +// InResponseTo carried inside the bearer SubjectConfirmationData. This element +// lives inside the and is therefore covered by the assertion +// signature even when the outer wrapper is not signed. +const getSubjectConfirmationInResponseTo = (assertion): string | undefined => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let confirmations = getAttribute(assertion, 'Subject.SubjectConfirmation'); + if (!confirmations) { + return undefined; + } + confirmations = Array.isArray(confirmations) ? confirmations : [confirmations]; + for (const confirmation of confirmations) { + const inResponseTo = getAttribute( + confirmation, + 'SubjectConfirmationData.@.InResponseTo' + ); + if (inResponseTo) { + return inResponseTo; + } + } + return undefined; +}; + +const getAssertionId = (assertion): string | undefined => { + return getAttribute(assertion, '@.ID'); +}; + +const saml20 = { + getInResponseTo, + getSubjectConfirmationInResponseTo, + getAssertionId, + validateExpiration, + validateAudience, + parse, +}; export default saml20; diff --git a/lib/typings.ts b/lib/typings.ts index 10f2bb05..4f420034 100644 --- a/lib/typings.ts +++ b/lib/typings.ts @@ -15,4 +15,18 @@ export interface SAMLProfile { claims: Record; issuer: string; sessionIndex: string; + // Signed assertion identifier and validity bound, exposed so callers can + // implement one-time replay protection keyed by assertionId until notOnOrAfter. + assertionId?: string; + notOnOrAfter?: string; +} + +// Identifiers from the signed assertion passed to a caller-supplied replay +// validator. The validator returns true when the assertion has already been +// used (a replay) and must be rejected. +export interface AssertionReplayInfo { + assertionId?: string; + sessionIndex?: string; + notOnOrAfter?: string; + inResponseTo?: string; } diff --git a/test/assets/certificates/testIdpCert.crt b/test/assets/certificates/testIdpCert.crt new file mode 100644 index 00000000..13edf743 --- /dev/null +++ b/test/assets/certificates/testIdpCert.crt @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDBzCCAe+gAwIBAgIUfmiCRt513V6v242KcE4fS3tXnqkwDQYJKoZIhvcNAQEL +BQAwEzERMA8GA1UEAwwIdGVzdC1pZHAwHhcNMjYwNjIyMjM1MzI2WhcNNDYwNjE3 +MjM1MzI2WjATMREwDwYDVQQDDAh0ZXN0LWlkcDCCASIwDQYJKoZIhvcNAQEBBQAD +ggEPADCCAQoCggEBAM6XayR93ey62b2EpioV03K2eW4xB3psPUdQQ5cxqUgjUjKy +jSVleHcCCfHf/uqVYibxlUc5tfTRARNdCx8lB6Ob+MZLdTI0NS2tso5LBHvgywpe +ZqdD3aBHVHczxGVAkTX18XdwR0Ri31gPcZGP+tChKInJeTPo6i/tVbWmON/kP3NJ +mWYHrWfZHe+vKuytAE7Oqqn+LKM45FR6KlFjWuW11voshatRRWnGARIBDiQL2477 +id7d/UnSXZcqYgq34OfpNAy56v25YPFn7t4xtMa/8ZJGfsbKX24JNvlwLXFKT0v5 +y0KOCiAjVRjuMrOB1pobVwDAnjtxQyn1QAuxfCcCAwEAAaNTMFEwHQYDVR0OBBYE +FFhkCT/OWx/bn8P1AQYq8vxSEBH5MB8GA1UdIwQYMBaAFFhkCT/OWx/bn8P1AQYq +8vxSEBH5MA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBAG2r5QR1 +WRfD25DrjK8zMcm4NxZCmBvrxHO+f1Z9I5pdCazXrRY66jMt+VVayYZ/eZez+tPH +PEeFNaHBQLnpQdI7HQsAYU+f79bgctSRMbeT3Mjdx5b8mhp9I256Zs/NyTYeEp8V +wk9sjvhrr/pBA+t6oQKJwvfK7+u3pE42eIWxPSYqNQRKrhKPX4Ej7/3EQyMNfIwo +KF2WJYnXhe2IyccsOXayHtQkTLXOccIYkLIbgTb6auZKSMo7EvPN+op0Vhvjk49u +VfPLI076c1mSFrVL/x0yQ8c0YFf0eAFCl4pcKrNp125gT9MW1QAMgqKV/7tx8RnW +zEstp1SkQL9ZCa0= +-----END CERTIFICATE----- diff --git a/test/assets/certificates/testIdpKey.pem b/test/assets/certificates/testIdpKey.pem new file mode 100644 index 00000000..2dbedd3f --- /dev/null +++ b/test/assets/certificates/testIdpKey.pem @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDOl2skfd3sutm9 +hKYqFdNytnluMQd6bD1HUEOXMalII1Iyso0lZXh3Agnx3/7qlWIm8ZVHObX00QET +XQsfJQejm/jGS3UyNDUtrbKOSwR74MsKXmanQ92gR1R3M8RlQJE19fF3cEdEYt9Y +D3GRj/rQoSiJyXkz6Oov7VW1pjjf5D9zSZlmB61n2R3vryrsrQBOzqqp/iyjOORU +eipRY1rltdb6LIWrUUVpxgESAQ4kC9uO+4ne3f1J0l2XKmIKt+Dn6TQMuer9uWDx +Z+7eMbTGv/GSRn7Gyl9uCTb5cC1xSk9L+ctCjgogI1UY7jKzgdaaG1cAwJ47cUMp +9UALsXwnAgMBAAECggEAAzkkkEywbjWaGRVdPFHb2zHSoi/8pGHU8OxlKZI6SGhY +q3bSse8r2nt7KT1r7kAHaIEjaZmSZ6/tGt68Qi+jN1/DGWDrAq4C2GQZ4ZN1DfO7 +Zz2Cz4BFEG+cd0GlAkloGpXsPwdO7Ve3kVmoVXOQH7or9j5g+DjdkoLa7/sYbxHK +6FNHQIXOsizZ+MhcIQBX3eraoqcpaWafED0nvwocJhtN7AVmVXulW79Fs8P09egh ++qTY456XXf/bkpMcedc4sWso3bvvA1j+iS0Rr8bHzAPqA1B3Z0iKgkmvJsJlm8fF +vgt7nOFsm8C15SmZikvPShvkxh8A4RHAQbpNY/hK1QKBgQDiZccgF89som7My9DA +ttN9OMclc5QC6mQ0E52PPKGIW3sSuDav03rg3hIyrpFcPa/hq8gq4nzByBL/nnL1 +bO4Fofi52Anicnn/O06M0eHvmP43DZ5mBiu4cf4x6IaQmf1U8JrUZF9lavyW04ai +aNTTRggIZlm7fBLQMhJdFrN4ywKBgQDpmquDvt6xMK+BrXzLQyBGDnaLeE/bt5IZ +kUzGGGxmc9VcZi7UftZOqlbuzS0qfE48Ck0miVdYpt7lL8BMtOXjoBu4MrwdQQMS +jYNynGfpiHY8Zr4zGoPqQDg56FdObo1RB5EfljFGZnTSFPW4oHbJg5loYyBoTMsm +3LTqJkTKlQKBgQCZjXJrP/r9sYX4/VwO+XGkAvh/XE7NU3C3KX66AeOFepaU8cCV +rJgxIC2zllcc+vHp2/sdqxP20t6f5TYPY9xkkaEDW5YIsqAwDmeOd2QIf/ocGO6Q +QCszJI3GB/IM7YS3MaGx4IobXV8IZVtxmCyRR3R3TgQad2LDNtLhtF3x1QKBgQDm +rJfPGYx3dfbo27KOWLOm2iNPJ7fb5BJ98s/YEUgBh0JZ4oE9zh27QlNjrfF6sZLj +kNyMQDSjUuxpblS6qisUMgcNRfQiAw+Qo3L4mt+1aM4waNhKSFWY3F9pNzf3OA2N +xSYWBc6UkRmsVYwrCzEhXjT/MltPAv3cWza+vJlTXQKBgClaYJ7A7Z5USwdTwNJ5 +UZp30uxKFMnSO9DhsmMgGPBQRsS5UPlDRHCQqoyx1aJyPuaaJICZL7QcCWwA1pCD +wO9vxti8MdAk32F5UlR5FQz6bWrsgLWWY7+eA+FAATxSoMZoxQ1aEO0eiKb86K2E +A55k9iw1jIhb1Oc4hjbjDrZ6 +-----END PRIVATE KEY----- diff --git a/test/lib/saml20.inResponseTo.attacks.spec.ts b/test/lib/saml20.inResponseTo.attacks.spec.ts new file mode 100644 index 00000000..6e1e0ce1 --- /dev/null +++ b/test/lib/saml20.inResponseTo.attacks.spec.ts @@ -0,0 +1,214 @@ +import assert from 'assert'; +import fs from 'fs'; +import { sign } from '../../lib/sign'; +import { validate } from '../../lib/response'; +import saml20 from '../../lib/saml20'; + +// Regression tests for the SP-initiated assertion-injection / InResponseTo +// replay-binding bypass and the validateExpiration NaN defect. +// +// Threat model: an attacker captures one validly signed (assertion-only-signed) +// victim assertion and replays it into their own freshly started SP-initiated +// login, where the caller enforces inResponseTo = attackerSessionId. The outer +// wrapper is unsigned, so the attacker can edit it freely. + +const privateKey = fs.readFileSync('./test/assets/certificates/testIdpKey.pem').toString(); +const cert = fs.readFileSync('./test/assets/certificates/testIdpCert.crt').toString(); + +const AUDIENCE = 'https://sp.jackson.example/saml'; +const VICTIM_REQ_ID = 'ORIGINAL_IDP_REQUEST_ID'; +const ATTACKER_SESSION_ID = '_attacker_fresh_session'; +const NAME_ID_CLAIM = 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier'; + +interface BuildOpts { + // Include InResponseTo on the outer wrapper (unsigned). + responseInResponseTo?: string | null; + // Include InResponseTo inside the signed bearer SubjectConfirmationData. + subjectInResponseTo?: string | null; + // Emit a Conditions validity window. + withConditionsWindow?: boolean; +} + +// Build an assertion-only-signed SAML Response. Only the is signed, +// which is the SAML default and what most IdPs emit. +function buildAssertionOnlySigned({ + responseInResponseTo = VICTIM_REQ_ID, + subjectInResponseTo = null, + withConditionsWindow = true, +}: BuildOpts = {}): string { + const now = new Date(); + const past = new Date(now.getTime() - 3600_000).toISOString(); + const future = new Date(now.getTime() + 3600_000).toISOString(); + const t = now.toISOString(); + + const responseIrtAttr = responseInResponseTo ? ` InResponseTo="${responseInResponseTo}"` : ''; + const subjectIrtAttr = subjectInResponseTo ? ` InResponseTo="${subjectInResponseTo}"` : ''; + const conditions = withConditionsWindow + ? `${AUDIENCE}` + : `${AUDIENCE}`; + + const xml = + `` + + `https://idp` + + `` + + `` + + `https://idp` + + `victim@target.com` + + `` + + conditions + + `urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport` + + `victim@target.com` + + `` + + ``; + + return sign(xml, privateKey, cert, '//*[local-name(.)="Assertion"]'); +} + +function stripResponseInResponseTo(xml: string): string { + return xml.replace(new RegExp(`(]*?) InResponseTo="${VICTIM_REQ_ID}"`), '$1'); +} + +async function expectReject(xml: string, message: string, opts: Record) { + await assert.rejects( + () => validate(xml, { audience: AUDIENCE, publicKey: cert, ...opts }), + (err: Error) => { + assert.strictEqual(err.message, message); + return true; + } + ); +} + +describe('saml20.attacks: InResponseTo replay binding (SP-initiated)', () => { + it('rejects an unchanged captured assertion replayed into a different session (negative control)', async () => { + const signed = buildAssertionOnlySigned({ subjectInResponseTo: VICTIM_REQ_ID }); + await expectReject(signed, 'Invalid InResponseTo.', { inResponseTo: ATTACKER_SESSION_ID }); + }); + + it('rejects when the outer InResponseTo is stripped and no signed InResponseTo is present', async () => { + // PoC: InResponseTo lived only on the unsigned wrapper. Stripping it must + // not silently skip the binding check. + const signed = buildAssertionOnlySigned({ subjectInResponseTo: null }); + await expectReject(stripResponseInResponseTo(signed), 'Invalid InResponseTo.', { + inResponseTo: ATTACKER_SESSION_ID, + }); + }); + + it('rejects when the outer InResponseTo is stripped but the signed assertion binds to the victim request', async () => { + // The signed SubjectConfirmationData still names the victim's request id, + // which does not match the attacker's session. + const signed = buildAssertionOnlySigned({ subjectInResponseTo: VICTIM_REQ_ID }); + await expectReject(stripResponseInResponseTo(signed), 'Invalid InResponseTo.', { + inResponseTo: ATTACKER_SESSION_ID, + }); + }); + + it('accepts a legitimate SP-initiated login bound via the signed SubjectConfirmationData', async () => { + // Outer wrapper carries no InResponseTo; the trustworthy value lives in the + // signed assertion and matches the caller-supplied request id. + const signed = buildAssertionOnlySigned({ + responseInResponseTo: null, + subjectInResponseTo: VICTIM_REQ_ID, + }); + const profile = await validate(signed, { + audience: AUDIENCE, + publicKey: cert, + inResponseTo: VICTIM_REQ_ID, + }); + assert.strictEqual(profile.claims[NAME_ID_CLAIM], 'victim@target.com'); + }); + + it('does not enforce binding for IdP-initiated logins (no inResponseTo supplied)', async () => { + const signed = buildAssertionOnlySigned({ responseInResponseTo: null, subjectInResponseTo: null }); + const profile = await validate(signed, { audience: AUDIENCE, publicKey: cert }); + assert.strictEqual(profile.claims[NAME_ID_CLAIM], 'victim@target.com'); + }); +}); + +describe('saml20: one-time assertion replay protection', () => { + it('rejects a second use of the same assertion when a replay validator is supplied', async () => { + const signed = buildAssertionOnlySigned({ subjectInResponseTo: VICTIM_REQ_ID }); + const seen = new Set(); + const assertionReplayValidator = async ({ assertionId }: { assertionId?: string }) => { + if (!assertionId) return false; + if (seen.has(assertionId)) return true; + seen.add(assertionId); + return false; + }; + + const opts = { + audience: AUDIENCE, + publicKey: cert, + inResponseTo: VICTIM_REQ_ID, + assertionReplayValidator, + }; + + const profile = await validate(signed, opts); + assert.strictEqual(profile.assertionId, '_assert1'); + + await assert.rejects( + () => validate(signed, opts), + (err: Error) => { + assert.strictEqual(err.message, 'Assertion has already been used (replay detected).'); + return true; + } + ); + }); + + it('surfaces errors thrown by the replay validator without leaking a valid profile', async () => { + const signed = buildAssertionOnlySigned({ subjectInResponseTo: VICTIM_REQ_ID }); + await assert.rejects( + () => + validate(signed, { + audience: AUDIENCE, + publicKey: cert, + inResponseTo: VICTIM_REQ_ID, + assertionReplayValidator: async () => { + throw new Error('store unavailable'); + }, + }), + (err: Error) => { + assert.strictEqual(err.message, 'An error occurred during assertion replay validation.'); + return true; + } + ); + }); +}); + +describe('saml20.validateExpiration: bounded validity window required', () => { + it('treats a missing Conditions window as expired (was "never expires")', () => { + assert.strictEqual(saml20.validateExpiration({}), false); + }); + + it('treats a missing NotOnOrAfter as expired', () => { + assert.strictEqual( + saml20.validateExpiration({ Conditions: { '@': { NotBefore: '2020-01-01T00:00:00Z' } } }), + false + ); + }); + + it('treats an unparseable date as expired', () => { + assert.strictEqual( + saml20.validateExpiration({ + Conditions: { '@': { NotBefore: 'not-a-date', NotOnOrAfter: 'also-bad' } }, + }), + false + ); + }); + + it('accepts a current, bounded validity window', () => { + const past = new Date(Date.now() - 3600_000).toISOString(); + const future = new Date(Date.now() + 3600_000).toISOString(); + assert.strictEqual( + saml20.validateExpiration({ Conditions: { '@': { NotBefore: past, NotOnOrAfter: future } } }), + true + ); + }); + + it('rejects an assertion whose signed Conditions carry no window during validate()', async () => { + const signed = buildAssertionOnlySigned({ + subjectInResponseTo: VICTIM_REQ_ID, + withConditionsWindow: false, + }); + await expectReject(signed, 'Assertion is expired.', { inResponseTo: VICTIM_REQ_ID }); + }); +}); diff --git a/test/lib/saml20.spec.ts b/test/lib/saml20.spec.ts index 656d7181..68f8a34f 100644 --- a/test/lib/saml20.spec.ts +++ b/test/lib/saml20.spec.ts @@ -138,18 +138,23 @@ describe('saml20.ts', function () { }); it('validateExpiration ok', function () { - const value = saml20.validateExpiration(assertion); - assert.strictEqual(value, true); - assert(saml20.validateExpiration(assertion)); + // A parsed assertion with a current, bounded validity window is valid. + const assertionWithWindow = { + Conditions: { + '@': { + NotBefore: new Date(Date.now() - 3600_000).toISOString(), + NotOnOrAfter: new Date(Date.now() + 3600_000).toISOString(), + }, + }, + }; + assert.strictEqual(saml20.validateExpiration(assertionWithWindow), true); }); it('validateExpiration not ok', function () { - try { - const value = saml20.validateExpiration('assertion'); - assert.strictEqual(value, true); - } catch (error) { - assert(error); - } + // A missing validity window must be treated as invalid (expired), not as + // "never expires". + assert.strictEqual(saml20.validateExpiration('assertion'), false); + assert.strictEqual(saml20.validateExpiration({}), false); }); it('getClaims with friendly names', function () { const attributes = [ From 7b57c0abcfa59218b7a9f04300c1e8548d512c4c Mon Sep 17 00:00:00 2001 From: Deepak Prabhakara Date: Fri, 26 Jun 2026 14:27:40 +0100 Subject: [PATCH 2/3] cater to SubjectConfirmationData.@.NotOnOrAfter --- lib/saml20.ts | 92 ++++++++++++++------ test/lib/saml20.inResponseTo.attacks.spec.ts | 69 ++++++++++++--- 2 files changed, 120 insertions(+), 41 deletions(-) diff --git a/lib/saml20.ts b/lib/saml20.ts index 8f4f8a46..82815112 100644 --- a/lib/saml20.ts +++ b/lib/saml20.ts @@ -138,28 +138,75 @@ const validateAudience = (assertion, realm, strictValidation = false) => { const clockSkewMs = 10 * 60 * 1000; // 10 minutes clock skew. +// Collect every SubjectConfirmationData element across all SubjectConfirmation +// entries. Bearer assertions carry their expiration here rather than (or in +// addition to) Conditions. +const getSubjectConfirmationData = (assertion): Record[] => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let confirmations = getAttribute(assertion, 'Subject.SubjectConfirmation'); + if (!confirmations) { + return []; + } + confirmations = Array.isArray(confirmations) ? confirmations : [confirmations]; + const data: Record[] = []; + for (const confirmation of confirmations) { + let scd = getAttribute | Record[]>( + confirmation, + 'SubjectConfirmationData' + ); + if (!scd) { + continue; + } + scd = Array.isArray(scd) ? scd : [scd]; + data.push(...scd); + } + return data; +}; + const validateExpiration = (assertion) => { - const dteNotBefore = getAttribute(assertion, 'Conditions.@.NotBefore'); - const dteNotOnOrAfter = getAttribute(assertion, 'Conditions.@.NotOnOrAfter'); - - // A bounded validity window is required. A missing or unparseable - // NotBefore/NotOnOrAfter must be treated as invalid (expired) rather than - // "never expires": new Date(undefined) yields NaN, and NaN comparisons are - // always false, so the original `!(now < NaN || now > NaN)` evaluated to true. - if (!dteNotBefore || !dteNotOnOrAfter) { - return false; + const now = Date.now(); + + // A SAML assertion need not carry a Conditions window: a bearer assertion is + // time-boxed by SubjectConfirmationData/@NotOnOrAfter instead. Gather every + // NotBefore (lower) and NotOnOrAfter (upper) bound from both places and + // enforce each one. An assertion with no upper bound anywhere is rejected + // rather than treated as "never expires" — that was the original NaN defect, + // where new Date(undefined) made `!(now < NaN || now > NaN)` evaluate to true. + const lowerBounds: string[] = []; + const upperBounds: string[] = []; + + const conditionsNotBefore = getAttribute(assertion, 'Conditions.@.NotBefore'); + const conditionsNotOnOrAfter = getAttribute(assertion, 'Conditions.@.NotOnOrAfter'); + if (conditionsNotBefore) lowerBounds.push(conditionsNotBefore); + if (conditionsNotOnOrAfter) upperBounds.push(conditionsNotOnOrAfter); + + for (const scd of getSubjectConfirmationData(assertion)) { + const attrs = (scd['@'] as Record | undefined) ?? {}; + if (attrs.NotBefore) lowerBounds.push(attrs.NotBefore); + if (attrs.NotOnOrAfter) upperBounds.push(attrs.NotOnOrAfter); } - const notBeforeMs = new Date(dteNotBefore).getTime(); - const notOnOrAfterMs = new Date(dteNotOnOrAfter).getTime(); - if (Number.isNaN(notBeforeMs) || Number.isNaN(notOnOrAfterMs)) { + // Require at least one enforceable expiration. + if (upperBounds.length === 0) { return false; } - const notBefore = notBeforeMs - clockSkewMs; - const notOnOrAfter = notOnOrAfterMs + clockSkewMs; - const now = Date.now(); - return !(now < notBefore || now > notOnOrAfter); + // Every present bound must be parseable and currently satisfied. A bound that + // is present but unparseable is treated as invalid. + for (const value of lowerBounds) { + const ms = new Date(value).getTime(); + if (Number.isNaN(ms) || now < ms - clockSkewMs) { + return false; + } + } + for (const value of upperBounds) { + const ms = new Date(value).getTime(); + if (Number.isNaN(ms) || now > ms + clockSkewMs) { + return false; + } + } + + return true; }; // InResponseTo read from the outer wrapper. Only trust this when the @@ -173,17 +220,8 @@ const getInResponseTo = (xml) => { // lives inside the and is therefore covered by the assertion // signature even when the outer wrapper is not signed. const getSubjectConfirmationInResponseTo = (assertion): string | undefined => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let confirmations = getAttribute(assertion, 'Subject.SubjectConfirmation'); - if (!confirmations) { - return undefined; - } - confirmations = Array.isArray(confirmations) ? confirmations : [confirmations]; - for (const confirmation of confirmations) { - const inResponseTo = getAttribute( - confirmation, - 'SubjectConfirmationData.@.InResponseTo' - ); + for (const scd of getSubjectConfirmationData(assertion)) { + const inResponseTo = (scd['@'] as Record | undefined)?.InResponseTo; if (inResponseTo) { return inResponseTo; } diff --git a/test/lib/saml20.inResponseTo.attacks.spec.ts b/test/lib/saml20.inResponseTo.attacks.spec.ts index 6e1e0ce1..d7fb49c6 100644 --- a/test/lib/saml20.inResponseTo.attacks.spec.ts +++ b/test/lib/saml20.inResponseTo.attacks.spec.ts @@ -27,6 +27,8 @@ interface BuildOpts { subjectInResponseTo?: string | null; // Emit a Conditions validity window. withConditionsWindow?: boolean; + // Emit NotOnOrAfter on the bearer SubjectConfirmationData. + withSubjectNotOnOrAfter?: boolean; } // Build an assertion-only-signed SAML Response. Only the is signed, @@ -35,6 +37,7 @@ function buildAssertionOnlySigned({ responseInResponseTo = VICTIM_REQ_ID, subjectInResponseTo = null, withConditionsWindow = true, + withSubjectNotOnOrAfter = true, }: BuildOpts = {}): string { const now = new Date(); const past = new Date(now.getTime() - 3600_000).toISOString(); @@ -43,6 +46,7 @@ function buildAssertionOnlySigned({ const responseIrtAttr = responseInResponseTo ? ` InResponseTo="${responseInResponseTo}"` : ''; const subjectIrtAttr = subjectInResponseTo ? ` InResponseTo="${subjectInResponseTo}"` : ''; + const subjectNotOnOrAfterAttr = withSubjectNotOnOrAfter ? ` NotOnOrAfter="${future}"` : ''; const conditions = withConditionsWindow ? `${AUDIENCE}` : `${AUDIENCE}`; @@ -54,7 +58,7 @@ function buildAssertionOnlySigned({ `` + `https://idp` + `victim@target.com` + - `` + + `` + conditions + `urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport` + `victim@target.com` + @@ -174,40 +178,77 @@ describe('saml20: one-time assertion replay protection', () => { }); }); -describe('saml20.validateExpiration: bounded validity window required', () => { - it('treats a missing Conditions window as expired (was "never expires")', () => { +describe('saml20.validateExpiration: at least one enforceable upper bound required', () => { + const past = () => new Date(Date.now() - 3600_000).toISOString(); + const future = () => new Date(Date.now() + 3600_000).toISOString(); + + it('treats an assertion with no expiration anywhere as expired (was "never expires")', () => { assert.strictEqual(saml20.validateExpiration({}), false); }); - it('treats a missing NotOnOrAfter as expired', () => { + it('treats a NotBefore-only Conditions (no upper bound) as expired', () => { + assert.strictEqual(saml20.validateExpiration({ Conditions: { '@': { NotBefore: past() } } }), false); + }); + + it('treats a present-but-unparseable bound as expired', () => { assert.strictEqual( - saml20.validateExpiration({ Conditions: { '@': { NotBefore: '2020-01-01T00:00:00Z' } } }), + saml20.validateExpiration({ Conditions: { '@': { NotBefore: past(), NotOnOrAfter: 'not-a-date' } } }), false ); }); - it('treats an unparseable date as expired', () => { + it('accepts a current Conditions validity window', () => { + assert.strictEqual( + saml20.validateExpiration({ Conditions: { '@': { NotBefore: past(), NotOnOrAfter: future() } } }), + true + ); + }); + + it('accepts a bearer assertion bounded only by SubjectConfirmationData/@NotOnOrAfter', () => { + // No Conditions window, but a signed bearer expiration is present. This is a + // spec-valid bearer assertion and must not be rejected. assert.strictEqual( saml20.validateExpiration({ - Conditions: { '@': { NotBefore: 'not-a-date', NotOnOrAfter: 'also-bad' } }, + Subject: { SubjectConfirmation: { SubjectConfirmationData: { '@': { NotOnOrAfter: future() } } } }, }), - false + true ); }); - it('accepts a current, bounded validity window', () => { - const past = new Date(Date.now() - 3600_000).toISOString(); - const future = new Date(Date.now() + 3600_000).toISOString(); + it('rejects when any SubjectConfirmationData upper bound is in the past', () => { assert.strictEqual( - saml20.validateExpiration({ Conditions: { '@': { NotBefore: past, NotOnOrAfter: future } } }), - true + saml20.validateExpiration({ + Subject: { + SubjectConfirmation: [ + { SubjectConfirmationData: { '@': { NotOnOrAfter: future() } } }, + { SubjectConfirmationData: { '@': { NotOnOrAfter: '2000-01-01T00:00:00Z' } } }, + ], + }, + }), + false ); }); - it('rejects an assertion whose signed Conditions carry no window during validate()', async () => { + it('accepts a bearer login (no Conditions window) end-to-end via validate()', async () => { + const signed = buildAssertionOnlySigned({ + responseInResponseTo: null, + subjectInResponseTo: VICTIM_REQ_ID, + withConditionsWindow: false, + withSubjectNotOnOrAfter: true, + }); + const profile = await validate(signed, { + audience: AUDIENCE, + publicKey: cert, + inResponseTo: VICTIM_REQ_ID, + }); + assert.strictEqual(profile.claims[NAME_ID_CLAIM], 'victim@target.com'); + }); + + it('rejects an assertion with no expiration bound anywhere end-to-end via validate()', async () => { const signed = buildAssertionOnlySigned({ subjectInResponseTo: VICTIM_REQ_ID, withConditionsWindow: false, + withSubjectNotOnOrAfter: false, }); await expectReject(signed, 'Assertion is expired.', { inResponseTo: VICTIM_REQ_ID }); }); From a1b19998a3ecb05d539acf104f890347275ebce8 Mon Sep 17 00:00:00 2001 From: Deepak Prabhakara Date: Fri, 26 Jun 2026 18:10:20 +0100 Subject: [PATCH 3/3] addressed code review suggestions --- lib/response.ts | 111 +++++++++++-------- lib/saml20.ts | 106 ++++++++++++------ lib/typings.ts | 6 + test/lib/saml20.inResponseTo.attacks.spec.ts | 76 ++++++++++++- 4 files changed, 221 insertions(+), 78 deletions(-) diff --git a/lib/response.ts b/lib/response.ts index 121d8475..9929979b 100644 --- a/lib/response.ts +++ b/lib/response.ts @@ -129,6 +129,11 @@ const validateInternal = async (rawAssertion, options, cb) => { // rawAssertion is our SAML Response. However, we still call it rawAssertion because of legacy naming convention const responseObj = await xmlToJs(rawAssertion, cb); + // xmlToJs already invoked cb with the parse error; stop so we do not call cb + // twice or dereference an undefined object below. + if (!responseObj) { + return; + } checkStatusCode(responseObj, cb); let signedXml: string | null; @@ -150,6 +155,7 @@ const validateInternal = async (rawAssertion, options, cb) => { const error = new WrapError('Invalid assertion.'); error.inner = e; cb(error); + return; } } @@ -166,6 +172,11 @@ const validateInternal = async (rawAssertion, options, cb) => { // Save the js object derived from xml and check status code const assertionObj = await xmlToJs(decryptedSignedXml, cb); // this is our assertion object + // xmlToJs already invoked cb with the parse error; stop before + // parseResponseAndVersion dereferences an undefined object. + if (!assertionObj) { + return; + } parseResponseAndVersion(assertionObj, responseObj, async function onParse(err, assertion, version) { if (err) { @@ -173,57 +184,69 @@ const validateInternal = async (rawAssertion, options, cb) => { return; } - const tokenHandler = tokenHandlers[version]; - - if (!options.bypassExpiration && !tokenHandler.validateExpiration(assertion)) { - cb(new Error('Assertion is expired.')); - return; - } - - if ( - options.audience && - !tokenHandler.validateAudience(assertion, options.audience, options.strictAudienceValidation) - ) { - cb(new Error('Invalid audience.')); - return; - } + // onParse is async (the replay validator may be), so any synchronous throw + // here would become an unhandled rejection and leave validate() pending. + // Convert unexpected failures into a clean rejection. + try { + const tokenHandler = tokenHandlers[version]; - // Fail closed: when the caller supplies an expected InResponseTo (an - // SP-initiated flow), the assertion must carry a matching, signed - // InResponseTo. A missing value is a mismatch, not a reason to skip the - // check, otherwise a captured assertion can be replayed into a different - // login by stripping InResponseTo from the unsigned wrapper. - if (options.inResponseTo && assertion.inResponseTo !== options.inResponseTo) { - cb(new Error('Invalid InResponseTo.')); - return; - } + if (!options.bypassExpiration && !tokenHandler.validateExpiration(assertion)) { + cb(new Error('Assertion is expired.')); + return; + } - // Optional one-time replay protection. The library is stateless, so the - // caller supplies the store. The callback receives the signed assertion - // identifiers and must return true when the assertion has already been - // seen (a replay), keying entries by assertion ID until NotOnOrAfter. - if (typeof options.assertionReplayValidator === 'function') { - let alreadyUsed: boolean; - try { - alreadyUsed = await options.assertionReplayValidator({ - assertionId: tokenHandler.getAssertionId(assertion), - sessionIndex: assertion.AuthnStatement?.['@']?.SessionIndex, - notOnOrAfter: assertion.Conditions?.['@']?.NotOnOrAfter, - inResponseTo: assertion.inResponseTo, - }); - } catch (e) { - const error = new WrapError('An error occurred during assertion replay validation.'); - error.inner = e; - cb(error); + if ( + options.audience && + !tokenHandler.validateAudience(assertion, options.audience, options.strictAudienceValidation) + ) { + cb(new Error('Invalid audience.')); return; } - if (alreadyUsed) { - cb(new Error('Assertion has already been used (replay detected).')); + + // Fail closed: when the caller supplies an expected InResponseTo (an + // SP-initiated flow), the assertion must carry a matching, signed + // InResponseTo. A missing value is a mismatch, not a reason to skip the + // check, otherwise a captured assertion can be replayed into a different + // login by stripping InResponseTo from the unsigned wrapper. + if (options.inResponseTo && assertion.inResponseTo !== options.inResponseTo) { + cb(new Error('Invalid InResponseTo.')); return; } - } - parseAttributes(assertion, tokenHandler, cb); + // Optional one-time replay protection. The library is stateless, so the + // caller supplies the store. The callback receives the signed assertion + // identifiers and must return true when the assertion has already been + // seen (a replay), keying entries by assertion ID until NotOnOrAfter. + if (typeof options.assertionReplayValidator === 'function') { + let alreadyUsed: boolean; + try { + alreadyUsed = await options.assertionReplayValidator({ + assertionId: tokenHandler.getAssertionId(assertion), + sessionIndex: assertion.AuthnStatement?.['@']?.SessionIndex, + // Effective expiry (Conditions or the latest bearer confirmation), so + // the caller can size the replay-cache TTL to cover the whole window + // an assertion can still validate. + notOnOrAfter: tokenHandler.getNotOnOrAfter(assertion), + inResponseTo: assertion.inResponseTo, + }); + } catch (e) { + const error = new WrapError('An error occurred during assertion replay validation.'); + error.inner = e; + cb(error); + return; + } + if (alreadyUsed) { + cb(new Error('Assertion has already been used (replay detected).')); + return; + } + } + + parseAttributes(assertion, tokenHandler, cb); + } catch (e) { + const error = new WrapError('An error occurred trying to validate the assertion.'); + error.inner = e; + cb(error); + } }); }; diff --git a/lib/saml20.ts b/lib/saml20.ts index 82815112..298ca1fe 100644 --- a/lib/saml20.ts +++ b/lib/saml20.ts @@ -102,7 +102,7 @@ const parse = (assertion) => { issuer: getProp(assertion, 'Issuer'), sessionIndex: getProp(assertion, 'AuthnStatement.@.SessionIndex'), assertionId: getAssertionId(assertion), - notOnOrAfter: getAttribute(assertion, 'Conditions.@.NotOnOrAfter'), + notOnOrAfter: getNotOnOrAfter(assertion), }; }; @@ -163,50 +163,61 @@ const getSubjectConfirmationData = (assertion): Record[] => { return data; }; -const validateExpiration = (assertion) => { +// Check a [NotBefore, NotOnOrAfter] window against now, applying clock skew. +// Returns 'unbounded' when no NotOnOrAfter is present (the window has no upper +// limit), 'valid' when now is inside the window, and 'invalid' when a bound is +// present but unparseable or now falls outside it. +type WindowResult = 'valid' | 'invalid' | 'unbounded'; +const checkWindow = (notBefore?: string, notOnOrAfter?: string): WindowResult => { const now = Date.now(); - // A SAML assertion need not carry a Conditions window: a bearer assertion is - // time-boxed by SubjectConfirmationData/@NotOnOrAfter instead. Gather every - // NotBefore (lower) and NotOnOrAfter (upper) bound from both places and - // enforce each one. An assertion with no upper bound anywhere is rejected - // rather than treated as "never expires" — that was the original NaN defect, - // where new Date(undefined) made `!(now < NaN || now > NaN)` evaluate to true. - const lowerBounds: string[] = []; - const upperBounds: string[] = []; - - const conditionsNotBefore = getAttribute(assertion, 'Conditions.@.NotBefore'); - const conditionsNotOnOrAfter = getAttribute(assertion, 'Conditions.@.NotOnOrAfter'); - if (conditionsNotBefore) lowerBounds.push(conditionsNotBefore); - if (conditionsNotOnOrAfter) upperBounds.push(conditionsNotOnOrAfter); + if (notBefore) { + const ms = new Date(notBefore).getTime(); + if (Number.isNaN(ms) || now < ms - clockSkewMs) { + return 'invalid'; + } + } - for (const scd of getSubjectConfirmationData(assertion)) { - const attrs = (scd['@'] as Record | undefined) ?? {}; - if (attrs.NotBefore) lowerBounds.push(attrs.NotBefore); - if (attrs.NotOnOrAfter) upperBounds.push(attrs.NotOnOrAfter); + if (!notOnOrAfter) { + return 'unbounded'; } + const ms = new Date(notOnOrAfter).getTime(); + if (Number.isNaN(ms) || now > ms + clockSkewMs) { + return 'invalid'; + } + return 'valid'; +}; - // Require at least one enforceable expiration. - if (upperBounds.length === 0) { +const validateExpiration = (assertion) => { + // The window is an absolute constraint on the assertion: when + // present it must be satisfied. A present-but-unparseable bound is invalid. + const conditionsNotBefore = getAttribute(assertion, 'Conditions.@.NotBefore'); + const conditionsNotOnOrAfter = getAttribute(assertion, 'Conditions.@.NotOnOrAfter'); + const conditionsResult = checkWindow(conditionsNotBefore, conditionsNotOnOrAfter); + if (conditionsResult === 'invalid') { return false; } - // Every present bound must be parseable and currently satisfied. A bound that - // is present but unparseable is treated as invalid. - for (const value of lowerBounds) { - const ms = new Date(value).getTime(); - if (Number.isNaN(ms) || now < ms - clockSkewMs) { - return false; - } + // When supplies a satisfied upper bound, the assertion is + // time-boxed and currently within its validity window. + if (conditionsResult === 'valid') { + return true; } - for (const value of upperBounds) { - const ms = new Date(value).getTime(); - if (Number.isNaN(ms) || now > ms + clockSkewMs) { - return false; + + // Otherwise fall back to the bearer SubjectConfirmationData. Per SAML 2.0 + // core (2.4.1.1) multiple SubjectConfirmation elements are alternatives: + // satisfying any one is sufficient. The assertion is valid if at least one + // confirmation is currently within its window AND carries an upper bound. + // If no satisfied upper bound exists anywhere, the assertion is rejected + // rather than treated as "never expires" (the original NaN defect). + for (const scd of getSubjectConfirmationData(assertion)) { + const attrs = (scd['@'] as Record | undefined) ?? {}; + if (checkWindow(attrs.NotBefore, attrs.NotOnOrAfter) === 'valid') { + return true; } } - return true; + return false; }; // InResponseTo read from the outer wrapper. Only trust this when the @@ -233,10 +244,39 @@ const getAssertionId = (assertion): string | undefined => { return getAttribute(assertion, '@.ID'); }; +// The effective NotOnOrAfter after which the assertion can no longer validate, +// mirroring validateExpiration: the absolute Conditions/@NotOnOrAfter when +// present, otherwise the latest bearer SubjectConfirmationData/@NotOnOrAfter +// (SubjectConfirmations are alternatives, so the assertion remains usable until +// the last of them lapses). Callers key replay-cache TTLs off this value, so it +// must reflect the real upper bound rather than only the Conditions window. +const getNotOnOrAfter = (assertion): string | undefined => { + const conditionsNotOnOrAfter = getAttribute(assertion, 'Conditions.@.NotOnOrAfter'); + if (conditionsNotOnOrAfter) { + return conditionsNotOnOrAfter; + } + + let latest: string | undefined; + let latestMs = -Infinity; + for (const scd of getSubjectConfirmationData(assertion)) { + const value = (scd['@'] as Record | undefined)?.NotOnOrAfter; + if (!value) { + continue; + } + const ms = new Date(value).getTime(); + if (!Number.isNaN(ms) && ms > latestMs) { + latestMs = ms; + latest = value; + } + } + return latest; +}; + const saml20 = { getInResponseTo, getSubjectConfirmationInResponseTo, getAssertionId, + getNotOnOrAfter, validateExpiration, validateAudience, parse, diff --git a/lib/typings.ts b/lib/typings.ts index 4f420034..abfb99b6 100644 --- a/lib/typings.ts +++ b/lib/typings.ts @@ -27,6 +27,12 @@ export interface SAMLProfile { export interface AssertionReplayInfo { assertionId?: string; sessionIndex?: string; + // The assertion's effective NotOnOrAfter (the absolute Conditions bound when + // present, otherwise the latest bearer SubjectConfirmationData bound). This is + // the raw value from the assertion. validateExpiration accepts up to a + // 10-minute clock-skew tolerance beyond it, so a replay cache should keep the + // entry until at least notOnOrAfter + 10 minutes to cover the full window in + // which the assertion can still validate. notOnOrAfter?: string; inResponseTo?: string; } diff --git a/test/lib/saml20.inResponseTo.attacks.spec.ts b/test/lib/saml20.inResponseTo.attacks.spec.ts index d7fb49c6..caa4cc18 100644 --- a/test/lib/saml20.inResponseTo.attacks.spec.ts +++ b/test/lib/saml20.inResponseTo.attacks.spec.ts @@ -158,6 +158,52 @@ describe('saml20: one-time assertion replay protection', () => { ); }); + it('passes the Conditions NotOnOrAfter to the replay validator for TTL sizing', async () => { + const signed = buildAssertionOnlySigned({ + subjectInResponseTo: VICTIM_REQ_ID, + withConditionsWindow: true, + }); + let captured: { assertionId?: string; notOnOrAfter?: string } | undefined; + const profile = await validate(signed, { + audience: AUDIENCE, + publicKey: cert, + inResponseTo: VICTIM_REQ_ID, + assertionReplayValidator: async (info: { assertionId?: string; notOnOrAfter?: string }) => { + captured = info; + return false; + }, + }); + assert.ok(captured, 'replay validator was called'); + assert.strictEqual(captured!.assertionId, '_assert1'); + assert.ok(captured!.notOnOrAfter, 'notOnOrAfter is provided'); + assert.strictEqual(captured!.notOnOrAfter, profile.notOnOrAfter); + assert.ok(new Date(captured!.notOnOrAfter!).getTime() > Date.now()); + }); + + it('falls back to SubjectConfirmationData NotOnOrAfter when there is no Conditions window', async () => { + // The assertion is bearer-bounded only; the replay validator must still get + // a concrete expiry so it does not have to guess a TTL. + const signed = buildAssertionOnlySigned({ + responseInResponseTo: null, + subjectInResponseTo: VICTIM_REQ_ID, + withConditionsWindow: false, + withSubjectNotOnOrAfter: true, + }); + let captured: { notOnOrAfter?: string } | undefined; + const profile = await validate(signed, { + audience: AUDIENCE, + publicKey: cert, + inResponseTo: VICTIM_REQ_ID, + assertionReplayValidator: async (info: { notOnOrAfter?: string }) => { + captured = info; + return false; + }, + }); + assert.ok(captured!.notOnOrAfter, 'notOnOrAfter is provided from SubjectConfirmationData'); + assert.strictEqual(captured!.notOnOrAfter, profile.notOnOrAfter); + assert.ok(new Date(captured!.notOnOrAfter!).getTime() > Date.now()); + }); + it('surfaces errors thrown by the replay validator without leaking a valid profile', async () => { const signed = buildAssertionOnlySigned({ subjectInResponseTo: VICTIM_REQ_ID }); await assert.rejects( @@ -215,13 +261,29 @@ describe('saml20.validateExpiration: at least one enforceable upper bound requir ); }); - it('rejects when any SubjectConfirmationData upper bound is in the past', () => { + it('accepts when one SubjectConfirmation is stale but another is still valid (alternatives, OR)', () => { + // SAML 2.0 core 2.4.1.1: multiple SubjectConfirmations are alternatives; + // satisfying any one is sufficient. assert.strictEqual( saml20.validateExpiration({ Subject: { SubjectConfirmation: [ + { SubjectConfirmationData: { '@': { NotOnOrAfter: '2000-01-01T00:00:00Z' } } }, { SubjectConfirmationData: { '@': { NotOnOrAfter: future() } } }, + ], + }, + }), + true + ); + }); + + it('rejects when every SubjectConfirmation upper bound is in the past', () => { + assert.strictEqual( + saml20.validateExpiration({ + Subject: { + SubjectConfirmation: [ { SubjectConfirmationData: { '@': { NotOnOrAfter: '2000-01-01T00:00:00Z' } } }, + { SubjectConfirmationData: { '@': { NotOnOrAfter: '2010-01-01T00:00:00Z' } } }, ], }, }), @@ -229,6 +291,18 @@ describe('saml20.validateExpiration: at least one enforceable upper bound requir ); }); + it('honors a valid Conditions window even if a bearer confirmation has lapsed', () => { + assert.strictEqual( + saml20.validateExpiration({ + Conditions: { '@': { NotBefore: past(), NotOnOrAfter: future() } }, + Subject: { + SubjectConfirmation: { SubjectConfirmationData: { '@': { NotOnOrAfter: '2000-01-01T00:00:00Z' } } }, + }, + }), + true + ); + }); + it('accepts a bearer login (no Conditions window) end-to-end via validate()', async () => { const signed = buildAssertionOnlySigned({ responseInResponseTo: null,