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