fix(sso): hold the domain proof under a row lock while granting trust

Two fixes from review.

The trust grant folded the ownership test into the UPDATE's WHERE clause, but
under READ COMMITTED the EXISTS subquery is evaluated against the statement's
original snapshot. A delete committing while the UPDATE waited on the provider
row could therefore still see the removed sso_domain row and grant trust after
ownership was gone. The grant now selects the proof FOR SHARE inside a
transaction before writing, so the delete blocks until it commits, and if the
delete committed first the select finds nothing and no trust is written.

Editing a SAML provider also broke on configs written by the previous commit:
hydration used `config.idpMetadata?.metadata || config.idpMetadata`, and
`{ metadata: '' }` is falsy at the property but truthy as an object, so an object
landed in a string field and failed validation on save. It now narrows on the
type and handles both the object and legacy bare-string shapes.

Author: waleedlatif1

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

@@ -115,16 +115,14 @@ describe('POST /api/auth/sso/register', () => {
     mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '1.2.3.4' })
     mockSecureFetchWithPinnedIP.mockRejectedValue(new Error('discovery not mocked for this test'))
     mockRegisterSSOProvider.mockResolvedValue({ id: 'row-1', providerId: 'acme-oidc' })
-    // The conditional trust UPDATE returns its row by default, i.e. the verified
-    // domain still existed at write time. Refusal tests override with [].
-    dbChainMockFns.returning.mockResolvedValue([{ id: 'granted' }])
     mockUpdateSSOProvider.mockResolvedValue({ providerId: 'acme-oidc' })
     // Default: the org has already verified the domain, so the ownership gate
-    // passes and each test exercises the logic beyond it. The gate is read twice
-    // for a successful org-scoped registration (fail-fast entry + authoritative
-    // re-check before the write); ownership at write time is re-tested inside the
-    // trust UPDATE itself, not by a third read. Gate-specific tests reset the
-    // queue to assert the unverified paths.
+    // passes and each test exercises the logic beyond it. A successful org-scoped
+    // registration reads it three times: the fail-fast entry gate, the
+    // authoritative re-check before the write, and the locking read inside the
+    // trust transaction. Gate-specific tests reset the queue to assert the
+    // unverified paths.
+    queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
     queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
     queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
   })
@@ -184,7 +182,7 @@ describe('POST /api/auth/sso/register', () => {
     queueMembers([{ organizationId: 'org1', role: 'owner' }])
     queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // entry gate: verified
     queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // pre-write re-check: verified
-    dbChainMockFns.returning.mockResolvedValue([]) // trust UPDATE matched nothing: revoked
+    queueTableRows(schemaMock.ssoDomain, []) // locking read in the grant: proof gone
     const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
     const json = await res.json()
     expect(res.status).toBe(403)
@@ -285,9 +283,13 @@ describe('POST /api/auth/sso/register', () => {
    */
   it('revokes domain trust when verification is removed during an update', async () => {
     queueMembers([{ organizationId: 'org1', role: 'owner' }])
+    resetDbChainMock()
+    queueMembers([{ organizationId: 'org1', role: 'owner' }])
+    queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // entry gate
+    queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // pre-write re-check
+    queueTableRows(schemaMock.ssoDomain, []) // locking read in the grant: proof gone
     queueProviders([])
     queueTableRows(schemaMock.ssoProvider, [{ id: 'p1' }]) // provider already owned → update path
-    dbChainMockFns.returning.mockResolvedValue([]) // trust UPDATE matched nothing
 
     const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
     expect(res.status).toBe(403)
@@ -304,7 +306,7 @@ describe('POST /api/auth/sso/register', () => {
     queueMembers([{ organizationId: 'org1', role: 'owner' }])
     queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
     queueTableRows(schemaMock.ssoDomain, [{ id: 'verified-domain' }])
-    dbChainMockFns.returning.mockResolvedValue([]) // trust UPDATE matched nothing
+    queueTableRows(schemaMock.ssoDomain, []) // locking read in the grant: proof gone
     const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
     expect(res.status).toBe(403)
     expect(mockRegisterSSOProvider).toHaveBeenCalledTimes(1) // it was created…

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

@@ -2,7 +2,7 @@ import { db, member, ssoDomain, ssoProvider } from '@sim/db'
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
 import { normalizeSSODomain } from '@sim/utils/sso-domain'
-import { and, eq, exists, isNull, sql } from 'drizzle-orm'
+import { and, eq, isNull, sql } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import { ssoRegistrationContract } from '@/lib/api/contracts/auth'
 import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
@@ -647,11 +647,16 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
     }
 
     /**
-     * Grants domain trust with the ownership test folded into the UPDATE's WHERE
-     * clause, so the write matches nothing once the proof is gone and reports that
-     * as `false`. Paired with the domain-delete route clearing this flag in the
-     * same transaction that removes the proof, the provider cannot end up trusted
-     * without current ownership in either commit order.
+     * Grants domain trust only while the proof is held under a row lock.
+     *
+     * Folding the ownership test into the UPDATE's WHERE clause is not sufficient:
+     * under READ COMMITTED the EXISTS subquery is evaluated against the statement's
+     * original snapshot, so a delete committing while the UPDATE waits on the
+     * provider row can still leave the subquery seeing the removed sso_domain row —
+     * granting trust after ownership is gone. Taking `FOR SHARE` on that row inside
+     * a transaction makes the two operations order properly: the delete's removal of
+     * sso_domain blocks until this commits, and if it committed first the SELECT
+     * finds nothing and no trust is written.
      *
      * Org-less (personal) SSO is a self-host-only path — Sim's UI always registers
      * org-scoped. It has no verified domain behind it, so it is trusted only when
@@ -663,28 +668,24 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
         await setProviderDomainVerified(!isHosted)
         return true
       }
-      const granted = await db
-        .update(ssoProvider)
-        .set({ domainVerified: true })
-        .where(
-          and(
-            ownerClause,
-            exists(
-              db
-                .select({ one: sql`1` })
-                .from(ssoDomain)
-                .where(
-                  and(
-                    eq(ssoDomain.organizationId, orgId),
-                    eq(ssoDomain.domain, domain),
-                    eq(ssoDomain.status, 'verified')
-                  )
-                )
+      return db.transaction(async (tx) => {
+        const [proof] = await tx
+          .select({ id: ssoDomain.id })
+          .from(ssoDomain)
+          .where(
+            and(
+              eq(ssoDomain.organizationId, orgId),
+              eq(ssoDomain.domain, domain),
+              eq(ssoDomain.status, 'verified')
             )
           )
-        )
-        .returning({ id: ssoProvider.id })
-      return granted.length > 0
+          .limit(1)
+          .for('share')
+        if (!proof) return false
+
+        await tx.update(ssoProvider).set({ domainVerified: true }).where(ownerClause)
+        return true
+      })
     }
 
     if (existingOwnedProvider) {

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

@@ -387,7 +387,14 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
         callbackUrl = config.callbackUrl || ''
         audience = config.audience || ''
         wantAssertionsSigned = config.wantAssertionsSigned ?? true
-        idpMetadata = config.idpMetadata?.metadata || config.idpMetadata || ''
+        // Two stored shapes: `{ metadata }` from the route, and a bare string from
+        // older rows. Narrow on the type rather than truthiness — `{ metadata: '' }`
+        // is falsy at `.metadata` but truthy as an object, which would put an object
+        // into this string field and fail validation on the next save.
+        idpMetadata =
+          typeof config.idpMetadata === 'string'
+            ? config.idpMetadata
+            : (config.idpMetadata?.metadata ?? '')
         mapping = config.mapping ?? {}
         identifierFormat = config.identifierFormat || ''
       }