fix: address sync review feedback

This commit is contained in:
Gustavo Valverde
2026-06-26 07:42:55 +01:00
parent 4bcda56649
commit cc1aed88d3
24 changed files with 654 additions and 295 deletions
+2 -2
View File
@@ -1,6 +1,6 @@
import type { BetterAuthPlugin, HookEndpointContext } from "@better-auth/core";
import { createAuthMiddleware } from "@better-auth/core/api";
import { getIp } from "@better-auth/core/utils/ip";
import { getIP } from "@better-auth/core/utils/ip";
import { base64Url } from "@better-auth/utils/base64";
import { createHash } from "@better-auth/utils/hash";
import { BetterAuthError } from "better-auth";
@@ -246,7 +246,7 @@ export function apiKey(
userId: apiKey.referenceId,
userAgent: ctx.request?.headers.get("user-agent") ?? null,
ipAddress: ctx.request
? getIp(ctx.request, ctx.context.options)
? getIP(ctx.request, ctx.context.options)
: null,
createdAt: new Date(),
updatedAt: new Date(),
+1 -1
View File
@@ -410,7 +410,7 @@ export {
optionsMiddleware,
} from "@better-auth/core/api";
export { APIError } from "@better-auth/core/error";
export { getIp } from "@better-auth/core/utils/ip";
export { getIP } from "@better-auth/core/utils/ip";
export { isAPIError } from "../utils/is-api-error";
export { type DispatchContext, dispatchAuthEndpoint } from "./dispatch";
export * from "./middlewares";
@@ -3,7 +3,7 @@ import type {
BetterAuthRateLimitStorage,
} from "@better-auth/core";
import { BetterAuthError } from "@better-auth/core/error";
import { createRateLimitKey, getIp } from "@better-auth/core/utils/ip";
import { createRateLimitKey, getIP } from "@better-auth/core/utils/ip";
import { normalizePathname } from "@better-auth/core/utils/url";
import type { RateLimit } from "../../types";
import { wildcardMatch } from "../../utils/wildcard";
@@ -338,7 +338,7 @@ async function resolveRateLimitConfig(req: Request, ctx: AuthContext) {
const path = normalizePathname(req.url, basePath);
let currentWindow = ctx.rateLimit.window;
let currentMax = ctx.rateLimit.max;
const ip = getIp(req, ctx.options);
const ip = getIP(req, ctx.options);
if (!ip && ctx.options.advanced?.ipAddress?.disableIpTracking) {
// IP tracking is explicitly disabled; per-IP rate limiting does not apply.
return null;
@@ -636,17 +636,7 @@ describe("forwarded IP chains", () => {
window: 10,
max: 20,
},
secondaryStorage: {
set(key, value) {
store.set(key, value);
},
get(key) {
return store.get(key) || null;
},
delete(key) {
store.delete(key);
},
},
secondaryStorage: createRateLimitSecondaryStorage(store),
});
for (let i = 0; i < 4; i++) {
@@ -690,17 +680,7 @@ describe("forwarded IP chains", () => {
trustedProxies: ["10.0.0.0/8"],
},
},
secondaryStorage: {
set(key, value) {
store.set(key, value);
},
get(key) {
return store.get(key) || null;
},
delete(key) {
store.delete(key);
},
},
secondaryStorage: createRateLimitSecondaryStorage(store),
});
// `<rotating spoofed>, <real client>, <trusted proxy>`: spoofed rotation
@@ -15,7 +15,7 @@ import type { DBAdapter, Where } from "@better-auth/core/db/adapter";
import type { InternalLogger } from "@better-auth/core/env";
import { APIError, BetterAuthError } from "@better-auth/core/error";
import { generateId } from "@better-auth/core/utils/id";
import { getIp } from "@better-auth/core/utils/ip";
import { getIP } from "@better-auth/core/utils/ip";
import { safeJSONParse } from "@better-auth/core/utils/json";
import { base64Url } from "@better-auth/utils/base64";
import { createHash } from "@better-auth/utils/hash";
@@ -415,7 +415,7 @@ export const createInternalAdapter = (
const defaultAdditionalFields = getSessionDefaultFields(options);
const data = {
...(sessionId ? { id: sessionId } : {}),
ipAddress: headers ? getIp(headers, options) || "" : "",
ipAddress: headers ? getIP(headers, options) || "" : "",
userAgent: headers?.get("user-agent") || "",
...rest,
/**
@@ -1,5 +1,5 @@
import type { BetterAuthPlugin } from "@better-auth/core";
import { getIp } from "@better-auth/core/utils/ip";
import { getIP } from "@better-auth/core/utils/ip";
import { middlewareResponse } from "../../utils/middleware-response";
import { wildcardMatch } from "../../utils/wildcard";
import { PACKAGE_VERSION } from "../../version";
@@ -63,7 +63,7 @@ export const captcha = (options: CaptchaOptions) =>
}
const captchaResponse = request.headers.get("x-captcha-response");
const remoteUserIP = getIp(request, ctx.options) ?? undefined;
const remoteUserIP = getIP(request, ctx.options) ?? undefined;
if (!captchaResponse) {
return middlewareResponse({
+32 -11
View File
@@ -289,33 +289,54 @@ export const siwe = (options: SIWEPluginOptions) => {
const domain =
options.emailDomainName ?? getOrigin(ctx.context.baseURL);
const normalizedEmail = email?.toLowerCase();
const walletEmail = `${walletAddress}@${domain}`;
// SIWE proves wallet control, not email ownership: bind the caller
// email only when unclaimed, else keep the wallet-derived address.
// email only when unclaimed and atomically reserved, else keep
// the wallet-derived address.
// Silent fallback (no distinct error) avoids an enumeration oracle.
// FIXME(siwe-contact-ownership): non-breaking floor; the durable fix
// drops the `email` body field and attaches a verified email via a
// separate authenticated link flow. Land on `next` after main->next sync.
let userEmail = `${walletAddress}@${domain}`;
let userEmail = walletEmail;
let emailClaimIdentifier: string | undefined;
if (!isAnon && normalizedEmail) {
const existingUser =
await ctx.context.internalAdapter.findUserByEmail(
normalizedEmail,
);
if (!existingUser) {
userEmail = normalizedEmail;
const identifier = `siwe-email-claim-${normalizedEmail}`;
const reserved =
await ctx.context.internalAdapter.reserveVerificationValue({
identifier,
value: walletAddress,
expiresAt: new Date(Date.now() + 60_000),
});
if (reserved) {
userEmail = normalizedEmail;
emailClaimIdentifier = identifier;
}
}
}
const { name, avatar } =
(await options.ensLookup?.({ walletAddress })) ?? {};
user = await ctx.context.internalAdapter.createUser(
{
name: name ?? walletAddress,
email: userEmail,
image: avatar ?? "",
},
{ method: "siwe" },
);
try {
user = await ctx.context.internalAdapter.createUser(
{
name: name ?? walletAddress,
email: userEmail,
image: avatar ?? "",
},
{ method: "siwe" },
);
} finally {
if (emailClaimIdentifier) {
await ctx.context.internalAdapter
.consumeVerificationValue(emailClaimIdentifier)
.catch(() => {});
}
}
// Create wallet address record
await ctx.context.adapter.create({
@@ -155,9 +155,17 @@ export async function verifyTwoFactor(ctx: GenericEndpointContext) {
if (attempts >= allowedAttempts) {
// Budget spent: cancel the whole challenge so every factor must
// start a new sign-in, and clear the now-dead cookie.
await ctx.context.internalAdapter
.consumeVerificationValue(signedTwoFactorCookie)
.catch(() => {});
try {
await ctx.context.internalAdapter.consumeVerificationValue(
signedTwoFactorCookie,
);
} catch {
expireCookie(ctx, twoFactorCookie);
throw APIError.from("INTERNAL_SERVER_ERROR", {
message: "Failed to invalidate two-factor challenge",
code: "FAILED_TO_INVALIDATE_TWO_FACTOR_CHALLENGE",
});
}
expireCookie(ctx, twoFactorCookie);
throw APIError.from(
"BAD_REQUEST",
+14 -2
View File
@@ -275,11 +275,16 @@ export const generateDrizzleSchema: SchemaGenerator = async ({
type += `.$onUpdate(${attr.onUpdate})`;
}
}
const referencesDisabledModel =
attr.references &&
(tables[getModelName(attr.references.model)]
?.disableMigrations ||
tables[attr.references.model]?.disableMigrations);
return `${fieldName}: ${type}${attr.required !== false ? ".notNull()" : ""}${
attr.unique ? ".unique()" : ""
}${
attr.references
attr.references && !referencesDisabledModel
? `.references(()=> ${getModelName(
attr.references.model,
)}.${getFieldName({ model: attr.references.model, field: attr.references.field })}, { onDelete: '${
@@ -337,6 +342,12 @@ export const generateDrizzleSchema: SchemaGenerator = async ({
for (const [fieldName, field] of foreignFields) {
const referencedModel = field.references!.model;
if (
tables[getModelName(referencedModel)]?.disableMigrations ||
tables[referencedModel]?.disableMigrations
) {
continue;
}
const relationKey = getModelName(referencedModel);
const fieldRef = `${getModelName(tableKey)}.${getFieldName({ model: tableKey, field: fieldName })}`;
const referenceRef = `${getModelName(referencedModel)}.${getFieldName({ model: referencedModel, field: field.references!.field || "id" })}`;
@@ -356,7 +367,8 @@ export const generateDrizzleSchema: SchemaGenerator = async ({
// 2. Find all OTHER tables that reference THIS table (creates "many" relations)
const otherModels = Object.entries(tables).filter(
([modelName]) => modelName !== tableKey,
([modelName, otherTable]) =>
modelName !== tableKey && !otherTable.disableMigrations,
);
// Map to track relations by model name to determine if unique or many
+15 -3
View File
@@ -72,11 +72,17 @@ export const generatePrismaSchema: SchemaGenerator = async ({
const manyToManyRelations = new Map();
for (const table in tables) {
if (tables[table]?.disableMigrations) {
continue;
}
const fields = tables[table]?.fields;
for (const field in fields) {
const attr = fields[field]!;
if (attr.references) {
const referencedOriginalModel = attr.references.model;
if (tables[referencedOriginalModel]?.disableMigrations) {
continue;
}
const referencedCustomModel =
tables[referencedOriginalModel]?.modelName || referencedOriginalModel;
const referencedModelNameCap = capitalizeFirstLetter(
@@ -374,6 +380,15 @@ export const generatePrismaSchema: SchemaGenerator = async ({
}
if (attr.references) {
const referencedOriginalModelName = getModelName(
attr.references.model,
);
if (
tables[referencedOriginalModelName]?.disableMigrations ||
tables[attr.references.model]?.disableMigrations
) {
continue;
}
if (
useUUIDs &&
provider === "postgresql" &&
@@ -382,9 +397,6 @@ export const generatePrismaSchema: SchemaGenerator = async ({
builder.model(modelName).field(fieldName).attribute(`db.Uuid`);
}
const referencedOriginalModelName = getModelName(
attr.references.model,
);
const referencedCustomModelName =
tables[referencedOriginalModelName]?.modelName ||
referencedOriginalModelName;
+14 -1
View File
@@ -1776,6 +1776,14 @@ describe("--dialect flag support", () => {
emittedTable: {
fields: {
name: { type: "string", required: true },
skippedTableId: {
type: "string",
required: false,
references: {
model: "skippedTable",
field: "id",
},
},
},
},
skippedTable: {
@@ -1804,7 +1812,10 @@ describe("--dialect flag support", () => {
});
expect(schema.code).toContain("emittedTable");
expect(schema.code).not.toContain("skippedTable");
expect(schema.code).toContain("skippedTableId");
expect(schema.code).not.toContain("export const skippedTable");
expect(schema.code).not.toContain("references(() => skippedTable");
expect(schema.code).not.toContain("skippedTable: one(skippedTable");
});
it("should not emit prisma models with disableMigration", async () => {
@@ -1821,6 +1832,8 @@ describe("--dialect flag support", () => {
});
expect(schema.code).toContain("EmittedTable");
expect(schema.code).toContain("skippedTableId");
expect(schema.code).not.toContain("SkippedTable");
expect(schema.code).not.toContain("skippedtable");
});
});
@@ -1,4 +1,4 @@
import { exportJWK, generateKeyPair, SignJWT } from "jose";
import { exportJWK, generateKeyPair, generateSecret, SignJWT } from "jose";
import { beforeEach, describe, expect, it, vi } from "vitest";
vi.mock("@better-fetch/fetch", () => ({
@@ -7,21 +7,28 @@ vi.mock("@better-fetch/fetch", () => ({
import { betterFetch } from "@better-fetch/fetch";
import { google } from "./google";
import { google, verifyGoogleIdToken } from "./google";
const mockedBetterFetch = vi.mocked(betterFetch);
const CLIENT_ID = "google-client-id";
const CLIENT_SECRET = "google-client-secret";
async function createSignedGoogleToken(payload: Record<string, unknown>) {
async function createSignedGoogleToken(
payload: Record<string, unknown>,
options: { kid?: string | null } = {},
) {
const { publicKey, privateKey } = await generateKeyPair("RS256", {
extractable: true,
});
const publicJWK = await exportJWK(publicKey);
publicJWK.kid = "test-google-key";
publicJWK.kid = options.kid ?? "test-google-key";
publicJWK.alg = "RS256";
publicJWK.use = "sig";
const protectedHeader =
options.kid === null
? ({ alg: "RS256" } as const)
: ({ alg: "RS256", kid: publicJWK.kid } as const);
const token = await new SignJWT({
sub: "google-user-123",
@@ -31,7 +38,7 @@ async function createSignedGoogleToken(payload: Record<string, unknown>) {
picture: "https://example.com/avatar.png",
...payload,
})
.setProtectedHeader({ alg: "RS256", kid: "test-google-key" })
.setProtectedHeader(protectedHeader)
.setIssuer("https://accounts.google.com")
.setAudience(CLIENT_ID)
.setIssuedAt()
@@ -209,3 +216,48 @@ describe("google hosted domain (hd) enforcement", () => {
});
});
});
describe("verifyGoogleIdToken", () => {
beforeEach(() => {
mockedBetterFetch.mockReset();
});
it("verifies an id token without requiring a kid header", async () => {
const { publicJWK, token } = await createSignedGoogleToken(
{},
{ kid: null },
);
mockedBetterFetch.mockResolvedValueOnce({
data: { keys: [publicJWK] },
} as never);
const payload = await verifyGoogleIdToken({
token,
audience: CLIENT_ID,
});
expect(payload?.sub).toBe("google-user-123");
});
it("rejects tokens signed with symmetric algorithms before fetching Google keys", async () => {
const secret = await generateSecret("HS256");
const token = await new SignJWT({
sub: "google-user-123",
email: "user@example.com",
})
.setProtectedHeader({ alg: "HS256" })
.setIssuer("https://accounts.google.com")
.setAudience(CLIENT_ID)
.setIssuedAt()
.setExpirationTime("1h")
.sign(secret);
const payload = await verifyGoogleIdToken({
token,
audience: CLIENT_ID,
});
expect(payload).toBeNull();
expect(mockedBetterFetch).not.toHaveBeenCalled();
});
});
+44 -16
View File
@@ -71,6 +71,16 @@ export interface GoogleOptions extends ProviderOptions<GoogleProfile> {
}
const GOOGLE_ID_TOKEN_MAX_AGE = "1h";
const GOOGLE_ID_TOKEN_ALGORITHMS = ["RS256"] as const;
type GoogleIdTokenAlgorithm = (typeof GOOGLE_ID_TOKEN_ALGORITHMS)[number];
function isGoogleIdTokenAlgorithm(
algorithm: unknown,
): algorithm is GoogleIdTokenAlgorithm {
return GOOGLE_ID_TOKEN_ALGORITHMS.includes(
algorithm as GoogleIdTokenAlgorithm,
);
}
export interface VerifyGoogleIdTokenOptions {
token: string;
@@ -88,22 +98,30 @@ export const verifyGoogleIdToken = async ({
nonce,
}: VerifyGoogleIdTokenOptions): Promise<JWTPayload | null> => {
try {
const { kid, alg: jwtAlg } = decodeProtectedHeader(token);
if (!kid || !jwtAlg) return null;
const { kid, alg } = decodeProtectedHeader(token);
if (!isGoogleIdTokenAlgorithm(alg)) return null;
const publicKey = await getGooglePublicKey(kid);
const { payload: jwtClaims } = await jwtVerify(token, publicKey, {
algorithms: [jwtAlg],
issuer: ["https://accounts.google.com", "accounts.google.com"],
audience,
maxTokenAge: GOOGLE_ID_TOKEN_MAX_AGE,
});
const publicKeys = await getGooglePublicKeys(kid);
for (const publicKey of publicKeys) {
try {
const { payload: jwtClaims } = await jwtVerify(token, publicKey, {
algorithms: GOOGLE_ID_TOKEN_ALGORITHMS,
issuer: ["https://accounts.google.com", "accounts.google.com"],
audience,
maxTokenAge: GOOGLE_ID_TOKEN_MAX_AGE,
});
if (nonce && jwtClaims.nonce !== nonce) {
return null;
if (nonce && jwtClaims.nonce !== nonce) {
return null;
}
return jwtClaims;
} catch {
continue;
}
}
return jwtClaims;
return null;
} catch {
return null;
}
@@ -248,10 +266,18 @@ export const google = (options: GoogleOptions) => {
};
export const getGooglePublicKey = async (kid: string) => {
const [publicKey] = await getGooglePublicKeys(kid);
if (!publicKey) {
throw new Error(`JWK with kid ${kid} not found`);
}
return publicKey;
};
const getGooglePublicKeys = async (kid?: string) => {
const { data } = await betterFetch<{
keys: Array<{
kid: string;
alg: string;
alg?: string;
kty: string;
use: string;
n: string;
@@ -265,10 +291,12 @@ export const getGooglePublicKey = async (kid: string) => {
});
}
const jwk = data.keys.find((key) => key.kid === kid);
if (!jwk) {
const jwks = kid ? data.keys.filter((key) => key.kid === kid) : data.keys;
if (!jwks.length) {
throw new Error(`JWK with kid ${kid} not found`);
}
return await importJWK(jwk, jwk.alg);
return Promise.all(
jwks.map((jwk) => importJWK(jwk, GOOGLE_ID_TOKEN_ALGORITHMS[0])),
);
};
+1 -1
View File
@@ -347,7 +347,7 @@ const DEFAULT_IP_HEADERS = ["x-forwarded-for"];
* `x-forwarded-for`), and falls back to localhost in development and test.
* Returns `null` when tracking is disabled or no trustworthy IP can be resolved.
*/
export function getIp(
export function getIP(
req: Request | Headers,
options: BetterAuthOptions,
): string | null {
+9 -10
View File
@@ -78,16 +78,19 @@ const deleteSCIMProviderConnectionBodySchema = z.object({
});
function getDefaultSSOProviderIds(pluginOptions: unknown): string[] {
const options =
pluginOptions && typeof pluginOptions === "object"
? (pluginOptions as Record<string, unknown>)
: null;
if (
!pluginOptions ||
typeof pluginOptions !== "object" ||
!("defaultSSO" in pluginOptions) ||
!Array.isArray(pluginOptions.defaultSSO)
!options ||
!("defaultSSO" in options) ||
!Array.isArray(options.defaultSSO)
) {
return [];
}
return pluginOptions.defaultSSO
return options.defaultSSO
.map((provider) => {
if (
provider &&
@@ -130,10 +133,6 @@ function resolveRequiredRoles(
return Array.from(new Set(["admin", creatorRole ?? "owner"]));
}
function isProviderOwnershipEnabled(opts: SCIMOptions): boolean {
return opts.providerOwnership?.enabled ?? true;
}
async function getSCIMUserOrgMemberships(
ctx: GenericEndpointContext,
userId: string,
@@ -467,7 +466,7 @@ export const generateSCIMToken = (opts: SCIMOptions) =>
providerId,
organizationId,
scimToken: await storeSCIMToken(ctx, opts, baseToken),
...(isProviderOwnershipEnabled(opts) ? { userId: user.id } : {}),
userId: user.id,
},
});
-9
View File
@@ -94,15 +94,6 @@ export interface SCIMGroupRoleGrant {
}
export type SCIMOptions = {
/**
* SCIM provider ownership configuration. When enabled, each provider
* connection is linked to the user who generated its token.
*
* @default true
*/
providerOwnership?: {
enabled: boolean;
};
/**
* Minimum organization role(s) required for SCIM management operations
* (generate-token, list/get/delete provider connections).
+28
View File
@@ -1250,6 +1250,34 @@ kBGIJYs=
expect(response.status).toBe(400);
});
it("should return 400 when SAML callbackUrl contains a fragment", async () => {
const { auth, getAuthHeaders } = createTestAuth(false);
const headers = await getAuthHeaders({
email: "owner@example.com",
password: "password123",
name: "Owner",
});
const response = await auth.api.registerSSOProvider({
body: {
providerId: "fragment-callback-provider",
issuer: "https://idp.example.com",
domain: "example.com",
samlConfig: {
entryPoint: "https://idp.example.com/sso",
cert: TEST_CERT,
callbackUrl: "http://localhost:3000/dashboard#saml",
spMetadata: {},
},
},
headers,
asResponse: true,
});
expect(response.status).toBe(400);
});
it("should return 400 when no fields provided", async () => {
const { auth, getAuthHeaders, registerSAMLProvider } =
createTestAuth(false);
+263 -142
View File
@@ -42,6 +42,14 @@ interface SSOProviderRecord {
samlConfig?: string | null;
}
type ProviderIdentitySnapshot = {
providerId: string;
issuer: string;
userId?: string | undefined;
oidcConfig?: OIDCConfig | string | null | undefined;
samlConfig?: SAMLConfig | string | null | undefined;
};
const ADMIN_ROLES = ["owner", "admin"];
const OIDC_IDENTITY_BOUNDARY_FIELDS = [
"authorizationEndpoint",
@@ -127,6 +135,96 @@ function samlIdentityBoundaryChanged(
);
}
function parseConfigSnapshot<T>(
config: T | string | null | undefined,
configType: "SAML" | "OIDC",
): T | undefined {
if (!config) {
return undefined;
}
if (typeof config === "string") {
return parseAndValidateConfig<T>(config, configType);
}
return config;
}
function ssoProviderIdentityBoundaryChanged(
current: ProviderIdentitySnapshot,
updated: ProviderIdentitySnapshot,
): boolean {
if (identityValueChanged(current.issuer, updated.issuer)) {
return true;
}
const currentSamlConfig = parseConfigSnapshot<SAMLConfig>(
current.samlConfig,
"SAML",
);
const updatedSamlConfig = parseConfigSnapshot<SAMLConfig>(
updated.samlConfig,
"SAML",
);
if (
currentSamlConfig &&
(!updatedSamlConfig ||
samlIdentityBoundaryChanged(currentSamlConfig, updatedSamlConfig))
) {
return true;
}
const currentOidcConfig = parseConfigSnapshot<OIDCConfig>(
current.oidcConfig,
"OIDC",
);
const updatedOidcConfig = parseConfigSnapshot<OIDCConfig>(
updated.oidcConfig,
"OIDC",
);
return Boolean(
currentOidcConfig &&
(!updatedOidcConfig ||
oidcIdentityBoundaryChanged(currentOidcConfig, updatedOidcConfig)),
);
}
async function lockSSOProviderRow(
adapter: AuthContext["adapter"],
providerId: string,
): Promise<SSOProviderRecord | null> {
const trx = await getCurrentAdapter(adapter);
return trx.update<SSOProviderRecord>({
model: "ssoProvider",
where: [{ field: "providerId", value: providerId }],
update: { providerId },
});
}
export async function lockSSOProviderForAccountLink(
ctx: { context: AuthContext },
provider: ProviderIdentitySnapshot,
) {
const lockedProvider = await lockSSOProviderRow(
ctx.context.adapter,
provider.providerId,
);
if (!lockedProvider) {
if (provider.userId === "default") {
return;
}
throw new APIError("CONFLICT", {
code: "SSO_PROVIDER_CHANGED",
message: "SSO provider changed while account linking was in progress",
});
}
if (ssoProviderIdentityBoundaryChanged(provider, lockedProvider)) {
throw new APIError("CONFLICT", {
code: "SSO_PROVIDER_CHANGED",
message: "SSO provider changed while account linking was in progress",
});
}
}
function getSSOProviderAdditionalFields(options?: SSOOptions) {
return (options?.schema?.ssoProvider?.additionalFields ?? {}) as Record<
string,
@@ -591,167 +689,190 @@ export const updateSSOProvider = (options: SSOOptions) => {
});
}
const existingProvider = await checkProviderAccess(ctx, providerId);
await checkProviderAccess(ctx, providerId);
const updateData: Partial<SSOProviderRecord> = {
...additionalFields,
};
let providerIdentityBoundaryChanged =
body.issuer !== undefined && body.issuer !== existingProvider.issuer;
if (body.issuer !== undefined) {
updateData.issuer = body.issuer;
}
if (body.domain !== undefined) {
updateData.domain = body.domain;
if (body.domain !== existingProvider.domain) {
updateData.domainVerified = false;
}
}
if (body.samlConfig) {
if (body.samlConfig.idpMetadata?.metadata) {
const maxMetadataSize =
options?.saml?.maxMetadataSize ?? DEFAULT_MAX_SAML_METADATA_SIZE;
if (
new TextEncoder().encode(body.samlConfig.idpMetadata.metadata)
.length > maxMetadataSize
) {
throw new APIError("BAD_REQUEST", {
message: `IdP metadata exceeds maximum allowed size (${maxMetadataSize} bytes)`,
const fullProvider = await runWithTransaction(
ctx.context.adapter,
async () => {
const trx = await getCurrentAdapter(ctx.context.adapter);
const existingProvider = await lockSSOProviderRow(
ctx.context.adapter,
providerId,
);
if (!existingProvider) {
throw new APIError("NOT_FOUND", {
message: "Provider not found",
});
}
}
if (
body.samlConfig.signatureAlgorithm !== undefined ||
body.samlConfig.digestAlgorithm !== undefined
) {
validateConfigAlgorithms(
{
signatureAlgorithm: body.samlConfig.signatureAlgorithm,
digestAlgorithm: body.samlConfig.digestAlgorithm,
},
options?.saml?.algorithms,
);
}
const updateData: Partial<SSOProviderRecord> = {
...additionalFields,
};
let providerIdentityBoundaryChanged =
body.issuer !== undefined &&
body.issuer !== existingProvider.issuer;
const currentSamlConfig = parseAndValidateConfig<SAMLConfig>(
existingProvider.samlConfig,
"SAML",
);
const updatedSamlConfig = mergeSAMLConfig(
currentSamlConfig,
body.samlConfig,
updateData.issuer ||
currentSamlConfig.issuer ||
existingProvider.issuer,
);
validateCertSources(updatedSamlConfig);
if (samlIdentityBoundaryChanged(currentSamlConfig, updatedSamlConfig)) {
providerIdentityBoundaryChanged = true;
}
updateData.samlConfig = JSON.stringify(updatedSamlConfig);
}
if (body.oidcConfig) {
try {
validateOIDCEndpointUrls(body.oidcConfig, (url) =>
ctx.context.isTrustedOrigin(url),
);
} catch (error) {
if (error instanceof DiscoveryError) {
throw mapDiscoveryErrorToAPIError(error);
if (body.issuer !== undefined) {
updateData.issuer = body.issuer;
}
throw error;
}
const currentOidcConfig = parseAndValidateConfig<OIDCConfig>(
existingProvider.oidcConfig,
"OIDC",
);
if (body.domain !== undefined) {
updateData.domain = body.domain;
if (body.domain !== existingProvider.domain) {
updateData.domainVerified = false;
}
}
const updatedOidcConfig = mergeOIDCConfig(
currentOidcConfig,
body.oidcConfig,
updateData.issuer ||
currentOidcConfig.issuer ||
existingProvider.issuer,
);
if (body.samlConfig) {
if (body.samlConfig.idpMetadata?.metadata) {
const maxMetadataSize =
options?.saml?.maxMetadataSize ??
DEFAULT_MAX_SAML_METADATA_SIZE;
if (
new TextEncoder().encode(body.samlConfig.idpMetadata.metadata)
.length > maxMetadataSize
) {
throw new APIError("BAD_REQUEST", {
message: `IdP metadata exceeds maximum allowed size (${maxMetadataSize} bytes)`,
});
}
}
// Validate: clientSecret is required for non-private_key_jwt auth
if (
updatedOidcConfig.tokenEndpointAuthentication !== "private_key_jwt" &&
!updatedOidcConfig.clientSecret
) {
throw new APIError("BAD_REQUEST", {
message:
"clientSecret is required when using client_secret_basic or client_secret_post authentication",
});
}
// Validate: private_key_jwt requires a key source at runtime
if (
updatedOidcConfig.tokenEndpointAuthentication === "private_key_jwt" &&
!options?.resolvePrivateKey &&
!options?.defaultSSO?.some(
(p: Record<string, unknown>) =>
p.providerId === providerId && "privateKey" in p && p.privateKey,
)
) {
throw new APIError("BAD_REQUEST", {
message:
"private_key_jwt authentication requires either a resolvePrivateKey callback or a privateKey in defaultSSO",
});
}
if (
body.samlConfig.signatureAlgorithm !== undefined ||
body.samlConfig.digestAlgorithm !== undefined
) {
validateConfigAlgorithms(
{
signatureAlgorithm: body.samlConfig.signatureAlgorithm,
digestAlgorithm: body.samlConfig.digestAlgorithm,
},
options?.saml?.algorithms,
);
}
if (oidcIdentityBoundaryChanged(currentOidcConfig, updatedOidcConfig)) {
providerIdentityBoundaryChanged = true;
}
const currentSamlConfig = parseAndValidateConfig<SAMLConfig>(
existingProvider.samlConfig,
"SAML",
);
updateData.oidcConfig = JSON.stringify(updatedOidcConfig);
}
const updatedSamlConfig = mergeSAMLConfig(
currentSamlConfig,
body.samlConfig,
updateData.issuer ||
currentSamlConfig.issuer ||
existingProvider.issuer,
);
if (providerIdentityBoundaryChanged) {
const linkedAccount = await ctx.context.adapter.findOne<{ id: string }>(
{
model: "account",
validateCertSources(updatedSamlConfig);
if (
samlIdentityBoundaryChanged(currentSamlConfig, updatedSamlConfig)
) {
providerIdentityBoundaryChanged = true;
}
updateData.samlConfig = JSON.stringify(updatedSamlConfig);
}
if (body.oidcConfig) {
try {
validateOIDCEndpointUrls(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",
);
const updatedOidcConfig = mergeOIDCConfig(
currentOidcConfig,
body.oidcConfig,
updateData.issuer ||
currentOidcConfig.issuer ||
existingProvider.issuer,
);
// Validate: clientSecret is required for non-private_key_jwt auth
if (
updatedOidcConfig.tokenEndpointAuthentication !==
"private_key_jwt" &&
!updatedOidcConfig.clientSecret
) {
throw new APIError("BAD_REQUEST", {
message:
"clientSecret is required when using client_secret_basic or client_secret_post authentication",
});
}
// Validate: private_key_jwt requires a key source at runtime
if (
updatedOidcConfig.tokenEndpointAuthentication ===
"private_key_jwt" &&
!options?.resolvePrivateKey &&
!options?.defaultSSO?.some(
(p: Record<string, unknown>) =>
p.providerId === providerId &&
"privateKey" in p &&
p.privateKey,
)
) {
throw new APIError("BAD_REQUEST", {
message:
"private_key_jwt authentication requires either a resolvePrivateKey callback or a privateKey in defaultSSO",
});
}
if (
oidcIdentityBoundaryChanged(currentOidcConfig, updatedOidcConfig)
) {
providerIdentityBoundaryChanged = true;
}
updateData.oidcConfig = JSON.stringify(updatedOidcConfig);
}
if (providerIdentityBoundaryChanged) {
const linkedAccount = await trx.findOne<{ id: string }>({
model: "account",
where: [{ field: "providerId", value: providerId }],
});
if (linkedAccount) {
// TODO(next): move SSO account links to immutable provider instance
// ids, then expose explicit relinking for identity-boundary changes.
throw new APIError("CONFLICT", {
message:
"Cannot change SSO provider identity fields while linked accounts exist",
});
}
}
await trx.update({
model: "ssoProvider",
where: [{ field: "providerId", value: providerId }],
},
);
if (linkedAccount) {
// TODO(next): move SSO account links to immutable provider instance
// ids, then expose explicit relinking for race-proof
// identity-boundary changes.
throw new APIError("CONFLICT", {
message:
"Cannot change SSO provider identity fields while linked accounts exist",
update: updateData,
});
}
}
await ctx.context.adapter.update({
model: "ssoProvider",
where: [{ field: "providerId", value: providerId }],
update: updateData,
});
const updatedProvider = await trx.findOne<SSOProviderRecord>({
model: "ssoProvider",
where: [{ field: "providerId", value: providerId }],
});
const fullProvider = await ctx.context.adapter.findOne<SSOProviderRecord>(
{
model: "ssoProvider",
where: [{ field: "providerId", value: providerId }],
if (!updatedProvider) {
throw new APIError("NOT_FOUND", {
message: "Provider not found after update",
});
}
return updatedProvider;
},
);
if (!fullProvider) {
throw new APIError("NOT_FOUND", {
message: "Provider not found after update",
});
}
return ctx.json(
sanitizeProvider(fullProvider, ctx.context.baseURL, options),
);
+43 -23
View File
@@ -1,3 +1,4 @@
import { runWithTransaction } from "@better-auth/core/context";
import { isAPIError } from "@better-auth/core/utils/is-api-error";
import type { User } from "better-auth";
import { APIError } from "better-auth/api";
@@ -35,6 +36,7 @@ import {
validateEmailDomain,
} from "../utils";
import { createIdP, createSP, findSAMLProvider } from "./helpers";
import { lockSSOProviderForAccountLink } from "./providers";
type RelayState = Awaited<ReturnType<typeof parseRelayState>>;
@@ -111,8 +113,23 @@ function buildSAMLRedirectUrl(
params: Record<string, string>,
): string {
const searchParams = new URLSearchParams(params);
const separator = url.includes("?") ? "&" : "?";
return `${url}${separator}${searchParams.toString()}`;
try {
const isRelativePath = url.startsWith("/") && !url.startsWith("//");
const parsedUrl = new URL(url, "http://better-auth.local");
for (const [key, value] of searchParams) {
parsedUrl.searchParams.set(key, value);
}
if (isRelativePath) {
return `${parsedUrl.pathname}${parsedUrl.search}${parsedUrl.hash}`;
}
return parsedUrl.toString();
} catch {
const hashIndex = url.indexOf("#");
const urlWithoutFragment = hashIndex === -1 ? url : url.slice(0, hashIndex);
const fragment = hashIndex === -1 ? "" : url.slice(hashIndex);
const separator = urlWithoutFragment.includes("?") ? "&" : "?";
return `${urlWithoutFragment}${separator}${searchParams.toString()}${fragment}`;
}
}
function toArray<T>(value: T | T[] | undefined): T[] {
@@ -517,27 +534,30 @@ export async function processSAMLResponse(
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: userInfo.emailVerified,
},
account: {
providerId,
accountId: userInfo.id as string,
accessToken: "",
refreshToken: "",
},
callbackURL: callbackUrl,
disableSignUp: options?.disableImplicitSignUp,
source: {
method: "sso-saml",
sso: { providerId, profile: attributes },
},
isTrustedProvider,
trustProviderByName: false,
result = await runWithTransaction(ctx.context.adapter, async () => {
await lockSSOProviderForAccountLink(ctx, provider);
return handleOAuthUserInfo(ctx, {
userInfo: {
email: userInfo.email as string,
name: (userInfo.name || userInfo.email) as string,
id: userInfo.id as string,
emailVerified: userInfo.emailVerified,
},
account: {
providerId,
accountId: userInfo.id as string,
accessToken: "",
refreshToken: "",
},
callbackURL: callbackUrl,
disableSignUp: options?.disableImplicitSignUp,
source: {
method: "sso-saml",
sso: { providerId, profile: attributes },
},
isTrustedProvider,
trustProviderByName: false,
});
});
} catch (e) {
if (isAPIError(e) && e.body?.code) {
+6 -1
View File
@@ -188,7 +188,12 @@ const samlConfigSchema = z.object({
})
.optional(),
audience: z.string().optional(),
callbackUrl: z.string().optional(),
callbackUrl: z
.string()
.refine((url) => !url.includes("#"), {
message: "callbackUrl must not contain a fragment",
})
.optional(),
idpMetadata: z
.object({
metadata: z.string().optional(),
+38 -33
View File
@@ -1,3 +1,4 @@
import { runWithTransaction } from "@better-auth/core/context";
import { isAPIError } from "@better-auth/core/utils/is-api-error";
import type {
PrivateKeyJwtSigningAlgorithm,
@@ -72,6 +73,7 @@ import {
import {
filterSSOProviderAdditionalFields,
hasOrgAdminRole,
lockSSOProviderForAccountLink,
} from "./providers";
import { getSafeRedirectUrl, processSAMLResponse } from "./saml-pipeline";
import {
@@ -1633,39 +1635,42 @@ async function handleOIDCCallback(
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,
source: {
method: "sso-oidc",
sso: { providerId: provider.providerId, profile: rawProfile },
},
isTrustedProvider,
// SSO provider ids are user-controlled and live in the same namespace
// as social providers. Never inherit trust from the global
// `trustedProviders` list by name — rely solely on the SSO-specific
// `isTrustedProvider` (verified domain ownership) computed above.
trustProviderByName: false,
linked = await runWithTransaction(ctx.context.adapter, async () => {
await lockSSOProviderForAccountLink(ctx, provider);
return 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,
source: {
method: "sso-oidc",
sso: { providerId: provider.providerId, profile: rawProfile },
},
isTrustedProvider,
// SSO provider ids are user-controlled and live in the same namespace
// as social providers. Never inherit trust from the global
// `trustedProviders` list by name — rely solely on the SSO-specific
// `isTrustedProvider` (verified domain ownership) computed above.
trustProviderByName: false,
});
});
} catch (e) {
if (isAPIError(e) && e.body?.code) {
+7
View File
@@ -40,6 +40,13 @@ export const referenceMiddleware = (
// Resolve once and expose on the context so the handler reads this exact id instead of re-deriving it.
if (customerType === "organization") {
if (!options.organization?.enabled) {
throw APIError.from(
"BAD_REQUEST",
STRIPE_ERROR_CODES.ORGANIZATION_SUBSCRIPTION_NOT_ENABLED,
);
}
// Organization subscriptions always require authorizeReference
if (!subscriptionOptions.authorizeReference) {
ctx.context.logger.error(
+59 -1
View File
@@ -263,9 +263,15 @@ describe("referenceMiddleware", () => {
test("should reject when authorizeReference is not defined", async ({
stripeOptions,
}) => {
const stripeOptionsWithOrg: StripeOptions = {
...stripeOptions,
organization: {
enabled: true,
},
};
const { client, sessionSetter } = await getTestInstance(
{
plugins: [stripe(stripeOptions)],
plugins: [stripe(stripeOptionsWithOrg)],
},
{
disableTestUser: true,
@@ -303,6 +309,9 @@ describe("referenceMiddleware", () => {
}) => {
const stripeOptionsWithAuth: StripeOptions = {
...stripeOptions,
organization: {
enabled: true,
},
subscription: {
...stripeOptions.subscription,
authorizeReference: async () => true,
@@ -348,6 +357,9 @@ describe("referenceMiddleware", () => {
}) => {
const stripeOptionsWithAuth: StripeOptions = {
...stripeOptions,
organization: {
enabled: true,
},
subscription: {
...stripeOptions.subscription,
authorizeReference: async () => false,
@@ -389,6 +401,52 @@ describe("referenceMiddleware", () => {
expect(res.error?.code).toBe("UNAUTHORIZED");
});
test("should reject organization customerType when organization support is disabled", async ({
stripeOptions,
}) => {
const stripeOptionsWithAuth: StripeOptions = {
...stripeOptions,
subscription: {
...stripeOptions.subscription,
authorizeReference: async () => true,
},
};
const { client, sessionSetter } = await getTestInstance(
{
plugins: [stripe(stripeOptionsWithAuth)],
},
{
disableTestUser: true,
clientOptions: {
plugins: [stripeClient({ subscription: true })],
},
},
);
await client.signUp.email(
{ ...testUser, email: "org-disabled@example.com" },
{ throw: true },
);
const headers = new Headers();
await client.signIn.email(
{ ...testUser, email: "org-disabled@example.com" },
{
throw: true,
onSuccess: sessionSetter(headers),
},
);
const res = await client.subscription.upgrade({
plan: "starter",
customerType: "organization",
referenceId: "org_123",
fetchOptions: { headers },
});
expect(res.error?.code).toBe("ORGANIZATION_SUBSCRIPTION_NOT_ENABLED");
});
test("should pass when authorizeReference returns true", async ({
stripeOptions,
}) => {
@@ -967,8 +967,7 @@ describe("stripe - organization customer", () => {
},
);
// Try to upgrade with organization customerType when organization is not enabled
// Without authorizeReference, middleware rejects organization subscriptions
// Try to upgrade with organization customerType when organization is not enabled.
const res = await client.subscription.upgrade({
plan: "starter",
customerType: "organization",
@@ -976,7 +975,7 @@ describe("stripe - organization customer", () => {
fetchOptions: { headers },
});
expect(res.error?.code).toBe("AUTHORIZE_REFERENCE_REQUIRED");
expect(res.error?.code).toBe("ORGANIZATION_SUBSCRIPTION_NOT_ENABLED");
});
test("should keep user and organization subscriptions separate", async ({