diff --git a/src/app/auth/callback/route.test.ts b/src/app/auth/callback/route.test.ts index 72fd7a8c..ca09b3c5 100644 --- a/src/app/auth/callback/route.test.ts +++ b/src/app/auth/callback/route.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { resolveRedirectPath } from "./route"; import { isInternalUrl } from "~/lib/url"; @@ -25,75 +25,60 @@ describe("isInternalUrl", () => { }); describe("resolveRedirectPath", () => { + beforeEach(() => { + vi.stubEnv("NEXT_PUBLIC_SITE_URL", "http://localhost:3000"); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + it("should accept valid internal path", () => { - const result = resolveRedirectPath({ - nextParam: "/dashboard", - origin: "http://localhost:3000", - forwardedHost: null, - }); + const result = resolveRedirectPath("/dashboard"); expect(result).toBe("/dashboard"); }); it("should reject external URL (open redirect prevention)", () => { - const result = resolveRedirectPath({ - nextParam: "https://evil.com/steal-session", - origin: "http://localhost:3000", - forwardedHost: null, - }); + const result = resolveRedirectPath("https://evil.com/steal-session"); expect(result).toBe("/"); }); it("should reject protocol-relative URL", () => { - const result = resolveRedirectPath({ - nextParam: "//evil.com/phishing", - origin: "http://localhost:3000", - forwardedHost: null, - }); + const result = resolveRedirectPath("//evil.com/phishing"); expect(result).toBe("/"); }); it("should handle paths with query params and hash", () => { - const result = resolveRedirectPath({ - nextParam: "/dashboard?tab=issues#top", - origin: "http://localhost:3000", - forwardedHost: null, - }); + const result = resolveRedirectPath("/dashboard?tab=issues#top"); expect(result).toBe("/dashboard?tab=issues#top"); }); it("should return fallback when nextParam is null", () => { - const result = resolveRedirectPath({ - nextParam: null, - origin: "http://localhost:3000", - forwardedHost: null, - }); + const result = resolveRedirectPath(null); expect(result).toBe("/"); }); - it("should accept absolute URL matching origin host", () => { - const result = resolveRedirectPath({ - nextParam: "http://localhost:3000/dashboard", - origin: "http://localhost:3000", - forwardedHost: null, - }); + it("should accept absolute URL matching site url", () => { + const result = resolveRedirectPath("http://localhost:3000/dashboard"); expect(result).toBe("/dashboard"); }); - it("should accept absolute URL matching forwarded host", () => { - const result = resolveRedirectPath({ - nextParam: "https://app.example.com/dashboard", - origin: "http://localhost:3000", - forwardedHost: "app.example.com", - }); + it("should reject absolute URL matching a different host (even if it was forwarded host previously)", () => { + // We simulate a scenario where attacker provided a different host. + // Since resolveRedirectPath now relies on NEXT_PUBLIC_SITE_URL, it should reject this. + const result = resolveRedirectPath("https://app.example.com/dashboard"); + expect(result).toBe("/"); + }); + + it("should accept absolute URL matching configured production site url", () => { + vi.stubEnv("NEXT_PUBLIC_SITE_URL", "https://pinpoint.com"); + const result = resolveRedirectPath("https://pinpoint.com/dashboard"); expect(result).toBe("/dashboard"); }); - it("should reject absolute URL not matching origin or forwarded host", () => { - const result = resolveRedirectPath({ - nextParam: "https://evil.com/dashboard", - origin: "http://test.local", - forwardedHost: "app.example.com", - }); + it("should reject mismatching site url when production url is configured", () => { + vi.stubEnv("NEXT_PUBLIC_SITE_URL", "https://pinpoint.com"); + const result = resolveRedirectPath("http://localhost:3000/dashboard"); expect(result).toBe("/"); }); }); diff --git a/src/app/auth/callback/route.ts b/src/app/auth/callback/route.ts index bbdf3062..feeb2268 100644 --- a/src/app/auth/callback/route.ts +++ b/src/app/auth/callback/route.ts @@ -9,14 +9,9 @@ import { createServerClient, type CookieOptions } from "@supabase/ssr"; import { NextResponse } from "next/server"; import { type NextRequest } from "next/server"; import type { EmailOtpType } from "@supabase/supabase-js"; -import { isInternalUrl } from "~/lib/url"; - -export function resolveRedirectPath(options: { - nextParam: string | null; - origin: string; - forwardedHost: string | null; -}): string { - const { nextParam, origin, forwardedHost } = options; +import { getSiteUrl, isInternalUrl } from "~/lib/url"; + +export function resolveRedirectPath(nextParam: string | null): string { const fallback = "/"; if (!nextParam) { @@ -27,16 +22,13 @@ export function resolveRedirectPath(options: { return nextParam; } - // Allow absolute URLs that point to this host only; drop origin to prevent open redirects. + // Allow absolute URLs that point to the configured site URL only. try { - const originHost = new URL(origin).host; - const allowedHosts = new Set([originHost]); - if (forwardedHost) { - allowedHosts.add(forwardedHost); - } + const siteUrl = getSiteUrl(); + const siteHost = new URL(siteUrl).host; + const parsed = new URL(nextParam); - const parsed = new URL(nextParam, origin); - if (allowedHosts.has(parsed.host)) { + if (parsed.host === siteHost) { const normalizedPath = `${parsed.pathname}${parsed.search}${parsed.hash}`; if (isInternalUrl(normalizedPath)) { return normalizedPath; @@ -63,30 +55,30 @@ export function resolveRedirectPath(options: { * @see https://supabase.com/docs/guides/auth/server-side/nextjs */ export async function GET(request: NextRequest): Promise { - const { searchParams, origin } = new URL(request.url); + const { searchParams } = new URL(request.url); const code = searchParams.get("code"); const tokenHash = searchParams.get("token_hash"); const typeParam = searchParams.get("type"); const nextParam = searchParams.get("next") ?? "/"; - const forwardedHost = request.headers.get("x-forwarded-host"); - const next = resolveRedirectPath({ nextParam, origin, forwardedHost }); - const isLocalEnv = process.env.NODE_ENV === "development"; + // Security: Always use configured site URL for redirects, never trust Host header + const next = resolveRedirectPath(nextParam); + const siteUrl = getSiteUrl(); + const shouldUseLoadingScreen = next === "/reset-password"; const redirectPath = shouldUseLoadingScreen ? `/auth/loading?next=${encodeURIComponent(next)}` : next; - const redirectToTarget = (): NextResponse => { - if (isLocalEnv) { - return NextResponse.redirect(`${origin}${redirectPath}`); - } + // Ensure redirectPath starts with / to avoid double slashes if siteUrl has trailing slash + // getSiteUrl() typically does not have trailing slash, but good to be safe. + // Actually getSiteUrl implementation: return `...` (no trailing slash usually). + // But redirectPath from resolveRedirectPath always starts with / due to isInternalUrl check. - if (forwardedHost) { - return NextResponse.redirect(`https://${forwardedHost}${redirectPath}`); - } + const targetUrl = `${siteUrl}${redirectPath}`; - return NextResponse.redirect(`${origin}${redirectPath}`); + const redirectToTarget = (): NextResponse => { + return NextResponse.redirect(targetUrl); }; const supabaseClient = createSupabaseClient(request); @@ -128,8 +120,9 @@ export async function GET(request: NextRequest): Promise { } // Auth failed or no code - redirect to error page + // Use siteUrl for error page redirect too return applyCookies( - NextResponse.redirect(`${origin}/auth/auth-code-error`), + NextResponse.redirect(`${siteUrl}/auth/auth-code-error`), pendingCookies ); }