From b790144a2e969f1f423c1226147edfb4e69664d1 Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Thu, 9 Apr 2026 18:30:01 +0100 Subject: [PATCH] fix(sso)!: harden SAML response validation (InResponseTo, Audience, SessionIndex) (#9055) --- .../saml-response-validation-hardening.md | 20 ++ packages/sso/src/routes/sso.ts | 212 ++++------------- packages/sso/src/saml.test.ts | 223 +++++++++++++++--- packages/sso/src/saml/index.ts | 2 + packages/sso/src/saml/response-validation.ts | 197 ++++++++++++++++ packages/sso/src/types.ts | 36 ++- 6 files changed, 491 insertions(+), 199 deletions(-) create mode 100644 .changeset/saml-response-validation-hardening.md create mode 100644 packages/sso/src/saml/response-validation.ts diff --git a/.changeset/saml-response-validation-hardening.md b/.changeset/saml-response-validation-hardening.md new file mode 100644 index 0000000000..0f923a4fc4 --- /dev/null +++ b/.changeset/saml-response-validation-hardening.md @@ -0,0 +1,20 @@ +--- +"@better-auth/sso": minor +--- + +fix(sso)!: harden SAML response validation (InResponseTo, Audience, SessionIndex) + +### Breaking Changes + +- **`allowIdpInitiated` now defaults to `false`** — IdP-initiated SSO (unsolicited SAML responses) is disabled by default. Set `saml.allowIdpInitiated: true` to restore the previous behavior. This aligns with the SAML2Int interoperability profile which recommends against IdP-initiated SSO due to its susceptibility to injection attacks. + +### Bug Fixes + +- **InResponseTo validation was completely non-functional** — The code read `extract.inResponseTo` (always `undefined`) instead of samlify's actual path `extract.response.inResponseTo`. SP-initiated InResponseTo validation now works as intended in both ACS handlers. +- **Audience Restriction was never validated** — SAML assertions issued for a different service provider were accepted without checking the `` element. Audience is now validated against the configured `samlConfig.audience` value per SAML 2.0 Core §2.5.1. +- **SessionIndex stored as object instead of string** — samlify returns `sessionIndex` from login responses as `{ authnInstant, sessionNotOnOrAfter, sessionIndex }`, but the code stored the whole object. SLO session-index comparisons always failed silently. The correct inner `sessionIndex` string is now extracted. + +### Improvements + +- Extracted shared `validateInResponseTo()` and `validateAudience()` into `packages/sso/src/saml/response-validation.ts`, eliminating ~160 lines of duplicated validation logic between the two ACS handlers. +- Fixed `SAMLAssertionExtract` type to match samlify's actual extractor output shape. diff --git a/packages/sso/src/routes/sso.ts b/packages/sso/src/routes/sso.ts index a035698b55..a4fac3f50e 100644 --- a/packages/sso/src/routes/sso.ts +++ b/packages/sso/src/routes/sso.ts @@ -43,7 +43,9 @@ import { mapDiscoveryErrorToAPIError, } from "../oidc"; import { + validateAudience, validateConfigAlgorithms, + validateInResponseTo, validateSAMLAlgorithms, validateSingleAssertion, } from "../saml"; @@ -2181,88 +2183,28 @@ export const callbackSSOSAML = (options?: SSOOptions) => { logger: ctx.context.logger, }); - const inResponseTo = (extract as SAMLAssertionExtract).inResponseTo as - | string - | undefined; - const shouldValidateInResponseTo = - options?.saml?.enableInResponseToValidation !== false; + const samlRedirectUrl = + relayState?.callbackURL || + parsedSamlConfig.callbackUrl || + ctx.context.baseURL; - if (shouldValidateInResponseTo) { - const allowIdpInitiated = options?.saml?.allowIdpInitiated !== false; + await validateInResponseTo(ctx, { + extract: extract as SAMLAssertionExtract, + providerId: provider.providerId, + options: { + enableInResponseToValidation: + options?.saml?.enableInResponseToValidation, + allowIdpInitiated: options?.saml?.allowIdpInitiated, + }, + redirectUrl: samlRedirectUrl, + }); - if (inResponseTo) { - let storedRequest: AuthnRequestRecord | null = null; - - const verification = - await ctx.context.internalAdapter.findVerificationValue( - `${constants.AUTHN_REQUEST_KEY_PREFIX}${inResponseTo}`, - ); - if (verification) { - try { - storedRequest = JSON.parse( - verification.value, - ) as AuthnRequestRecord; - if (storedRequest && storedRequest.expiresAt < Date.now()) { - storedRequest = null; - } - } catch { - storedRequest = null; - } - } - - if (!storedRequest) { - ctx.context.logger.error( - "SAML InResponseTo validation failed: unknown or expired request ID", - { inResponseTo, providerId: provider.providerId }, - ); - const redirectUrl = - relayState?.callbackURL || - parsedSamlConfig.callbackUrl || - ctx.context.baseURL; - throw ctx.redirect( - `${redirectUrl}?error=invalid_saml_response&error_description=Unknown+or+expired+request+ID`, - ); - } - - if (storedRequest.providerId !== provider.providerId) { - ctx.context.logger.error( - "SAML InResponseTo validation failed: provider mismatch", - { - inResponseTo, - expectedProvider: storedRequest.providerId, - actualProvider: provider.providerId, - }, - ); - - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - `${constants.AUTHN_REQUEST_KEY_PREFIX}${inResponseTo}`, - ); - const redirectUrl = - relayState?.callbackURL || - parsedSamlConfig.callbackUrl || - ctx.context.baseURL; - throw ctx.redirect( - `${redirectUrl}?error=invalid_saml_response&error_description=Provider+mismatch`, - ); - } - - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - `${constants.AUTHN_REQUEST_KEY_PREFIX}${inResponseTo}`, - ); - } else if (!allowIdpInitiated) { - ctx.context.logger.error( - "SAML IdP-initiated SSO rejected: InResponseTo missing and allowIdpInitiated is false", - { providerId: provider.providerId }, - ); - const redirectUrl = - relayState?.callbackURL || - parsedSamlConfig.callbackUrl || - ctx.context.baseURL; - throw ctx.redirect( - `${redirectUrl}?error=unsolicited_response&error_description=IdP-initiated+SSO+not+allowed`, - ); - } - } + validateAudience(ctx, { + extract: extract as SAMLAssertionExtract, + expectedAudience: parsedSamlConfig.audience, + providerId: provider.providerId, + redirectUrl: samlRedirectUrl, + }); // Assertion Replay Protection const samlContent = (parsedResponse as any).samlContent as @@ -2450,7 +2392,8 @@ export const callbackSSOSAML = (options?: SSOOptions) => { sessionId: session.id, providerId: provider.providerId, nameID: extract.nameID, - sessionIndex: (extract as SAMLAssertionExtract).sessionIndex, + sessionIndex: (extract as SAMLAssertionExtract).sessionIndex + ?.sessionIndex, }; await ctx.context.internalAdapter .createVerificationValue({ @@ -2703,87 +2646,28 @@ export const acsEndpoint = (options?: SSOOptions) => { logger: ctx.context.logger, }); - const inResponseToAcs = (extract as SAMLAssertionExtract).inResponseTo as - | string - | undefined; - const shouldValidateInResponseToAcs = - options?.saml?.enableInResponseToValidation !== false; + const acsRedirectUrl = + relayState?.callbackURL || + parsedSamlConfig.callbackUrl || + ctx.context.baseURL; - if (shouldValidateInResponseToAcs) { - const allowIdpInitiated = options?.saml?.allowIdpInitiated !== false; + await validateInResponseTo(ctx, { + extract: extract as SAMLAssertionExtract, + providerId, + options: { + enableInResponseToValidation: + options?.saml?.enableInResponseToValidation, + allowIdpInitiated: options?.saml?.allowIdpInitiated, + }, + redirectUrl: acsRedirectUrl, + }); - if (inResponseToAcs) { - let storedRequest: AuthnRequestRecord | null = null; - - const verification = - await ctx.context.internalAdapter.findVerificationValue( - `${constants.AUTHN_REQUEST_KEY_PREFIX}${inResponseToAcs}`, - ); - if (verification) { - try { - storedRequest = JSON.parse( - verification.value, - ) as AuthnRequestRecord; - if (storedRequest && storedRequest.expiresAt < Date.now()) { - storedRequest = null; - } - } catch { - storedRequest = null; - } - } - - if (!storedRequest) { - ctx.context.logger.error( - "SAML InResponseTo validation failed: unknown or expired request ID", - { inResponseTo: inResponseToAcs, providerId }, - ); - const redirectUrl = - relayState?.callbackURL || - parsedSamlConfig.callbackUrl || - ctx.context.baseURL; - throw ctx.redirect( - `${redirectUrl}?error=invalid_saml_response&error_description=Unknown+or+expired+request+ID`, - ); - } - - if (storedRequest.providerId !== providerId) { - ctx.context.logger.error( - "SAML InResponseTo validation failed: provider mismatch", - { - inResponseTo: inResponseToAcs, - expectedProvider: storedRequest.providerId, - actualProvider: providerId, - }, - ); - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - `${constants.AUTHN_REQUEST_KEY_PREFIX}${inResponseToAcs}`, - ); - const redirectUrl = - relayState?.callbackURL || - parsedSamlConfig.callbackUrl || - ctx.context.baseURL; - throw ctx.redirect( - `${redirectUrl}?error=invalid_saml_response&error_description=Provider+mismatch`, - ); - } - - await ctx.context.internalAdapter.deleteVerificationByIdentifier( - `${constants.AUTHN_REQUEST_KEY_PREFIX}${inResponseToAcs}`, - ); - } else if (!allowIdpInitiated) { - ctx.context.logger.error( - "SAML IdP-initiated SSO rejected: InResponseTo missing and allowIdpInitiated is false", - { providerId }, - ); - const redirectUrl = - relayState?.callbackURL || - parsedSamlConfig.callbackUrl || - ctx.context.baseURL; - throw ctx.redirect( - `${redirectUrl}?error=unsolicited_response&error_description=IdP-initiated+SSO+not+allowed`, - ); - } - } + validateAudience(ctx, { + extract: extract as SAMLAssertionExtract, + expectedAudience: parsedSamlConfig.audience, + providerId, + redirectUrl: acsRedirectUrl, + }); // Assertion Replay Protection const samlContentAcs = Buffer.from(SAMLResponse, "base64").toString( @@ -2971,7 +2855,8 @@ export const acsEndpoint = (options?: SSOOptions) => { sessionId: session.id, providerId, nameID: extract.nameID, - sessionIndex: (extract as SAMLAssertionExtract).sessionIndex, + sessionIndex: (extract as SAMLAssertionExtract).sessionIndex + ?.sessionIndex, }; await ctx.context.internalAdapter .createVerificationValue({ @@ -3186,7 +3071,10 @@ async function handleLogoutRequest( } const { nameID } = parsed.extract; - const sessionIndex = (parsed.extract as SAMLAssertionExtract).sessionIndex; + // LogoutRequest's sessionIndex is a plain string (text node), unlike the + // login response's multi-attribute object. Don't cast to SAMLAssertionExtract. + const sessionIndex = (parsed.extract as { sessionIndex?: string }) + .sessionIndex; const key = `${constants.SAML_SESSION_KEY_PREFIX}${providerId}:${nameID}`; const stored = await ctx.context.internalAdapter.findVerificationValue(key); diff --git a/packages/sso/src/saml.test.ts b/packages/sso/src/saml.test.ts index a0c85a157c..83e47f2664 100644 --- a/packages/sso/src/saml.test.ts +++ b/packages/sso/src/saml.test.ts @@ -1011,6 +1011,12 @@ describe("SAML SSO", async () => { attributes: userInfo.attributes, }; }), + saml: { + // Disable for the shared instance — InResponseTo validation has + // dedicated tests. Enabling it here would require every test to + // go through the SP-initiated flow to store an AuthnRequest first. + enableInResponseToValidation: false, + }, }; const auth = betterAuth({ @@ -1586,7 +1592,7 @@ describe("SAML SSO", async () => { it("should initiate SAML login and fallback to callbackUrl on invalid RelayState", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -1732,7 +1738,12 @@ describe("SAML SSO", async () => { it("should reject SAML sign-in when disableImplicitSignUp is true and user doesn't exist", async () => { const { auth: authWithDisabledSignUp, signInWithTestUser } = await getTestInstance({ - plugins: [sso({ disableImplicitSignUp: true })], + plugins: [ + sso({ + disableImplicitSignUp: true, + saml: { enableInResponseToValidation: false }, + }), + ], }); const { headers } = await signInWithTestUser(); @@ -1796,7 +1807,12 @@ describe("SAML SSO", async () => { it("should reject SAML ACS (IdP-initiated) when disableImplicitSignUp is true and user doesn't exist", async () => { const { auth: authWithDisabledSignUp, signInWithTestUser } = await getTestInstance({ - plugins: [sso({ disableImplicitSignUp: true })], + plugins: [ + sso({ + disableImplicitSignUp: true, + saml: { enableInResponseToValidation: false }, + }), + ], }); const { headers } = await signInWithTestUser(); @@ -1862,7 +1878,7 @@ describe("SAML SSO", async () => { trustedProviders: [], }, }, - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -1941,7 +1957,7 @@ describe("SAML SSO", async () => { trustedProviders: ["trusted-saml-provider"], }, }, - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }, ); @@ -2076,10 +2092,10 @@ describe("SAML SSO", async () => { expect(response.status).toBe(302); const redirectLocation = response.headers.get("location") || ""; - expect(redirectLocation).toContain("error=unsolicited_response"); + expect(redirectLocation).toContain("error=invalid_saml_response"); }); - it("should allow unsolicited SAML response when allowIdpInitiated is true (default)", async () => { + it("should allow unsolicited SAML response when allowIdpInitiated is explicitly true", async () => { const { auth, signInWithTestUser } = await getTestInstance({ plugins: [ sso({ @@ -2277,7 +2293,10 @@ describe("SAML SSO", async () => { expect(response.status).toBe(302); const redirectLocation = response.headers.get("location") || ""; - expect(redirectLocation).toContain("error=unsolicited_response"); + // The mock IdP response includes InResponseTo, but no matching + // AuthnRequest was stored — so it's rejected as unknown request ID, + // not as unsolicited (which would require InResponseTo to be absent). + expect(redirectLocation).toContain("error=invalid_saml_response"); }); it("should use verification table for InResponseTo validation", async () => { @@ -2343,10 +2362,148 @@ describe("SAML SSO", async () => { ), ); - // Should reject unsolicited response, proving validation is active + // The mock response has InResponseTo but no matching stored request expect(response.status).toBe(302); const redirectLocation = response.headers.get("location") || ""; - expect(redirectLocation).toContain("error=unsolicited_response"); + expect(redirectLocation).toContain("error=invalid_saml_response"); + }); + + /** + * @see https://github.com/better-auth/better-auth/issues/8607 + */ + it("should reject SAML response with mismatched audience restriction", async () => { + const { auth, signInWithTestUser } = await getTestInstance({ + plugins: [ + sso({ + saml: { + enableInResponseToValidation: false, + }, + }), + ], + }); + + const { headers } = await signInWithTestUser(); + + await auth.api.registerSSOProvider({ + body: { + providerId: "audience-mismatch-provider", + issuer: "http://localhost:8081", + domain: "http://localhost:8081", + samlConfig: { + entryPoint: "http://localhost:8081/api/sso/saml2/idp/post", + cert: certificate, + callbackUrl: "http://localhost:3000/dashboard", + audience: "https://wrong-audience.example.com", + wantAssertionsSigned: false, + signatureAlgorithm: "sha256", + digestAlgorithm: "sha256", + idpMetadata: { + metadata: idpMetadata, + }, + spMetadata: { + metadata: spMetadata, + binding: "post", + }, + }, + }, + headers, + }); + + let samlResponse: any; + await betterFetch("http://localhost:8081/api/sso/saml2/idp/post", { + onSuccess: async (context) => { + samlResponse = await context.data; + }, + }); + + const response = await auth.handler( + new Request( + "http://localhost:3000/api/auth/sso/saml2/callback/audience-mismatch-provider", + { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + body: new URLSearchParams({ + SAMLResponse: samlResponse.samlResponse, + }), + }, + ), + ); + + expect(response.status).toBe(302); + const redirectLocation = response.headers.get("location") || ""; + expect(redirectLocation).toContain( + "error=invalid_saml_response&error_description=Audience+mismatch", + ); + }); + + it("should accept SAML response when audience matches configured value", async () => { + const { auth, signInWithTestUser } = await getTestInstance({ + plugins: [ + sso({ + saml: { + enableInResponseToValidation: false, + }, + }), + ], + }); + + const { headers } = await signInWithTestUser(); + + // The mock IdP's SP entity ID is the audience it sends in assertions + // Configure the provider's audience to match it + await auth.api.registerSSOProvider({ + body: { + providerId: "audience-match-provider", + issuer: "http://localhost:8081", + domain: "http://localhost:8081", + samlConfig: { + entryPoint: "http://localhost:8081/api/sso/saml2/idp/post", + cert: certificate, + callbackUrl: "http://localhost:3000/dashboard", + audience: "http://localhost:3001/api/sso/saml2/sp/metadata", + wantAssertionsSigned: false, + signatureAlgorithm: "sha256", + digestAlgorithm: "sha256", + idpMetadata: { + metadata: idpMetadata, + }, + spMetadata: { + metadata: spMetadata, + binding: "post", + }, + }, + }, + headers, + }); + + let samlResponse: any; + await betterFetch("http://localhost:8081/api/sso/saml2/idp/post", { + onSuccess: async (context) => { + samlResponse = await context.data; + }, + }); + + const response = await auth.handler( + new Request( + "http://localhost:3000/api/auth/sso/saml2/callback/audience-match-provider", + { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + body: new URLSearchParams({ + SAMLResponse: samlResponse.samlResponse, + }), + }, + ), + ); + + // Should not redirect to an error — the audience matches + expect(response.status).toBe(302); + const redirectLocation = response.headers.get("location") || ""; + expect(redirectLocation).not.toContain("error="); }); /** @@ -2354,7 +2511,7 @@ describe("SAML SSO", async () => { */ it("should correctly parse verification-ID-based RelayState on ACS route (SP-initiated)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -2439,7 +2596,7 @@ describe("SAML SSO", async () => { it("should redirect to signIn callbackURL when relay_state cookie is missing on callback route (cross-site POST)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -2454,7 +2611,6 @@ describe("SAML SSO", async () => { cert: certificate, callbackUrl: "http://localhost:3000/api/auth/sso/saml2/callback/saml-provider", - audience: "http://localhost:3000", wantAssertionsSigned: false, signatureAlgorithm: "sha256", digestAlgorithm: "sha256", @@ -2518,7 +2674,7 @@ describe("SAML SSO", async () => { it("should redirect to signIn callbackURL when relay_state cookie is missing on ACS route (cross-site POST)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -2533,7 +2689,6 @@ describe("SAML SSO", async () => { cert: certificate, callbackUrl: "http://localhost:3000/api/auth/sso/saml2/sp/acs/saml-provider", - audience: "http://localhost:3000", wantAssertionsSigned: false, signatureAlgorithm: "sha256", digestAlgorithm: "sha256", @@ -2989,7 +3144,7 @@ describe("SSO Provider Config Parsing", () => { describe("SAML SSO - IdP Initiated Flow", () => { it("should handle IdP-initiated flow with GET after POST redirect", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3106,7 +3261,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should prevent redirect loop when callbackUrl points to callback route", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3178,7 +3333,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should handle GET request with RelayState in query", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3260,7 +3415,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should handle GET request when POST redirects to callback URL (original issue scenario)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3348,7 +3503,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should prevent open redirect with malicious RelayState URL", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3422,7 +3577,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should prevent open redirect via GET with malicious RelayState", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3508,7 +3663,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should allow relative path redirects", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3577,7 +3732,7 @@ describe("SAML SSO - IdP Initiated Flow", () => { it("should block protocol-relative URL attacks (//evil.com)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3883,7 +4038,7 @@ describe("SAML ACS Origin Check Bypass", () => { describe("Positive: SAML endpoints allow external IdP origins", () => { it("should allow SAML callback POST from external IdP origin", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3937,7 +4092,7 @@ describe("SAML ACS Origin Check Bypass", () => { it("should allow ACS endpoint POST from external IdP origin", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -3990,7 +4145,7 @@ describe("SAML ACS Origin Check Bypass", () => { describe("Negative: Non-SAML endpoints remain protected", () => { it("should block POST to sign-up with untrusted origin when origin check is enabled", async () => { const { auth } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], advanced: { disableCSRFCheck: false, disableOriginCheck: false, @@ -4214,7 +4369,7 @@ describe("SAML SSO - Size Limit Validation", () => { describe("SAML SSO - Assertion Replay Protection", () => { it("should reject replayed SAML assertion (same assertion submitted twice)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -4294,7 +4449,7 @@ describe("SAML SSO - Assertion Replay Protection", () => { it("should reject replayed SAML assertion on ACS endpoint", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -4374,7 +4529,7 @@ describe("SAML SSO - Assertion Replay Protection", () => { it("should reject cross-endpoint replay (callback → ACS)", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -4455,7 +4610,7 @@ describe("SAML SSO - Assertion Replay Protection", () => { describe("SAML SSO - Single Assertion Validation", () => { it("should reject SAML response with multiple assertions on callback endpoint", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -4529,7 +4684,7 @@ describe("SAML SSO - Single Assertion Validation", () => { it("should reject SAML response with multiple assertions on ACS endpoint", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -4742,7 +4897,7 @@ describe("SAML SSO - Single Assertion Validation", () => { it("should accept valid SAML response with exactly one assertion", async () => { const { auth, signInWithTestUser } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -4801,7 +4956,7 @@ describe("SAML SSO - Single Assertion Validation", () => { it("should normalize email to lowercase in SAML authentication to prevent duplicate creation", async () => { const { auth, client, signInWithTestUser, db } = await getTestInstance({ - plugins: [sso()], + plugins: [sso({ saml: { enableInResponseToValidation: false } })], }); const { headers } = await signInWithTestUser(); @@ -5544,6 +5699,7 @@ describe("SAML provisionUser should only be called for new users", async () => { plugins: [ sso({ provisionUser: provisionUserFn, + saml: { enableInResponseToValidation: false }, }), ], }); @@ -5659,6 +5815,7 @@ describe("SAML provisionUserOnEveryLogin should call provisionUser on every sign sso({ provisionUser: provisionUserFn, provisionUserOnEveryLogin: true, + saml: { enableInResponseToValidation: false }, }), ], }); diff --git a/packages/sso/src/saml/index.ts b/packages/sso/src/saml/index.ts index a42f2b6dda..353c8fc160 100644 --- a/packages/sso/src/saml/index.ts +++ b/packages/sso/src/saml/index.ts @@ -11,3 +11,5 @@ export { } from "./algorithms"; export { validateSingleAssertion } from "./assertions"; + +export { validateAudience, validateInResponseTo } from "./response-validation"; diff --git a/packages/sso/src/saml/response-validation.ts b/packages/sso/src/saml/response-validation.ts new file mode 100644 index 0000000000..9555b1debd --- /dev/null +++ b/packages/sso/src/saml/response-validation.ts @@ -0,0 +1,197 @@ +import type { GenericEndpointContext } from "@better-auth/core"; +import { AUTHN_REQUEST_KEY_PREFIX } from "../constants"; +import type { SAMLAssertionExtract } from "../types"; + +function errorRedirectUrl( + base: string, + error: string, + description: string, +): string { + try { + const url = new URL(base); + url.searchParams.set("error", error); + url.searchParams.set("error_description", description); + return url.toString(); + } catch { + // Relative URL — fall back to manual construction. + // Split off any fragment so query params stay before the hash. + const hashIdx = base.indexOf("#"); + const path = hashIdx >= 0 ? base.slice(0, hashIdx) : base; + const hash = hashIdx >= 0 ? base.slice(hashIdx + 1) : undefined; + const separator = path.includes("?") ? "&" : "?"; + const query = `error=${encodeURIComponent(error)}&error_description=${encodeURIComponent(description)}`; + return `${path}${separator}${query}${hash ? `#${hash}` : ""}`; + } +} + +interface AuthnRequestRecord { + id: string; + providerId: string; + createdAt: number; + expiresAt: number; +} + +export interface InResponseToValidationContext { + extract: SAMLAssertionExtract; + providerId: string; + options: { + enableInResponseToValidation?: boolean; + allowIdpInitiated?: boolean; + }; + redirectUrl: string; +} + +/** + * Validates the InResponseTo attribute of a SAML Response. + * + * This binds the IdP's Response to a specific SP-initiated AuthnRequest, + * preventing replay attacks, unsolicited response injection, and + * cross-provider assertion swaps. + * + * The InResponseTo value lives at `extract.response.inResponseTo` in + * samlify's parsed output (not at the top level). + */ +export async function validateInResponseTo( + c: GenericEndpointContext, + ctx: InResponseToValidationContext, +): Promise { + if (ctx.options.enableInResponseToValidation === false) { + return; + } + + const inResponseTo = ctx.extract.response?.inResponseTo; + const allowIdpInitiated = ctx.options.allowIdpInitiated ?? false; + + if (inResponseTo) { + let storedRequest: AuthnRequestRecord | null = null; + + const verification = await c.context.internalAdapter.findVerificationValue( + `${AUTHN_REQUEST_KEY_PREFIX}${inResponseTo}`, + ); + + if (verification) { + try { + storedRequest = JSON.parse(verification.value) as AuthnRequestRecord; + if (storedRequest && storedRequest.expiresAt < Date.now()) { + storedRequest = null; + } + } catch { + storedRequest = null; + } + } + + if (!storedRequest) { + c.context.logger.error( + "SAML InResponseTo validation failed: unknown or expired request ID", + { inResponseTo, providerId: ctx.providerId }, + ); + throw c.redirect( + errorRedirectUrl( + ctx.redirectUrl, + "invalid_saml_response", + "Unknown or expired request ID", + ), + ); + } + + if (storedRequest.providerId !== ctx.providerId) { + c.context.logger.error( + "SAML InResponseTo validation failed: provider mismatch", + { + inResponseTo, + expectedProvider: storedRequest.providerId, + actualProvider: ctx.providerId, + }, + ); + await c.context.internalAdapter.deleteVerificationByIdentifier( + `${AUTHN_REQUEST_KEY_PREFIX}${inResponseTo}`, + ); + throw c.redirect( + errorRedirectUrl( + ctx.redirectUrl, + "invalid_saml_response", + "Provider mismatch", + ), + ); + } + + // Single-use: delete the stored request after successful validation + await c.context.internalAdapter.deleteVerificationByIdentifier( + `${AUTHN_REQUEST_KEY_PREFIX}${inResponseTo}`, + ); + } else if (!allowIdpInitiated) { + c.context.logger.error( + "SAML IdP-initiated SSO rejected: InResponseTo missing and allowIdpInitiated is false", + { providerId: ctx.providerId }, + ); + throw c.redirect( + errorRedirectUrl( + ctx.redirectUrl, + "unsolicited_response", + "IdP-initiated SSO not allowed", + ), + ); + } +} + +export interface AudienceValidationContext { + extract: SAMLAssertionExtract; + expectedAudience: string | undefined; + providerId: string; + redirectUrl: string; +} + +/** + * Validates the AudienceRestriction of a SAML assertion. + * + * Per SAML 2.0 Core §2.5.1, an assertion's Audience element specifies + * the intended recipient SP. Without this check, an assertion issued + * for a different SP (e.g., another application sharing the same IdP) + * could be accepted. + */ +export function validateAudience( + c: GenericEndpointContext, + ctx: AudienceValidationContext, +): void { + if (!ctx.expectedAudience) { + return; + } + + const audience = ctx.extract.audience; + + if (!audience) { + c.context.logger.error( + "SAML assertion missing AudienceRestriction but audience is configured — rejecting", + { providerId: ctx.providerId }, + ); + throw c.redirect( + errorRedirectUrl( + ctx.redirectUrl, + "invalid_saml_response", + "Audience restriction missing", + ), + ); + } + + // samlify returns a string for a single Audience element, or an array + // when multiple values are present in the restriction. + const audiences = Array.isArray(audience) ? audience : [audience]; + + if (!audiences.includes(ctx.expectedAudience)) { + c.context.logger.error( + "SAML audience mismatch: assertion was issued for a different service provider", + { + expected: ctx.expectedAudience, + received: audiences, + providerId: ctx.providerId, + }, + ); + throw c.redirect( + errorRedirectUrl( + ctx.redirectUrl, + "invalid_saml_response", + "Audience mismatch", + ), + ); + } +} diff --git a/packages/sso/src/types.ts b/packages/sso/src/types.ts index b67f68e28a..09e36e9c12 100644 --- a/packages/sso/src/types.ts +++ b/packages/sso/src/types.ts @@ -95,15 +95,39 @@ export interface SAMLSessionRecord { sessionIndex?: string; } -/** Parsed SAML assertion extract from samlify */ +/** + * Parsed SAML login response extract from samlify. + * + * samlify's extractor nests multi-attribute XML elements as objects: + * - `response` (Response/@ID, @IssueInstant, @Destination, @InResponseTo) + * - `sessionIndex` (AuthnStatement/@AuthnInstant, @SessionNotOnOrAfter, @SessionIndex) + * - `conditions` (Conditions/@NotBefore, @NotOnOrAfter) + * + * Single-value elements remain as strings: `nameID`, `audience`. + */ export interface SAMLAssertionExtract { nameID?: string; - sessionIndex?: string; - inResponseTo?: string; + /** + * From `` — samlify extracts all 3 attributes as an object. + * To get the SessionIndex string, read `sessionIndex.sessionIndex`. + */ + sessionIndex?: { + authnInstant?: string; + sessionNotOnOrAfter?: string; + sessionIndex?: string; + }; conditions?: { notBefore?: string; notOnOrAfter?: string; }; + response?: { + id?: string; + issueInstant?: string; + destination?: string; + inResponseTo?: string; + }; + /** Single string or array when multiple `` elements are present. */ + audience?: string | string[]; } type BaseSSOProvider = { @@ -323,9 +347,13 @@ export interface SSOOptions { * When true, responses without InResponseTo are accepted. * When false, all responses must correlate to a stored AuthnRequest. * + * IdP-initiated SSO is a known attack vector — the SAML2Int + * interoperability profile recommends against it. Only enable + * this if your IdP requires it and you understand the risks. + * * Only applies when InResponseTo validation is enabled. * - * @default true + * @default false */ allowIdpInitiated?: boolean; /**