Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { RetryError } from '@console/dynamic-plugin-sdk/src/utils/error/http-error';
import {
shouldLogout,
unescapeGoUnicode,
isK8sUrl,
validateStatus,
} from '@console/shared/src/utils/console-fetch-utils';
import { coFetch } from '../console-fetch';
Expand Down Expand Up @@ -41,25 +41,25 @@ describe('consoleFetch', () => {
headers.set('content-type', 'application/json');

it('logs out users who get a 401 from k8s', () => {
expect(shouldLogout('/api/kubernetes/api/v1/pods')).toEqual(true);
expect(isK8sUrl('/api/kubernetes/api/v1/pods')).toEqual(true);
});

it('respects basePath and logs out users who get a 401 from k8s', () => {
const originalBasePath = window.SERVER_FLAGS.basePath;
window.SERVER_FLAGS.basePath = '/blah/';
expect(shouldLogout('/blah/api/kubernetes/api/v1/pods')).toEqual(true);
expect(isK8sUrl('/blah/api/kubernetes/api/v1/pods')).toEqual(true);
window.SERVER_FLAGS.basePath = originalBasePath;
});

it('does not log out users who get a 401 from chargeback', () => {
expect(
shouldLogout('/api/kubernetes/api/v1/namespaces/prd354/services/chargeback/proxy/api'),
isK8sUrl('/api/kubernetes/api/v1/namespaces/prd354/services/chargeback/proxy/api'),
).toEqual(false);
});

it('does not log out users who get a 401 from graphs', () => {
expect(
shouldLogout(
isK8sUrl(
'/api/kubernetes/api/v1/proxy/namespaces/tectonic-system/services/prometheus:9090/api/v1/query?query=100%20-%20(sum(rate(node_cpu%7Bjob%3D%22node-exporter%22%2Cmode%3D%22idle%22%7D%5B2m%5D))%20%2F%20count(node_cpu%7Bjob%3D%22node-exporter%22%2C%20mode%3D%22idle%22%7D))%20*%20100',
),
).toEqual(false);
Expand Down
23 changes: 19 additions & 4 deletions frontend/packages/console-shared/src/utils/console-fetch-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ export const applyConsoleHeaders = (url: string, options: RequestInit): RequestI
return options;
};

// TODO: url can be url or path, but shouldLogout only handles paths
export const shouldLogout = (url: string): boolean => {
// TODO: url can be url or path, but isK8sUrl only handles paths
export const isK8sUrl = (url: string): boolean => {
const k8sRegex = new RegExp(`^${window.SERVER_FLAGS.basePath}api/kubernetes/`);
// 401 from k8s. show logout screen
if (k8sRegex.test(url)) {
Expand Down Expand Up @@ -151,22 +151,37 @@ export const validateStatus = async (
method: string,
retry: boolean,
) => {
const isK8sRequest = isK8sUrl(url);
if (response.ok || response.status === 304) {
// Reset redirect counter on successful k8s request
if (isK8sRequest) {
// We can't use regular import from outside this package, so a dynamic import is required
// This also breaks a nasty cycle - authSvc.logout calls coFetch (which calls validateStatus)
import('@console/internal/module/auth')
.then((m) => m.authSvc)
.then((authSvc) => {
authSvc.resetRedirectCount();
})
.catch((e) => {
// eslint-disable-next-line no-console
console.error('Error resetting redirect counter', e);
});
}
return response;
}

if (retry && response.status === 429) {
throw new RetryError();
}

if (response.status === 401 && shouldLogout(url)) {
if (response.status === 401 && isK8sRequest) {
const next = window.location.pathname + window.location.search + window.location.hash;

// This also breaks a nasty cycle - authSvc.logout calls coFetch (which calls validateStatus)
import('@console/internal/module/auth')
.then((m) => m.authSvc)
.then((authSvc) => {
authSvc.logout(next);
authSvc.handle401(next);
})
.catch((e) => {
// eslint-disable-next-line no-console
Expand Down
70 changes: 70 additions & 0 deletions frontend/public/module/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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."

// 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
// 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.

resetAuthRedirectCount();
return;
}

// Proceed with normal logout flow
authSvc.logout(next);
},

// Reset redirect counter (called on successful k8s requests)
resetRedirectCount: () => {
resetAuthRedirectCount();
},
};