Skip to content

Commit 94fdf84

Browse files
icecrasher321claude
andcommitted
fix(desktop): stop the OAuth connect callback from failing on a bare-path callback URL
The desktop connect launcher passed better-auth a same-origin path as its callbackURL. Better Auth stores that value verbatim in the OAuth state, and the callback's credential-draft reader parsed it with a bare `new URL()`, which rejects a path. That throw happened inside the `account.create.before` database hook, which better-auth's OAuth callback does not guard, so the provider redirect landed on a 500 after authorization had already succeeded. Send an absolute URL from the connect page, matching the workspace-scoped branch and every other connect surface, and accept a path-absolute callback URL in the draft reader so the shape can never fail the callback again. Protocol-relative and malformed values still throw, keeping an unreadable binding loud. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9ff1d77 commit 94fdf84

5 files changed

Lines changed: 178 additions & 9 deletions

File tree

apps/sim/app/desktop/connect/connect-launcher.tsx

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,13 @@ import { DesktopHandoffShell } from '@/app/desktop/components/desktop-handoff-sh
88

99
interface ConnectLauncherProps {
1010
providerId: string
11-
/** Same-origin path better-auth returns the browser to after the callback. */
12-
completePath: string
11+
/**
12+
* Absolute URL better-auth returns the browser to after the callback. Better
13+
* Auth stores it verbatim in the OAuth state and the callback reads the
14+
* credential draft back off it, so a bare path would be parsed without an
15+
* origin — keep this a full URL, as every other connect surface passes.
16+
*/
17+
completeUrl: string
1318
}
1419

1520
/**
@@ -19,7 +24,7 @@ interface ConnectLauncherProps {
1924
* leaves for the provider immediately, so the UI is just a brief interstitial
2025
* plus an error state with retry.
2126
*/
22-
export function ConnectLauncher({ providerId, completePath }: ConnectLauncherProps) {
27+
export function ConnectLauncher({ providerId, completeUrl }: ConnectLauncherProps) {
2328
const startedRef = useRef(false)
2429
const [error, setError] = useState<string | null>(null)
2530

@@ -28,18 +33,18 @@ export function ConnectLauncher({ providerId, completePath }: ConnectLauncherPro
2833
try {
2934
await client.oauth2.link({
3035
providerId,
31-
callbackURL: completePath,
36+
callbackURL: completeUrl,
3237
// Failed flows bounce to the same complete page (which forwards the
3338
// failure to the loopback) instead of waiting out the handoff TTL.
3439
// Do NOT bake in a query param here: better-auth appends its own
3540
// `&error=<code>`, and a second `error` key deserializes to an array
3641
// that the complete page can't read — so it would look like success.
37-
errorCallbackURL: completePath,
42+
errorCallbackURL: completeUrl,
3843
})
3944
} catch (err) {
4045
setError(getErrorMessage(err, 'Could not start the connection.'))
4146
}
42-
}, [providerId, completePath])
47+
}, [providerId, completeUrl])
4348

4449
useEffect(() => {
4550
if (startedRef.current) return
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockGetSession, mockRedirect } = vi.hoisted(() => ({
7+
mockGetSession: vi.fn(),
8+
mockRedirect: vi.fn((url: string) => {
9+
throw new Error(`NEXT_REDIRECT:${url}`)
10+
}),
11+
}))
12+
13+
vi.mock('@/lib/auth', () => ({
14+
auth: { api: { getSession: mockGetSession } },
15+
getSession: vi.fn(),
16+
}))
17+
18+
vi.mock('@/lib/auth/auth-client', () => ({
19+
client: { oauth2: { link: vi.fn() } },
20+
signOut: vi.fn(),
21+
}))
22+
23+
vi.mock('@/lib/core/utils/urls', () => ({
24+
getBaseUrl: () => 'https://sim.test',
25+
}))
26+
27+
/** Keeps the landing-page barrel the real shell pulls in out of this graph. */
28+
vi.mock('@/app/desktop/components/desktop-handoff-shell', () => ({
29+
DesktopHandoffShell: () => null,
30+
}))
31+
32+
vi.mock('next/navigation', () => ({
33+
redirect: mockRedirect,
34+
}))
35+
36+
vi.mock('next/headers', () => ({
37+
headers: vi.fn(async () => new Headers()),
38+
}))
39+
40+
import DesktopConnectPage from '@/app/desktop/connect/page'
41+
42+
const VALID_STATE = 'a'.repeat(32)
43+
const PORT = '57979'
44+
45+
function pageProps(params: Record<string, string>) {
46+
return { searchParams: Promise.resolve(params) }
47+
}
48+
49+
async function renderPage(params: Record<string, string>) {
50+
const result = (await DesktopConnectPage(pageProps(params))) as unknown as {
51+
type: { name: string }
52+
props: Record<string, unknown>
53+
}
54+
return result
55+
}
56+
57+
describe('DesktopConnectPage', () => {
58+
beforeEach(() => {
59+
vi.clearAllMocks()
60+
mockGetSession.mockResolvedValue({ user: { id: 'user-1', email: 'user@example.com' } })
61+
})
62+
63+
it('hands the launcher an absolute complete URL so the callback can read the draft back', async () => {
64+
// Better Auth stores `callbackURL` verbatim, and the OAuth callback parses it
65+
// with `new URL`. A bare path threw there, failing the whole callback with a
66+
// 500 after the provider had already authorized.
67+
const result = await renderPage({
68+
provider: 'google-email',
69+
state: VALID_STATE,
70+
port: PORT,
71+
draftId: 'draft-1',
72+
})
73+
74+
expect(result.type.name).toBe('ConnectLauncher')
75+
expect(result.props.providerId).toBe('google-email')
76+
77+
const completeUrl = new URL(result.props.completeUrl as string)
78+
expect(completeUrl.origin).toBe('https://sim.test')
79+
expect(completeUrl.pathname).toBe('/desktop/connect/complete')
80+
expect(completeUrl.searchParams.get('state')).toBe(VALID_STATE)
81+
expect(completeUrl.searchParams.get('port')).toBe(PORT)
82+
expect(completeUrl.searchParams.get('credentialDraftId')).toBe('draft-1')
83+
})
84+
85+
it('keeps the complete URL absolute when no draft rides along', async () => {
86+
const result = await renderPage({
87+
provider: 'google-email',
88+
state: VALID_STATE,
89+
port: PORT,
90+
})
91+
92+
expect(result.type.name).toBe('ConnectLauncher')
93+
expect(() => new URL(result.props.completeUrl as string)).not.toThrow()
94+
})
95+
96+
it('sends a workspace-scoped connect to the authorize route with an absolute callback', async () => {
97+
await expect(
98+
DesktopConnectPage(
99+
pageProps({
100+
provider: 'google-email',
101+
state: VALID_STATE,
102+
port: PORT,
103+
workspaceId: 'workspace-1',
104+
})
105+
)
106+
).rejects.toThrow('NEXT_REDIRECT:')
107+
108+
const authorize = new URL(mockRedirect.mock.calls[0][0])
109+
expect(authorize.pathname).toBe('/api/auth/oauth2/authorize')
110+
expect(authorize.searchParams.get('providerId')).toBe('google-email')
111+
expect(authorize.searchParams.get('workspaceId')).toBe('workspace-1')
112+
expect(authorize.searchParams.get('callbackURL')).toBe(
113+
`https://sim.test/desktop/connect/complete?state=${VALID_STATE}&port=${PORT}`
114+
)
115+
})
116+
117+
it('rejects a malformed request without reading the session', async () => {
118+
const invalid = [
119+
{ provider: 'Google', state: VALID_STATE, port: PORT },
120+
{ provider: 'google-email', state: 'short', port: PORT },
121+
{ provider: 'google-email', state: VALID_STATE },
122+
{ provider: 'google-email', state: VALID_STATE, port: PORT, draftId: 'bad draft' },
123+
]
124+
125+
for (const params of invalid) {
126+
const result = await renderPage(params)
127+
expect(result.type.name).toBe('InvalidRequest')
128+
}
129+
expect(mockGetSession).not.toHaveBeenCalled()
130+
})
131+
})

apps/sim/app/desktop/connect/page.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ export default async function DesktopConnectPage({ searchParams }: DesktopConnec
125125
return (
126126
<ConnectLauncher
127127
providerId={providerId}
128-
completePath={buildConnectCompletePath(state, port, draftId)}
128+
completeUrl={`${getBaseUrl()}${buildConnectCompletePath(state, port, draftId)}`}
129129
/>
130130
)
131131
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,11 +119,23 @@ describe('parseCredentialDraftIdFromCallbackUrl', () => {
119119
).toBe('draft-1')
120120
})
121121

122+
it('reads a same-origin path, which Better Auth stores verbatim', () => {
123+
expect(
124+
parseCredentialDraftIdFromCallbackUrl(
125+
'/desktop/connect/complete?state=abc&port=57979&credentialDraftId=draft-1'
126+
)
127+
).toBe('draft-1')
128+
expect(
129+
parseCredentialDraftIdFromCallbackUrl('/desktop/connect/complete?state=abc&port=57979')
130+
).toBeUndefined()
131+
})
132+
122133
it('fails closed for malformed or non-string callback state', () => {
123134
expect(() => parseCredentialDraftIdFromCallbackUrl({})).toThrow(
124135
'OAuth state callback URL must be a string'
125136
)
126137
expect(() => parseCredentialDraftIdFromCallbackUrl('not a URL')).toThrow()
138+
expect(() => parseCredentialDraftIdFromCallbackUrl('//elsewhere.test/path')).toThrow()
127139
})
128140
})
129141

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

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,34 @@ type AvailableOAuthCredentialDraftBinding = Extract<
2525

2626
const oauthCredentialDraftBindings = new WeakMap<object, AvailableOAuthCredentialDraftBinding>()
2727

28-
/** Extracts a draft binding from Better Auth state and rejects malformed callback state. */
28+
/**
29+
* Origin a path-absolute callback URL is resolved against. Only the query string
30+
* is ever read, so the origin is immaterial — a placeholder keeps this parse
31+
* independent of `NEXT_PUBLIC_APP_URL`, which `getBaseUrl()` throws without.
32+
*/
33+
const SAME_ORIGIN_CALLBACK_BASE = 'http://callback.invalid'
34+
35+
/**
36+
* Extracts a draft binding from Better Auth state and rejects malformed callback state.
37+
*
38+
* Better Auth stores `callbackURL` verbatim, so a path-absolute value is as
39+
* valid as a full URL. Bare `new URL()` rejects the former, and since this runs
40+
* inside the `account.create.before` database hook — which Better Auth's OAuth
41+
* callback does not guard — that rejection surfaced as a 500 on the callback
42+
* itself rather than a failed connection. Only those two shapes are accepted:
43+
* a protocol-relative `//host/path` is not a path, and still throws along with
44+
* everything else malformed, so an unreadable binding stays loud.
45+
*/
2946
export function parseCredentialDraftIdFromCallbackUrl(callbackUrl: unknown): string | undefined {
3047
if (callbackUrl === undefined) return undefined
3148
if (typeof callbackUrl !== 'string') {
3249
throw new Error('OAuth state callback URL must be a string')
3350
}
34-
return new URL(callbackUrl).searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ?? undefined
51+
const isPathAbsolute = callbackUrl.startsWith('/') && !callbackUrl.startsWith('//')
52+
const url = isPathAbsolute
53+
? new URL(callbackUrl, SAME_ORIGIN_CALLBACK_BASE)
54+
: new URL(callbackUrl)
55+
return url.searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ?? undefined
3556
}
3657

3758
/** Reads an exact draft binding without falling back when OAuth state is unavailable. */

0 commit comments

Comments
 (0)