From 32ddd8e659340fdeafe9e50537e4c6b01b33491d Mon Sep 17 00:00:00 2001 From: Brian Fox <878612+onematchfox@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:48:47 +0200 Subject: [PATCH] fix(ui): preserve post-login redirect target through oauth2-proxy sign-in When oauth2-proxy intercepts an unauthenticated request it serves its sign-in page carrying the original destination as `.Redirect` (e.g. `/oauth2/sign_in?rd=%2Fagents%2Ffoo`). `sign_in.html` template ignored that and unconditionally redirected to `/login`, and `/login`'s "Sign in with SSO" link was hardcoded to `rd=/` -- so any login, expired- cookie or not, always landed back on the home page instead of the page the user was trying to reach. Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com> --- helm/kagent/templates/_helpers.tpl | 25 +++++++ .../templates/oauth2-proxy-templates.yaml | 12 +-- helm/kagent/values.yaml | 3 + ui/src/auth/loginRedirect.test.ts | 61 +++++++++++++++ ui/src/auth/loginRedirect.ts | 40 ++++++++++ ui/src/auth/reauthenticate.ts | 15 +++- ui/src/pages/LoginPage.test.tsx | 74 +++++++++++++++++++ ui/src/pages/LoginPage.tsx | 45 +++++++---- 8 files changed, 249 insertions(+), 26 deletions(-) create mode 100644 ui/src/auth/loginRedirect.test.ts create mode 100644 ui/src/auth/loginRedirect.ts create mode 100644 ui/src/pages/LoginPage.test.tsx diff --git a/helm/kagent/templates/_helpers.tpl b/helm/kagent/templates/_helpers.tpl index 760d210baa..1e9fd445fa 100644 --- a/helm/kagent/templates/_helpers.tpl +++ b/helm/kagent/templates/_helpers.tpl @@ -294,3 +294,28 @@ kagent.substrate.ateApiEndpoint. {{- include "substrate.atenetRouter.url" . -}} {{- end -}} {{- end -}} + +{{/* +Body of oauth2-proxy's custom sign_in.html template (see +templates/oauth2-proxy-templates.yaml). Kept as its own named template, rather +than inline in that ConfigMap, so oauth2-proxy.extraEnv in values.yaml can hash +the content. + +oauth2-proxy renders this as its own Go html/template (not a Helm template) when +it shows the sign-in page to an unauthenticated visitor -- e.g. a request to +/agents/foo is served this page at /oauth2/sign_in?rd=%2Fagents%2Ffoo. +`Redirect` is oauth2-proxy's template variable carrying that original +destination (escaped with a Helm string-literal action so Helm emits it for +oauth2-proxy to evaluate, instead of trying to evaluate it itself). It is +forwarded to kagent's branded /login page. +*/}} +{{- define "kagent.oauth2ProxySignInHTML" -}} + + + + + + +Redirecting to login... + +{{- end -}} diff --git a/helm/kagent/templates/oauth2-proxy-templates.yaml b/helm/kagent/templates/oauth2-proxy-templates.yaml index 0223c9d219..a153383066 100644 --- a/helm/kagent/templates/oauth2-proxy-templates.yaml +++ b/helm/kagent/templates/oauth2-proxy-templates.yaml @@ -7,13 +7,9 @@ metadata: labels: {{- include "kagent.labels" . | nindent 4 }} data: + # The body lives in the kagent.oauth2ProxySignInHTML named template + # (_helpers.tpl) so oauth2-proxy.extraEnv in values.yaml can hash the content to + # force a rollout when it changes. sign_in.html: | - - - - - - - Redirecting to login... - + {{- include "kagent.oauth2ProxySignInHTML" . | nindent 4 }} {{- end }} diff --git a/helm/kagent/values.yaml b/helm/kagent/values.yaml index 775faeafe0..fa68e70973 100644 --- a/helm/kagent/values.yaml +++ b/helm/kagent/values.yaml @@ -788,6 +788,9 @@ oauth2-proxy: # Cluster-specific OIDC settings - override these per deployment # These are set as env vars and referenced in args for easy patching extraEnv: + # Forces a rollout whenever the sign_in.html ConfigMap's content changes. + - name: KAGENT_OAUTH2_PROXY_SIGNIN_TEMPLATE_CHECKSUM + value: '{{ include "kagent.oauth2ProxySignInHTML" . | sha256sum }}' - name: OIDC_ISSUER_URL value: "" - name: OIDC_REDIRECT_URL diff --git a/ui/src/auth/loginRedirect.test.ts b/ui/src/auth/loginRedirect.test.ts new file mode 100644 index 0000000000..095f5bf402 --- /dev/null +++ b/ui/src/auth/loginRedirect.test.ts @@ -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"); + }); +}); diff --git a/ui/src/auth/loginRedirect.ts b/ui/src/auth/loginRedirect.ts new file mode 100644 index 0000000000..2238029dd3 --- /dev/null +++ b/ui/src/auth/loginRedirect.ts @@ -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 "/"; + } +} diff --git a/ui/src/auth/reauthenticate.ts b/ui/src/auth/reauthenticate.ts index 456af378b9..b372be9003 100644 --- a/ui/src/auth/reauthenticate.ts +++ b/ui/src/auth/reauthenticate.ts @@ -40,17 +40,26 @@ function returnTo(location: Pick): str } /** - * The URL that restarts the flow and comes back here. + * 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, ): string { - const start = runtimeConfig().ssoRedirectPath; - return `${start}?rd=${encodeURIComponent(returnTo(location))}`; + return ssoStartUrl(returnTo(location)); } /** diff --git a/ui/src/pages/LoginPage.test.tsx b/ui/src/pages/LoginPage.test.tsx new file mode 100644 index 0000000000..ead26335cb --- /dev/null +++ b/ui/src/pages/LoginPage.test.tsx @@ -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( + + + + + , + ); +} + +/** + * 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"); + }); +}); diff --git a/ui/src/pages/LoginPage.tsx b/ui/src/pages/LoginPage.tsx index a1e9412b4d..c5a940f628 100644 --- a/ui/src/pages/LoginPage.tsx +++ b/ui/src/pages/LoginPage.tsx @@ -1,8 +1,9 @@ import { Button, Card, Typography } from "antd"; import { useTheme } from "@emotion/react"; -import { useNavigate } from "react-router-dom"; +import { useNavigate, useSearchParams } from "react-router-dom"; import { paths } from "@/router/routes"; -import { reauthenticationUrl } from "@/auth/reauthenticate"; +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 = () => { - window.location.assign(reauthenticationUrl(window.location)); + const rd = searchParams.get("rd"); + + // 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. + // 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)), + ); }; const copy = status === "expired" ? { - blurb: "Your session has expired. Sign in again to continue.", - action: "Sign in with SSO", - onClick: startSso, - } + blurb: "Your session has expired. Sign in again to continue.", + action: "Sign in with SSO", + onClick: startSso, + } : status === "authenticated" ? { - blurb: `Signed in as ${user?.displayName ?? "your account"}.`, - action: "Continue", - onClick: enterApp, - } + 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, - }; + blurb: "No authentication proxy is configured for this deployment.", + action: "Continue", + onClick: enterApp, + }; return (