github-starred/better-auth
2
0
Fork 0
mirror of https://github.com/better-auth/better-auth.git synced 2026-05-30 10:56:31 -05:00
Code Issues 2.5k Packages Projects Releases 112 Wiki Activity

[PR #9055] [MERGED] fix(sso)!: harden SAML response validation (InResponseTo, Audience, SessionIndex) #25305

New Issue
Closed
opened 2026-04-15 22:49:20 -05:00 by GiteaMirror · 0 comments
GiteaMirror commented 2026-04-15 22:49:20 -05:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/better-auth/better-auth/pull/9055
Author: @gustavovalverde
Created: 4/9/2026
Status: ✅ Merged
Merged: 4/9/2026
Merged by: @gustavovalverde

Base: next ← Head: fix/saml-response-validation

📝 Commits (6)

  • 8988627 fix(sso)!: harden SAML response validation
  • d00ae32 chore(sso): address review feedback
  • fd2d4dd fix(sso): use URL API for error redirects
  • 681a68d fix(sso): handle relative callback URLs in error redirects
  • 594addb fix(sso): preserve fragment in relative redirect URLs
  • 8b4f624 fix(sso): resolve TS18048 for path possibly undefined

📊 Changes

6 files changed (+491 additions, -199 deletions)

View changed files

➕ .changeset/saml-response-validation-hardening.md (+20 -0)
📝 packages/sso/src/routes/sso.ts (+50 -162)
📝 packages/sso/src/saml.test.ts (+190 -33)
📝 packages/sso/src/saml/index.ts (+2 -0)
➕ packages/sso/src/saml/response-validation.ts (+197 -0)
📝 packages/sso/src/types.ts (+32 -4)

📄 Description

Summary

Fixes three security/correctness bugs in SAML response handling and extracts shared validation logic to eliminate code duplication between the two ACS handlers.

  • InResponseTo validation was dead code:extract.inResponseTo is always undefined because samlify nests it under extract.response.inResponseTo. Every SP-initiated flow was silently treated as IdP-initiated.
  • Audience Restriction never validated: Assertions issued for a different SP were accepted. Now validated against samlConfig.audience per SAML 2.0 Core §2.5.1. Handles both single and multi-valued <Audience> elements.
  • SessionIndex stored as object, not string: samlify's login response extractor returns sessionIndex as { authnInstant, sessionNotOnOrAfter, sessionIndex }, but the code stored the whole object. SLO session-index comparisons always failed silently.

Breaking Changes

  • allowIdpInitiated now defaults to false: IdP-initiated SSO is disabled by default, aligning with the SAML2Int interoperability profile. Set saml: { allowIdpInitiated: true } to restore previous behavior.

Fixes #8607
Closes #8608
Closes #8647

Credit

This PR incorporates work from:

  • @evanenochs (#8608) — InResponseTo path fix + SP-initiated regression test
  • @jonathansamines (#8608 review) — identified unnecessary test modifications
  • @Copilot (#8608 review) — identified missing SP-initiated test coverage

Summary by cubic

Hardens SAML response validation in @better-auth/sso by fixing InResponseTo and Audience checks, storing the correct SessionIndex for SLO, and centralizing validation into shared helpers. Error redirects now use the URL API with a safe fallback that preserves existing query strings and fragments.

  • Bug Fixes

    • Fixed InResponseTo lookup to read extract.response.inResponseTo, so SP‑initiated flows validate correctly on both handlers.
    • Added Audience Restriction validation against samlConfig.audience (supports single and multiple <Audience> values).
    • Store the inner sessionIndex string (and handle LogoutRequest’s plain string) so SLO comparisons work as intended.
    • Build error redirect URLs with the URL API and a relative-URL fallback that preserves fragments, preventing malformed redirects when the callback URL already has query params.
    • Resolved TS18048 (“path possibly undefined”) in validation helpers.

  • Migration

    • IdP‑initiated SSO now defaults to off. To restore previous behavior, set saml: { allowIdpInitiated: true }.

Written for commit 8b4f624a7d. Summary will update on new commits.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/better-auth/better-auth/pull/9055 **Author:** [@gustavovalverde](https://github.com/gustavovalverde) **Created:** 4/9/2026 **Status:** ✅ Merged **Merged:** 4/9/2026 **Merged by:** [@gustavovalverde](https://github.com/gustavovalverde) **Base:** `next` ← **Head:** `fix/saml-response-validation` --- ### 📝 Commits (6) - [`8988627`](https://github.com/better-auth/better-auth/commit/89886270b595880b72c71b381a6ac36ff79d80ec) fix(sso)!: harden SAML response validation - [`d00ae32`](https://github.com/better-auth/better-auth/commit/d00ae322a8e1408d7154bbbe83628b53a82f2c7f) chore(sso): address review feedback - [`fd2d4dd`](https://github.com/better-auth/better-auth/commit/fd2d4dd63846defb59f84e008269e7de1c694c1d) fix(sso): use URL API for error redirects - [`681a68d`](https://github.com/better-auth/better-auth/commit/681a68dd98789c108d0010e6f0209c671948d1e3) fix(sso): handle relative callback URLs in error redirects - [`594addb`](https://github.com/better-auth/better-auth/commit/594addb47bb0961747cd3b847fa8d8c78f4b6ebe) fix(sso): preserve fragment in relative redirect URLs - [`8b4f624`](https://github.com/better-auth/better-auth/commit/8b4f624a7de48569fc7b9016dc9cd6853314ff50) fix(sso): resolve TS18048 for path possibly undefined ### 📊 Changes **6 files changed** (+491 additions, -199 deletions) <details> <summary>View changed files</summary> ➕ `.changeset/saml-response-validation-hardening.md` (+20 -0) 📝 `packages/sso/src/routes/sso.ts` (+50 -162) 📝 `packages/sso/src/saml.test.ts` (+190 -33) 📝 `packages/sso/src/saml/index.ts` (+2 -0) ➕ `packages/sso/src/saml/response-validation.ts` (+197 -0) 📝 `packages/sso/src/types.ts` (+32 -4) </details> ### 📄 Description ## Summary Fixes three security/correctness bugs in SAML response handling and extracts shared validation logic to eliminate code duplication between the two ACS handlers. - **InResponseTo validation was dead code**:`extract.inResponseTo` is always `undefined` because samlify nests it under `extract.response.inResponseTo`. Every SP-initiated flow was silently treated as IdP-initiated. - **Audience Restriction never validated**: Assertions issued for a different SP were accepted. Now validated against `samlConfig.audience` per SAML 2.0 Core §2.5.1. Handles both single and multi-valued `<Audience>` elements. - **SessionIndex stored as object, not string**: samlify's login response extractor returns `sessionIndex` as `{ authnInstant, sessionNotOnOrAfter, sessionIndex }`, but the code stored the whole object. SLO session-index comparisons always failed silently. ### Breaking Changes - **`allowIdpInitiated` now defaults to `false`**: IdP-initiated SSO is disabled by default, aligning with the SAML2Int interoperability profile. Set `saml: { allowIdpInitiated: true }` to restore previous behavior. Fixes #8607 Closes #8608 Closes #8647 ### Credit This PR incorporates work from: - @evanenochs (#8608) — InResponseTo path fix + SP-initiated regression test - @jonathansamines (#8608 review) — identified unnecessary test modifications - @Copilot (#8608 review) — identified missing SP-initiated test coverage <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Hardens SAML response validation in `@better-auth/sso` by fixing InResponseTo and Audience checks, storing the correct SessionIndex for SLO, and centralizing validation into shared helpers. Error redirects now use the URL API with a safe fallback that preserves existing query strings and fragments. - **Bug Fixes** - Fixed InResponseTo lookup to read `extract.response.inResponseTo`, so SP‑initiated flows validate correctly on both handlers. - Added Audience Restriction validation against `samlConfig.audience` (supports single and multiple `<Audience>` values). - Store the inner `sessionIndex` string (and handle LogoutRequest’s plain string) so SLO comparisons work as intended. - Build error redirect URLs with the URL API and a relative-URL fallback that preserves fragments, preventing malformed redirects when the callback URL already has query params. - Resolved TS18048 (“path possibly undefined”) in validation helpers. - **Migration** - IdP‑initiated SSO now defaults to off. To restore previous behavior, set `saml: { allowIdpInitiated: true }`. <sup>Written for commit 8b4f624a7de48569fc7b9016dc9cd6853314ff50. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
GiteaMirror added the pull-request label 2026-04-15 22:49:20 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
adapter
astro
awaiting external contributor
blocked
breaking
breaking change
bug
c-devops
core
credentials
database
dependencies
devops
devtools
docs
documentation
duplicate
elysia
enhancement
enterprise
expo
express
fastify
good first issue
help wanted
hono
identity
infra
integration
invalid
javascript
locked
maintenance
need-more-information
needs: info
needs: repro
nextjs
nuxt
oauth
organization
P0
payments
perf
platform
plugin
pull-request
Mirrored from GitHub Pull Request
 question
ready
regression
remix
security
social-provider
solid
stale
svelte
tanstack-start
tracking
version-bump
vue
wontfix
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/better-auth#25305
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?