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..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 @@ -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 }) }) 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..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 @@ -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( 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' } }