From 84f4a8bb17052537b870f16647d3b41d80738577 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 10 Aug 2026 15:35:06 +0200 Subject: [PATCH] fix(site/src): prevent protocol-relative login redirects (#27363) --- site/src/pages/LoginPage/LoginPage.test.tsx | 58 +++++++++++++++++++++ site/src/pages/LoginPage/LoginPage.tsx | 13 +---- site/src/utils/redirect.test.ts | 43 +++++++++++++++ site/src/utils/redirect.ts | 25 +++++++-- 4 files changed, 123 insertions(+), 16 deletions(-) diff --git a/site/src/pages/LoginPage/LoginPage.test.tsx b/site/src/pages/LoginPage/LoginPage.test.tsx index 88ab785260..3299c07e97 100644 --- a/site/src/pages/LoginPage/LoginPage.test.tsx +++ b/site/src/pages/LoginPage/LoginPage.test.tsx @@ -192,6 +192,64 @@ describe("LoginPage", () => { await screen.findByText("Home"); }); + it("does not follow a protocol-relative redirect after password login (CDM-02-001)", async () => { + // Given - user is NOT signed in + let loggedIn = false; + server.use( + http.get("/api/v2/users/me", () => { + if (!loggedIn) { + return HttpResponse.json( + { message: "no user here" }, + { status: 401 }, + ); + } + return HttpResponse.json(MockUserOwner); + }), + http.post("/api/v2/users/login", () => { + loggedIn = true; + return HttpResponse.json({ + session_token: "test-session-token", + }); + }), + ); + + // When - the redirect param decodes to https://cure53.de//cure53.de, + // whose pathname is the protocol-relative url //cure53.de. + renderWithRouter( + createMemoryRouter( + [ + { + path: "/login", + element: , + }, + { + path: "/", + element:

Home

, + }, + ], + { + initialEntries: ["/login?redirect=https://cure53.de/%2fcure53.de"], + }, + ), + ); + + await waitForLoaderToBeRemoved(); + + await userEvent.type(screen.getByLabelText(/Email/), "test@coder.com"); + await userEvent.type(screen.getByLabelText(/Password/), "password"); + fireEvent.click(await screen.findByText("Sign In")); + + // Then - the malicious redirect must be replaced with the fallback + // path on both navigation paths (hard reload and SPA navigation). + await waitFor(() => { + expect(locationHrefSpy).toHaveBeenCalledWith("/"); + }); + expect(locationHrefSpy).not.toHaveBeenCalledWith( + expect.stringContaining("//cure53.de"), + ); + await screen.findByText("Home"); + }); + it("redirects to /oauth2/authorize via server-side redirect when signed in", async () => { // Given - user is signed in server.use( diff --git a/site/src/pages/LoginPage/LoginPage.tsx b/site/src/pages/LoginPage/LoginPage.tsx index a745f7f033..d72d9cafa1 100644 --- a/site/src/pages/LoginPage/LoginPage.tsx +++ b/site/src/pages/LoginPage/LoginPage.tsx @@ -27,12 +27,6 @@ const LoginPage: FC = () => { const { metadata } = useEmbeddedMetadata(); const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); let redirectError: Error | null = null; - let redirectUrl: URL | null = null; - try { - redirectUrl = new URL(redirectTo); - } catch { - // Do nothing - } const isApiRouteRedirect = redirectTo.startsWith("/api/v2") || @@ -61,12 +55,7 @@ const LoginPage: FC = () => { // error state if it doesn't. redirectError = new Error("unable to redirect"); } else { - return ( - - ); + return ; } } diff --git a/site/src/utils/redirect.test.ts b/site/src/utils/redirect.test.ts index 7b6a8f0587..a57dfc1012 100644 --- a/site/src/utils/redirect.test.ts +++ b/site/src/utils/redirect.test.ts @@ -32,5 +32,48 @@ describe("redirect helper functions", () => { sanitizeRedirect("https://www.example.com/bar?baz=1&quux=2"), ).toEqual("/bar?baz=1&quux=2"); }); + it("drops the hash", () => { + expect(sanitizeRedirect("/foo?a=1#bar")).toEqual("/foo?a=1"); + }); + it("strips the authority of a protocol-relative url", () => { + expect(sanitizeRedirect("//evil.com/path")).toEqual("/path"); + }); + it("treats backslashes as slashes, not path characters", () => { + expect(sanitizeRedirect("/\\evil.com")).toEqual("/"); + }); + it("keeps an encoded slash encoded so it stays same-origin", () => { + expect(sanitizeRedirect("/%2fevil.com")).toEqual("/%2fevil.com"); + }); + + // Regression tests for Cure53 CDM-02-001: a URL's pathname can itself + // start with "//", and a string starting with "//" is a + // protocol-relative URL when assigned to `location.href`. None of + // these inputs may produce a redirect that leaves the origin. + describe("open redirect hardening (CDM-02-001)", () => { + it("rejects the PoC redirect after query-string decoding", () => { + // /login?redirect=https://cure53.de/%2fcure53.de is decoded + // once by URLSearchParams inside retrieveRedirect. + const redirect = retrieveRedirect( + "?redirect=https://cure53.de/%2fcure53.de", + ); + expect(redirect).toEqual("https://cure53.de//cure53.de"); + expect(sanitizeRedirect(redirect)).toEqual("/"); + }); + it("rejects a double-slash pathname in an absolute url", () => { + expect(sanitizeRedirect("https://cure53.de//cure53.de")).toEqual("/"); + }); + it("rejects a relative path that normalizes to protocol-relative", () => { + expect(sanitizeRedirect("/.//evil.com")).toEqual("/"); + }); + it("rejects dot-segment traversal that escapes a path prefix", () => { + expect(sanitizeRedirect("/api/v2/../../..//evil.com")).toEqual("/"); + }); + it("rejects tab characters stripped by the url parser", () => { + expect(sanitizeRedirect("https://x/\t/evil.com")).toEqual("/"); + }); + it("falls back to / for unparsable urls", () => { + expect(sanitizeRedirect("http://[invalid")).toEqual("/"); + }); + }); }); }); diff --git a/site/src/utils/redirect.ts b/site/src/utils/redirect.ts index 39dd2a17b4..7552665835 100644 --- a/site/src/utils/redirect.ts +++ b/site/src/utils/redirect.ts @@ -23,9 +23,26 @@ export const retrieveRedirect = (search: string): string => { }; /** - * Ensures the redirect is not an open redirect, aka it's relative + * Ensures the redirect is not an open redirect, aka it's relative. + * + * A parsed URL's pathname can itself start with "//" (via percent-encoded + * slashes, backslashes, or dot-segment normalization), and a string starting + * with "//" is a protocol-relative URL when assigned to `location.href`. + * Building a path is therefore not enough; the candidate is re-parsed + * against our own origin and rejected if it would resolve anywhere else. + * See Cure53 CDM-02-001 (coder/security-disclosures#164). */ -export const sanitizeRedirect = (redirectTo: string) => { - const sanitizedUrl = new URL(redirectTo, location.origin); - return sanitizedUrl.pathname + sanitizedUrl.search; +export const sanitizeRedirect = (redirectTo: string): string => { + const fallbackRedirect = "/"; + try { + const url = new URL(redirectTo, location.origin); + const candidate = url.pathname + url.search; + const resolved = new URL(candidate, location.origin); + if (resolved.origin !== location.origin) { + return fallbackRedirect; + } + return candidate; + } catch { + return fallbackRedirect; + } };