diff --git a/apps/sim/ee/sso/components/sso-form.test.tsx b/apps/sim/ee/sso/components/sso-form.test.tsx new file mode 100644 index 00000000000..cd1dc712c46 --- /dev/null +++ b/apps/sim/ee/sso/components/sso-form.test.tsx @@ -0,0 +1,80 @@ +/** + * @vitest-environment jsdom + */ +import type { ReactNode } from 'react' +import { renderToString } from 'react-dom/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockUseSearchParams } = vi.hoisted(() => ({ + mockUseSearchParams: vi.fn(), +})) + +vi.mock('next/navigation', () => ({ + useRouter: () => ({ push: vi.fn(), replace: vi.fn() }), + useSearchParams: mockUseSearchParams, +})) + +vi.mock('next/link', () => ({ + default: ({ href, children }: { href: string; children?: ReactNode }) => ( + {children} + ), +})) + +vi.mock('@sim/emcn', () => ({ + Button: ({ children }: { children?: ReactNode }) => , + Input: () => , + Label: ({ children }: { children?: ReactNode }) => {children}, + cn: (...values: unknown[]) => values.filter(Boolean).join(' '), +})) + +vi.mock('@/lib/auth/auth-client', () => ({ + client: { signIn: { sso: vi.fn() } }, +})) + +vi.mock('@/app/(auth)/components', () => ({ + AuthSubmitButton: ({ children }: { children?: ReactNode }) => ( + + ), +})) + +vi.mock('@/lib/core/config/env', () => ({ + getEnv: () => 'true', + isFalsy: (value: unknown) => value === undefined || value === 'false', +})) + +import SSOForm from '@/ee/sso/components/sso-form' + +function renderFirstFrame(search: string): string { + mockUseSearchParams.mockReturnValue(new URLSearchParams(search)) + return renderToString() +} + +/** + * `renderToString` produces the markup of the first frame with no effects run, + * which is exactly the window in which a callback URL seeded from an effect is + * still the `/workspace` default. + */ +describe('SSOForm callback URL', () => { + beforeEach(() => { + mockUseSearchParams.mockReset() + }) + + it('carries a valid callbackUrl on the first rendered frame', () => { + const html = renderFirstFrame('callbackUrl=/workspace/abc/w/xyz') + + expect(html).toContain(encodeURIComponent('/workspace/abc/w/xyz')) + }) + + it('falls back to /workspace when no callbackUrl is present', () => { + const html = renderFirstFrame('') + + expect(html).toContain(`/login?callbackUrl=${encodeURIComponent('/workspace')}`) + }) + + it('rejects an off-origin callbackUrl and falls back to /workspace', () => { + const html = renderFirstFrame('callbackUrl=https://evil.example.com/steal') + + expect(html).not.toContain('evil.example.com') + expect(html).toContain(`/login?callbackUrl=${encodeURIComponent('/workspace')}`) + }) +}) diff --git a/apps/sim/ee/sso/components/sso-form.tsx b/apps/sim/ee/sso/components/sso-form.tsx index a9a2e216835..4fab9653a21 100644 --- a/apps/sim/ee/sso/components/sso-form.tsx +++ b/apps/sim/ee/sso/components/sso-form.tsx @@ -36,21 +36,27 @@ export default function SSOForm() { const [email, setEmail] = useState('') const [emailErrors, setEmailErrors] = useState([]) const [showEmailValidationError, setShowEmailValidationError] = useState(false) - const [callbackUrl, setCallbackUrl] = useState('/workspace') const emailEnabled = !isFalsy(getEnv('NEXT_PUBLIC_EMAIL_PASSWORD_SIGNUP_ENABLED')) + /** + * Derived during render rather than seeded into state from an effect: the + * first painted frame otherwise carries the `/workspace` default, so the + * "Sign in with email" and "Sign up" links briefly point at the wrong + * destination on any deep link carrying `?callbackUrl=`. + */ + const callbackParam = searchParams?.get('callbackUrl') ?? null + const isCallbackValid = callbackParam !== null && validateCallbackUrl(callbackParam) + const callbackUrl = callbackParam !== null && isCallbackValid ? callbackParam : '/workspace' + useEffect(() => { - if (searchParams) { - const callback = searchParams.get('callbackUrl') - if (callback) { - if (validateCallbackUrl(callback)) { - setCallbackUrl(callback) - } else { - logger.warn('Invalid callback URL detected and blocked:', { url: callback }) - } - } + if (callbackParam !== null && !isCallbackValid) { + logger.warn('Invalid callback URL detected and blocked:', { url: callbackParam }) + } + }, [callbackParam, isCallbackValid]) + useEffect(() => { + if (searchParams) { const emailParam = searchParams.get('email') if (emailParam) { setEmail(emailParam) diff --git a/apps/sim/lib/core/security/input-validation.test.ts b/apps/sim/lib/core/security/input-validation.test.ts index f2472683d09..3d548cfdec1 100644 --- a/apps/sim/lib/core/security/input-validation.test.ts +++ b/apps/sim/lib/core/security/input-validation.test.ts @@ -1,4 +1,4 @@ -import { envFlagsMock, resetEnvFlagsMock } from '@sim/testing' +import { defaultMockEnv, envFlagsMock, resetEnvFlagsMock, resetEnvMock, setEnv } from '@sim/testing' import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest' import { validateAirtableId, @@ -2136,6 +2136,7 @@ describe('validateCallbackUrl', () => { }) afterEach(() => { + resetEnvMock() if (originalWindow === undefined) { ;(globalThis as { window?: unknown }).window = undefined } else { @@ -2187,12 +2188,28 @@ describe('validateCallbackUrl', () => { ;(globalThis as { window?: unknown }).window = undefined }) - it('falls back to placeholder origin and still rejects cross-origin URLs', () => { + it('resolves against the configured app origin and still rejects cross-origin URLs', () => { expect(validateCallbackUrl('/workspace')).toBe(true) expect(validateCallbackUrl('//evil.com')).toBe(false) expect(validateCallbackUrl('https://evil.com')).toBe(false) expect(validateCallbackUrl('javascript:alert(1)')).toBe(false) }) + + /** + * The server verdict has to match what the browser will decide once it + * hydrates, or a callback URL derived during render yields one destination + * in the SSR markup and another after hydration. + */ + it('accepts an absolute same-origin URL, matching the browser verdict', () => { + expect(validateCallbackUrl(`${defaultMockEnv.NEXT_PUBLIC_APP_URL}/workspace/abc`)).toBe(true) + }) + + it('stays fail-closed on absolute URLs when the app URL is unset', () => { + setEnv({ NEXT_PUBLIC_APP_URL: undefined }) + + expect(validateCallbackUrl(`${defaultMockEnv.NEXT_PUBLIC_APP_URL}/workspace/abc`)).toBe(false) + expect(validateCallbackUrl('/workspace')).toBe(true) + }) }) }) diff --git a/apps/sim/lib/core/security/input-validation.ts b/apps/sim/lib/core/security/input-validation.ts index fb802353daa..f69cf7f735b 100644 --- a/apps/sim/lib/core/security/input-validation.ts +++ b/apps/sim/lib/core/security/input-validation.ts @@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger' import { isLoopbackIp, isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf' import * as ipaddr from 'ipaddr.js' import { isHosted } from '@/lib/core/config/env-flags' +import { getBaseUrl } from '@/lib/core/utils/urls' const logger = createLogger('InputValidation') @@ -1234,6 +1235,31 @@ export function validatePaginationCursor( const CALLBACK_URL_SERVER_BASE = 'https://callback-url-validator.invalid' +/** + * Origin a callback URL is resolved and compared against. + * + * The browser uses its own origin. Server-side there is no `window`, so it uses + * the deployment's configured origin — which is what the browser will compare + * against once it hydrates. Using a sentinel here instead made the server reject + * every absolute URL, including the same-origin ones this function documents as + * valid, so a component deriving a callback URL during render produced one + * destination in the SSR markup and a different one after hydration. + * + * Falls back to the sentinel when the app URL is unset or unparseable, which + * keeps the server fail-closed: every absolute URL is rejected, as before. + */ +function getCallbackValidationOrigin(): string { + if (typeof window !== 'undefined') { + return window.location.origin + } + + try { + return new URL(getBaseUrl()).origin + } catch { + return CALLBACK_URL_SERVER_BASE + } +} + /** * Validates a callback URL to prevent open redirect attacks. * @@ -1263,7 +1289,7 @@ export function validateCallbackUrl(url: string): boolean { try { if (typeof url !== 'string' || url.length === 0) return false - const base = typeof window === 'undefined' ? CALLBACK_URL_SERVER_BASE : window.location.origin + const base = getCallbackValidationOrigin() const parsed = new URL(url, base) return parsed.origin === base } catch (error) { diff --git a/apps/sim/lib/core/utils/urls.test.ts b/apps/sim/lib/core/utils/urls.test.ts index b476f76d073..bcbe797df6e 100644 --- a/apps/sim/lib/core/utils/urls.test.ts +++ b/apps/sim/lib/core/utils/urls.test.ts @@ -56,15 +56,20 @@ describe('getBaseUrl', () => { expect(getBaseUrl()).toBe('https://app.example.com') }) - it('falls back to the page origin instead of throwing when the injected env is missing', () => { + /** + * Never guesses from `window.location.origin`: an opaque origin (a sandboxed + * iframe) serializes to the truthy string `'null'`, which would silently + * produce `null/api/...` rather than surfacing the misconfiguration. + */ + it('throws in the browser rather than guessing from the page origin', () => { setLocation('https://www.sim.ai/workspace/ws-1/w/wf-1') - expect(getBaseUrl()).toBe('https://www.sim.ai') + expect(() => getBaseUrl()).toThrow('NEXT_PUBLIC_APP_URL must be configured') }) it('treats a whitespace-only NEXT_PUBLIC_APP_URL as unset', () => { mockGetEnv.mockImplementation((key) => (key === 'NEXT_PUBLIC_APP_URL' ? ' ' : undefined)) setLocation('https://www.sim.ai/') - expect(getBaseUrl()).toBe('https://www.sim.ai') + expect(() => getBaseUrl()).toThrow('NEXT_PUBLIC_APP_URL must be configured') }) }) diff --git a/apps/sim/lib/core/utils/urls.ts b/apps/sim/lib/core/utils/urls.ts index 59abca57512..5f13016e80b 100644 --- a/apps/sim/lib/core/utils/urls.ts +++ b/apps/sim/lib/core/utils/urls.ts @@ -25,31 +25,26 @@ function normalizeBaseUrl(url: string): string { * Returns the base URL of the application from NEXT_PUBLIC_APP_URL * This ensures webhooks, callbacks, and other integrations always use the correct public URL * - * In the browser, falls back to the page's own origin when the injected env is - * unavailable. Client-side callers only ever want a URL back to the app they are - * already served from, so the origin is a correct answer — and a throw here - * during render tears down the whole page through the error boundary. Server-side - * callers (webhooks, callbacks, emails) have no origin to fall back to and must - * still fail loudly on a misconfigured deployment. + * Deliberately has no browser fallback to `window.location.origin`. The value is + * injected before hydration by ``, so an empty read means the + * deployment is misconfigured — and a same-origin guess would hide that. It also + * would not be safe to guess: an opaque origin (a sandboxed iframe, and `/chat/*` + * is embeddable) serializes to the string `'null'`, which is truthy and would + * silently produce `null/api/...` at every call site. * * @returns The base URL string (e.g., 'http://localhost:3000' or 'https://example.com') - * @throws Error if NEXT_PUBLIC_APP_URL is not configured and no browser origin exists + * @throws Error if NEXT_PUBLIC_APP_URL is not configured */ export function getBaseUrl(): string { const baseUrl = getEnv('NEXT_PUBLIC_APP_URL')?.trim() - if (baseUrl) { - return normalizeBaseUrl(baseUrl) - } - - const browserOrigin = getBrowserOrigin() - if (browserOrigin) { - return browserOrigin + if (!baseUrl) { + throw new Error( + 'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly' + ) } - throw new Error( - 'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly' - ) + return normalizeBaseUrl(baseUrl) } /** diff --git a/packages/testing/src/mocks/urls.mock.ts b/packages/testing/src/mocks/urls.mock.ts index ab013442723..2dab1b1560e 100644 --- a/packages/testing/src/mocks/urls.mock.ts +++ b/packages/testing/src/mocks/urls.mock.ts @@ -26,18 +26,14 @@ function hasHttpProtocol(url: string): boolean { function getBaseUrlImpl(): string { const baseUrl = readEnv('NEXT_PUBLIC_APP_URL')?.trim() - if (baseUrl) { - // Mirrors the real module: protocol-less values get https:// under isProd. - const protocol = envFlagsMock.isProd ? 'https://' : 'http://' - return hasHttpProtocol(baseUrl) ? baseUrl : `${protocol}${baseUrl}` + if (!baseUrl) { + throw new Error( + 'NEXT_PUBLIC_APP_URL must be configured for webhooks and callbacks to work correctly' + ) } - // Mirrors the real module: the browser falls back to its own origin, only - // server-side (no `window`) callers throw. - const browserOrigin = getBrowserOriginImpl() - if (browserOrigin) return browserOrigin - throw new Error( - '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}` } function getInternalApiBaseUrlImpl(): string {