mirror of
https://github.com/better-auth/better-auth.git
synced 2026-07-27 10:55:09 -05:00
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)
This commit is contained in:
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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: {},
|
||||
|
||||
@@ -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<Member, "role">): boolean {
|
||||
return member.role.split(",").some((r) => ADMIN_ROLES.includes(r.trim()));
|
||||
}
|
||||
|
||||
type ParsedCert = ReturnType<typeof parseCertificate>;
|
||||
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<string>();
|
||||
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<OIDCConfig>(
|
||||
existingProvider.oidcConfig,
|
||||
"OIDC",
|
||||
|
||||
@@ -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<ReturnType<typeof handleOAuthUserInfo>>;
|
||||
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
|
||||
|
||||
@@ -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 = <O extends SSOOptions>(options: O) => {
|
||||
}
|
||||
|
||||
if (ctx.body.organizationId) {
|
||||
const organization = await ctx.context.adapter.findOne({
|
||||
const member = await ctx.context.adapter.findOne<Member>({
|
||||
model: "member",
|
||||
where: [
|
||||
{
|
||||
@@ -399,11 +403,17 @@ export const registerSSOProvider = <O extends SSOOptions>(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 = <O extends SSOOptions>(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<ReturnType<typeof handleOAuthUserInfo>>;
|
||||
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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user