Skip to content

Commit 6b9261c

Browse files
committed
fix(sso): close register TOCTOU with compensating delete + harden edges
Audit-driven hardening: - Close the residual register TOCTOU: registerSSOProvider is create-only (throws if the providerId exists), so a compensating delete after the write is provably safe — it can only remove the just-created row. If verification was revoked during the write, roll the provider back and 403. - Verify is now idempotent under concurrency: a same-org row already flipped to verified by a racing request returns 200, not a confusing 409. - Grandfather backfill + self-host script now match normalizeSSODomain's dominant transforms (lower + trim + strip leading wildcard) so a non-canonical legacy domain can't miss the runtime gate's lookup. Prod backfill result is unchanged. - Cleanup: drop dead default export, align card radius to sibling convention.
1 parent b40100b commit 6b9261c

7 files changed

Lines changed: 89 additions & 15 deletions

File tree

apps/sim/app/api/auth/sso/register/route.test.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,11 @@ describe('POST /api/auth/sso/register', () => {
9696
mockRegisterSSOProvider.mockResolvedValue({ providerId: 'acme-oidc' })
9797
// Default: the org has already verified the domain, so the ownership gate
9898
// passes and each test exercises the logic beyond it. The gate is checked
99-
// twice for org-scoped registration (fail-fast entry + authoritative
100-
// re-check before the write), so queue two rows. Gate-specific tests reset
101-
// the queue to assert the unverified path.
99+
// three times for a successful org-scoped registration (fail-fast entry +
100+
// authoritative re-check before the write + compensating re-check after the
101+
// write), so queue three rows. Gate-specific tests reset the queue to assert
102+
// the unverified paths.
103+
queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
102104
queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
103105
queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
104106
})
@@ -152,6 +154,20 @@ describe('POST /api/auth/sso/register', () => {
152154
expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
153155
})
154156

157+
it('rolls back the newly-created provider if verification is revoked after the write', async () => {
158+
resetDbChainMock()
159+
queueMembers([{ organizationId: 'org1', role: 'owner' }])
160+
queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // entry gate: verified
161+
queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // pre-write re-check: verified
162+
queueTableRows(schemaMock.ssoDomain, []) // post-write compensating check: revoked
163+
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
164+
const json = await res.json()
165+
expect(res.status).toBe(403)
166+
expect(json.code).toBe('SSO_DOMAIN_NOT_VERIFIED')
167+
expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) // it was created…
168+
expect(dbChainMockFns.delete).toHaveBeenCalled() // …then rolled back
169+
})
170+
155171
it('rejects a domain already registered by another organization', async () => {
156172
queueMembers([{ organizationId: 'org-attacker', role: 'owner' }])
157173
queueProviders([{ domain: 'acme.com', userId: 'u-victim', organizationId: 'org-victim' }])

apps/sim/app/api/auth/sso/register/route.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,31 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
555555
headers,
556556
})
557557

558+
// Close the residual TOCTOU between the re-check above and Better Auth
559+
// persisting the provider: the verified sso_domain row could be removed in
560+
// that window. registerSSOProvider is create-only (it throws if the
561+
// providerId already exists), so a successful call ALWAYS created this exact
562+
// row — this compensating delete can never remove a pre-existing provider.
563+
// Scoped to (providerId, orgId) as defense-in-depth; personal SSO is not
564+
// gated, so this only runs for org-scoped registration.
565+
if (orgId && !(await isOrgDomainVerified())) {
566+
await db
567+
.delete(ssoProvider)
568+
.where(
569+
and(
570+
eq(ssoProvider.providerId, registration.providerId),
571+
eq(ssoProvider.organizationId, orgId)
572+
)
573+
)
574+
logger.warn('Rolled back SSO provider: domain verification revoked mid-registration', {
575+
domain,
576+
orgId,
577+
providerId: registration.providerId,
578+
userId: session.user.id,
579+
})
580+
return domainNotVerifiedResponse()
581+
}
582+
558583
logger.info('SSO provider registered successfully', {
559584
providerId,
560585
providerType,

apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,18 @@ describe('verify org domain route', () => {
9292
)
9393
})
9494

95+
it('is idempotent when a concurrent same-org request already verified the row', async () => {
96+
queueAdminWithPendingRow()
97+
queueTableRows(ssoDomain, []) // verified-elsewhere check → none
98+
dbChainMockFns.returning.mockResolvedValueOnce([]) // our conditional update lost the race
99+
queueTableRows(ssoDomain, [{ ...PENDING_ROW, status: 'verified' }]) // re-read: now verified
100+
const res = await POST(createMockRequest('POST'), routeContext)
101+
expect(res.status).toBe(200)
102+
const body = await res.json()
103+
expect(body.data.domain).toMatchObject({ status: 'verified' })
104+
expect(mockRecordAudit).not.toHaveBeenCalled() // the winning request recorded it
105+
})
106+
95107
it('409s when the row changed (deleted/re-tokenized) during the DNS lookup', async () => {
96108
queueAdminWithPendingRow()
97109
queueTableRows(ssoDomain, []) // verified-elsewhere check → none

apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,18 @@ export const POST = withRouteHandler(
125125
}
126126

127127
if (updated.length === 0) {
128+
// The conditional update matched nothing. If a concurrent request for this
129+
// same org already flipped the row to verified, treat this as an idempotent
130+
// success; otherwise the challenge is genuinely stale (row deleted or
131+
// re-tokenized mid-lookup) and we ask the caller to retry.
132+
const [current] = await db
133+
.select()
134+
.from(ssoDomain)
135+
.where(and(eq(ssoDomain.id, domainId), eq(ssoDomain.organizationId, organizationId)))
136+
.limit(1)
137+
if (current?.status === 'verified') {
138+
return NextResponse.json({ success: true, data: { domain: toDomainResponse(current) } })
139+
}
128140
return NextResponse.json(
129141
{ error: 'The domain changed during verification. Refresh and try again.' },
130142
{ status: 409 }

apps/sim/ee/sso/components/domain-settings.tsx

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ function DomainRow({ organizationId, domain, onRemove }: DomainRowProps) {
5151
}
5252

5353
return (
54-
<div className='flex flex-col gap-3 rounded-[10px] border border-[var(--border-1)] px-3 py-3'>
54+
<div className='flex flex-col gap-3 rounded-lg border border-[var(--border-1)] px-3 py-3'>
5555
<div className='flex items-center justify-between gap-2'>
5656
<span className='truncate text-[var(--text-body)] text-sm'>{domain.domain}</span>
5757
<div className='flex items-center gap-2'>
@@ -193,5 +193,3 @@ export function DomainSettings({ organizationId }: DomainSettingsProps) {
193193
</>
194194
)
195195
}
196-
197-
export default DomainSettings

packages/db/migrations/0266_sso_domain_verification.sql

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,26 @@ CREATE UNIQUE INDEX "sso_domain_org_domain_unique" ON "sso_domain" USING btree (
1818
CREATE UNIQUE INDEX "sso_domain_verified_unique" ON "sso_domain" USING btree ("domain") WHERE status = 'verified';--> statement-breakpoint
1919
-- Grandfather existing org-scoped SSO provider domains as verified: they were
2020
-- claimed under the old first-come model, so treat them as owned/verified to
21-
-- avoid breaking live SSO. DISTINCT ON keeps one verified row per domain
22-
-- (the partial unique index is global on verified rows); the token is a
23-
-- placeholder since these rows are already verified.
21+
-- avoid breaking live SSO. The normalized value must match normalizeSSODomain
22+
-- (the runtime gate's canonicalizer) so grandfathered lookups hit — hence
23+
-- lower + trim + strip a leading wildcard label; other artifacts (protocol,
24+
-- port, path) never occur in stored SSO domains. Computing it in a subquery
25+
-- keeps the DISTINCT ON dedup key identical to the inserted value, so the
26+
-- global partial unique index on verified rows can never be violated. The
27+
-- token is a placeholder since these rows are already verified.
2428
INSERT INTO "sso_domain" ("id", "organization_id", "domain", "status", "verification_token", "verified_at", "created_at", "updated_at")
25-
SELECT DISTINCT ON (lower("domain"))
29+
SELECT DISTINCT ON ("norm_domain")
2630
gen_random_uuid()::text,
2731
"organization_id",
28-
lower("domain"),
32+
"norm_domain",
2933
'verified',
3034
gen_random_uuid()::text,
3135
now(),
3236
now(),
3337
now()
34-
FROM "sso_provider"
35-
WHERE "organization_id" IS NOT NULL
36-
ORDER BY lower("domain"), "organization_id";
38+
FROM (
39+
SELECT "organization_id", lower(regexp_replace(btrim("domain"), '^\*\.', '')) AS "norm_domain"
40+
FROM "sso_provider"
41+
WHERE "organization_id" IS NOT NULL
42+
) AS "normalized"
43+
ORDER BY "norm_domain", "organization_id";

packages/db/scripts/register-sso-provider.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -653,7 +653,11 @@ async function registerSSOProvider(): Promise<boolean> {
653653
// verified record and the UI/API would treat it as unverified. Org-scoped
654654
// only — user-scoped providers have no org to attach a domain to.
655655
if (providerData.organizationId) {
656-
const normalizedDomain = ssoConfig.domain.trim().toLowerCase()
656+
// Match normalizeSSODomain's dominant transforms (lower + trim + strip a
657+
// leading wildcard) so this row lines up with the runtime gate's lookup.
658+
// normalizeSSODomain itself lives in apps/sim and can't be imported here
659+
// (packages never import apps); other artifacts don't occur in SSO_DOMAIN.
660+
const normalizedDomain = ssoConfig.domain.trim().toLowerCase().replace(/^\*\./, '')
657661
const existingDomain = await tx
658662
.select({ id: ssoDomain.id })
659663
.from(ssoDomain)

0 commit comments

Comments
 (0)