From 8f3574c48bd02c92dfe42f3fa7050f958043e1f1 Mon Sep 17 00:00:00 2001 From: Thanasis G <96360514+gthanasis@users.noreply.github.com> Date: Wed, 30 Sep 2026 09:02:04 +0000 Subject: [PATCH] fix(core): Skip the preAuthentication retry while the stored token is fresh (no-changelog) (#38927) Co-authored-by: Claude Opus 5 --- .../__tests__/authentication.test.ts | 108 ++++++++++++++++++ .../utils/request-helpers/authentication.ts | 42 +++++-- .../utils/request-helpers/oauth.ts | 8 +- .../AtlassianServiceAccountApi.credentials.ts | 28 ++++- ...ssianServiceAccountApi.credentials.test.ts | 20 +++- .../Confluence/test/transport/index.test.ts | 5 +- .../nodes/Confluence/transport/index.ts | 9 +- packages/workflow/src/interfaces.ts | 8 ++ 8 files changed, 208 insertions(+), 20 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts index c8248c80f80..0de762e4e87 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts @@ -125,6 +125,36 @@ describe('httpRequestWithAuthentication', () => { ); }); + // An explicit preAuthenticationRetryStatusCode replaces the default, so a caller can leave 401 out. + test('does NOT refresh on 401 when preAuthenticationRetryStatusCode excludes it', async () => { + mockAdditionalData.credentialsHelper.getParentTypes.mockReturnValue([]); + mockThis.getCredentials.mockResolvedValue({ sessionToken: 'stale' }); + const requestOptions: IHttpRequestOptions = { method: 'GET', url: `${baseUrl}/items` }; + mockAdditionalData.credentialsHelper.authenticate.mockResolvedValue(requestOptions); + + const error401 = Object.assign(new Error('401 - session expired'), { + response: { status: 401 }, + }); + request.mockRejectedValueOnce(error401); + + await expect( + httpRequestWithAuthentication.call( + mockThis, + 'testSessionAuth', + requestOptions, + mockWorkflow, + mockNode, + mockAdditionalData, + { preAuthenticationRetryStatusCode: [403, 404] }, + ), + ).rejects.toSatisfy( + (thrown: unknown) => thrown instanceof NodeApiError && thrown.cause === error401, + ); + + expect(request).toHaveBeenCalledTimes(1); + expect(mockAdditionalData.credentialsHelper.preAuthentication).toHaveBeenCalledTimes(1); + }); + // By default this path's generic preAuthentication retry only fires on exactly // 401 (unlike the legacy requestWithAuthentication, which retries on any error — // see the "requestWithAuthentication (legacy)" suite below) — a 404/403 here @@ -196,6 +226,84 @@ describe('httpRequestWithAuthentication', () => { expect(request).toHaveBeenCalledTimes(2); }); + // `skipPreAuthenticationRetryWhileTokenIsFresh`: a 403/404 also means "does not exist", so + // gate those on the expiry the credential stored, or a batch of missing resources costs one + // token exchange plus one credential write each. + describe('skipPreAuthenticationRetryWhileTokenIsFresh', () => { + const options = { + preAuthenticationRetryStatusCode: [401, 403, 404], + skipPreAuthenticationRetryWhileTokenIsFresh: true, + }; + + const runWith = async ( + credentials: ICredentialDataDecryptedObject, + status: number, + requestOptions: IHttpRequestOptions = { method: 'GET', url: `${baseUrl}/items` }, + ) => { + mockAdditionalData.credentialsHelper.getParentTypes.mockReturnValue([]); + mockThis.getCredentials.mockResolvedValue(credentials); + mockAdditionalData.credentialsHelper.preAuthentication + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ accessToken: 'fresh' }); + mockAdditionalData.credentialsHelper.authenticate.mockResolvedValue(requestOptions); + request + .mockRejectedValueOnce( + Object.assign(new Error(`${status} - failed`), { response: { status } }), + ) + .mockResolvedValueOnce({ ok: true }); + + return await httpRequestWithAuthentication.call( + mockThis, + 'testServiceAccount', + requestOptions, + mockWorkflow, + mockNode, + mockAdditionalData, + options, + ); + }; + + test('does not retry a 404 while the stored token is still live', async () => { + await expect( + runWith({ accessToken: 'live', n8n_expires_at: String(Date.now() + 3_600_000) }, 404), + ).rejects.toThrow(NodeApiError); + + expect(request).toHaveBeenCalledTimes(1); + // only the unforced call that runs before the request + expect(mockAdditionalData.credentialsHelper.preAuthentication).toHaveBeenCalledTimes(1); + }); + + test('retries a 404 once the stored token is at its expiry', async () => { + const result = await runWith( + { accessToken: 'stale', n8n_expires_at: String(Date.now() - 1_000) }, + 404, + ); + + expect(result).toEqual({ ok: true }); + expect(request).toHaveBeenCalledTimes(2); + }); + + test.each([ + ['unknown', {}], + ['unparsable', { n8n_expires_at: 'tomorrow' }], + ])('retries a 404 when the stored expiry is %s', async (_label, stored) => { + const result = await runWith({ accessToken: 'stale', ...stored }, 404); + + expect(result).toEqual({ ok: true }); + expect(request).toHaveBeenCalledTimes(2); + }); + + test('still retries a 401 while the stored token reads as live, since the server rejected it', async () => { + const result = await runWith( + { accessToken: 'revoked', n8n_expires_at: String(Date.now() + 3_600_000) }, + 401, + ); + + expect(result).toEqual({ ok: true }); + expect(request).toHaveBeenCalledTimes(2); + }); + }); + test('refreshes but does NOT resend a drained form-data body on 401; the original error surfaces', async () => { mockAdditionalData.credentialsHelper.getParentTypes.mockReturnValue([]); mockThis.getCredentials.mockResolvedValue({ sessionToken: 'stale' }); diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts index c4dda1ea017..d67d0366f51 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts @@ -18,7 +18,35 @@ import { ExecutionBaseError, NodeApiError, NodeOperationError } from 'n8n-workfl import { callEvalMockHandler, normalizeLegacyRequest } from '@/execution-engine/eval-mock-helpers'; import { proxyRequestToAxios } from './legacy-request-adapter'; -import { hasSingleUseBody, isTokenExpiredStatusCode, requestOAuth1, requestOAuth2 } from './oauth'; +import { + hasSingleUseBody, + isTokenExpiryInFuture, + isTokenExpiredStatusCode, + requestOAuth1, + requestOAuth2, +} from './oauth'; + +/** + * Whether a failed request earns the generic `preAuthentication` refresh-and-resend. Only the + * statuses in `preAuthenticationRetryStatusCode` (default 401) do; an explicit list replaces the + * default. A 401 means the server rejected the token, so it skips the expiry gate. Any other + * configured status can be ambiguous (a gateway that answers 404 both for an expired token and for + * a resource that does not exist), so `skipPreAuthenticationRetryWhileTokenIsFresh` lets a caller + * gate those on the expiry the credential stored in `n8n_expires_at`, instead of paying a token + * exchange and a credential write per missing resource. An absent or unparsable expiry still + * retries. + */ +function shouldRetryAfterPreAuthentication( + status: unknown, + credentials: ICredentialDataDecryptedObject, + options?: IAdditionalCredentialOptions, +): boolean { + if (!isTokenExpiredStatusCode(status, options?.preAuthenticationRetryStatusCode ?? 401)) { + return false; + } + if (status === 401 || options?.skipPreAuthenticationRetryWhileTokenIsFresh !== true) return true; + return !isTokenExpiryInFuture(credentials.n8n_expires_at); +} export async function httpRequestWithAuthentication( this: IAllExecuteFunctions, @@ -110,16 +138,16 @@ export async function httpRequestWithAuthentication( // if there is a pre authorization method defined and // the method failed due to unauthorized request if ( - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - isTokenExpiredStatusCode( - error.response?.status, - additionalCredentialOptions?.preAuthenticationRetryStatusCode ?? 401, - ) && additionalData.credentialsHelper.preAuthentication !== undefined && // OAuth 401s are already retried inside requestOAuth1/2 and leave // credentialsDecrypted unset; with nothing refreshed, resending the same // request (possibly with a consumed single-use body) could only fail again - credentialsDecrypted !== undefined + credentialsDecrypted !== undefined && + shouldRetryAfterPreAuthentication( + error.response?.status, + credentialsDecrypted, + additionalCredentialOptions, + ) ) { try { // try to refresh the credentials diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/oauth.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/oauth.ts index 6e0d23b87f0..27b1657b652 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/oauth.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/oauth.ts @@ -340,9 +340,9 @@ const TOKEN_EXPIRY_BUFFER_MS = 60_000; * Whether the stored token still has time left on it. An absent or unparsable expiry counts as * expired, so callers fall back to refreshing when the expiry is unknown. */ -function isStoredTokenUnexpired(credentials: OAuth2CredentialData): boolean { - const expiresAt = Number(credentials.oauthTokenData?.n8n_expires_at); - return Number.isFinite(expiresAt) && Date.now() + TOKEN_EXPIRY_BUFFER_MS < expiresAt; +export function isTokenExpiryInFuture(expiresAt: unknown): boolean { + const timestamp = Number(expiresAt); + return Number.isFinite(timestamp) && Date.now() + TOKEN_EXPIRY_BUFFER_MS < timestamp; } function isSingleUseValue(value: unknown): boolean { @@ -503,7 +503,7 @@ export async function requestOAuth2( if (shouldSkipTokenRefresh) return false; if (!isTokenExpiredStatusCode(status, tokenExpiredStatusCode)) return false; if (status === 401 || oAuth2Options?.skipRefreshWhileTokenIsFresh !== true) return true; - return !isStoredTokenUnexpired(credentials); + return !isTokenExpiryInFuture(credentials.oauthTokenData?.n8n_expires_at); }; const refreshCtx: RefreshOAuth2TokenContext = { diff --git a/packages/nodes-base/credentials/AtlassianServiceAccountApi.credentials.ts b/packages/nodes-base/credentials/AtlassianServiceAccountApi.credentials.ts index ee4f7d7b34b..3e8bf84aae5 100644 --- a/packages/nodes-base/credentials/AtlassianServiceAccountApi.credentials.ts +++ b/packages/nodes-base/credentials/AtlassianServiceAccountApi.credentials.ts @@ -18,7 +18,20 @@ import { const TOKEN_URL = 'https://auth.atlassian.com/oauth/token'; -export async function getAccessToken(credentials: ICredentialDataDecryptedObject): Promise { +/** + * Epoch-millisecond expiry for the minted token, stored alongside it so core can tell a live + * token from one that has to be renewed when the gateway answers an ambiguous 403/404 (ENT-408). + * An omitted or unusable `expires_in` yields '', which core reads as "unknown" and retries on. + */ +function expiresAtFrom(response: object): string { + const expiresIn = Number((response as { expires_in?: unknown }).expires_in); + if (!Number.isFinite(expiresIn) || expiresIn <= 0) return ''; + return String(Date.now() + expiresIn * 1000); +} + +export async function getAccessToken( + credentials: ICredentialDataDecryptedObject, +): Promise<{ accessToken: string; n8n_expires_at: string }> { const stringOrEmpty = (value: unknown) => (typeof value === 'string' ? value.trim() : ''); const clientId = stringOrEmpty(credentials.clientId); const clientSecret = stringOrEmpty(credentials.clientSecret); @@ -61,7 +74,7 @@ export async function getAccessToken(credentials: ICredentialDataDecryptedObject throw new OperationalError('Atlassian authentication did not return an access token'); } - return response.access_token; + return { accessToken: response.access_token, n8n_expires_at: expiresAtFrom(response) }; } export class AtlassianServiceAccountApi implements ICredentialType { @@ -83,6 +96,14 @@ export class AtlassianServiceAccountApi implements ICredentialType { }, default: '', }, + { + // Written by `preAuthentication`, read by core's refresh-and-resend gate. Declared so it + // survives `applyDefaultsAndOverwrites`, which drops undeclared credential fields. + displayName: 'Access Token Expires At', + name: 'n8n_expires_at', + type: 'hidden', + default: '', + }, { displayName: "The service account's scopes are chosen when its OAuth 2.0 credential is created in Atlassian administration and can't be changed afterwards. Product permissions still apply on top — grant the service account access to projects and spaces like any other user.", @@ -114,8 +135,7 @@ export class AtlassianServiceAccountApi implements ICredentialType { ]; async preAuthentication(this: IHttpRequestHelper, credentials: ICredentialDataDecryptedObject) { - const accessToken = await getAccessToken(credentials); - return { accessToken }; + return await getAccessToken(credentials); } async authenticate( diff --git a/packages/nodes-base/credentials/test/AtlassianServiceAccountApi.credentials.test.ts b/packages/nodes-base/credentials/test/AtlassianServiceAccountApi.credentials.test.ts index 14a961f63cd..c830e941723 100644 --- a/packages/nodes-base/credentials/test/AtlassianServiceAccountApi.credentials.test.ts +++ b/packages/nodes-base/credentials/test/AtlassianServiceAccountApi.credentials.test.ts @@ -75,7 +75,9 @@ describe('AtlassianServiceAccountApi Credential', () => { it('exchanges client credentials for an access token without a scope parameter', async () => { const result = await callPreAuthentication(baseCredentials); - expect(result).toEqual({ accessToken: 'abc' }); + expect(result.accessToken).toBe('abc'); + // Stored so core can skip the refresh-and-resend while the token is still live + expect(Number(result.n8n_expires_at)).toBeGreaterThan(Date.now() + 3_500_000); expect(requestMock).toHaveBeenCalledTimes(1); const options = requestMock.mock.calls[0][0] as IHttpRequestOptions; expect(options.url).toBe('https://auth.atlassian.com/oauth/token'); @@ -91,6 +93,22 @@ describe('AtlassianServiceAccountApi Credential', () => { expect(requestsMock).toHaveBeenCalledWith({ useDefaultSsrfPolicy: 'unsafe' }); }); + it.each([ + ['omitted', {}], + ['zero', { expires_in: 0 }], + ['not a number', { expires_in: 'soon' }], + ])( + 'leaves the expiry empty when expires_in is %s, so core keeps retrying', + async (_label, extra) => { + requestMock.mockResolvedValue({ access_token: 'abc', ...extra }); + + expect(await getAccessToken(baseCredentials)).toEqual({ + accessToken: 'abc', + n8n_expires_at: '', + }); + }, + ); + it('trims whitespace from the client ID and secret', async () => { await getAccessToken({ ...baseCredentials, clientId: ' client-id ' }); diff --git a/packages/nodes-base/nodes/Confluence/test/transport/index.test.ts b/packages/nodes-base/nodes/Confluence/test/transport/index.test.ts index 4b353af5b9a..274ef92cfbf 100644 --- a/packages/nodes-base/nodes/Confluence/test/transport/index.test.ts +++ b/packages/nodes-base/nodes/Confluence/test/transport/index.test.ts @@ -666,7 +666,10 @@ describe('credential routing (authentication selector)', () => { expect(mockHttpRequestWithAuthentication).toHaveBeenLastCalledWith( 'atlassianServiceAccountApi', expect.anything(), - { preAuthenticationRetryStatusCode: [401, 403, 404] }, + { + preAuthenticationRetryStatusCode: [401, 403, 404], + skipPreAuthenticationRetryWhileTokenIsFresh: true, + }, ); }); diff --git a/packages/nodes-base/nodes/Confluence/transport/index.ts b/packages/nodes-base/nodes/Confluence/transport/index.ts index 37d9b49d962..b6a102bbb2b 100644 --- a/packages/nodes-base/nodes/Confluence/transport/index.ts +++ b/packages/nodes-base/nodes/Confluence/transport/index.ts @@ -18,13 +18,16 @@ export const SERVICE_ACCOUNT_CREDENTIAL_NAME = 'atlassianServiceAccountApi'; // The gateway answers 403/404 for an expired token, not the 401 n8n's credential-refresh // helpers look for (ENT-408). 401 stays, since a revoked token still gets one, and -// `skipRefreshWhileTokenIsFresh` keeps a genuinely missing page from paying for a refresh. -// The two credentials take different refresh paths, hence the two option names. +// the `skip...WhileTokenIsFresh` options keep a genuinely missing page from paying for a refresh. +// The two credentials take different refresh paths, hence the two sets of option names. const ADDITIONAL_CREDENTIAL_OPTIONS: Record = { [CONFLUENCE_CREDENTIAL_NAME]: { oauth2: { tokenExpiredStatusCode: [401, 403, 404], skipRefreshWhileTokenIsFresh: true }, }, - [SERVICE_ACCOUNT_CREDENTIAL_NAME]: { preAuthenticationRetryStatusCode: [401, 403, 404] }, + [SERVICE_ACCOUNT_CREDENTIAL_NAME]: { + preAuthenticationRetryStatusCode: [401, 403, 404], + skipPreAuthenticationRetryWhileTokenIsFresh: true, + }, }; /** diff --git a/packages/workflow/src/interfaces.ts b/packages/workflow/src/interfaces.ts index e81d9cea906..92ff7b2612d 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -56,6 +56,14 @@ export interface IAdditionalCredentialOptions { * override when a gateway signals an expired token with a different status. */ preAuthenticationRetryStatusCode?: number | number[]; + /** + * Whether a `preAuthenticationRetryStatusCode` other than 401 only forces the + * refresh-and-resend when the token the credential stored in `n8n_expires_at` is at or past + * its expiry. The counterpart of `IOAuth2Options.skipRefreshWhileTokenIsFresh`, for + * credentials that mint their own token in `preAuthentication`. A listed 401 and an unknown + * expiry still retry. + */ + skipPreAuthenticationRetryWhileTokenIsFresh?: boolean; } export type IAllExecuteFunctions =