diff --git a/apps/sim/app/desktop/connect/connect-launcher.tsx b/apps/sim/app/desktop/connect/connect-launcher.tsx index c1c8bfde4a4..fdb6276aea3 100644 --- a/apps/sim/app/desktop/connect/connect-launcher.tsx +++ b/apps/sim/app/desktop/connect/connect-launcher.tsx @@ -8,8 +8,13 @@ import { DesktopHandoffShell } from '@/app/desktop/components/desktop-handoff-sh interface ConnectLauncherProps { providerId: string - /** Same-origin path better-auth returns the browser to after the callback. */ - completePath: string + /** + * Absolute URL better-auth returns the browser to after the callback. Better + * Auth stores it verbatim in the OAuth state and the callback reads the + * credential draft back off it, so a bare path would be parsed without an + * origin — keep this a full URL, as every other connect surface passes. + */ + completeUrl: string } /** @@ -19,7 +24,7 @@ interface ConnectLauncherProps { * leaves for the provider immediately, so the UI is just a brief interstitial * plus an error state with retry. */ -export function ConnectLauncher({ providerId, completePath }: ConnectLauncherProps) { +export function ConnectLauncher({ providerId, completeUrl }: ConnectLauncherProps) { const startedRef = useRef(false) const [error, setError] = useState(null) @@ -28,18 +33,18 @@ export function ConnectLauncher({ providerId, completePath }: ConnectLauncherPro try { await client.oauth2.link({ providerId, - callbackURL: completePath, + callbackURL: completeUrl, // Failed flows bounce to the same complete page (which forwards the // failure to the loopback) instead of waiting out the handoff TTL. // Do NOT bake in a query param here: better-auth appends its own // `&error=`, and a second `error` key deserializes to an array // that the complete page can't read — so it would look like success. - errorCallbackURL: completePath, + errorCallbackURL: completeUrl, }) } catch (err) { setError(getErrorMessage(err, 'Could not start the connection.')) } - }, [providerId, completePath]) + }, [providerId, completeUrl]) useEffect(() => { if (startedRef.current) return diff --git a/apps/sim/app/desktop/connect/page.test.tsx b/apps/sim/app/desktop/connect/page.test.tsx new file mode 100644 index 00000000000..d44f769d668 --- /dev/null +++ b/apps/sim/app/desktop/connect/page.test.tsx @@ -0,0 +1,160 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockGetSession, mockRedirect, baseUrl } = vi.hoisted(() => ({ + mockGetSession: vi.fn(), + mockRedirect: vi.fn((url: string) => { + throw new Error(`NEXT_REDIRECT:${url}`) + }), + /** Mutable so a test can give the deployment a trailing-slash base URL. */ + baseUrl: { value: 'https://sim.test' }, +})) + +vi.mock('@/lib/auth', () => ({ + auth: { api: { getSession: mockGetSession } }, + getSession: vi.fn(), +})) + +vi.mock('@/lib/auth/auth-client', () => ({ + client: { oauth2: { link: vi.fn() } }, + signOut: vi.fn(), +})) + +vi.mock('@/lib/core/utils/urls', () => ({ + getBaseUrl: () => baseUrl.value, +})) + +/** Keeps the landing-page barrel the real shell pulls in out of this graph. */ +vi.mock('@/app/desktop/components/desktop-handoff-shell', () => ({ + DesktopHandoffShell: () => null, +})) + +vi.mock('next/navigation', () => ({ + redirect: mockRedirect, +})) + +vi.mock('next/headers', () => ({ + headers: vi.fn(async () => new Headers()), +})) + +import DesktopConnectPage from '@/app/desktop/connect/page' + +const VALID_STATE = 'a'.repeat(32) +const PORT = '57979' + +function pageProps(params: Record) { + return { searchParams: Promise.resolve(params) } +} + +async function renderPage(params: Record) { + const result = (await DesktopConnectPage(pageProps(params))) as unknown as { + type: { name: string } + props: Record + } + return result +} + +describe('DesktopConnectPage', () => { + beforeEach(() => { + vi.clearAllMocks() + baseUrl.value = 'https://sim.test' + mockGetSession.mockResolvedValue({ user: { id: 'user-1', email: 'user@example.com' } }) + }) + + it('hands the launcher an absolute complete URL so the callback can read the draft back', async () => { + // Better Auth stores `callbackURL` verbatim, and the OAuth callback parses it + // with `new URL`. A bare path threw there, failing the whole callback with a + // 500 after the provider had already authorized. + const result = await renderPage({ + provider: 'google-email', + state: VALID_STATE, + port: PORT, + draftId: 'draft-1', + }) + + expect(result.type.name).toBe('ConnectLauncher') + expect(result.props.providerId).toBe('google-email') + + const completeUrl = new URL(result.props.completeUrl as string) + expect(completeUrl.origin).toBe('https://sim.test') + expect(completeUrl.pathname).toBe('/desktop/connect/complete') + expect(completeUrl.searchParams.get('state')).toBe(VALID_STATE) + expect(completeUrl.searchParams.get('port')).toBe(PORT) + expect(completeUrl.searchParams.get('credentialDraftId')).toBe('draft-1') + }) + + it('keeps the complete URL absolute when no draft rides along', async () => { + const result = await renderPage({ + provider: 'google-email', + state: VALID_STATE, + port: PORT, + }) + + expect(result.type.name).toBe('ConnectLauncher') + expect(() => new URL(result.props.completeUrl as string)).not.toThrow() + }) + + it('keeps the completion route intact when the deployment base URL has a trailing slash', async () => { + // `//desktop/connect/complete` matches no route, so the provider result + // would never reach the loopback and the connect would hang. + baseUrl.value = 'https://sim.test/' + + const launcher = await renderPage({ + provider: 'google-email', + state: VALID_STATE, + port: PORT, + }) + expect(new URL(launcher.props.completeUrl as string).pathname).toBe('/desktop/connect/complete') + + await expect( + DesktopConnectPage( + pageProps({ + provider: 'google-email', + state: VALID_STATE, + port: PORT, + workspaceId: 'workspace-1', + }) + ) + ).rejects.toThrow('NEXT_REDIRECT:') + const callbackUrl = new URL(mockRedirect.mock.calls[0][0]).searchParams.get('callbackURL') + expect(new URL(callbackUrl as string).pathname).toBe('/desktop/connect/complete') + }) + + it('sends a workspace-scoped connect to the authorize route with an absolute callback', async () => { + await expect( + DesktopConnectPage( + pageProps({ + provider: 'google-email', + state: VALID_STATE, + port: PORT, + workspaceId: 'workspace-1', + }) + ) + ).rejects.toThrow('NEXT_REDIRECT:') + + const authorize = new URL(mockRedirect.mock.calls[0][0]) + expect(authorize.pathname).toBe('/api/auth/oauth2/authorize') + expect(authorize.searchParams.get('providerId')).toBe('google-email') + expect(authorize.searchParams.get('workspaceId')).toBe('workspace-1') + expect(authorize.searchParams.get('callbackURL')).toBe( + `https://sim.test/desktop/connect/complete?state=${VALID_STATE}&port=${PORT}` + ) + }) + + it('rejects a malformed request without reading the session', async () => { + const invalid = [ + { provider: 'Google', state: VALID_STATE, port: PORT }, + { provider: 'google-email', state: 'short', port: PORT }, + { provider: 'google-email', state: VALID_STATE }, + { provider: 'google-email', state: VALID_STATE, port: PORT, draftId: 'bad draft' }, + ] + + for (const params of invalid) { + const result = await renderPage(params) + expect(result.type.name).toBe('InvalidRequest') + } + expect(mockGetSession).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/desktop/connect/page.tsx b/apps/sim/app/desktop/connect/page.tsx index 4e473134b6b..9110d733599 100644 --- a/apps/sim/app/desktop/connect/page.tsx +++ b/apps/sim/app/desktop/connect/page.tsx @@ -34,6 +34,17 @@ function InvalidRequest() { ) } +/** + * Absolute URL better-auth returns the browser to once the OAuth callback is + * done. Composed through the URL API rather than concatenated, so a trailing + * slash on `NEXT_PUBLIC_APP_URL` cannot yield a `//desktop/...` pathname that + * matches no route — this page is what bounces the result to the app's + * loopback, so a base-URL typo would otherwise strand the whole flow. + */ +function buildConnectCompleteUrl(state: string, port: number, draftId?: string): string { + return new URL(buildConnectCompletePath(state, port, draftId), getBaseUrl()).toString() +} + /** * Desktop OAuth-connect landing. The desktop app opens this page in the * 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 const authorize = new URL('/api/auth/oauth2/authorize', getBaseUrl()) authorize.searchParams.set('providerId', providerId) authorize.searchParams.set('workspaceId', workspaceId) - authorize.searchParams.set( - 'callbackURL', - `${getBaseUrl()}${buildConnectCompletePath(state, port)}` - ) + authorize.searchParams.set('callbackURL', buildConnectCompleteUrl(state, port)) if (credentialId) { authorize.searchParams.set('credentialId', credentialId) } @@ -125,7 +133,7 @@ export default async function DesktopConnectPage({ searchParams }: DesktopConnec return ( ) } diff --git a/apps/sim/lib/core/utils/internal-api-base-url.test.ts b/apps/sim/lib/core/utils/internal-api-base-url.test.ts index 4b346111a0c..dff09ac175e 100644 --- a/apps/sim/lib/core/utils/internal-api-base-url.test.ts +++ b/apps/sim/lib/core/utils/internal-api-base-url.test.ts @@ -12,7 +12,15 @@ * @vitest-environment node */ import { resetEnvMock, setEnv } from '@sim/testing' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' + +/** + * `vitest.setup.ts` mocks this module globally with a hand-written mirror, so + * without this the suite would assert against that mirror rather than the + * function it names — and any drift between the two would pass unnoticed. + */ +vi.unmock('@/lib/core/utils/urls') + import { getInternalApiBaseUrl } from '@/lib/core/utils/urls' const PUBLIC_URL = 'https://sim.ai' @@ -33,6 +41,17 @@ describe('getInternalApiBaseUrl', () => { expect(getInternalApiBaseUrl()).toBe(LOOPBACK) }) + /** Callers concatenate `${base}/api/...`, exactly as they do with getBaseUrl(). */ + it('strips a trailing slash from the internal URL', () => { + setEnv({ + INTERNAL_API_BASE_URL: `${LOOPBACK}/`, + NEXT_PUBLIC_APP_URL: PUBLIC_URL, + DB_APP_NAME: 'sim', + }) + + expect(getInternalApiBaseUrl()).toBe(LOOPBACK) + }) + it('IGNORES the internal URL on a Trigger.dev worker and falls back to the public URL', () => { setEnv({ INTERNAL_API_BASE_URL: LOOPBACK, diff --git a/apps/sim/lib/core/utils/urls.test.ts b/apps/sim/lib/core/utils/urls.test.ts index bcbe797df6e..0f472f3d9a9 100644 --- a/apps/sim/lib/core/utils/urls.test.ts +++ b/apps/sim/lib/core/utils/urls.test.ts @@ -56,6 +56,43 @@ describe('getBaseUrl', () => { expect(getBaseUrl()).toBe('https://app.example.com') }) + /** + * Call sites build `${getBaseUrl()}/path`, so a trailing slash would give them + * a `//path` pathname that matches no route — and would break the + * `startsWith(`${base}/`)` prefix checks that decide whether a redirect target + * is our own, silently sending those redirects to their fallback instead. + */ + it('strips trailing slashes so concatenated paths stay single-slashed', () => { + for (const configured of ['https://app.example.com/', 'https://app.example.com///']) { + mockGetEnv.mockImplementation((key) => + key === 'NEXT_PUBLIC_APP_URL' ? configured : undefined + ) + expect(getBaseUrl()).toBe('https://app.example.com') + expect(new URL(`${getBaseUrl()}/desktop/connect/complete`).pathname).toBe( + '/desktop/connect/complete' + ) + } + }) + + /** + * Pins the trim's shape — it must not eat more than the trailing slashes. + * Not a claim that a path-prefixed deployment works: the app declares no Next + * `basePath`, so such a value could not address its routes either way. + */ + it('trims only trailing slashes, never interior ones', () => { + mockGetEnv.mockImplementation((key) => + key === 'NEXT_PUBLIC_APP_URL' ? 'https://example.com/a/b/' : undefined + ) + expect(getBaseUrl()).toBe('https://example.com/a/b') + }) + + it('adds the protocol and strips the trailing slash together', () => { + mockGetEnv.mockImplementation((key) => + key === 'NEXT_PUBLIC_APP_URL' ? 'app.example.com/' : undefined + ) + expect(getBaseUrl()).toBe('http://app.example.com') + }) + /** * Never guesses from `window.location.origin`: an opaque origin (a sandboxed * iframe) serializes to the truthy string `'null'`, which would silently diff --git a/apps/sim/lib/core/utils/urls.ts b/apps/sim/lib/core/utils/urls.ts index 8db88438988..08d6a9c7673 100644 --- a/apps/sim/lib/core/utils/urls.ts +++ b/apps/sim/lib/core/utils/urls.ts @@ -12,13 +12,26 @@ function hasHttpProtocol(url: string): boolean { return /^https?:\/\//i.test(url) } +/** + * Brings a configured base URL to the no-trailing-slash form {@link SITE_URL} + * documents: adds the protocol when the operator omitted it, then strips + * trailing slashes. + * + * Call sites overwhelmingly build URLs as `${base}/path`, so a base spelled + * `https://host/` gives every one of them a `//path` pathname that matches no + * route, and breaks the `startsWith(`${base}/`)` prefix checks that decide + * whether a redirect target is our own. Normalizing once here is what lets + * those call sites stay simple instead of each defending against the operator's + * spelling. + * + * Trailing slashes are the only spelling this absorbs. The app declares no Next + * `basePath`, so its routes are served at the origin root and a path-prefixed + * value could not address them however this normalized it. + */ function normalizeBaseUrl(url: string): string { - if (hasHttpProtocol(url)) { - return url - } - const protocol = isProd ? 'https://' : 'http://' - return `${protocol}${url}` + const withProtocol = hasHttpProtocol(url) ? url : `${protocol}${url}` + return withProtocol.replace(/\/+$/, '') } /** @@ -89,7 +102,9 @@ export function getInternalApiBaseUrl(): string { ) } - return internalBaseUrl + // Protocol is proven present above, so this only trims trailing slashes — + // callers concatenate `${base}/api/...` exactly as they do with getBaseUrl(). + return normalizeBaseUrl(internalBaseUrl) } /** diff --git a/apps/sim/lib/credentials/draft-processor.test.ts b/apps/sim/lib/credentials/draft-processor.test.ts index 00401d165d4..579f02375c4 100644 --- a/apps/sim/lib/credentials/draft-processor.test.ts +++ b/apps/sim/lib/credentials/draft-processor.test.ts @@ -119,11 +119,23 @@ describe('parseCredentialDraftIdFromCallbackUrl', () => { ).toBe('draft-1') }) + it('reads the relative callback URL Better Auth documents and stores verbatim', () => { + expect( + parseCredentialDraftIdFromCallbackUrl( + '/desktop/connect/complete?state=abc&port=57979&credentialDraftId=draft-1' + ) + ).toBe('draft-1') + expect( + parseCredentialDraftIdFromCallbackUrl('/desktop/connect/complete?state=abc&port=57979') + ).toBeUndefined() + }) + it('fails closed for malformed or non-string callback state', () => { expect(() => parseCredentialDraftIdFromCallbackUrl({})).toThrow( 'OAuth state callback URL must be a string' ) expect(() => parseCredentialDraftIdFromCallbackUrl('not a URL')).toThrow() + expect(() => parseCredentialDraftIdFromCallbackUrl('//elsewhere.test/path')).toThrow() }) }) diff --git a/apps/sim/lib/credentials/draft-processor.ts b/apps/sim/lib/credentials/draft-processor.ts index c375dd8d48c..e49f1214787 100644 --- a/apps/sim/lib/credentials/draft-processor.ts +++ b/apps/sim/lib/credentials/draft-processor.ts @@ -25,13 +25,40 @@ type AvailableOAuthCredentialDraftBinding = Extract< const oauthCredentialDraftBindings = new WeakMap() -/** Extracts a draft binding from Better Auth state and rejects malformed callback state. */ +/** + * Base a path-absolute callback URL is resolved against. Only the query string + * is ever read, so the origin reaches nothing — an RFC 2606 `.invalid` host says + * so at a glance, and keeps this parse independent of `NEXT_PUBLIC_APP_URL`, + * whose absence would otherwise turn a state read into a configuration throw. + */ +const CALLBACK_URL_RESOLUTION_BASE = 'http://callback.invalid' + +/** + * Extracts a draft binding from Better Auth state and rejects malformed callback state. + * + * Better Auth documents `callbackURL` as a reference relative to the app + * (`/dashboard`) and stores whatever it is handed verbatim, so a path and a full + * URL are equally legitimate — these two shapes are what is accepted. Bare + * `new URL()` rejects the path form, and because this runs inside the + * `account.create.before` database hook, which Better Auth's OAuth callback does + * not guard, that rejection surfaced as a 500 on the callback rather than a + * failed connection. + * + * A network-path reference (`//host/path`, RFC 3986 §4.2) is not a path and + * still throws, as does anything else malformed: a callback URL we cannot read + * must stay loud rather than read as "carried no draft", which would fall back + * to guessing the draft from the user and provider alone. + */ export function parseCredentialDraftIdFromCallbackUrl(callbackUrl: unknown): string | undefined { if (callbackUrl === undefined) return undefined if (typeof callbackUrl !== 'string') { throw new Error('OAuth state callback URL must be a string') } - return new URL(callbackUrl).searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ?? undefined + const isPathAbsolute = callbackUrl.startsWith('/') && !callbackUrl.startsWith('//') + const url = isPathAbsolute + ? new URL(callbackUrl, CALLBACK_URL_RESOLUTION_BASE) + : new URL(callbackUrl) + return url.searchParams.get(OAUTH_CREDENTIAL_DRAFT_CALLBACK_PARAM) ?? undefined } /** Reads an exact draft binding without falling back when OAuth state is unavailable. */ diff --git a/packages/testing/src/mocks/urls.mock.ts b/packages/testing/src/mocks/urls.mock.ts index 8889ee23860..1daf6540f94 100644 --- a/packages/testing/src/mocks/urls.mock.ts +++ b/packages/testing/src/mocks/urls.mock.ts @@ -24,6 +24,17 @@ function hasHttpProtocol(url: string): boolean { return /^https?:\/\//i.test(url) } +/** + * Mirrors the real module's `normalizeBaseUrl`: protocol-less values get + * https:// under isProd, then trailing slashes are stripped so `${base}/path` + * stays single-slashed at every call site. + */ +function normalizeBaseUrl(url: string): string { + const protocol = envFlagsMock.isProd ? 'https://' : 'http://' + const withProtocol = hasHttpProtocol(url) ? url : `${protocol}${url}` + return withProtocol.replace(/\/+$/, '') +} + function getBaseUrlImpl(): string { const baseUrl = readEnv('NEXT_PUBLIC_APP_URL')?.trim() if (!baseUrl) { @@ -31,9 +42,7 @@ function getBaseUrlImpl(): string { 'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly' ) } - // Mirrors the real module: protocol-less values get https:// under isProd. - const protocol = envFlagsMock.isProd ? 'https://' : 'http://' - return hasHttpProtocol(baseUrl) ? baseUrl : `${protocol}${baseUrl}` + return normalizeBaseUrl(baseUrl) } function getInternalApiBaseUrlImpl(): string { @@ -47,7 +56,7 @@ function getInternalApiBaseUrlImpl(): string { 'INTERNAL_API_BASE_URL must include protocol (http:// or https://), e.g. http://sim-app.default.svc.cluster.local:3000' ) } - return internalBaseUrl + return normalizeBaseUrl(internalBaseUrl) } function ensureAbsoluteUrlImpl(pathOrUrl: string): string {