diff --git a/apps/sim/app/api/auth/[...all]/route.test.ts b/apps/sim/app/api/auth/[...all]/route.test.ts index 4c577409990..3207be6bd32 100644 --- a/apps/sim/app/api/auth/[...all]/route.test.ts +++ b/apps/sim/app/api/auth/[...all]/route.test.ts @@ -50,7 +50,7 @@ describe('auth catch-all route (DISABLE_AUTH get-session)', () => { 'http://localhost:3000/api/auth/get-session' ) - const res = await GET(req as any) + const res = await GET(req) const json = await res.json() expect(handlerMocks.ensureAnonymousUserExists).toHaveBeenCalledTimes(1) @@ -68,7 +68,7 @@ describe('auth catch-all route (DISABLE_AUTH get-session)', () => { handlerMocks.betterAuthGET.mockResolvedValueOnce( new NextResponse(JSON.stringify({ data: { ok: true } }), { headers: { 'content-type': 'application/json' }, - }) as any + }) ) const req = createMockRequest( @@ -78,7 +78,7 @@ describe('auth catch-all route (DISABLE_AUTH get-session)', () => { 'http://localhost:3000/api/auth/get-session' ) - const res = await GET(req as any) + const res = await GET(req) const json = await res.json() expect(handlerMocks.ensureAnonymousUserExists).not.toHaveBeenCalled() @@ -100,7 +100,7 @@ describe('auth catch-all route organization mutations', () => { 'http://localhost:3000/api/auth/organization/create' ) - const res = await POST(req as any) + const res = await POST(req) const json = await res.json() expect(res.status).toBe(404) @@ -115,7 +115,7 @@ describe('auth catch-all route organization mutations', () => { handlerMocks.betterAuthPOST.mockResolvedValueOnce( new NextResponse(JSON.stringify({ data: { ok: true } }), { headers: { 'content-type': 'application/json' }, - }) as any + }) ) const req = createMockRequest( @@ -125,10 +125,77 @@ describe('auth catch-all route organization mutations', () => { 'http://localhost:3000/api/auth/organization/set-active' ) - const res = await POST(req as any) + const res = await POST(req) const json = await res.json() expect(handlerMocks.betterAuthPOST).toHaveBeenCalledTimes(1) expect(json).toEqual({ data: { ok: true } }) }) }) + +describe('auth catch-all route SSO provider mutations', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it.each([ + 'sso/update-provider', + 'sso/delete-provider', + 'sso/request-domain-verification', + 'sso/verify-domain', + ])('blocks the plugin-served %s endpoint', async (path) => { + const req = createMockRequest('POST', undefined, {}, `http://localhost:3000/api/auth/${path}`) + + const res = await POST(req) + const json = await res.json() + + expect(res.status).toBe(404) + expect(handlerMocks.betterAuthPOST).not.toHaveBeenCalled() + expect(json).toEqual({ + error: 'SSO provider mutations are handled by application API routes.', + }) + }) + + it.each([ + 'sso/saml2/callback/acme', + 'sso/saml2/sp/acs/acme', + 'sso/saml2/sp/slo/acme', + 'sso/saml2/logout/acme', + ])('allows the SAML protocol endpoint %s', async (path) => { + const { NextResponse } = await import('next/server') + handlerMocks.betterAuthPOST.mockResolvedValueOnce( + new NextResponse(JSON.stringify({ data: { ok: true } }), { + headers: { 'content-type': 'application/json' }, + }) + ) + + const req = createMockRequest('POST', undefined, {}, `http://localhost:3000/api/auth/${path}`) + + const res = await POST(req) + const json = await res.json() + + expect(handlerMocks.betterAuthPOST).toHaveBeenCalledTimes(1) + expect(json).toEqual({ data: { ok: true } }) + }) + + it('leaves the SSO sign-in endpoint reachable', async () => { + const { NextResponse } = await import('next/server') + handlerMocks.betterAuthPOST.mockResolvedValueOnce( + new NextResponse(JSON.stringify({ data: { url: 'https://idp.example.com' } }), { + headers: { 'content-type': 'application/json' }, + }) + ) + + const req = createMockRequest( + 'POST', + undefined, + {}, + 'http://localhost:3000/api/auth/sign-in/sso' + ) + + const res = await POST(req) + + expect(handlerMocks.betterAuthPOST).toHaveBeenCalledTimes(1) + expect(await res.json()).toEqual({ data: { url: 'https://idp.example.com' } }) + }) +}) diff --git a/apps/sim/app/api/auth/[...all]/route.ts b/apps/sim/app/api/auth/[...all]/route.ts index 8456afff4cf..51f2fdd5282 100644 --- a/apps/sim/app/api/auth/[...all]/route.ts +++ b/apps/sim/app/api/auth/[...all]/route.ts @@ -10,6 +10,14 @@ export const dynamic = 'force-dynamic' const { GET: betterAuthGET, POST: betterAuthPOST } = toNextJsHandler(auth.handler) const SAFE_ORGANIZATION_POST_PATHS = new Set(['organization/check-slug', 'organization/set-active']) +/** + * SAML protocol endpoints the IdP posts to (`saml2/callback/:id`, + * `saml2/sp/acs/:id`, `saml2/sp/slo/:id`, `saml2/logout/:id`). These are the + * only SSO paths the plugin must keep serving on POST — every other SSO POST + * endpoint it registers is a provider mutation. + */ +const SAML_PROTOCOL_POST_PREFIX = 'sso/saml2/' + function getAuthPath(request: NextRequest): string { const pathname = request.nextUrl?.pathname ?? new URL(request.url).pathname return pathname.replace('/api/auth/', '') @@ -19,6 +27,30 @@ function isBlockedOrganizationMutationPath(path: string): boolean { return path.startsWith('organization/') && !SAFE_ORGANIZATION_POST_PATHS.has(path) } +/** + * SSO provider configuration is owned by `/api/auth/sso/register`, which proves + * domain ownership before granting trust and restricts the attribute mapping to + * `id`/`email`/`name`/`image`. The plugin's own `sso/update-provider` bypasses + * both: it is gated only on provider ownership and merges the caller's config, + * so a provider owner could add `mapping.emailVerified` — a change the plugin's + * identity-boundary guard does not consider, so it never trips the linked-account + * conflict — and then assert an arbitrary victim's email as verified to auto-link + * into their account. `sso/delete-provider` likewise lets an owner drop a login + * path outside the application's flow. + * + * `trustEmailVerified: false` independently defuses that claim, so these two + * guards are layered, not redundant: this one keeps provider configuration + * owned by the register route (which alone proves domain ownership) and is what + * stops the mapping rewrite from becoming live again if that option is ever + * reconsidered. + * + * Deny-by-default rather than a blocklist so a future plugin version cannot + * introduce another unshadowed provider mutation. + */ +function isBlockedSsoMutationPath(path: string): boolean { + return path.startsWith('sso/') && !path.startsWith(SAML_PROTOCOL_POST_PREFIX) +} + export const GET = withRouteHandler(async (request: NextRequest) => { const path = getAuthPath(request) @@ -40,5 +72,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => { ) } + if (isBlockedSsoMutationPath(path)) { + return NextResponse.json( + { error: 'SSO provider mutations are handled by application API routes.' }, + { status: 404 } + ) + } + return betterAuthPOST(request) }) diff --git a/apps/sim/lib/auth/auth.ts b/apps/sim/lib/auth/auth.ts index 7906d93707d..915cc5d0cdd 100644 --- a/apps/sim/lib/auth/auth.ts +++ b/apps/sim/lib/auth/auth.ts @@ -1169,14 +1169,24 @@ export const auth = betterAuth({ ? [ sso({ /** - * Honor the IdP's `email_verified` claim so the local account is - * verified rather than forced to false. + * MUST stay false. Better Auth's link gate is + * `!isTrustedProvider && !userInfo.emailVerified`, so a true + * `email_verified` claim substitutes for the domain binding + * entirely: an IdP could assert any address — including one from a + * domain it does not own — and auto-link into that user's existing + * account. Since a provider row can be registered by any Enterprise + * org admin (and by any signed-in user when self-hosted), trusting + * the claim makes every account reachable from any tenant's IdP. * - * This is not what enables linking — Entra omits the claim entirely, - * and SAML ignores it without an explicit `mapping.emailVerified`. - * `domainVerification` below establishes linking trust. + * Turning it on only ever set `emailVerified` on the local row; it + * was never what made linking work. Entra omits the claim, and SAML + * ignores it without an explicit `mapping.emailVerified` that the + * register contract does not accept — so SSO users are created + * unverified either way, and `domainVerification` below is the sole + * linking trust source, which is what `trustProviderByName: false` + * already assumes. */ - trustEmailVerified: true, + trustEmailVerified: false, /** * Marks a provider authoritative for its domain, which is what lets an * SSO sign-in auto-link to an existing same-email account. Without it @@ -1187,9 +1197,10 @@ export const auth = betterAuth({ * proven by the `sso_domain` flow before registration, and the register * route mirrors that decision onto this flag. * - * It narrows nothing on its own — an IdP asserting `email_verified` - * links regardless of domain (see `trustEmailVerified` above). It - * exists so linking survives IdPs that omit the claim. + * With `trustEmailVerified` off this is the only path to linking, and + * it is domain-scoped: `isTrustedProvider` additionally requires + * `validateEmailDomain(userInfo.email, provider.domain)`, so a + * provider can only ever claim identities inside the domain it proved. */ domainVerification: { enabled: true }, organizationProvisioning: { diff --git a/apps/sim/lib/auth/sso-trust.test.ts b/apps/sim/lib/auth/sso-trust.test.ts new file mode 100644 index 00000000000..1b620d4529c --- /dev/null +++ b/apps/sim/lib/auth/sso-trust.test.ts @@ -0,0 +1,39 @@ +/** + * @vitest-environment node + * + * Locks the SSO linking trust model. Better Auth's account-link gate is + * `!isTrustedProvider && !userInfo.emailVerified`, so a truthy + * `trustEmailVerified` lets any registered IdP assert an out-of-domain address + * as verified and auto-link into that user's account, bypassing the + * domain-verification proof entirely. + */ +import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing' +import { afterAll, expect, it, vi } from 'vitest' + +const { ssoOptions } = vi.hoisted(() => ({ + ssoOptions: { current: undefined as Record | undefined }, +})) + +vi.mock('@better-auth/sso', () => ({ + sso: (options: Record) => { + ssoOptions.current = options + return { id: 'sso' } + }, +})) + +setEnvFlags({ isSsoEnabled: true }) + +afterAll(resetEnvFlagsMock) + +it('never trusts the IdP-supplied email_verified claim for SSO linking', async () => { + await import('@/lib/auth/auth') + + expect(ssoOptions.current).toBeDefined() + expect(ssoOptions.current?.trustEmailVerified).toBe(false) +}) + +it('keeps domain verification as the sole SSO linking trust source', async () => { + await import('@/lib/auth/auth') + + expect(ssoOptions.current?.domainVerification).toEqual({ enabled: true }) +})