From 057ff4aaa5b188f3963ad43217bc9ac1680b93a4 Mon Sep 17 00:00:00 2001 From: juprodh Date: Sat, 22 Aug 2026 13:09:17 +0800 Subject: [PATCH] Redirect only to a path on this site after set-theme and set-locale Signed-off-by: juprodh --- src/atproto/routes.ts | 25 ++--------- src/lib/safe-redirect.ts | 31 ++++++++++++++ src/server/routes/locale.ts | 5 +-- src/server/routes/theme.ts | 5 +-- tests/lib/safe-redirect.test.ts | 66 ++++++++++++++++++++++++++++++ tests/server/routes/locale.test.ts | 17 ++++++++ tests/server/routes/theme.test.ts | 39 ++++++++++++++++++ 7 files changed, 161 insertions(+), 27 deletions(-) create mode 100644 src/lib/safe-redirect.ts create mode 100644 tests/lib/safe-redirect.test.ts create mode 100644 tests/server/routes/theme.test.ts diff --git a/src/atproto/routes.ts b/src/atproto/routes.ts index 4e91c92..d642b04 100644 --- a/src/atproto/routes.ts +++ b/src/atproto/routes.ts @@ -6,6 +6,7 @@ import { fmt, resolveLocale, t } from "../lib/i18n/index.ts"; import { LIMITS } from "../lib/limits.ts"; import { rateLimitByIp } from "../lib/rate-limit.ts"; import { htmlResponse } from "../lib/response.ts"; +import { safeRedirectPath } from "../lib/safe-redirect.ts"; import { createAppSession, deleteAppSession, @@ -14,30 +15,12 @@ import { loginPage } from "../views/login.ts"; import { getDevAccounts } from "./env.ts"; import { getClient, getSessionFromRequest } from "./session.ts"; -// Validate the post-login return path to prevent open redirects. The old -// `startsWith("/") && !includes("//")` check let `/\evil.com` through — browsers -// normalize the backslash to `//evil.com` and leave the site. Resolve against a -// placeholder origin and require the origin to be unchanged, returning only -// path+search+hash. Defaults to "/" on anything suspicious or malformed. +// The post-login destination, read from the returnTo cookie set at /login. export function getSafeReturnTo(cookieHeader: string | null): string { const match = cookieHeader?.match(/(?:^|;\s*)returnTo=([^;]+)/); - let raw: string | null = null; + if (!match?.[1]) return "/"; try { - raw = match?.[1] ? decodeURIComponent(match[1]) : null; - } catch { - return "/"; - } - if (!raw) return "/"; - if (!raw.startsWith("/") || raw.startsWith("//") || raw.startsWith("/\\")) { - return "/"; - } - try { - const base = "https://placeholder.invalid"; - const u = new URL(raw, base); - // A backslash, %2f, etc. that normalizes to a new authority changes the - // origin away from the placeholder → reject. - if (u.origin !== base) return "/"; - return u.pathname + u.search + u.hash; + return safeRedirectPath(decodeURIComponent(match[1])); } catch { return "/"; } diff --git a/src/lib/safe-redirect.ts b/src/lib/safe-redirect.ts new file mode 100644 index 0000000..d07f528 --- /dev/null +++ b/src/lib/safe-redirect.ts @@ -0,0 +1,31 @@ +import { resolvePublicUrl } from "./urls.ts"; + +// Only a path that cannot name another origin: "//evil.com" and "/\evil.com" +// both leave the site. Checked after parsing, which is what a browser acts on. +export function safeRedirectPath(raw: string | null | undefined): string { + if (!raw?.startsWith("/")) return "/"; + try { + const base = "https://placeholder.invalid"; + const url = new URL(raw, base); + if (url.origin !== base) return "/"; + const path = url.pathname + url.search + url.hash; + return path.startsWith("//") || path.startsWith("/\\") ? "/" : path; + } catch { + return "/"; + } +} + +// Where a form that has no page of its own sends the visitor back to. Referer is +// the client's to set, so it decides nothing beyond a path on this site. +export function safeRefererPath(request: Request): string { + const referer = request.headers.get("referer"); + if (!referer) return "/"; + const site = resolvePublicUrl(request); + try { + const url = new URL(referer, site); + if (url.host !== new URL(site).host) return "/"; + return safeRedirectPath(url.pathname + url.search + url.hash); + } catch { + return "/"; + } +} diff --git a/src/server/routes/locale.ts b/src/server/routes/locale.ts index cd394ae..83db7d0 100644 --- a/src/server/routes/locale.ts +++ b/src/server/routes/locale.ts @@ -1,6 +1,7 @@ import { Elysia } from "elysia"; import { type Locale, SUPPORTED_LOCALES } from "../../lib/i18n/index.ts"; import { LIMITS } from "../../lib/limits.ts"; +import { safeRefererPath } from "../../lib/safe-redirect.ts"; export const localeRoutes = new Elysia().post( "/set-locale", @@ -12,12 +13,10 @@ export const localeRoutes = new Elysia().post( return new Response("Invalid locale", { status: 400 }); } - const referer = request.headers.get("referer") || "/"; - return new Response(null, { status: 302, headers: { - Location: referer, + Location: safeRefererPath(request), "Set-Cookie": `locale=${locale}; Path=/; SameSite=Lax; Max-Age=${LIMITS.cacheTtl.cookie}`, }, }); diff --git a/src/server/routes/theme.ts b/src/server/routes/theme.ts index df3f901..54761f8 100644 --- a/src/server/routes/theme.ts +++ b/src/server/routes/theme.ts @@ -1,5 +1,6 @@ import { Elysia } from "elysia"; import { LIMITS } from "../../lib/limits.ts"; +import { safeRefererPath } from "../../lib/safe-redirect.ts"; import { USER_THEMES, type UserTheme } from "../../views/theme/index.ts"; export const themeRoutes = new Elysia().post( @@ -12,12 +13,10 @@ export const themeRoutes = new Elysia().post( return new Response("Invalid theme", { status: 400 }); } - const referer = request.headers.get("referer") || "/"; - return new Response(null, { status: 302, headers: { - Location: referer, + Location: safeRefererPath(request), "Set-Cookie": `theme=${theme}; Path=/; SameSite=Lax; Max-Age=${LIMITS.cacheTtl.cookie}`, }, }); diff --git a/tests/lib/safe-redirect.test.ts b/tests/lib/safe-redirect.test.ts new file mode 100644 index 0000000..512ca80 --- /dev/null +++ b/tests/lib/safe-redirect.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, test } from "bun:test"; +import { + safeRedirectPath, + safeRefererPath, +} from "../../src/lib/safe-redirect.ts"; + +function withReferer( + referer: string | null, + url = "http://localhost/set-theme", +) { + const headers = referer ? { Referer: referer } : undefined; + return safeRefererPath(new Request(url, { headers })); +} + +describe("safeRedirectPath", () => { + test("keeps a path on this site whole", () => { + expect(safeRedirectPath("/")).toBe("/"); + expect(safeRedirectPath("/@alice/wiki/note?x=1#h")).toBe( + "/@alice/wiki/note?x=1#h", + ); + }); + + test("refuses anything that can name another origin", () => { + for (const raw of [ + "//evil.com", + "/\\evil.com", + "/\t\\evil.com", + "/\\/evil.com", + "https://evil.com", + "evil.com", + "", + null, + ]) { + expect(safeRedirectPath(raw)).toBe("/"); + } + }); +}); + +describe("safeRefererPath", () => { + test("returns the path a same-site referer names", () => { + expect(withReferer("http://localhost/@alice/wiki?x=1#h")).toBe( + "/@alice/wiki?x=1#h", + ); + }); + + test("a referer on another host sends the visitor to /", () => { + expect(withReferer("https://evil.com/phish")).toBe("/"); + expect(withReferer("https://localhost.evil.com/phish")).toBe("/"); + }); + + test("matches on host, since TLS terminates at the proxy", () => { + expect( + withReferer( + "https://lichen.wiki/@alice/wiki", + "http://lichen.wiki/set-theme", + ), + ).toBe("/@alice/wiki"); + }); + + test("a relative or missing referer is still a path on this site", () => { + expect(withReferer("/@alice/wiki")).toBe("/@alice/wiki"); + expect(withReferer("//evil.com/phish")).toBe("/"); + expect(withReferer(null)).toBe("/"); + expect(withReferer("http://")).toBe("/"); + }); +}); diff --git a/tests/server/routes/locale.test.ts b/tests/server/routes/locale.test.ts index 1ef90ad..85f29d1 100644 --- a/tests/server/routes/locale.test.ts +++ b/tests/server/routes/locale.test.ts @@ -36,6 +36,23 @@ describe("locale route", () => { expect(res.headers.get("Location")).toBe("/wiki/my-wiki"); }); + test("a cross-site referer cannot redirect off the apex", async () => { + const body = new URLSearchParams({ locale: "en" }); + const res = await app.handle( + new Request("http://localhost/set-locale", { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Referer: "https://evil.com/phish", + }, + body: body.toString(), + }), + ); + + expect(res.status).toBe(302); + expect(res.headers.get("Location")).toBe("/"); + }); + test("POST /set-locale rejects invalid locale", async () => { const body = new URLSearchParams({ locale: "xx" }); const res = await app.handle( diff --git a/tests/server/routes/theme.test.ts b/tests/server/routes/theme.test.ts new file mode 100644 index 0000000..424a19a --- /dev/null +++ b/tests/server/routes/theme.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, test } from "bun:test"; +import { Elysia } from "elysia"; +import { themeRoutes } from "../../../src/server/routes/theme.ts"; + +const app = new Elysia().use(themeRoutes); + +function setTheme(theme: string, referer?: string) { + return app.handle( + new Request("http://localhost/set-theme", { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + ...(referer ? { Referer: referer } : {}), + }, + body: new URLSearchParams({ theme }).toString(), + }), + ); +} + +describe("theme route", () => { + test("POST /set-theme sets the cookie and returns to the page", async () => { + const res = await setTheme("dark", "http://localhost/@alice/wiki"); + + expect(res.status).toBe(302); + expect(res.headers.get("Set-Cookie") ?? "").toContain("theme=dark"); + expect(res.headers.get("Location")).toBe("/@alice/wiki"); + }); + + test("a cross-site referer cannot redirect off the apex", async () => { + const res = await setTheme("dark", "https://evil.com/phish"); + + expect(res.status).toBe(302); + expect(res.headers.get("Location")).toBe("/"); + }); + + test("POST /set-theme rejects an unknown theme", async () => { + expect((await setTheme("constructor")).status).toBe(400); + }); +}); -- 2.51.2