Skip to content

Commit 36aace7

Browse files
authored
fix(auth): correct callback URL resolution across SSR and hydration (#6217)
* fix(sso): derive the SSO callback URL during render `callbackUrl` was seeded into `useState('/workspace')` and overwritten from a `useEffect` that read `searchParams`, so the first painted frame always carried the default. On any deep link with `?callbackUrl=`, the "Sign in with email" and "Sign up" links briefly pointed at `/workspace` instead of the requested destination, and a click landing in that window navigated to the wrong place. - derive `callbackUrl` from `searchParams` during render; the validation gate is unchanged, so an off-origin or malformed value still falls back to `/workspace` - keep the warning for a rejected value in an effect, now keyed on the param itself rather than the `searchParams` object, so it fires once per actual change instead of once per identity change - add first-frame tests via `renderToString`, which runs no effects and so pins exactly the window the old code got wrong * fix(auth): resolve callback URLs against the app origin server-side `validateCallbackUrl` compared against a sentinel base (`https://callback-url-validator.invalid`) when `window` was undefined, so the server rejected every absolute URL — including the same-origin ones the function documents as valid. A component deriving a callback URL during render therefore produced one destination in the SSR markup and a different one after hydration. The exposure was not new to the SSO form: `login-form.tsx` and `signup-form.tsx` already derive their callback URL during render on `force-dynamic` pages, so both carried the same divergence. - resolve against the deployment's own origin server-side, so the server reaches the same verdict the browser will after hydration - fall back to the sentinel when the app URL is unset or unparseable, which keeps the server fail-closed: absolute URLs are rejected, as before - cover the absolute same-origin case and the unset-app-URL fallback in the existing suite; all 15 open-redirect rejection cases are unchanged * fix(env): drop the getBaseUrl browser-origin fallback The fallback was added in #6214 as a safety net while the real cause — the hosted env script losing its `beforeInteractive` strategy — was fixed in the same PR. With the injection ordering restored, `window.__ENV` is populated before hydration, so the fallback is unreachable in any correctly configured deployment. Guessing the origin was also unsafe in the one case it could still fire. An opaque origin — a sandboxed iframe, and `/chat/*` is deliberately embeddable — serializes to the string `'null'`, which is truthy, so `getBaseUrl()` would have returned `'null'` and every call site would have silently built `null/api/...`. A throw surfaces the misconfiguration instead of encoding it into request URLs. - restore the unconditional throw when NEXT_PUBLIC_APP_URL is unset or blank - mirror it back in the shared testing mock - flip the two fallback tests to assert the throw, keeping whitespace-only coverage Server-side behavior is unchanged: there was never a `window` to fall back to, so callers that already guard `getBaseUrl()` (`getBaseDomain`, `validateCallbackUrl`) keep their existing fail-closed paths.
1 parent 09eba8a commit 36aace7

7 files changed

Lines changed: 169 additions & 44 deletions

File tree

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import type { ReactNode } from 'react'
5+
import { renderToString } from 'react-dom/server'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
const { mockUseSearchParams } = vi.hoisted(() => ({
9+
mockUseSearchParams: vi.fn(),
10+
}))
11+
12+
vi.mock('next/navigation', () => ({
13+
useRouter: () => ({ push: vi.fn(), replace: vi.fn() }),
14+
useSearchParams: mockUseSearchParams,
15+
}))
16+
17+
vi.mock('next/link', () => ({
18+
default: ({ href, children }: { href: string; children?: ReactNode }) => (
19+
<a href={href}>{children}</a>
20+
),
21+
}))
22+
23+
vi.mock('@sim/emcn', () => ({
24+
Button: ({ children }: { children?: ReactNode }) => <button type='button'>{children}</button>,
25+
Input: () => <input />,
26+
Label: ({ children }: { children?: ReactNode }) => <span>{children}</span>,
27+
cn: (...values: unknown[]) => values.filter(Boolean).join(' '),
28+
}))
29+
30+
vi.mock('@/lib/auth/auth-client', () => ({
31+
client: { signIn: { sso: vi.fn() } },
32+
}))
33+
34+
vi.mock('@/app/(auth)/components', () => ({
35+
AuthSubmitButton: ({ children }: { children?: ReactNode }) => (
36+
<button type='submit'>{children}</button>
37+
),
38+
}))
39+
40+
vi.mock('@/lib/core/config/env', () => ({
41+
getEnv: () => 'true',
42+
isFalsy: (value: unknown) => value === undefined || value === 'false',
43+
}))
44+
45+
import SSOForm from '@/ee/sso/components/sso-form'
46+
47+
function renderFirstFrame(search: string): string {
48+
mockUseSearchParams.mockReturnValue(new URLSearchParams(search))
49+
return renderToString(<SSOForm />)
50+
}
51+
52+
/**
53+
* `renderToString` produces the markup of the first frame with no effects run,
54+
* which is exactly the window in which a callback URL seeded from an effect is
55+
* still the `/workspace` default.
56+
*/
57+
describe('SSOForm callback URL', () => {
58+
beforeEach(() => {
59+
mockUseSearchParams.mockReset()
60+
})
61+
62+
it('carries a valid callbackUrl on the first rendered frame', () => {
63+
const html = renderFirstFrame('callbackUrl=/workspace/abc/w/xyz')
64+
65+
expect(html).toContain(encodeURIComponent('/workspace/abc/w/xyz'))
66+
})
67+
68+
it('falls back to /workspace when no callbackUrl is present', () => {
69+
const html = renderFirstFrame('')
70+
71+
expect(html).toContain(`/login?callbackUrl=${encodeURIComponent('/workspace')}`)
72+
})
73+
74+
it('rejects an off-origin callbackUrl and falls back to /workspace', () => {
75+
const html = renderFirstFrame('callbackUrl=https://evil.example.com/steal')
76+
77+
expect(html).not.toContain('evil.example.com')
78+
expect(html).toContain(`/login?callbackUrl=${encodeURIComponent('/workspace')}`)
79+
})
80+
})

apps/sim/ee/sso/components/sso-form.tsx

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,21 +36,27 @@ export default function SSOForm() {
3636
const [email, setEmail] = useState('')
3737
const [emailErrors, setEmailErrors] = useState<string[]>([])
3838
const [showEmailValidationError, setShowEmailValidationError] = useState(false)
39-
const [callbackUrl, setCallbackUrl] = useState('/workspace')
4039

4140
const emailEnabled = !isFalsy(getEnv('NEXT_PUBLIC_EMAIL_PASSWORD_SIGNUP_ENABLED'))
4241

42+
/**
43+
* Derived during render rather than seeded into state from an effect: the
44+
* first painted frame otherwise carries the `/workspace` default, so the
45+
* "Sign in with email" and "Sign up" links briefly point at the wrong
46+
* destination on any deep link carrying `?callbackUrl=`.
47+
*/
48+
const callbackParam = searchParams?.get('callbackUrl') ?? null
49+
const isCallbackValid = callbackParam !== null && validateCallbackUrl(callbackParam)
50+
const callbackUrl = callbackParam !== null && isCallbackValid ? callbackParam : '/workspace'
51+
4352
useEffect(() => {
44-
if (searchParams) {
45-
const callback = searchParams.get('callbackUrl')
46-
if (callback) {
47-
if (validateCallbackUrl(callback)) {
48-
setCallbackUrl(callback)
49-
} else {
50-
logger.warn('Invalid callback URL detected and blocked:', { url: callback })
51-
}
52-
}
53+
if (callbackParam !== null && !isCallbackValid) {
54+
logger.warn('Invalid callback URL detected and blocked:', { url: callbackParam })
55+
}
56+
}, [callbackParam, isCallbackValid])
5357

58+
useEffect(() => {
59+
if (searchParams) {
5460
const emailParam = searchParams.get('email')
5561
if (emailParam) {
5662
setEmail(emailParam)

apps/sim/lib/core/security/input-validation.test.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { envFlagsMock, resetEnvFlagsMock } from '@sim/testing'
1+
import { defaultMockEnv, envFlagsMock, resetEnvFlagsMock, resetEnvMock, setEnv } from '@sim/testing'
22
import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest'
33
import {
44
validateAirtableId,
@@ -2136,6 +2136,7 @@ describe('validateCallbackUrl', () => {
21362136
})
21372137

21382138
afterEach(() => {
2139+
resetEnvMock()
21392140
if (originalWindow === undefined) {
21402141
;(globalThis as { window?: unknown }).window = undefined
21412142
} else {
@@ -2187,12 +2188,28 @@ describe('validateCallbackUrl', () => {
21872188
;(globalThis as { window?: unknown }).window = undefined
21882189
})
21892190

2190-
it('falls back to placeholder origin and still rejects cross-origin URLs', () => {
2191+
it('resolves against the configured app origin and still rejects cross-origin URLs', () => {
21912192
expect(validateCallbackUrl('/workspace')).toBe(true)
21922193
expect(validateCallbackUrl('//evil.com')).toBe(false)
21932194
expect(validateCallbackUrl('https://evil.com')).toBe(false)
21942195
expect(validateCallbackUrl('javascript:alert(1)')).toBe(false)
21952196
})
2197+
2198+
/**
2199+
* The server verdict has to match what the browser will decide once it
2200+
* hydrates, or a callback URL derived during render yields one destination
2201+
* in the SSR markup and another after hydration.
2202+
*/
2203+
it('accepts an absolute same-origin URL, matching the browser verdict', () => {
2204+
expect(validateCallbackUrl(`${defaultMockEnv.NEXT_PUBLIC_APP_URL}/workspace/abc`)).toBe(true)
2205+
})
2206+
2207+
it('stays fail-closed on absolute URLs when the app URL is unset', () => {
2208+
setEnv({ NEXT_PUBLIC_APP_URL: undefined })
2209+
2210+
expect(validateCallbackUrl(`${defaultMockEnv.NEXT_PUBLIC_APP_URL}/workspace/abc`)).toBe(false)
2211+
expect(validateCallbackUrl('/workspace')).toBe(true)
2212+
})
21962213
})
21972214
})
21982215

apps/sim/lib/core/security/input-validation.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger'
22
import { isLoopbackIp, isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf'
33
import * as ipaddr from 'ipaddr.js'
44
import { isHosted } from '@/lib/core/config/env-flags'
5+
import { getBaseUrl } from '@/lib/core/utils/urls'
56

67
const logger = createLogger('InputValidation')
78

@@ -1234,6 +1235,31 @@ export function validatePaginationCursor(
12341235

12351236
const CALLBACK_URL_SERVER_BASE = 'https://callback-url-validator.invalid'
12361237

1238+
/**
1239+
* Origin a callback URL is resolved and compared against.
1240+
*
1241+
* The browser uses its own origin. Server-side there is no `window`, so it uses
1242+
* the deployment's configured origin — which is what the browser will compare
1243+
* against once it hydrates. Using a sentinel here instead made the server reject
1244+
* every absolute URL, including the same-origin ones this function documents as
1245+
* valid, so a component deriving a callback URL during render produced one
1246+
* destination in the SSR markup and a different one after hydration.
1247+
*
1248+
* Falls back to the sentinel when the app URL is unset or unparseable, which
1249+
* keeps the server fail-closed: every absolute URL is rejected, as before.
1250+
*/
1251+
function getCallbackValidationOrigin(): string {
1252+
if (typeof window !== 'undefined') {
1253+
return window.location.origin
1254+
}
1255+
1256+
try {
1257+
return new URL(getBaseUrl()).origin
1258+
} catch {
1259+
return CALLBACK_URL_SERVER_BASE
1260+
}
1261+
}
1262+
12371263
/**
12381264
* Validates a callback URL to prevent open redirect attacks.
12391265
*
@@ -1263,7 +1289,7 @@ export function validateCallbackUrl(url: string): boolean {
12631289
try {
12641290
if (typeof url !== 'string' || url.length === 0) return false
12651291

1266-
const base = typeof window === 'undefined' ? CALLBACK_URL_SERVER_BASE : window.location.origin
1292+
const base = getCallbackValidationOrigin()
12671293
const parsed = new URL(url, base)
12681294
return parsed.origin === base
12691295
} catch (error) {

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

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

59-
it('falls back to the page origin instead of throwing when the injected env is missing', () => {
59+
/**
60+
* Never guesses from `window.location.origin`: an opaque origin (a sandboxed
61+
* iframe) serializes to the truthy string `'null'`, which would silently
62+
* produce `null/api/...` rather than surfacing the misconfiguration.
63+
*/
64+
it('throws in the browser rather than guessing from the page origin', () => {
6065
setLocation('https://www.sim.ai/workspace/ws-1/w/wf-1')
61-
expect(getBaseUrl()).toBe('https://www.sim.ai')
66+
expect(() => getBaseUrl()).toThrow('NEXT_PUBLIC_APP_URL must be configured')
6267
})
6368

6469
it('treats a whitespace-only NEXT_PUBLIC_APP_URL as unset', () => {
6570
mockGetEnv.mockImplementation((key) => (key === 'NEXT_PUBLIC_APP_URL' ? ' ' : undefined))
6671
setLocation('https://www.sim.ai/')
67-
expect(getBaseUrl()).toBe('https://www.sim.ai')
72+
expect(() => getBaseUrl()).toThrow('NEXT_PUBLIC_APP_URL must be configured')
6873
})
6974
})
7075

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

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,31 +25,26 @@ function normalizeBaseUrl(url: string): string {
2525
* Returns the base URL of the application from NEXT_PUBLIC_APP_URL
2626
* This ensures webhooks, callbacks, and other integrations always use the correct public URL
2727
*
28-
* In the browser, falls back to the page's own origin when the injected env is
29-
* unavailable. Client-side callers only ever want a URL back to the app they are
30-
* already served from, so the origin is a correct answer — and a throw here
31-
* during render tears down the whole page through the error boundary. Server-side
32-
* callers (webhooks, callbacks, emails) have no origin to fall back to and must
33-
* still fail loudly on a misconfigured deployment.
28+
* Deliberately has no browser fallback to `window.location.origin`. The value is
29+
* injected before hydration by `<PublicEnvScript>`, so an empty read means the
30+
* deployment is misconfigured — and a same-origin guess would hide that. It also
31+
* would not be safe to guess: an opaque origin (a sandboxed iframe, and `/chat/*`
32+
* is embeddable) serializes to the string `'null'`, which is truthy and would
33+
* silently produce `null/api/...` at every call site.
3434
*
3535
* @returns The base URL string (e.g., 'http://localhost:3000' or 'https://example.com')
36-
* @throws Error if NEXT_PUBLIC_APP_URL is not configured and no browser origin exists
36+
* @throws Error if NEXT_PUBLIC_APP_URL is not configured
3737
*/
3838
export function getBaseUrl(): string {
3939
const baseUrl = getEnv('NEXT_PUBLIC_APP_URL')?.trim()
4040

41-
if (baseUrl) {
42-
return normalizeBaseUrl(baseUrl)
43-
}
44-
45-
const browserOrigin = getBrowserOrigin()
46-
if (browserOrigin) {
47-
return browserOrigin
41+
if (!baseUrl) {
42+
throw new Error(
43+
'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly'
44+
)
4845
}
4946

50-
throw new Error(
51-
'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly'
52-
)
47+
return normalizeBaseUrl(baseUrl)
5348
}
5449

5550
/**

packages/testing/src/mocks/urls.mock.ts

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,14 @@ function hasHttpProtocol(url: string): boolean {
2626

2727
function getBaseUrlImpl(): string {
2828
const baseUrl = readEnv('NEXT_PUBLIC_APP_URL')?.trim()
29-
if (baseUrl) {
30-
// Mirrors the real module: protocol-less values get https:// under isProd.
31-
const protocol = envFlagsMock.isProd ? 'https://' : 'http://'
32-
return hasHttpProtocol(baseUrl) ? baseUrl : `${protocol}${baseUrl}`
29+
if (!baseUrl) {
30+
throw new Error(
31+
'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly'
32+
)
3333
}
34-
// Mirrors the real module: the browser falls back to its own origin, only
35-
// server-side (no `window`) callers throw.
36-
const browserOrigin = getBrowserOriginImpl()
37-
if (browserOrigin) return browserOrigin
38-
throw new Error(
39-
'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly'
40-
)
34+
// Mirrors the real module: protocol-less values get https:// under isProd.
35+
const protocol = envFlagsMock.isProd ? 'https://' : 'http://'
36+
return hasHttpProtocol(baseUrl) ? baseUrl : `${protocol}${baseUrl}`
4137
}
4238

4339
function getInternalApiBaseUrlImpl(): string {

0 commit comments

Comments
 (0)