From c7b90d00518e587331d1f4b2ff5eb1901a56208a Mon Sep 17 00:00:00 2001 From: trananhkiet Date: Thu, 23 Jul 2026 08:21:56 +0000 Subject: [PATCH] fix(core): Handle MCP OAuth2 token expiry and PKCE negotiation (#34494) Co-authored-by: Tran Anh Kiet <22877233+trananhkiet@users.noreply.github.com> --- .../client-oauth2/src/client-oauth2-token.ts | 1 + packages/@n8n/client-oauth2/src/types.ts | 4 + .../nodes/mcp/shared/__test__/utils.test.ts | 102 ++++++++ .../nodes-langchain/nodes/mcp/shared/utils.ts | 36 ++- .../src/__tests__/credentials-helper.test.ts | 58 +++++ .../oauth2-credential.controller.test.ts | 191 +++++++++++++++ .../oauth/oauth2-credential.controller.ts | 7 +- packages/cli/src/credentials-helper.ts | 4 +- .../src/oauth/__tests__/oauth.service.test.ts | 231 +++++++++++++++++- packages/cli/src/oauth/oauth.service.ts | 75 ++++-- .../request-helpers/__tests__/oauth.test.ts | 53 ++++ .../utils/request-helpers/oauth.ts | 20 +- 12 files changed, 751 insertions(+), 31 deletions(-) diff --git a/packages/@n8n/client-oauth2/src/client-oauth2-token.ts b/packages/@n8n/client-oauth2/src/client-oauth2-token.ts index e44870ed294..8092508c90c 100644 --- a/packages/@n8n/client-oauth2/src/client-oauth2-token.ts +++ b/packages/@n8n/client-oauth2/src/client-oauth2-token.ts @@ -9,6 +9,7 @@ export interface ClientOAuth2TokenData extends Record { const MockedClient = Client as MockedClass; describe('utils', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + describe('tryRefreshOAuth2Token', () => { it('should refresh an OAuth2 token without headers', async () => { const ctx = mockDeep(); @@ -150,6 +154,104 @@ describe('utils', () => { expect(result.headers).toBeUndefined(); }); + it('should ignore a provider expiry field when the internal expiry is unknown', async () => { + const ctx = mockDeep(); + const credentials = { + oauthTokenData: { + access_token: 'access-token', + refresh_token: 'refresh-token', + expires_at: '1700000060', + }, + }; + ctx.getCredentials.mockResolvedValue(credentials); + + const result = await getAuthHeaders(ctx, 'mcpOAuth2Api'); + + expect(result).toEqual({ + headers: { Authorization: 'Bearer access-token' }, + credentials, + }); + expect(ctx.helpers.refreshOAuth2Token).not.toHaveBeenCalled(); + }); + + it('should not proactively refresh legacy credentials without an absolute expiry', async () => { + const ctx = mockDeep(); + const credentials = { + oauthTokenData: { + access_token: 'access-token', + refresh_token: 'refresh-token', + expires_in: '3600', + }, + }; + ctx.getCredentials.mockResolvedValue(credentials); + + const result = await getAuthHeaders(ctx, 'mcpOAuth2Api'); + + expect(result.headers).toEqual({ Authorization: 'Bearer access-token' }); + expect(ctx.helpers.refreshOAuth2Token).not.toHaveBeenCalled(); + }); + + it('should refresh mcpOAuth2Api credentials before the access token expires', async () => { + const now = 1_700_000_000_000; + vi.spyOn(Date, 'now').mockReturnValue(now); + const ctx = mockDeep(); + const credentials = { + oauthTokenData: { + access_token: 'access-token', + refresh_token: 'refresh-token', + n8n_expires_at: String(now + 60_000), + }, + }; + ctx.getCredentials.mockResolvedValue(credentials); + ctx.helpers.refreshOAuth2Token.mockResolvedValue({ + access_token: 'new-access-token', + }); + + const result = await getAuthHeaders(ctx, 'mcpOAuth2Api'); + + expect(result.headers).toEqual({ Authorization: 'Bearer new-access-token' }); + expect(ctx.helpers.refreshOAuth2Token).toHaveBeenCalledWith('mcpOAuth2Api'); + }); + + it('should not refresh mcpOAuth2Api credentials when the access token is still valid', async () => { + const now = 1_700_000_000_000; + vi.spyOn(Date, 'now').mockReturnValue(now); + const ctx = mockDeep(); + const credentials = { + oauthTokenData: { + access_token: 'access-token', + refresh_token: 'refresh-token', + n8n_expires_at: String(now + 10 * 60_000), + }, + }; + ctx.getCredentials.mockResolvedValue(credentials); + + const result = await getAuthHeaders(ctx, 'mcpOAuth2Api'); + + expect(result.headers).toEqual({ Authorization: 'Bearer access-token' }); + expect(ctx.helpers.refreshOAuth2Token).not.toHaveBeenCalled(); + }); + + it('should not immediately refresh a short-lived token', async () => { + const now = 1_700_000_000_000; + vi.spyOn(Date, 'now').mockReturnValue(now); + const ctx = mockDeep(); + const credentials = { + oauthTokenData: { + access_token: 'access-token', + refresh_token: 'refresh-token', + expires_in: '60', + n8n_expires_at: String(now + 60_000), + }, + }; + ctx.getCredentials.mockResolvedValue(credentials); + + const result = await getAuthHeaders(ctx, 'mcpOAuth2Api'); + + expect(result.headers).toEqual({ Authorization: 'Bearer access-token' }); + expect(ctx.helpers.refreshOAuth2Token).not.toHaveBeenCalled(); + }); + it('should return the headers and credentials for headerAuth', async () => { const ctx = mockDeep(); const credentials = { diff --git a/packages/@n8n/nodes-langchain/nodes/mcp/shared/utils.ts b/packages/@n8n/nodes-langchain/nodes/mcp/shared/utils.ts index d1ac743e476..65e7672ae54 100644 --- a/packages/@n8n/nodes-langchain/nodes/mcp/shared/utils.ts +++ b/packages/@n8n/nodes-langchain/nodes/mcp/shared/utils.ts @@ -73,6 +73,13 @@ type OnUnauthorizedHandler = ( headers?: Record, ) => Promise | null>; +const OAUTH2_REFRESH_BUFFER_MS = 2 * 60 * 1000; +const OAUTH2_REFRESH_BUFFER_RATIO = 0.1; + +type McpOAuth2Credentials = ICredentialDataDecryptedObject & { + oauthTokenData?: ClientOAuth2TokenData; +}; + type ConnectMcpClientError = | { type: 'invalid_url'; error: Error } | { type: 'connection'; error: Error } @@ -338,8 +345,26 @@ function createAuthFetch( }; } +function shouldRefreshOAuth2Token(credentials: McpOAuth2Credentials): boolean { + const tokenData = credentials.oauthTokenData; + if (!tokenData?.refresh_token) return false; + + const expiresAt = Number(tokenData.n8n_expires_at); + if (!Number.isFinite(expiresAt)) { + return false; + } + + const expiresInMs = Number(tokenData.expires_in) * 1000; + const refreshBufferMs = + Number.isFinite(expiresInMs) && expiresInMs > 0 + ? Math.min(OAUTH2_REFRESH_BUFFER_MS, expiresInMs * OAUTH2_REFRESH_BUFFER_RATIO) + : OAUTH2_REFRESH_BUFFER_MS; + + return Date.now() + refreshBufferMs >= expiresAt; +} + export async function getAuthHeaders( - ctx: Pick, + ctx: IExecuteFunctions | ISupplyDataFunctions | ILoadOptionsFunctions, authentication: McpAuthenticationOption, ): Promise<{ headers?: Record; @@ -347,11 +372,18 @@ export async function getAuthHeaders( }> { if (isMcpOAuth2Authentication(authentication)) { const credentials = await ctx - .getCredentials<{ oauthTokenData?: { access_token?: string } }>(authentication) + .getCredentials(authentication) .catch(() => null); if (!credentials) return {}; + if (shouldRefreshOAuth2Token(credentials)) { + const refreshedHeaders = await tryRefreshOAuth2Token(ctx, authentication); + if (refreshedHeaders) { + return { headers: refreshedHeaders, credentials }; + } + } + if (!credentials.oauthTokenData?.access_token) { return { credentials }; } diff --git a/packages/cli/src/__tests__/credentials-helper.test.ts b/packages/cli/src/__tests__/credentials-helper.test.ts index c292b91fa2e..3723d1036c2 100644 --- a/packages/cli/src/__tests__/credentials-helper.test.ts +++ b/packages/cli/src/__tests__/credentials-helper.test.ts @@ -229,6 +229,64 @@ describe('CredentialsHelper', () => { ), ).rejects.toThrow('save workflow to view'); }); + + test('preserves PKCE flag negotiated by dynamic client registration', async () => { + const credentialType: ICredentialType = { + name: 'mcpOAuth2Api', + displayName: 'MCP OAuth2 API', + properties: [ + { + displayName: 'Use Dynamic Client Registration', + name: 'useDynamicClientRegistration', + type: 'boolean', + default: true, + }, + ], + }; + mockNodesAndCredentials.getCredential.calledWith(credentialType.name).mockReturnValue({ + type: credentialType, + sourcePath: '', + }); + const credentialsOverwrites = mock(); + credentialsOverwrites.applyOverwrite.mockImplementation((_type, data) => data); + const helper = new CredentialsHelper( + new CredentialTypes(mockNodesAndCredentials), + credentialsOverwrites, + credentialsRepository, + dynamicCredentialProxy, + secretsProviderRepository, + licenseState, + externalSecretsConfig, + mock(), + ); + + const result = await helper.applyDefaultsAndOverwrites( + mock({ variables: {} }), + { + useDynamicClientRegistration: true, + clientId: 'registered-client-id', + clientSecret: 'registered-secret', + authUrl: 'https://auth.example.com/authorize', + accessTokenUrl: 'https://auth.example.com/token', + grantType: 'authorizationCode', + authentication: 'body', + usePkce: true, + }, + credentialType.name, + 'internal', + ); + + expect(result).toMatchObject({ + useDynamicClientRegistration: true, + clientId: 'registered-client-id', + clientSecret: 'registered-secret', + authUrl: 'https://auth.example.com/authorize', + accessTokenUrl: 'https://auth.example.com/token', + grantType: 'authorizationCode', + authentication: 'body', + usePkce: true, + }); + }); }); describe('authenticate', () => { diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts index c831ecc00a2..186fc53fbcf 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth2-credential.controller.test.ts @@ -813,6 +813,197 @@ describe('OAuth2CredentialController', () => { expect(oauthService.encryptAndSaveData).toHaveBeenCalled(); }); + it('should send code_verifier for authorization code flow when PKCE is enabled', async () => { + const now = 1_700_000_000_000; + vi.spyOn(Date, 'now').mockReturnValue(now); + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = vi.fn().mockResolvedValue({ + data: { + access_token: 'new_token', + expires_at: 'provider-expiry', + expires_in: 3600, + }, + }); + vi.mocked(ClientOAuth2).mockImplementation(function () { + return { code: { getToken: mockGetToken } } as any; + }); + + const mockResolvedCredential = mock({ id: '1' }); + const mockState = { + token: 'token', + cid: '1', + userId: '123', + origin: 'static-credential' as const, + createdAt: timestamp, + data: 'encrypted-data', + }; + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret', codeVerifier: 'code_verifier' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + scope: 'openid', + grantType: 'authorizationCode', + authentication: 'header', + usePkce: true, + }, + mockState, + { csrfSecret: 'csrf-secret', codeVerifier: 'code_verifier' }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { + code: 'auth_code', + state: validState, + }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); + + await controller.handleCallback(req, res); + + expect(mockGetToken).toHaveBeenCalledWith( + expect.stringContaining('code=auth_code'), + expect.objectContaining({ + body: { code_verifier: 'code_verifier' }, + }), + ); + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, + expect.objectContaining({ + oauthTokenData: expect.objectContaining({ + expires_at: 'provider-expiry', + n8n_expires_at: String(now + 3_600_000), + }), + }), + ); + }); + + it('should send code_verifier with client authentication fields for body-authenticated authorization code flow', async () => { + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + const mockGetToken = vi.fn().mockResolvedValue({ + data: { access_token: 'new_token' }, + }); + vi.mocked(ClientOAuth2).mockImplementation(function () { + return { code: { getToken: mockGetToken } } as any; + }); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { csrfSecret: 'csrf-secret', codeVerifier: 'code_verifier' }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + grantType: 'authorizationCode', + authentication: 'body', + usePkce: true, + }, + { + token: 'token', + cid: '1', + userId: '123', + origin: 'static-credential', + createdAt: timestamp, + data: 'encrypted-data', + }, + { csrfSecret: 'csrf-secret', codeVerifier: 'code_verifier' }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { code: 'auth_code', state: validState }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); + + await controller.handleCallback(req, res); + + expect(mockGetToken).toHaveBeenCalledWith( + expect.stringContaining('code=auth_code'), + expect.objectContaining({ + body: { + code_verifier: 'code_verifier', + client_id: 'client_id', + client_secret: 'client_secret', + }, + }), + ); + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, + expect.objectContaining({ + oauthTokenData: expect.objectContaining({ access_token: 'new_token' }), + }), + ); + }); + + it('should preserve the existing absolute expiry when the token response omits expires_in', async () => { + const now = 1_700_000_000_000; + const existingExpiresAt = String(now + 60_000); + vi.spyOn(Date, 'now').mockReturnValue(now); + const { ClientOAuth2 } = await import('@n8n/client-oauth2'); + vi.mocked(ClientOAuth2).mockImplementation(function () { + return { + code: { getToken: vi.fn().mockResolvedValue({ data: { access_token: 'new_token' } }) }, + } as any; + }); + + const mockResolvedCredential = mock({ id: '1' }); + oauthService.resolveCredential.mockResolvedValueOnce([ + mockResolvedCredential, + { + oauthTokenData: { + access_token: 'old_token', + expires_in: 3600, + n8n_expires_at: existingExpiresAt, + }, + }, + { + clientId: 'client_id', + clientSecret: 'client_secret', + authUrl: 'https://example.domain/oauth2/auth', + accessTokenUrl: 'https://example.domain/oauth2/token', + grantType: 'authorizationCode', + authentication: 'header', + }, + { + token: 'token', + cid: '1', + userId: '123', + origin: 'static-credential', + createdAt: timestamp, + data: 'encrypted-data', + }, + { csrfSecret: 'csrf-secret' }, + ]); + oauthService.getBaseUrl.mockReturnValue('http://localhost:5678/rest/oauth2-credential'); + externalHooks.run.mockResolvedValue(undefined); + + const req = mock({ + query: { code: 'auth_code', state: validState }, + originalUrl: '/oauth2-credential/callback?code=auth_code&state=state', + }); + + await controller.handleCallback(req, res); + + expect(oauthService.encryptAndSaveData).toHaveBeenCalledWith( + mockResolvedCredential, + expect.objectContaining({ + oauthTokenData: expect.objectContaining({ + access_token: 'new_token', + expires_in: 3600, + n8n_expires_at: existingExpiresAt, + }), + }), + ); + }); + it('should handle body authentication method', async () => { const { ClientOAuth2 } = await import('@n8n/client-oauth2'); const mockGetToken = vi.fn().mockResolvedValue({ diff --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index d4ea2e7c792..7dceeff4330 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -58,7 +58,7 @@ export class OAuth2CredentialController { const oAuthOptions = this.convertCredentialToOptions(oauthCredentials); - const isPkce = oauthCredentials.grantType === 'pkce'; + const isPkce = oauthCredentials.grantType === 'pkce' || oauthCredentials.usePkce === true; const isBodyAuth = oauthCredentials.authentication === 'body'; const body: Record = { ...(oAuthOptions.body ?? {}) }; @@ -111,6 +111,11 @@ export class OAuth2CredentialController { ...tokenResponse, } as ICredentialDataDecryptedObject; + const expiresInSeconds = Number(tokenResponse.expires_in); + if (Number.isFinite(expiresInSeconds) && expiresInSeconds > 0) { + oauthTokenData.n8n_expires_at = String(Date.now() + expiresInSeconds * 1000); + } + if (typeof state.resource === 'string') { oauthTokenData.resource = state.resource; } diff --git a/packages/cli/src/credentials-helper.ts b/packages/cli/src/credentials-helper.ts index 646ab1da49c..fde22da0c9f 100644 --- a/packages/cli/src/credentials-helper.ts +++ b/packages/cli/src/credentials-helper.ts @@ -587,8 +587,7 @@ export class CredentialsHelper extends ICredentialsHelper { decryptedData.allowedDomains = decryptedDataOriginal.allowedDomains; } - // When using dynamic client registration, fields - // for client ID, secret, auth URL, access token URL, grant type and authentication + // When using dynamic client registration, OAuth fields negotiated at runtime // are not shown in the UI, so we need to copy them from the original data. if (decryptedData.useDynamicClientRegistration) { decryptedData.clientId = decryptedDataOriginal.clientId; @@ -597,6 +596,7 @@ export class CredentialsHelper extends ICredentialsHelper { decryptedData.accessTokenUrl = decryptedDataOriginal.accessTokenUrl; decryptedData.grantType = decryptedDataOriginal.grantType; decryptedData.authentication = decryptedDataOriginal.authentication; + decryptedData.usePkce = decryptedDataOriginal.usePkce; } const additionalKeys = getAdditionalKeys(additionalData, mode, null, { diff --git a/packages/cli/src/oauth/__tests__/oauth.service.test.ts b/packages/cli/src/oauth/__tests__/oauth.service.test.ts index 2c1d800fa39..ed04e932473 100644 --- a/packages/cli/src/oauth/__tests__/oauth.service.test.ts +++ b/packages/cli/src/oauth/__tests__/oauth.service.test.ts @@ -2254,11 +2254,12 @@ describe('OauthService', () => { expect(callArgs[1]).toHaveProperty('authUrl', 'https://example.domain/oauth2/auth'); expect(callArgs[1]).toHaveProperty('accessTokenUrl', 'https://example.domain/oauth2/token'); expect(callArgs[1]).toHaveProperty('clientId', 'registered_client_id'); - expect(callArgs[1]).toHaveProperty('clientSecret', 'registered_client_secret'); + expect(callArgs[1]).not.toHaveProperty('clientSecret'); expect(callArgs[1]).toHaveProperty('scope', 'openid profile'); expect(callArgs[1]).toHaveProperty('grantType', 'pkce'); expect(callArgs[1]).not.toHaveProperty('csrfSecret'); expect(callArgs[1]).not.toHaveProperty('codeVerifier'); + expect(callArgs[2]).toEqual(expect.arrayContaining(['authentication', 'clientSecret'])); expect(cacheService.set).toHaveBeenCalledWith( expect.stringMatching(/^oauth:flow:/), expect.objectContaining({ @@ -3729,6 +3730,9 @@ describe('OauthService', () => { const url = new URL(options.authorizationUri ?? ''); if (options.resource) url.searchParams.set('resource', options.resource); if (options.state) url.searchParams.set('state', options.state); + for (const [key, value] of Object.entries(options.query ?? {})) { + if (typeof value === 'string') url.searchParams.set(key, value); + } return url.toString(); }, }), @@ -3889,6 +3893,7 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com/issuer', toUpdate, + [], ); expect(httpClientMock.get).toHaveBeenNthCalledWith( @@ -3944,6 +3949,7 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('clientCredentials'); @@ -3966,6 +3972,35 @@ describe('OauthService', () => { }, ); + it('should clear a stale PKCE flag when selecting client credentials', async () => { + const oauthCredentials = makeDcrCredentials({ usePkce: true }); + const toUpdate = {}; + + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: makeMetadata({ + grant_types_supported: ['client_credentials'], + token_endpoint_auth_methods_supported: ['client_secret_basic'], + }), + }); + vi.mocked(httpClientMock.post).mockResolvedValueOnce({ + data: { client_id: 'registered-client-id', client_secret: 'registered-secret' }, + }); + + await (service as any).performDynamicClientRegistration( + oauthCredentials, + 'https://auth.example.com', + toUpdate, + [], + ); + + expect(oauthCredentials).toMatchObject({ + grantType: 'clientCredentials', + usePkce: false, + }); + expect(toUpdate).toEqual(expect.objectContaining({ usePkce: false })); + expect((service as any).shouldUsePkce(oauthCredentials)).toBe(false); + }); + it('should default client credentials flow to header authentication when the server omits token_endpoint_auth_methods_supported', async () => { const oauthCredentials = makeDcrCredentials(); const toUpdate = {}; @@ -3981,6 +4016,7 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('clientCredentials'); @@ -4017,6 +4053,7 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('authorizationCode'); @@ -4043,6 +4080,7 @@ describe('OauthService', () => { makeDcrCredentials(), 'https://auth.example.com', {}, + [], ), ).rejects.toThrow('No supported grant type and authentication method found'); expect(httpClientMock.post).not.toHaveBeenCalled(); @@ -4073,14 +4111,17 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('authorizationCode'); expect(oauthCredentials.authentication).toBe(authentication); + expect(oauthCredentials.usePkce).toBe(true); expect(toUpdate).toEqual( expect.objectContaining({ grantType: 'authorizationCode', authentication, + usePkce: true, clientId: 'registered-client-id', clientSecret: 'registered-secret', }), @@ -4095,6 +4136,49 @@ describe('OauthService', () => { }, ); + it('should honor the advertised order when both client authentication methods are supported', async () => { + const oauthCredentials = makeDcrCredentials(); + const toUpdate = {}; + + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: makeMetadata({ + grant_types_supported: ['authorization_code'], + token_endpoint_auth_methods_supported: ['client_secret_post', 'client_secret_basic'], + code_challenge_methods_supported: ['S256'], + }), + }); + vi.mocked(httpClientMock.post).mockResolvedValueOnce({ + data: { client_id: 'registered-client-id', client_secret: 'registered-secret' }, + }); + + await (service as any).performDynamicClientRegistration( + oauthCredentials, + 'https://auth.example.com', + toUpdate, + [], + ); + + expect(oauthCredentials.grantType).toBe('authorizationCode'); + expect(oauthCredentials.authentication).toBe('body'); + expect(oauthCredentials.usePkce).toBe(true); + expect(toUpdate).toEqual( + expect.objectContaining({ + grantType: 'authorizationCode', + authentication: 'body', + usePkce: true, + clientId: 'registered-client-id', + clientSecret: 'registered-secret', + }), + ); + expect(httpClientMock.post).toHaveBeenCalledWith( + 'https://auth.example.com/oauth2/register', + expect.objectContaining({ + grant_types: ['authorization_code', 'refresh_token'], + token_endpoint_auth_method: 'client_secret_post', + }), + ); + }); + it('should register PKCE when the server supports both S256 and the none auth method', async () => { const oauthCredentials = makeDcrCredentials(); const toUpdate = {}; @@ -4107,13 +4191,14 @@ describe('OauthService', () => { }), }); vi.mocked(httpClientMock.post).mockResolvedValueOnce({ - data: { client_id: 'registered-client-id' }, + data: { client_id: 'registered-client-id', client_secret: 'ignored-secret' }, }); await (service as any).performDynamicClientRegistration( oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('pkce'); @@ -4126,6 +4211,38 @@ describe('OauthService', () => { ); }); + it('should clear stale client authentication when DCR selects public-client PKCE', async () => { + const oauthCredentials = makeDcrCredentials({ + clientSecret: 'stale-secret', + authentication: 'header', + }); + const toUpdate = {}; + const toDelete: string[] = []; + + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: makeMetadata({ + grant_types_supported: ['authorization_code'], + token_endpoint_auth_methods_supported: ['none'], + code_challenge_methods_supported: ['S256'], + }), + }); + vi.mocked(httpClientMock.post).mockResolvedValueOnce({ + data: { client_id: 'registered-client-id' }, + }); + + await (service as any).performDynamicClientRegistration( + oauthCredentials, + 'https://auth.example.com', + toUpdate, + toDelete, + ); + + expect(oauthCredentials.grantType).toBe('pkce'); + expect(oauthCredentials.clientSecret).toBeUndefined(); + expect(oauthCredentials.authentication).toBeUndefined(); + expect(toDelete).toEqual(expect.arrayContaining(['clientSecret', 'authentication'])); + }); + it('should register PKCE when the server supports S256 and omits token_endpoint_auth_methods_supported', async () => { const oauthCredentials = makeDcrCredentials(); const toUpdate = {}; @@ -4144,6 +4261,7 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('pkce'); @@ -4174,6 +4292,7 @@ describe('OauthService', () => { oauthCredentials, 'https://auth.example.com', toUpdate, + [], ); expect(oauthCredentials.grantType).toBe('pkce'); @@ -4312,6 +4431,7 @@ describe('OauthService', () => { oauthCredentials, 'https://as.example.com', {}, + [], ); expect(requestMock).toHaveBeenCalledWith( @@ -4470,6 +4590,113 @@ describe('OauthService', () => { ); }); + it('should include PKCE parameters for confidential authorization-code clients that advertise S256', async () => { + const pkceChallenge = await import('pkce-challenge'); + vi.mocked(pkceChallenge.default).mockResolvedValue({ + code_verifier: 'code_verifier', + code_challenge: 'code_challenge', + }); + vi.spyOn(service, 'getOAuthCredentials').mockResolvedValue(makeDcrCredentials()); + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: { + authorization_servers: ['https://auth.example.com'], + resource: 'https://mcp.example.com/mcp', + }, + }); + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: { + authorization_endpoint: 'https://auth.example.com/oauth2/auth', + token_endpoint: 'https://auth.example.com/oauth2/token', + registration_endpoint: 'https://auth.example.com/oauth2/register', + grant_types_supported: ['authorization_code', 'refresh_token'], + token_endpoint_auth_methods_supported: ['client_secret_basic'], + code_challenge_methods_supported: ['S256'], + }, + }); + vi.mocked(httpClientMock.post).mockResolvedValueOnce({ + data: { client_id: 'registered-client-id', client_secret: 'registered-secret' }, + }); + + const authUri = await service.generateAOauth2AuthUri(credential, { + cid: credential.id, + origin: 'static-credential', + userId: 'user-id', + }); + + const url = new URL(authUri); + expect(url.searchParams.get('code_challenge')).toBe('code_challenge'); + expect(url.searchParams.get('code_challenge_method')).toBe('S256'); + expect(httpClientMock.post).toHaveBeenCalledWith( + 'https://auth.example.com/oauth2/register', + expect.objectContaining({ + token_endpoint_auth_method: 'client_secret_basic', + }), + ); + expect(service.encryptAndSaveData).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + grantType: 'authorizationCode', + authentication: 'header', + usePkce: true, + }), + [], + ); + expect(cacheService.set).toHaveBeenLastCalledWith( + expect.any(String), + expect.objectContaining({ codeVerifier: 'code_verifier' }), + expect.any(Number), + ); + }); + + it('should persist stale client authentication cleanup when DCR selects public-client PKCE', async () => { + const pkceChallenge = await import('pkce-challenge'); + vi.mocked(pkceChallenge.default).mockResolvedValue({ + code_verifier: 'code_verifier', + code_challenge: 'code_challenge', + }); + vi.spyOn(service, 'getOAuthCredentials').mockResolvedValue( + makeDcrCredentials({ + clientSecret: 'stale-secret', + authentication: 'header', + }), + ); + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: { + authorization_servers: ['https://auth.example.com'], + resource: 'https://mcp.example.com/mcp', + }, + }); + vi.mocked(httpClientMock.get).mockResolvedValueOnce({ + data: { + authorization_endpoint: 'https://auth.example.com/oauth2/auth', + token_endpoint: 'https://auth.example.com/oauth2/token', + registration_endpoint: 'https://auth.example.com/oauth2/register', + grant_types_supported: ['authorization_code'], + token_endpoint_auth_methods_supported: ['none'], + code_challenge_methods_supported: ['S256'], + }, + }); + vi.mocked(httpClientMock.post).mockResolvedValueOnce({ + data: { client_id: 'registered-client-id' }, + }); + + await service.generateAOauth2AuthUri(credential, { + cid: credential.id, + origin: 'static-credential', + userId: 'user-id', + }); + + expect(service.encryptAndSaveData).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + grantType: 'pkce', + usePkce: true, + clientId: 'registered-client-id', + }), + expect.arrayContaining(['authentication', 'clientSecret']), + ); + }); + it('should omit resource when discovery and credential input do not provide one', async () => { vi.spyOn(service, 'getOAuthCredentials').mockResolvedValue(makeDcrCredentials()); vi.mocked(httpClientMock.get).mockResolvedValueOnce({ diff --git a/packages/cli/src/oauth/oauth.service.ts b/packages/cli/src/oauth/oauth.service.ts index 50b7f18152f..a6d1df5ddea 100644 --- a/packages/cli/src/oauth/oauth.service.ts +++ b/packages/cli/src/oauth/oauth.service.ts @@ -905,6 +905,7 @@ export class OauthService { await this.getOAuthCredentials(credential); const toUpdate: ICredentialDataDecryptedObject = {}; + const toDelete: string[] = []; let authorizationServerUrl = oauthCredentials.serverUrl; let discoveredScopes: string[] | undefined; @@ -929,6 +930,7 @@ export class OauthService { oauthCredentials, authorizationServerUrl!, toUpdate, + toDelete, discoveredScopes, ); } @@ -951,7 +953,7 @@ export class OauthService { await this.externalHooks.run('oauth2.authenticate', [oAuthOptions]); const flowState: OauthFlowState = { csrfSecret, stateData: csrfData }; - if (oauthCredentials.grantType === 'pkce') { + if (this.shouldUsePkce(oauthCredentials)) { const { code_verifier, code_challenge } = await pkceChallenge(); oAuthOptions.query = { ...oAuthOptions.query, @@ -965,8 +967,8 @@ export class OauthService { // Only persist DCR-driven updates to the credential. CSRF/PKCE state lives in the cache // to avoid cross-user races on shared credentials. - if (Object.keys(toUpdate).length > 0) { - await this.encryptAndSaveData(credential, toUpdate); + if (Object.keys(toUpdate).length > 0 || toDelete.length > 0) { + await this.encryptAndSaveData(credential, toUpdate, toDelete); } const oAuthObj = new ClientOAuth2(oAuthOptions); @@ -984,6 +986,7 @@ export class OauthService { oauthCredentials: OAuth2CredentialData, authorizationServerUrl: string, toUpdate: ICredentialDataDecryptedObject, + toDelete: string[], discoveredResourceScopes?: string[], ): Promise { // Step 2: Discover Authorization Server Metadata (RFC 8414 / OpenID Connect) @@ -1062,16 +1065,21 @@ export class OauthService { toUpdate.scope = scope; } - const { grantType, authentication } = this.selectGrantTypeAndAuthenticationMethod( + const { grantType, authentication, usePkce } = this.selectGrantTypeAndAuthenticationMethod( metadataValidation.data.grant_types_supported ?? ['authorization_code', 'implicit'], metadataValidation.data.token_endpoint_auth_methods_supported ?? [], metadataValidation.data.code_challenge_methods_supported ?? [], ); oauthCredentials.grantType = grantType; toUpdate.grantType = grantType; + oauthCredentials.usePkce = usePkce; + toUpdate.usePkce = usePkce; if (authentication) { oauthCredentials.authentication = authentication; toUpdate.authentication = authentication; + } else { + delete oauthCredentials.authentication; + toDelete.push('authentication'); } const { grant_types, token_endpoint_auth_method } = this.mapGrantTypeAndAuthenticationMethod( @@ -1110,9 +1118,12 @@ export class OauthService { const { client_id, client_secret } = registrationValidation.data; oauthCredentials.clientId = client_id; toUpdate.clientId = client_id; - if (client_secret) { + if (authentication && client_secret) { oauthCredentials.clientSecret = client_secret; toUpdate.clientSecret = client_secret; + } else { + delete oauthCredentials.clientSecret; + toDelete.push('clientSecret'); } } @@ -1299,6 +1310,13 @@ export class OauthService { return options; } + private shouldUsePkce(credential: OAuth2CredentialData): boolean { + return ( + credential.grantType === 'pkce' || + (credential.grantType === 'authorizationCode' && credential.usePkce === true) + ); + } + /** * Fetches a `.well-known` discovery document and returns its parsed JSON body. * Only a 200 is accepted (RFC 8414 / RFC 9728 / OpenID Connect discovery endpoints respond with 200). @@ -1391,7 +1409,11 @@ export class OauthService { grantTypes: string[], tokenEndpointAuthMethods: string[], codeChallengeMethods: string[], - ): { grantType: OAuth2GrantType; authentication?: OAuth2AuthenticationMethod } { + ): { + grantType: OAuth2GrantType; + authentication?: OAuth2AuthenticationMethod; + usePkce: boolean; + } { const supportsPkce = codeChallengeMethods.includes('S256'); if (grantTypes.includes('authorization_code')) { @@ -1401,46 +1423,55 @@ export class OauthService { supportsPkce && (tokenEndpointAuthMethods.length === 0 || tokenEndpointAuthMethods.includes('none')) ) { - return { grantType: 'pkce' }; + return { grantType: 'pkce', usePkce: true }; } - if (tokenEndpointAuthMethods.includes('client_secret_basic')) { - return { grantType: 'authorizationCode', authentication: 'header' }; - } - - if (tokenEndpointAuthMethods.includes('client_secret_post')) { - return { grantType: 'authorizationCode', authentication: 'body' }; + const authentication = this.selectClientSecretAuthenticationMethod(tokenEndpointAuthMethods); + if (authentication) { + return { + grantType: 'authorizationCode', + authentication, + usePkce: supportsPkce, + }; } // S256 advertised alongside only unrecognized methods: fall back to public-client PKCE. if (supportsPkce) { - return { grantType: 'pkce' }; + return { grantType: 'pkce', usePkce: true }; } // Server omitted token_endpoint_auth_methods_supported: default to client_secret_basic (RFC 8414). if (tokenEndpointAuthMethods.length === 0) { - return { grantType: 'authorizationCode', authentication: 'header' }; + return { grantType: 'authorizationCode', authentication: 'header', usePkce: false }; } } if (grantTypes.includes('client_credentials')) { - if (tokenEndpointAuthMethods.includes('client_secret_basic')) { - return { grantType: 'clientCredentials', authentication: 'header' }; - } - - if (tokenEndpointAuthMethods.includes('client_secret_post')) { - return { grantType: 'clientCredentials', authentication: 'body' }; + const authentication = this.selectClientSecretAuthenticationMethod(tokenEndpointAuthMethods); + if (authentication) { + return { grantType: 'clientCredentials', authentication, usePkce: false }; } // Server omitted token_endpoint_auth_methods_supported: default to client_secret_basic (RFC 8414). if (tokenEndpointAuthMethods.length === 0) { - return { grantType: 'clientCredentials', authentication: 'header' }; + return { grantType: 'clientCredentials', authentication: 'header', usePkce: false }; } } throw new BadRequestError('No supported grant type and authentication method found'); } + private selectClientSecretAuthenticationMethod( + tokenEndpointAuthMethods: string[], + ): OAuth2AuthenticationMethod | undefined { + for (const authMethod of tokenEndpointAuthMethods) { + if (authMethod === 'client_secret_basic') return 'header'; + if (authMethod === 'client_secret_post') return 'body'; + } + + return undefined; + } + private mapGrantTypeAndAuthenticationMethod( grantType: OAuth2GrantType, authentication?: OAuth2AuthenticationMethod, diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/oauth.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/oauth.test.ts index 569ef43f83e..275c113f885 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/oauth.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/oauth.test.ts @@ -58,6 +58,10 @@ describe('refreshOAuth2Token', () => { } as unknown as ICredentialDataDecryptedObject); }); + afterEach(() => { + vi.restoreAllMocks(); + }); + test('should refresh the OAuth2 token with pkce grant type', async () => { mockThis.getCredentials.mockResolvedValue({ ...mockCredentialData, @@ -101,6 +105,54 @@ describe('refreshOAuth2Token', () => { ); }); + test('should store an absolute expiry when refreshed token has expires_in', async () => { + const now = 1_700_000_000_000; + const dateNow = vi.spyOn(Date, 'now').mockReturnValue(now); + mockThis.getCredentials.mockResolvedValue({ + ...mockCredentialData, + clientSecret: undefined, + grantType: 'pkce', + }); + nock(baseUrl) + .post('/token', { + client_id: 'test-client-id', + grant_type: 'refresh_token', + refresh_token: 'old-refresh-token', + }) + .reply(200, { + access_token: 'new-token', + refresh_token: 'new-refresh-token', + expires_in: '3600', + }); + + const result = await refreshOAuth2Token.call( + mockThis, + 'test-credentials-type', + mockNode, + mockAdditionalData, + ); + + expect(result).toEqual({ + access_token: 'new-token', + refresh_token: 'new-refresh-token', + expires_in: '3600', + n8n_expires_at: String(now + 3_600_000), + }); + expect( + mockAdditionalData.credentialsHelper.updateCredentialsOauthTokenData, + ).toHaveBeenCalledWith( + mockNode.credentials!['test-credentials-type'], + 'test-credentials-type', + expect.objectContaining({ + oauthTokenData: expect.objectContaining({ + n8n_expires_at: String(now + 3_600_000), + }), + }), + mockAdditionalData, + ); + dateNow.mockRestore(); + }); + test('should refresh the OAuth2 token with client credentials grant type', async () => { mockThis.getCredentials.mockResolvedValue({ ...mockCredentialData, @@ -176,6 +228,7 @@ describe('refreshOAuth2Token', () => { expect(result).toEqual({ access_token: 'new-token', refresh_token: 'new-refresh-token', + resource: 'https://mcp.example.com/', }); expect( mockAdditionalData.credentialsHelper.updateCredentialsOauthTokenData, 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 efaba3a2ee5..6cd3b6d662d 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 @@ -84,6 +84,18 @@ function buildSigningToken( ); } +function addExpiresAt(tokenData: ClientOAuth2TokenData): ClientOAuth2TokenData { + const expiresInSeconds = Number(tokenData.expires_in); + if (!Number.isFinite(expiresInSeconds) || expiresInSeconds <= 0) { + return tokenData; + } + + return { + ...tokenData, + n8n_expires_at: String(Date.now() + expiresInSeconds * 1000), + }; +} + interface RefreshOAuth2TokenContext { credentials: OAuth2CredentialData; token: ClientOAuth2Token; @@ -228,7 +240,7 @@ async function refreshOrFetchToken(ctx: RefreshOAuth2TokenContext): Promise