From 0eab151240683d75c7e2fe242e676e7a28edc4ed Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 10 Mar 2026 19:10:06 -0300 Subject: [PATCH 1/6] fix(refresh-token): add retry mechanism with back off to emit new AT --- .../__test__/extra-questions.test.js | 3 + .../inputs/upload-input-v2/dropzone.js | 7 +- .../security/__tests__/methods.test.js | 221 ++++++++++++++++++ src/components/security/constants.js | 1 + src/components/security/methods.js | 67 ++++-- 5 files changed, 277 insertions(+), 22 deletions(-) create mode 100644 src/components/security/__tests__/methods.test.js diff --git a/src/components/extra-questions/__test__/extra-questions.test.js b/src/components/extra-questions/__test__/extra-questions.test.js index d5724e10..6e7e4f54 100644 --- a/src/components/extra-questions/__test__/extra-questions.test.js +++ b/src/components/extra-questions/__test__/extra-questions.test.js @@ -11,6 +11,9 @@ import {toSlug} from '../../../utils/methods'; Enzyme.configure({adapter: new Adapter()}); +// jsdom does not implement scrollIntoView +Element.prototype.scrollIntoView = jest.fn(); + const questions = [ { "id": 93, diff --git a/src/components/inputs/upload-input-v2/dropzone.js b/src/components/inputs/upload-input-v2/dropzone.js index 34920423..d4522ed0 100644 --- a/src/components/inputs/upload-input-v2/dropzone.js +++ b/src/components/inputs/upload-input-v2/dropzone.js @@ -4,6 +4,7 @@ import 'dropzone/dist/dropzone.css'; import {Icon} from './icon' import PropTypes from 'prop-types'; import {getAccessToken, initLogOut} from '../../security/methods'; +import {AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR} from '../../security/constants'; import {getMD5} from "../../../utils/crypto"; let Dropzone = null; @@ -61,7 +62,11 @@ export class DropzoneJS extends React.Component { } catch (e) { console.log(e); this.onError(e); - initLogOut(); + // only logout on genuine auth errors, not transient network failures + if (!e.message || !e.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR)) { + initLogOut(); + } + return; } if (options.maxFiles && options.maxFiles < (this.state.files.length + this.props.uploadCount)) { done('Max files reached.'); diff --git a/src/components/security/__tests__/methods.test.js b/src/components/security/__tests__/methods.test.js new file mode 100644 index 00000000..c759c852 --- /dev/null +++ b/src/components/security/__tests__/methods.test.js @@ -0,0 +1,221 @@ +import { + AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR, + AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR, +} from '../constants'; + +import { refreshAccessToken } from '../methods'; + +// Mock utils/methods imports used by security/methods +jest.mock('../../../utils/methods', () => ({ + base64URLEncode: jest.fn(), + getAuthCallback: jest.fn(), + getCurrentLocation: jest.fn(() => ({ replace: jest.fn() })), + getCurrentPathName: jest.fn(() => '/'), + getFromLocalStorage: jest.fn(), + removeFromLocalStorage: jest.fn(), + getOrigin: jest.fn(() => 'http://localhost'), + putOnLocalStorage: jest.fn(), + retryPromise: jest.fn(), + setSessionClearingState: jest.fn(), +})); + +jest.mock('../../../utils/crypto', () => ({ + getRandomBytes: jest.fn(), + getSHA256: jest.fn(), +})); + +jest.mock('moment-timezone', () => { + const m = jest.fn(() => ({ unix: () => 1000 })); + m.unix = jest.fn(() => 1000); + return { __esModule: true, default: m }; +}); + +jest.mock('browser-tabs-lock', () => { + return jest.fn().mockImplementation(() => ({ + acquireLock: jest.fn(), + releaseLock: jest.fn(), + })); +}); + +jest.mock('js-cookie', () => ({ + set: jest.fn(), + remove: jest.fn(), + get: jest.fn(), +})); + +jest.mock('idtoken-verifier', () => { + return jest.fn().mockImplementation(() => ({ + decode: jest.fn(), + })); +}); + +jest.mock('../actions', () => ({ + SET_LOGGED_USER: 'SET_LOGGED_USER', +})); + +const { setSessionClearingState } = require('../../../utils/methods'); + +// Helper to set window globals needed by methods.js +const setupWindowGlobals = () => { + global.window = global.window || {}; + global.window.IDP_BASE_URL = 'https://idp.example.com'; + global.window.OAUTH2_CLIENT_ID = 'test-client-id'; + global.window.OAUTH2_FLOW = 'code'; + global.window.SCOPES = 'openid offline_access'; + global.window.OAUTH2_USE_REFRESH_TOKEN = true; +}; + +beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers(); + setupWindowGlobals(); +}); + +afterEach(() => { + jest.useRealTimers(); +}); + +describe('refreshAccessToken', () => { + + it('should return tokens on successful response', async () => { + const mockResponse = { + status: 200, + json: jest.fn().mockResolvedValue({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + id_token: 'new-id-token', + }), + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + const result = await refreshAccessToken('old-refresh-token'); + + expect(result).toEqual({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + id_token: 'new-id-token', + }); + expect(global.fetch).toHaveBeenCalledTimes(1); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR on HTTP 400', async () => { + const mockResponse = { + status: 400, + statusText: 'Bad Request', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('revoked-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR); + + expect(setSessionClearingState).toHaveBeenCalledWith(true); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 500', async () => { + const mockResponse = { + status: 500, + statusText: 'Internal Server Error', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 502', async () => { + const mockResponse = { + status: 502, + statusText: 'Bad Gateway', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 503', async () => { + const mockResponse = { + status: 503, + statusText: 'Service Unavailable', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on network failure', async () => { + global.fetch = jest.fn().mockRejectedValue(new TypeError('Failed to fetch')); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on DNS error', async () => { + global.fetch = jest.fn().mockRejectedValue(new TypeError('getaddrinfo ENOTFOUND idp.example.com')); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + }); + + it('should not call setSessionClearingState on network error', async () => { + global.fetch = jest.fn().mockRejectedValue(new TypeError('Network request failed')); + + try { + await refreshAccessToken('valid-token'); + } catch (e) { + // expected + } + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); + + it('should call setSessionClearingState(true) only on HTTP 400', async () => { + const mockResponse = { + status: 400, + statusText: 'Bad Request', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + try { + await refreshAccessToken('revoked-token'); + } catch (e) { + // expected + } + + expect(setSessionClearingState).toHaveBeenCalledWith(true); + expect(setSessionClearingState).toHaveBeenCalledTimes(1); + }); + + it('should include status in error message for 5xx', async () => { + const mockResponse = { + status: 503, + statusText: 'Service Unavailable', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: 503 - Service Unavailable`); + }); + + it('should include original message in error for network failure', async () => { + global.fetch = jest.fn().mockRejectedValue(new TypeError('Failed to fetch')); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: Failed to fetch`); + }); +}); diff --git a/src/components/security/constants.js b/src/components/security/constants.js index 7abd447c..2900fa7a 100644 --- a/src/components/security/constants.js +++ b/src/components/security/constants.js @@ -3,6 +3,7 @@ export const AUTH_ERROR_MISSING_REFRESH_TOKEN = 'AUTH_ERROR_MISSING_REFRESH_TOKE export const AUTH_ERROR_ACCESS_TOKEN_EXPIRED = 'AUTH_ERROR_ACCESS_TOKEN_EXPIRED'; export const AUTH_ERROR_LOCK_ACQUIRE_ERROR = 'AUTH_ERROR_LOCK_ACQUIRE_ERROR' export const AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR = 'AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR'; +export const AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR = 'AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR'; export const AUTH_ERROR_ID_TOKEN_INVALID = 'AUTH_ERROR_ID_TOKEN_INVALID'; export const AUTH_ERROR_MISSING_OTP_PARAM = 'AUTH_ERROR_MISSING_OTP_PARAM'; export const AUTH_ERROR_MISSING_PKCE_PARAM = 'AUTH_ERROR_MISSING_PKCE_PARAM'; diff --git a/src/components/security/methods.js b/src/components/security/methods.js index bce0568f..044aa8cd 100644 --- a/src/components/security/methods.js +++ b/src/components/security/methods.js @@ -45,6 +45,7 @@ import { AUTH_ERROR_MISSING_REFRESH_TOKEN, AUTH_ERROR_LOCK_ACQUIRE_ERROR, AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR, + AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR, AUTH_ERROR_ID_TOKEN_INVALID, AUTH_ERROR_MISSING_OTP_PARAM, AUTH_ERROR_MISSING_PKCE_PARAM, @@ -262,23 +263,43 @@ export const emitAccessToken = async (code, backUrl = null) => { } }; +export const MAX_RETRIES = 5; +export const BACKOFF_BASE_MS = 1000; + +const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => { + for (let attempt = 0; attempt <= maxRetries; attempt++) { + try { + return await fn(); + } catch (err) { + // auth errors (HTTP 400) are fatal — never retry + if (err.message && err.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR)) { + throw err; + } + // network/5xx errors are retryable — retry unless exhausted + if (attempt === maxRetries) { + throw err; + } + const delay = baseDelayMs * Math.pow(2, attempt); + console.log(`retryWithBackoff retry ${attempt + 1}/${maxRetries} in ${delay}ms`); + await new Promise(resolve => setTimeout(resolve, delay)); + } + } +}; + const processRefreshToken = async (flow, refreshToken) => { if (flow === RESPONSE_TYPE_CODE && useOAuth2RefreshToken()) { - //console.log('getAccessToken getting new access token, access token got void'); if (!refreshToken) { clearAuthInfo(); throw Error(AUTH_ERROR_MISSING_REFRESH_TOKEN); } - let response = await refreshAccessToken(refreshToken); + let response = await retryWithBackoff(() => refreshAccessToken(refreshToken)); let {access_token, expires_in, refresh_token, id_token} = response; - //console.log(`getAccessToken access_token ${access_token} expires_in ${expires_in} refresh_token ${refresh_token}`); if (typeof refresh_token === 'undefined') { refresh_token = null; // not using rotate policy } storeAuthInfo(access_token, expires_in, refresh_token, id_token); - //console.log(`getAccessToken access_token ${access_token} [NEW]`); return access_token; } clearAuthInfo(); @@ -400,33 +421,37 @@ export const refreshAccessToken = async (refresh_token) => { "refresh_token": refresh_token }; + let response; try { - const response = await fetch(`${baseUrl}/oauth2/token`, { + response = await fetch(`${baseUrl}/oauth2/token`, { method: 'POST', headers: { 'Accept': 'application/json', 'Content-Type': 'application/json' }, body: JSON.stringify(payload) - }).then((response) => { - if (response.status === 400) { - let currentLocation = getCurrentPathName(); - setSessionClearingState(true); - throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: ${response.status} - ${response.statusText}`); - } - return response; - - }).catch(function (error) { - throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: ${error.message}`); }); + } catch (networkError) { + // fetch rejects on network failures (DNS, timeout, no connectivity) + console.log('refreshAccessToken network error:', networkError.message); + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${networkError.message}`); + } - const json = await response.json(); - let {access_token, refresh_token, expires_in, id_token} = json; - return {access_token, refresh_token, expires_in, id_token} - } catch (err) { - console.log(err); - throw err; + if (response.status === 400) { + // token is genuinely revoked — this is a real auth error + setSessionClearingState(true); + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: ${response.status} - ${response.statusText}`); + } + + if (response.status >= 500) { + // server error — transient, should be retried + console.log(`refreshAccessToken server error: ${response.status} - ${response.statusText}`); + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${response.status} - ${response.statusText}`); } + + const json = await response.json(); + let {access_token, refresh_token: new_refresh_token, expires_in, id_token} = json; + return {access_token, refresh_token: new_refresh_token, expires_in, id_token} } export const storeAuthInfo = (accessToken, expiresIn, refreshToken = null, idToken = null) => { From d079fbf2b8495a67eec7853120e597e77340f179 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 11 Mar 2026 01:28:28 -0300 Subject: [PATCH 2/6] chore(ci): add jest run --- .github/workflows/jest.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .github/workflows/jest.yml diff --git a/.github/workflows/jest.yml b/.github/workflows/jest.yml new file mode 100644 index 00000000..607d6e08 --- /dev/null +++ b/.github/workflows/jest.yml @@ -0,0 +1,15 @@ +name: jest + +on: [push, pull_request] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 0 + - uses: actions/setup-node@v3 + with: + node-version: 18 + - run: yarn install + - run: yarn test From 2c4c42fc6f2c68ba789182bca45682c41b51b843 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 11 Mar 2026 01:29:31 -0300 Subject: [PATCH 3/6] chore(ci): add build test --- .github/workflows/build_test.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .github/workflows/build_test.yml diff --git a/.github/workflows/build_test.yml b/.github/workflows/build_test.yml new file mode 100644 index 00000000..ba5d3dbc --- /dev/null +++ b/.github/workflows/build_test.yml @@ -0,0 +1,15 @@ +name: build_test + +on: [push, pull_request] +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 0 + - uses: actions/setup-node@v3 + with: + node-version: 18 + - run: yarn install + - run: yarn build From 23fa7c27322a4e24a26b00a58089410e893ea428 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 11 Mar 2026 07:36:25 -0300 Subject: [PATCH 4/6] fix(refresh-token): fix retry exhaustion bug, dropzone accept callback, and response.ok handling * Fix retryWithBackoff exhaustion guard (attempt === maxRetries was dead code after loop bound change to < maxRetries; errors were silently swallowed, function returned undefined instead of throwing). * Fix Dropzone accept callback to call done() on error path, preventing files from getting stuck in an indeterminate state. * Use response.ok instead of checking individual status codes, closing a gap where 4xx codes other than 400 fell through unhandled. * Add ok property to existing test mocks to match the new response.ok check. * Export retryWithBackoff and add 5 unit tests covering retry count, auth error short-circuit, transient recovery, and exponential backoff. * Add defensive check for missing access_token in refresh response. --- .../inputs/upload-input-v2/dropzone.js | 1 + .../security/__tests__/methods.test.js | 85 ++++++++++++++++++- src/components/security/methods.js | 49 +++++------ 3 files changed, 110 insertions(+), 25 deletions(-) diff --git a/src/components/inputs/upload-input-v2/dropzone.js b/src/components/inputs/upload-input-v2/dropzone.js index d4522ed0..f8377ac7 100644 --- a/src/components/inputs/upload-input-v2/dropzone.js +++ b/src/components/inputs/upload-input-v2/dropzone.js @@ -66,6 +66,7 @@ export class DropzoneJS extends React.Component { if (!e.message || !e.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR)) { initLogOut(); } + done(e.message || 'Auth error'); return; } if (options.maxFiles && options.maxFiles < (this.state.files.length + this.props.uploadCount)) { diff --git a/src/components/security/__tests__/methods.test.js b/src/components/security/__tests__/methods.test.js index c759c852..86b9ad77 100644 --- a/src/components/security/__tests__/methods.test.js +++ b/src/components/security/__tests__/methods.test.js @@ -3,7 +3,7 @@ import { AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR, } from '../constants'; -import { refreshAccessToken } from '../methods'; +import { refreshAccessToken, retryWithBackoff } from '../methods'; // Mock utils/methods imports used by security/methods jest.mock('../../../utils/methods', () => ({ @@ -79,6 +79,7 @@ describe('refreshAccessToken', () => { it('should return tokens on successful response', async () => { const mockResponse = { + ok: true, status: 200, json: jest.fn().mockResolvedValue({ access_token: 'new-access-token', @@ -102,6 +103,7 @@ describe('refreshAccessToken', () => { it('should throw AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR on HTTP 400', async () => { const mockResponse = { + ok: false, status: 400, statusText: 'Bad Request', }; @@ -116,6 +118,7 @@ describe('refreshAccessToken', () => { it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 500', async () => { const mockResponse = { + ok: false, status: 500, statusText: 'Internal Server Error', }; @@ -130,6 +133,7 @@ describe('refreshAccessToken', () => { it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 502', async () => { const mockResponse = { + ok: false, status: 502, statusText: 'Bad Gateway', }; @@ -142,6 +146,7 @@ describe('refreshAccessToken', () => { it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 503', async () => { const mockResponse = { + ok: false, status: 503, statusText: 'Service Unavailable', }; @@ -184,6 +189,7 @@ describe('refreshAccessToken', () => { it('should call setSessionClearingState(true) only on HTTP 400', async () => { const mockResponse = { + ok: false, status: 400, statusText: 'Bad Request', }; @@ -201,6 +207,7 @@ describe('refreshAccessToken', () => { it('should include status in error message for 5xx', async () => { const mockResponse = { + ok: false, status: 503, statusText: 'Service Unavailable', }; @@ -219,3 +226,79 @@ describe('refreshAccessToken', () => { .toThrow(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: Failed to fetch`); }); }); + +describe('retryWithBackoff', () => { + + it('should return on first success without retrying', async () => { + jest.useRealTimers(); + const fn = jest.fn().mockResolvedValue('ok'); + + const result = await retryWithBackoff(fn, 3, 1); + + expect(result).toBe('ok'); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('should retry network errors up to maxRetries then throw', async () => { + jest.useRealTimers(); + const networkError = new Error('network down'); + const fn = jest.fn().mockRejectedValue(networkError); + + await expect(retryWithBackoff(fn, 3, 1)) + .rejects + .toThrow('network down'); + + expect(fn).toHaveBeenCalledTimes(3); + }); + + it('should succeed after transient failures', async () => { + jest.useRealTimers(); + const fn = jest.fn() + .mockRejectedValueOnce(new Error('transient')) + .mockRejectedValueOnce(new Error('transient')) + .mockResolvedValue('recovered'); + + const result = await retryWithBackoff(fn, 5, 1); + + expect(result).toBe('recovered'); + expect(fn).toHaveBeenCalledTimes(3); + }); + + it('should not retry auth errors (HTTP 400)', async () => { + jest.useRealTimers(); + const authError = new Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: 400 - Bad Request`); + const fn = jest.fn().mockRejectedValue(authError); + + await expect(retryWithBackoff(fn, 5, 1)) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR); + + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('should apply exponential backoff delays', async () => { + jest.useRealTimers(); + const setTimeoutSpy = jest.spyOn(global, 'setTimeout'); + + const fn = jest.fn() + .mockRejectedValueOnce(new Error('fail 1')) + .mockRejectedValueOnce(new Error('fail 2')) + .mockResolvedValue('ok'); + + const baseDelay = 100; + const result = await retryWithBackoff(fn, 5, baseDelay); + + expect(result).toBe('ok'); + expect(fn).toHaveBeenCalledTimes(3); + + // Extract the delay arguments from setTimeout calls made by retryWithBackoff + const retryDelays = setTimeoutSpy.mock.calls + .map(call => call[1]) + .filter(delay => delay >= baseDelay); + + // 100 * 2^0 = 100ms, 100 * 2^1 = 200ms + expect(retryDelays).toEqual([100, 200]); + + setTimeoutSpy.mockRestore(); + }); +}); diff --git a/src/components/security/methods.js b/src/components/security/methods.js index 044aa8cd..028fd2d3 100644 --- a/src/components/security/methods.js +++ b/src/components/security/methods.js @@ -14,6 +14,7 @@ import moment from "moment-timezone"; import request from 'superagent/lib/client'; import SuperTokensLock from 'browser-tabs-lock'; import Cookies from 'js-cookie' + let http = request; /** @@ -143,7 +144,7 @@ export const getAuthUrl = ( * @param idToken * @returns {*} */ -export const getLogoutUrl = (idToken= null) => { +export const getLogoutUrl = (idToken = null) => { let baseUrl = getOAuth2IDPBaseUrl(); let oauth2ClientId = getOAuth2ClientId(); let url = URI(`${baseUrl}/oauth2/end-session`); @@ -162,7 +163,7 @@ export const getLogoutUrl = (idToken= null) => { "state": state, } - if(idToken) + if (idToken) queryParams.id_token_hint = idToken; return url.query(queryParams); @@ -228,7 +229,7 @@ export const emitAccessToken = async (code, backUrl = null) => { let redirectUri = getAuthCallback(); let pkce = JSON.parse(getFromLocalStorage(PKCE, true)); - if(!pkce) + if (!pkce) throw Error(AUTH_ERROR_MISSING_PKCE_PARAM); if (backUrl != null) @@ -266,7 +267,7 @@ export const emitAccessToken = async (code, backUrl = null) => { export const MAX_RETRIES = 5; export const BACKOFF_BASE_MS = 1000; -const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => { +export const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => { for (let attempt = 0; attempt <= maxRetries; attempt++) { try { return await fn(); @@ -276,7 +277,7 @@ const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACK throw err; } // network/5xx errors are retryable — retry unless exhausted - if (attempt === maxRetries) { + if (attempt === maxRetries - 1) { throw err; } const delay = baseDelayMs * Math.pow(2, attempt); @@ -338,13 +339,12 @@ const _getAccessToken = async () => { * @returns {Promise<*|undefined>} */ export const getAccessToken = async () => { - if(navigator?.locks){ + if (navigator?.locks) { return await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => { console.log(`openstack-uicore-foundation::Security::methods::getAccessToken web lock api`, lock); return await _getAccessToken(); }); - } - else { + } else { if ( await retryPromise( () => Lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, GET_TOKEN_SILENTLY_LOCK_KEY_TIMEOUT), @@ -383,13 +383,12 @@ const _clearAccessToken = () => { export const clearAccessToken = async () => { // see https://developer.mozilla.org/en-US/docs/Web/API/Web_Locks_API - if(navigator?.locks){ + if (navigator?.locks) { await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => { console.log(`openstack-uicore-foundation::Security::methods::clearAccessToken web lock api`, lock); _clearAccessToken(); }); - } - else { + } else { if ( await retryPromise( () => Lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, GET_TOKEN_SILENTLY_LOCK_KEY_TIMEOUT), @@ -401,8 +400,7 @@ export const clearAccessToken = async () => { } finally { await Lock.releaseLock(GET_TOKEN_SILENTLY_LOCK_KEY); } - } - else{ + } else { // error on locking throw Error(AUTH_ERROR_LOCK_ACQUIRE_ERROR); } @@ -437,20 +435,24 @@ export const refreshAccessToken = async (refresh_token) => { throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${networkError.message}`); } - if (response.status === 400) { + if (!response.ok) { + console.log(`refreshAccessToken server error: ${response.status} - ${response.statusText}`); + if (response.status >= 500) { + // server error — transient, should be retried + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${response.status} - ${response.statusText}`); + } // token is genuinely revoked — this is a real auth error setSessionClearingState(true); throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: ${response.status} - ${response.statusText}`); } - if (response.status >= 500) { - // server error — transient, should be retried - console.log(`refreshAccessToken server error: ${response.status} - ${response.statusText}`); - throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${response.status} - ${response.statusText}`); - } - const json = await response.json(); let {access_token, refresh_token: new_refresh_token, expires_in, id_token} = json; + // Defensively ensure we never propagate an undefined access token. + if (!access_token) { + setSessionClearingState(true); + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: missing access_token in refresh response`); + } return {access_token, refresh_token: new_refresh_token, expires_in, id_token} } @@ -478,9 +480,8 @@ export const storeAuthInfo = (accessToken, expiresIn, refreshToken = null, idTok if (idToken) { authInfo[ID_TOKEN] = idToken; - Cookies.set(ID_TOKEN, idToken, { secure: true, sameSite: 'Lax' }); - } - else{ + Cookies.set(ID_TOKEN, idToken, {secure: true, sameSite: 'Lax'}); + } else { Cookies.remove(ID_TOKEN); } @@ -563,7 +564,7 @@ export const validateIdToken = (idToken, issuer, audience) => { }); let storedNonce = getFromLocalStorage(NONCE, true); - if(!storedNonce) + if (!storedNonce) throw Error(AUTH_ERROR_MISSING_NONCE_PARAM); let jwt = verifier.decode(idToken); From 31056df6617ac62b92f207c99a023a2e849840b4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 11 Mar 2026 07:45:11 -0300 Subject: [PATCH 5/6] fix(refresh-token): The loop bound is now attempt < maxRetries * consistent with the exhaustion guard attempt === maxRetries - 1. With MAX_RETRIES = 5, the function makes exactly 5 attempts (0 through 4), no dead code. --- src/components/security/methods.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/security/methods.js b/src/components/security/methods.js index 028fd2d3..dc1278df 100644 --- a/src/components/security/methods.js +++ b/src/components/security/methods.js @@ -268,7 +268,7 @@ export const MAX_RETRIES = 5; export const BACKOFF_BASE_MS = 1000; export const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => { - for (let attempt = 0; attempt <= maxRetries; attempt++) { + for (let attempt = 0; attempt < maxRetries; attempt++) { try { return await fn(); } catch (err) { From dbdb8e0c5a349dbab27ad10b59830d1a9bf85a2d Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 11 Mar 2026 07:50:37 -0300 Subject: [PATCH 6/6] fix(refresh-token): improved HTTP error handling --- .../security/__tests__/methods.test.js | 78 +++++++++++++++++-- src/components/security/methods.js | 36 +++++---- 2 files changed, 96 insertions(+), 18 deletions(-) diff --git a/src/components/security/__tests__/methods.test.js b/src/components/security/__tests__/methods.test.js index 86b9ad77..f1e6089a 100644 --- a/src/components/security/__tests__/methods.test.js +++ b/src/components/security/__tests__/methods.test.js @@ -157,6 +157,36 @@ describe('refreshAccessToken', () => { .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); }); + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 408 (Request Timeout)', async () => { + const mockResponse = { + ok: false, + status: 408, + statusText: 'Request Timeout', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); + + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 429 (Too Many Requests)', async () => { + const mockResponse = { + ok: false, + status: 429, + statusText: 'Too Many Requests', + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); + it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on network failure', async () => { global.fetch = jest.fn().mockRejectedValue(new TypeError('Failed to fetch')); @@ -225,6 +255,32 @@ describe('refreshAccessToken', () => { .rejects .toThrow(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: Failed to fetch`); }); + + it('should throw retryable error when response body is not valid JSON', async () => { + const mockResponse = { + ok: true, + status: 200, + json: jest.fn().mockRejectedValue(new SyntaxError('Unexpected token < in JSON')), + }; + global.fetch = jest.fn().mockResolvedValue(mockResponse); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: invalid JSON response from IDP`); + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); + + it('should throw retryable error when fetch is aborted by timeout', async () => { + const abortError = new DOMException('The operation was aborted.', 'AbortError'); + global.fetch = jest.fn().mockRejectedValue(abortError); + + await expect(refreshAccessToken('valid-token')) + .rejects + .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + + expect(setSessionClearingState).not.toHaveBeenCalled(); + }); }); describe('retryWithBackoff', () => { @@ -241,7 +297,7 @@ describe('retryWithBackoff', () => { it('should retry network errors up to maxRetries then throw', async () => { jest.useRealTimers(); - const networkError = new Error('network down'); + const networkError = new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: network down`); const fn = jest.fn().mockRejectedValue(networkError); await expect(retryWithBackoff(fn, 3, 1)) @@ -254,8 +310,8 @@ describe('retryWithBackoff', () => { it('should succeed after transient failures', async () => { jest.useRealTimers(); const fn = jest.fn() - .mockRejectedValueOnce(new Error('transient')) - .mockRejectedValueOnce(new Error('transient')) + .mockRejectedValueOnce(new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: transient`)) + .mockRejectedValueOnce(new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: transient`)) .mockResolvedValue('recovered'); const result = await retryWithBackoff(fn, 5, 1); @@ -276,13 +332,25 @@ describe('retryWithBackoff', () => { expect(fn).toHaveBeenCalledTimes(1); }); + it('should not retry unexpected errors (fail fast)', async () => { + jest.useRealTimers(); + const unexpectedError = new TypeError('Cannot read properties of undefined'); + const fn = jest.fn().mockRejectedValue(unexpectedError); + + await expect(retryWithBackoff(fn, 5, 1)) + .rejects + .toThrow('Cannot read properties of undefined'); + + expect(fn).toHaveBeenCalledTimes(1); + }); + it('should apply exponential backoff delays', async () => { jest.useRealTimers(); const setTimeoutSpy = jest.spyOn(global, 'setTimeout'); const fn = jest.fn() - .mockRejectedValueOnce(new Error('fail 1')) - .mockRejectedValueOnce(new Error('fail 2')) + .mockRejectedValueOnce(new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: fail 1`)) + .mockRejectedValueOnce(new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: fail 2`)) .mockResolvedValue('ok'); const baseDelay = 100; diff --git a/src/components/security/methods.js b/src/components/security/methods.js index dc1278df..705ce514 100644 --- a/src/components/security/methods.js +++ b/src/components/security/methods.js @@ -266,18 +266,16 @@ export const emitAccessToken = async (code, backUrl = null) => { export const MAX_RETRIES = 5; export const BACKOFF_BASE_MS = 1000; +export const REFRESH_TOKEN_FETCH_TIMEOUT_MS = 10000; export const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => { for (let attempt = 0; attempt < maxRetries; attempt++) { try { return await fn(); } catch (err) { - // auth errors (HTTP 400) are fatal — never retry - if (err.message && err.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR)) { - throw err; - } - // network/5xx errors are retryable — retry unless exhausted - if (attempt === maxRetries - 1) { + // only retry transient network/server errors — everything else fails fast + const isRetryable = err.message && err.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + if (!isRetryable || attempt === maxRetries - 1) { throw err; } const delay = baseDelayMs * Math.pow(2, attempt); @@ -339,7 +337,7 @@ const _getAccessToken = async () => { * @returns {Promise<*|undefined>} */ export const getAccessToken = async () => { - if (navigator?.locks) { + if (typeof navigator !== 'undefined' && navigator.locks) { return await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => { console.log(`openstack-uicore-foundation::Security::methods::getAccessToken web lock api`, lock); return await _getAccessToken(); @@ -383,7 +381,7 @@ const _clearAccessToken = () => { export const clearAccessToken = async () => { // see https://developer.mozilla.org/en-US/docs/Web/API/Web_Locks_API - if (navigator?.locks) { + if (typeof navigator !== 'undefined' && navigator.locks) { await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => { console.log(`openstack-uicore-foundation::Security::methods::clearAccessToken web lock api`, lock); _clearAccessToken(); @@ -419,6 +417,9 @@ export const refreshAccessToken = async (refresh_token) => { "refresh_token": refresh_token }; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), REFRESH_TOKEN_FETCH_TIMEOUT_MS); + let response; try { response = await fetch(`${baseUrl}/oauth2/token`, { @@ -427,18 +428,21 @@ export const refreshAccessToken = async (refresh_token) => { 'Accept': 'application/json', 'Content-Type': 'application/json' }, - body: JSON.stringify(payload) + body: JSON.stringify(payload), + signal: controller.signal }); } catch (networkError) { - // fetch rejects on network failures (DNS, timeout, no connectivity) + // fetch rejects on network failures (DNS, timeout, no connectivity, abort) console.log('refreshAccessToken network error:', networkError.message); throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${networkError.message}`); + } finally { + clearTimeout(timeoutId); } if (!response.ok) { console.log(`refreshAccessToken server error: ${response.status} - ${response.statusText}`); - if (response.status >= 500) { - // server error — transient, should be retried + if (response.status >= 500 || response.status === 408 || response.status === 429) { + // transient error (server error, request timeout, rate limit) — should be retried throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${response.status} - ${response.statusText}`); } // token is genuinely revoked — this is a real auth error @@ -446,7 +450,13 @@ export const refreshAccessToken = async (refresh_token) => { throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: ${response.status} - ${response.statusText}`); } - const json = await response.json(); + let json; + try { + json = await response.json(); + } catch (parseError) { + // IDP returned non-JSON (HTML error page, empty body, etc.) — treat as transient + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: invalid JSON response from IDP`); + } let {access_token, refresh_token: new_refresh_token, expires_in, id_token} = json; // Defensively ensure we never propagate an undefined access token. if (!access_token) {