fix(ui): preserve post-login redirect target through oauth2-proxy sign-in
#2533
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { sanitizeRedirect } from "./loginRedirect"; | ||
|
|
||
| /** | ||
| * `rd` arrives from the query string, so every case here is a link somebody | ||
| * could send. The ones that must not survive are the ones that leave the origin. | ||
| */ | ||
| describe("sanitizeRedirect", () => { | ||
| it("keeps a same-origin path", () => { | ||
| expect(sanitizeRedirect("/agents/kagent/k8s-agent/chat")).toBe( | ||
| "/agents/kagent/k8s-agent/chat", | ||
| ); | ||
| }); | ||
|
|
||
| it("keeps the query and fragment with it", () => { | ||
| expect(sanitizeRedirect("/agents/foo?tab=logs#latest")).toBe( | ||
| "/agents/foo?tab=logs#latest", | ||
| ); | ||
| }); | ||
|
|
||
| it("falls back to the front door when there is no destination", () => { | ||
| expect(sanitizeRedirect(undefined)).toBe("/"); | ||
| expect(sanitizeRedirect(null)).toBe("/"); | ||
| expect(sanitizeRedirect("")).toBe("/"); | ||
| }); | ||
|
|
||
| it("rejects an absolute URL", () => { | ||
| expect(sanitizeRedirect("https://evil.example.com/phish")).toBe("/"); | ||
| }); | ||
|
|
||
| it("rejects a protocol-relative URL", () => { | ||
| expect(sanitizeRedirect("//evil.example.com/phish")).toBe("/"); | ||
| }); | ||
|
|
||
| it("rejects a backslash the URL Standard reads as a second slash", () => { | ||
| expect(sanitizeRedirect("/\\evil.example.com/phish")).toBe("/"); | ||
| }); | ||
|
|
||
| it("rejects a tab-smuggled protocol-relative URL", () => { | ||
| // The parser strips the tab before resolving, so this is `//evil...`. | ||
| expect(sanitizeRedirect("/\t/evil.example.com/phish")).toBe("/"); | ||
| }); | ||
|
|
||
| it("rejects a dot segment that normalizes back into a protocol-relative path", () => { | ||
| // Same-origin to the parser, but `.` is resolved away and what comes out is | ||
| // `//evil.example.com/phish` — protocol-relative again for whoever reads it next. | ||
| expect(sanitizeRedirect("/.//evil.example.com/phish")).toBe("/"); | ||
| expect(sanitizeRedirect("/a/../..//evil.example.com/phish")).toBe("/"); | ||
| expect(sanitizeRedirect("/./\\evil.example.com/phish")).toBe("/"); | ||
| }); | ||
|
|
||
| it("rejects a different scheme entirely", () => { | ||
| expect(sanitizeRedirect("javascript:alert(1)")).toBe("/"); | ||
| }); | ||
|
|
||
| it("treats a bare host with no leading slash as a path segment", () => { | ||
| // Matches URL semantics: with no scheme and no leading "/", this resolves | ||
| // against the current path rather than naming a new host. | ||
| expect(sanitizeRedirect("evil.example.com/phish")).toBe("/evil.example.com/phish"); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
| * Validating the destination oauth2-proxy hands back to the sign-in page. | ||
| * | ||
| * An unauthenticated request to `/agents/foo` is answered by oauth2-proxy's | ||
| * `sign_in.html`, which forwards to `/login?rd=%2Fagents%2Ffoo`. That `rd` is | ||
| * attacker-controllable — a crafted `/login?rd=...` link is a URL anybody can | ||
| * send — and it is handed straight back to the proxy as the place to land after | ||
| * a successful sign-in. So it is checked here before it is used. | ||
| */ | ||
|
|
||
| // Any fixed placeholder works: it is never dereferenced, only used as the base | ||
| // for URL parsing so we can tell whether `rd` stayed same-origin. | ||
| const SENTINEL_ORIGIN = "http://kagent-login-redirect.invalid"; | ||
|
|
||
| /** | ||
| * The `rd` value if it is a same-origin path, `/` otherwise. | ||
| * | ||
| * Only a same-origin relative path is safe to return to. An absolute URL, a | ||
| * protocol-relative `//host/...`, or a disguised variant of either — a | ||
| * backslash, or a tab the URL Standard strips before parsing — would send an | ||
| * authenticated session off to somebody else's site the moment sign-in | ||
| * completed. | ||
| * | ||
| * The `//` check is on the *parsed* path rather than the input, because `.` and | ||
| * `..` segments are resolved away first: `/.//evil.example.com` is same-origin | ||
| * to the parser and normalizes to `//evil.example.com`, which is protocol- | ||
| * relative again by the time anything else reads it. | ||
| */ | ||
| export function sanitizeRedirect(rd: string | null | undefined): string { | ||
| if (!rd) return "/"; | ||
| try { | ||
| const url = new URL(rd, SENTINEL_ORIGIN); | ||
| if (url.origin !== SENTINEL_ORIGIN || url.pathname.startsWith("//")) { | ||
| return "/"; | ||
| } | ||
| return `${url.pathname}${url.search}${url.hash}`; | ||
| } catch { | ||
| return "/"; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,17 +40,26 @@ function returnTo(location: Pick<Location, "pathname" | "search" | "hash">): str | |
| } | ||
|
|
||
| /** | ||
| * The URL that restarts the flow and comes back to `target`. | ||
| * | ||
| * `rd` is oauth2-proxy's own parameter for it. Without it the proxy returns the reader | ||
| * to whatever it defaults to, which is how signing in again used to cost somebody the | ||
| * page they were reading. | ||
| * | ||
| * Takes the destination rather than reading `window.location`, because the sign-in page | ||
| * is the one place where those differ: a reader who was sent there by the proxy is | ||
| * *at* `/login`, and the page they wanted is in the query string. See `LoginPage`. | ||
| */ | ||
| export function ssoStartUrl(target: string): string { | ||
| const start = runtimeConfig().ssoRedirectPath; | ||
| return `${start}?rd=${encodeURIComponent(target)}`; | ||
| } | ||
|
|
||
| /** The URL that restarts the flow and comes back to the page being read. */ | ||
| export function reauthenticationUrl( | ||
| location: Pick<Location, "pathname" | "search" | "hash">, | ||
| ): string { | ||
| return ssoStartUrl(returnTo(location)); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { ThemeProvider } from "@emotion/react"; | ||
| import { MemoryRouter } from "react-router-dom"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { themeFor } from "@/theme/theme"; | ||
| import { LoginPage } from "./LoginPage"; | ||
|
|
||
| const useAuth = vi.hoisted(() => vi.fn()); | ||
|
|
||
| vi.mock("@/auth", () => ({ useAuth })); | ||
|
|
||
| const assign = vi.fn(); | ||
|
|
||
| beforeEach(() => { | ||
| assign.mockClear(); | ||
| useAuth.mockReturnValue({ status: "expired", user: undefined }); | ||
| window.environmentVariables = { SSO_REDIRECT_PATH: "/oauth2/start" }; | ||
| Object.defineProperty(window, "location", { | ||
| configurable: true, | ||
| value: { pathname: "/login", search: "", hash: "", assign }, | ||
| }); | ||
| }); | ||
|
|
||
| function renderAt(entry: string) { | ||
| render( | ||
| <ThemeProvider theme={themeFor("dark")}> | ||
| <MemoryRouter initialEntries={[entry]}> | ||
| <LoginPage /> | ||
| </MemoryRouter> | ||
| </ThemeProvider>, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * The deep link a signed-out reader followed, which oauth2-proxy's `sign_in.html` | ||
| * forwards here as `rd`. It is the whole point of the page carrying a query string. | ||
| */ | ||
| describe("LoginPage sign-in destination", () => { | ||
| it("returns the reader to the page the proxy intercepted", async () => { | ||
| renderAt("/login?rd=%2Fagents%2Fkagent%2Fk8s-agent%2Fchat"); | ||
|
|
||
| await userEvent.click(screen.getByTestId("login-submit")); | ||
|
|
||
| expect(assign).toHaveBeenCalledWith( | ||
| "/oauth2/start?rd=%2Fagents%2Fkagent%2Fk8s-agent%2Fchat", | ||
| ); | ||
| }); | ||
|
|
||
| it("refuses a destination that leaves the origin", async () => { | ||
| // `/login?rd=...` is a link anybody can send, so an off-site `rd` must not | ||
| // become where an authenticated session lands. | ||
| renderAt("/login?rd=https%3A%2F%2Fevil.example.com%2Fphish"); | ||
|
|
||
| await userEvent.click(screen.getByTestId("login-submit")); | ||
|
|
||
| expect(assign).toHaveBeenCalledWith("/oauth2/start?rd=%2F"); | ||
| }); | ||
|
|
||
| it("falls back to the page being read when there is no rd", async () => { | ||
| // Arriving from the header's "Session expired" button: nothing was intercepted, | ||
| // and the reader is still where they were. | ||
| Object.defineProperty(window, "location", { | ||
| configurable: true, | ||
| value: { pathname: "/agents/foo", search: "?tab=logs", hash: "", assign }, | ||
| }); | ||
|
|
||
| renderAt("/login"); | ||
|
|
||
| await userEvent.click(screen.getByTestId("login-submit")); | ||
|
|
||
| expect(assign).toHaveBeenCalledWith("/oauth2/start?rd=%2Fagents%2Ffoo%3Ftab%3Dlogs"); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,8 +1,9 @@ | ||||||
| import { Button, Card, Typography } from "antd"; | ||||||
| import { useTheme } from "@emotion/react"; | ||||||
| import { useNavigate, useSearchParams } from "react-router-dom"; | ||||||
| import { paths } from "@/router/routes"; | ||||||
| import { sanitizeRedirect } from "@/auth/loginRedirect"; | ||||||
| import { reauthenticationUrl, ssoStartUrl } from "@/auth/reauthenticate"; | ||||||
| import { useAuth } from "@/auth"; | ||||||
|
|
||||||
| const { Title, Paragraph, Text } = Typography; | ||||||
|
|
@@ -21,34 +22,48 @@ const { Title, Paragraph, Text } = Typography; | |||||
| export function LoginPage() { | ||||||
| const theme = useTheme(); | ||||||
| const navigate = useNavigate(); | ||||||
| const [searchParams] = useSearchParams(); | ||||||
| const { status, user } = useAuth(); | ||||||
|
|
||||||
| const enterApp = () => navigate(paths.dashboard); | ||||||
| // Carries `rd`, so signing in returns the reader to where they were rather than to | ||||||
| // whatever the proxy defaults to. Dropping it is how re-authenticating used to cost | ||||||
| // somebody the page they were reading. | ||||||
| // | ||||||
| // The forwarded `rd` is sanitized: it reaches this page through a query string, so a | ||||||
| // crafted `/login?rd=https://evil.example.com` link is a URL anybody can send. | ||||||
| const startSso = () => { | ||||||
| const rd = searchParams.get("rd"); | ||||||
|
Collaborator
There was a problem hiding this comment. [nit]
🤖 written by Claude |
||||||
|
|
||||||
| // There are two ways to arrive here: | ||||||
| // 1. A reader who clicked "Session expired" in the header is still on the | ||||||
| // page they were reading. | ||||||
|
Collaborator
There was a problem hiding this comment. [nit] Trailing whitespace, the only such line in
Suggested change
🤖 written by Claude |
||||||
| // 2. A reader who typed `/agents/foo` while signed out never got there at | ||||||
| // all: oauth2-proxy answered with its `sign_in.html`, which forwards to | ||||||
| // `/login?rd=%2Fagents%2Ffoo`. | ||||||
| window.location.assign( | ||||||
| rd === null ? reauthenticationUrl(window.location) : ssoStartUrl(sanitizeRedirect(rd)), | ||||||
|
Collaborator
There was a problem hiding this comment. Level: 🟠 Medium · Not Blocking
The fix belongs in the unchanged onClick={() =>
navigate(
`${paths.login}?rd=${encodeURIComponent(location.pathname + location.search + location.hash)}`,
)
}🤖 written by Claude |
||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
| const copy = | ||||||
| status === "expired" | ||||||
| ? { | ||||||
| blurb: "Your session has expired. Sign in again to continue.", | ||||||
|
Collaborator
There was a problem hiding this comment. [nit] This ternary's three object literals were reindented away from the surrounding style (compare 🤖 written by Claude |
||||||
| action: "Sign in with SSO", | ||||||
| onClick: startSso, | ||||||
| } | ||||||
| : status === "authenticated" | ||||||
| ? { | ||||||
| blurb: `Signed in as ${user?.displayName ?? "your account"}.`, | ||||||
| action: "Continue", | ||||||
| onClick: enterApp, | ||||||
| } | ||||||
| : { | ||||||
| blurb: "No authentication proxy is configured for this deployment.", | ||||||
| action: "Continue", | ||||||
| onClick: enterApp, | ||||||
| }; | ||||||
|
|
||||||
| return ( | ||||||
| <div | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Level: 🟡 Low · Not Blocking
No helm-unittest suite in
helm/kagent/tests/covers the new ConfigMap, and assertingsign_in.htmlcontains{{ or .Redirect "/" | urlquery }}verbatim is what catches the{{ "{{" }}escaping breaking silently.🤖 written by Claude