mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(auth): scope SSO account linking to the verified domain and fence plugin provider mutations (#6738)
* fix(auth): fence off plugin-served SSO provider mutation endpoints The auth catch-all forwarded every non-organization POST to the better-auth SSO plugin, leaving sso/update-provider and sso/delete-provider reachable alongside the app-owned sso/register route. update-provider is gated only on provider ownership and merges the caller's samlConfig, so a provider owner could set mapping.emailVerified — a field the register contract deliberately omits and the plugin's identity-boundary guard does not inspect, so it never trips the linked-account conflict. With trustEmailVerified enabled, a subsequent assertion carrying an arbitrary verified email auto-links to that user's account. Block SSO POST paths by default, allowing only the sso/saml2/ protocol endpoints the IdP posts to, mirroring the existing organization fence. * fix(auth): stop trusting IdP email_verified for SSO account linking Better Auth's link gate is `!isTrustedProvider && !userInfo.emailVerified`, so trustEmailVerified let a true email_verified claim stand in for the domain binding. Any principal able to register an SSO provider — an Enterprise org admin, or any signed-in user when self-hosted — could point it at an IdP they control, assert an arbitrary victim's address as verified, and auto-link into that account across tenant boundaries, persisting as an account row. With it off, linking requires isTrustedProvider, which is domainVerified plus validateEmailDomain(email, provider.domain) — a provider can only claim identities inside the domain it proved. That is the model the codebase already documents for trustProviderByName: false. The option only ever set emailVerified on the local row; it was never what made linking work, since Entra omits the claim and SAML ignores it without a mapping the register contract does not accept. * docs(auth): note that the SSO fence and trustEmailVerified are layered * test(auth): drop unnecessary any casts from the auth catch-all tests createMockRequest already returns a NextRequest and the handler mocks are untyped vi.fn()s, so every cast in the file was suppressing type checking for no reason.
This commit is contained in:
@@ -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' } })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
@@ -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: {
|
||||
|
||||
@@ -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<string, unknown> | undefined },
|
||||
}))
|
||||
|
||||
vi.mock('@better-auth/sso', () => ({
|
||||
sso: (options: Record<string, unknown>) => {
|
||||
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 })
|
||||
})
|
||||
Reference in New Issue
Block a user