Skip to content

Commit c9abeff

Browse files
elsmrelsmr
authored
fix: Harden form and trigger response handling (#23061)
1 parent e77037c commit c9abeff

File tree

16 files changed

+404
-27
lines changed

16 files changed

+404
-27
lines changed

‎packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/templates.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ export function createPage({
156156
}
157157
},
158158
allowFileUploads: ${sanitizedAllowFileUploads},
159-
allowedFilesMimeTypes: '${sanitizedAllowedFilesMimeTypes}',
159+
allowedFilesMimeTypes: ${JSON.stringify(sanitizedAllowedFilesMimeTypes)},
160160
i18n: {
161161
${Object.keys(sanitizedI18nConfig).length ? `en: ${JSON.stringify(sanitizedI18nConfig)},` : ''}
162162
},

‎packages/cli/src/webhooks/__tests__/waiting-forms.test.ts‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { IExecutionResponse, ExecutionRepository } from '@n8n/db';
22
import type express from 'express';
33
import { mock } from 'jest-mock-extended';
4+
import { getWebhookSandboxCSP } from 'n8n-core';
45
import { FORM_NODE_TYPE, WAITING_FORMS_EXECUTION_STATUS, type Workflow } from 'n8n-workflow';
56

67
import type { WaitingWebhookRequest } from '../webhook.types';
@@ -315,4 +316,115 @@ describe('WaitingForms', () => {
315316
expect(workflow.active).toBe(false);
316317
});
317318
});
319+
320+
describe('executeWebhook - default completion page', () => {
321+
it('should set CSP header when rendering default completion page', async () => {
322+
const execution = mock<IExecutionResponse>({
323+
finished: true,
324+
status: 'success',
325+
data: {
326+
resultData: {
327+
lastNodeExecuted: 'LastNode',
328+
runData: {},
329+
error: undefined, // Must be explicitly set to undefined; jest-mock-extended returns a truthy mock if omitted
330+
},
331+
},
332+
workflowData: {
333+
id: 'workflow1',
334+
name: 'Test Workflow',
335+
nodes: [
336+
{
337+
name: 'LastNode',
338+
type: 'other-node-type',
339+
typeVersion: 1,
340+
position: [0, 0],
341+
parameters: {},
342+
},
343+
],
344+
connections: {},
345+
active: false,
346+
activeVersionId: undefined,
347+
settings: {},
348+
staticData: {},
349+
isArchived: false,
350+
createdAt: new Date(),
351+
updatedAt: new Date(),
352+
},
353+
});
354+
executionRepository.findSingleExecution.mockResolvedValue(execution);
355+
356+
const req = mock<WaitingWebhookRequest>({
357+
headers: {},
358+
params: {
359+
path: '123',
360+
suffix: undefined,
361+
},
362+
});
363+
364+
const res = mock<express.Response>();
365+
366+
const result = await waitingForms.executeWebhook(req, res);
367+
368+
expect(res.setHeader).toHaveBeenCalledWith('Content-Security-Policy', getWebhookSandboxCSP());
369+
expect(res.render).toHaveBeenCalledWith('form-trigger-completion', {
370+
title: 'Form Submitted',
371+
message: 'Your response has been recorded',
372+
formTitle: 'Form Submitted',
373+
});
374+
expect(result).toEqual({ noWebhookResponse: true });
375+
});
376+
377+
it('should include sandbox directive in CSP header for security', async () => {
378+
const execution = mock<IExecutionResponse>({
379+
finished: true,
380+
status: 'success',
381+
data: {
382+
resultData: {
383+
lastNodeExecuted: 'LastNode',
384+
runData: {},
385+
error: undefined, // Must be explicitly set to undefined; jest-mock-extended returns a truthy mock if omitted
386+
},
387+
},
388+
workflowData: {
389+
id: 'workflow1',
390+
name: 'Test Workflow',
391+
nodes: [
392+
{
393+
name: 'LastNode',
394+
type: 'other-node-type',
395+
typeVersion: 1,
396+
position: [0, 0],
397+
parameters: {},
398+
},
399+
],
400+
connections: {},
401+
active: false,
402+
activeVersionId: undefined,
403+
settings: {},
404+
staticData: {},
405+
isArchived: false,
406+
createdAt: new Date(),
407+
updatedAt: new Date(),
408+
},
409+
});
410+
executionRepository.findSingleExecution.mockResolvedValue(execution);
411+
412+
const req = mock<WaitingWebhookRequest>({
413+
headers: {},
414+
params: {
415+
path: '123',
416+
suffix: undefined,
417+
},
418+
});
419+
420+
const res = mock<express.Response>();
421+
422+
await waitingForms.executeWebhook(req, res);
423+
424+
expect(res.setHeader).toHaveBeenCalledWith(
425+
'Content-Security-Policy',
426+
expect.stringContaining('sandbox'),
427+
);
428+
});
429+
});
318430
});

‎packages/cli/src/webhooks/__tests__/webhook-helpers.test.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,50 @@ describe('handleFormRedirectionCase', () => {
155155
const result = handleFormRedirectionCase(data, workflowStartNode);
156156
expect(result).toEqual(data);
157157
});
158+
159+
test('should block javascript: URLs for security', () => {
160+
const data: IWebhookResponseCallbackData = {
161+
responseCode: 302,
162+
headers: { location: 'javascript:alert(document.domain)' },
163+
};
164+
const workflowStartNode = mock<INode>({
165+
type: FORM_NODE_TYPE,
166+
parameters: {},
167+
});
168+
const result = handleFormRedirectionCase(data, workflowStartNode);
169+
expect(result.responseCode).toBe(200);
170+
expect(result.data).toBeUndefined();
171+
expect((result?.headers as IDataObject)?.location).toBeUndefined();
172+
});
173+
174+
test('should block data: URLs for security', () => {
175+
const data: IWebhookResponseCallbackData = {
176+
responseCode: 302,
177+
headers: { location: 'data:text/html,<script>alert(1)</script>' },
178+
};
179+
const workflowStartNode = mock<INode>({
180+
type: FORM_NODE_TYPE,
181+
parameters: {},
182+
});
183+
const result = handleFormRedirectionCase(data, workflowStartNode);
184+
expect(result.responseCode).toBe(200);
185+
expect(result.data).toBeUndefined();
186+
expect((result?.headers as IDataObject)?.location).toBeUndefined();
187+
});
188+
189+
test('should allow https: URLs', () => {
190+
const data: IWebhookResponseCallbackData = {
191+
responseCode: 302,
192+
headers: { location: 'https://example.com/callback' },
193+
};
194+
const workflowStartNode = mock<INode>({
195+
type: FORM_NODE_TYPE,
196+
parameters: {},
197+
});
198+
const result = handleFormRedirectionCase(data, workflowStartNode);
199+
expect(result.responseCode).toBe(200);
200+
expect(result.data).toEqual({ redirectURL: 'https://example.com/callback' });
201+
});
158202
});
159203

160204
describe('setupResponseNodePromise', () => {

‎packages/cli/src/webhooks/waiting-forms.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { IExecutionResponse } from '@n8n/db';
22
import { Service } from '@n8n/di';
33
import type express from 'express';
44
import type { IRunData } from 'n8n-workflow';
5+
import { getWebhookSandboxCSP } from 'n8n-core';
56
import {
67
FORM_NODE_TYPE,
78
WAIT_NODE_TYPE,
@@ -127,6 +128,7 @@ export class WaitingForms extends WaitingWebhooks {
127128
);
128129

129130
if (!completionPage) {
131+
res.setHeader('Content-Security-Policy', getWebhookSandboxCSP());
130132
res.render('form-trigger-completion', {
131133
title: 'Form Submitted',
132134
message: 'Your response has been recorded',

‎packages/cli/src/webhooks/webhook-helpers.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import {
4141
FORM_TRIGGER_NODE_TYPE,
4242
NodeOperationError,
4343
OperationalError,
44+
tryToParseUrl,
4445
UnexpectedError,
4546
WAIT_NODE_TYPE,
4647
WorkflowConfigurationError,
@@ -234,10 +235,20 @@ export const handleFormRedirectionCase = (
234235
(data?.headers as IDataObject)?.location &&
235236
String(data?.responseCode).startsWith('3')
236237
) {
238+
const locationUrl = String((data?.headers as IDataObject)?.location);
239+
let validatedUrl: string | undefined;
240+
try {
241+
validatedUrl = tryToParseUrl(locationUrl);
242+
} catch {
243+
// Invalid URL, don't redirect
244+
}
245+
237246
data.responseCode = 200;
238-
data.data = {
239-
redirectURL: (data?.headers as IDataObject)?.location,
240-
};
247+
if (validatedUrl) {
248+
data.data = {
249+
redirectURL: validatedUrl,
250+
};
251+
}
241252
(data.headers as IDataObject).location = undefined;
242253
}
243254

‎packages/cli/templates/form-trigger-completion.handlebars‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,10 +230,7 @@
230230
} catch (e) {}
231231
232232
if (json?.redirectURL) {
233-
const url = json.redirectURL.includes("://")
234-
? json.redirectURL
235-
: "https://" + json.redirectURL;
236-
window.location.replace(url);
233+
window.location.replace(json.redirectURL);
237234
}
238235
})
239236
.catch(function (error) {

‎packages/cli/templates/form-trigger.handlebars‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,8 +1016,7 @@
10161016
}
10171017
10181018
if (json?.redirectURL) {
1019-
const url = json.redirectURL.includes("://") ? json.redirectURL : "https://" + json.redirectURL;
1020-
window.location.replace(url);
1019+
window.location.replace(json.redirectURL);
10211020
return;
10221021
}
10231022

‎packages/nodes-base/nodes/Form/test/Form.node.test.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ describe('Form Node', () => {
9292
it('should render form for GET request', async () => {
9393
const mockResponseObject = {
9494
render: jest.fn(),
95+
setHeader: jest.fn(),
9596
};
9697
mockWebhookFunctions.getResponseObject.mockReturnValue(
9798
mockResponseObject as unknown as Response,
@@ -232,10 +233,11 @@ describe('Form Node', () => {
232233
appendAttribution: 'test',
233234
formTitle: 'test',
234235
message: 'Test Message',
235-
redirectUrl: '',
236+
redirectUrl: undefined,
236237
title: 'Test Title',
237238
responseBinary: encodeURIComponent(JSON.stringify('')),
238239
responseText: '',
240+
dangerousCustomCss: undefined,
239241
},
240242
},
241243
{
@@ -246,10 +248,11 @@ describe('Form Node', () => {
246248
appendAttribution: 'test',
247249
formTitle: 'test',
248250
message: 'Test Message',
249-
redirectUrl: '',
251+
redirectUrl: undefined,
250252
title: 'Test Title',
251253
responseText: '<div>hey</div><script>alert("hi")</script>',
252254
responseBinary: encodeURIComponent(JSON.stringify('')),
255+
dangerousCustomCss: undefined,
253256
},
254257
},
255258
{
@@ -260,10 +263,11 @@ describe('Form Node', () => {
260263
appendAttribution: 'test',
261264
formTitle: 'test',
262265
message: 'Test Message',
263-
redirectUrl: '',
266+
redirectUrl: undefined,
264267
responseBinary: encodeURIComponent(JSON.stringify('')),
265268
title: 'Test Title',
266269
responseText: 'my text over here',
270+
dangerousCustomCss: undefined,
267271
},
268272
},
269273
];
@@ -315,6 +319,7 @@ describe('Form Node', () => {
315319
it('should pass customCss to form template', async () => {
316320
const mockResponseObject = {
317321
render: jest.fn(),
322+
setHeader: jest.fn(),
318323
};
319324
mockWebhookFunctions.getResponseObject.mockReturnValue(
320325
mockResponseObject as unknown as Response,
@@ -360,6 +365,8 @@ describe('Form Node', () => {
360365
if (paramName === 'respondWith') return 'text';
361366
if (paramName === 'completionTitle') return 'Completion Title';
362367
if (paramName === 'completionMessage') return 'Completion Message';
368+
if (paramName === 'redirectUrl') return '';
369+
if (paramName === 'responseText') return '';
363370
if (paramName === 'options')
364371
return {
365372
customCss: '.completion-container { color: blue; }',

0 commit comments

Comments
 (0)