Skip to content

Commit 90f7f8c

Browse files
authored
fix(credentials): send a conflicting OAuth reconnect back with a reason instead of a 500 (#9391)
* fix(credentials): send a conflicting OAuth reconnect back with a reason instead of a 500 * fix(credentials): keep the refused grant's tokens on the identity's existing rows * fix(credentials): keep stored scope and resume connectors on a refused reconnect
1 parent 5b85be3 commit 90f7f8c

8 files changed

Lines changed: 635 additions & 76 deletions

File tree

‎apps/sim/hooks/use-oauth-return.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,28 @@ describe('resolveOAuthCallbackError', () => {
121121
text: 'The "New Gmail" connection didn’t finish. Try again.',
122122
})
123123
})
124+
125+
it('names the credential that already uses the account a reconnect chose', () => {
126+
const reason = 'That Google account is already connected in this workspace as "Personal".'
127+
const url = new URL('https://sim.ai/workspace/workspace-1/integrations')
128+
url.searchParams.set('error', 'credential_account_conflict')
129+
url.searchParams.set('error_description', reason)
130+
131+
expect(resolveOAuthCallbackError(url.toString(), context)).toEqual({
132+
kind: 'error',
133+
text: reason,
134+
})
135+
})
136+
137+
it('keeps a provider-supplied error description out of the message', () => {
138+
const url = new URL('https://sim.ai/workspace/workspace-1/integrations')
139+
url.searchParams.set('error', 'access_denied')
140+
url.searchParams.set('error_description', 'Visit example.test to restore access')
141+
142+
expect(resolveOAuthCallbackError(url.toString(), context)?.text).toBe(
143+
'The "New Gmail" connection didn’t finish. Try again.'
144+
)
145+
})
124146
})
125147

126148
describe('buildKnowledgeBaseOAuthReturnUrl', () => {

‎apps/sim/hooks/use-oauth-return.ts‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
type OAuthReturnContext,
1818
readOAuthReturnContext,
1919
} from '@/lib/credentials/client-state'
20+
import { OAUTH_CREDENTIAL_ACCOUNT_CONFLICT_ERROR } from '@/lib/credentials/draft-constants'
2021
import {
2122
hasOAuthCredentialChanged,
2223
OAUTH_CHAT_ATTEMPT_MAX_AGE_MS,
@@ -124,13 +125,26 @@ export function resolveOAuthCallbackError(
124125
callbackUrl: string,
125126
ctx: Pick<OAuthReturnContext, 'displayName'>
126127
): OAuthResultMessage | null {
127-
if (!new URL(callbackUrl).searchParams.has('error')) return null
128+
const params = new URL(callbackUrl).searchParams
129+
if (!params.has('error')) return null
128130
return {
129131
kind: 'error',
130-
text: `The "${ctx.displayName}" connection didn’t finish. Try again.`,
132+
text:
133+
accountConflictDescription(params) ??
134+
`The "${ctx.displayName}" connection didn’t finish. Try again.`,
131135
}
132136
}
133137

138+
/**
139+
* The server-written reason a reconnect was refused because the chosen account
140+
* already backs another credential. Other `error_description` values come from
141+
* the provider and stay hidden behind the generic message.
142+
*/
143+
function accountConflictDescription(params: URLSearchParams): string | null {
144+
if (params.get('error') !== OAUTH_CREDENTIAL_ACCOUNT_CONFLICT_ERROR) return null
145+
return params.get('error_description') || null
146+
}
147+
134148
function consumeOAuthCallbackError(ctx: OAuthReturnContext): OAuthResultMessage | null {
135149
const result = resolveOAuthCallbackError(window.location.href, ctx)
136150
if (!result) return null
@@ -221,9 +235,13 @@ async function verifyOAuthChatAttempt(queryClient: QueryClient, attemptId: strin
221235
const attempt = readOAuthChatAttempt(attemptId)
222236
if (!attempt || Date.now() - attempt.requestedAt > OAUTH_CHAT_ATTEMPT_MAX_AGE_MS) return
223237

224-
if (new URL(window.location.href).searchParams.has('error')) {
238+
const callbackParams = new URL(window.location.href).searchParams
239+
if (callbackParams.has('error')) {
225240
setOAuthChatAttemptStatus(attempt.id, 'failed')
226-
toast.error(`The ${attempt.displayName} connection didn’t finish. Try again.`)
241+
toast.error(
242+
accountConflictDescription(callbackParams) ??
243+
`The ${attempt.displayName} connection didn’t finish. Try again.`
244+
)
227245
return
228246
}
229247

‎apps/sim/lib/auth/auth.ts‎

Lines changed: 85 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import { reportFreebuffConversion } from '@/lib/analytics/freebuff.server'
4040
import { getAccessControlConfig, isEmailBlockedByAccessControl } from '@/lib/auth/access-control'
4141
import { holdAdminRemovalStops, stopAdminRemovedRuns } from '@/lib/auth/admin-removal-stops'
4242
import { createAnonymousSession, ensureAnonymousUserExists } from '@/lib/auth/anonymous'
43+
import { stableExternalAccountId } from '@/lib/auth/connectors/account-id'
4344
import { buildConnectorProviders } from '@/lib/auth/connectors/providers'
4445
import {
4546
applyRegistrationGate,
@@ -142,10 +143,12 @@ import { validateCallbackUrl } from '@/lib/core/security/input-validation'
142143
import { PlatformEvents } from '@/lib/core/telemetry'
143144
import { trustedProxies } from '@/lib/core/utils/request'
144145
import { getBaseUrl, isLocalhostUrl, parseOriginList } from '@/lib/core/utils/urls'
146+
import { CredentialAccountConflictError } from '@/lib/credentials/draft-hooks'
145147
import {
146148
captureOAuthCredentialDraftBinding,
147149
consumeOAuthCredentialDraftBinding,
148150
processCredentialDraft,
151+
refuseConflictingCredentialReconnect,
149152
} from '@/lib/credentials/draft-processor'
150153
import { sendEmail } from '@/lib/messaging/email/mailer'
151154
import { getFromEmailAddress, getPersonalEmailFrom } from '@/lib/messaging/email/utils'
@@ -294,6 +297,40 @@ async function fetchSalesforceInstanceUrl(
294297
}
295298
}
296299

300+
/**
301+
* A fresh Slack connect re-issues the installation's rotating token chain,
302+
* invalidating the copies held by sibling account rows for the same team (Slack
303+
* bot tokens are per-installation, not per-grant). Propagates the new chain so
304+
* every sibling is valid again, and clears the installation's dead flag. A
305+
* failure is logged, never thrown: the connect itself already succeeded.
306+
*/
307+
async function propagateFreshSlackConnect(account: {
308+
userId: string
309+
providerId: string
310+
accountId: string
311+
accessToken?: string | null
312+
refreshToken?: string | null
313+
accessTokenExpiresAt?: Date | null
314+
}): Promise<void> {
315+
if (account.providerId !== 'slack' || !account.accessToken) return
316+
try {
317+
const teamId = extractSlackTeamId(account.accountId)
318+
if (!teamId) return
319+
// Clear the dead flag before fanning out: the connect itself proves the
320+
// installation has live tokens, and a fan-out failure must not leave the
321+
// hour-long flag blocking refreshes.
322+
await clearOAuthRefreshDeadFlag(`slack:${teamId}`)
323+
await fanOutSlackTokenChain(teamId, {
324+
accessToken: account.accessToken,
325+
refreshToken: account.refreshToken ?? null,
326+
accessTokenExpiresAt: account.accessTokenExpiresAt ?? null,
327+
})
328+
logger.info('Propagated Slack installation token chain', { userId: account.userId, teamId })
329+
} catch (error) {
330+
logger.error('Failed to propagate Slack token chain', { userId: account.userId, error })
331+
}
332+
}
333+
297334
export const auth = betterAuth({
298335
baseURL: getBaseUrl(),
299336
// Where Better Auth sends OAuth callbacks that fail before the flow state is
@@ -500,10 +537,12 @@ export const auth = betterAuth({
500537
create: {
501538
before: async (account, context) => {
502539
const modifiedAccount = { ...account }
540+
let reconnectDraftId: string | undefined
503541

504542
if (context?.path.startsWith('/oauth2/callback/')) {
543+
let draftId: string | undefined
505544
try {
506-
await captureOAuthCredentialDraftBinding(context, () => getOAuthState())
545+
draftId = await captureOAuthCredentialDraftBinding(context, () => getOAuthState())
507546
} catch (error) {
508547
logger.error('[account.create.before] Failed to read OAuth credential draft state', {
509548
userId: account.userId,
@@ -512,6 +551,7 @@ export const auth = betterAuth({
512551
})
513552
throw error
514553
}
554+
reconnectDraftId = draftId
515555
}
516556

517557
if (account.accessToken && isSalesforceOAuthProviderId(account.providerId)) {
@@ -538,25 +578,42 @@ export const auth = betterAuth({
538578
}
539579
}
540580

581+
/**
582+
* The after-hook runs once the callback has already answered, so a
583+
* reconnect onto an account another credential in the workspace uses
584+
* has to be refused here, before the row exists, to send the user
585+
* back with a reason instead of a 500. The fresh grant still lands
586+
* on the identity's existing rows: the provider may already have
587+
* rotated their tokens away (Slack installations do).
588+
*/
589+
if (context && reconnectDraftId) {
590+
const conflictRedirectUrl = await refuseConflictingCredentialReconnect({
591+
draftId: reconnectDraftId,
592+
account: modifiedAccount,
593+
oauthState: await getOAuthState(),
594+
})
595+
if (conflictRedirectUrl) {
596+
consumeOAuthCredentialDraftBinding(context)
597+
await propagateFreshSlackConnect(modifiedAccount)
598+
throw new APIError('FOUND', undefined, { location: conflictRedirectUrl })
599+
}
600+
}
601+
541602
return { data: modifiedAccount }
542603
},
543604
after: async (account, context) => {
544605
/**
545606
* Migrate credentials from stale account rows to the newly created one.
546607
*
547-
* Each `getUserInfo` in `lib/auth/connectors/providers.ts` appends a
548-
* random UUID to the stable external ID so that Better Auth never
549-
* blocks cross-user connections — keep the two in step. This means
550-
* re-connecting the same external identity creates a new row. We detect
551-
* the stale siblings here by comparing the stable prefix (everything
552-
* before the trailing UUID), migrate any credential FKs to the new row,
553-
* then delete the stale rows.
608+
* Re-connecting the same external identity creates a new row (see
609+
* `stableExternalAccountId`). We detect the stale siblings here by
610+
* comparing the stable prefix, migrate any credential FKs to the new
611+
* row, then delete the stale rows.
554612
*/
555613
try {
556-
const UUID_SUFFIX_RE = /-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/
557-
const stablePrefix = account.accountId.replace(UUID_SUFFIX_RE, '')
614+
const stablePrefix = stableExternalAccountId(account.accountId)
558615

559-
if (stablePrefix && stablePrefix !== account.accountId) {
616+
if (stablePrefix) {
560617
const siblings = await db
561618
.select({ id: schema.account.id, accountId: schema.account.accountId })
562619
.from(schema.account)
@@ -569,7 +626,7 @@ export const auth = betterAuth({
569626
)
570627

571628
const staleRows = siblings.filter(
572-
(row) => row.accountId.replace(UUID_SUFFIX_RE, '') === stablePrefix
629+
(row) => stableExternalAccountId(row.accountId) === stablePrefix
573630
)
574631

575632
if (staleRows.length > 0) {
@@ -598,40 +655,7 @@ export const auth = betterAuth({
598655
})
599656
}
600657

601-
/**
602-
* A fresh Slack connect re-issues the installation's rotating token
603-
* chain, invalidating the copies held by sibling account rows for the
604-
* same team (Slack bot tokens are per-installation, not per-grant).
605-
* Propagate the new chain so every sibling is valid again, and clear
606-
* the installation's dead flag.
607-
*/
608-
if (account.providerId === 'slack' && account.accessToken) {
609-
try {
610-
const teamId = extractSlackTeamId(account.accountId)
611-
if (teamId) {
612-
// Clear the dead flag before fanning out: the connect itself
613-
// proves the installation has live tokens, and a fan-out
614-
// failure must not leave the hour-long flag blocking refreshes.
615-
await clearOAuthRefreshDeadFlag(`slack:${teamId}`)
616-
await fanOutSlackTokenChain(teamId, {
617-
accessToken: account.accessToken,
618-
refreshToken: account.refreshToken ?? null,
619-
accessTokenExpiresAt: account.accessTokenExpiresAt ?? null,
620-
})
621-
logger.info('[account.create.after] Propagated Slack installation token chain', {
622-
userId: account.userId,
623-
teamId,
624-
newAccountId: account.id,
625-
})
626-
}
627-
} catch (error) {
628-
logger.error('[account.create.after] Failed to propagate Slack token chain', {
629-
userId: account.userId,
630-
accountId: account.id,
631-
error,
632-
})
633-
}
634-
}
658+
await propagateFreshSlackConnect(account)
635659

636660
const isOAuth2Callback = context?.path.startsWith('/oauth2/callback/') === true
637661
const credentialDraftBinding = context
@@ -653,12 +677,21 @@ export const auth = betterAuth({
653677
accountId: account.id,
654678
})
655679
} catch (error) {
656-
logger.error('[account.create.after] Failed to process credential draft', {
657-
userId: account.userId,
658-
providerId: account.providerId,
659-
error,
660-
})
661-
if (credentialDraftBinding.draftId) throw error
680+
if (error instanceof CredentialAccountConflictError) {
681+
logger.warn('[account.create.after] Refused a conflicting credential reconnect', {
682+
userId: account.userId,
683+
providerId: account.providerId,
684+
credentialId: error.credentialId,
685+
conflictingCredentialId: error.conflictingCredentialId,
686+
})
687+
} else {
688+
logger.error('[account.create.after] Failed to process credential draft', {
689+
userId: account.userId,
690+
providerId: account.providerId,
691+
error,
692+
})
693+
if (credentialDraftBinding.draftId) throw error
694+
}
662695
}
663696
}
664697

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
const RANDOM_UUID_SUFFIX = /-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/
2+
3+
/**
4+
* Returns the provider's stable identity inside a connector account id.
5+
*
6+
* Each `getUserInfo` in `lib/auth/connectors/providers.ts` appends a random
7+
* UUID to the stable external id so Better Auth never blocks cross-user
8+
* connections, which means every connect of the same identity creates a new
9+
* account row. Rows whose ids share this prefix are the same external identity.
10+
* Returns `null` for an id that carries no such suffix.
11+
*/
12+
export function stableExternalAccountId(accountId: string): string | null {
13+
const stable = accountId.replace(RANDOM_UUID_SUFFIX, '')
14+
return stable && stable !== accountId ? stable : null
15+
}

0 commit comments

Comments
 (0)