From 775b120ad5336b4c82319d51dd6b8985acb81f82 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 01:15:39 -0700 Subject: [PATCH 1/4] fix(sso): re-grant provider trust when an already-verified domain is re-submitted --- .../domains/[domainId]/verify/route.test.ts | 25 +++++++++++++--- .../[id]/domains/[domainId]/verify/route.ts | 30 ++++++++++++++----- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts index 7e8dda9dd06..775b4817ead 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts @@ -143,12 +143,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([]) // lost the race - queueTableRows(ssoDomain, [{ ...PENDING_ROW, status: 'verified' }]) - await POST(createMockRequest('POST'), routeContext) + 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([]) // 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 }) }) diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts index 6142de1afb1..e52fb541409 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts @@ -101,6 +101,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 +129,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 +160,15 @@ 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 while its own trust flag is off — an update whose grant was + // refused reverts to the previous config and clears it. Re-running + // verification is the obvious way to fix that, so it must actually do + // something; the proof is present, which is exactly what 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( From 97161ec1653b32da42a8bf6287511a6e1010fd2f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 01:21:36 -0700 Subject: [PATCH 2/4] fix(sso): distinguish a failed DNS lookup from a missing record, and label the domain fields --- .../domains/[domainId]/verify/route.test.ts | 17 ++++++- .../[id]/domains/[domainId]/verify/route.ts | 15 +++++- .../components/verified-domains-section.tsx | 10 +++- .../lib/auth/sso/domain-verification.test.ts | 28 ++++++----- apps/sim/lib/auth/sso/domain-verification.ts | 48 +++++++++++-------- 5 files changed, 81 insertions(+), 37 deletions(-) diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts index 775b4817ead..73ee4ac4a73 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts @@ -68,17 +68,30 @@ 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('on our side') }) + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + it('verifies the domain and records an audit event', async () => { queueAdminWithPendingRow() queueTableRows(ssoDomain, []) // verified-elsewhere check → none diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts index e52fb541409..e198a1b6e2d 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts @@ -70,8 +70,19 @@ 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: the record may well be correct, so this must not read as the + // admin's mistake or they will go hunting through DNS for a fault that is ours. + if (lookup === 'unavailable') { + return NextResponse.json( + { + error: + "We couldn't complete the DNS lookup — this is a problem on our side, not with your record. Try again in a few minutes.", + }, + { status: 503 } + ) + } + if (lookup === 'absent') { return NextResponse.json( { error: diff --git a/apps/sim/ee/sso/components/verified-domains-section.tsx b/apps/sim/ee/sso/components/verified-domains-section.tsx index a4e44f08bbc..011115fe8d3 100644 --- a/apps/sim/ee/sso/components/verified-domains-section.tsx +++ b/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) { - +
setNewDomain(event.target.value)} onKeyDown={(event) => { diff --git a/apps/sim/lib/auth/sso/domain-verification.test.ts b/apps/sim/lib/auth/sso/domain-verification.test.ts index e44dfcc8f3f..842a1b72609 100644 --- a/apps/sim/lib/auth/sso/domain-verification.test.ts +++ b/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') }) }) }) diff --git a/apps/sim/lib/auth/sso/domain-verification.ts b/apps/sim/lib/auth/sso/domain-verification.ts index 79e8d1ef575..5e1efb63d16 100644 --- a/apps/sim/lib/auth/sso/domain-verification.ts +++ b/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=` 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 { +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 { 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' } } From f5a3de6b6a104b8328ccfddb0d0691f4c6c1fcfd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 01:24:38 -0700 Subject: [PATCH 3/4] chore(sso): tighten the re-grant rationale comment --- .../organizations/[id]/domains/[domainId]/verify/route.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts index e198a1b6e2d..3f43cca2aa0 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts @@ -171,11 +171,9 @@ 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 while its own trust flag is off — an update whose grant was - // refused reverts to the previous config and clears it. Re-running - // verification is the obvious way to fix that, so it must actually do - // something; the proof is present, which is exactly what authorizes this. + // 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 }) From 45ec9a825321d122fbf9c54d6f8147601a1d8e99 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 6 Aug 2026 01:27:07 -0700 Subject: [PATCH 4/4] fix(sso): state what a failed DNS lookup tells us instead of assigning blame --- .../[id]/domains/[domainId]/verify/route.test.ts | 4 +++- .../organizations/[id]/domains/[domainId]/verify/route.ts | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts index 73ee4ac4a73..4a7b76741d2 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.test.ts @@ -88,7 +88,9 @@ describe('verify org domain route', () => { mockCheckDomainTxtRecord.mockResolvedValue('unavailable') const res = await POST(createMockRequest('POST'), routeContext) expect(res.status).toBe(503) - expect(await res.json()).toMatchObject({ error: expect.stringContaining('on our side') }) + expect(await res.json()).toMatchObject({ + error: expect.stringContaining("couldn't complete the DNS lookup"), + }) expect(mockRecordAudit).not.toHaveBeenCalled() }) diff --git a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts index 3f43cca2aa0..f48cd7c8510 100644 --- a/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts +++ b/apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts @@ -71,13 +71,14 @@ export const POST = withRouteHandler( } const lookup = await checkDomainTxtRecord(row.domain, row.verificationToken) - // 503, not 422: the record may well be correct, so this must not read as the - // admin's mistake or they will go hunting through DNS for a fault that is ours. + // 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 — this is a problem on our side, not with your record. Try again in a few minutes.", + "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 } )