diff --git a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts index 570181aa78fed..488bcf61ff1b6 100644 --- a/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts +++ b/packages/cli/src/controllers/oauth/__tests__/oauth1-credential.controller.test.ts @@ -4,6 +4,7 @@ import type { Response } from 'express'; import { mock } from 'jest-mock-extended'; import { Cipher } from 'n8n-core'; import { Logger } from 'n8n-core'; +import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; import nock from 'nock'; import { Time } from '@/constants'; @@ -19,8 +20,11 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { ExternalHooks } from '@/external-hooks'; import type { OAuthRequest } from '@/requests'; import { SecretsHelper } from '@/secrets-helpers.ee'; +import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; import { mockInstance } from '@test/mocking'; +jest.mock('@/workflow-execute-additional-data'); + describe('OAuth1CredentialController', () => { mockInstance(Logger); mockInstance(ExternalHooks); @@ -28,6 +32,9 @@ describe('OAuth1CredentialController', () => { mockInstance(VariablesService, { getAllCached: async () => [], }); + const additionalData = mock(); + (WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData); + const cipher = mockInstance(Cipher); const credentialsHelper = mockInstance(CredentialsHelper); const credentialsRepository = mockInstance(CredentialsRepository); @@ -106,6 +113,14 @@ describe('OAuth1CredentialController', () => { }), ); expect(cipher.encrypt).toHaveBeenCalledWith({ csrfSecret }); + expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( + additionalData, + credential, + credential.type, + 'internal', + undefined, + false, + ); }); }); @@ -235,6 +250,14 @@ describe('OAuth1CredentialController', () => { }), ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); + expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( + additionalData, + credential, + credential.type, + 'internal', + undefined, + true, + ); }); }); }); 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 1984d12f59f72..d075dcd19859c 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 @@ -4,6 +4,7 @@ import { type Response } from 'express'; import { mock } from 'jest-mock-extended'; import { Cipher } from 'n8n-core'; import { Logger } from 'n8n-core'; +import type { IWorkflowExecuteAdditionalData } from 'n8n-workflow'; import nock from 'nock'; import { CREDENTIAL_BLANKING_VALUE, Time } from '@/constants'; @@ -19,14 +20,20 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { ExternalHooks } from '@/external-hooks'; import type { OAuthRequest } from '@/requests'; import { SecretsHelper } from '@/secrets-helpers.ee'; +import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; import { mockInstance } from '@test/mocking'; +jest.mock('@/workflow-execute-additional-data'); + describe('OAuth2CredentialController', () => { mockInstance(Logger); mockInstance(SecretsHelper); mockInstance(VariablesService, { getAllCached: async () => [], }); + const additionalData = mock(); + (WorkflowExecuteAdditionalData.getBase as jest.Mock).mockReturnValue(additionalData); + const cipher = mockInstance(Cipher); const externalHooks = mockInstance(ExternalHooks); const credentialsHelper = mockInstance(CredentialsHelper); @@ -108,6 +115,14 @@ describe('OAuth2CredentialController', () => { type: 'oAuth2Api', }), ); + expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( + additionalData, + credential, + credential.type, + 'internal', + undefined, + false, + ); }); }); @@ -256,6 +271,14 @@ describe('OAuth2CredentialController', () => { }), ); expect(res.render).toHaveBeenCalledWith('oauth-callback'); + expect(credentialsHelper.getDecrypted).toHaveBeenCalledWith( + additionalData, + credential, + credential.type, + 'internal', + undefined, + true, + ); }); it('merges oauthTokenData if it already exists', async () => { diff --git a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts index bac924a0239f9..5098cec28d3c7 100644 --- a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts +++ b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts @@ -86,9 +86,30 @@ export abstract class AbstractOAuthController { return await WorkflowExecuteAdditionalData.getBase(); } - protected async getDecryptedData( + /** + * Allow decrypted data to evaluate expressions that include $secrets and apply overwrites + */ + protected async getDecryptedDataForAuthUri( credential: ICredentialsDb, additionalData: IWorkflowExecuteAdditionalData, + ) { + return await this.getDecryptedData(credential, additionalData, false); + } + + /** + * Do not apply overwrites here because that removes the CSRF state, and breaks the oauth flow + */ + protected async getDecryptedDataForCallback( + credential: ICredentialsDb, + additionalData: IWorkflowExecuteAdditionalData, + ) { + return await this.getDecryptedData(credential, additionalData, true); + } + + private async getDecryptedData( + credential: ICredentialsDb, + additionalData: IWorkflowExecuteAdditionalData, + raw: boolean, ) { return await this.credentialsHelper.getDecrypted( additionalData, @@ -96,7 +117,7 @@ export abstract class AbstractOAuthController { credential.type, 'internal', undefined, - true, + raw, ); } @@ -183,7 +204,10 @@ export abstract class AbstractOAuthController { } const additionalData = await this.getAdditionalData(); - const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); + const decryptedDataOriginal = await this.getDecryptedDataForCallback( + credential, + additionalData, + ); const oauthCredentials = this.applyDefaultsAndOverwrites( credential, decryptedDataOriginal, diff --git a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts index 0211c463a9d2a..fe9d22edd8faa 100644 --- a/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth1-credential.controller.ts @@ -35,7 +35,7 @@ export class OAuth1CredentialController extends AbstractOAuthController { async getAuthUri(req: OAuthRequest.OAuth1Credential.Auth): Promise { const credential = await this.getCredential(req); const additionalData = await this.getAdditionalData(); - const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); + const decryptedDataOriginal = await this.getDecryptedDataForAuthUri(credential, additionalData); const oauthCredentials = this.applyDefaultsAndOverwrites( credential, decryptedDataOriginal, diff --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index e188670fded11..b9ed0d11268ad 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -23,7 +23,7 @@ export class OAuth2CredentialController extends AbstractOAuthController { async getAuthUri(req: OAuthRequest.OAuth2Credential.Auth): Promise { const credential = await this.getCredential(req); const additionalData = await this.getAdditionalData(); - const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData); + const decryptedDataOriginal = await this.getDecryptedDataForAuthUri(credential, additionalData); // At some point in the past we saved hidden scopes to credentials (but shouldn't) // Delete scope before applying defaults to make sure new scopes are present on reconnect