fix: revoke OpenShift OAuth token server-side and prevent re-login loop on logout#709
Open
jkilzi wants to merge 7 commits into
Open
fix: revoke OpenShift OAuth token server-side and prevent re-login loop on logout#709jkilzi wants to merge 7 commits into
jkilzi wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughOpenShift logout now revokes the OAuth access token server-side, and the standalone UI records a logout flag so the login page does not immediately reselect a single provider after redirecting to /login. Tests cover token-name derivation, revocation requests, and redirect helper updates. ChangesOpenShift logout revocation
Standalone logout redirect state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 4 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/auth/helpers_test.go`:
- Around line 9-20: The test helper function redirectBaseMatchesRequest is
missing proper failure attribution. Add a *testing.T parameter as the first
argument to the redirectBaseMatchesRequest function signature, then call
t.Helper() at the beginning of the function body to ensure test failures are
correctly attributed to the caller rather than the helper itself. Update all
call sites of this function to pass the *testing.T value from the calling test.
In `@proxy/auth/openshift_test.go`:
- Around line 7-73: Consolidate the four separate Logout test functions
(TestOpenShiftLogout_ReturnsLogoutURLDerivedFromAuthURL,
TestOpenShiftLogout_StripsExistingPathAndQuery,
TestOpenShiftLogout_FallsBackGracefullyWhenAuthURLIsEmpty, and
TestOpenShiftLogout_FallsBackGracefullyWhenAuthURLIsInvalid) into a single
table-driven test. Create a test case struct containing fields for authURL,
token, redirectURL, expectedLogoutURL, and expectedError, then define a slice of
test cases capturing all four scenarios. Replace the individual test functions
with a single table-driven test that loops through cases using t.Run(), calling
the OpenShiftAuthHandler Logout method for each case and validating the returned
logout URL and error against expected values.
In `@proxy/auth/openshift.go`:
- Around line 141-145: The logout URL construction in the openshift.go file does
not validate the Scheme component of the parsed URL, which can result in a
malformed URL like "://.../logout" when parsing URLs with empty schemes such as
"//oauth.example.com/x". In the validation condition where you check for parsing
errors and empty Host (around line 141-142), add an additional check to ensure
parsed.Scheme is not empty. If the scheme is empty, the function should return
the graceful fallback of an empty string and nil, just like it does for parsing
errors or missing host values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 63794846-233e-4f07-9cd9-f66f5d7fcb43
📒 Files selected for processing (3)
proxy/auth/helpers_test.goproxy/auth/openshift.goproxy/auth/openshift_test.go
Collaborator
Author
|
Regarding the Unchecked-Errors warning: the |
jkilzi
added a commit
that referenced
this pull request
Jun 23, 2026
- Add parsed.Scheme guard to prevent malformed logout URL for scheme-less inputs like "//host/path" (url.Parse returns Host but empty Scheme) - Use url.URL struct to build logout URL instead of string concatenation - Add *testing.T + t.Helper() to redirectBaseMatchesRequest; log only scheme://host in error message to avoid leaking query parameters - Consolidate four separate Logout test functions into a single table-driven TestOpenShiftLogout per project conventions Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/auth/openshift_test.go`:
- Around line 25-34: The test suite for the OpenShift authentication handler is
missing a regression test case for scheme-less auth URLs in the format
`//host/path`. These URLs represent an edge case where URL parsing may populate
the Host field while leaving Scheme empty, which needs explicit coverage. Add a
new test case to the existing test table (alongside the "empty auth URL falls
back" and "invalid auth URL falls back" cases) with a name field describing the
scheme-less scenario, an authURL field containing a scheme-less URL like
`//example.com/path`, and a wantURL field set to empty string to verify it falls
back correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5ed145fe-7579-42a1-9ec8-607add991b58
📒 Files selected for processing (4)
proxy/auth/helpers_test.goproxy/auth/openshift.goproxy/auth/openshift_test.goproxy/auth/redirect_test.go
jkilzi
added a commit
that referenced
this pull request
Jun 23, 2026
- Add parsed.Scheme guard to prevent malformed logout URL for scheme-less inputs like "//host/path" (url.Parse returns Host but empty Scheme) - Use url.URL struct to build logout URL instead of string concatenation - Add *testing.T + t.Helper() to redirectBaseMatchesRequest; log only scheme://host in error message to avoid leaking query parameters - Consolidate four separate Logout test functions into a single table-driven TestOpenShiftLogout per project conventions Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com>
jkilzi
force-pushed
the
fix/openshift-logout
branch
from
4eec374 to
a152e3a
Compare
OpenShiftAuthHandler.Logout was returning ("", nil) since PR #393
(EDM-2612), which cleared only the FlightCtl session cookie but left
the OpenShift OAuth browser session active. Because the login page
auto-redirects when a single non-K8s provider is configured, users
were silently re-authenticated immediately after clicking Logout.
The original discovery-based approach ({apiServerURL}/.well-known/
oauth-authorization-server) was problematic when apiServerURL pointed
to the K8s API server, producing https://api.cluster:6443/logout as
the logout target — an invalid endpoint.
Fix: derive the logout URL directly from authURL (the OAuth
authorization endpoint). authURL always holds the OAuth server host
regardless of how apiServerURL is configured, so taking scheme://host
and appending /logout gives the correct
https://oauth-openshift.apps.{cluster}/logout without any HTTP
round-trip.
Fixes #708
Assisted-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com>
- Add parsed.Scheme guard to prevent malformed logout URL for scheme-less inputs like "//host/path" (url.Parse returns Host but empty Scheme) - Use url.URL struct to build logout URL instead of string concatenation - Add *testing.T + t.Helper() to redirectBaseMatchesRequest; log only scheme://host in error message to avoid leaking query parameters - Consolidate four separate Logout test functions into a single table-driven TestOpenShiftLogout per project conventions - Add scheme-less auth URL regression test case to TestOpenShiftLogout - Add doc comments to openshiftClientForRedirect, Logout, and GetLoginRedirectURL to improve docstring coverage - Add comment explaining intentional url.Parse error swallow fallback Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
jkilzi
force-pushed
the
fix/openshift-logout
branch
from
a152e3a to
9cdadf4
Compare
…logout
Replace the browser redirect to `{oauth-server}/logout` (which returned
HTTP 405 for unauthenticated GET requests) with a server-side token
revocation call:
DELETE /apis/oauth.openshift.io/v1/oauthaccesstokens/{name}
The token name is derived from the raw access token via SHA-256 +
base64url encoding for `sha256~`-prefixed tokens; legacy tokens are used
as-is (the value is the resource name). The DELETE is authenticated with
the user's own Bearer token, so no extra service-account permission is
needed. The proxy always returns an empty redirect URL, leaving the
frontend to navigate to `/login`.
The frontend is also updated to prevent the silent re-login loop that
occurred when `LoginPage` detected a single non-K8s provider and
immediately re-initiated the OAuth flow (even though the FlightCtl
session had just been cleared):
- `logout()` in `apiCalls.ts` sets a `sessionStorage` flag
(`flightctl.justLoggedOut`) before navigating to `/login`.
- `LoginPage` reads and clears the flag; when set, it skips the
single-provider auto-redirect and shows the provider selector
instead, so the user must click explicitly to log back in.
Unit tests for `openShiftTokenName` and the new `Logout` behaviour are
added in `openshift_test.go`; `helpers_test.go` gains a `TestMain` that
initialises the logger so tests can exercise the warning paths.
Closes #708
Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
jkilzi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/auth/openshift.go`:
- Around line 160-162: Normalize the base URL used for token revocation in the
OpenShift auth flow so query strings and fragments don’t corrupt the request
path. In the revocation logic around openShiftTokenName and the revokeURL
construction, parse o.apiServerURL, clear RawQuery and Fragment, and rebuild the
base from Scheme and Host before appending the
/apis/oauth.openshift.io/v1/oauthaccesstokens/... path. This ensures the DELETE
in the logout/revoke path always targets the correct OAuthAccessToken endpoint.
- Around line 164-176: The OpenShift token revocation request in the auth flow
can block indefinitely because the local http.Client has no timeout. Update the
revocation path around the client.Do call in the revocation helper to use a
bounded timeout, either by setting an http.Client Timeout or by creating the
request with a context deadline, so the logout flow fails fast if the API server
stalls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 02b2b183-5eee-47c8-a16a-17f5da9b713e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
apps/standalone/src/app/components/Login/LoginPage.tsxapps/standalone/src/app/utils/apiCalls.tsproxy/auth/helpers_test.goproxy/auth/openshift.goproxy/auth/openshift_test.goproxy/auth/redirect_test.go
ESLint sort-imports rule requires named imports within a declaration to be sorted alphabetically. JUST_LOGGED_OUT_KEY (uppercase) sorts before apiProxy. Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Line was over the print-width limit; Prettier wraps the ternary expression. Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Two issues found by CodeRabbit on the Logout() implementation: 1. URL normalization: o.apiServerURL can carry a path, query string, or fragment when derived from AuthorizationUrl (e.g. .../oauth/authorize?x=y). Appending /apis/oauth.openshift.io/... to such a value would corrupt the DELETE path. Introduce openShiftAPIServerBase() to extract only scheme+host before constructing the revocation URL. 2. Missing timeout: the revocation http.Client had no Timeout, so a stalled API server would block the proxy handler indefinitely. Set a 10s timeout so logout degrades fast rather than hanging. Tests added for openShiftAPIServerBase() and for the end-to-end behaviour when apiServerURL contains path/query components. Signed-off-by: Jonathan Kilzi <jkilzi@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
|
Thanks! I see for OCP (kubeadmin) it's behaving better now because it brings the user to the selection screen. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #708
Summary
OpenShiftAuthHandler.Logoutwas a no-op since PR EDM-2612: Logout method in OCP is POST. Remove cookie on logout #393 (EDM-2612), clearing only the FlightCtl session cookie but leaving the OpenShift OAuth session aliveLoginPageimmediately re-initiated the OAuth flow → users were silently re-authenticated after clicking Logout{oauth-server}/logout(the approach attempted in the previous revision of this PR) returned HTTP 405 Method Not Allowed for unauthenticated GET requestsFix — proxy (Go)
OpenShiftAuthHandler.Logoutnow revokes the access token server-side:The token resource name is derived from the raw token value:
sha256~-prefixed tokens are hashed with SHA-256 and base64url-encoded; legacy tokens are used as-is. The proxy always returns an empty redirect URL — no browser redirect needed.Fix — frontend (TypeScript)
To prevent the silent re-login loop after the session is cleared:
logout()inapiCalls.tssets asessionStorageflag (flightctl.justLoggedOut) before navigating to/loginLoginPagereads and clears the flag; when set, it skips the single-provider auto-redirect (suppressAutoSelect) and displays the provider selector so the user must click explicitly to log back inTest plan
TestOpenShiftTokenName_SHA256Prefix— correct SHA-256 + base64url derivation forsha256~tokensTestOpenShiftTokenName_LegacyToken— legacy token returned unchangedTestOpenShiftLogout_RevokesTokenViaDelete— DELETE is issued to the correct URL with Bearer auth; 200/204 treated as successTestOpenShift_LogoutRevocationLogged_OnUnexpectedStatus— unexpected status is logged as a warning, not an errorTestOpenShiftLogout_NoOpWhenTokenEmpty— no HTTP call when token is emptyTestOpenShiftLogout_NoOpWhenAPIServerURLEmpty— no HTTP call whenapiServerURLis unset/loginshowing the provider selector without auto-redirecting