Blame · Line-by-line history
safe-redirect.test.ts
Each line is annotated with the commit that last touched it. Click any SHA to jump to that commit and see the surrounding change.
| e9a4574 | 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 | ||
| 16 | import { describe, it, expect } from "bun:test"; | |
| 17 | import { readFileSync } from "node:fs"; | |
| 18 | import { join } from "node:path"; | |
| 19 | import { safeRedirect, isSafeRedirect } from "../lib/safe-redirect"; | |
| 20 | ||
| 21 | describe("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 | ||
| 58 | describe("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 | ||
| 88 | function 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 | ||
| 98 | describe("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 | }); |