From 95c99d93ad4ea776c652b336dd77fa239dfac143 Mon Sep 17 00:00:00 2001 From: Marc Littlemore Date: Thu, 6 Feb 2025 17:18:19 +0000 Subject: [PATCH 1/2] fix: allow in credentials --- .../cli/src/controllers/oauth/abstract-oauth.controller.ts | 3 ++- .../src/controllers/oauth/oauth2-credential.controller.ts | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts index bac924a0239f9..8498e6429d0f2 100644 --- a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts +++ b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts @@ -89,6 +89,7 @@ export abstract class AbstractOAuthController { protected async getDecryptedData( credential: ICredentialsDb, additionalData: IWorkflowExecuteAdditionalData, + raw = true, ) { return await this.credentialsHelper.getDecrypted( additionalData, @@ -96,7 +97,7 @@ export abstract class AbstractOAuthController { credential.type, 'internal', undefined, - true, + raw, ); } diff --git a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts index e188670fded11..93814f5c280ad 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -23,7 +23,10 @@ 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); + + // Allow decrypted data to evaluate expressions that include $secrets and apply overwrites + // This should only be done for the intial request to the auth URL which passes the client secret + const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData, false); // 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 From bf238c5e9f5c7c4fc84696e93e9bcd6dcc8d4dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 7 Feb 2025 17:08:18 +0100 Subject: [PATCH 2/2] make oauth1 consistent with oauth2. add tests --- .../oauth1-credential.controller.test.ts | 23 +++++++++++++++ .../oauth2-credential.controller.test.ts | 23 +++++++++++++++ .../oauth/abstract-oauth.controller.ts | 29 +++++++++++++++++-- .../oauth/oauth1-credential.controller.ts | 2 +- .../oauth/oauth2-credential.controller.ts | 5 +--- 5 files changed, 74 insertions(+), 8 deletions(-) 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 8498e6429d0f2..5098cec28d3c7 100644 --- a/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts +++ b/packages/cli/src/controllers/oauth/abstract-oauth.controller.ts @@ -86,10 +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, - raw = true, + ) { + 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, @@ -184,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 93814f5c280ad..b9ed0d11268ad 100644 --- a/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts +++ b/packages/cli/src/controllers/oauth/oauth2-credential.controller.ts @@ -23,10 +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(); - - // Allow decrypted data to evaluate expressions that include $secrets and apply overwrites - // This should only be done for the intial request to the auth URL which passes the client secret - const decryptedDataOriginal = await this.getDecryptedData(credential, additionalData, false); + 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