From 84f091d3e5f9c661e373acd0c058ee158965b6e8 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Wed, 5 Jun 2024 09:25:39 +0200 Subject: [PATCH] fix(HTTP Request Node): Sanitize secrets of predefined credentials (#9612) --- packages/core/src/NodeExecuteFunctions.ts | 3 +- .../nodes/HttpRequest/GenericFunctions.ts | 62 +++++- .../HttpRequest/V3/HttpRequestV3.node.ts | 46 ++++- .../HttpRequest/test/utils/utils.test.ts | 179 ++++++++++++------ packages/workflow/src/Interfaces.ts | 3 + 5 files changed, 222 insertions(+), 71 deletions(-) diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 08a45f27f1..efa160567d 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -2844,7 +2844,8 @@ const getCommonWorkflowFunctions = ( getInstanceBaseUrl: () => additionalData.instanceBaseUrl, getInstanceId: () => Container.get(InstanceSettings).instanceId, getTimezone: () => getTimezone(workflow), - + getCredentialsProperties: (type: string) => + additionalData.credentialsHelper.getCredentialsProperties(type), prepareOutputData: async (outputData) => [outputData], }); diff --git a/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts b/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts index 54a37a5953..235389a379 100644 --- a/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts +++ b/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts @@ -1,16 +1,20 @@ import type { SecureContextOptions } from 'tls'; import type { + ICredentialDataDecryptedObject, IDataObject, INodeExecutionData, + INodeProperties, IOAuth2Options, IRequestOptions, } from 'n8n-workflow'; import set from 'lodash/set'; +import isPlainObject from 'lodash/isPlainObject'; import FormData from 'form-data'; -import type { HttpSslAuthCredentials } from './interfaces'; import { formatPrivateKey } from '../../utils/utilities'; +import type { HttpSslAuthCredentials } from './interfaces'; +import get from 'lodash/get'; export type BodyParameter = { name: string; @@ -29,7 +33,33 @@ export const replaceNullValues = (item: INodeExecutionData) => { return item; }; -export function sanitizeUiMessage(request: IRequestOptions, authDataKeys: IAuthDataSanitizeKeys) { +export const REDACTED = '**hidden**'; + +function isObject(obj: unknown): obj is IDataObject { + return isPlainObject(obj); +} + +function redact(obj: T, secrets: string[]): T { + if (typeof obj === 'string') { + return secrets.reduce((safe, secret) => safe.replace(secret, REDACTED), obj) as T; + } + + if (Array.isArray(obj)) { + return obj.map((item) => redact(item, secrets)) as T; + } else if (isObject(obj)) { + for (const [key, value] of Object.entries(obj)) { + (obj as IDataObject)[key] = redact(value, secrets); + } + } + + return obj; +} + +export function sanitizeUiMessage( + request: IRequestOptions, + authDataKeys: IAuthDataSanitizeKeys, + secrets?: string[], +) { let sendRequest = request as unknown as IDataObject; // Protect browser from sending large binary data @@ -38,7 +68,7 @@ export function sanitizeUiMessage(request: IRequestOptions, authDataKeys: IAuthD ...request, body: `Binary data got replaced with this text. Original was a Buffer with a size of ${ (request.body as string).length - } byte.`, + } bytes.`, }; } @@ -50,7 +80,7 @@ export function sanitizeUiMessage(request: IRequestOptions, authDataKeys: IAuthD // eslint-disable-next-line @typescript-eslint/no-loop-func (acc: IDataObject, curr) => { acc[curr] = authDataKeys[requestProperty].includes(curr) - ? '** hidden **' + ? REDACTED : (sendRequest[requestProperty] as IDataObject)[curr]; return acc; }, @@ -59,9 +89,33 @@ export function sanitizeUiMessage(request: IRequestOptions, authDataKeys: IAuthD }; } + if (secrets && secrets.length > 0) { + return redact(sendRequest, secrets); + } + return sendRequest; } +export function getSecrets( + properties: INodeProperties[], + credentials: ICredentialDataDecryptedObject, +): string[] { + const sensitivePropNames = new Set( + properties.filter((prop) => prop.typeOptions?.password).map((prop) => prop.name), + ); + + const secrets = Object.entries(credentials) + .filter(([propName]) => sensitivePropNames.has(propName)) + .map(([_, value]) => value) + .filter((value): value is string => typeof value === 'string'); + const oauthAccessToken = get(credentials, 'oauthTokenData.access_token'); + if (typeof oauthAccessToken === 'string') { + secrets.push(oauthAccessToken); + } + + return secrets; +} + export const getOAuth2AdditionalParameters = (nodeCredentialType: string) => { const oAuth2Options: { [credentialType: string]: IOAuth2Options } = { bitlyOAuth2Api: { diff --git a/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts b/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts index 386721c3b1..d3a18dd6fd 100644 --- a/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts +++ b/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts @@ -29,14 +29,15 @@ import type { BodyParameter, IAuthDataSanitizeKeys } from '../GenericFunctions'; import { binaryContentTypes, getOAuth2AdditionalParameters, + getSecrets, prepareRequestBody, reduceAsync, replaceNullValues, sanitizeUiMessage, setAgentOptions, } from '../GenericFunctions'; -import { keysToLowercase } from '@utils/utilities'; import type { HttpSslAuthCredentials } from '../interfaces'; +import { keysToLowercase } from '@utils/utilities'; function toText(data: T) { if (typeof data === 'object' && data !== null) { @@ -1299,7 +1300,12 @@ export class HttpRequestV3 implements INodeType { requestInterval: number; }; - const sanitazedRequests: IDataObject[] = []; + const requests: Array<{ + options: IRequestOptions; + authKeys: IAuthDataSanitizeKeys; + credentialType?: string; + }> = []; + for (let itemIndex = 0; itemIndex < items.length; itemIndex++) { if (authentication === 'genericCredentialType') { genericCredentialType = this.getNodeParameter('genericAuthType', 0) as string; @@ -1696,11 +1702,11 @@ export class HttpRequestV3 implements INodeType { } } - try { - const sanitazedRequestOptions = sanitizeUiMessage(requestOptions, authDataKeys); - this.sendMessageToUI(sanitazedRequestOptions); - sanitazedRequests.push(sanitazedRequestOptions); - } catch (e) {} + requests.push({ + options: requestOptions, + authKeys: authDataKeys, + credentialType: nodeCredentialType, + }); if (pagination && pagination.paginationMode !== 'off') { let continueExpression = '={{false}}'; @@ -1827,7 +1833,29 @@ export class HttpRequestV3 implements INodeType { requestPromises.push(requestWithAuthentication); } } - const promisesResponses = await Promise.allSettled(requestPromises); + + const sanitizedRequests: IDataObject[] = []; + const promisesResponses = await Promise.allSettled( + requestPromises.map( + async (requestPromise, itemIndex) => + await requestPromise.finally(async () => { + try { + // Secrets need to be read after the request because secrets could have changed + // For example: OAuth token refresh, preAuthentication + const { options, authKeys, credentialType } = requests[itemIndex]; + let secrets: string[] = []; + if (credentialType) { + const properties = this.getCredentialsProperties(credentialType); + const credentials = await this.getCredentials(credentialType, itemIndex); + secrets = getSecrets(properties, credentials); + } + const sanitizedRequestOptions = sanitizeUiMessage(options, authKeys, secrets); + sanitizedRequests.push(sanitizedRequestOptions); + this.sendMessageToUI(sanitizedRequestOptions); + } catch (e) {} + }), + ), + ); let responseData: any; for (let itemIndex = 0; itemIndex < items.length; itemIndex++) { @@ -1842,7 +1870,7 @@ export class HttpRequestV3 implements INodeType { responseData.reason.error = Buffer.from(responseData.reason.error as Buffer).toString(); } const error = new NodeApiError(this.getNode(), responseData as JsonObject, { itemIndex }); - set(error, 'context.request', sanitazedRequests[itemIndex]); + set(error, 'context.request', sanitizedRequests[itemIndex]); throw error; } else { removeCircularRefs(responseData.reason as JsonObject); diff --git a/packages/nodes-base/nodes/HttpRequest/test/utils/utils.test.ts b/packages/nodes-base/nodes/HttpRequest/test/utils/utils.test.ts index 716b59e9b8..b3e9d5fbd7 100644 --- a/packages/nodes-base/nodes/HttpRequest/test/utils/utils.test.ts +++ b/packages/nodes-base/nodes/HttpRequest/test/utils/utils.test.ts @@ -1,75 +1,140 @@ import type { IRequestOptions } from 'n8n-workflow'; -import { prepareRequestBody, setAgentOptions } from '../../GenericFunctions'; +import { + REDACTED, + prepareRequestBody, + sanitizeUiMessage, + setAgentOptions, +} from '../../GenericFunctions'; import type { BodyParameter, BodyParametersReducer } from '../../GenericFunctions'; -describe('HTTP Node Utils, prepareRequestBody', () => { - it('should call default reducer', async () => { - const bodyParameters: BodyParameter[] = [ - { - name: 'foo.bar', - value: 'baz', - }, - ]; - const defaultReducer: BodyParametersReducer = jest.fn(); +describe('HTTP Node Utils', () => { + describe('prepareRequestBody', () => { + it('should call default reducer', async () => { + const bodyParameters: BodyParameter[] = [ + { + name: 'foo.bar', + value: 'baz', + }, + ]; + const defaultReducer: BodyParametersReducer = jest.fn(); - await prepareRequestBody(bodyParameters, 'json', 3, defaultReducer); + await prepareRequestBody(bodyParameters, 'json', 3, defaultReducer); - expect(defaultReducer).toBeCalledTimes(1); - expect(defaultReducer).toBeCalledWith({}, { name: 'foo.bar', value: 'baz' }); - }); + expect(defaultReducer).toBeCalledTimes(1); + expect(defaultReducer).toBeCalledWith({}, { name: 'foo.bar', value: 'baz' }); + }); - it('should call process dot notations', async () => { - const bodyParameters: BodyParameter[] = [ - { - name: 'foo.bar.spam', - value: 'baz', - }, - ]; - const defaultReducer: BodyParametersReducer = jest.fn(); + it('should call process dot notations', async () => { + const bodyParameters: BodyParameter[] = [ + { + name: 'foo.bar.spam', + value: 'baz', + }, + ]; + const defaultReducer: BodyParametersReducer = jest.fn(); - const result = await prepareRequestBody(bodyParameters, 'json', 4, defaultReducer); + const result = await prepareRequestBody(bodyParameters, 'json', 4, defaultReducer); - expect(defaultReducer).toBeCalledTimes(0); - expect(result).toBeDefined(); - expect(result).toEqual({ foo: { bar: { spam: 'baz' } } }); - }); -}); - -describe('HTTP Node Utils, setAgentOptions', () => { - it("should not have agentOptions as it's undefined", async () => { - const requestOptions: IRequestOptions = { - method: 'GET', - uri: 'https://example.com', - }; - - const sslCertificates = undefined; - - setAgentOptions(requestOptions, sslCertificates); - - expect(requestOptions).toEqual({ - method: 'GET', - uri: 'https://example.com', + expect(defaultReducer).toBeCalledTimes(0); + expect(result).toBeDefined(); + expect(result).toEqual({ foo: { bar: { spam: 'baz' } } }); }); }); - it('should have agentOptions set', async () => { - const requestOptions: IRequestOptions = { - method: 'GET', - uri: 'https://example.com', - }; + describe('setAgentOptions', () => { + it("should not have agentOptions as it's undefined", async () => { + const requestOptions: IRequestOptions = { + method: 'GET', + uri: 'https://example.com', + }; - const sslCertificates = { - ca: 'mock-ca', - }; + const sslCertificates = undefined; - setAgentOptions(requestOptions, sslCertificates); + setAgentOptions(requestOptions, sslCertificates); - expect(requestOptions).toStrictEqual({ - method: 'GET', - uri: 'https://example.com', - agentOptions: { + expect(requestOptions).toEqual({ + method: 'GET', + uri: 'https://example.com', + }); + }); + + it('should have agentOptions set', async () => { + const requestOptions: IRequestOptions = { + method: 'GET', + uri: 'https://example.com', + }; + + const sslCertificates = { ca: 'mock-ca', - }, + }; + + setAgentOptions(requestOptions, sslCertificates); + + expect(requestOptions).toStrictEqual({ + method: 'GET', + uri: 'https://example.com', + agentOptions: { + ca: 'mock-ca', + }, + }); + }); + }); + + describe('sanitizeUiMessage', () => { + it('should remove large Buffers', async () => { + const requestOptions: IRequestOptions = { + method: 'POST', + uri: 'https://example.com', + body: Buffer.alloc(900000), + }; + + expect(sanitizeUiMessage(requestOptions, {}).body).toEqual( + 'Binary data got replaced with this text. Original was a Buffer with a size of 900000 bytes.', + ); + }); + + it('should remove keys that contain sensitive data', async () => { + const requestOptions: IRequestOptions = { + method: 'POST', + uri: 'https://example.com', + body: { sessionToken: 'secret', other: 'foo' }, + headers: { authorization: 'secret', other: 'foo' }, + auth: { user: 'user', password: 'secret' }, + }; + + expect( + sanitizeUiMessage(requestOptions, { + headers: ['authorization'], + body: ['sessionToken'], + auth: ['password'], + }), + ).toEqual({ + body: { sessionToken: REDACTED, other: 'foo' }, + headers: { other: 'foo', authorization: REDACTED }, + auth: { user: 'user', password: REDACTED }, + method: 'POST', + uri: 'https://example.com', + }); + }); + + it('should remove secrets', async () => { + const requestOptions: IRequestOptions = { + method: 'POST', + uri: 'https://example.com', + body: { nested: { secret: 'secretAccessToken' } }, + headers: { authorization: 'secretAccessToken', other: 'foo' }, + }; + + expect(sanitizeUiMessage(requestOptions, {}, ['secretAccessToken'])).toEqual({ + body: { + nested: { + secret: REDACTED, + }, + }, + headers: { authorization: REDACTED, other: 'foo' }, + method: 'POST', + uri: 'https://example.com', + }); }); }); }); diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index 7485cdbf48..01ac6d1b58 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -216,6 +216,8 @@ export abstract class ICredentialsHelper { type: string, data: ICredentialDataDecryptedObject, ): Promise; + + abstract getCredentialsProperties(type: string): INodeProperties[]; } export interface IAuthenticateBase { @@ -813,6 +815,7 @@ export type NodeTypeAndVersion = { export interface FunctionsBase { logger: Logger; getCredentials(type: string, itemIndex?: number): Promise; + getCredentialsProperties(type: string): INodeProperties[]; getExecutionId(): string; getNode(): INode; getWorkflow(): IWorkflowMetadata;