fix(sso): re-grant provider trust when an already-verified domain is re-submitted (#6320)

* fix(sso): re-grant provider trust when an already-verified domain is re-submitted

* fix(sso): distinguish a failed DNS lookup from a missing record, and label the domain fields

* chore(sso): tighten the re-grant rationale comment

* fix(sso): state what a failed DNS lookup tells us instead of assigning blame

Author: waleedlatif1

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

@@ -68,17 +68,32 @@ describe('verify org domain route', () => {
       user: { id: 'user-1', name: 'Admin', email: 'admin@acme.dev' },
     })
     mockIsEnterprise.mockResolvedValue(true)
-    mockCheckDomainTxtRecord.mockResolvedValue(true)
+    mockCheckDomainTxtRecord.mockResolvedValue('present')
   })
 
   it('422s when the TXT record is not found', async () => {
     queueAdminWithPendingRow()
-    mockCheckDomainTxtRecord.mockResolvedValue(false)
+    mockCheckDomainTxtRecord.mockResolvedValue('absent')
     const res = await POST(createMockRequest('POST'), routeContext)
     expect(res.status).toBe(422)
     expect(mockRecordAudit).not.toHaveBeenCalled()
   })
 
+  /**
+   * A failed lookup says nothing about the admin's DNS, so it must not be reported
+   * as a missing record — that sends them hunting through their zone for our fault.
+   */
+  it('503s (not 422) when the DNS lookup itself could not complete', async () => {
+    queueAdminWithPendingRow()
+    mockCheckDomainTxtRecord.mockResolvedValue('unavailable')
+    const res = await POST(createMockRequest('POST'), routeContext)
+    expect(res.status).toBe(503)
+    expect(await res.json()).toMatchObject({
+      error: expect.stringContaining("couldn't complete the DNS lookup"),
+    })
+    expect(mockRecordAudit).not.toHaveBeenCalled()
+  })
+
   it('verifies the domain and records an audit event', async () => {
     queueAdminWithPendingRow()
     queueTableRows(ssoDomain, []) // verified-elsewhere check → none
@@ -143,12 +158,29 @@ describe('verify org domain route', () => {
     expect(grantWhere).toBeDefined()
   })
 
-  it('does not grant trust when the conditional update matched no row', async () => {
+  /**
+   * A provider can hold a verified domain while its own trust flag is off, after
+   * an update whose grant was refused reverted the config and cleared it. Re-running
+   * verification is the obvious recovery, so an already-verified domain must still
+   * re-grant instead of returning success having done nothing.
+   */
+  it('re-grants trust when the domain is already verified', async () => {
+    queueAdminWithPendingRow()
+    queueTableRows(ssoDomain, [])
+    dbChainMockFns.returning.mockResolvedValueOnce([]) // conditional update matched nothing
+    queueTableRows(ssoDomain, [{ ...PENDING_ROW, status: 'verified' }]) // re-read: verified
+    const res = await POST(createMockRequest('POST'), routeContext)
+    expect(res.status).toBe(200)
+    expect(dbChainMockFns.set).toHaveBeenCalledWith({ domainVerified: true })
+  })
+
+  it('does not grant trust when the challenge is genuinely stale', async () => {
     queueAdminWithPendingRow()
     queueTableRows(ssoDomain, [])
-    dbChainMockFns.returning.mockResolvedValueOnce([]) // lost the race
-    queueTableRows(ssoDomain, [{ ...PENDING_ROW, status: 'verified' }])
-    await POST(createMockRequest('POST'), routeContext)
+    dbChainMockFns.returning.mockResolvedValueOnce([]) // conditional update matched nothing
+    queueTableRows(ssoDomain, []) // re-read: row deleted or re-tokenized
+    const res = await POST(createMockRequest('POST'), routeContext)
+    expect(res.status).toBe(409)
     expect(dbChainMockFns.set).not.toHaveBeenCalledWith({ domainVerified: true })
   })
 

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

@@ -70,8 +70,20 @@ export const POST = withRouteHandler(
       return NextResponse.json({ success: true, data: { domain: toDomainResponse(row) } })
     }
 
-    const recordPresent = await checkDomainTxtRecord(row.domain, row.verificationToken)
-    if (!recordPresent) {
+    const lookup = await checkDomainTxtRecord(row.domain, row.verificationToken)
+    // 503, not 422: we learned nothing about their record, so this must not read
+    // as a missing one. SERVFAIL can mean either a fault of ours or a broken zone
+    // of theirs, so the message states what we know rather than assigning blame.
+    if (lookup === 'unavailable') {
+      return NextResponse.json(
+        {
+          error:
+            "We couldn't complete the DNS lookup, so we can't tell yet whether your record is published. Try again in a few minutes — if it keeps failing, check that your domain's nameservers are responding.",
+        },
+        { status: 503 }
+      )
+    }
+    if (lookup === 'absent') {
       return NextResponse.json(
         {
           error:
@@ -101,6 +113,17 @@ export const POST = withRouteHandler(
     // instead of mapping an undefined row or trusting a superseded challenge. A
     // concurrent cross-org verification trips the partial unique index; surface
     // that as a 409 rather than an unhandled 500.
+    /**
+     * Providers this proof covers. Normalized the way migration 0268 stored these
+     * rows (lower, trimmed, leading `*.` dropped) and identical to the expression
+     * the deletion path revokes with, so granting and revoking can never diverge.
+     */
+    const providersOnDomain = (verifiedDomain: string) =>
+      and(
+        eq(ssoProvider.organizationId, organizationId),
+        sql`lower(regexp_replace(btrim(${ssoProvider.domain}), '^\\*\\.', '')) = ${verifiedDomain}`
+      )
+
     let updated: (typeof row)[]
     try {
       updated = await db.transaction(async (tx) => {
@@ -118,18 +141,12 @@ export const POST = withRouteHandler(
 
         // Restore trust this proof covers, mirroring the revocation on delete.
         // Without it a delete-then-reverify leaves the provider untrusted, and
-        // since that flag gates sign-in the org sits in a silent SSO outage. The
-        // comparison matches the revoking one exactly so the two stay symmetric.
+        // since that flag gates sign-in the org sits in a silent SSO outage.
         if (flipped.length > 0) {
           await tx
             .update(ssoProvider)
             .set({ domainVerified: true })
-            .where(
-              and(
-                eq(ssoProvider.organizationId, organizationId),
-                sql`lower(regexp_replace(btrim(${ssoProvider.domain}), '^\\*\\.', '')) = ${flipped[0].domain}`
-              )
-            )
+            .where(providersOnDomain(flipped[0].domain))
         }
 
         return flipped
@@ -155,6 +172,13 @@ export const POST = withRouteHandler(
         .where(and(eq(ssoDomain.id, domainId), eq(ssoDomain.organizationId, organizationId)))
         .limit(1)
       if (current?.status === 'verified') {
+        // Re-grant rather than returning early: a provider can hold a verified
+        // domain with its own flag off, after an update whose grant was refused
+        // reverted the config. The proof is present, which authorizes this.
+        await db
+          .update(ssoProvider)
+          .set({ domainVerified: true })
+          .where(providersOnDomain(current.domain))
         return NextResponse.json({ success: true, data: { domain: toDomainResponse(current) } })
       }
       return NextResponse.json(

apps/sim/ee/sso/components/verified-domains-section.tsx

@@ -17,6 +17,9 @@ import {
   useVerifyOrganizationDomain,
 } from '@/ee/sso/hooks/domains'
 
+/** Ties the "Add a domain" label to its input, so clicking the label focuses it. */
+const ADD_DOMAIN_FIELD_ID = 'sso-add-domain'
+
 interface VerifiedDomainsSectionProps {
   organizationId: string
 }
@@ -65,16 +68,19 @@ function DomainRow({ organizationId, domain, onRemove }: DomainRowProps) {
           <SettingRow
             label='Host / name'
             description='Some DNS providers append your zone automatically. If yours does, enter this host with the trailing zone removed.'
+            htmlFor={`${domain.id}-challenge-host`}
           >
             <ChipCopyInput
+              id={`${domain.id}-challenge-host`}
               value={domain.challengeHost}
               copyLabel='Copy host'
               inputClassName='font-mono'
             />
           </SettingRow>
 
-          <SettingRow label='Value'>
+          <SettingRow label='Value' htmlFor={`${domain.id}-challenge-value`}>
             <ChipCopyInput
+              id={`${domain.id}-challenge-value`}
               value={domain.txtRecordValue}
               copyLabel='Copy value'
               inputClassName='font-mono'
@@ -139,9 +145,11 @@ export function VerifiedDomainsSection({ organizationId }: VerifiedDomainsSectio
           <SettingRow
             label='Add a domain'
             description='Verify a domain your organization owns before configuring SSO for it. Verifying proves you control the domain, so no one else can point it at their identity provider.'
+            htmlFor={ADD_DOMAIN_FIELD_ID}
           >
             <div className='flex items-center gap-2'>
               <ChipInput
+                id={ADD_DOMAIN_FIELD_ID}
                 value={newDomain}
                 onChange={(event) => setNewDomain(event.target.value)}
                 onKeyDown={(event) => {

apps/sim/lib/auth/sso/domain-verification.test.ts

@@ -107,13 +107,13 @@ describe('domain-verification helpers', () => {
 
     it('verifies when the exact value is published', async () => {
       mockResolveTxt.mockResolvedValue([[EXPECTED]])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(true)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('present')
     })
 
     it('joins a value split across 255-char chunks before comparing', async () => {
       const midpoint = Math.floor(EXPECTED.length / 2)
       mockResolveTxt.mockResolvedValue([[EXPECTED.slice(0, midpoint), EXPECTED.slice(midpoint)]])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(true)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('present')
     })
 
     it('finds the match among unrelated TXT records on the same host', async () => {
@@ -122,37 +122,41 @@ describe('domain-verification helpers', () => {
         ['facebook-domain-verification=abc123'],
         [EXPECTED],
       ])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(true)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('present')
     })
 
     it('tolerates padding a DNS panel added around the value', async () => {
       mockResolveTxt.mockResolvedValue([[`  ${EXPECTED}  `]])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(true)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('present')
     })
 
     it('rejects a near-miss value (no partial or prefix match)', async () => {
       mockResolveTxt.mockResolvedValue([[`${EXPECTED}extra`], [EXPECTED.slice(0, -1)]])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(false)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('absent')
     })
 
     it('rejects another org token published on the same host', async () => {
       mockResolveTxt.mockResolvedValue([[buildTxtRecordValue('someone-elses-token')]])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(false)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('absent')
     })
 
-    it('returns false (never throws) when the record is absent', async () => {
+    it('reports absent (never throws) when the record is not published', async () => {
       mockResolveTxt.mockRejectedValue(Object.assign(new Error('no data'), { code: 'ENODATA' }))
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(false)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('absent')
     })
 
-    it('returns false (never throws) when resolution fails for an infrastructure reason', async () => {
+    /**
+     * Distinct from `absent`: our resolver failed, so we learned nothing about the
+     * admin's DNS and must not tell them their record is missing.
+     */
+    it('reports unavailable when resolution fails for an infrastructure reason', async () => {
       mockResolveTxt.mockRejectedValue(Object.assign(new Error('timeout'), { code: 'ETIMEOUT' }))
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(false)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('unavailable')
     })
 
-    it('returns false when the host has no TXT records at all', async () => {
+    it('reports absent when the host has no TXT records at all', async () => {
       mockResolveTxt.mockResolvedValue([])
-      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe(false)
+      await expect(checkDomainTxtRecord('acme.com', TOKEN)).resolves.toBe('absent')
     })
   })
 })

apps/sim/lib/auth/sso/domain-verification.ts

@@ -98,13 +98,25 @@ export function generateVerificationToken(): string {
 }
 
 /**
- * Resolves the challenge host's TXT records against public nameservers and
- * returns true when the expected `sim-domain-verification=<token>` value is
- * present. Never throws — resolution failures (NXDOMAIN, timeout, missing
- * record) resolve to `false` so a not-yet-propagated record simply reads as
- * unverified.
+ * Outcome of a TXT challenge lookup.
+ *
+ * `absent` and `unavailable` are kept apart because they place the fault on
+ * opposite sides: the first means the admin's record is not published yet, the
+ * second means our own resolver path failed and we learned nothing about their
+ * DNS. Collapsing both to "not found" tells an admin to fix a record that may
+ * already be correct.
  */
-export async function checkDomainTxtRecord(domain: string, token: string): Promise<boolean> {
+export type DomainTxtLookup = 'present' | 'absent' | 'unavailable'
+
+/**
+ * Resolves the challenge host's TXT records against public nameservers. Never
+ * throws: a missing record resolves to `absent`, and an infrastructure failure
+ * (blocked egress, timeout, SERVFAIL) to `unavailable`.
+ */
+export async function checkDomainTxtRecord(
+  domain: string,
+  token: string
+): Promise<DomainTxtLookup> {
   const host = buildChallengeHost(domain)
   const expected = buildTxtRecordValue(token)
 
@@ -115,24 +127,20 @@ export async function checkDomainTxtRecord(domain: string, token: string): Promi
     // would otherwise fail an exact match forever with no way for the admin to
     // tell why. Concatenation happens first, so trimming cannot corrupt a
     // legitimate chunk boundary.
-    return records.some((chunks) => chunks.join('').trim() === expected)
+    return records.some((chunks) => chunks.join('').trim() === expected) ? 'present' : 'absent'
   } catch (error) {
     const code = (error as NodeJS.ErrnoException)?.code
     if (code && RECORD_ABSENT_DNS_CODES.has(code)) {
       logger.debug('TXT verification record not published yet', { host, code })
-    } else {
-      // Not a missing record — our resolver path itself is failing (blocked
-      // egress, timeout, SERVFAIL). Log at ERROR, not warn: the default minimum
-      // level in production is ERROR, so anything below it is dropped and the
-      // fault stays invisible while the admin is told their record "isn't
-      // published yet". This is a genuine infrastructure fault, so ERROR is also
-      // the honest severity.
-      logger.error('TXT verification lookup failed for an infrastructure reason', {
-        host,
-        code,
-        error: getErrorMessage(error),
-      })
+      return 'absent'
     }
-    return false
+    // Our resolver path itself is failing. Log at ERROR: production's minimum
+    // level drops anything lower, so a warn would keep the fault invisible.
+    logger.error('TXT verification lookup failed for an infrastructure reason', {
+      host,
+      code,
+      error: getErrorMessage(error),
+    })
+    return 'unavailable'
   }
 }