Skip to content

Commit 08f3320

Browse files
ivovivov
authored
Merge commit from fork
* fix(core): Transform function expressions to arrow functions * Update import path due to lib upgrade * Tighten comments * Simplify * Tighten tests * More accurate naming * Reduce diff * Update lockfile
1 parent 008cd8d commit 08f3320

File tree

9 files changed

+253
-119
lines changed

9 files changed

+253
-119
lines changed

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
"sqlite3"
8989
],
9090
"overrides": {
91+
"ast-types": "0.16.1",
9192
"@azure/identity": "^4.3.0",
9293
"@lezer/common": "^1.2.0",
9394
"@mistralai/mistralai": "^1.10.0",

‎packages/workflow/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
"dependencies": {
5353
"@n8n/errors": "workspace:^",
5454
"@n8n/tournament": "1.0.6",
55-
"ast-types": "0.15.2",
55+
"ast-types": "0.16.1",
5656
"callsites": "catalog:",
5757
"esprima-next": "5.8.4",
5858
"form-data": "catalog:",

‎packages/workflow/src/expression-evaluator-proxy.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
import { Tournament } from '@n8n/tournament';
22

3-
import { DollarSignValidator, PrototypeSanitizer } from './expression-sandboxing';
3+
import {
4+
DollarSignValidator,
5+
FunctionThisSanitizer,
6+
PrototypeSanitizer,
7+
} from './expression-sandboxing';
48

59
type Evaluator = (expr: string, data: unknown) => string | null | (() => unknown);
610
type ErrorHandler = (error: Error) => void;
711

812
const errorHandler: ErrorHandler = () => {};
913
const tournamentEvaluator = new Tournament(errorHandler, undefined, undefined, {
10-
before: [],
14+
before: [FunctionThisSanitizer],
1115
after: [PrototypeSanitizer, DollarSignValidator],
1216
});
1317
const evaluator: Evaluator = tournamentEvaluator.execute.bind(tournamentEvaluator);

‎packages/workflow/src/expression-sandboxing.ts‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { type ASTAfterHook, astBuilders as b, astVisit } from '@n8n/tournament';
1+
import { type ASTAfterHook, type ASTBeforeHook, astBuilders as b, astVisit } from '@n8n/tournament';
22

33
import { ExpressionError } from './errors';
44
import { isSafeObjectProperty } from './utils';
@@ -8,6 +8,10 @@ const sanitizerIdentifier = b.identifier(sanitizerName);
88

99
export const DOLLAR_SIGN_ERROR = 'Cannot access "$" without calling it as a function';
1010

11+
const EMPTY_CONTEXT = b.objectExpression([
12+
b.property('init', b.identifier('process'), b.objectExpression([])),
13+
]);
14+
1115
/**
1216
* Helper to check if an expression is a valid property access with $ as the property.
1317
* Returns true for obj.$ or obj.nested.$ but false for bare $ or other expression contexts.
@@ -49,6 +53,69 @@ const isValidDollarPropertyAccess = (expr: unknown): boolean => {
4953
return isPropertyDollar && !isObjectDollar && isObjectValid;
5054
};
5155

56+
/**
57+
* Prevents regular functions from binding their `this` to the Node.js global.
58+
*/
59+
export const FunctionThisSanitizer: ASTBeforeHook = (ast, dataNode) => {
60+
astVisit(ast, {
61+
visitCallExpression(path) {
62+
const { node } = path;
63+
64+
if (node.callee.type !== 'FunctionExpression') {
65+
this.traverse(path);
66+
return;
67+
}
68+
69+
const fnExpression = node.callee;
70+
71+
/**
72+
* Called function expressions (IIFEs) - both anonymous and named:
73+
*
74+
* ```js
75+
* (function(x) { return x * 2; })(5)
76+
* (function factorial(n) { return n <= 1 ? 1 : n * factorial(n-1); })(5)
77+
*
78+
* // become
79+
*
80+
* (function(x) { return x * 2; }).call({ process: {} }, 5)
81+
* (function factorial(n) { return n <= 1 ? 1 : n * factorial(n-1); }).call({ process: {} }, 5)
82+
* ```
83+
*/
84+
this.traverse(path); // depth first to transform inside out
85+
const callExpression = b.callExpression(
86+
b.memberExpression(fnExpression, b.identifier('call')),
87+
[EMPTY_CONTEXT, ...node.arguments],
88+
);
89+
path.replace(callExpression);
90+
return false;
91+
},
92+
93+
visitFunctionExpression(path) {
94+
const { node } = path;
95+
96+
/**
97+
* Callable function expressions (callbacks) - both anonymous and named:
98+
*
99+
* ```js
100+
* [1, 2, 3].map(function(n) { return n * 2; })
101+
* [1, 2, 3].map(function factorial(n) { return n <= 1 ? 1 : n * factorial(n-1); })
102+
*
103+
* // become
104+
*
105+
* [1, 2, 3].map((function(n) { return n * 2; }).bind({ process: {} }))
106+
* [1, 2, 3].map((function factorial(n) { return n <= 1 ? 1 : n * factorial(n-1); }).bind({ process: {} }))
107+
* ```
108+
*/
109+
this.traverse(path);
110+
const boundFunction = b.callExpression(b.memberExpression(node, b.identifier('bind')), [
111+
EMPTY_CONTEXT,
112+
]);
113+
path.replace(boundFunction);
114+
return false;
115+
},
116+
});
117+
};
118+
52119
/**
53120
* Validates that the $ identifier is only used in allowed contexts.
54121
* This prevents user errors like `{{ $ }}` which would return the function object itself.

‎packages/workflow/src/extensions/expression-extension.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* eslint-disable @typescript-eslint/no-unsafe-member-access */
2-
import type { ExpressionKind } from 'ast-types/gen/kinds';
2+
import type { ExpressionKind } from 'ast-types/lib/gen/kinds';
33
import type { Config as EsprimaConfig } from 'esprima-next';
44
import { parse as esprimaParse } from 'esprima-next';
55
import { DateTime } from 'luxon';

‎packages/workflow/src/utils.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,15 @@ export function hasKey<T extends PropertyKey>(value: unknown, key: T): value is
332332
return value !== null && typeof value === 'object' && value.hasOwnProperty(key);
333333
}
334334

335-
const unsafeObjectProperties = new Set(['__proto__', 'prototype', 'constructor', 'getPrototypeOf']);
335+
const unsafeObjectProperties = new Set([
336+
'__proto__',
337+
'prototype',
338+
'constructor',
339+
'getPrototypeOf',
340+
'mainModule',
341+
'binding',
342+
'_load',
343+
]);
336344

337345
/**
338346
* Checks if a property key is safe to use on an object, preventing prototype pollution.

‎packages/workflow/test/expression-sandboxing.test.ts‎

Lines changed: 140 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { Tournament } from '@n8n/tournament';
22

33
import {
44
DollarSignValidator,
5+
FunctionThisSanitizer,
56
PrototypeSanitizer,
67
sanitizer,
78
DOLLAR_SIGN_ERROR,
@@ -14,7 +15,7 @@ const tournament = new Tournament(
1415
undefined,
1516
undefined,
1617
{
17-
before: [],
18+
before: [FunctionThisSanitizer],
1819
after: [PrototypeSanitizer, DollarSignValidator],
1920
},
2021
);
@@ -226,3 +227,141 @@ describe('PrototypeSanitizer', () => {
226227
});
227228
});
228229
});
230+
231+
describe('FunctionThisSanitizer', () => {
232+
describe('call expression where callee is function expression', () => {
233+
it('should transform call expression', () => {
234+
const result = tournament.execute('{{ (function() { return this.process; })() }}', {
235+
__sanitize: sanitizer,
236+
});
237+
expect(result).toEqual({});
238+
});
239+
240+
it('should handle recursive call expression', () => {
241+
const result = tournament.execute(
242+
'{{ (function factorial(n) { return n <= 1 ? 1 : n * factorial(n - 1); })(5) }}',
243+
{
244+
__sanitize: sanitizer,
245+
},
246+
);
247+
expect(result).toBe(120);
248+
});
249+
250+
it('should not expose process.env through named function', () => {
251+
const result = tournament.execute('{{ (function test(){ return this.process.env })() }}', {
252+
__sanitize: sanitizer,
253+
});
254+
expect(result).toBe(undefined);
255+
});
256+
257+
it('should still allow access to workflow data via variables', () => {
258+
const result = tournament.execute('{{ (function() { return $json.value; })() }}', {
259+
__sanitize: sanitizer,
260+
$json: { value: 'test-value' },
261+
});
262+
expect(result).toBe('test-value');
263+
});
264+
265+
it('should handle nested call expression', () => {
266+
const result = tournament.execute(
267+
'{{ (function() { return (function() { return this.process; })(); })() }}',
268+
{
269+
__sanitize: sanitizer,
270+
},
271+
);
272+
expect(result).toEqual({});
273+
});
274+
275+
it('should handle nested recursive call expression', () => {
276+
const result = tournament.execute(
277+
'{{ (function() { return (function factorial(n) { return n <= 1 ? 1 : n * factorial(n - 1); })(5); })() }}',
278+
{
279+
__sanitize: sanitizer,
280+
},
281+
);
282+
expect(result).toBe(120);
283+
});
284+
});
285+
286+
describe('function expression', () => {
287+
it('should transform function expression', () => {
288+
const result = tournament.execute('{{ [1].map(function() { return this.process; }) }}', {
289+
__sanitize: sanitizer,
290+
});
291+
expect(result).toEqual([{}]);
292+
});
293+
294+
it('should handle recursive function expression', () => {
295+
const result = tournament.execute(
296+
'{{ [1, 2, 3, 4, 5].map(function factorial(n) { return n <= 1 ? 1 : n * factorial(n - 1); }) }}',
297+
{
298+
__sanitize: sanitizer,
299+
},
300+
);
301+
expect(result).toEqual([1, 2, 6, 24, 120]);
302+
});
303+
304+
it('should handle nested function expression', () => {
305+
const result = tournament.execute(
306+
'{{ [1, 2, 3].map(function(n) { return function() { return n * 2; }; }).map(function(fn) { return fn(); }) }}',
307+
{
308+
__sanitize: sanitizer,
309+
},
310+
);
311+
expect(result).toEqual([2, 4, 6]);
312+
});
313+
314+
it('should handle nested recursion', () => {
315+
const result = tournament.execute(
316+
'{{ (function fibonacci(n) { return n <= 1 ? n : fibonacci(n - 1) + fibonacci(n - 2); })(7) }}',
317+
{
318+
__sanitize: sanitizer,
319+
},
320+
);
321+
expect(result).toBe(13);
322+
});
323+
});
324+
325+
describe('process.env security', () => {
326+
it('should bind function expressions to empty process object', () => {
327+
const processResult = tournament.execute('{{ (function(){ return this.process })() }}', {
328+
__sanitize: sanitizer,
329+
});
330+
expect(processResult).toEqual({});
331+
expect(Object.keys(processResult as object)).toEqual([]);
332+
333+
const envResult = tournament.execute('{{ (function(){ return this.process.env })() }}', {
334+
__sanitize: sanitizer,
335+
});
336+
expect(envResult).toBe(undefined);
337+
});
338+
339+
it('should block process.env in nested functions', () => {
340+
const result = tournament.execute(
341+
'{{ (function outer(){ return (function inner(){ return this.process.env })(); })() }}',
342+
{
343+
__sanitize: sanitizer,
344+
},
345+
);
346+
expect(result).toBe(undefined);
347+
});
348+
349+
it('should block process.env in callbacks', () => {
350+
const result = tournament.execute(
351+
'{{ [1].map(function(){ return this.process.env; })[0] }}',
352+
{
353+
__sanitize: sanitizer,
354+
},
355+
);
356+
expect(result).toBe(undefined);
357+
});
358+
359+
it('should still allow access to workflow variables', () => {
360+
const result = tournament.execute('{{ (function(){ return $json.value })() }}', {
361+
__sanitize: sanitizer,
362+
$json: { value: 'workflow-data' },
363+
});
364+
expect(result).toBe('workflow-data');
365+
});
366+
});
367+
});

‎packages/workflow/test/utils.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,9 @@ describe('isSafeObjectProperty', () => {
379379
['prototype', false],
380380
['constructor', false],
381381
['getPrototypeOf', false],
382+
['mainModule', false],
383+
['binding', false],
384+
['_load', false],
382385
['safeKey', true],
383386
['anotherKey', true],
384387
['toString', true],

0 commit comments

Comments
 (0)