Skip to content

Commit 43aba98

Browse files
fix(desktop): stop the OAuth connect callback from failing on a bare-path callback URL (#7005)
* 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> * fix(desktop): compose the connect completion URL through the URL API Concatenating `getBaseUrl()` with the completion path leaves the result dependent on how the deployment spelled `NEXT_PUBLIC_APP_URL`: the helper only adds a missing protocol, so a trailing slash produced `//desktop/connect/complete`, a pathname that matches no route. The completion page is what bounces the OAuth result to the desktop app's loopback, so that typo would have stranded the flow just past the callback it was meant to fix. Both callback URLs in the page — the launcher's and the workspace-scoped authorize redirect's — now go through one helper that resolves the path against the base with `new URL`, matching how the same function already builds the authorize URL, with coverage for a trailing-slash base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(urls): give base URLs the no-trailing-slash form their call sites assume `getBaseUrl()` returned `NEXT_PUBLIC_APP_URL` as the operator spelled it, while almost every consumer builds `${base}/path`. A base configured with a trailing slash therefore produced a `//path` pathname that matches no route, and broke the `startsWith(`${base}/`)` prefix checks that decide whether a redirect target is our own — the OAuth authorize route rejected its own completion callback and fell back to the workspace page, so the desktop handoff never ran on those deployments. The previous commit fixed one such URL; this fixes the reason it was wrong, for the ~30 concatenation sites that share the assumption. `normalizeBaseUrl` now strips trailing slashes alongside the protocol it already added, which is the invariant SITE_URL has always documented. A path-prefixed base keeps its path. `getInternalApiBaseUrl` gets the same treatment, since its callers concatenate identically. `@sim/testing`'s urls mock is a hand-written mirror of this module, so it moves in step. `internal-api-base-url.test.ts` now unmocks the module it names — otherwise it asserts against that mirror and any drift between the two passes unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(urls): stop claiming path-prefixed base URLs are supported The previous commit's doc and test said a path-prefixed base keeps its path. That reads as support for a deployment shape the app does not have: there is no Next `basePath`, so routes are served at the origin root and such a value could not address them however the base were normalized. Every documented example is origin-only. Says only what is true — trailing slashes are the one spelling absorbed — and reframes the test as pinning the trim's shape rather than asserting a path-prefixed deployment works. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent bbf408b commit 43aba98

9 files changed

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

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,17 @@ function InvalidRequest() {
3434
)
3535
}
3636

37+
/**
38+
* Absolute URL better-auth returns the browser to once the OAuth callback is
39+
* done. Composed through the URL API rather than concatenated, so a trailing
40+
* slash on `NEXT_PUBLIC_APP_URL` cannot yield a `//desktop/...` pathname that
41+
* matches no route — this page is what bounces the result to the app's
42+
* loopback, so a base-URL typo would otherwise strand the whole flow.
43+
*/
44+
function buildConnectCompleteUrl(state: string, port: number, draftId?: string): string {
45+
return new URL(buildConnectCompletePath(state, port, draftId), getBaseUrl()).toString()
46+
}
47+
3748
/**
3849
* Desktop OAuth-connect landing. The desktop app opens this page in the
3950
* system browser with the provider to connect, a one-time state, and the port
@@ -112,10 +123,7 @@ export default async function DesktopConnectPage({ searchParams }: DesktopConnec
112123
const authorize = new URL('/api/auth/oauth2/authorize', getBaseUrl())
113124
authorize.searchParams.set('providerId', providerId)
114125
authorize.searchParams.set('workspaceId', workspaceId)
115-
authorize.searchParams.set(
116-
'callbackURL',
117-
`${getBaseUrl()}${buildConnectCompletePath(state, port)}`
118-
)
126+
authorize.searchParams.set('callbackURL', buildConnectCompleteUrl(state, port))
119127
if (credentialId) {
120128
authorize.searchParams.set('credentialId', credentialId)
121129
}
@@ -125,7 +133,7 @@ export default async function DesktopConnectPage({ searchParams }: DesktopConnec
125133
return (
126134
<ConnectLauncher
127135
providerId={providerId}
128-
completePath={buildConnectCompletePath(state, port, draftId)}
136+
completeUrl={buildConnectCompleteUrl(state, port, draftId)}
129137
/>
130138
)
131139
}

apps/sim/lib/core/utils/internal-api-base-url.test.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,15 @@
1212
* @vitest-environment node
1313
*/
1414
import { resetEnvMock, setEnv } from '@sim/testing'
15-
import { afterEach, describe, expect, it } from 'vitest'
15+
import { afterEach, describe, expect, it, vi } from 'vitest'
16+
17+
/**
18+
* `vitest.setup.ts` mocks this module globally with a hand-written mirror, so
19+
* without this the suite would assert against that mirror rather than the
20+
* function it names — and any drift between the two would pass unnoticed.
21+
*/
22+
vi.unmock('@/lib/core/utils/urls')
23+
1624
import { getInternalApiBaseUrl } from '@/lib/core/utils/urls'
1725

1826
const PUBLIC_URL = 'https://sim.ai'
@@ -33,6 +41,17 @@ describe('getInternalApiBaseUrl', () => {
3341
expect(getInternalApiBaseUrl()).toBe(LOOPBACK)
3442
})
3543

44+
/** Callers concatenate `${base}/api/...`, exactly as they do with getBaseUrl(). */
45+
it('strips a trailing slash from the internal URL', () => {
46+
setEnv({
47+
INTERNAL_API_BASE_URL: `${LOOPBACK}/`,
48+
NEXT_PUBLIC_APP_URL: PUBLIC_URL,
49+
DB_APP_NAME: 'sim',
50+
})
51+
52+
expect(getInternalApiBaseUrl()).toBe(LOOPBACK)
53+
})
54+
3655
it('IGNORES the internal URL on a Trigger.dev worker and falls back to the public URL', () => {
3756
setEnv({
3857
INTERNAL_API_BASE_URL: LOOPBACK,

apps/sim/lib/core/utils/urls.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,43 @@ describe('getBaseUrl', () => {
5656
expect(getBaseUrl()).toBe('https://app.example.com')
5757
})
5858

59+
/**
60+
* Call sites build `${getBaseUrl()}/path`, so a trailing slash would give them
61+
* a `//path` pathname that matches no route — and would break the
62+
* `startsWith(`${base}/`)` prefix checks that decide whether a redirect target
63+
* is our own, silently sending those redirects to their fallback instead.
64+
*/
65+
it('strips trailing slashes so concatenated paths stay single-slashed', () => {
66+
for (const configured of ['https://app.example.com/', 'https://app.example.com///']) {
67+
mockGetEnv.mockImplementation((key) =>
68+
key === 'NEXT_PUBLIC_APP_URL' ? configured : undefined
69+
)
70+
expect(getBaseUrl()).toBe('https://app.example.com')
71+
expect(new URL(`${getBaseUrl()}/desktop/connect/complete`).pathname).toBe(
72+
'/desktop/connect/complete'
73+
)
74+
}
75+
})
76+
77+
/**
78+
* Pins the trim's shape — it must not eat more than the trailing slashes.
79+
* Not a claim that a path-prefixed deployment works: the app declares no Next
80+
* `basePath`, so such a value could not address its routes either way.
81+
*/
82+
it('trims only trailing slashes, never interior ones', () => {
83+
mockGetEnv.mockImplementation((key) =>
84+
key === 'NEXT_PUBLIC_APP_URL' ? 'https://example.com/a/b/' : undefined
85+
)
86+
expect(getBaseUrl()).toBe('https://example.com/a/b')
87+
})
88+
89+
it('adds the protocol and strips the trailing slash together', () => {
90+
mockGetEnv.mockImplementation((key) =>
91+
key === 'NEXT_PUBLIC_APP_URL' ? 'app.example.com/' : undefined
92+
)
93+
expect(getBaseUrl()).toBe('http://app.example.com')
94+
})
95+
5996
/**
6097
* Never guesses from `window.location.origin`: an opaque origin (a sandboxed
6198
* iframe) serializes to the truthy string `'null'`, which would silently

apps/sim/lib/core/utils/urls.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,26 @@ function hasHttpProtocol(url: string): boolean {
1212
return /^https?:\/\//i.test(url)
1313
}
1414

15+
/**
16+
* Brings a configured base URL to the no-trailing-slash form {@link SITE_URL}
17+
* documents: adds the protocol when the operator omitted it, then strips
18+
* trailing slashes.
19+
*
20+
* Call sites overwhelmingly build URLs as `${base}/path`, so a base spelled
21+
* `https://host/` gives every one of them a `//path` pathname that matches no
22+
* route, and breaks the `startsWith(`${base}/`)` prefix checks that decide
23+
* whether a redirect target is our own. Normalizing once here is what lets
24+
* those call sites stay simple instead of each defending against the operator's
25+
* spelling.
26+
*
27+
* Trailing slashes are the only spelling this absorbs. The app declares no Next
28+
* `basePath`, so its routes are served at the origin root and a path-prefixed
29+
* value could not address them however this normalized it.
30+
*/
1531
function normalizeBaseUrl(url: string): string {
16-
if (hasHttpProtocol(url)) {
17-
return url
18-
}
19-
2032
const protocol = isProd ? 'https://' : 'http://'
21-
return `${protocol}${url}`
33+
const withProtocol = hasHttpProtocol(url) ? url : `${protocol}${url}`
34+
return withProtocol.replace(/\/+$/, '')
2235
}
2336

2437
/**
@@ -89,7 +102,9 @@ export function getInternalApiBaseUrl(): string {
89102
)
90103
}
91104

92-
return internalBaseUrl
105+
// Protocol is proven present above, so this only trims trailing slashes —
106+
// callers concatenate `${base}/api/...` exactly as they do with getBaseUrl().
107+
return normalizeBaseUrl(internalBaseUrl)
93108
}
94109

95110
/**

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 the relative callback URL Better Auth documents and 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

0 commit comments

Comments
 (0)