Skip to content

Commit 7b74299

Browse files
fix(credentials): close OAuth draft edge cases
1 parent 147b161 commit 7b74299

12 files changed

Lines changed: 118 additions & 29 deletions

File tree

apps/sim/app/api/auth/instagram/authorize/route.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ describe('Instagram authorize route', () => {
7777
expect(response.headers.get('set-cookie')).toContain(
7878
'instagram_credential_draft_id=draft-created'
7979
)
80+
expect(response.headers.get('set-cookie')).toContain('Max-Age=900')
8081
expect(mocks.createCredentialConnection).toHaveBeenCalledWith({
8182
principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' },
8283
input: { workspaceId: 'workspace-1', providerId: 'instagram' },

apps/sim/app/api/auth/instagram/authorize/route.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { getBaseUrl } from '@/lib/core/utils/urls'
1010
import { isSameOrigin } from '@/lib/core/utils/validation'
1111
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1212
import { createCredentialConnection } from '@/lib/credentials/application/create-credential-connection'
13+
import { CREDENTIAL_DRAFT_TTL_SECONDS } from '@/lib/credentials/draft-constants'
1314
import { getCanonicalScopesForProvider } from '@/lib/oauth/utils'
1415

1516
const logger = createLogger('InstagramAuthorize')
@@ -20,7 +21,6 @@ const INSTAGRAM_STATE_COOKIE = 'instagram_oauth_state'
2021
const INSTAGRAM_RETURN_URL_COOKIE = 'instagram_return_url'
2122
const INSTAGRAM_CREDENTIAL_DRAFT_COOKIE = 'instagram_credential_draft_id'
2223
const INSTAGRAM_STATE_COOKIE_PATH = '/api/auth'
23-
const INSTAGRAM_STATE_COOKIE_MAX_AGE_SECONDS = 60 * 10
2424

2525
export const GET = withRouteHandler(async (request: NextRequest) => {
2626
try {
@@ -81,15 +81,15 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
8181
httpOnly: true,
8282
secure: process.env.NODE_ENV === 'production',
8383
sameSite: 'lax',
84-
maxAge: INSTAGRAM_STATE_COOKIE_MAX_AGE_SECONDS,
84+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
8585
path: INSTAGRAM_STATE_COOKIE_PATH,
8686
})
8787
if (credentialDraftId) {
8888
response.cookies.set(INSTAGRAM_CREDENTIAL_DRAFT_COOKIE, credentialDraftId, {
8989
httpOnly: true,
9090
secure: process.env.NODE_ENV === 'production',
9191
sameSite: 'lax',
92-
maxAge: INSTAGRAM_STATE_COOKIE_MAX_AGE_SECONDS,
92+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
9393
path: INSTAGRAM_STATE_COOKIE_PATH,
9494
})
9595
} else {
@@ -104,7 +104,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
104104
httpOnly: true,
105105
secure: process.env.NODE_ENV === 'production',
106106
sameSite: 'lax',
107-
maxAge: INSTAGRAM_STATE_COOKIE_MAX_AGE_SECONDS,
107+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
108108
path: INSTAGRAM_STATE_COOKIE_PATH,
109109
})
110110
}

apps/sim/app/api/auth/trello/authorize/route.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { env } from '@/lib/core/config/env'
88
import { getBaseUrl } from '@/lib/core/utils/urls'
99
import { isSameOrigin } from '@/lib/core/utils/validation'
1010
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
11+
import { CREDENTIAL_DRAFT_TTL_SECONDS } from '@/lib/credentials/draft-constants'
1112
import { getCanonicalScopesForProvider } from '@/lib/oauth/utils'
1213

1314
const logger = createLogger('TrelloAuthorize')
@@ -18,7 +19,6 @@ const TRELLO_STATE_COOKIE = 'trello_oauth_state'
1819
const TRELLO_RETURN_URL_COOKIE = 'trello_return_url'
1920
const TRELLO_CREDENTIAL_DRAFT_COOKIE = 'trello_credential_draft_id'
2021
const TRELLO_STATE_COOKIE_PATH = '/api/auth/trello'
21-
const TRELLO_STATE_COOKIE_MAX_AGE_SECONDS = 60 * 10
2222

2323
export const GET = withRouteHandler(async (request: NextRequest) => {
2424
try {
@@ -58,15 +58,15 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
5858
httpOnly: true,
5959
secure: process.env.NODE_ENV === 'production',
6060
sameSite: 'lax',
61-
maxAge: TRELLO_STATE_COOKIE_MAX_AGE_SECONDS,
61+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
6262
path: TRELLO_STATE_COOKIE_PATH,
6363
})
6464
if (draftId) {
6565
response.cookies.set(TRELLO_CREDENTIAL_DRAFT_COOKIE, draftId, {
6666
httpOnly: true,
6767
secure: process.env.NODE_ENV === 'production',
6868
sameSite: 'lax',
69-
maxAge: TRELLO_STATE_COOKIE_MAX_AGE_SECONDS,
69+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
7070
path: TRELLO_STATE_COOKIE_PATH,
7171
})
7272
} else {
@@ -80,7 +80,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
8080
httpOnly: true,
8181
secure: process.env.NODE_ENV === 'production',
8282
sameSite: 'lax',
83-
maxAge: TRELLO_STATE_COOKIE_MAX_AGE_SECONDS,
83+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
8484
path: TRELLO_STATE_COOKIE_PATH,
8585
})
8686
} else {

apps/sim/lib/auth/auth.ts

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ import {
9797
import { PlatformEvents } from '@/lib/core/telemetry'
9898
import { getBaseUrl, isLocalhostUrl, parseOriginList } from '@/lib/core/utils/urls'
9999
import {
100-
OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM,
100+
parseCredentialDraftIdFromCallbackUrl,
101101
processCredentialDraft,
102102
} from '@/lib/credentials/draft-processor'
103103
import { sendEmail } from '@/lib/messaging/email/mailer'
@@ -529,14 +529,17 @@ export const auth = betterAuth({
529529
let credentialDraftId: string | undefined
530530
try {
531531
const oauthState = await getOAuthState()
532-
const rawCallbackUrl = oauthState?.callbackURL
533-
if (rawCallbackUrl !== undefined && typeof rawCallbackUrl !== 'string') {
534-
throw new Error('OAuth state callback URL must be a string')
535-
}
536-
credentialDraftId = rawCallbackUrl
537-
? (new URL(rawCallbackUrl).searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ??
538-
undefined)
539-
: undefined
532+
credentialDraftId = parseCredentialDraftIdFromCallbackUrl(oauthState?.callbackURL)
533+
} catch (error) {
534+
logger.error('[account.create.after] Failed to read OAuth credential draft state', {
535+
userId: account.userId,
536+
providerId: account.providerId,
537+
error,
538+
})
539+
throw error
540+
}
541+
542+
try {
540543
await processCredentialDraft({
541544
draftId: credentialDraftId,
542545
userId: account.userId,

apps/sim/lib/credentials/connect-draft.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ import { generateId } from '@sim/utils/id'
55
import { and, eq, gt, isNull, lt } from 'drizzle-orm'
66
import { OrchestrationError } from '@/lib/core/orchestration/types'
77
import { defaultCredentialDisplayName } from '@/lib/credentials/display-name'
8+
import { CREDENTIAL_DRAFT_TTL_MS } from '@/lib/credentials/draft-constants'
89
import { credentialProviderMatchesService, getAllOAuthServices } from '@/lib/oauth/utils'
910

1011
const logger = createLogger('OAuthConnectDraft')
11-
const DRAFT_TTL_MS = 15 * 60 * 1000
1212

1313
export type ConnectDraft = typeof pendingCredentialDraft.$inferSelect
1414

@@ -63,7 +63,7 @@ export async function createConnectDraft(params: {
6363
}
6464

6565
const now = new Date()
66-
const expiresAt = new Date(now.getTime() + DRAFT_TTL_MS)
66+
const expiresAt = new Date(now.getTime() + CREDENTIAL_DRAFT_TTL_MS)
6767
await db
6868
.delete(pendingCredentialDraft)
6969
.where(
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
export const CREDENTIAL_DRAFT_TTL_MS = 15 * 60 * 1000
2+
export const CREDENTIAL_DRAFT_TTL_SECONDS = CREDENTIAL_DRAFT_TTL_MS / 1000
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { auditMock, auditMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const mocks = vi.hoisted(() => ({
8+
clearDeadFlag: vi.fn(),
9+
}))
10+
11+
vi.mock('@sim/audit', () => auditMock)
12+
vi.mock('@/lib/oauth/terminal-errors', () => ({ clearDeadFlag: mocks.clearDeadFlag }))
13+
vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
14+
15+
import { handleReconnectCredential } from '@/lib/credentials/draft-hooks'
16+
17+
describe('handleReconnectCredential', () => {
18+
beforeEach(() => {
19+
vi.clearAllMocks()
20+
resetDbChainMock()
21+
})
22+
23+
it('audits a reconnect with the credential current name instead of draft presentation', async () => {
24+
queueTableRows(schemaMock.credential, [
25+
{ id: 'credential-1', accountId: null, displayName: 'Renamed Gmail' },
26+
])
27+
queueTableRows(schemaMock.credential, [])
28+
29+
await handleReconnectCredential({
30+
draft: { credentialId: 'credential-1' },
31+
newAccountId: 'account-new',
32+
workspaceId: 'workspace-1',
33+
userId: 'user-1',
34+
now: new Date('2026-08-14T18:00:00.000Z'),
35+
})
36+
37+
expect(auditMockFns.mockRecordAudit).toHaveBeenCalledWith(
38+
expect.objectContaining({
39+
resourceId: 'credential-1',
40+
resourceName: 'Renamed Gmail',
41+
description: 'Reconnected OAuth credential "Renamed Gmail" to a new account',
42+
})
43+
)
44+
})
45+
})

apps/sim/lib/credentials/draft-hooks.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ export async function handleCreateCredentialFromDraft(params: {
105105
* the dead flag. Callers treat that timestamp as proof the reconnect landed.
106106
*/
107107
export async function handleReconnectCredential(params: {
108-
draft: { credentialId: string | null; workspaceId: string; displayName: string }
108+
draft: { credentialId: string | null }
109109
newAccountId: string
110110
workspaceId: string
111111
userId: string
@@ -115,7 +115,11 @@ export async function handleReconnectCredential(params: {
115115
if (!draft.credentialId) return
116116

117117
const [existingCredential] = await db
118-
.select({ id: schema.credential.id, accountId: schema.credential.accountId })
118+
.select({
119+
id: schema.credential.id,
120+
accountId: schema.credential.accountId,
121+
displayName: schema.credential.displayName,
122+
})
119123
.from(schema.credential)
120124
.where(eq(schema.credential.id, draft.credentialId))
121125
.limit(1)
@@ -125,6 +129,7 @@ export async function handleReconnectCredential(params: {
125129
}
126130

127131
const oldAccountId = existingCredential.accountId
132+
const displayName = existingCredential.displayName
128133
const accountChanged = oldAccountId !== newAccountId
129134

130135
if (accountChanged) {
@@ -171,10 +176,10 @@ export async function handleReconnectCredential(params: {
171176
action: AuditAction.CREDENTIAL_RECONNECTED,
172177
resourceType: AuditResourceType.CREDENTIAL,
173178
resourceId: draft.credentialId,
174-
resourceName: draft.displayName,
179+
resourceName: displayName,
175180
description: accountChanged
176-
? `Reconnected OAuth credential "${draft.displayName}" to a new account`
177-
: `Reconnected OAuth credential "${draft.displayName}"`,
181+
? `Reconnected OAuth credential "${displayName}" to a new account`
182+
: `Reconnected OAuth credential "${displayName}"`,
178183
metadata: { oldAccountId, newAccountId },
179184
})
180185

apps/sim/lib/credentials/draft-processor.test.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ vi.mock('@/lib/credentials/draft-hooks', () => ({
2020
handleReconnectCredential: mockHandleReconnectCredential,
2121
}))
2222

23-
import { processCredentialDraft } from '@/lib/credentials/draft-processor'
23+
import {
24+
parseCredentialDraftIdFromCallbackUrl,
25+
processCredentialDraft,
26+
} from '@/lib/credentials/draft-processor'
2427

2528
function credentialDraft(id: string, workspaceId: string) {
2629
return {
@@ -103,3 +106,20 @@ describe('processCredentialDraft', () => {
103106
expect(dbChainMockFns.delete).not.toHaveBeenCalled()
104107
})
105108
})
109+
110+
describe('parseCredentialDraftIdFromCallbackUrl', () => {
111+
it('extracts the exact draft id from a valid callback URL', () => {
112+
expect(
113+
parseCredentialDraftIdFromCallbackUrl(
114+
'https://sim.test/oauth/credential-connected?credentialDraftId=draft-1'
115+
)
116+
).toBe('draft-1')
117+
})
118+
119+
it('fails closed for malformed or non-string callback state', () => {
120+
expect(() => parseCredentialDraftIdFromCallbackUrl({})).toThrow(
121+
'OAuth state callback URL must be a string'
122+
)
123+
expect(() => parseCredentialDraftIdFromCallbackUrl('not a URL')).toThrow()
124+
})
125+
})

apps/sim/lib/credentials/draft-processor.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@ const logger = createLogger('CredentialDraftProcessor')
1111

1212
export const OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM = 'credentialDraftId'
1313

14+
/** Extracts a draft binding from Better Auth state and rejects malformed callback state. */
15+
export function parseCredentialDraftIdFromCallbackUrl(callbackUrl: unknown): string | undefined {
16+
if (callbackUrl === undefined) return undefined
17+
if (typeof callbackUrl !== 'string') {
18+
throw new Error('OAuth state callback URL must be a string')
19+
}
20+
return new URL(callbackUrl).searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ?? undefined
21+
}
22+
1423
interface ProcessCredentialDraftParams {
1524
draftId?: string
1625
userId: string

0 commit comments

Comments
 (0)