Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Webhook Node): Binary data handling #7804

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions packages/cli/src/WebhookHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,14 +304,20 @@ export async function executeWebhook(
additionalData.httpRequest = req;
additionalData.httpResponse = res;

const binaryData = workflow.expression.getSimpleParameterValue(
workflowStartNode,
'={{$parameter["options"]["binaryData"]}}',
executionMode,
additionalKeys,
undefined,
false,
);
let binaryData;

const nodeVersion = workflowStartNode.typeVersion;
if (nodeVersion === 1) {
// binaryData option is removed in versions higher than 1
binaryData = workflow.expression.getSimpleParameterValue(
workflowStartNode,
'={{$parameter["options"]["binaryData"]}}',
executionMode,
additionalKeys,
undefined,
false,
);
}

let didSendResponse = false;
let runExecutionDataMerge = {};
Expand All @@ -321,6 +327,7 @@ export async function executeWebhook(
let webhookResultData: IWebhookResponseData;

// if `Webhook` or `Wait` node, and binaryData is enabled, skip pre-parse the request-body
// always falsy for versions higher than 1
if (!binaryData) {
const { contentType, encoding } = req;
if (contentType === 'multipart/form-data') {
Expand All @@ -337,7 +344,19 @@ export async function executeWebhook(
});
});
} else {
await parseBody(req);
if (nodeVersion > 1) {
if (
contentType?.startsWith('application/json') ||
contentType?.startsWith('text/plain') ||
contentType?.startsWith('application/x-www-form-urlencoded') ||
contentType?.endsWith('/xml') ||
contentType?.endsWith('+xml')
) {
await parseBody(req);
}
} else {
await parseBody(req);
}
}
}

Expand Down
25 changes: 22 additions & 3 deletions packages/nodes-base/nodes/Webhook/Webhook.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class Webhook extends Node {
icon: 'file:webhook.svg',
name: 'webhook',
group: ['trigger'],
version: 1,
version: [1, 1.1],
description: 'Starts the workflow when a webhook is called',
eventTriggerDescription: 'Waiting for you to call the Test URL',
activationMessage: 'You can now make calls to your production webhook URL.',
Expand Down Expand Up @@ -125,6 +125,17 @@ export class Webhook extends Node {
return this.handleFormData(context);
}

const nodeVersion = context.getNode().typeVersion;
if (nodeVersion > 1 && !req.body && !options.rawBody) {
try {
return await this.handleBinaryData(context);
} catch (error) {}
}
Comment on lines +129 to +133
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means that now every single GET webhook gets a .binary object set, even though there is not binary data present.
This causes errors here.
We need to

  1. improve the checks here to create .binary if there actually any data (or maybe just check if the method is POST, PATCH, or `PUT).
  2. update the workflowExecuteAfter hook to make sure that restoreBinaryDataId does not fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improve the checks here to create .binary if there actually any data

it's checked in handleBinaryData, agree that we could add check for httpMethod type as generally only those methods would have binaries

			const binaryPropertyName = (options.binaryPropertyName || 'data') as string;
			const fileName = req.contentDisposition?.filename ?? uuid();
			const binaryData = await context.nodeHelpers.copyBinaryFile(
				binaryFile.path,
				fileName,
				req.contentType ?? 'application/octet-stream',
			);

			if (!binaryData.data) {
				return { workflowData: [[returnItem]] };
			}

			returnItem.binary![binaryPropertyName] = binaryData;


if (options.rawBody && !req.rawBody) {
await req.readRawBody();
}

const response: INodeExecutionData = {
json: {
headers: req.headers,
Expand All @@ -135,7 +146,7 @@ export class Webhook extends Node {
binary: options.rawBody
? {
data: {
data: req.rawBody.toString(BINARY_ENCODING),
data: (req.rawBody ?? '').toString(BINARY_ENCODING),
mimeType: req.contentType ?? 'application/json',
},
}
Expand Down Expand Up @@ -215,6 +226,7 @@ export class Webhook extends Node {
};

let count = 0;

for (const key of Object.keys(files)) {
const processFiles: MultiPartFormData.File[] = [];
let multiFile = false;
Expand Down Expand Up @@ -247,6 +259,7 @@ export class Webhook extends Node {
count += 1;
}
}

return { workflowData: [[returnItem]] };
}

Expand All @@ -272,12 +285,18 @@ export class Webhook extends Node {

const binaryPropertyName = (options.binaryPropertyName || 'data') as string;
const fileName = req.contentDisposition?.filename ?? uuid();
returnItem.binary![binaryPropertyName] = await context.nodeHelpers.copyBinaryFile(
const binaryData = await context.nodeHelpers.copyBinaryFile(
binaryFile.path,
fileName,
req.contentType ?? 'application/octet-stream',
);

if (!binaryData.data) {
return { workflowData: [[returnItem]] };
}

returnItem.binary![binaryPropertyName] = binaryData;

return { workflowData: [[returnItem]] };
} catch (error) {
throw new NodeOperationError(context.getNode(), error as Error);
Expand Down
32 changes: 31 additions & 1 deletion packages/nodes-base/nodes/Webhook/description.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ export const optionsProperty: INodeProperties = {
displayOptions: {
show: {
'/httpMethod': ['PATCH', 'PUT', 'POST'],
'@version': [1],
},
},
default: false,
Expand All @@ -212,15 +213,28 @@ export const optionsProperty: INodeProperties = {
name: 'binaryPropertyName',
type: 'string',
default: 'data',
required: true,
displayOptions: {
show: {
binaryData: [true],
'@version': [1],
},
},
description:
'Name of the binary property to write the data of the received file to. If the data gets received via "Form-Data Multipart" it will be the prefix and a number starting with 0 will be attached to it.',
},
{
displayName: 'Binary Property',
name: 'binaryPropertyName',
type: 'string',
default: 'data',
displayOptions: {
hide: {
'@version': [1],
},
},
description:
'Name of the binary property to write the data of the received file to, only relevant if binary data is received',
},
{
displayName: 'Ignore Bots',
name: 'ignoreBots',
Expand Down Expand Up @@ -248,6 +262,9 @@ export const optionsProperty: INodeProperties = {
name: 'rawBody',
type: 'boolean',
displayOptions: {
show: {
'@version': [1],
},
hide: {
binaryData: [true],
noResponseBody: [true],
Expand All @@ -257,6 +274,19 @@ export const optionsProperty: INodeProperties = {
// eslint-disable-next-line n8n-nodes-base/node-param-description-boolean-without-whether
description: 'Raw body (binary)',
},
{
displayName: 'Raw Body',
name: 'rawBody',
type: 'boolean',
displayOptions: {
hide: {
noResponseBody: [true],
'@version': [1],
},
},
default: false,
description: 'Whether to return the raw body',
},
{
displayName: 'Response Data',
name: 'responseData',
Expand Down
Loading