diff --git a/helm/kagent/templates/_helpers.tpl b/helm/kagent/templates/_helpers.tpl
index 760d210ba..1e9fd445f 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 0223c9d21..a15338306 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 775faeafe..fa68e7097 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 000000000..095f5bf40
--- /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 000000000..2238029dd
--- /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 456af378b..b372be900 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 000000000..ead26335c
--- /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 a1e9412b4..c5a940f62 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 (