From e551f18e88f463c79b023f0202db0e22602629e5 Mon Sep 17 00:00:00 2001 From: zane1502 Date: Sat, 17 Oct 2026 07:14:56 -0700 Subject: [PATCH] fix: secure idempotency keys, guard wallet storage, skip /auth/me, return to bounty after sign-in --- src/app/auth/callback/CallbackClient.tsx | 64 +++++++++++------------- src/app/connect/ConnectPanel.tsx | 6 +++ src/components/bounty/ClaimButton.tsx | 6 ++- src/context/AuthContext.tsx | 39 ++++++--------- src/context/WalletContext.tsx | 33 ++++++++++-- src/lib/auth.ts | 18 +++++++ src/lib/return-to.test.ts | 37 ++++++++++++++ src/lib/return-to.ts | 37 ++++++++++++++ src/lib/utils.test.ts | 16 ++++++ src/lib/utils.ts | 16 +++--- 10 files changed, 204 insertions(+), 68 deletions(-) create mode 100644 src/lib/return-to.test.ts create mode 100644 src/lib/return-to.ts diff --git a/src/app/auth/callback/CallbackClient.tsx b/src/app/auth/callback/CallbackClient.tsx index fcad4963..014ef225 100644 --- a/src/app/auth/callback/CallbackClient.tsx +++ b/src/app/auth/callback/CallbackClient.tsx @@ -3,6 +3,7 @@ import { useEffect, useState } from "react"; import { useRouter, useSearchParams } from "next/navigation"; import { useAuth } from "@/context/AuthContext"; +import { takeReturnTo } from "@/lib/return-to"; import type { UserRole } from "@/types"; const ROLE_REDIRECT_MAP: Record = { @@ -64,6 +65,25 @@ export function CallbackClient() { const code = searchParams.get("code"); const token = searchParams.get("token"); + async function completeSignIn(accessToken: string) { + const resolvedUser = await login(accessToken); + // A null user here means the token exchange did not resolve a + // profile. Landing on a dashboard would render that dashboard's + // demo/mock data as if it were real, so surface the failure instead. + if (!resolvedUser) { + setError("sign-in-failed"); + return; + } + // A validated same-origin destination (e.g. the bounty the user was + // claiming) wins over the default landing pages. + router.replace( + takeReturnTo() ?? + (shouldFinishConnect(resolvedUser.roles, resolvedUser.stellarAddress) + ? "/connect" + : redirectForRoles(resolvedUser.roles)), + ); + } + async function handleCallback() { // Try code-based handoff first (backend redirects with ?code=) if (code) { @@ -85,57 +105,31 @@ export function CallbackClient() { throw new Error("No access token in response"); } - await login(accessToken); - router.replace("/dashboard/contributor"); - return; + await completeSignIn(accessToken); } catch { - setError("Could not complete sign-in. Please try again."); - return; + setError("sign-in-failed"); } + return; } // Fallback: direct token auth if (token) { + // Strip the JWT from the URL immediately so it doesn't persist in + // browser history, referrer headers, or server logs (#9). + window.history.replaceState({}, "", window.location.pathname); try { - await login(token); - router.replace("/dashboard/contributor"); + await completeSignIn(token); } catch { - setError("Could not complete sign-in. Please try again."); + setError("sign-in-failed"); } return; } // No code or token - setError("No token was returned by GitHub sign-in."); - } - - void handleCallback(); - if (!token) { - // eslint-disable-next-line react-hooks/set-state-in-effect setError("no-token"); - return; } - // Strip the JWT from the URL immediately so it doesn't persist in - // browser history, referrer headers, or server logs (#9). - window.history.replaceState({}, "", window.location.pathname); - - login(token) - .then((resolvedUser) => { - // A null user here means the token exchange did not resolve a - // profile. Landing on a dashboard would render that dashboard's - // demo/mock data as if it were real, so surface the failure instead. - if (!resolvedUser) { - setError("sign-in-failed"); - return; - } - router.replace( - shouldFinishConnect(resolvedUser.roles, resolvedUser.stellarAddress) - ? "/connect" - : redirectForRoles(resolvedUser.roles), - ); - }) - .catch(() => setError("sign-in-failed")); + void handleCallback(); }, [searchParams, login, router]); const copy = diff --git a/src/app/connect/ConnectPanel.tsx b/src/app/connect/ConnectPanel.tsx index 3aea8805..f78f38ea 100644 --- a/src/app/connect/ConnectPanel.tsx +++ b/src/app/connect/ConnectPanel.tsx @@ -52,6 +52,7 @@ import { GITHUB_OAUTH_URL } from "@/lib/config"; import { useAuth } from "@/context/AuthContext"; import { useWallet } from "@/context/WalletContext"; import { messages, t } from "@/lib/messages"; +import { storeReturnTo } from "@/lib/return-to"; export function ConnectPanel() { const { user, loading: authLoading } = useAuth(); @@ -137,6 +138,11 @@ export function ConnectPanel() { announced as a link containing a button. */ + storeReturnTo(new URLSearchParams(window.location.search).get("returnTo")) + } className="mt-4 flex h-10 w-full items-center justify-center rounded-full bg-slate-900 text-sm font-medium text-white hover:bg-slate-800 focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-indigo-500 dark:bg-white dark:text-slate-900 dark:hover:bg-slate-200" > {messages["connect.github.cta"]} diff --git a/src/components/bounty/ClaimButton.tsx b/src/components/bounty/ClaimButton.tsx index 3e11d4ce..c74d19bb 100644 --- a/src/components/bounty/ClaimButton.tsx +++ b/src/components/bounty/ClaimButton.tsx @@ -122,7 +122,11 @@ export function ClaimButton({ // from the JWT), so a signed-out click goes to /connect rather than // firing a request that can only 401. if (!user) { - router.push('/connect'); + // Carry the current page through sign-in so the user lands back here. + const here = window.location.pathname + window.location.search; + router.push( + here === '/' ? '/connect' : `/connect?returnTo=${encodeURIComponent(here)}`, + ); return; } dispatch({ type: 'RESET' }); diff --git a/src/context/AuthContext.tsx b/src/context/AuthContext.tsx index 672fed85..94702283 100644 --- a/src/context/AuthContext.tsx +++ b/src/context/AuthContext.tsx @@ -8,7 +8,13 @@ import { useRef, useState, } from "react"; -import { getToken, setToken as persistToken, clearToken, TOKEN_KEY } from "@/lib/auth"; +import { + getToken, + getTokenSubject, + setToken as persistToken, + clearToken, + TOKEN_KEY, +} from "@/lib/auth"; import { apiRequest, ApiRequestError } from "@/lib/api"; import { useCrossTabStorage } from "@/hooks/useCrossTabStorage"; import type { AuthUser } from "@/types"; @@ -29,7 +35,6 @@ interface AuthContextValue { loading: boolean; /** True when the auth backend is unreachable and the session state is uncertain (#587) */ degraded: boolean; - login: (token: string) => Promise; login: (token: string) => Promise; logout: () => void; // Resolves with the session's user (null when there's no valid token, after @@ -68,33 +73,22 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { const token = getToken(); if (!token) { setUser(null); - setLoading(false); - setDegraded(false); - return; - } - try { - const session = await apiRequest<{ userId: string; username: string }>( - "/auth/me", - ); - const profile = await apiRequest(`/users/${session.userId}`); - setUser(profile); - setDegraded(false); - } catch { - clearToken(); - setUser(null); - setDegraded(true); - } finally { setLoading(false); setDegraded(false); return null; } + // `/auth/me` only returns `{ userId, username }`, so resolving a session + // through it costs two sequential requests. The JWT already carries the + // user id in `sub`; fetch the profile directly and keep `/auth/me` as the + // fallback for tokens whose subject can't be decoded. + const subject = getTokenSubject(token); const MAX_RETRIES = 3; for (let attempt = 0; attempt < MAX_RETRIES; attempt++) { try { - const session = await apiRequest<{ userId: string; username: string }>( - "/auth/me", - ); - const profile = await apiRequest(`/users/${session.userId}`); + const userId = + subject ?? + (await apiRequest<{ userId: string; username: string }>("/auth/me")).userId; + const profile = await apiRequest(`/users/${userId}`); setUser(profile); setDegraded(false); return profile; @@ -178,7 +172,6 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { }, []); return ( - {children} diff --git a/src/context/WalletContext.tsx b/src/context/WalletContext.tsx index 79543f50..14ced30d 100644 --- a/src/context/WalletContext.tsx +++ b/src/context/WalletContext.tsx @@ -22,6 +22,33 @@ import { useCrossTabStorage } from "@/hooks/useCrossTabStorage"; const WALLET_KEY = "mergefi_wallet_address"; +// Best-effort storage helpers, same policy as lib/auth.ts: storage access +// throws in some private modes / with blocked site data, and that must never +// break the mount effect, logout sync or wallet linking. +function readStoredWallet(): string | null { + try { + return window.localStorage.getItem(WALLET_KEY); + } catch { + return null; + } +} + +function writeStoredWallet(address: string) { + try { + window.localStorage.setItem(WALLET_KEY, address); + } catch { + // In-memory state is still correct for this session. + } +} + +function clearStoredWallet() { + try { + window.localStorage.removeItem(WALLET_KEY); + } catch { + // Ignore — the in-memory state is what the UI renders from. + } +} + /** * Durability of the current wallet connection. * @@ -132,7 +159,7 @@ export function WalletProvider({ children }: { children: React.ReactNode }) { // so this provider (mounted on every route) doesn't add to the // critical path to interactivity for routes that don't need it yet. const id = window.setTimeout(() => { - const stored = window.localStorage.getItem(WALLET_KEY); + const stored = readStoredWallet(); if (stored) { setAddress(stored); // This app only ever signs against the build's configured network, @@ -205,7 +232,7 @@ export function WalletProvider({ children }: { children: React.ReactNode }) { useEffect(() => { if (authLoading) return; if (user === null && addressRef.current !== null) { - window.localStorage.removeItem(WALLET_KEY); + clearStoredWallet(); setNetwork(null); setAddress(null); setAddressMismatch(false); @@ -287,7 +314,7 @@ export function WalletProvider({ children }: { children: React.ReactNode }) { setNetwork(STELLAR_NETWORK); setAddressMismatch(false); setNetworkMismatch(false); - window.localStorage.setItem(WALLET_KEY, walletAddress); + writeStoredWallet(walletAddress); if (user) { try { diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 1c4b6b75..f324e915 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -26,3 +26,21 @@ export function clearToken() { // Best-effort, same as setToken above. } } + +/** + * Read the `sub` claim (the user id) out of a JWT without verifying it. + * This is only used to skip a round trip: the backend still authenticates the + * token on the `/users/:id` request. Returns null for anything malformed. + */ +export function getTokenSubject(token: string): string | null { + try { + const payload = token.split(".")[1]; + if (!payload) return null; + const b64 = payload.replace(/-/g, "+").replace(/_/g, "/"); + const json = atob(b64.padEnd(b64.length + ((4 - (b64.length % 4)) % 4), "=")); + const sub = (JSON.parse(json) as { sub?: unknown }).sub; + return typeof sub === "string" && sub.length > 0 ? sub : null; + } catch { + return null; + } +} diff --git a/src/lib/return-to.test.ts b/src/lib/return-to.test.ts new file mode 100644 index 00000000..c8ed30d4 --- /dev/null +++ b/src/lib/return-to.test.ts @@ -0,0 +1,37 @@ +import { sanitizeReturnTo, storeReturnTo, takeReturnTo } from "./return-to"; + +describe("sanitizeReturnTo", () => { + it("accepts same-origin paths", () => { + expect(sanitizeReturnTo("/issues/42?tab=claims#top")).toBe("/issues/42?tab=claims#top"); + }); + + it.each([ + "https://evil.example", + "//evil.example", + "/\\evil.example", + "javascript:alert(1)", + "issues/1", + "/connect", + "/auth/callback", + "/a\nb", + "", + null, + ])("rejects %p", (value) => { + expect(sanitizeReturnTo(value as string | null)).toBeNull(); + }); +}); + +describe("returnTo storage", () => { + afterEach(() => window.sessionStorage.clear()); + + it("round-trips once and then clears", () => { + storeReturnTo("/issues/7"); + expect(takeReturnTo()).toBe("/issues/7"); + expect(takeReturnTo()).toBeNull(); + }); + + it("does not store an unsafe value", () => { + storeReturnTo("//evil.example"); + expect(takeReturnTo()).toBeNull(); + }); +}); diff --git a/src/lib/return-to.ts b/src/lib/return-to.ts new file mode 100644 index 00000000..d49d22e7 --- /dev/null +++ b/src/lib/return-to.ts @@ -0,0 +1,37 @@ +const RETURN_TO_KEY = "mergefi_return_to"; + +/** + * Validate a post-sign-in destination. Only same-origin absolute paths are + * accepted (no scheme, no `//host`, no backslash tricks, no control chars), so + * a crafted `?returnTo=` can't turn the sign-in flow into an open redirect. + * Sign-in pages themselves are rejected to avoid redirect loops. + */ +export function sanitizeReturnTo(value: string | null | undefined): string | null { + if (!value || value.length > 2048) return null; + if (!value.startsWith("/") || value.startsWith("//")) return null; + if (/[\\\u0000-\u001f\u007f]/.test(value)) return null; + if (/^\/(connect|auth)(\/|\?|#|$)/.test(value)) return null; + return value; +} + +/** Remember the destination across the cross-origin GitHub OAuth round trip. */ +export function storeReturnTo(value: string | null | undefined) { + const safe = sanitizeReturnTo(value); + try { + if (safe) window.sessionStorage.setItem(RETURN_TO_KEY, safe); + else window.sessionStorage.removeItem(RETURN_TO_KEY); + } catch { + // Best-effort (storage can throw in private modes): sign-in still works. + } +} + +/** Read and clear the remembered destination, re-validating it. */ +export function takeReturnTo(): string | null { + try { + const value = window.sessionStorage.getItem(RETURN_TO_KEY); + window.sessionStorage.removeItem(RETURN_TO_KEY); + return sanitizeReturnTo(value); + } catch { + return null; + } +} diff --git a/src/lib/utils.test.ts b/src/lib/utils.test.ts index 2f2e9195..497617f5 100644 --- a/src/lib/utils.test.ts +++ b/src/lib/utils.test.ts @@ -401,6 +401,22 @@ describe("generateIdempotencyKey", () => { const keys = new Set(Array.from({ length: 50 }, () => generateIdempotencyKey())); expect(keys.size).toBe(50); }); + + it("builds a v4 UUID from getRandomValues when randomUUID is unavailable", () => { + const original = crypto.randomUUID; + Object.defineProperty(crypto, "randomUUID", { value: undefined, configurable: true }); + const spy = jest.spyOn(Math, "random"); + try { + const key = generateIdempotencyKey(); + expect(key).toMatch( + /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, + ); + expect(spy).not.toHaveBeenCalled(); + } finally { + spy.mockRestore(); + Object.defineProperty(crypto, "randomUUID", { value: original, configurable: true }); + } + }); }); // ─── toCents / toMajorUnits ──────────────────────────────────────────────── diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 1b1356e0..b04c143f 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -385,12 +385,16 @@ export function generateIdempotencyKey(): string { if (typeof crypto !== "undefined" && crypto.randomUUID) { return crypto.randomUUID(); } - // Fallback for environments without crypto.randomUUID (very unlikely in - // modern browsers, but defensive). - return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, (c) => { - const r = (Math.random() * 16) | 0; - return (c === "x" ? r : (r & 0x3) | 0x8).toString(16); - }); + // crypto.randomUUID only exists in secure contexts, so a plain-http staging + // deploy lands here. crypto.getRandomValues is available on insecure + // origins; Math.random is predictable and collision-prone, and a collision + // replays another request's cached response instead of executing. + const bytes = new Uint8Array(16); + crypto.getRandomValues(bytes); + bytes[6] = (bytes[6] & 0x0f) | 0x40; // version 4 + bytes[8] = (bytes[8] & 0x3f) | 0x80; // RFC 4122 variant + const hex = Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join(""); + return `${hex.slice(0, 8)}-${hex.slice(8, 12)}-${hex.slice(12, 16)}-${hex.slice(16, 20)}-${hex.slice(20)}`; } // ---------------------------------------------------------------------------