diff --git a/.changeset/expr-restricted-evaluation.md b/.changeset/expr-restricted-evaluation.md new file mode 100644 index 000000000..b4d95f962 --- /dev/null +++ b/.changeset/expr-restricted-evaluation.md @@ -0,0 +1,7 @@ +--- +"@gitbook/expr": minor +"@gitbook/react-openapi": patch +"gitbook": patch +--- + +Restrict expressions to an allowlist of methods and syntax, and report unsupported expressions when parsing. diff --git a/bun.lock b/bun.lock index 1fa67ddd8..4954a3938 100644 --- a/bun.lock +++ b/bun.lock @@ -82,7 +82,7 @@ "acorn-walk": "^8.3.4", "assert-never": "catalog:", "escodegen": "^2.1.0", - "eval-estree-expression": "^3.0.1", + "eval-estree-expression": "^4.0.0", }, "devDependencies": { "@tsconfig/node20": "catalog:", @@ -2052,7 +2052,7 @@ "etag": ["etag@1.8.1", "", {}, "sha512-aIL5Fx7mawVa300al2BnEE4iNvo1qETxLrPI/o05L7z6go7fCw1J6EQmbK4FmJ2AS7kgVF/KEZWufBfdClMcPg=="], - "eval-estree-expression": ["eval-estree-expression@3.0.1", "", {}, "sha512-zTLKGbiVdQYp4rQkSoXPibrFf5ZoPn6jzExegRLEQ13F+FSxu5iLgaRH6hlDs2kWSUa6vp8yD20cdJi0me6pEw=="], + "eval-estree-expression": ["eval-estree-expression@4.0.0", "", { "dependencies": { "get-value": "^4.0.1" } }, "sha512-QWT9DtTs+LgiKycu8eP8F3gPMD95dR4tmGyvHfpliEhS38yZ4TrNZOVKVtAzyZSLw2PP9/JB1JlIWZBuy/AZ0w=="], "event-emitter": ["event-emitter@0.3.5", "", { "dependencies": { "d": "1", "es5-ext": "~0.10.14" } }, "sha512-D9rRn9y7kLPnJ+hMq7S/nhvoKwwvVJahBi2BPmx3bvbsEdK3W9ii8cBSGjP+72/LnM4n6fo3+dkCX5FeTQruXA=="], @@ -2170,6 +2170,8 @@ "get-uri": ["get-uri@6.0.5", "", { "dependencies": { "basic-ftp": "^5.0.2", "data-uri-to-buffer": "^6.0.2", "debug": "^4.3.4" } }, "sha512-b1O07XYq8eRuVzBNgJLstU6FYc1tS6wnMtF1I1D9lE8LxZSOGZ7LhxN54yPP6mGw5f2CkXY2BQUL9Fx41qvcIg=="], + "get-value": ["get-value@4.1.0", "", {}, "sha512-mKjrL73VxCUixi0Qe9OuerDVkm+475FeNwM5zZpjK6TK/a7fqv/dGI7vkRgvWkyw1xQ9Fl+wKuOUgPkE6aYB3w=="], + "gitbook": ["gitbook@workspace:packages/gitbook"], "github-slugger": ["github-slugger@2.0.0", "", {}, "sha512-IaOQ9puYtjrkq7Y0Ygl9KDZnrf/aiUJYUpVf89y8kyaxbRG7Y1SrX/jaumrv81vc61+kiMempujsM3Yw7w5qcw=="], diff --git a/packages/expr/package.json b/packages/expr/package.json index 06363f51c..98f3ac424 100644 --- a/packages/expr/package.json +++ b/packages/expr/package.json @@ -16,7 +16,7 @@ "acorn-walk": "^8.3.4", "assert-never": "catalog:", "escodegen": "^2.1.0", - "eval-estree-expression": "^3.0.1" + "eval-estree-expression": "^4.0.0" }, "devDependencies": { "@tsconfig/strictest": "catalog:", diff --git a/packages/expr/src/__tests__/runtime.test.ts b/packages/expr/src/__tests__/runtime.test.ts index 875794948..79c176999 100644 --- a/packages/expr/src/__tests__/runtime.test.ts +++ b/packages/expr/src/__tests__/runtime.test.ts @@ -1,7 +1,15 @@ -import { describe, expect, it } from 'bun:test'; +import { parseExpressionAt } from 'acorn'; +import { afterEach, beforeEach, describe, expect, it, mock } from 'bun:test'; import { ExpressionError } from '../errors'; +import { + ALLOWED_METHODS, + MAX_CALLBACK_CALLS, + UnsafeExpressionError, + evaluateRestricted, +} from '../restricted'; import { ExpressionRuntime } from '../runtime'; +import { SymbolArray, SymbolString } from '../symbols'; import type { Logger } from '../types'; const SILENT_LOGGER: Logger = { @@ -10,6 +18,17 @@ const SILENT_LOGGER: Logger = { error: () => {}, }; +const getInputs = () => ({ user: { role: 'editor' }, tags: ['b', 'a'], count: 1 }); + +// Returns the thrown error instead of throwing, so that side effects can be asserted first. +const attempt = (fn: () => unknown) => { + try { + return fn(); + } catch (error) { + return error; + } +}; + describe('ExpressionRuntime', () => { const runtime = new ExpressionRuntime(SILENT_LOGGER); @@ -66,6 +85,51 @@ describe('ExpressionRuntime', () => { inputs: { x: 2 }, expectedResult: [2, 4, 6], }, + { + scenario: 'array filter', + condition: 'reviews.filter(review => review.status === "approved").length >= 2', + inputs: { + reviews: [ + { status: 'approved' }, + { status: 'pending' }, + { status: 'approved' }, + ], + }, + expectedResult: true, + }, + { + scenario: 'array some', + condition: 'tags.some(tag => tag.startsWith("b") && tag.length === x)', + inputs: { tags: ['a', 'bb'], x: 2 }, + expectedResult: true, + }, + { + scenario: 'nested callbacks reading outer parameters', + condition: 'required.every(id => reviews.some(review => review.reviewer === id))', + inputs: { + required: ['u1', 'u2'], + reviews: [{ reviewer: 'u2' }, { reviewer: 'u1' }], + }, + expectedResult: true, + }, + { + scenario: 'callback parameter shadowing an input', + condition: 'tags.some(role => role === "b") && role === "admin"', + inputs: { tags: ['a', 'b'], role: 'admin' }, + expectedResult: true, + }, + { + scenario: 'array literal includes', + condition: '["admin", "editor"].includes(user.role)', + inputs: { user: { role: 'admin' } }, + expectedResult: true, + }, + { + scenario: 'array and string at', + condition: 'tags.at(-1).at(0)', + inputs: { tags: ['a', 'bc'] }, + expectedResult: 'b', + }, { scenario: 'string startsWith', condition: 'user.role.startsWith("ad")', @@ -102,6 +166,79 @@ describe('ExpressionRuntime', () => { inputs: { user: { role: ' admin ' } }, expectedResult: true, }, + { + scenario: 'string length and index', + condition: 'user.role.length === 5 && user.role[0] === "a"', + inputs: { user: { role: 'admin' } }, + expectedResult: true, + }, + { + scenario: 'missing property', + condition: 'user.plan === undefined && missing === undefined', + inputs: { user: { role: 'admin' } }, + expectedResult: true, + }, + { + scenario: 'boolean claim', + condition: 'visitor.claims.flag === true', + inputs: { visitor: { claims: { flag: true } } }, + expectedResult: true, + }, + { + scenario: 'nested claims joined with ||', + condition: 'visitor.claims.group.a === true || visitor.claims.group.b === true', + inputs: { visitor: { claims: { group: { b: true } } } }, + expectedResult: true, + }, + { + scenario: 'negated guard', + condition: "!visitor.claims.unsigned || visitor.claims.unsigned.region !== 'eu'", + inputs: { visitor: { claims: { unsigned: { region: 'us' } } } }, + expectedResult: true, + }, + { + scenario: 'negated guard with a missing claim', + condition: "!visitor.claims.unsigned || visitor.claims.unsigned.region !== 'eu'", + inputs: { visitor: { claims: {} } }, + expectedResult: true, + }, + { + scenario: 'key with a colon', + condition: "visitor.claims['custom:tier'] !== 'gold'", + inputs: { visitor: { claims: { 'custom:tier': 'silver' } } }, + expectedResult: true, + }, + { + scenario: 'key with a colon and a missing claim', + condition: "visitor.claims['custom:tier'] !== 'gold'", + inputs: { visitor: { claims: {} } }, + expectedResult: true, + }, + { + scenario: 'URL-style key', + condition: "visitor.claims['https://example.com/flag'] === true", + inputs: { visitor: { claims: { 'https://example.com/flag': true } } }, + expectedResult: true, + }, + { + scenario: 'claim array includes', + condition: "visitor.claims.products.includes('a')", + inputs: { visitor: { claims: { products: ['a', 'b'] } } }, + expectedResult: true, + }, + { + scenario: 'array literal includes a claim', + condition: "['a', 'b'].includes(visitor.claims.org)", + inputs: { visitor: { claims: { org: 'a' } } }, + expectedResult: true, + }, + { + scenario: '&& guard with a dashed key', + condition: + "visitor.claims.unsigned && visitor.claims.unsigned['early-access'] === true", + inputs: { visitor: { claims: { unsigned: { 'early-access': true } } } }, + expectedResult: true, + }, ])( 'should properly evaluate/safeEvaluate a valid conditional expression: $scenario', ({ condition, inputs, expectedResult }) => { @@ -126,11 +263,6 @@ describe('ExpressionRuntime', () => { condition: 'while (1) {}', inputs: {}, }, - { - scenario: 'unsafe expression', - condition: '[1, 2, 3].map(() => { while (1) {}})', - inputs: {}, - }, ]; it.each(INVALID_EXPRESSSIONS)( @@ -148,6 +280,303 @@ describe('ExpressionRuntime', () => { expect(() => runtime.evaluate(condition, inputs)).toThrowError(ExpressionError); } ); + + it.each([ + { scenario: 'string method', condition: 'visitor.claims.plan.startsWith("p")' }, + { scenario: 'array method', condition: 'visitor.claims.products.includes("a")' }, + { scenario: 'callback method', condition: 'visitor.claims.products.some(p => p)' }, + { scenario: 'missing parent', condition: 'visitor.claims.org.name.toLowerCase()' }, + ])( + 'should give undefined when calling a method on a missing claim: $scenario', + ({ condition }) => { + const inputs = { visitor: { claims: {} } }; + + expect(runtime.evaluate(condition, inputs)).toBeUndefined(); + expect(runtime.safeEvaluate(condition, inputs)).toEqual({ value: undefined }); + expect(runtime.evaluateBoolean(condition, inputs)).toBe(false); + } + ); + }); + + describe('unsafe expressions', () => { + // Each expression tries to call this function, which only an escape from the restrictions can reach. + const PAYLOAD = '__expressionPayload'; + const CALL_PAYLOAD = `globalThis.${PAYLOAD}()`; + const payload = mock(() => true); + + beforeEach(() => { + payload.mockClear(); + Reflect.set(globalThis, PAYLOAD, payload); + }); + + afterEach(() => { + Reflect.deleteProperty(globalThis, PAYLOAD); + }); + + const UNSAFE_EXPRESSIONS = [ + { + scenario: 'array key bypassing the constructor check', + condition: `({})[["constructor"]][["constructor"]]("${CALL_PAYLOAD}")()`, + }, + { + scenario: 'array key on an input value', + condition: `user.role[["constructor"]][["constructor"]]("${CALL_PAYLOAD}")()`, + }, + { + scenario: 'array key on a method', + condition: `"".includes[["constructor"]]("${CALL_PAYLOAD}")()`, + }, + { + scenario: 'global access in a callback', + condition: `[1].some(() => ${CALL_PAYLOAD})`, + }, + { + scenario: 'computed constructor key in a callback', + condition: `[1].some(x => x["constr" + "uctor"]["constr" + "uctor"]("${CALL_PAYLOAD}")())`, + }, + { + scenario: 'computed constructor key in a map callback', + condition: `[1].map(x => x["constr" + "uctor"]["constr" + "uctor"]("${CALL_PAYLOAD}")())`, + }, + { + scenario: 'arrow function called through implicit toString', + condition: `({ toString: () => ${CALL_PAYLOAD} }) + ""`, + }, + { scenario: 'inherited property', condition: 'user.constructor' }, + { + scenario: 'inherited property with an array key', + condition: 'user[["constructor"]]', + }, + { + scenario: 'inherited property of a string', + condition: 'user.role.toString', + runtimeOnly: true, + }, + { scenario: 'inherited property of a number', condition: 'count.constructor' }, + { scenario: 'prototype access', condition: 'user.__proto__' }, + { scenario: 'inherited variable', condition: 'toString', runtimeOnly: true }, + { scenario: 'arrow function outside a callback', condition: '[() => 1].length' }, + { + scenario: 'block-bodied arrow function', + condition: '[1, 2, 3].some(() => { while (1) {} })', + }, + { scenario: 'function expression', condition: '[1].some(function () { return 1 })' }, + { scenario: 'async arrow callback', condition: 'tags.some(async t => t)' }, + { + scenario: 'async function callback', + condition: 'tags.some(async function (t) { return t })', + }, + { scenario: 'immediately invoked async arrow', condition: '(async () => 1)()' }, + { scenario: 'object literal', condition: '({ role: "admin" }).role' }, + { scenario: 'constructor call', condition: 'new Date()' }, + { scenario: 'eval', condition: `eval("${CALL_PAYLOAD}")` }, + { scenario: 'eval as a method', condition: `globalThis.eval("${CALL_PAYLOAD}")` }, + { scenario: 'new Function', condition: `new Function("${CALL_PAYLOAD}")()` }, + { scenario: 'Function without new', condition: `Function("${CALL_PAYLOAD}")()` }, + { + scenario: 'new on a function expression', + condition: `new function () { ${CALL_PAYLOAD} }()`, + }, + { scenario: 'method outside the standard library', condition: '"a".padStart(1e9)' }, + { scenario: 'string repeat', condition: 'user.role.repeat(1e9)' }, + { scenario: 'string repeat in a callback', condition: 'tags.map(t => t.repeat(1e9))' }, + { scenario: 'mutating array method', condition: 'tags.reverse()' }, + { + scenario: 'callback for a method not taking one', + condition: 'tags.includes(t => t)', + }, + { + scenario: 'inherited variable in a callback', + condition: 'tags.some(t => toString)', + runtimeOnly: true, + }, + { + scenario: 'callback parameter named __proto__', + condition: 'tags.some(__proto__ => 1)', + }, + { + scenario: 'callback parameter named constructor', + condition: 'tags.some(constructor => 1)', + }, + { + scenario: 'expression deeper than the library limit', + condition: `${'!'.repeat(60)}true`, + runtimeOnly: true, + }, + { + scenario: 'array literal longer than the library limit', + condition: `[${'1,'.repeat(10_001)}].length`, + runtimeOnly: true, + }, + { + scenario: 'inherited property of a callback parameter', + condition: 'tags.some(t => t.constructor)', + }, + { scenario: 'non-arrow callback', condition: 'tags.map(Function)' }, + { scenario: 'callback with an extra argument', condition: 'tags.some(t => t, "a")' }, + { scenario: 'missing callback', condition: 'tags.some()' }, + { + scenario: 'too many nested callback calls', + condition: + '[1, 2, 3, 4, 5, 6, 7, 8, 9, 10].map(a => [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].map(b => [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].map(c => [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].map(d => d))))', + runtimeOnly: true, + }, + { scenario: 'method on a plain object', condition: 'user.hasOwnProperty("role")' }, + { scenario: 'method on a function', condition: '[1].some.call([1], n => n)' }, + { + scenario: 'non-primitive argument', + condition: '[user].includes(user)', + runtimeOnly: true, + }, + { scenario: 'regex operator', condition: 'user.role =~ /^ed/' }, + { scenario: 'regex argument', condition: 'user.role.includes(/(a+)+b/)' }, + { scenario: 'BigInt literal', condition: '2n ** 64n' }, + { + scenario: 'BigInt literal in a callback', + condition: '[1, 2, 3, 4].some((n) => 9n ** 9n ** 9n)', + }, + { scenario: 'BigInt literal in an array', condition: '[9n][0] ** 9n' }, + { scenario: 'BigInt literal in a template', condition: '`${9n ** 9n ** 9n}`' }, + { scenario: 'delete', condition: 'delete user.role' }, + { scenario: 'assignment', condition: 'user.role = "admin"' }, + { scenario: 'update', condition: 'count++' }, + ]; + + it.each(UNSAFE_EXPRESSIONS)( + 'should throw with evaluate and return the error with safeEvaluate: $scenario', + ({ condition }) => { + const evaluated = attempt(() => runtime.evaluate(condition, getInputs())); + const safeEvaluated = runtime.safeEvaluate(condition, getInputs()); + + expect(payload).not.toHaveBeenCalled(); + expect(evaluated).toBeInstanceOf(ExpressionError); + expect(safeEvaluated.value).toBeUndefined(); + expect(safeEvaluated.error).toBeInstanceOf(ExpressionError); + } + ); + + it.each(UNSAFE_EXPRESSIONS)( + 'should evaluate to false with evaluateBoolean/evaluateBooleanAll: $scenario', + ({ condition }) => { + const evaluated = runtime.evaluateBoolean(condition, getInputs()); + const evaluatedAll = runtime.evaluateBooleanAll(['true', condition], getInputs()); + + expect(payload).not.toHaveBeenCalled(); + expect(evaluated).toBe(false); + expect(evaluatedAll).toBe(false); + } + ); + + it.each(UNSAFE_EXPRESSIONS)( + 'should throw without executing code with evaluateTemplate: $scenario', + ({ condition }) => { + const evaluated = attempt(() => + runtime.evaluateTemplate(`Hello {{ ${condition} }}`, getInputs()) + ); + + expect(payload).not.toHaveBeenCalled(); + expect(evaluated).toBeInstanceOf(ExpressionError); + } + ); + + it.each(UNSAFE_EXPRESSIONS)('should not mutate the inputs: $scenario', ({ condition }) => { + const inputs = getInputs(); + attempt(() => runtime.evaluate(condition, inputs)); + expect(inputs).toEqual(getInputs()); + }); + + it.each(UNSAFE_EXPRESSIONS)( + 'should report the error when parsing, for editors: $scenario', + ({ condition, runtimeOnly }) => { + const parsed = attempt(() => runtime.parse(condition)); + const { errors } = runtime.parseTemplate(`Hello {{ ${condition} }}`); + + if (runtimeOnly) { + // Depends on the values, so only enforced when evaluating. + expect(parsed).not.toBeInstanceOf(Error); + expect(errors).toEqual([]); + } else { + expect(parsed).toBeInstanceOf(ExpressionError); + expect((parsed as ExpressionError).location).toBeDefined(); + expect(errors).toHaveLength(1); + } + } + ); + + it.each(UNSAFE_EXPRESSIONS)( + 'should not execute code when evaluating without validating first: $scenario', + ({ condition }) => { + const expression = parseExpressionAt(condition, 0, { ecmaVersion: 'latest' }); + const evaluated = attempt(() => evaluateRestricted(expression, getInputs())); + + expect(payload).not.toHaveBeenCalled(); + expect( + evaluated instanceof UnsafeExpressionError || evaluated instanceof RangeError + ).toBe(true); + } + ); + + it('should point the parse error at the offending token', () => { + const condition = 'user.role.padStart(10)'; + const error = attempt(() => runtime.parse(condition)) as ExpressionError; + + expect(error.message).toBe('Method "padStart" is not supported'); + expect(error.location).toEqual({ line: 1, column: 10 }); + expect(condition.slice(error.token?.start, error.token?.end)).toBe('padStart'); + }); + + it('should keep the location of the error when evaluating', () => { + const condition = 'user.role.padStart(10)'; + const error = attempt(() => runtime.evaluate(condition, getInputs())); + + expect(error).toBeInstanceOf(UnsafeExpressionError); + expect((error as ExpressionError).location).toEqual({ line: 1, column: 10 }); + }); + + it('should explain why async callbacks are rejected', () => { + const error = attempt(() => runtime.parse('tags.some(async t => t)')); + + expect((error as ExpressionError).message).toBe('Async functions are not supported'); + }); + + it('should not validate when parsing loosely for autocomplete', () => { + expect(() => runtime.parse('user.constructor', { loose: true })).not.toThrow(); + }); + + it.each(['constructor', '__proto__', 'prototype', '__defineGetter__', '__lookupGetter__'])( + 'should not read an own "%s" key of the inputs', + (key) => { + // JSON.parse creates own properties, even for `__proto__`. + const inputs = JSON.parse(`{ "data": { "${key}": "value" }, "${key}": "value" }`); + + expect(() => runtime.evaluate(`data["${key}"]`, inputs)).toThrow(ExpressionError); + expect(() => runtime.evaluate(key, inputs)).toThrow(ExpressionError); + expect(runtime.evaluateBoolean(`data["${key}"] === "value"`, inputs)).toBe(false); + } + ); + + it('should limit the number of callback calls per evaluation', () => { + const condition = 'items.map(item => item).length > 0'; + const atLimit = { items: Array.from({ length: MAX_CALLBACK_CALLS }, (_, i) => i) }; + const overLimit = { items: [...atLimit.items, MAX_CALLBACK_CALLS] }; + + expect(runtime.evaluate(condition, atLimit)).toBe(true); + // The budget is per evaluation, not shared across evaluations. + expect(runtime.evaluate(condition, atLimit)).toBe(true); + expect(() => runtime.evaluate(condition, overLimit)).toThrow(ExpressionError); + expect(runtime.evaluateBoolean(condition, overLimit)).toBe(false); + }); + + it('should only allow the standard library offered in the editor and reviewed additions', () => { + const stringMethods = SymbolString().methods.map((method) => method.name); + const arrayMethods = SymbolArray({ items: SymbolString() }).methods.map( + (method) => method.name + ); + + // Extend only after reviewing that a method is safe (see `ALLOWED_METHODS`). + expect(ALLOWED_METHODS.string).toEqual(new Set(stringMethods)); + expect(ALLOWED_METHODS.array).toEqual(new Set([...arrayMethods, 'filter', 'map'])); + }); }); describe('parse', () => { diff --git a/packages/expr/src/restricted/evaluate.ts b/packages/expr/src/restricted/evaluate.ts new file mode 100644 index 000000000..9b60eb3ac --- /dev/null +++ b/packages/expr/src/restricted/evaluate.ts @@ -0,0 +1,236 @@ +import type { + ArrowFunctionExpression, + CallExpression, + Expression, + Identifier, + Literal, + MemberExpression, + Node, + UnaryExpression, +} from 'acorn'; +import evalESTreeExpr from 'eval-estree-expression'; + +import { + ALLOWED_METHODS, + CALLBACK_METHODS, + MAX_CALLBACK_CALLS, + UNSAFE_KEYS, + UnsafeExpressionError, + getCallbackParams, +} from './rules'; + +/** + * Evaluate a parsed expression with the restrictions, enforcing the rules that depend on values. + */ +export function evaluateRestricted(expression: Expression, inputs: object): unknown { + return evalESTreeExpr.evaluate.sync(expression, inputs, { + // The library's own function support compiles code with `Function()`: calls and + // arrow functions are only handled by our visitors. + functions: false, + withMembers: true, + visitors: RESTRICTED_VISITORS, + }); +} + +/** + * Callback calls so far, per evaluator (the library creates one per evaluation). + */ +const callbackCalls = new WeakMap(); + +type Visit = (node: Node, context: object, parent?: Node) => unknown; +type Evaluator = { visit: Visit }; + +/** + * Visitors overriding eval-estree-expression's defaults so that expressions can only read own + * properties of plain data and call the methods of the standard library. + */ +const RESTRICTED_VISITORS = { + Identifier(this: Evaluator, node: Identifier, context: object): unknown { + return readVariable(context, node.name); + }, + + MemberExpression(this: Evaluator, node: MemberExpression, context: object): unknown { + const object = this.visit(node.object, context, node); + const key = node.computed + ? this.visit(node.property, context, node) + : (node.property as Identifier).name; + + // Reject rather than coerce, so that keys like `["constructor"]` can't slip through. + if (typeof key !== 'string' && typeof key !== 'number') { + throw new UnsafeExpressionError('Property keys must be strings or numbers'); + } + return readOwnProperty(object, String(key)); + }, + + CallExpression(this: Evaluator, node: CallExpression, context: object): unknown { + const callee = node.callee; + if (callee.type !== 'MemberExpression' || callee.computed || callee.optional) { + throw new UnsafeExpressionError('Only method calls are supported'); + } + + const method = (callee.property as Identifier).name; + const receiver = this.visit(callee.object, context, callee); + + // Like reading a property of a missing claim, calling a method on one gives `undefined`. + if ( + (receiver === null || receiver === undefined) && + (ALLOWED_METHODS.string.has(method) || ALLOWED_METHODS.array.has(method)) + ) { + return undefined; + } + + const fn = getAllowedMethod(receiver, method); + if (!fn) { + throw new UnsafeExpressionError(`Method "${method}" is not supported`); + } + + if (CALLBACK_METHODS.has(method)) { + const [callback] = node.arguments; + if (node.arguments.length !== 1 || callback?.type !== 'ArrowFunctionExpression') { + throw new UnsafeExpressionError(`"${method}" only accepts an arrow function`); + } + return Reflect.apply(fn, receiver, [createCallback(this, callback, context)]); + } + + const args = node.arguments.map((arg) => { + if (arg.type === 'SpreadElement') { + throw new UnsafeExpressionError('Spread arguments are not supported'); + } + const value = this.visit(arg, context, node); + if (!isPrimitive(value)) { + throw new UnsafeExpressionError('Only primitive values can be arguments'); + } + return value; + }); + + return Reflect.apply(fn, receiver, args); + }, + + ArrowFunctionExpression(): never { + throw new UnsafeExpressionError('Arrow functions are only supported as callbacks'); + }, + + FunctionExpression(): never { + throw new UnsafeExpressionError('Functions are not supported'); + }, + + ObjectExpression(): never { + throw new UnsafeExpressionError('Object literals are not supported'); + }, + + Literal(node: Literal): unknown { + // Regexes are only useful to the library's `=~` operator, and BigInt exponentiation can hang. + if (node.regex || typeof node.value === 'bigint') { + throw new UnsafeExpressionError('Regex and BigInt literals are not supported'); + } + return node.value; + }, + + UnaryExpression(this: Evaluator, node: UnaryExpression, context: object): unknown { + // The library implements `delete` by mutating the inputs. + if (node.operator === 'delete') { + throw new UnsafeExpressionError('"delete" is not supported'); + } + return Object.getPrototypeOf(this).UnaryExpression.call(this, node, context); + }, + + AssignmentExpression(): never { + throw new UnsafeExpressionError('Assignments are not supported'); + }, + + UpdateExpression(): never { + throw new UnsafeExpressionError('Update expressions are not supported'); + }, + + NewExpression(): never { + throw new UnsafeExpressionError('"new" is not supported'); + }, + + TaggedTemplateExpression(): never { + throw new UnsafeExpressionError('Tagged templates are not supported'); + }, + + ThisExpression(): never { + throw new UnsafeExpressionError('"this" is not supported'); + }, +}; + +/** + * Create a callback evaluating the arrow function's body with the evaluator, instead of compiling it. + */ +function createCallback(evaluator: Evaluator, node: ArrowFunctionExpression, context: object) { + const params = getCallbackParams(node); + + return (...args: unknown[]) => { + const calls = (callbackCalls.get(evaluator) ?? 0) + 1; + if (calls > MAX_CALLBACK_CALLS) { + throw new UnsafeExpressionError('Too many callback calls'); + } + callbackCalls.set(evaluator, calls); + + const variables = new Map(params.map((name, index) => [name, args[index]])); + return evaluator.visit(node.body, new CallbackScope(variables, context), node); + }; +} + +/** + * Variables of a callback call, falling back to the enclosing scope. + * A Map avoids copying the inputs on each call. + */ +class CallbackScope { + constructor( + readonly variables: Map, + readonly parent: object + ) {} +} + +function readVariable(scope: object, name: string): unknown { + if (UNSAFE_KEYS.has(name)) { + throw new UnsafeExpressionError(`"${name}" is not accessible`); + } + if (scope instanceof CallbackScope) { + return scope.variables.has(name) + ? scope.variables.get(name) + : readVariable(scope.parent, name); + } + return readOwnProperty(scope, name); +} + +/** + * Read an own property, treating any attempt to reach an inherited one (e.g. `constructor`) as unsafe. + */ +function readOwnProperty(object: unknown, key: string): unknown { + if (UNSAFE_KEYS.has(key)) { + throw new UnsafeExpressionError(`Property "${key}" is not accessible`); + } + if (object === null || object === undefined) { + return undefined; + } + if (typeof object === 'function') { + throw new UnsafeExpressionError('Functions are not accessible'); + } + + // Wrapping primitives exposes string indexes and `length` as own properties. + const target = Object(object); + if (Object.hasOwn(target, key)) { + return target[key]; + } + if (key in target) { + throw new UnsafeExpressionError(`Property "${key}" is not accessible`); + } + return undefined; +} + +function getAllowedMethod(receiver: unknown, method: string): Function | null { + if (typeof receiver === 'string' && ALLOWED_METHODS.string.has(method)) { + return Reflect.get(String.prototype, method); + } + if (Array.isArray(receiver) && ALLOWED_METHODS.array.has(method)) { + return Reflect.get(Array.prototype, method); + } + return null; +} + +function isPrimitive(value: unknown): boolean { + return value === null || (typeof value !== 'object' && typeof value !== 'function'); +} diff --git a/packages/expr/src/restricted/index.ts b/packages/expr/src/restricted/index.ts new file mode 100644 index 000000000..2a4d92a85 --- /dev/null +++ b/packages/expr/src/restricted/index.ts @@ -0,0 +1,3 @@ +export { evaluateRestricted } from './evaluate'; +export { ALLOWED_METHODS, MAX_CALLBACK_CALLS, UnsafeExpressionError } from './rules'; +export { validateExpression } from './validate'; diff --git a/packages/expr/src/restricted/rules.ts b/packages/expr/src/restricted/rules.ts new file mode 100644 index 000000000..f87b58c3f --- /dev/null +++ b/packages/expr/src/restricted/rules.ts @@ -0,0 +1,78 @@ +import type { AnyNode, ArrowFunctionExpression } from 'acorn'; + +import { ExpressionError } from '../errors'; + +/** + * Methods that expressions are allowed to call, per receiver type. + * Must include the standard library offered in the editor (see `symbols.ts`). Only add methods + * after review: they must not mutate their receiver, and their output must stay bounded by their input. + */ +export const ALLOWED_METHODS = { + string: new Set([ + 'at', + 'endsWith', + 'includes', + 'startsWith', + 'toLowerCase', + 'toUpperCase', + 'trim', + ]), + array: new Set(['at', 'every', 'filter', 'includes', 'map', 'some']), +}; + +/** + * Array methods taking a callback, the only place where arrow functions are allowed. + */ +export const CALLBACK_METHODS = new Set(['every', 'filter', 'map', 'some']); + +/** + * Maximum number of callback calls in a single evaluation, as nested callbacks multiply and + * visitors control the size of unsigned claims. Enough for a pass over an identity provider's + * group claim (capped around 200 entries), or a small nested pair of arrays. + */ +export const MAX_CALLBACK_CALLS = 250; + +/** + * Names never readable as variables or properties, even as own properties of the inputs. + * Replaces (and extends) the library's own denylist, as our visitors override the ones applying it. + */ +export const UNSAFE_KEYS = new Set([ + '__defineGetter__', + '__defineSetter__', + '__lookupGetter__', + '__lookupSetter__', + '__proto__', + 'constructor', + 'prototype', +]); + +/** + * Thrown when an expression uses something the restrictions don't allow. + */ +export class UnsafeExpressionError extends ExpressionError {} + +/** + * Create an error pointing at the node, so that editors can highlight it. + */ +export function unsupported(node: AnyNode, message: string): UnsafeExpressionError { + return new UnsafeExpressionError(message, node.loc?.start); +} + +/** + * Parameter names of an arrow function, if it has the shape allowed for callbacks. + */ +export function getCallbackParams(node: ArrowFunctionExpression): string[] { + if (node.async) { + throw unsupported(node, 'Async functions are not supported'); + } + if (!node.expression) { + throw unsupported(node, 'Only single-expression arrow functions are supported'); + } + + return node.params.map((param) => { + if (param.type !== 'Identifier' || UNSAFE_KEYS.has(param.name)) { + throw unsupported(param, 'Only simple arrow function parameters are supported'); + } + return param.name; + }); +} diff --git a/packages/expr/src/restricted/validate.ts b/packages/expr/src/restricted/validate.ts new file mode 100644 index 000000000..896b5d2ec --- /dev/null +++ b/packages/expr/src/restricted/validate.ts @@ -0,0 +1,137 @@ +import type { AnyNode, CallExpression, Identifier } from 'acorn'; + +import { + ALLOWED_METHODS, + CALLBACK_METHODS, + UNSAFE_KEYS, + getCallbackParams, + unsupported, +} from './rules'; + +/** + * Check an expression against the restrictions without evaluating it, so that editors can report + * violations. Rules depending on values (receiver types, inherited properties, limits) are only + * enforced by `evaluateRestricted`. + */ +export function validateExpression(node: AnyNode): void { + switch (node.type) { + case 'Literal': + if (node.regex || typeof node.value === 'bigint') { + throw unsupported( + node, + 'Regular expressions and BigInt literals are not supported' + ); + } + return; + case 'Identifier': + return assertSafeKey(node.name, node); + case 'TemplateLiteral': + return node.expressions.forEach(validateExpression); + case 'ArrayExpression': + for (const element of node.elements) { + if (element) { + validateExpression( + element.type === 'SpreadElement' ? element.argument : element + ); + } + } + return; + case 'UnaryExpression': + if (node.operator === 'delete') { + throw unsupported(node, '"delete" is not supported'); + } + return validateExpression(node.argument); + case 'BinaryExpression': + case 'LogicalExpression': + validateExpression(node.left); + return validateExpression(node.right); + case 'ConditionalExpression': + validateExpression(node.test); + validateExpression(node.consequent); + return validateExpression(node.alternate); + case 'SequenceExpression': + return node.expressions.forEach(validateExpression); + case 'MemberExpression': + validateExpression(node.object); + if (!node.computed) { + return assertSafeKey((node.property as Identifier).name, node.property); + } + if (node.property.type === 'Literal') { + return assertSafeKey(String(node.property.value), node.property); + } + if (node.property.type === 'ArrayExpression') { + throw unsupported(node.property, 'Property keys must be strings or numbers'); + } + return validateExpression(node.property); + case 'CallExpression': + return validateCall(node); + case 'ArrowFunctionExpression': + throw unsupported(node, 'Arrow functions are only supported as callbacks'); + case 'ObjectExpression': + throw unsupported(node, 'Object literals are not supported'); + case 'AssignmentExpression': + // `a =~ /regex/` parses as an assignment of `~/regex/`. + throw unsupported( + node, + node.right.type === 'UnaryExpression' && node.right.operator === '~' + ? 'The "=~" regex operator is not supported' + : 'Assignments are not supported' + ); + default: + throw unsupported( + node, + UNSUPPORTED_SYNTAX[node.type] ?? `${node.type} is not supported` + ); + } +} + +/** + * Errors shown to authors for rejected syntax. + */ +const UNSUPPORTED_SYNTAX: Partial> = { + AwaitExpression: '"await" is not supported', + ChainExpression: 'Optional chaining ("?.") is not supported', + ClassExpression: 'Classes are not supported', + FunctionExpression: 'Functions are not supported', + ImportExpression: '"import" is not supported', + NewExpression: '"new" is not supported', + TaggedTemplateExpression: 'Tagged templates are not supported', + ThisExpression: '"this" is not supported', + UpdateExpression: '"++" and "--" are not supported', + YieldExpression: '"yield" is not supported', +}; + +function validateCall(node: CallExpression): void { + const callee = node.callee; + if (callee.type !== 'MemberExpression' || callee.computed || node.optional) { + throw unsupported(node, 'Only method calls are supported'); + } + validateExpression(callee.object); + + const method = (callee.property as Identifier).name; + if (!ALLOWED_METHODS.string.has(method) && !ALLOWED_METHODS.array.has(method)) { + throw unsupported(callee.property, `Method "${method}" is not supported`); + } + + if (CALLBACK_METHODS.has(method)) { + const [callback] = node.arguments; + if (node.arguments.length !== 1 || callback?.type !== 'ArrowFunctionExpression') { + throw unsupported(node, `"${method}" only accepts an arrow function`); + } + getCallbackParams(callback); + return validateExpression(callback.body); + } + + for (const arg of node.arguments) { + if (arg.type === 'SpreadElement') { + throw unsupported(arg, 'Spread arguments are not supported'); + } + validateExpression(arg); + } +} + +function assertSafeKey(key: string, node: AnyNode): void { + if (UNSAFE_KEYS.has(key)) { + throw unsupported(node, `"${key}" is not accessible`); + } +} diff --git a/packages/expr/src/runtime.ts b/packages/expr/src/runtime.ts index 2e9ca1fec..54dbdebbf 100644 --- a/packages/expr/src/runtime.ts +++ b/packages/expr/src/runtime.ts @@ -11,10 +11,11 @@ import { import { parse as parseLoose } from 'acorn-loose'; import escodegen from 'escodegen'; import evalESTreeExpr from 'eval-estree-expression'; -const { evaluate, variables } = evalESTreeExpr; +const { variables } = evalESTreeExpr; import { AutoComplete } from './autocomplete'; import { ExpressionError } from './errors'; +import { UnsafeExpressionError, evaluateRestricted, validateExpression } from './restricted'; import type { SymbolsTable } from './symbols'; import type { TemplatePart } from './template'; import { parseTemplate as parseTemplateParts } from './template'; @@ -39,6 +40,7 @@ export class ExpressionRuntime { /** * Evaluates an expression based on the given inputs/context. + * Throws an `ExpressionError` for invalid expressions, including those the restrictions don't allow. */ public evaluate(expr: string, inputs: object): unknown { try { @@ -48,12 +50,12 @@ export class ExpressionRuntime { throw new ExpressionError('Invalid nodes found when parsing'); } - return evaluate.sync(parsed.result, inputs, { - functions: true, - withMembers: true, - generate: escodegen.generate, - }); + return evaluateRestricted(parsed.result, inputs); } catch (error) { + // Keep the location, so that editors can point at the offending token. + if (error instanceof UnsafeExpressionError) { + throw error; + } throw error instanceof Error ? new ExpressionError(error.message) : new ExpressionError('Unexpected error'); @@ -213,6 +215,11 @@ export class ExpressionRuntime { // Return information on the other nodes as invalid nodes const invalidNodes = ast.body.filter(filterOutModuleDeclarationStatement); + // Loose parsing serves autocomplete on partial expressions, which are expected to be incomplete. + if (!options.loose) { + validateRestrictedExpression(expr, statement.expression); + } + return { result: statement.expression, invalidNodes, @@ -265,6 +272,25 @@ function createExpressionErrorFromSyntaxError( return new ExpressionError(errorMessage, loc, token); } + +/** + * Validate an expression against the restrictions, pointing the error at the offending token for editors. + */ +function validateRestrictedExpression(code: string, expression: Expression): void { + try { + validateExpression(expression); + } catch (error) { + if (error instanceof UnsafeExpressionError && error.location) { + throw new UnsafeExpressionError( + error.message, + error.location, + getTokenAtLoc(code, error.location) + ); + } + throw error; + } +} + function getTokenAtLoc(code: string, errorLoc: Position): Token | undefined { const tokens = tokenizer(code, { ecmaVersion: 'latest', diff --git a/packages/expr/types/eval-estree-expression.d.ts b/packages/expr/types/eval-estree-expression.d.ts index 6e542ca21..236a51b72 100644 --- a/packages/expr/types/eval-estree-expression.d.ts +++ b/packages/expr/types/eval-estree-expression.d.ts @@ -27,6 +27,10 @@ declare module 'eval-estree-expression' { * Used with the variables method to return nested variables (e.g., variables with dot notation, like foo.bar.baz). Default: undefined */ withMembers?: boolean; + /** + * Custom visitors by node type, taking precedence over the built-in ones. Called with the evaluator as `this`. + */ + visitors?: Record; } /**