refactor(core): Improve expressions handling (#24177)

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);

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);
+		},
 	});
 };
 

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({});
+		});
+	});
 });