diff --git a/backend/app/gateway/routers/auth.py b/backend/app/gateway/routers/auth.py index 8b490c9cb..f9c1c3c01 100644 --- a/backend/app/gateway/routers/auth.py +++ b/backend/app/gateway/routers/auth.py @@ -827,7 +827,8 @@ async def oauth_callback( # ── Issue DeerFlow session ─────────────────────────────────────── token = create_access_token(str(user.id), token_version=user.token_version) - redirect_target = state_payload.next_path or "/workspace" + # Revalidate as defense-in-depth if future state writers populate this target. + redirect_target = validate_next_param(state_payload.next_path) or "/workspace" frontend_base = oidc_config.frontend_base_url or "" callback_redirect = f"{frontend_base}/auth/callback?next={urllib.parse.quote(redirect_target)}" @@ -855,7 +856,8 @@ def validate_next_param(next_param: str | None) -> str | None: """Validate and sanitize the ``next`` redirect parameter. Only allows relative paths starting with ``/``. Rejects protocol-relative - URLs (``//``), absolute URLs, and URLs with embedded protocols. + URLs (``//``), absolute URLs, URLs with embedded protocols, and backslashes + that URL parsers may reinterpret as forward slashes. """ if not next_param: return None @@ -863,6 +865,8 @@ def validate_next_param(next_param: str | None) -> str | None: return None if next_param.startswith("//") or next_param.startswith("http://") or next_param.startswith("https://"): return None + if "\\" in next_param: + return None if ":" in next_param: return None return next_param diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index b2b6aa1a4..bec069b16 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -1130,8 +1130,13 @@ def test_authenticate_skips_rehash_for_v2_hash(): mock_repo.update_user.assert_not_called() -def test_validate_next_param_rejects_colon_paths(): +def test_validate_next_param_rejects_unsafe_paths(): from app.gateway.routers.auth import validate_next_param assert validate_next_param("/workspace") == "/workspace" + assert validate_next_param("/workspace/chats/new?tab=recent#top") == "/workspace/chats/new?tab=recent#top" assert validate_next_param("/:evil") is None + assert validate_next_param("/\\evil.example") is None + assert validate_next_param("/foo\\bar") is None + assert validate_next_param("//evil.example") is None + assert validate_next_param("https://evil.example") is None diff --git a/frontend/src/app/(auth)/auth/callback/page.tsx b/frontend/src/app/(auth)/auth/callback/page.tsx index a48ade103..79a7d6e0e 100644 --- a/frontend/src/app/(auth)/auth/callback/page.tsx +++ b/frontend/src/app/(auth)/auth/callback/page.tsx @@ -1,19 +1,9 @@ "use client"; import { useRouter, useSearchParams } from "next/navigation"; -import { useEffect, useState, useCallback, useRef } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; -/** - * Validates the next parameter — only allows relative paths starting with /. - */ -function validateNextParam(next: string | null): string { - if (!next) return "/workspace"; - if (!next.startsWith("/") || next.startsWith("//")) return "/workspace"; - if (next.startsWith("http://") || next.startsWith("https://")) - return "/workspace"; - if (next.includes(":")) return "/workspace"; - return next; -} +import { resolveAuthNextPath } from "@/core/auth/next-path"; export default function AuthCallbackPage() { const router = useRouter(); @@ -27,7 +17,7 @@ export default function AuthCallbackPage() { if (calledRef.current) return; calledRef.current = true; - const next = validateNextParam(searchParams.get("next")); + const next = resolveAuthNextPath(searchParams.get("next")); try { const res = await fetch("/api/v1/auth/me", { credentials: "include" }); diff --git a/frontend/src/app/(auth)/login/page.tsx b/frontend/src/app/(auth)/login/page.tsx index b7acbc309..de392b113 100644 --- a/frontend/src/app/(auth)/login/page.tsx +++ b/frontend/src/app/(auth)/login/page.tsx @@ -10,6 +10,7 @@ import { Button } from "@/components/ui/button"; import { FlickeringGrid } from "@/components/ui/flickering-grid"; import { Input } from "@/components/ui/input"; import { useAuth } from "@/core/auth/AuthProvider"; +import { resolveAuthNextPath } from "@/core/auth/next-path"; import { loadRememberLoginPreference, saveRememberLoginPreference, @@ -22,39 +23,6 @@ import { import { parseAuthError } from "@/core/auth/types"; import { useI18n } from "@/core/i18n/hooks"; -/** - * Validate next parameter - * Prevent open redirect attacks - * Per RFC-001: Only allow relative paths starting with / - */ -function validateNextParam(next: string | null): string | null { - if (!next) { - return null; - } - - // Need start with / (relative path) - if (!next.startsWith("/")) { - return null; - } - - // Disallow protocol-relative URLs - if ( - next.startsWith("//") || - next.startsWith("http://") || - next.startsWith("https://") - ) { - return null; - } - - // Disallow URLs with different protocols (e.g., javascript:, data:, etc) - if (next.includes(":") && !next.startsWith("/")) { - return null; - } - - // Valid relative path - return next; -} - export default function LoginPage() { const router = useRouter(); const searchParams = useSearchParams(); @@ -94,7 +62,7 @@ export default function LoginPage() { // Get next parameter for validated redirect const nextParam = searchParams.get("next"); - const redirectPath = validateNextParam(nextParam) ?? "/workspace"; + const redirectPath = resolveAuthNextPath(nextParam); const regularSignupAllowed = canCreateRegularAccount({ // A failed probe must not expose registration while the system's setup // state is unknown. Existing users can still sign in normally. diff --git a/frontend/src/core/auth/next-path.ts b/frontend/src/core/auth/next-path.ts new file mode 100644 index 000000000..41a96b263 --- /dev/null +++ b/frontend/src/core/auth/next-path.ts @@ -0,0 +1,26 @@ +export const DEFAULT_AUTH_NEXT_PATH = "/workspace"; + +export function validateAuthNextPath( + nextPath: string | null | undefined, +): string | null { + if (!nextPath) { + return null; + } + if (!nextPath.startsWith("/")) { + return null; + } + if (nextPath.startsWith("//")) { + return null; + } + if (nextPath.includes("\\") || nextPath.includes(":")) { + return null; + } + return nextPath; +} + +export function resolveAuthNextPath( + nextPath: string | null | undefined, + fallback = DEFAULT_AUTH_NEXT_PATH, +): string { + return validateAuthNextPath(nextPath) ?? fallback; +} diff --git a/frontend/tests/unit/core/auth/next-path.test.ts b/frontend/tests/unit/core/auth/next-path.test.ts new file mode 100644 index 000000000..1d9a06e29 --- /dev/null +++ b/frontend/tests/unit/core/auth/next-path.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, test } from "@rstest/core"; + +import { + resolveAuthNextPath, + validateAuthNextPath, +} from "@/core/auth/next-path"; + +describe("auth next path validation", () => { + test("accepts local absolute paths", () => { + expect(validateAuthNextPath("/workspace")).toBe("/workspace"); + expect(validateAuthNextPath("/workspace/chats/new?tab=recent#top")).toBe( + "/workspace/chats/new?tab=recent#top", + ); + }); + + test("rejects external and ambiguous redirects", () => { + expect(validateAuthNextPath(null)).toBeNull(); + expect(validateAuthNextPath("workspace")).toBeNull(); + expect(validateAuthNextPath("//evil.example")).toBeNull(); + expect(validateAuthNextPath("https://evil.example")).toBeNull(); + expect(validateAuthNextPath("/:evil")).toBeNull(); + expect(validateAuthNextPath("/\\evil.example")).toBeNull(); + expect(validateAuthNextPath("/foo\\bar")).toBeNull(); + }); + + test("falls back for unsafe paths", () => { + expect(resolveAuthNextPath("/\\evil.example")).toBe("/workspace"); + expect(resolveAuthNextPath("/safe", "/fallback")).toBe("/safe"); + }); +});