Pre-launch — Gluecron is in final validation. Public signups and git hosting for non-owner users open after launch review.
ccantynz/Gluecron.com
Commite9a4574

fix(auth): validate redirect targets instead of following them blindly

fix(auth): validate redirect targets instead of following them blindly

Every `?redirect=` sink in the auth flow passed the query value straight
to c.redirect(), with no same-origin check anywhere in the codebase —
there was no such helper to call. /login?redirect=https://evil.example
sent the browser to the attacker's site from a link carrying our own
domain. That is the classic phishing primitive, and a sign-in page is the
worst place for it: the victim has just been asked for a password and the
attacker picks where they land immediately afterwards.

Five sinks in auth.tsx (register completion, the already-authed /login
bounce, POST /login success, and both 2FA paths) plus one form-supplied
target in personal-chat.tsx.

safeRedirect accepts a rooted relative path and nothing else. Absolute
URLs are refused even when they point at our own host — allowing them
means parsing and comparing origins, and origin comparison is where this
class of bug comes from. Also refused: protocol-relative //host, the
backslash variants browsers fold into it, scheme URLs, and control
characters (header injection). Hono percent-decodes the query before a
handler sees it, so %2f%2f and %0d%0a arrive already decoded and are
caught as the plain forms.

Dropped from this pass after reading them: OAuth `state` is used for CSRF
matching and redirects to fixed local paths, and SAML RelayState is
generated server-side by generateId — neither is caller-controlled.

The test asserts the sinks call the helper, not just that the helper
works, because the bug was an absent check. Both fault classes proven by
injection: restoring one bare sink fails 1 test, dropping the
protocol-relative guard fails 3.

Suite: 3461 pass, 0 fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ccantynz-alt committed on July 29, 2026Parent: 303c5e2
4 files changed+189−6e9a4574cbb04ce7d41470269944347a94a88e301
4 changed files+189−6
Addedsrc/__tests__/safe-redirect.test.ts+117−0View fileUnifiedSplit
1/**
2 * Regression guard: open redirect on the sign-in flow.
3 *
4 * Every `?redirect=` sink in auth.tsx passed the query value straight to
5 * `c.redirect()`. `/login?redirect=https://evil.example` sent the browser to
6 * the attacker's site from a link carrying the platform's own domain — the
7 * classic phishing primitive, and at its worst on a sign-in page, because
8 * the victim has just been asked for a password and the attacker chooses
9 * where they land immediately afterwards.
10 *
11 * Hono percent-decodes the query value before a handler sees it, so
12 * `%2f%2fevil.example` arrives as `//evil.example` and `%0d%0a` arrives as
13 * real control characters. The decoded value is what these cases cover.
14 */
15
16import { describe, it, expect } from "bun:test";
17import { readFileSync } from "node:fs";
18import { join } from "node:path";
19import { safeRedirect, isSafeRedirect } from "../lib/safe-redirect";
20
21describe("isSafeRedirect rejects off-origin targets", () => {
22 const attacks: Array<[string, string]> = [
23 ["https://evil.example", "absolute URL"],
24 ["http://evil.example/x", "absolute URL, plain http"],
25 ["//evil.example", "protocol-relative — browsers treat it as absolute"],
26 ["//evil.example/path?a=b", "protocol-relative with a path"],
27 ["/\\evil.example", "browsers fold the backslash to a slash"],
28 ["\\\\evil.example", "UNC-style, folds to protocol-relative"],
29 ["javascript:alert(1)", "scheme URL"],
30 ["data:text/html,<script>", "data URL"],
31 [" /dashboard", "leading whitespace, no leading slash"],
32 ["dashboard", "bare relative path, not rooted"],
33 ["", "empty"],
34 ];
35
36 for (const [value, why] of attacks) {
37 it(`rejects ${JSON.stringify(value)} (${why})`, () => {
38 expect(isSafeRedirect(value)).toBe(false);
39 expect(safeRedirect(value, "/dashboard")).toBe("/dashboard");
40 });
41 }
42
43 it("rejects control characters (header injection)", () => {
44 expect(isSafeRedirect("/foo\r\nSet-Cookie: a=b")).toBe(false);
45 expect(isSafeRedirect("/foo\nLocation: https://evil.example")).toBe(false);
46 expect(isSafeRedirect("/foo\tbar")).toBe(false);
47 expect(isSafeRedirect("/foo\x00bar")).toBe(false);
48 });
49
50 it("rejects non-strings", () => {
51 expect(isSafeRedirect(undefined)).toBe(false);
52 expect(isSafeRedirect(null)).toBe(false);
53 expect(isSafeRedirect(42)).toBe(false);
54 expect(isSafeRedirect(["/a"])).toBe(false);
55 });
56});
57
58describe("isSafeRedirect accepts ordinary same-origin paths", () => {
59 const ok = [
60 "/",
61 "/dashboard",
62 "/onboarding?welcome=1",
63 "/ccantynz/Gluecron.com/pulls?state=open",
64 "/settings/tokens#new",
65 "/a/b/c/d/e",
66 "/repo/with-dash_and.dot",
67 "/search?q=hello+world&page=2",
68 ];
69
70 for (const value of ok) {
71 it(`accepts ${JSON.stringify(value)}`, () => {
72 expect(isSafeRedirect(value)).toBe(true);
73 expect(safeRedirect(value, "/fallback")).toBe(value);
74 });
75 }
76
77 it("defaults the fallback to the site root", () => {
78 expect(safeRedirect("https://evil.example")).toBe("/");
79 });
80});
81
82// --- the sinks must actually use it ----------------------------------------
83//
84// The bug was an absent check, so testing the helper alone would not have
85// caught it. Strip ONLY line comments: a block-comment regex eats route
86// paths, which contain a slash followed by a star.
87
88function sourceWithoutLineComments(...rel: string[]): string {
89 const text = readFileSync(join(import.meta.dir, "..", ...rel), "utf8");
90 const stripped = text
91 .split("\n")
92 .filter((l) => !l.trim().startsWith("//"))
93 .join("\n");
94 expect(stripped.length).toBeGreaterThan(0);
95 return stripped;
96}
97
98describe("redirect sinks are validated", () => {
99 it("auth.tsx never reads the redirect query without validating it", () => {
100 const src = sourceWithoutLineComments("routes", "auth.tsx");
101 // Every read of the parameter must be wrapped. If a new sink is added
102 // with a bare `c.req.query("redirect")`, this fails.
103 const bare = src.match(/(?<!safeRedirect\(\s*)c\.req\.query\("redirect"\)/g);
104 expect(bare).toBeNull();
105 // ...and the helper is genuinely in use, so the check above cannot pass
106 // vacuously by the sinks having been deleted.
107 expect(src).toContain("safeRedirect(c.req.query(\"redirect\")");
108 const uses = src.split("safeRedirect(").length - 1;
109 expect(uses).toBeGreaterThanOrEqual(5);
110 });
111
112 it("personal-chat.tsx validates its form-supplied redirect", () => {
113 const src = sourceWithoutLineComments("routes", "personal-chat.tsx");
114 expect(src).toContain("safeRedirect(body.redirect");
115 expect(src).not.toContain('String(body.redirect || "/chat")');
116 });
117});
Addedsrc/lib/safe-redirect.ts+58−0View fileUnifiedSplit
1/**
2 * Same-origin validation for user-supplied redirect targets.
3 *
4 * Every `?redirect=` sink in the auth flow used to pass the query value
5 * straight to `c.redirect()`. `/login?redirect=https://evil.example` sent the
6 * browser to the attacker's site carrying the platform's own domain in the
7 * link the victim clicked — the classic phishing primitive, and worse on a
8 * sign-in page than anywhere else, because the victim has just been asked to
9 * type a password and the attacker controls where they land immediately
10 * after. It also lets an attacker bounce a freshly-authenticated user to a
11 * page that harvests whatever the app puts in the URL.
12 *
13 * The policy here is deliberately narrow: accept a relative path on this
14 * origin, reject everything else. Absolute URLs are refused even when they
15 * point at our own host — allowing them means parsing and comparing origins,
16 * and origin comparison is exactly where these bugs come from.
17 *
18 * Rejected, with the reason each one matters:
19 *
20 * https://evil.example absolute URL, different origin
21 * //evil.example protocol-relative; browsers treat it as absolute
22 * /\evil.example browsers fold a backslash to a slash, so this
23 * behaves like the protocol-relative case above
24 * javascript:alert(1) scheme URL, no leading slash
25 * /foo\r\nSet-Cookie: x control characters, i.e. header injection
26 * (empty / missing) nothing to honour
27 *
28 * Accepted: `/dashboard`, `/owner/repo/pulls?state=open#tab`, `/` — a single
29 * leading slash, no backslash anywhere, no control characters.
30 *
31 * Note on encoding: Hono has already percent-decoded the query value by the
32 * time it reaches here, so `%2f%2fevil.example` arrives as `//evil.example`
33 * and `%0d%0a` arrives as real control characters. Both are caught above.
34 * Validate the decoded value — never the raw one.
35 */
36
37/**
38 * A single leading slash, not followed by another slash, then any run of
39 * characters that are neither backslashes nor C0/C1 control characters.
40 */
41const SAFE_PATH = /^\/(?!\/)[^\\\x00-\x1f\x7f-\x9f]*$/;
42
43/** True if `value` is a relative path safe to redirect to on this origin. */
44export function isSafeRedirect(value: unknown): value is string {
45 if (typeof value !== "string") return false;
46 if (value.length === 0) return false;
47 return SAFE_PATH.test(value);
48}
49
50/**
51 * Return `value` when it is a safe same-origin path, otherwise `fallback`.
52 *
53 * Always use this — never the raw query value — when building a `Location`
54 * header from anything the caller supplied.
55 */
56export function safeRedirect(value: unknown, fallback: string = "/"): string {
57 return isSafeRedirect(value) ? value : fallback;
58}
Modifiedsrc/routes/auth.tsx+12−5View fileUnifiedSplit
5151} from "../views/ui";
5252import { softAuth } from "../middleware/auth";
5353import type { AuthEnv } from "../middleware/auth";
54import { safeRedirect } from "../lib/safe-redirect";
5455
5556const auth = new Hono<AuthEnv>();
5657
329330
330331 // P3 — default landing is /onboarding (the guided first-five-minutes
331332 // flow). The `redirect=` query is still honoured for OAuth-style flows.
332 const redirect = c.req.query("redirect") || "/onboarding?welcome=1";
333 const redirect = safeRedirect(
334 c.req.query("redirect"),
335 "/onboarding?welcome=1"
336 );
333337 return c.redirect(redirect);
334338});
335339
339343 const existing = c.get("user");
340344 const error = c.req.query("error");
341345 const success = c.req.query("success");
342 const redirect = c.req.query("redirect") || "";
346 // Empty fallback is deliberate: the sign-in form needs to know whether a
347 // target was supplied at all. An unsafe one is dropped to "" so it is
348 // neither followed here nor carried into the form below.
349 const redirect = safeRedirect(c.req.query("redirect"), "");
343350 if (existing) return c.redirect(redirect || "/dashboard");
344351 const ssoCfg = await getSsoConfig();
345352 const ssoEnabled =
412419 const body = await c.req.parseBody();
413420 const identifier = String(body.username || "").trim();
414421 const password = String(body.password || "");
415 const redirect = c.req.query("redirect") || "/";
422 const redirect = safeRedirect(c.req.query("redirect"), "/");
416423 const ip =
417424 c.req.header("x-forwarded-for")?.split(",")[0]?.trim() ||
418425 c.req.header("x-real-ip") ||
590597 const token = getCookie(c, "session");
591598 if (!token) return c.redirect("/login");
592599 const error = c.req.query("error");
593 const redirect = c.req.query("redirect") || "/";
600 const redirect = safeRedirect(c.req.query("redirect"), "/");
594601 return c.html(
595602 <Layout title="Two-factor authentication" user={null}>
596603 <AuthMobileStyle />
639646 if (!token) return c.redirect("/login");
640647 const body = await c.req.parseBody();
641648 const code = String(body.code || "").trim();
642 const redirect = c.req.query("redirect") || "/";
649 const redirect = safeRedirect(c.req.query("redirect"), "/");
643650
644651 if (!code) {
645652 return c.redirect(
Modifiedsrc/routes/personal-chat.tsx+2−1View fileUnifiedSplit
4141import { getUnreadCount } from "../lib/unread";
4242import { isAiAvailable } from "../lib/ai-client";
4343import { audit } from "../lib/notify";
44import { safeRedirect } from "../lib/safe-redirect";
4445import {
4546 appendPersonalUserMessage,
4647 createPersonalChat,
208209 metadata: { enabled: !!newValue, requested },
209210 });
210211
211 const redirectTo = String(body.redirect || "/chat");
212 const redirectTo = safeRedirect(body.redirect, "/chat");
212213 return c.redirect(
213214 redirectTo +
214215 (redirectTo.includes("?") ? "&" : "?") +
215216