fix(ui): preserve post-login redirect target through oauth2-proxy sign-in

[onematchfox]

#2532 - against main

[Charlesthebird]

The UI on main has been updated to Vite + React Router instead of Next JS. So the UI folder changes will need to be moved around, and this will need to get the search params using useSearchParams.

[onematchfox]

The UI on main has been updated to Vite + React Router instead of Next JS. So the UI folder changes will need to be moved around, and this will need to get the search params using useSearchParams.

Done.

[Charlesthebird]

This automated review is a first pass. A manual review will follow once the blockers are addressed.

---

Nothing blocks merge. The core fix works end to end, and both points from the last round are addressed. One Medium: the no-rd fallback branch is unreachable in the real app, and the test covering it asserts a state the router cannot produce.

What I checked

• ✅ sanitizeRedirect returns / for 17 vectors beyond its own tests — ///host, ////host, \\host, \/host, a leading space, tab or newline before //host, https:host, https:/\host, /..//host, //host\@kagent.invalid/, and http://kagent-login-redirect.invalid//evil.
• ✅ /%2F%2Fhost stays percent-encoded through the parser, so it resolves as a path segment rather than a host.
• ✅ Rendered the named template in a minimal chart: sign_in.html emits {{ or .Redirect "/" | urlquery }} verbatim, and the checksum env var resolves to 41963eed….
• ✅ oauth2-proxy 10.7's deployment.yaml:257 runs tpl over extraEnv, while podAnnotations at line 51 is plain toYaml — a checksum/config annotation would have shipped the literal {{ include … }}.
• ✅ The ConfigMap is volume-mounted at values.yaml:751-759, and oauth2-proxy parses /templates once at startup.
• ✅ Executed the template through Go's html/template: urlquery is not double-applied in the content= attribute or the <script> string, and /a"><script>alert(1)</script> emits as %2Fa%22%3E%3Cscript%3E….
• ✅ oauth2-proxy's own IsValidRedirect rejects // and /\ independently, so a miss in the UI guard is caught downstream.
• ✅ AuthProvider.tsx:81 calls startReauthentication() while still on the original page, so the automatic path's rd was already correct and stays untouched.

---

🤖 written by Claude

[Charlesthebird]

Level: 🟠 Medium · Not Blocking

AppHeader.tsx:45 does navigate(paths.login), a pushState onto a standalone route, so startSso reads /login from window.location and this branch always produces rd=%2Flogin.

LoginPage.test.tsx sets window.location.pathname = "/agents/foo" while rendering at /login, which the router cannot produce, so the test passes on a case that never occurs. playwright/tests/auth/auth-modes.spec.ts:101 already reaches that button, so the path is live.

The fix belongs in the unchanged src/components/Structure/AppHeader.tsx:45, so the destination arrives here as rd:

onClick={() =>
  navigate(
    `${paths.login}?rd=${encodeURIComponent(location.pathname + location.search + location.hash)}`,
  )
}

---

🤖 written by Claude

[Charlesthebird]

Level: 🟡 Low · Not Blocking

No helm-unittest suite in helm/kagent/tests/ covers the new ConfigMap, and asserting sign_in.html contains {{ or .Redirect "/" | urlquery }} verbatim is what catches the {{ "{{" }} escaping breaking silently.

---

🤖 written by Claude

[Charlesthebird]

Level: 🟡 Low · Not Blocking

The checksum sits in the list line 788 tells operators to override per deployment, so replacing extraEnv drops the rollout trigger without a word.

Suggested change

	# Forces a rollout whenever the sign_in.html ConfigMap's content changes.
	# Forces a rollout whenever the sign_in.html ConfigMap's content changes.
	# Keep this entry if you override extraEnv, or template changes will not restart the proxy.

---

🤖 written by Claude

[Charlesthebird]

[nit]
Level: 🟡 Low · Not Blocking

This ternary's three object literals were reindented away from the surrounding style (compare AgentTemplateForm.tsx:379 and mcpServerRequest.ts:233), which accounts for 30 of the file's 45 changed lines.

---

🤖 written by Claude

[Charlesthebird]

[nit]
Level: 🟡 Low · Not Blocking

Trailing whitespace, the only such line in ui/src.

Suggested change

	// page they were reading.
	// page they were reading.

---

🤖 written by Claude

[Charlesthebird]

[nit]
Level: 🟡 Low · Not Blocking

?rd= yields "" rather than null, so it takes the sanitize branch below and lands on / — harmless, though the rd === null split reads as if it covers that case.

---

🤖 written by Claude

[Charlesthebird]

Thanks for updating this! It looks good to me.

I kicked off the automated review above, and the one medium test note might be worth addressing, but I don't think it's a blocker.