From 9247cb8528c91afcbfbbe324e3233b3d1f69ffa3 Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Sat, 30 May 2026 23:26:57 +0100 Subject: [PATCH] fix(sso): enforce SSRF validation, org-admin registration, SLO-by-token, and error encoding on refactored routes (#9574, #9220, #9818, #9722, #9702) Re-applies five main fixes onto next's rebuilt SSO routes. next's refactor kept the supporting helpers (the SSRF host check, the SAMLSessionRecord token field) but dropped the call sites, so the merge left them present yet unused. This wires them back. - #9574: registerSSOProvider and updateSSOProvider now run validateSkipDiscoveryEndpoints on user-supplied OIDC endpoints, rejecting private/link-local hosts (for example 169.254.169.254). The merge included the helper but never called it, leaving advisory GHSA-5rr4-8452-hf4v open on next. Validation runs even without skipDiscovery. - #9220: registering an SSO provider requires an organization owner/admin role when the organization plugin is active, blocking a low-privilege member from registering a malicious IdP. - #9818: SAML Single Logout revokes the session by token; the SAML session record now carries the session token. - #9722: the OIDC callback error redirect encodes the error value. - #9702: a before-callback hook rejection in the OIDC and SAML callbacks redirects to the per-flow errorURL with the hook's code, instead of surfacing a raw response. Two stale IDP-bounce assertions are updated to next's migrated state-error vocabulary (state_not_found). All SSO regression suites pass (SSRF, org-admin, SLO, hook-rejection, encoding). (cherry picked from commit c3b238f71a6f66914f12db5f615504e2cf855bfc) --- packages/sso/src/oidc.test.ts | 4 +- packages/sso/src/providers.test.ts | 3 - packages/sso/src/routes/providers.ts | 27 ++++-- packages/sso/src/routes/saml-pipeline.ts | 48 +++++++---- packages/sso/src/routes/sso.ts | 102 ++++++++++++++++------- 5 files changed, 126 insertions(+), 58 deletions(-) diff --git a/packages/sso/src/oidc.test.ts b/packages/sso/src/oidc.test.ts index 446a2f1a8f..0e4c1ba9d4 100644 --- a/packages/sso/src/oidc.test.ts +++ b/packages/sso/src/oidc.test.ts @@ -2279,7 +2279,7 @@ describe("SSO OIDC IDP-initiated bounce", async () => { expect(res.status).toBe(302); const location = res.headers.get("location") || ""; expect(location).not.toContain("http://localhost:8080/authorize"); - expect(location).toContain("please_restart_the_process"); + expect(location).toContain("state_not_found"); }); it("should carry ssoProviderId in the bounced state when options.redirectURI is configured", async () => { @@ -2329,6 +2329,6 @@ describe("SSO OIDC IDP-initiated bounce", async () => { ); expect(res.status).toBe(302); - expect(res.headers.get("location")).toContain("please_restart_the_process"); + expect(res.headers.get("location")).toContain("state_not_found"); }); }); diff --git a/packages/sso/src/providers.test.ts b/packages/sso/src/providers.test.ts index b43c309c51..51e12393ca 100644 --- a/packages/sso/src/providers.test.ts +++ b/packages/sso/src/providers.test.ts @@ -1467,7 +1467,6 @@ kBGIJYs= samlConfig: { entryPoint: "https://idp.example.com/sso", cert: TEST_CERT, - callbackUrl: "http://localhost:3000/api/sso/callback", audience: "my-audience", wantAssertionsSigned: true, spMetadata: {}, @@ -1516,7 +1515,6 @@ kBGIJYs= samlConfig: { entryPoint: "https://idp.example.com/sso", cert: TEST_CERT, - callbackUrl: "http://localhost:3000/api/sso/callback", audience: "my-audience", wantAssertionsSigned: true, spMetadata: {}, @@ -1558,7 +1556,6 @@ kBGIJYs= samlConfig: { entryPoint: "https://idp.example.com/sso", cert: TEST_CERT, - callbackUrl: "http://localhost:3000/api/sso/callback", audience: "my-audience", wantAssertionsSigned: true, spMetadata: {}, diff --git a/packages/sso/src/routes/providers.ts b/packages/sso/src/routes/providers.ts index feb9931115..15b515ddc6 100644 --- a/packages/sso/src/routes/providers.ts +++ b/packages/sso/src/routes/providers.ts @@ -6,6 +6,11 @@ import { } from "better-auth/api"; import * as z from "zod"; import { DEFAULT_MAX_SAML_METADATA_SIZE } from "../constants"; +import { + DiscoveryError, + mapDiscoveryErrorToAPIError, + validateSkipDiscoveryEndpoints, +} from "../oidc"; import { resolveSigningCerts, validateCertSources, @@ -29,6 +34,10 @@ interface SSOProviderRecord { const ADMIN_ROLES = ["owner", "admin"]; +export function hasOrgAdminRole(member: Pick): boolean { + return member.role.split(",").some((r) => ADMIN_ROLES.includes(r.trim())); +} + type ParsedCert = ReturnType; type SanitizedCert = ParsedCert | { error: string }; @@ -67,9 +76,7 @@ async function isOrgAdmin( { field: "organizationId", value: organizationId }, ], }); - if (!member) return false; - const roles = member.role.split(","); - return roles.some((r) => ADMIN_ROLES.includes(r.trim())); + return member ? hasOrgAdminRole(member) : false; } async function batchCheckOrgAdmin( @@ -93,8 +100,7 @@ async function batchCheckOrgAdmin( const adminOrgIds = new Set(); for (const member of members) { - const roles = member.role.split(","); - if (roles.some((r: string) => ADMIN_ROLES.includes(r.trim()))) { + if (hasOrgAdminRole(member)) { adminOrgIds.add(member.organizationId); } } @@ -504,6 +510,17 @@ export const updateSSOProvider = (options: SSOOptions) => { } if (body.oidcConfig) { + try { + validateSkipDiscoveryEndpoints(body.oidcConfig, (url) => + ctx.context.isTrustedOrigin(url), + ); + } catch (error) { + if (error instanceof DiscoveryError) { + throw mapDiscoveryErrorToAPIError(error); + } + throw error; + } + const currentOidcConfig = parseAndValidateConfig( existingProvider.oidcConfig, "OIDC", diff --git a/packages/sso/src/routes/saml-pipeline.ts b/packages/sso/src/routes/saml-pipeline.ts index 62afac8320..1c9d6d83c0 100644 --- a/packages/sso/src/routes/saml-pipeline.ts +++ b/packages/sso/src/routes/saml-pipeline.ts @@ -1,3 +1,4 @@ +import { isAPIError } from "@better-auth/core/utils/is-api-error"; import type { User } from "better-auth"; import { APIError } from "better-auth/api"; import { setSessionCookie } from "better-auth/cookies"; @@ -380,24 +381,36 @@ export async function processSAMLResponse( validateEmailDomain(userInfo.email as string, provider.domain)); const postAuthRedirect = relayState?.callbackURL || ctx.context.baseURL; + const errorUrl = relayState?.errorURL || samlRedirectUrl; - const result = await handleOAuthUserInfo(ctx, { - userInfo: { - email: userInfo.email as string, - name: (userInfo.name || userInfo.email) as string, - id: userInfo.id as string, - emailVerified: Boolean(userInfo.emailVerified), - }, - account: { - providerId, - accountId: userInfo.id as string, - accessToken: "", - refreshToken: "", - }, - callbackURL: postAuthRedirect, - disableSignUp: options?.disableImplicitSignUp, - isTrustedProvider, - }); + let result: Awaited>; + try { + result = await handleOAuthUserInfo(ctx, { + userInfo: { + email: userInfo.email as string, + name: (userInfo.name || userInfo.email) as string, + id: userInfo.id as string, + emailVerified: Boolean(userInfo.emailVerified), + }, + account: { + providerId, + accountId: userInfo.id as string, + accessToken: "", + refreshToken: "", + }, + callbackURL: postAuthRedirect, + disableSignUp: options?.disableImplicitSignUp, + isTrustedProvider, + }); + } catch (e) { + if (isAPIError(e) && e.body?.code) { + const params = new URLSearchParams({ error: e.body.code }); + if (e.body.message) params.set("error_description", e.body.message); + const sep = errorUrl.includes("?") ? "&" : "?"; + throw ctx.redirect(`${errorUrl}${sep}${params.toString()}`); + } + throw e; + } if (result.error) { throw ctx.redirect( @@ -442,6 +455,7 @@ export async function processSAMLResponse( const samlSessionKey = `${constants.SAML_SESSION_KEY_PREFIX}${providerId}:${extract.nameID}`; const samlSessionData: SAMLSessionRecord = { sessionId: session.id, + sessionToken: session.token, providerId, nameID: extract.nameID, sessionIndex: (extract as SAMLAssertionExtract).sessionIndex diff --git a/packages/sso/src/routes/sso.ts b/packages/sso/src/routes/sso.ts index f221830fe3..a062cb35be 100644 --- a/packages/sso/src/routes/sso.ts +++ b/packages/sso/src/routes/sso.ts @@ -1,3 +1,4 @@ +import { isAPIError } from "@better-auth/core/utils/is-api-error"; import { BetterFetchError, betterFetch } from "@better-fetch/fetch"; import type { PrivateKeyJwtSigningAlgorithm, @@ -36,12 +37,14 @@ import { discoverOIDCConfig, ensureRuntimeDiscovery, mapDiscoveryErrorToAPIError, + validateSkipDiscoveryEndpoints, } from "../oidc"; import { validateCertSources, validateConfigAlgorithms } from "../saml"; import { SAML_ERROR_CODES } from "../saml/error-codes"; import { generateRelayState } from "../saml-state"; import type { AuthnRequestRecord, + Member, OIDCConfig, SAMLConfig, SAMLSessionRecord, @@ -56,6 +59,7 @@ import { createSP, findSAMLProvider, } from "./helpers"; +import { hasOrgAdminRole } from "./providers"; import { getSafeRedirectUrl, processSAMLResponse } from "./saml-pipeline"; import { registerSSOProviderBodySchema } from "./schemas"; @@ -386,7 +390,7 @@ export const registerSSOProvider = (options: O) => { } if (ctx.body.organizationId) { - const organization = await ctx.context.adapter.findOne({ + const member = await ctx.context.adapter.findOne({ model: "member", where: [ { @@ -399,11 +403,17 @@ export const registerSSOProvider = (options: O) => { }, ], }); - if (!organization) { + if (!member) { throw new APIError("BAD_REQUEST", { message: "You are not a member of the organization", }); } + if (ctx.context.hasPlugin("organization") && !hasOrgAdminRole(member)) { + throw new APIError("FORBIDDEN", { + message: + "You must be an organization owner or admin to register SSO providers", + }); + } } const existingProvider = await ctx.context.adapter.findOne({ @@ -425,6 +435,19 @@ export const registerSSOProvider = (options: O) => { }); } + if (body.oidcConfig) { + try { + validateSkipDiscoveryEndpoints(body.oidcConfig, (url) => + ctx.context.isTrustedOrigin(url), + ); + } catch (error) { + if (error instanceof DiscoveryError) { + throw mapDiscoveryErrorToAPIError(error); + } + throw error; + } + } + let hydratedOIDCConfig: HydratedOIDCConfig | null = null; if (body.oidcConfig && !body.oidcConfig.skipDiscovery) { try { @@ -1442,33 +1465,48 @@ async function handleOIDCCallback( (provider as { domainVerified?: boolean }).domainVerified === true && validateEmailDomain(userInfo.email, provider.domain); - const linked = await handleOAuthUserInfo(ctx, { - userInfo: { - email: userInfo.email, - name: userInfo.name || "", - id: userInfo.id, - image: userInfo.image, - emailVerified: options?.trustEmailVerified - ? userInfo.emailVerified || false - : false, - }, - account: { - idToken: tokenResponse.idToken, - accessToken: tokenResponse.accessToken, - refreshToken: tokenResponse.refreshToken, - accountId: userInfo.id, - providerId: provider.providerId, - accessTokenExpiresAt: tokenResponse.accessTokenExpiresAt, - refreshTokenExpiresAt: tokenResponse.refreshTokenExpiresAt, - scope: tokenResponse.scopes?.join(","), - }, - callbackURL, - disableSignUp: options?.disableImplicitSignUp && !requestSignUp, - overrideUserInfo: config.overrideUserInfo, - isTrustedProvider, - }); + let linked: Awaited>; + try { + linked = await handleOAuthUserInfo(ctx, { + userInfo: { + email: userInfo.email, + name: userInfo.name || "", + id: userInfo.id, + image: userInfo.image, + emailVerified: options?.trustEmailVerified + ? userInfo.emailVerified || false + : false, + }, + account: { + idToken: tokenResponse.idToken, + accessToken: tokenResponse.accessToken, + refreshToken: tokenResponse.refreshToken, + accountId: userInfo.id, + providerId: provider.providerId, + accessTokenExpiresAt: tokenResponse.accessTokenExpiresAt, + refreshTokenExpiresAt: tokenResponse.refreshTokenExpiresAt, + scope: tokenResponse.scopes?.join(","), + }, + callbackURL, + disableSignUp: options?.disableImplicitSignUp && !requestSignUp, + overrideUserInfo: config.overrideUserInfo, + isTrustedProvider, + }); + } catch (e) { + if (isAPIError(e) && e.body?.code) { + const baseURL = errorURL || callbackURL; + const params = new URLSearchParams({ error: e.body.code }); + if (e.body.message) params.set("error_description", e.body.message); + const sep = baseURL.includes("?") ? "&" : "?"; + throw ctx.redirect(`${baseURL}${sep}${params.toString()}`); + } + throw e; + } if (linked.error) { - throw ctx.redirect(`${errorURL || callbackURL}?error=${linked.error}`); + const baseURL = errorURL || callbackURL; + const params = new URLSearchParams({ error: linked.error }); + const sep = baseURL.includes("?") ? "&" : "?"; + throw ctx.redirect(`${baseURL}${sep}${params.toString()}`); } const { session, user } = linked.data!; @@ -1997,7 +2035,7 @@ async function handleLogoutRequest( sessionIndex === data.sessionIndex ) { await ctx.context.internalAdapter - .deleteSession(data.sessionId) + .deleteSession(data.sessionToken) .catch((e: unknown) => ctx.context.logger.warn("Failed to delete session during SLO", { error: e, @@ -2036,7 +2074,9 @@ async function handleLogoutRequest( const currentSession = await getSessionFromCtx(ctx); if (currentSession?.session) { - await ctx.context.internalAdapter.deleteSession(currentSession.session.id); + await ctx.context.internalAdapter.deleteSession( + currentSession.session.token, + ); } deleteSessionCookie(ctx); @@ -2179,7 +2219,7 @@ export const initiateSLO = (options?: SSOOptions) => { ), ); - await ctx.context.internalAdapter.deleteSession(session.session.id); + await ctx.context.internalAdapter.deleteSession(session.session.token); deleteSessionCookie(ctx);