Skip to content

Commit ae46594

Browse files
fix(credentials): fail closed without breaking auth
1 parent d419ada commit ae46594

5 files changed

Lines changed: 134 additions & 23 deletions

File tree

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { createMockRequest } from '@sim/testing'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const mocks = vi.hoisted(() => ({
8+
getSession: vi.fn(),
9+
requireConfiguredOAuthClient: vi.fn(),
10+
}))
11+
12+
vi.mock('@/lib/auth', () => ({
13+
getSession: mocks.getSession,
14+
}))
15+
16+
vi.mock('@/lib/core/config/env-capabilities.server', () => ({
17+
requireConfiguredOAuthClient: mocks.requireConfiguredOAuthClient,
18+
}))
19+
20+
vi.mock('@/lib/core/utils/urls', () => ({
21+
getBaseUrl: () => 'https://sim.test',
22+
}))
23+
24+
vi.mock('@/lib/oauth/shopify-state', () => ({
25+
createShopifyOAuthState: () => 'signed-state',
26+
}))
27+
28+
vi.mock('@/lib/oauth/utils', () => ({
29+
getScopesForService: () => ['read_products'],
30+
}))
31+
32+
import { GET } from '@/app/api/auth/shopify/authorize/route'
33+
34+
describe('Shopify authorize route', () => {
35+
beforeEach(() => {
36+
vi.clearAllMocks()
37+
mocks.getSession.mockResolvedValue({ user: { id: 'user-1' } })
38+
mocks.requireConfiguredOAuthClient.mockReturnValue({
39+
values: {
40+
SHOPIFY_CLIENT_ID: 'shopify-client',
41+
SHOPIFY_CLIENT_SECRET: 'shopify-secret',
42+
},
43+
})
44+
})
45+
46+
it('keeps the post-connect return URL for the full credential draft lifetime', async () => {
47+
const request = createMockRequest(
48+
'GET',
49+
undefined,
50+
{},
51+
'https://sim.test/api/auth/shopify/authorize?shop=test-store.myshopify.com&returnUrl=https%3A%2F%2Fsim.test%2Foauth%2Fcredential-connected&draftId=draft-1'
52+
)
53+
54+
const response = await GET(request)
55+
56+
expect(response.status).toBe(307)
57+
expect(response.headers.get('set-cookie')).toContain('shopify_return_url=')
58+
expect(response.headers.get('set-cookie')).toContain('Max-Age=900')
59+
})
60+
})

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { requireConfiguredOAuthClient } from '@/lib/core/config/env-capabilities
99
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'
12+
import { CREDENTIAL_DRAFT_TTL_SECONDS } from '@/lib/credentials/draft-constants'
1213
import { createShopifyOAuthState } from '@/lib/oauth/shopify-state'
1314
import { getScopesForService } from '@/lib/oauth/utils'
1415

@@ -209,7 +210,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
209210
httpOnly: true,
210211
secure: process.env.NODE_ENV === 'production',
211212
sameSite: 'lax',
212-
maxAge: 60 * 10,
213+
maxAge: CREDENTIAL_DRAFT_TTL_SECONDS,
213214
path: '/',
214215
})
215216
} else {

apps/sim/lib/auth/auth.ts

Lines changed: 22 additions & 22 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-
parseCredentialDraftIdFromCallbackUrl,
100+
loadOAuthCredentialDraftBinding,
101101
processCredentialDraft,
102102
} from '@/lib/credentials/draft-processor'
103103
import { sendEmail } from '@/lib/messaging/email/mailer'
@@ -526,33 +526,33 @@ export const auth = betterAuth({
526526
}
527527
}
528528

529-
let credentialDraftId: string | undefined
530-
try {
531-
const oauthState = await getOAuthState()
532-
credentialDraftId = parseCredentialDraftIdFromCallbackUrl(oauthState?.callbackURL)
533-
} catch (error) {
529+
const credentialDraftBinding = await loadOAuthCredentialDraftBinding(() =>
530+
getOAuthState()
531+
)
532+
if (credentialDraftBinding.status === 'unavailable') {
534533
logger.error('[account.create.after] Failed to read OAuth credential draft state', {
535534
userId: account.userId,
536535
providerId: account.providerId,
537-
error,
536+
error: credentialDraftBinding.error,
538537
})
539-
throw error
540538
}
541539

542-
try {
543-
await processCredentialDraft({
544-
draftId: credentialDraftId,
545-
userId: account.userId,
546-
providerId: account.providerId,
547-
accountId: account.id,
548-
})
549-
} catch (error) {
550-
logger.error('[account.create.after] Failed to process credential draft', {
551-
userId: account.userId,
552-
providerId: account.providerId,
553-
error,
554-
})
555-
if (credentialDraftId) throw error
540+
if (credentialDraftBinding.status === 'available') {
541+
try {
542+
await processCredentialDraft({
543+
draftId: credentialDraftBinding.draftId,
544+
userId: account.userId,
545+
providerId: account.providerId,
546+
accountId: account.id,
547+
})
548+
} catch (error) {
549+
logger.error('[account.create.after] Failed to process credential draft', {
550+
userId: account.userId,
551+
providerId: account.providerId,
552+
error,
553+
})
554+
if (credentialDraftBinding.draftId) throw error
555+
}
556556
}
557557

558558
try {

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ vi.mock('@/lib/credentials/draft-hooks', () => ({
2121
}))
2222

2323
import {
24+
loadOAuthCredentialDraftBinding,
2425
parseCredentialDraftIdFromCallbackUrl,
2526
processCredentialDraft,
2627
} from '@/lib/credentials/draft-processor'
@@ -123,3 +124,29 @@ describe('parseCredentialDraftIdFromCallbackUrl', () => {
123124
expect(() => parseCredentialDraftIdFromCallbackUrl('not a URL')).toThrow()
124125
})
125126
})
127+
128+
describe('loadOAuthCredentialDraftBinding', () => {
129+
it('returns the exact draft id when OAuth state is readable', async () => {
130+
await expect(
131+
loadOAuthCredentialDraftBinding(async () => ({
132+
callbackURL: 'https://sim.test/oauth/credential-connected?credentialDraftId=draft-exact',
133+
}))
134+
).resolves.toEqual({ status: 'available', draftId: 'draft-exact' })
135+
})
136+
137+
it('marks unreadable OAuth state unavailable instead of permitting legacy draft fallback', async () => {
138+
const stateError = new Error('OAuth state is unavailable')
139+
140+
await expect(
141+
loadOAuthCredentialDraftBinding(async () => {
142+
throw stateError
143+
})
144+
).resolves.toEqual({ status: 'unavailable', error: stateError })
145+
})
146+
147+
it('marks malformed callback state unavailable without throwing from the account hook', async () => {
148+
const binding = await loadOAuthCredentialDraftBinding(async () => ({ callbackURL: null }))
149+
150+
expect(binding.status).toBe('unavailable')
151+
})
152+
})

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

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

1212
export const OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM = 'credentialDraftId'
1313

14+
interface OAuthStateWithCallbackUrl {
15+
callbackURL?: unknown
16+
}
17+
18+
type OAuthCredentialDraftBinding =
19+
| { status: 'available'; draftId?: string }
20+
| { status: 'unavailable'; error: unknown }
21+
1422
/** Extracts a draft binding from Better Auth state and rejects malformed callback state. */
1523
export function parseCredentialDraftIdFromCallbackUrl(callbackUrl: unknown): string | undefined {
1624
if (callbackUrl === undefined) return undefined
@@ -20,6 +28,21 @@ export function parseCredentialDraftIdFromCallbackUrl(callbackUrl: unknown): str
2028
return new URL(callbackUrl).searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ?? undefined
2129
}
2230

31+
/** Reads an exact draft binding without falling back when OAuth state is unavailable. */
32+
export async function loadOAuthCredentialDraftBinding(
33+
loadOAuthState: () => Promise<OAuthStateWithCallbackUrl | null | undefined>
34+
): Promise<OAuthCredentialDraftBinding> {
35+
try {
36+
const oauthState = await loadOAuthState()
37+
return {
38+
status: 'available',
39+
draftId: parseCredentialDraftIdFromCallbackUrl(oauthState?.callbackURL),
40+
}
41+
} catch (error) {
42+
return { status: 'unavailable', error }
43+
}
44+
}
45+
2346
interface ProcessCredentialDraftParams {
2447
draftId?: string
2548
userId: string

0 commit comments

Comments
 (0)