diff --git a/apps/e2e/tests/admin/admin.spec.ts b/apps/e2e/tests/admin/admin.spec.ts index e2cd54eb9..4e93df237 100644 --- a/apps/e2e/tests/admin/admin.spec.ts +++ b/apps/e2e/tests/admin/admin.spec.ts @@ -103,6 +103,9 @@ test.describe('Admin', () => { ), ]); + // TODO: find out why we need to reload the page only in CI + await page.reload(); + await expect(page.getByText('Banned').first()).toBeVisible(); await page.context().clearCookies(); @@ -149,7 +152,8 @@ test.describe('Admin', () => { ), ]); - await page.waitForTimeout(250); + // TODO: find out why we need to reload the page only in CI + await page.reload(); // Verify ban badge is removed await expect(page.getByText('Banned')).not.toBeVisible(); diff --git a/apps/web/app/auth/sign-in/page.tsx b/apps/web/app/auth/sign-in/page.tsx index 015c3c778..ccb552613 100644 --- a/apps/web/app/auth/sign-in/page.tsx +++ b/apps/web/app/auth/sign-in/page.tsx @@ -1,6 +1,7 @@ import Link from 'next/link'; import { SignInMethodsContainer } from '@kit/auth/sign-in'; +import { getSafeRedirectPath } from '@kit/shared/utils'; import { Button } from '@kit/ui/button'; import { Heading } from '@kit/ui/heading'; import { Trans } from '@kit/ui/trans'; @@ -29,7 +30,7 @@ async function SignInPage({ searchParams }: SignInPageProps) { const paths = { callback: pathsConfig.auth.callback, - returnPath: next || pathsConfig.app.home, + returnPath: getSafeRedirectPath(next, pathsConfig.app.home), joinTeam: pathsConfig.app.joinTeam, }; diff --git a/apps/web/app/auth/verify/page.tsx b/apps/web/app/auth/verify/page.tsx index aa128c91b..c7b731502 100644 --- a/apps/web/app/auth/verify/page.tsx +++ b/apps/web/app/auth/verify/page.tsx @@ -1,6 +1,7 @@ import { redirect } from 'next/navigation'; import { MultiFactorChallengeContainer } from '@kit/auth/mfa'; +import { getSafeRedirectPath } from '@kit/shared/utils'; import { checkRequiresMultiFactorAuthentication } from '@kit/supabase/check-requires-mfa'; import { getSupabaseServerClient } from '@kit/supabase/server-client'; @@ -38,7 +39,7 @@ async function VerifyPage(props: Props) { } const nextPath = (await props.searchParams).next; - const redirectPath = nextPath ?? pathsConfig.app.home; + const redirectPath = getSafeRedirectPath(nextPath, pathsConfig.app.home); return ( diff --git a/apps/web/proxy.ts b/apps/web/proxy.ts index fd1632bb8..f9cf4d054 100644 --- a/apps/web/proxy.ts +++ b/apps/web/proxy.ts @@ -4,6 +4,7 @@ import { NextResponse } from 'next/server'; import { CsrfError, createCsrfProtect } from '@edge-csrf/nextjs'; import { isSuperAdmin } from '@kit/admin'; +import { getSafeRedirectPath } from '@kit/shared/utils'; import { checkRequiresMultiFactorAuthentication } from '@kit/supabase/check-requires-mfa'; import { createMiddlewareClient } from '@kit/supabase/middleware-client'; @@ -158,8 +159,10 @@ async function getPatterns() { // If user is logged in and does not need to verify MFA, // redirect to home page. if (!isVerifyMfa) { - const nextPath = - req.nextUrl.searchParams.get('next') ?? pathsConfig.app.home; + const nextPath = getSafeRedirectPath( + req.nextUrl.searchParams.get('next'), + pathsConfig.app.home, + ); return NextResponse.redirect( new URL(nextPath, req.nextUrl.origin).href, diff --git a/package.json b/package.json index d0284ef85..d38b41272 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "next-supabase-saas-kit-turbo", - "version": "2.21.11", + "version": "2.21.12", "private": true, "sideEffects": false, "engines": { diff --git a/packages/features/auth/src/components/sign-up-methods-container.tsx b/packages/features/auth/src/components/sign-up-methods-container.tsx index f73caaaca..41fc73566 100644 --- a/packages/features/auth/src/components/sign-up-methods-container.tsx +++ b/packages/features/auth/src/components/sign-up-methods-container.tsx @@ -2,7 +2,7 @@ import type { Provider } from '@supabase/supabase-js'; -import { isBrowser } from '@kit/shared/utils'; +import { isBrowser, isSafeRedirectPath } from '@kit/shared/utils'; import { If } from '@kit/ui/if'; import { Separator } from '@kit/ui/separator'; import { Trans } from '@kit/ui/trans'; @@ -114,7 +114,8 @@ function getCallbackUrl(props: { const searchParams = new URLSearchParams(window.location.search); const next = searchParams.get('next'); - if (next) { + // Only pass through the next param if it's a safe internal path + if (next && isSafeRedirectPath(next)) { url.searchParams.set('next', next); } diff --git a/packages/features/team-accounts/src/components/invitations/sign-out-invitation-button.tsx b/packages/features/team-accounts/src/components/invitations/sign-out-invitation-button.tsx index d9e1b15dc..3cfb166a0 100644 --- a/packages/features/team-accounts/src/components/invitations/sign-out-invitation-button.tsx +++ b/packages/features/team-accounts/src/components/invitations/sign-out-invitation-button.tsx @@ -1,5 +1,6 @@ 'use client'; +import { getSafeRedirectPath } from '@kit/shared/utils'; import { useSignOut } from '@kit/supabase/hooks/use-sign-out'; import { Button } from '@kit/ui/button'; import { Trans } from '@kit/ui/trans'; @@ -16,7 +17,11 @@ export function SignOutInvitationButton( variant={'ghost'} onClick={async () => { await signOut.mutateAsync(); - window.location.assign(props.nextPath); + + // Validate the path to prevent open redirect attacks + const safePath = getSafeRedirectPath(props.nextPath, '/'); + + window.location.assign(safePath); }} > diff --git a/packages/shared/src/utils.ts b/packages/shared/src/utils.ts index 4ec9ae71a..debd3e5cf 100644 --- a/packages/shared/src/utils.ts +++ b/packages/shared/src/utils.ts @@ -21,3 +21,43 @@ export function formatCurrency(params: { currency: params.currencyCode, }).format(Number(params.value)); } + +/** + * @name isSafeRedirectPath + * @description Checks if a path is safe for redirects (prevents open redirect attacks). + * Safe paths must: + * - Start with a single `/` + * - NOT start with `//` (protocol-relative URLs) + * - NOT contain `://` (absolute URLs) + * - NOT contain backslash (URL normalization attacks) + */ +export function isSafeRedirectPath(path: string): boolean { + if (!path || typeof path !== 'string') return false; + + // Must start with exactly one forward slash (relative path) + if (!path.startsWith('/') || path.startsWith('//')) return false; + + // Must not contain protocol indicators + if (path.includes('://')) return false; + + // Must not contain backslashes (can be normalized to forward slashes) + if (path.includes('\\')) return false; + + return true; +} + +/** + * @name getSafeRedirectPath + * @description Returns the path if safe, otherwise returns the fallback. + * Use this to validate user-supplied redirect URLs to prevent open redirect attacks. + */ +export function getSafeRedirectPath( + path: string | null | undefined, + fallback: string, +): string { + if (path && isSafeRedirectPath(path)) { + return path; + } + + return fallback; +} \ No newline at end of file diff --git a/packages/supabase/package.json b/packages/supabase/package.json index 39a469410..852c4b2da 100644 --- a/packages/supabase/package.json +++ b/packages/supabase/package.json @@ -21,6 +21,9 @@ "./auth": "./src/auth.ts", "./types": "./src/types.ts" }, + "dependencies": { + "@kit/shared": "workspace:*" + }, "devDependencies": { "@kit/eslint-config": "workspace:*", "@kit/prettier-config": "workspace:*", diff --git a/packages/supabase/src/auth-callback.service.ts b/packages/supabase/src/auth-callback.service.ts index 28d12ea68..cb033533d 100644 --- a/packages/supabase/src/auth-callback.service.ts +++ b/packages/supabase/src/auth-callback.service.ts @@ -6,6 +6,8 @@ import { SupabaseClient, } from '@supabase/supabase-js'; +import { isSafeRedirectPath } from '@kit/shared/utils'; + /** * @name createAuthCallbackService * @description Creates an instance of the AuthCallbackService @@ -71,9 +73,11 @@ class AuthCallbackService { const errorPath = params.errorPath ?? '/auth/callback/error'; - // remove the query params from the url + // remove the auth-related query params from the url searchParams.delete('token_hash'); + searchParams.delete('type'); searchParams.delete('next'); + searchParams.delete('callback'); // if we have a next path, we redirect to that path if (nextPath) { @@ -132,7 +136,11 @@ class AuthCallbackService { const nextUrlPathFromParams = searchParams.get('next'); const errorPath = params.errorPath ?? '/auth/callback/error'; - const nextUrl = nextUrlPathFromParams ?? params.redirectPath; + // Validate the next URL to prevent open redirect attacks + const nextUrl = + nextUrlPathFromParams && isSafeRedirectPath(nextUrlPathFromParams) + ? nextUrlPathFromParams + : params.redirectPath; if (authCode) { try { @@ -188,6 +196,7 @@ class AuthCallbackService { /** * Parses a redirect URL and extracts the destination path and query params * Handles nested 'next' parameters for chained redirects + * Validates paths to prevent open redirect attacks */ private parseRedirectDestination(redirectParam: string | null): { path: string; @@ -197,29 +206,45 @@ class AuthCallbackService { return null; } + // First, try as a simple relative path with optional query string + const [pathPart, queryPart] = redirectParam.split('?') as [ + string, + string | undefined, + ]; + + if (isSafeRedirectPath(pathPart)) { + return { + path: pathPart, + params: new URLSearchParams(queryPart ?? ''), + }; + } + + // Handle full URLs (e.g., from Supabase callback parameter) try { - const redirectUrl = new URL(redirectParam); + const url = new URL(redirectParam); - // check for nested 'next' parameter (chained redirect) - const nestedNext = redirectUrl.searchParams.get('next'); + // Check for nested 'next' parameter - this is the final destination + const nestedNext = url.searchParams.get('next'); - if (nestedNext) { - // use the nested path as the final destination + if (nestedNext && isSafeRedirectPath(nestedNext)) { return { path: nestedNext, - params: redirectUrl.searchParams, + params: url.searchParams, }; } - // no nested redirect, use the pathname directly - return { - path: redirectUrl.pathname, - params: redirectUrl.searchParams, - }; + // No nested next, use pathname if safe + if (isSafeRedirectPath(url.pathname)) { + return { + path: url.pathname, + params: url.searchParams, + }; + } } catch { - // invalid URL, ignore - return null; + // Invalid URL, ignore } + + return null; } private isLocalhost(host: string | null) { diff --git a/packages/supabase/src/hooks/use-link-identity-with-provider.ts b/packages/supabase/src/hooks/use-link-identity-with-provider.ts index b1cc4501e..8b346e40c 100644 --- a/packages/supabase/src/hooks/use-link-identity-with-provider.ts +++ b/packages/supabase/src/hooks/use-link-identity-with-provider.ts @@ -2,6 +2,8 @@ import type { Provider } from '@supabase/supabase-js'; import { useMutation } from '@tanstack/react-query'; +import { getSafeRedirectPath } from '@kit/shared/utils'; + import { useSupabase } from './use-supabase'; export function useLinkIdentityWithProvider( @@ -14,7 +16,12 @@ export function useLinkIdentityWithProvider( const mutationFn = async (provider: Provider) => { const origin = window.location.origin; - const redirectToPath = props.redirectToPath ?? '/home/settings'; + + // Validate the redirect path to prevent open redirect attacks + const redirectToPath = getSafeRedirectPath( + props.redirectToPath, + '/home/settings', + ); const url = new URL('/auth/callback', origin); url.searchParams.set('redirectTo', redirectToPath); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 615aa7080..88078f876 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1409,6 +1409,10 @@ importers: version: 19.2.7 packages/supabase: + dependencies: + '@kit/shared': + specifier: workspace:* + version: link:../shared devDependencies: '@kit/eslint-config': specifier: workspace:* @@ -14427,7 +14431,7 @@ snapshots: eslint: 9.39.1(jiti@2.6.1) eslint-import-resolver-node: 0.3.9 eslint-import-resolver-typescript: 3.10.1(eslint-plugin-import@2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)) - eslint-plugin-import: 2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint-import-resolver-typescript@3.10.1)(eslint@9.39.1(jiti@2.6.1)) + eslint-plugin-import: 2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint-import-resolver-typescript@3.10.1(eslint-plugin-import@2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)) eslint-plugin-jsx-a11y: 6.10.2(eslint@9.39.1(jiti@2.6.1)) eslint-plugin-react: 7.37.5(eslint@9.39.1(jiti@2.6.1)) eslint-plugin-react-hooks: 7.0.1(eslint@9.39.1(jiti@2.6.1)) @@ -14466,7 +14470,7 @@ snapshots: tinyglobby: 0.2.15 unrs-resolver: 1.11.1 optionalDependencies: - eslint-plugin-import: 2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint-import-resolver-typescript@3.10.1)(eslint@9.39.1(jiti@2.6.1)) + eslint-plugin-import: 2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint-import-resolver-typescript@3.10.1(eslint-plugin-import@2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)) transitivePeerDependencies: - supports-color @@ -14481,7 +14485,7 @@ snapshots: transitivePeerDependencies: - supports-color - eslint-plugin-import@2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint-import-resolver-typescript@3.10.1)(eslint@9.39.1(jiti@2.6.1)): + eslint-plugin-import@2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint-import-resolver-typescript@3.10.1(eslint-plugin-import@2.32.0(@typescript-eslint/parser@8.48.1(eslint@9.39.1(jiti@2.6.1))(typescript@5.9.3))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)))(eslint@9.39.1(jiti@2.6.1)): dependencies: '@rtsao/scc': 1.1.0 array-includes: 3.1.9