diff --git a/src/dashboard/auth.ts b/src/dashboard/auth.ts index 511680f..4a708ce 100644 --- a/src/dashboard/auth.ts +++ b/src/dashboard/auth.ts @@ -199,6 +199,22 @@ async function userInAnyOrg(token: string, login: string, allowedOrgs: string[]) return null; } +/** + * A `next`/return-to value we're willing to redirect a freshly-signed-in user + * to. Must be a same-origin absolute path — this covers both the SPA routes + * (served at `/`, e.g. `/overview`, `/repos/acme/x/pr/1`) and the legacy + * dashboard (`/dashboard/...`). Reject protocol-relative (`//evil.com`) and + * backslash (`/\evil.com`) forms so this can never become an open redirect. + */ +function isSafeReturnPath(next: unknown): next is string { + return ( + typeof next === "string" && + next.startsWith("/") && + !next.startsWith("//") && + !next.startsWith("/\\") + ); +} + export function createAuth(cfg: AuthConfig | null): AuthRuntime | null { if (!cfg) return null; @@ -217,7 +233,7 @@ export function createAuth(cfg: AuthConfig | null): AuthRuntime | null { const routes: AuthRuntime["routes"] = (router) => { router.get("/auth/login", (req, res) => { const state = crypto.randomBytes(18).toString("base64url"); - const next_ = typeof req.query.next === "string" ? req.query.next : "/dashboard"; + const next_ = isSafeReturnPath(req.query.next) ? req.query.next : "/dashboard"; const stateBody = Buffer.from(JSON.stringify({ state, next: next_, iat: Math.floor(Date.now() / 1000) })).toString("base64url"); const stateSig = sign(cfg.sessionSecret, stateBody); setCookie(res, STATE_COOKIE, `${stateBody}.${stateSig}`, 600); @@ -304,7 +320,7 @@ export function createAuth(cfg: AuthConfig | null): AuthRuntime | null { { login: user.login, via: loginMatch ? "login-allowlist" : `org:${matchingOrg}` }, "Dashboard login", ); - const nextUrl = parsedState.next && parsedState.next.startsWith("/dashboard") ? parsedState.next : "/dashboard"; + const nextUrl = isSafeReturnPath(parsedState.next) ? parsedState.next : "/dashboard"; res.redirect(nextUrl); }); diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 2136abe..99a9348 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -33,6 +33,29 @@ export class ApiError extends Error { const BASE = "/api/v1"; +// A 401 from the JSON API means the cookie session has expired or was never +// established. Every request the SPA makes is cookie-based, and the server only +// answers 401 when OAuth is enabled (open mode never gates on a session), so a +// 401 unambiguously means "sign in again". Bounce the whole page to the server +// login route — it 302s to GitHub OAuth and returns here afterwards (`next`). +// Without this the SPA just paints an "Authentication required" error card with +// no visible way to re-auth (the old workaround was to hunt for /dashboard, +// sign out, then sign back in). +let redirectingToLogin = false; + +function redirectToLogin(path: string): void { + // Nothing to redirect in SSR/tests or demo mode. Public reads (/public/*) are + // served BEFORE the auth gate, so a 401 there isn't a session problem — never + // hijack the no-auth share viewer into a GitHub login. + if (typeof window === "undefined" || DEMO || path.startsWith("/public/")) return; + // A screen fires many queries at once; the first 401 wins the redirect and the + // rest are no-ops, so we never stack navigations. + if (redirectingToLogin) return; + redirectingToLogin = true; + const returnTo = window.location.pathname + window.location.search + window.location.hash; + window.location.assign(`/dashboard/auth/login?next=${encodeURIComponent(returnTo)}`); +} + export async function apiGet(path: string, params?: Record): Promise { const url = new URL(BASE + path, window.location.origin); if (params) { @@ -71,6 +94,9 @@ export async function apiGet(path: string, params?: Record( } if (!resp.ok) { + // A write that 401s means the session lapsed mid-action — re-auth, keeping + // the user on the page they were working from. + if (resp.status === 401) redirectToLogin(path); const body = (json as { error?: ApiErrorBody })?.error; throw new ApiError(resp.status, body ?? { code: "http_error", message: `HTTP ${resp.status}` }); }