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>
2026-08-31 01:11:53 +08:00 · 2026-08-22 20:28:00 -07:00
parent bbf408bf30
commit 43aba98527
9 changed files with 316 additions and 24 deletions
@@ -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<string | null>(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=<code>`, 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
@@ -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<string, string>) {
+  return { searchParams: Promise.resolve(params) }
+}
+
+async function renderPage(params: Record<string, string>) {
+  const result = (await DesktopConnectPage(pageProps(params))) as unknown as {
+    type: { name: string }
+    props: Record<string, unknown>
+  }
+  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()
+  })
+})
@@ -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 (
    <ConnectLauncher
      providerId={providerId}
-      completePath={buildConnectCompletePath(state, port, draftId)}
+      completeUrl={buildConnectCompleteUrl(state, port, draftId)}
    />
  )
 }
@@ -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,
@@ -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
@@ -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)
 }

 /**
@@ -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()
  })
 })

@@ -25,13 +25,40 @@ type AvailableOAuthCredentialDraftBinding = Extract<

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

-/** 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. */
@@ -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 {