From ac9bbd6a29ac0886b18fb7dc852679de98ef14da Mon Sep 17 00:00:00 2001 From: Junho Yeo Date: Mon, 1 Jun 2026 14:25:57 -0700 Subject: [PATCH] fix(auth): constrain GitHub OAuth return paths Validation * Validation tier: Tier 3 - auth redirect safety affects security-sensitive runtime behavior. * git diff --check: PASS * git diff --cached --check: PASS * bun x vitest run __tests__/api/githubAuthReturnTo.test.ts: PASS * bun x eslint src/app/api/auth/github/route.ts src/app/api/auth/github/callback/route.ts src/lib/auth/returnTo.ts __tests__/api/githubAuthReturnTo.test.ts: PASS * Ledger: not applicable - not required for selected validation tier/change family. * Version: not applicable - not required for selected validation tier/change family. * Not run: bun x tsc --noEmit --pretty false - project-wide check is blocked by unrelated existing frontend errors outside this diff. Rollback * git revert HEAD --- .../__tests__/api/githubAuthReturnTo.test.ts | 191 ++++++++++++++++++ .../src/app/api/auth/github/callback/route.ts | 5 +- .../frontend/src/app/api/auth/github/route.ts | 3 +- packages/frontend/src/lib/auth/returnTo.ts | 55 +++++ 4 files changed, 251 insertions(+), 3 deletions(-) create mode 100644 packages/frontend/__tests__/api/githubAuthReturnTo.test.ts create mode 100644 packages/frontend/src/lib/auth/returnTo.ts diff --git a/packages/frontend/__tests__/api/githubAuthReturnTo.test.ts b/packages/frontend/__tests__/api/githubAuthReturnTo.test.ts new file mode 100644 index 000000000..9652f2172 --- /dev/null +++ b/packages/frontend/__tests__/api/githubAuthReturnTo.test.ts @@ -0,0 +1,191 @@ +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; + +const mockState = vi.hoisted(() => { + const cookieStore = { + get: vi.fn(), + set: vi.fn(), + delete: vi.fn(), + }; + const cookies = vi.fn(async () => cookieStore); + const getAuthorizationUrl = vi.fn(() => "https://github.com/login/oauth/authorize"); + const exchangeCodeForToken = vi.fn(async () => "access-token"); + const getGitHubUser = vi.fn(async () => ({ + id: 123, + login: "alice", + name: "Alice", + avatar_url: "https://avatars.example/alice.png", + email: "alice@example.com", + })); + const getGitHubUserEmail = vi.fn(async () => "alice@example.com"); + const createSession = vi.fn(async () => "session-token"); + const setSessionCookie = vi.fn(async () => undefined); + const generateRandomString = vi.fn(() => "state-token"); + + const selectLimit = vi.fn(async () => [ + { + id: "user-1", + githubId: 123, + username: "alice", + displayName: "Alice", + avatarUrl: "https://avatars.example/alice.png", + email: "alice@example.com", + }, + ]); + const selectWhere = vi.fn(() => ({ limit: selectLimit })); + const selectFrom = vi.fn(() => ({ where: selectWhere })); + const updateWhere = vi.fn(async () => undefined); + const updateSet = vi.fn(() => ({ where: updateWhere })); + const insertReturning = vi.fn(async () => [{ id: "user-1" }]); + const insertValues = vi.fn(() => ({ returning: insertReturning })); + const db = { + select: vi.fn(() => ({ from: selectFrom })), + update: vi.fn(() => ({ set: updateSet })), + insert: vi.fn(() => ({ values: insertValues })), + }; + + return { + cookieStore, + cookies, + getAuthorizationUrl, + exchangeCodeForToken, + getGitHubUser, + getGitHubUserEmail, + createSession, + setSessionCookie, + generateRandomString, + db, + selectLimit, + reset() { + cookieStore.get.mockReset(); + cookieStore.set.mockReset(); + cookieStore.delete.mockReset(); + cookies.mockClear(); + getAuthorizationUrl.mockClear(); + exchangeCodeForToken.mockClear(); + getGitHubUser.mockClear(); + getGitHubUserEmail.mockClear(); + createSession.mockClear(); + setSessionCookie.mockClear(); + generateRandomString.mockClear(); + db.select.mockClear(); + db.update.mockClear(); + db.insert.mockClear(); + selectLimit.mockClear(); + }, + }; +}); + +vi.mock("next/headers", () => ({ + cookies: mockState.cookies, +})); + +vi.mock("@/lib/auth/github", () => ({ + getAuthorizationUrl: mockState.getAuthorizationUrl, + exchangeCodeForToken: mockState.exchangeCodeForToken, + getGitHubUser: mockState.getGitHubUser, + getGitHubUserEmail: mockState.getGitHubUserEmail, +})); + +vi.mock("@/lib/auth/session", () => ({ + createSession: mockState.createSession, + setSessionCookie: mockState.setSessionCookie, +})); + +vi.mock("@/lib/auth/utils", () => ({ + generateRandomString: mockState.generateRandomString, +})); + +vi.mock("@/lib/db", () => ({ + db: mockState.db, + users: { + id: "users.id", + githubId: "users.githubId", + }, +})); + +vi.mock("drizzle-orm", () => ({ + eq: vi.fn(() => "eq"), +})); + +type StartRouteExports = typeof import("../../src/app/api/auth/github/route"); +type CallbackRouteExports = typeof import("../../src/app/api/auth/github/callback/route"); + +let startGET: StartRouteExports["GET"]; +let callbackGET: CallbackRouteExports["GET"]; + +beforeAll(async () => { + const [startRoute, callbackRoute] = await Promise.all([ + import("../../src/app/api/auth/github/route"), + import("../../src/app/api/auth/github/callback/route"), + ]); + startGET = startRoute.GET; + callbackGET = callbackRoute.GET; +}); + +beforeEach(() => { + mockState.reset(); + process.env.NEXT_PUBLIC_URL = "https://tokscale.ai"; +}); + +describe("GitHub OAuth returnTo safety", () => { + it.each([ + ["/settings", "/settings"], + ["/device?code=abc", "/device?code=abc"], + ["https://evil.test/path", "/leaderboard"], + ["//evil.test/path", "/leaderboard"], + ["@evil.test/path", "/leaderboard"], + ["\\evil.test\\path", "/leaderboard"], + ["%2f%2fevil.test/path", "/leaderboard"], + ["%5cevil.test%5cpath", "/leaderboard"], + ])("stores safe returnTo value for %s", async (returnTo, expected) => { + await startGET( + new Request( + `https://tokscale.ai/api/auth/github?returnTo=${encodeURIComponent(returnTo)}` + ) + ); + + expect(mockState.cookieStore.set).toHaveBeenCalledTimes(1); + const [, rawValue] = mockState.cookieStore.set.mock.calls[0]; + expect(JSON.parse(rawValue)).toMatchObject({ + state: "state-token", + returnTo: expected, + }); + }); + + it.each([ + ["@evil.test/path"], + ["https://evil.test/path"], + ["//evil.test/path"], + ["\\evil.test\\path"], + ["%2f%2fevil.test/path"], + ["/%5cevil.test"], + ])("falls back when callback cookie returnTo is unsafe: %s", async (returnTo) => { + mockState.cookieStore.get.mockReturnValue({ + value: JSON.stringify({ state: "state-token", returnTo }), + }); + + const response = await callbackGET( + new Request( + "https://tokscale.ai/api/auth/github/callback?code=ok&state=state-token" + ) + ); + + expect(response.headers.get("location")).toBe("https://tokscale.ai/leaderboard"); + }); + + it("redirects to a safe same-origin relative callback returnTo", async () => { + mockState.cookieStore.get.mockReturnValue({ + value: JSON.stringify({ state: "state-token", returnTo: "/device?code=abc" }), + }); + + const response = await callbackGET( + new Request( + "https://tokscale.ai/api/auth/github/callback?code=ok&state=state-token" + ) + ); + + expect(response.headers.get("location")).toBe( + "https://tokscale.ai/device?code=abc" + ); + }); +}); diff --git a/packages/frontend/src/app/api/auth/github/callback/route.ts b/packages/frontend/src/app/api/auth/github/callback/route.ts index 654c6fd27..29d56ec5b 100644 --- a/packages/frontend/src/app/api/auth/github/callback/route.ts +++ b/packages/frontend/src/app/api/auth/github/callback/route.ts @@ -5,6 +5,7 @@ import { getGitHubUser, getGitHubUserEmail, } from "@/lib/auth/github"; +import { sanitizeAuthReturnTo } from "@/lib/auth/returnTo"; import { createSession, setSessionCookie } from "@/lib/auth/session"; import { db, users } from "@/lib/db"; import { eq } from "drizzle-orm"; @@ -105,8 +106,8 @@ export async function GET(request: Request) { await setSessionCookie(sessionToken); // Redirect to return URL - const returnTo = storedState.returnTo || "/leaderboard"; - return NextResponse.redirect(`${baseUrl}${returnTo}`); + const returnTo = sanitizeAuthReturnTo(storedState.returnTo); + return NextResponse.redirect(new URL(returnTo, baseUrl)); } catch (err) { console.error("GitHub OAuth callback error:", err); return NextResponse.redirect(`${baseUrl}/leaderboard?error=auth_failed`); diff --git a/packages/frontend/src/app/api/auth/github/route.ts b/packages/frontend/src/app/api/auth/github/route.ts index b371b2be9..d32d224ab 100644 --- a/packages/frontend/src/app/api/auth/github/route.ts +++ b/packages/frontend/src/app/api/auth/github/route.ts @@ -1,11 +1,12 @@ import { NextResponse } from "next/server"; import { cookies } from "next/headers"; import { getAuthorizationUrl } from "@/lib/auth/github"; +import { sanitizeAuthReturnTo } from "@/lib/auth/returnTo"; import { generateRandomString } from "@/lib/auth/utils"; export async function GET(request: Request) { const { searchParams } = new URL(request.url); - const returnTo = searchParams.get("returnTo") || "/leaderboard"; + const returnTo = sanitizeAuthReturnTo(searchParams.get("returnTo")); // Generate CSRF state const state = generateRandomString(32); diff --git a/packages/frontend/src/lib/auth/returnTo.ts b/packages/frontend/src/lib/auth/returnTo.ts new file mode 100644 index 000000000..741d032e8 --- /dev/null +++ b/packages/frontend/src/lib/auth/returnTo.ts @@ -0,0 +1,55 @@ +const DEFAULT_AUTH_RETURN_TO = "/leaderboard"; +const RETURN_TO_BASE_URL = "https://tokscale.invalid"; + +function hasUnsafeRelativeStart(value: string): boolean { + return !value.startsWith("/") || value.startsWith("//"); +} + +export function sanitizeAuthReturnTo(value: string | null | undefined): string { + if (!value) { + return DEFAULT_AUTH_RETURN_TO; + } + + const raw = value.trim(); + if (!raw || raw.includes("\\")) { + return DEFAULT_AUTH_RETURN_TO; + } + + let decoded: string; + try { + decoded = decodeURIComponent(raw); + } catch { + return DEFAULT_AUTH_RETURN_TO; + } + + if ( + decoded.includes("\\") || + hasUnsafeRelativeStart(raw) || + hasUnsafeRelativeStart(decoded) + ) { + return DEFAULT_AUTH_RETURN_TO; + } + + const baseUrl = new URL(RETURN_TO_BASE_URL); + let parsed: URL; + let decodedParsed: URL; + try { + parsed = new URL(raw, baseUrl); + decodedParsed = new URL(decoded, baseUrl); + } catch { + return DEFAULT_AUTH_RETURN_TO; + } + + if ( + parsed.origin !== baseUrl.origin || + decodedParsed.origin !== baseUrl.origin || + parsed.username || + parsed.password || + decodedParsed.username || + decodedParsed.password + ) { + return DEFAULT_AUTH_RETURN_TO; + } + + return `${parsed.pathname}${parsed.search}${parsed.hash}` || DEFAULT_AUTH_RETURN_TO; +}