Skip to content

Commit ebf0f6f

Browse files
LucasLefevreVincentSchippefilt
LucasLefevre
authored and
VincentSchippefilt
committed
[IMP] compiler: force validated strings
The formula compiler builds JS source by string concatenation and feeds it to `new Function(...)`. Several of those strings came from user input (function names, operator symbols), so any value that slipped past the parser's validation could end up executed as code. This commit introduces a `JsString` branded type plus a `jsStr` tagged-template helper: generated code can only be assembled from values explicitly marked trusted, and untrusted strings must go through `dangerouslyCreateJsStr`, which makes the trust decision auditable. closes #8694 Task: 6185314 Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
1 parent d98b43e commit ebf0f6f

4 files changed

Lines changed: 178 additions & 51 deletions

File tree

‎src/formulas/code_builder.ts‎

Lines changed: 73 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,65 +2,117 @@
22
* Block of code that produces a value.
33
*/
44
export interface FunctionCode {
5-
readonly returnExpression: string;
5+
readonly returnExpression: JsString;
66
/**
77
* Return the same function code but with the return expression assigned to a variable.
88
*/
99
assignResultToVariable(): FunctionCode;
1010
}
1111

12+
class JsString extends String {
13+
// `brand` makes a plain `string` not assignable to `JsString`.
14+
// code.append("return 1;") // type error
15+
// code.append(jsStr`return 1;`) // ok
16+
private declare brand: never;
17+
}
18+
19+
type SafeJsValue = JsString | number | boolean | (JsString | number | boolean)[];
20+
21+
/**
22+
* Creates a JsString from a raw string, bypassing the template string interpolation checks.
23+
* This can lead to security vulnerabilities if the string is not trusted!
24+
*/
25+
export function dangerouslyCreateJsStr(trustedStr: string): JsString {
26+
return new JsString(trustedStr);
27+
}
28+
29+
/**
30+
* Creates a JsString from a template string, ensuring that all interpolated values are safe.
31+
*/
32+
export function jsStr(strings: TemplateStringsArray, ...values: SafeJsValue[]): JsString {
33+
let str = "";
34+
for (let i = 0; i < strings.length; i++) {
35+
const value = values[i];
36+
if (i >= values.length) {
37+
str += strings[i];
38+
} else if (isSafeJsValue(value)) {
39+
str += strings[i] + value;
40+
} else if (Array.isArray(value) && value.every(isSafeJsValue)) {
41+
// same behavior as array interpolation in template strings
42+
str += strings[i] + value.map((v) => v.toString()).join(",");
43+
} else {
44+
throw new Error(`Invalid interpolated value at index ${i}: ${value}`);
45+
}
46+
}
47+
return dangerouslyCreateJsStr(str);
48+
}
49+
50+
function isSafeJsValue(value: unknown): boolean {
51+
return value instanceof JsString || typeof value === "number" || typeof value === "boolean";
52+
}
53+
1254
export class FunctionCodeBuilder {
13-
private code: string = "";
55+
private code: JsString[] = [];
1456

1557
constructor(private scope: Scope = new Scope()) {}
1658

17-
append(...lines: (string | FunctionCode)[]) {
18-
this.code += lines.map((line) => line.toString()).join("\n") + "\n";
59+
append(...lines: (JsString | FunctionCode)[]) {
60+
for (const line of lines) {
61+
if (line instanceof FunctionCodeImpl) {
62+
this.code.push(...line.code);
63+
} else if (line instanceof JsString) {
64+
this.code.push(line);
65+
} else {
66+
throw new Error(`Invalid line: ${line}`);
67+
}
68+
}
1969
}
2070

21-
return(expression: string): FunctionCode {
71+
return(expression: JsString): FunctionCode {
72+
if (!isSafeJsValue(expression)) {
73+
throw new Error(`Expected JsString, got ${expression}`);
74+
}
2275
return new FunctionCodeImpl(this.scope, this.code, expression);
2376
}
2477

2578
toString(): string {
26-
return indentCode(this.code);
79+
return indentCode(this.code.join("\n"));
2780
}
2881
}
2982

3083
class FunctionCodeImpl implements FunctionCode {
31-
private readonly code: string;
32-
constructor(private readonly scope: Scope, code: string, readonly returnExpression: string) {
33-
this.code = indentCode(code);
34-
}
35-
36-
toString(): string {
37-
return this.code;
38-
}
84+
constructor(
85+
private readonly scope: Scope,
86+
readonly code: JsString[],
87+
readonly returnExpression: JsString
88+
) {}
3989

4090
assignResultToVariable(): FunctionCode {
4191
if (this.scope.isAlreadyDeclared(this.returnExpression)) {
4292
return this;
4393
}
4494
const variableName = this.scope.nextVariableName();
4595
const code = new FunctionCodeBuilder(this.scope);
46-
code.append(this.code);
47-
code.append(`const ${variableName} = ${this.returnExpression};`);
96+
code.append(...this.code);
97+
code.append(jsStr`const ${variableName} = ${this.returnExpression};`);
4898
return code.return(variableName);
4999
}
50100
}
51101

52102
export class Scope {
53103
private nextId = 1;
104+
// use string instead of JsString because Set uses reference equality and
105+
// we want to consider two JsString with the same content as the same variable
54106
private declaredVariables: Set<string> = new Set();
55107

56-
nextVariableName(): string {
57-
const name = `_${this.nextId++}`;
58-
this.declaredVariables.add(name);
108+
nextVariableName(): JsString {
109+
const name = jsStr`_${this.nextId++}`;
110+
this.declaredVariables.add(name.toString());
59111
return name;
60112
}
61113

62-
isAlreadyDeclared(name: string): boolean {
63-
return this.declaredVariables.has(name);
114+
isAlreadyDeclared(name: JsString): boolean {
115+
return this.declaredVariables.has(name.toString());
64116
}
65117
}
66118

‎src/formulas/compiler.ts‎

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { concat, unquote } from "../helpers/misc";
55
import { parseNumber } from "../helpers/numbers";
66
import { _t } from "../translation";
77
import { CoreGetters } from "../types/core_getters";
8-
import { BadExpressionError, UnknownFunctionError } from "../types/errors";
8+
import { BadExpressionError, EvaluationError, UnknownFunctionError } from "../types/errors";
99
import { DEFAULT_LOCALE } from "../types/locale";
1010
import {
1111
ApplyRangeChange,
@@ -16,7 +16,13 @@ import {
1616
UID,
1717
} from "../types/misc";
1818
import { Range, RangeStringOptions } from "../types/range";
19-
import { FunctionCode, FunctionCodeBuilder, Scope } from "./code_builder";
19+
import {
20+
dangerouslyCreateJsStr,
21+
FunctionCode,
22+
FunctionCodeBuilder,
23+
jsStr,
24+
Scope,
25+
} from "./code_builder";
2026
import { AST, ASTFuncall, iterateAstNodes, parseTokens } from "./parser";
2127
import { rangeTokenize } from "./range_tokenizer";
2228
import { Token } from "./tokenizer";
@@ -364,6 +370,9 @@ function compileTokens(tokens: Token[]): ICompiledFormula {
364370
try {
365371
return compileTokensOrThrow(tokens);
366372
} catch (error) {
373+
if (!(error instanceof EvaluationError)) {
374+
throw error;
375+
}
367376
return {
368377
tokens,
369378
literalValues: { numbers: [], strings: [] },
@@ -398,7 +407,7 @@ function compileTokensOrThrow(tokens: Token[]): ICompiledFormula {
398407
const compiledAST = compileAST(ast);
399408
const code = new FunctionCodeBuilder();
400409
code.append(compiledAST);
401-
code.append(`return ${compiledAST.returnExpression};`);
410+
code.append(jsStr`return ${compiledAST.returnExpression};`);
402411
const baseFunction = new Function(
403412
"deps", // the dependencies in the current formula
404413
"ref", // a function to access a certain dependency at a given index
@@ -455,25 +464,31 @@ function compileTokensOrThrow(tokens: Token[]): ICompiledFormula {
455464
function compileAST(ast: AST, hasRange = false): FunctionCode {
456465
const code = new FunctionCodeBuilder(scope);
457466
if (ast.debug) {
458-
code.append("debugger;");
459-
code.append(`ctx["debug"] = true;`);
467+
code.append(jsStr`debugger;`);
468+
code.append(jsStr`ctx["debug"] = true;`);
460469
}
461470
switch (ast.type) {
462471
case "BOOLEAN":
463-
return code.return(`{ value: ${ast.value} }`);
472+
return code.return(jsStr`{ value: ${ast.value} }`);
464473
case "NUMBER":
465-
return code.return(`this.literalValues.numbers[${numberCount++}]`);
474+
return code.return(jsStr`this.literalValues.numbers[${numberCount++}]`);
466475
case "STRING":
467-
return code.return(`this.literalValues.strings[${stringCount++}]`);
476+
return code.return(jsStr`this.literalValues.strings[${stringCount++}]`);
468477
case "REFERENCE":
469478
return code.return(
470-
`${ast.value.includes(":") || hasRange ? `range` : `ref`}(deps[${dependencyCount++}])`
479+
jsStr`${
480+
ast.value.includes(":") || hasRange ? jsStr`range` : jsStr`ref`
481+
}(deps[${dependencyCount++}])`
471482
);
472483
case "FUNCALL":
473484
const args = compileFunctionArgs(ast).map((arg) => arg.assignResultToVariable());
474485
code.append(...args);
475486
const fnName = ast.value.toUpperCase();
476-
return code.return(`ctx['${fnName}'](${args.map((arg) => arg.returnExpression)})`);
487+
if (!Object.hasOwn(functions, fnName)) {
488+
throw new Error(`Unknown function: "${fnName}"`);
489+
}
490+
const jsFnName = dangerouslyCreateJsStr(fnName); // validated with known functions
491+
return code.return(jsStr`ctx['${jsFnName}'](${args.map((arg) => arg.returnExpression)})`);
477492
case "ARRAY": {
478493
// a literal array is compiled into function calls
479494
const arrayFunctionCall: ASTFuncall = {
@@ -492,26 +507,32 @@ function compileTokensOrThrow(tokens: Token[]): ICompiledFormula {
492507
return compileAST(arrayFunctionCall);
493508
}
494509
case "UNARY_OPERATION": {
495-
const fnName = UNARY_OPERATOR_MAP[ast.value];
510+
if (!Object.hasOwn(UNARY_OPERATOR_MAP, ast.value)) {
511+
throw new Error(`Unknown operator: "${ast.value}"`);
512+
}
513+
const fnName = dangerouslyCreateJsStr(UNARY_OPERATOR_MAP[ast.value]); // validated with known operators
496514
const operand = compileAST(ast.operand, ast.value === "#").assignResultToVariable(); // hasRange is true to avoid vectorization of SPILLED.RANGE
497515
code.append(operand);
498-
return code.return(`ctx['${fnName}'](${operand.returnExpression})`);
516+
return code.return(jsStr`ctx['${fnName}'](${operand.returnExpression})`);
499517
}
500518
case "BIN_OPERATION": {
501-
const fnName = OPERATOR_MAP[ast.value];
519+
if (!Object.hasOwn(OPERATOR_MAP, ast.value)) {
520+
throw new Error(`Unknown operator: "${ast.value}"`);
521+
}
522+
const fnName = dangerouslyCreateJsStr(OPERATOR_MAP[ast.value]); // validated with known operators
502523
const left = compileAST(ast.left, false).assignResultToVariable();
503524
const right = compileAST(ast.right, false).assignResultToVariable();
504525
code.append(left);
505526
code.append(right);
506527
return code.return(
507-
`ctx['${fnName}'](${left.returnExpression}, ${right.returnExpression})`
528+
jsStr`ctx['${fnName}'](${left.returnExpression}, ${right.returnExpression})`
508529
);
509530
}
510531
case "SYMBOL":
511532
const symbolIndex = symbols.indexOf(ast.value);
512-
return code.return(`getSymbolValue(this.symbols[${symbolIndex}], ${hasRange})`);
533+
return code.return(jsStr`getSymbolValue(this.symbols[${symbolIndex}], ${hasRange})`);
513534
case "EMPTY":
514-
return code.return("undefined");
535+
return code.return(jsStr`undefined`);
515536
}
516537
}
517538
}

‎tests/collaborative/collaborative_monkey_party.test.ts‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ import seedrandom from "seedrandom";
22
import {
33
Command,
44
CoreCommand,
5-
Model,
6-
UnboundedZone,
75
isPositionDependent,
86
isRangeDependant,
97
isSheetDependent,
108
isTargetDependent,
9+
Model,
10+
UnboundedZone,
1111
} from "../../src";
12-
import { FunctionCodeBuilder } from "../../src/formulas/code_builder";
12+
import { dangerouslyCreateJsStr, FunctionCodeBuilder } from "../../src/formulas/code_builder";
1313
import { deepCopy, deepEquals, range } from "../../src/helpers/misc";
1414
import { reorderZone } from "../../src/helpers/zones";
1515
import { MockTransportService } from "../__mocks__/transport_service";
@@ -111,37 +111,40 @@ function assignUser(commands: CoreCommand[], users: Model[]): UserAction[] {
111111
}
112112

113113
function actionsToTestCode(testTitle: string, actions: UserAction[][]) {
114-
const code = new FunctionCodeBuilder();
115-
code.append(`test("${testTitle}", () => {`);
116-
code.append("const { network, alice, bob, charlie } = setupCollaborativeEnv();");
114+
const code: string[] = [];
115+
code.push(`test("${testTitle}", () => {`);
116+
code.push("const { network, alice, bob, charlie } = setupCollaborativeEnv();");
117117
for (const commandGroup of actions) {
118118
if (commandGroup.length === 1) {
119119
appendCommand(code, commandGroup[0]);
120120
} else {
121-
code.append("network.concurrent(() => {");
121+
code.push("network.concurrent(() => {");
122122
for (const action of commandGroup) {
123123
appendCommand(code, action);
124124
}
125-
code.append("});");
125+
code.push("});");
126126
}
127127
}
128-
code.append(
128+
code.push(
129129
"expect([alice, bob, charlie]).toHaveSynchronizedEvaluation();",
130130
"expect([alice, bob, charlie]).toHaveSynchronizedExportedData();",
131131
"});"
132132
);
133-
return code.toString();
133+
// format the code with the code builder
134+
const builder = new FunctionCodeBuilder();
135+
builder.append(...code.map(dangerouslyCreateJsStr));
136+
return builder.toString();
134137
}
135138

136-
function appendCommand(code: FunctionCodeBuilder, { user, command }: UserAction) {
139+
function appendCommand(code: string[], { user, command }: UserAction) {
137140
const userName = user.getters.getCurrentClient().name.toLowerCase();
138141
if (command.type === "REQUEST_UNDO") {
139-
code.append(`undo(${userName});`);
142+
code.push(`undo(${userName});`);
140143
} else if (command.type === "REQUEST_REDO") {
141-
code.append(`redo(${userName});`);
144+
code.push(`redo(${userName});`);
142145
} else {
143146
const cmdPayload = JSON.stringify({ ...command, type: undefined });
144-
code.append(`${userName}.dispatch("${command.type}", ${cmdPayload});`);
147+
code.push(`${userName}.dispatch("${command.type}", ${cmdPayload});`);
145148
}
146149
}
147150

‎tests/helpers/code_builder.test.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import { FunctionCodeBuilder, jsStr } from "../../src/formulas/code_builder";
2+
3+
describe("code builder", () => {
4+
test("can inject safe values", () => {
5+
expect(jsStr`const a = ${jsStr`'hello'`};`.toString()).toBe(`const a = 'hello';`);
6+
expect(jsStr`const a = ${jsStr`123`};`.toString()).toBe(`const a = 123;`);
7+
expect(jsStr`const a = ${123};`.toString()).toBe(`const a = 123;`);
8+
expect(jsStr`const a = ${true};`.toString()).toBe(`const a = true;`);
9+
expect(jsStr`const a = ${false};`.toString()).toBe(`const a = false;`);
10+
});
11+
12+
test("can inject array of safe values", () => {
13+
expect(jsStr`const a = [${[jsStr`'hello'`, jsStr`123`, 123, true, false]}];`.toString()).toBe(
14+
`const a = ['hello',123,123,true,false];`
15+
);
16+
});
17+
18+
test("cannot inject unsafe values", () => {
19+
// @ts-expect-error
20+
expect(() => jsStr`const a = ${"primitive string"};`).toThrow();
21+
// @ts-expect-error
22+
expect(() => jsStr`const a = ${{ a: 1 }};`).toThrow();
23+
// @ts-expect-error
24+
expect(() => jsStr`const a = ${new String("string")};`).toThrow();
25+
});
26+
27+
test("can inject safe values", () => {
28+
const code = new FunctionCodeBuilder();
29+
code.append(jsStr`const a = 123;`);
30+
expect(code.toString()).toBe("const a = 123;");
31+
expect(code.return(jsStr`a`).returnExpression.toString()).toBe("a");
32+
expect(
33+
code
34+
.return(jsStr`a`)
35+
.assignResultToVariable()
36+
.returnExpression.toString()
37+
).toBe("_1");
38+
});
39+
40+
test("cannot add unsafe values", () => {
41+
const code = new FunctionCodeBuilder();
42+
// @ts-expect-error
43+
expect(() => code.append("const a = 1;")).toThrow();
44+
// @ts-expect-error
45+
expect(() => code.append({ a: 1 })).toThrow();
46+
// @ts-expect-error
47+
expect(() => code.append(new String("string"))).toThrow();
48+
// @ts-expect-error
49+
expect(() => code.return(new String("string"))).toThrow();
50+
});
51+
});

0 commit comments

Comments
 (0)