-
Notifications
You must be signed in to change notification settings - Fork 702
OCPBUGS-59353: Fix authentication redirect loop on repeated 401 responses #15814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,10 @@ const name = 'name'; | |||||||||||||||||
| const email = 'email'; | ||||||||||||||||||
| const clearLocalStorageKeys = [userID, name, email]; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Constants for redirect loop detection | ||||||||||||||||||
| const AUTH_REDIRECT_COUNT_KEY = 'auth-redirect-count'; | ||||||||||||||||||
| const MAX_AUTH_REDIRECTS = 3; | ||||||||||||||||||
|
|
||||||||||||||||||
| const setNext = (next: string) => { | ||||||||||||||||||
| if (!next) { | ||||||||||||||||||
| return; | ||||||||||||||||||
|
|
@@ -51,6 +55,39 @@ const clearLocalStorage = (keys: string[]) => { | |||||||||||||||||
| }); | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Helper functions for redirect counter | ||||||||||||||||||
| const getAuthRedirectCount = () => { | ||||||||||||||||||
| try { | ||||||||||||||||||
| const count = sessionStorage.getItem(AUTH_REDIRECT_COUNT_KEY); | ||||||||||||||||||
| return count ? parseInt(count, 10) : 0; | ||||||||||||||||||
| } catch (e) { | ||||||||||||||||||
| // eslint-disable-next-line no-console | ||||||||||||||||||
| console.error('Failed to get auth redirect count from sessionStorage', e); | ||||||||||||||||||
| return 0; | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| const incrementAuthRedirectCount = () => { | ||||||||||||||||||
| try { | ||||||||||||||||||
| const count = getAuthRedirectCount() + 1; | ||||||||||||||||||
| sessionStorage.setItem(AUTH_REDIRECT_COUNT_KEY, count.toString()); | ||||||||||||||||||
| return count; | ||||||||||||||||||
| } catch (e) { | ||||||||||||||||||
| // eslint-disable-next-line no-console | ||||||||||||||||||
| console.error('Failed to increment auth redirect count in sessionStorage', e); | ||||||||||||||||||
| return 0; | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| const resetAuthRedirectCount = () => { | ||||||||||||||||||
| try { | ||||||||||||||||||
| sessionStorage.removeItem(AUTH_REDIRECT_COUNT_KEY); | ||||||||||||||||||
| } catch (e) { | ||||||||||||||||||
| // eslint-disable-next-line no-console | ||||||||||||||||||
| console.error('Failed to reset auth redirect count in sessionStorage', e); | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| export const authSvc = { | ||||||||||||||||||
| userID: () => { | ||||||||||||||||||
| const id = loginStateItem(userID); | ||||||||||||||||||
|
|
@@ -126,4 +163,37 @@ export const authSvc = { | |||||||||||||||||
| window.location.assign(loginURL); | ||||||||||||||||||
| } | ||||||||||||||||||
| }, | ||||||||||||||||||
|
|
||||||||||||||||||
| // Handle 401 responses with redirect loop detection | ||||||||||||||||||
| handle401: (next) => { | ||||||||||||||||||
| const redirectCount = incrementAuthRedirectCount(); | ||||||||||||||||||
|
|
||||||||||||||||||
| // If we've exceeded the max redirects, redirect to the error page | ||||||||||||||||||
| if (redirectCount > MAX_AUTH_REDIRECTS) { | ||||||||||||||||||
| // eslint-disable-next-line no-console | ||||||||||||||||||
| console.error( | ||||||||||||||||||
| `Authentication redirect loop detected (${redirectCount} consecutive 401 responses). Redirecting to error page.`, | ||||||||||||||||||
| ); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Build error page URL with query parameters | ||||||||||||||||||
| const errorURL = new URL(loginErrorURL || '/auth/error', window.location.origin); | ||||||||||||||||||
| errorURL.searchParams.set('error', 'redirect_loop_detected'); | ||||||||||||||||||
| errorURL.searchParams.set('error_type', 'auth'); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Avoid redirecting if we're already on the error page | ||||||||||||||||||
| if (![window.location.href, window.location.pathname].includes(loginErrorURL)) { | ||||||||||||||||||
| window.location.href = errorURL.toString(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+183
to
+186
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent error path checking pattern. Line 116 uses // Avoid redirecting if we're already on the error page
- if (![window.location.href, window.location.pathname].includes(loginErrorURL)) {
+ if (!isLoginErrorPath(window.location.pathname)) {
window.location.href = errorURL.toString();
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| resetAuthRedirectCount(); | ||||||||||||||||||
| return; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Proceed with normal logout flow | ||||||||||||||||||
| authSvc.logout(next); | ||||||||||||||||||
| }, | ||||||||||||||||||
|
|
||||||||||||||||||
| // Reset redirect counter (called on successful k8s requests) | ||||||||||||||||||
| resetRedirectCount: () => { | ||||||||||||||||||
| resetAuthRedirectCount(); | ||||||||||||||||||
| }, | ||||||||||||||||||
| }; | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-by-one: error page triggers after 4 attempts, not 3.
PR description says "after three failed authentication attempts" but
redirectCount > MAX_AUTH_REDIRECTS(where MAX_AUTH_REDIRECTS=3) triggers on the 4th attempt. Use>=to match documented behavior.📝 Committable suggestion
🤖 Prompt for AI Agents