mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): use hard reload after login to reload metadata (#24239)
After password login, `navigate('/')` performed a client-side SPA
navigation, leaving the pre-authentication `<meta>` tags (including
`userAppearance`) empty in the DOM. The `ThemeProvider` read empty
metadata and fell through to `DEFAULT_THEME` until the API query
resolved, causing a visible flash of the wrong theme. A page refresh
fixed it because the server re-rendered HTML with the session cookie
present.
Replace `navigate('/')` with `window.location.href` so the server
re-renders HTML with all metadata tags populated (`userAppearance`,
`user`, `permissions`, etc.) via the new session cookie. This matches
the pattern already used for API route redirects on the same page. Also
uses `redirectTo` instead of hardcoded "/" so the redirect query
parameter is respected for password login.
Fixes https://github.com/coder/coder/issues/20050
> [!NOTE]
> Generated by Coder Agents
<details><summary>Decision log</summary>
- Chose `window.location.href` over invalidating React Query caches
because a hard reload is the only way to get fresh server-rendered meta
tags. The API query approach still has a flash between mount and
response.
- Sanitized redirect URL with `redirectUrl.pathname` (same as existing
`<Navigate>` path) to prevent open redirects via absolute URLs.
- Removed unused `useNavigate` import.
</details>
---------
Co-authored-by: Kayla はな <kayla@tree.camp>
This commit is contained in:
co-authored by
Kayla はな
parent
1feb183a87
commit
a2b9b74f4a
@@ -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: <LoginPage />,
|
||||
},
|
||||
{
|
||||
path: "/",
|
||||
element: <h1>Home</h1>,
|
||||
},
|
||||
],
|
||||
{ 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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 `<Navigate>`, 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}
|
||||
/>
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user