fix(k8s-client): redirect to oauth2-proxy on 401 instead of failing silently#34
fix(k8s-client): redirect to oauth2-proxy on 401 instead of failing silently#34IvanHunters wants to merge 1 commit into
Conversation
…ilently When the kc-access cookie expires, k8s API calls come back with 401 (after the matching cozystack PR makes oauth2-proxy answer 401 instead of redirecting). The previous behaviour left the SPA stuck on a stale page until the user cleared the cache. K8sClient now treats 401 as an auth boundary: it fires a one-shot top-level navigation to `/oauth2/start?rd=<current-url>`, which is the oauth2-proxy convention for re-authenticating and bouncing the user back. The handler is overridable through `K8sClientConfig.onUnauthorized` for non-oauth2-proxy deployments and is a no-op outside the browser. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughK8sClient now supports callback-driven handling of HTTP 401 unauthorized responses. A new optional ChangesUnauthorized response callback handling
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request adds unauthorized request handling to the K8sClient via an optional onUnauthorized callback, which defaults to redirecting the user to an OAuth2 start page. Feedback on the changes highlights an issue where the unauthorizedHandled flag remains true indefinitely if a custom handler is used in a Single Page Application (SPA) context without a full page reload. It is recommended to provide a way to reset this flag, either automatically on successful requests or via a public method.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private handleUnauthorized(): void { | ||
| if (this.unauthorizedHandled) return | ||
| this.unauthorizedHandled = true | ||
| this.onUnauthorized() | ||
| } |
There was a problem hiding this comment.
If onUnauthorized is overridden with a custom handler that does not trigger a full page reload (e.g., showing a login modal, performing a silent token refresh, or navigating via a SPA router), unauthorizedHandled will remain true indefinitely. This prevents any subsequent session expirations from triggering the unauthorized handler again during the lifetime of the SPA.
To support these scenarios, we should expose a public method to reset this flag once re-authentication succeeds. Additionally, you may want to automatically reset this.unauthorizedHandled = false upon any successful request (where res.ok is true) in request and watch.
public resetUnauthorized(): void {
this.unauthorizedHandled = false
}
private handleUnauthorized(): void {
if (this.unauthorizedHandled) return
this.unauthorizedHandled = true
this.onUnauthorized()
}
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the change is correct and well-scoped, but it ships new auth behavior with no tests, which this repo's mandatory-TDD policy treats as a blocker.
Business context: when the oauth2-proxy session expires mid-session, Kubernetes API calls return 401 and currently fail silently (React Query doesn't retry 401, and the connection-error banner deliberately ignores 401/403); this PR redirects to /oauth2/start?rd=<current-url> so the user re-authenticates.
Blockers
B1: New 401-redirect behavior has no test coverage
File: packages/k8s-client/src/client.ts
Issue: The PR adds a public onUnauthorized config field, a default redirect side-effect, a stateful one-shot guard (unauthorizedHandled), and two new 401 call sites (request() and watch()) — with no accompanying test.
Evidence: No test references onUnauthorized / handleUnauthorized / 401 under apps/console/src/__tests__/k8s-client/. The repo's CLAUDE.md mandates TDD ("No code without corresponding tests"), and the client test harness already exists (client.subresource.test.ts stubs fetch via vi.stubGlobal and constructs a K8sClient with config), so the gap is cheap to close.
Impact: The subtlest logic in the diff — the one-shot guard and the 401-vs-403 distinction the design depends on — is unprotected against regression. A later refactor of the status check or the guard could silently re-introduce redirect storms, or redirect on 403 (which would break the in-page permission notices that rely on 403 being handled gracefully).
Fix: Add tests, each failing without the change and passing with it:
request()→ 401 callsonUnauthorizedonce; 403 does not; a 2xx does not.watch()initial fetch → 401 callsonUnauthorized(and the stream still rejects viaonError).- one-shot guard: two 401s on one client instance call
onUnauthorizedexactly once. - default handler builds
/oauth2/start?rd=<encodeURIComponent(pathname + search + hash)>(stubwindow.location, assertassign).
Non-blocking follow-ups
- The redirect assumes oauth2-proxy owns
/oauth2/*upstream of nginx. The bundled nginx config doesn't proxy/oauth2/*(itslocation /falls through totry_files … /index.html), so hitting/oauth2/startagainst nginx alone would serve the SPA (200), not redirect. This holds in the documented production topology (oauth2-proxy in front) and matches the existing/oauth2/userinfoand/oauth2/sign_outusage — flagging only so the dependency is conscious; no change needed. rdincludeswindow.location.hash. Browsers don't send the fragment to the server on a top-level navigation, so oauth2-proxy can't honor it; the encoded hash is harmless but inert. Optional to drop.
Summary
When the
kc-accesscookie expires, k8s API calls come back with401(after the matching cozystack/cozystack PR makesoauth2-proxyanswer401for/apiand/apisinstead of redirecting). Previously the SPA was left stuck on a stale page until the user manually cleared the cache.K8sClientnow treats401as an auth boundary: it fires a one-shot top-level navigation to/oauth2/start?rd=<current-url>, which is theoauth2-proxyconvention for re-authenticating and bouncing the user back. The handler is overridable throughK8sClientConfig.onUnauthorizedfor non-oauth2-proxydeployments and is a no-op outside the browser.Related: cozystack/cozystack#2788
Test plan
pnpm typecheckpasses/oauth2/start?rd=...and the user is bounced back after re-auth401s do not cause repeated redirects (unauthorizedHandledlatch)Summary by CodeRabbit