From 97952148365eeabca2d0819063d97de2a9991e4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 13 Jan 2026 10:10:27 +0100 Subject: [PATCH] refactor(core): Improve expressions handling (#24177) --- .../src/expression-evaluator-proxy.ts | 8 +- .../workflow/src/expression-sandboxing.ts | 46 ++++++++- .../test/expression-sandboxing.test.ts | 95 ++++++++++++++++++- 3 files changed, 139 insertions(+), 10 deletions(-) diff --git a/packages/workflow/src/expression-evaluator-proxy.ts b/packages/workflow/src/expression-evaluator-proxy.ts index 6cea0ba9ebd..2c5db7a6efc 100644 --- a/packages/workflow/src/expression-evaluator-proxy.ts +++ b/packages/workflow/src/expression-evaluator-proxy.ts @@ -1,17 +1,13 @@ import { Tournament } from '@n8n/tournament'; -import { - DollarSignValidator, - FunctionThisSanitizer, - PrototypeSanitizer, -} from './expression-sandboxing'; +import { DollarSignValidator, ThisSanitizer, PrototypeSanitizer } from './expression-sandboxing'; type Evaluator = (expr: string, data: unknown) => string | null | (() => unknown); type ErrorHandler = (error: Error) => void; const errorHandler: ErrorHandler = () => {}; const tournamentEvaluator = new Tournament(errorHandler, undefined, undefined, { - before: [FunctionThisSanitizer], + before: [ThisSanitizer], after: [PrototypeSanitizer, DollarSignValidator], }); const evaluator: Evaluator = tournamentEvaluator.execute.bind(tournamentEvaluator); diff --git a/packages/workflow/src/expression-sandboxing.ts b/packages/workflow/src/expression-sandboxing.ts index d2ed66d80a0..62d18989f76 100644 --- a/packages/workflow/src/expression-sandboxing.ts +++ b/packages/workflow/src/expression-sandboxing.ts @@ -12,6 +12,10 @@ const EMPTY_CONTEXT = b.objectExpression([ b.property('init', b.identifier('process'), b.objectExpression([])), ]); +const SAFE_GLOBAL = b.objectExpression([]); + +const SAFE_THIS = b.sequenceExpression([b.literal(0), EMPTY_CONTEXT]); + /** * Helper to check if an expression is a valid property access with $ as the property. * Returns true for obj.$ or obj.nested.$ but false for bare $ or other expression contexts. @@ -53,10 +57,12 @@ const isValidDollarPropertyAccess = (expr: unknown): boolean => { return isPropertyDollar && !isObjectDollar && isObjectValid; }; +const GLOBAL_IDENTIFIERS = new Set(['globalThis']); + /** * Prevents regular functions from binding their `this` to the Node.js global. */ -export const FunctionThisSanitizer: ASTBeforeHook = (ast, dataNode) => { +export const ThisSanitizer: ASTBeforeHook = (ast, dataNode) => { astVisit(ast, { visitCallExpression(path) { const { node } = path; @@ -113,6 +119,44 @@ export const FunctionThisSanitizer: ASTBeforeHook = (ast, dataNode) => { path.replace(boundFunction); return false; }, + + visitIdentifier(path) { + this.traverse(path); + const { node } = path; + + if (GLOBAL_IDENTIFIERS.has(node.name)) { + const parent: unknown = path.parent; + const isPropertyName = + typeof parent === 'object' && + parent !== null && + 'name' in parent && + parent.name === 'property'; + + if (!isPropertyName) path.replace(SAFE_GLOBAL); + } + }, + + visitThisExpression(path) { + this.traverse(path); + + /** + * Replace `this` with a safe context object. + * This prevents arrow functions from accessing the real global context: + * + * ```js + * (() => this?.process)() // becomes (() => (0, { process: {} })?.process)() + * ``` + * + * Arrow functions don't have their own `this` binding - they inherit from + * the outer lexical scope. Without this fix, `this` inside an arrow function + * would resolve to the Node.js global object, exposing process.env and other + * sensitive data. + * + * We use SAFE_THIS (a sequence expression) instead of EMPTY_CONTEXT directly + * to ensure the object literal is unambiguously parsed as an expression. + */ + path.replace(SAFE_THIS); + }, }); }; diff --git a/packages/workflow/test/expression-sandboxing.test.ts b/packages/workflow/test/expression-sandboxing.test.ts index 1c492d84814..1874988a1a5 100644 --- a/packages/workflow/test/expression-sandboxing.test.ts +++ b/packages/workflow/test/expression-sandboxing.test.ts @@ -2,7 +2,7 @@ import { Tournament } from '@n8n/tournament'; import { DollarSignValidator, - FunctionThisSanitizer, + ThisSanitizer, PrototypeSanitizer, sanitizer, DOLLAR_SIGN_ERROR, @@ -16,7 +16,7 @@ const tournament = new Tournament( undefined, undefined, { - before: [FunctionThisSanitizer], + before: [ThisSanitizer], after: [PrototypeSanitizer, DollarSignValidator], }, ); @@ -395,7 +395,7 @@ describe('PrototypeSanitizer', () => { }); }); -describe('FunctionThisSanitizer', () => { +describe('ThisSanitizer', () => { describe('call expression where callee is function expression', () => { it('should transform call expression', () => { const result = tournament.execute('{{ (function() { return this.process; })() }}', { @@ -531,4 +531,93 @@ describe('FunctionThisSanitizer', () => { expect(result).toBe('workflow-data'); }); }); + + describe('globalThis access via arrow functions', () => { + it('should replace globalThis with empty object', () => { + const result = tournament.execute('{{ (() => globalThis)() }}', { + __sanitize: sanitizer, + }); + expect(result).toEqual({}); + expect(result).not.toBe(globalThis); + }); + + it('should block process.env access via globalThis', () => { + const result = tournament.execute('{{ (() => globalThis.process)() }}', { + __sanitize: sanitizer, + }); + expect(result).toBe(undefined); + }); + + it('should block chained globalThis access', () => { + const result = tournament.execute('{{ ((g) => g.process)((() => globalThis)()) }}', { + __sanitize: sanitizer, + }); + expect(result).toBe(undefined); + }); + + it('should block env access via nested arrow functions', () => { + // This payload attempts to access process.env via chained arrow functions + // With the fix, globalThis becomes {}, so g.process is undefined, + // and accessing .env on undefined throws an error - which is the desired security outcome + expect(() => { + tournament.execute('{{ ((p) => p.env)(((g) => g.process)((() => globalThis)())) }}', { + __sanitize: sanitizer, + }); + }).toThrow(); + }); + + it('should replace globalThis with empty object in non-arrow contexts too', () => { + // globalThis is replaced with {} at AST level, regardless of context + const result = tournament.execute('{{ globalThis }}', { + __sanitize: sanitizer, + }); + expect(result).toEqual({}); + }); + + it('should still allow access to workflow data via variables', () => { + const result = tournament.execute('{{ (() => $json.value)() }}', { + __sanitize: sanitizer, + $json: { value: 'test-value' }, + }); + expect(result).toBe('test-value'); + }); + }); + + describe('this access via arrow functions', () => { + it('should replace this with safe context in arrow functions', () => { + const result = tournament.execute('{{ (() => this)() }}', { + __sanitize: sanitizer, + }); + expect(result).toEqual({ process: {} }); + }); + + it('should block process.env access via this in arrow functions', () => { + const result = tournament.execute('{{ (() => this?.process)() }}', { + __sanitize: sanitizer, + }); + expect(result).toEqual({}); + expect(result).not.toHaveProperty('env'); + }); + + it('should block this access in nested arrow functions', () => { + const result = tournament.execute('{{ (() => (() => this)())() }}', { + __sanitize: sanitizer, + }); + expect(result).toEqual({ process: {} }); + }); + + it('should block this?.process?.env access pattern', () => { + const result = tournament.execute('{{ (() => this?.process?.env)() }}', { + __sanitize: sanitizer, + }); + expect(result).toBe(undefined); + }); + + it('should still work with this in regular function expressions', () => { + const result = tournament.execute('{{ (function() { return this.process; })() }}', { + __sanitize: sanitizer, + }); + expect(result).toEqual({}); + }); + }); });