diff --git a/packages/api-key/src/index.ts b/packages/api-key/src/index.ts index 92a1f08809..493e645841 100644 --- a/packages/api-key/src/index.ts +++ b/packages/api-key/src/index.ts @@ -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(), diff --git a/packages/better-auth/src/api/index.ts b/packages/better-auth/src/api/index.ts index 686241de37..89999daed9 100644 --- a/packages/better-auth/src/api/index.ts +++ b/packages/better-auth/src/api/index.ts @@ -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"; diff --git a/packages/better-auth/src/api/rate-limiter/index.ts b/packages/better-auth/src/api/rate-limiter/index.ts index c5d84596e4..d0f7d53956 100644 --- a/packages/better-auth/src/api/rate-limiter/index.ts +++ b/packages/better-auth/src/api/rate-limiter/index.ts @@ -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; diff --git a/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts b/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts index 3c5879cdf7..34c1c76194 100644 --- a/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts +++ b/packages/better-auth/src/api/rate-limiter/rate-limiter.test.ts @@ -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), }); // `, , `: spoofed rotation diff --git a/packages/better-auth/src/db/internal-adapter.ts b/packages/better-auth/src/db/internal-adapter.ts index e41bd4234b..2e75f6a1b8 100644 --- a/packages/better-auth/src/db/internal-adapter.ts +++ b/packages/better-auth/src/db/internal-adapter.ts @@ -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, /** diff --git a/packages/better-auth/src/plugins/captcha/index.ts b/packages/better-auth/src/plugins/captcha/index.ts index 1397c010ee..750e55281a 100644 --- a/packages/better-auth/src/plugins/captcha/index.ts +++ b/packages/better-auth/src/plugins/captcha/index.ts @@ -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({ diff --git a/packages/better-auth/src/plugins/siwe/index.ts b/packages/better-auth/src/plugins/siwe/index.ts index 4ab551464b..72c16fba0f 100644 --- a/packages/better-auth/src/plugins/siwe/index.ts +++ b/packages/better-auth/src/plugins/siwe/index.ts @@ -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({ diff --git a/packages/better-auth/src/plugins/two-factor/verify-two-factor.ts b/packages/better-auth/src/plugins/two-factor/verify-two-factor.ts index 9be6001421..2bc864bab0 100644 --- a/packages/better-auth/src/plugins/two-factor/verify-two-factor.ts +++ b/packages/better-auth/src/plugins/two-factor/verify-two-factor.ts @@ -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", diff --git a/packages/cli/src/generators/drizzle.ts b/packages/cli/src/generators/drizzle.ts index 8020aa69fa..23da66189b 100644 --- a/packages/cli/src/generators/drizzle.ts +++ b/packages/cli/src/generators/drizzle.ts @@ -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 diff --git a/packages/cli/src/generators/prisma.ts b/packages/cli/src/generators/prisma.ts index 91dbf4c5a5..ed0733bb7a 100644 --- a/packages/cli/src/generators/prisma.ts +++ b/packages/cli/src/generators/prisma.ts @@ -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; diff --git a/packages/cli/test/generate.test.ts b/packages/cli/test/generate.test.ts index 5622945bdb..b8bf6a4c26 100644 --- a/packages/cli/test/generate.test.ts +++ b/packages/cli/test/generate.test.ts @@ -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"); }); }); diff --git a/packages/core/src/social-providers/google.test.ts b/packages/core/src/social-providers/google.test.ts index f1dbfc8a8e..af172f2828 100644 --- a/packages/core/src/social-providers/google.test.ts +++ b/packages/core/src/social-providers/google.test.ts @@ -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) { +async function createSignedGoogleToken( + payload: Record, + 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) { 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(); + }); +}); diff --git a/packages/core/src/social-providers/google.ts b/packages/core/src/social-providers/google.ts index b97df9ab29..b26e94337f 100644 --- a/packages/core/src/social-providers/google.ts +++ b/packages/core/src/social-providers/google.ts @@ -71,6 +71,16 @@ export interface GoogleOptions extends ProviderOptions { } 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 => { 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])), + ); }; diff --git a/packages/core/src/utils/ip.ts b/packages/core/src/utils/ip.ts index 35416c1a79..a109cde3db 100644 --- a/packages/core/src/utils/ip.ts +++ b/packages/core/src/utils/ip.ts @@ -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 { diff --git a/packages/scim/src/routes.ts b/packages/scim/src/routes.ts index 2e03ccca9b..223cf63056 100644 --- a/packages/scim/src/routes.ts +++ b/packages/scim/src/routes.ts @@ -78,16 +78,19 @@ const deleteSCIMProviderConnectionBodySchema = z.object({ }); function getDefaultSSOProviderIds(pluginOptions: unknown): string[] { + const options = + pluginOptions && typeof pluginOptions === "object" + ? (pluginOptions as Record) + : 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, }, }); diff --git a/packages/scim/src/types.ts b/packages/scim/src/types.ts index 2725f7d525..f5468bc336 100644 --- a/packages/scim/src/types.ts +++ b/packages/scim/src/types.ts @@ -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). diff --git a/packages/sso/src/providers.test.ts b/packages/sso/src/providers.test.ts index 03a6a941ba..0ee7994553 100644 --- a/packages/sso/src/providers.test.ts +++ b/packages/sso/src/providers.test.ts @@ -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); diff --git a/packages/sso/src/routes/providers.ts b/packages/sso/src/routes/providers.ts index 88e68e95be..2c04ce2fb2 100644 --- a/packages/sso/src/routes/providers.ts +++ b/packages/sso/src/routes/providers.ts @@ -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( + config: T | string | null | undefined, + configType: "SAML" | "OIDC", +): T | undefined { + if (!config) { + return undefined; + } + if (typeof config === "string") { + return parseAndValidateConfig(config, configType); + } + return config; +} + +function ssoProviderIdentityBoundaryChanged( + current: ProviderIdentitySnapshot, + updated: ProviderIdentitySnapshot, +): boolean { + if (identityValueChanged(current.issuer, updated.issuer)) { + return true; + } + + const currentSamlConfig = parseConfigSnapshot( + current.samlConfig, + "SAML", + ); + const updatedSamlConfig = parseConfigSnapshot( + updated.samlConfig, + "SAML", + ); + if ( + currentSamlConfig && + (!updatedSamlConfig || + samlIdentityBoundaryChanged(currentSamlConfig, updatedSamlConfig)) + ) { + return true; + } + + const currentOidcConfig = parseConfigSnapshot( + current.oidcConfig, + "OIDC", + ); + const updatedOidcConfig = parseConfigSnapshot( + updated.oidcConfig, + "OIDC", + ); + return Boolean( + currentOidcConfig && + (!updatedOidcConfig || + oidcIdentityBoundaryChanged(currentOidcConfig, updatedOidcConfig)), + ); +} + +async function lockSSOProviderRow( + adapter: AuthContext["adapter"], + providerId: string, +): Promise { + const trx = await getCurrentAdapter(adapter); + return trx.update({ + 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 = { - ...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 = { + ...additionalFields, + }; + let providerIdentityBoundaryChanged = + body.issuer !== undefined && + body.issuer !== existingProvider.issuer; - const currentSamlConfig = parseAndValidateConfig( - 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( - 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) => - 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( + 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( + 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) => + 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({ + model: "ssoProvider", + where: [{ field: "providerId", value: providerId }], + }); - const fullProvider = await ctx.context.adapter.findOne( - { - 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), ); diff --git a/packages/sso/src/routes/saml-pipeline.ts b/packages/sso/src/routes/saml-pipeline.ts index 4ab04ce8fa..1128fabed3 100644 --- a/packages/sso/src/routes/saml-pipeline.ts +++ b/packages/sso/src/routes/saml-pipeline.ts @@ -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>; @@ -111,8 +113,23 @@ function buildSAMLRedirectUrl( params: Record, ): 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(value: T | T[] | undefined): T[] { @@ -517,27 +534,30 @@ export async function processSAMLResponse( 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: 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) { diff --git a/packages/sso/src/routes/schemas.ts b/packages/sso/src/routes/schemas.ts index 1bcfb012ff..46fd85127c 100644 --- a/packages/sso/src/routes/schemas.ts +++ b/packages/sso/src/routes/schemas.ts @@ -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(), diff --git a/packages/sso/src/routes/sso.ts b/packages/sso/src/routes/sso.ts index 06959feedd..a3382b0953 100644 --- a/packages/sso/src/routes/sso.ts +++ b/packages/sso/src/routes/sso.ts @@ -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>; 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) { diff --git a/packages/stripe/src/middleware.ts b/packages/stripe/src/middleware.ts index 74dbc4c7e7..b3f5ed06be 100644 --- a/packages/stripe/src/middleware.ts +++ b/packages/stripe/src/middleware.ts @@ -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( diff --git a/packages/stripe/test/middleware.test.ts b/packages/stripe/test/middleware.test.ts index a0c3522c61..3ff9ba53d4 100644 --- a/packages/stripe/test/middleware.test.ts +++ b/packages/stripe/test/middleware.test.ts @@ -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, }) => { diff --git a/packages/stripe/test/stripe-organization.test.ts b/packages/stripe/test/stripe-organization.test.ts index c047fef870..5b1761fa27 100644 --- a/packages/stripe/test/stripe-organization.test.ts +++ b/packages/stripe/test/stripe-organization.test.ts @@ -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 ({