diff --git a/site/src/pages/LoginPage/LoginPage.test.tsx b/site/src/pages/LoginPage/LoginPage.test.tsx index 7cf06e92eb..3d27e98440 100644 --- a/site/src/pages/LoginPage/LoginPage.test.tsx +++ b/site/src/pages/LoginPage/LoginPage.test.tsx @@ -12,6 +12,11 @@ import { server } from "#/testHelpers/server"; import LoginPage from "./LoginPage"; describe("LoginPage", () => { + // Capture original values before any test stubs take effect. + const origLocationOrigin = window.location.origin; + const origLocationHref = window.location.href; + const locationHrefSpy = vi.fn(); + beforeEach(() => { server.use( // Appear logged out @@ -19,6 +24,22 @@ describe("LoginPage", () => { return HttpResponse.json({ message: "no user here" }, { status: 401 }); }), ); + // Stub the location global so tests can intercept server-side redirects + // without actually navigating away. + vi.stubGlobal("location", { + origin: origLocationOrigin, + get href() { + return origLocationHref; + }, + set href(url: string) { + locationHrefSpy(url); + }, + }); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + locationHrefSpy.mockReset(); }); it("shows an error message if SignIn fails", async () => { @@ -91,6 +112,7 @@ describe("LoginPage", () => { ); expect(document.getElementById("signin-password-error")).toBeNull(); }); + it("redirects to the setup page if there is no first user", async () => { // Given server.use( @@ -121,6 +143,55 @@ describe("LoginPage", () => { await screen.findByText("Setup"); }); + it("navigates to the home page after successful password login", 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 + renderWithRouter( + createMemoryRouter( + [ + { + path: "/login", + element: , + }, + { + path: "/", + element:

Home

, + }, + ], + { initialEntries: ["/login"] }, + ), + ); + + 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 component uses React Router navigation for standard + // redirects so the new session cookie is picked up by the next route. + await screen.findByText("Home"); + }); + it("redirects to /oauth2/authorize via server-side redirect when signed in", async () => { // Given - user is signed in server.use( @@ -132,23 +203,6 @@ describe("LoginPage", () => { const redirectPath = "/oauth2/authorize?client_id=xxx&response_type=code&redirect_uri=https%3A%2F%2Fexample.com%2Fcallback"; - // Spy on window.location.href assignment - const locationHrefSpy = vi.fn(); - const originalLocation = window.location; - Object.defineProperty(window, "location", { - configurable: true, - value: { - ...originalLocation, - origin: originalLocation.origin, - set href(url: string) { - locationHrefSpy(url); - }, - get href() { - return originalLocation.href; - }, - }, - }); - // When renderWithRouter( createMemoryRouter( @@ -166,17 +220,10 @@ describe("LoginPage", () => { ), ); - // Then - it should perform a server-side redirect, not a React navigate + // Then - the full redirect path (including query params) must be + // preserved for the OAuth2 authorization flow to complete. await waitFor(() => { - expect(locationHrefSpy).toHaveBeenCalledWith( - expect.stringContaining("/oauth2/authorize"), - ); - }); - - // Cleanup - Object.defineProperty(window, "location", { - configurable: true, - value: originalLocation, + expect(locationHrefSpy).toHaveBeenCalledWith(redirectPath); }); }); @@ -204,24 +251,6 @@ describe("LoginPage", () => { const redirectPath = "/oauth2/authorize?client_id=xxx&response_type=code&redirect_uri=https%3A%2F%2Fexample.com%2Fcallback"; - // Spy on window.location.href - const originalLocation = window.location; - const locationHrefSpy = vi.fn(); - - Object.defineProperty(window, "location", { - configurable: true, - value: { - ...originalLocation, - origin: originalLocation.origin, - set href(url: string) { - locationHrefSpy(url); - }, - get href() { - return originalLocation.href; - }, - }, - }); - // When renderWithRouter( createMemoryRouter( @@ -241,26 +270,14 @@ describe("LoginPage", () => { await waitForLoaderToBeRemoved(); - const email = screen.getByLabelText(/Email/); - const password = screen.getByLabelText(/Password/); + await userEvent.type(screen.getByLabelText(/Email/), "test@coder.com"); + await userEvent.type(screen.getByLabelText(/Password/), "password"); + fireEvent.click(await screen.findByText("Sign In")); - await userEvent.type(email, "test@coder.com"); - await userEvent.type(password, "password"); - - const signInButton = await screen.findByText("Sign In"); - fireEvent.click(signInButton); - - // Then - it should hard redirect to OAuth endpoint + // Then - the full redirect path (including query params) must be + // preserved for the OAuth2 authorization flow to complete. await waitFor(() => { - expect(locationHrefSpy).toHaveBeenCalledWith( - expect.stringContaining("/oauth2/authorize"), - ); - }); - - // Cleanup - Object.defineProperty(window, "location", { - configurable: true, - value: originalLocation, + expect(locationHrefSpy).toHaveBeenCalledWith(redirectPath); }); }); }); diff --git a/site/src/pages/LoginPage/LoginPage.tsx b/site/src/pages/LoginPage/LoginPage.tsx index a7323e2740..a745f7f033 100644 --- a/site/src/pages/LoginPage/LoginPage.tsx +++ b/site/src/pages/LoginPage/LoginPage.tsx @@ -1,17 +1,17 @@ import { type FC, useEffect } from "react"; import { useQuery } from "react-query"; -import { Navigate, useLocation, useNavigate } from "react-router"; +import { Navigate, useLocation } from "react-router"; import { buildInfo } from "#/api/queries/buildInfo"; import { authMethods } from "#/api/queries/users"; import { useAuthContext } from "#/contexts/auth/AuthProvider"; import { useEmbeddedMetadata } from "#/hooks/useEmbeddedMetadata"; import { getApplicationName } from "#/utils/appearance"; -import { retrieveRedirect } from "#/utils/redirect"; +import { retrieveRedirect, sanitizeRedirect } from "#/utils/redirect"; import { sendDeploymentEvent } from "#/utils/telemetry"; import { LoginPageView } from "./LoginPageView"; const LoginPage: FC = () => { - const location = useLocation(); + const routerLocation = useLocation(); const { isLoading, isSignedIn, @@ -22,9 +22,8 @@ const LoginPage: FC = () => { user, } = useAuthContext(); const authMethodsQuery = useQuery(authMethods()); - const redirectTo = retrieveRedirect(location.search); + const redirectTo = retrieveRedirect(routerLocation.search); const applicationName = getApplicationName(); - const navigate = useNavigate(); const { metadata } = useEmbeddedMetadata(); const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); let redirectError: Error | null = null; @@ -52,13 +51,12 @@ const LoginPage: FC = () => { }, [isSignedIn, buildInfoQuery.data, user?.id]); if (isSignedIn) { - // The reason we need `window.location.href` for api redirects is that + // The reason we need `location.href` for api redirects is that // we need the page to reload and make a request to the backend. If we // use ``, react would handle the redirect itself and never // request the page from the backend. if (isApiRouteRedirect) { - const sanitizedUrl = new URL(redirectTo, window.location.origin); - window.location.href = sanitizedUrl.pathname + sanitizedUrl.search; + location.href = sanitizeRedirect(redirectTo); // Setting the href should immediately request a new page. Show an // error state if it doesn't. redirectError = new Error("unable to redirect"); @@ -87,7 +85,11 @@ const LoginPage: FC = () => { isSigningIn={isSigningIn} onSignIn={async ({ email, password }) => { await signIn(email, password); - navigate("/"); + // Use a hard reload instead of React Router navigation + // so the server re-renders the HTML with all metadata + // tags populated (userAppearance, user, permissions, + // etc.) using the new session cookie. + location.href = sanitizeRedirect(redirectTo); }} redirectTo={redirectTo} /> diff --git a/site/src/utils/redirect.test.ts b/site/src/utils/redirect.test.ts index 3007845e85..7b6a8f0587 100644 --- a/site/src/utils/redirect.test.ts +++ b/site/src/utils/redirect.test.ts @@ -1,4 +1,4 @@ -import { embedRedirect, retrieveRedirect } from "./redirect"; +import { embedRedirect, retrieveRedirect, sanitizeRedirect } from "./redirect"; describe("redirect helper functions", () => { describe("embedRedirect", () => { @@ -17,4 +17,20 @@ describe("redirect helper functions", () => { expect(result).toEqual("/workspaces"); }); }); + + describe("sanitizeRedirect", () => { + it("is a no-op for a relative path", () => { + expect(sanitizeRedirect("/bar/baz")).toEqual("/bar/baz"); + }); + it("removes the origin from url", () => { + expect(sanitizeRedirect("http://www.evil.com/bar/baz")).toEqual( + "/bar/baz", + ); + }); + it("preserves search params", () => { + expect( + sanitizeRedirect("https://www.example.com/bar?baz=1&quux=2"), + ).toEqual("/bar?baz=1&quux=2"); + }); + }); }); diff --git a/site/src/utils/redirect.ts b/site/src/utils/redirect.ts index 3ae0a69d30..39dd2a17b4 100644 --- a/site/src/utils/redirect.ts +++ b/site/src/utils/redirect.ts @@ -21,3 +21,11 @@ export const retrieveRedirect = (search: string): string => { const redirect = searchParams.get("redirect"); return redirect ? redirect : defaultRedirect; }; + +/** + * Ensures the redirect is not an open redirect, aka it's relative + */ +export const sanitizeRedirect = (redirectTo: string) => { + const sanitizedUrl = new URL(redirectTo, location.origin); + return sanitizedUrl.pathname + sanitizedUrl.search; +};