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.

Sign up for GitHub

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

Jump to bottom

fix(core): Webhooks responding with binary data should not prematurely end the response stream #9063

Merged
merged 1 commit into from
May 7, 2024
Merged

fix(core): Webhooks responding with binary data should not prematurely end the response stream #9063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 14 additions & 17 deletions packages/cli/src/WebhookHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import type express from 'express';
import { Container } from 'typedi';
import get from 'lodash/get';
import { pipeline } from 'stream/promises';
import { finished } from 'stream/promises';
import formidable from 'formidable';

import { BinaryDataService, NodeExecuteFunctions } from 'n8n-core';
Expand All @@ -30,6 +30,7 @@ import type {
IWebhookResponseData,
IWorkflowDataProxyAdditionalKeys,
IWorkflowExecuteAdditionalData,
WebhookResponseMode,
Workflow,
WorkflowExecuteMode,
} from 'n8n-workflow';
Expand Down Expand Up @@ -272,7 +273,7 @@ export async function executeWebhook(
additionalKeys,
undefined,
'onReceived',
);
) as WebhookResponseMode;
const responseCode = workflow.expression.getSimpleParameterValue(
workflowStartNode,
webhookData.webhookDescription.responseCode as string,
Expand All @@ -291,7 +292,7 @@ export async function executeWebhook(
'firstEntryJson',
);

if (!['onReceived', 'lastNode', 'responseNode'].includes(responseMode as string)) {
if (!['onReceived', 'lastNode', 'responseNode'].includes(responseMode)) {
// If the mode is not known we error. Is probably best like that instead of using
// the default that people know as early as possible (probably already testing phase)
// that something does not resolve properly.
Expand Down Expand Up @@ -562,7 +563,8 @@ export async function executeWebhook(
if (binaryData?.id) {
res.header(response.headers);
const stream = await Container.get(BinaryDataService).getAsStream(binaryData.id);
await pipeline(stream, res);
stream.pipe(res, { end: false });
await finished(stream);
responseCallback(null, { noWebhookResponse: true });
} else if (Buffer.isBuffer(response.body)) {
res.header(response.headers);
Expand Down Expand Up @@ -595,6 +597,7 @@ export async function executeWebhook(
});
}

process.nextTick(() => res.end());
didSendResponse = true;
})
.catch(async (error) => {
Expand Down Expand Up @@ -659,17 +662,9 @@ export async function executeWebhook(
return data;
}

if (responseMode === 'responseNode') {
if (!didSendResponse) {
// Return an error if no Webhook-Response node did send any data
responseCallback(null, {
data: {
message: 'Workflow executed successfully',
},
responseCode,
});
didSendResponse = true;
}
// in `responseNode` mode `responseCallback` is called by `responsePromise`
if (responseMode === 'responseNode' && responsePromise) {
await Promise.allSettled([responsePromise.promise()]);
return undefined;
}

Expand Down Expand Up @@ -795,14 +790,16 @@ export async function executeWebhook(
res.setHeader('Content-Type', binaryData.mimeType);
if (binaryData.id) {
const stream = await Container.get(BinaryDataService).getAsStream(binaryData.id);
await pipeline(stream, res);
stream.pipe(res, { end: false });
await finished(stream);
} else {
res.end(Buffer.from(binaryData.data, BINARY_ENCODING));
res.write(Buffer.from(binaryData.data, BINARY_ENCODING));
}

responseCallback(null, {
noWebhookResponse: true,
});
process.nextTick(() => res.end());
}
} else if (responseData === 'noData') {
// Return without data
Expand Down
2 changes: 1 addition & 1 deletion packages/workflow/src/Interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1834,7 +1834,7 @@ export interface IWebhookResponseData {
}

export type WebhookResponseData = 'allEntries' | 'firstEntryJson' | 'firstEntryBinary' | 'noData';
export type WebhookResponseMode = 'onReceived' | 'lastNode';
export type WebhookResponseMode = 'onReceived' | 'lastNode' | 'responseNode';

export interface INodeTypes {
getByName(nodeType: string): INodeType | IVersionedNodeType;
Expand Down
Loading