OCPBUGS-59353: Fix authentication redirect loop on repeated 401 responses#15814
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-59353, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR refactors authentication error handling by renaming Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
component/core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/public/__tests__/co-fetch.spec.ts(2 hunks)frontend/public/co-fetch.ts(2 hunks)frontend/public/module/auth.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/module/auth.jsfrontend/public/__tests__/co-fetch.spec.tsfrontend/public/co-fetch.ts
🧬 Code graph analysis (1)
frontend/public/__tests__/co-fetch.spec.ts (1)
frontend/public/co-fetch.ts (1)
isK8sURL(39-57)
🪛 ast-grep (0.40.0)
frontend/public/co-fetch.ts
[warning] 39-39: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${window.SERVER_FLAGS.basePath}api/kubernetes/)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (4)
frontend/public/__tests__/co-fetch.spec.ts (1)
4-39: LGTM!The test updates correctly reflect the rename from
shouldLogouttoisK8sURL. Test coverage appropriately validates K8s URL detection including basePath handling and proxy exclusions.frontend/public/co-fetch.ts (2)
38-57: Rename and regex patterns look correct.The static analysis hint flags potential ReDoS, but
window.SERVER_FLAGS.basePathis server-controlled configuration rather than user input, so this is low risk in practice. The regex construction pattern is consistent with existing codebase usage.
65-81: LGTM!Good implementation: caching
isK8sURLresult avoids redundant regex evaluations, the counter reset is correctly scoped to K8s requests only, and 401 handling is properly delegated to the centralized auth service.frontend/public/module/auth.js (1)
58-89: LGTM!Good defensive coding with try/catch around sessionStorage operations. Using sessionStorage (per-tab, persists across redirects, clears on tab close) is the right choice for tracking redirect loops within a session.
| const redirectCount = incrementAuthRedirectCount(); | ||
|
|
||
| // If we've exceeded the max redirects, redirect to the error page | ||
| if (redirectCount > MAX_AUTH_REDIRECTS) { |
There was a problem hiding this comment.
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.
- if (redirectCount > MAX_AUTH_REDIRECTS) {
+ if (redirectCount >= MAX_AUTH_REDIRECTS) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (redirectCount > MAX_AUTH_REDIRECTS) { | |
| if (redirectCount >= MAX_AUTH_REDIRECTS) { |
🤖 Prompt for AI Agents
In frontend/public/module/auth.js at line 172, the redirect guard uses
`redirectCount > MAX_AUTH_REDIRECTS` which triggers the error page on the 4th
attempt when MAX_AUTH_REDIRECTS is 3; change the comparison to `redirectCount >=
MAX_AUTH_REDIRECTS` so the error page is shown after three failed authentication
attempts as documented. Ensure tests or comments reflect the inclusive limit and
update any related constants or docs if they assert "after three attempts."
| // Avoid redirecting if we're already on the error page | ||
| if (![window.location.href, window.location.pathname].includes(loginErrorURL)) { | ||
| window.location.href = errorURL.toString(); | ||
| } |
There was a problem hiding this comment.
Inconsistent error path checking pattern.
Line 116 uses isLoginErrorPath(window.location.pathname) which properly compares normalized paths. This check directly compares with loginErrorURL which could be a full URL or relative path, potentially causing mismatches. Use the existing helper for consistency.
// 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Avoid redirecting if we're already on the error page | |
| if (![window.location.href, window.location.pathname].includes(loginErrorURL)) { | |
| window.location.href = errorURL.toString(); | |
| } | |
| // Avoid redirecting if we're already on the error page | |
| if (!isLoginErrorPath(window.location.pathname)) { | |
| window.location.href = errorURL.toString(); | |
| } |
🤖 Prompt for AI Agents
In frontend/public/module/auth.js around lines 183 to 186, the current check
compares window.location against loginErrorURL directly which can be a full URL
or relative path and is inconsistent with the earlier
isLoginErrorPath(window.location.pathname) usage; replace the direct comparison
with a call to isLoginErrorPath using window.location.pathname (or normalize
loginErrorURL to a path and call the helper) so the same normalized path-check
helper is used consistently to decide whether to redirect to the error page.
|
/retest |
1 similar comment
|
/retest |
rhamilto
left a comment
rhamilto
left a comment
There was a problem hiding this comment.
/lgtm
But Claude has some relevant feedback:
Assessment
✅ Strengths
1. Proper use of sessionStorage - Persists across redirects but clears when tab closes, perfect for per-session tracking
2. Defensive error handling - All sessionStorage operations wrapped in try/catch with console logging
3. Smart reset logic - Counter resets on ANY successful K8s request, preventing false positives during normal auth flows
4. Prevents recursive errors - Checks if already on error page before redirecting
5. Descriptive error parameters - Error page receives context via query params (error=redirect_loop_detected&error_type=auth)
6. Function rename improves clarity - isK8sURL() better describes what the function actually does
7. Approved and labeled as critical - Has approval and appropriate severity labeling
⚠️ Observations
1. Missing test coverage for new functionality - The new handle401() and counter logic aren't tested. Consider adding tests for:
- Counter increments on consecutive 401s
- Redirect to error page after 3 failures
- Counter reset on successful requests
- sessionStorage error handling paths
2. Magic number threshold - The MAX_AUTH_REDIRECTS = 3 value appears arbitrary. Consider if this should be configurable or if 3 is well-reasoned (though 3 seems reasonable).
3. Error page existence - The code assumes /auth/error exists or that loginErrorURL is defined. No verification that the error page can handle these query parameters.
4. Counter never expires - The counter persists for the entire tab session. If a user has legitimate auth issues that take multiple attempts to resolve, they might hit the
limit. Consider adding a timestamp-based expiration (e.g., reset after 5 minutes).
5. No telemetry/monitoring - When redirect loop is detected, only a console.error is logged. Consider adding telemetry to track how often this occurs in production.
6. Duplicate condition check - Line 184 checks both window.location.href and window.location.pathname against loginErrorURL, but loginErrorURL is likely a full URL, so the
pathname check may never match.
🔍 Edge Cases to Consider
1. What if sessionStorage is disabled? - Code handles gracefully with try/catch, but counter effectively doesn't work. User could still get stuck in loop.
2. Multiple tabs - Each tab has independent sessionStorage, so a user could hit the loop in one tab but not others (this is probably desirable behavior).
3. Partial auth success - If some K8s endpoints succeed (401 counter resets) but others fail, the loop protection might not trigger when it should.
---
Recommendation
Approve with suggestions for follow-up ✅
The PR effectively solves a critical production issue with a reasonable implementation. The core logic is sound: track consecutive failures, break the loop after a threshold,
reset on success.
Required before merge:
- None - the code is functional as-is
Suggested follow-ups:
1. Add unit tests for the new handle401() method and counter logic
2. Verify the error page exists and handles the query parameters appropriately
3. Consider adding a time-based expiration to the counter
4. Add telemetry/monitoring when redirect loops are detected
5. Document the fallback behavior when sessionStorage is unavailable
The PR is ready to merge given its critical severity, but the test coverage gap should be addressed in a follow-up to prevent regressions.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-59353, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-59353, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
TheRealJon
force-pushed
the
OCPBUGS-59353
branch
from
a8f50e0 to
523dc4a
Compare
openshift-ci
Bot
added
component/sdk
Add detection and handling for redirect loops that can occur when the console repeatedly receives 401 responses from the Kubernetes API. Track consecutive 401s using sessionStorage and redirect to an error page after 3 failed authentication attempts to prevent infinite loops. The redirect counter is reset on any successful Kubernetes API request, ensuring normal authentication flows are not affected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
TheRealJon
force-pushed
the
OCPBUGS-59353
branch
from
523dc4a to
75d3e8c
Compare
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@rhamilto: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@TheRealJon: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@TheRealJon: Jira Issue Verification Checks: Jira Issue OCPBUGS-59353 Jira Issue OCPBUGS-59353 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick release-4.22 |
|
@jhadvig: new pull request created: #16437 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
10 participants
Add detection and handling for redirect loops that can occur when the console repeatedly receives 401 responses from the Kubernetes API. Track consecutive 401s using sessionStorage and redirect to an error page after 3 failed authentication attempts to prevent infinite loops.
The redirect counter is reset on any successful Kubernetes API request, ensuring normal authentication flows are not affected.
🤖 Generated with Claude Code