fix(site/src): prevent protocol-relative login redirects (#27363)

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: <LoginPage />,
+					},
+					{
+						path: "/",
+						element: <h1>Home</h1>,
+					},
+				],
+				{
+					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(

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 (
-				<Navigate
-					to={redirectUrl ? redirectUrl.pathname : redirectTo}
-					replace
-				/>
-			);
+			return <Navigate to={sanitizeRedirect(redirectTo)} replace />;
 		}
 	}
 

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("/");
+			});
+		});
 	});
 });

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;
+	}
 };