Skip to content

Commit bde8d26

Browse files
committed
fix(rules:if): evaluate expressions via jsep AST, fix null !~ and reject \${VAR}
- Replace regex-based string manipulation in evaluateRuleIf with jsep AST parsing for correct operator precedence and cleaner expression handling - Fix: null !~ /pattern/ now returns true (undefined variable does not match, so negated check passes) instead of always returning false - Fix: reject \${VAR} curly-bracket syntax in the new evaluator, matching the guard already present in the legacy _evaluateRuleIf path
1 parent 14df5b2 commit bde8d26

3 files changed

Lines changed: 130 additions & 5 deletions

File tree

bun.lock

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
"fetch-schema": "curl https://gitlab.com/gitlab-org/gitlab/-/raw/master/app/assets/javascripts/editor/schema/ci.json -sf > src/schema.json"
2525
},
2626
"dependencies": {
27+
"@jsep-plugin/regex": "^1.0.4",
2728
"ajv": "8.x.x",
2829
"axios": "1.x.x",
2930
"base64url": "3.x.x",
@@ -37,6 +38,7 @@
3738
"fs-extra": "11.x.x",
3839
"globby": "16.x.x",
3940
"js-yaml": "4.x.x",
41+
"jsep": "^1.4.0",
4042
"jsonpointer": "5.x.x",
4143
"micromatch": "4.x.x",
4244
"object-traversal": "1.x.x",

src/utils.ts

Lines changed: 122 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import "./global.js";
22
import {RE2JS} from "re2js";
33
import chalk from "chalk-template";
4+
import jsep from "jsep";
5+
import jsepRegex from "@jsep-plugin/regex";
46
import {Job, JobRule, Need, Service} from "./job.js";
57
import {needsComplex} from "./data-expander.js";
68
import fs from "fs-extra";
@@ -16,6 +18,10 @@ import {AxiosRequestConfig} from "axios";
1618
import path from "node:path";
1719
import {Argv} from "./argv.js";
1820

21+
jsep.plugins.register(jsepRegex); // /pattern/flags literals
22+
jsep.addBinaryOp("=~", 10); // regex match operator
23+
jsep.addBinaryOp("!~", 10); // regex non-match operator
24+
1925
type RuleResultOpt = {
2026
argv: Argv;
2127
cwd: string;
@@ -213,10 +219,122 @@ export class Utils {
213219
return {when, allowFailure, variables: ruleVariable, needs: ruleNeeds};
214220
}
215221

222+
// Reconstruct the source string for an atomic (non-logical) jsep node.
223+
// Identifiers keep their name ($VAR), literals use their raw form so that
224+
// strings retain quotes and regexes retain slashes and flags.
225+
private static _nodeToAtom (node: jsep.Expression, envs: {[key: string]: string}): string {
226+
switch (node.type) {
227+
case "Identifier": {
228+
const n = node as jsep.Identifier;
229+
return n.name;
230+
// return this.expandTextWith(n.name, {
231+
// unescape: JSON.stringify("$"),
232+
// variable: (name) => JSON.stringify(envs[name] ?? null).replaceAll("\\\\", "\\"),
233+
// });
234+
}
235+
case "Literal": return (node as jsep.Literal).raw;
236+
case "BinaryExpression": {
237+
const n = node as jsep.BinaryExpression;
238+
return `${Utils._nodeToAtom(n.left, envs)} ${n.operator} ${Utils._nodeToAtom(n.right, envs)}`;
239+
}
240+
default: return "";
241+
}
242+
}
243+
244+
static stripQuotes (str: string) {
245+
if (str.length < 2) return str;
246+
const first = str[0];
247+
const last = str[str.length - 1];
248+
if ((first === "\"" && last === "\"") || (first === "'" && last === "'")) {
249+
return str.slice(1, -1);
250+
}
251+
return str;
252+
}
253+
216254
static evaluateRuleIf (ruleIf: string | undefined, envs: {[key: string]: string}): boolean {
217255
if (ruleIf === undefined) return true;
218256
assert(!/\$\{\w+\}/.test(ruleIf), chalk`rules:rule if invalid expression syntax: {blueBright ${ruleIf}}\nuse {green $VAR} not {red \${VAR\}} in rules:if`);
219257
let evalStr = ruleIf;
258+
evalStr = this.expandTextWith(evalStr, {
259+
unescape: JSON.stringify("$"),
260+
variable: (name) => JSON.stringify(envs[name] ?? null).replaceAll("\\\\", "\\"),
261+
}); // replace all $VAR by their values
262+
263+
const flagsToBinary = (flags: string): number => {
264+
let binary = 0;
265+
if (flags.includes("i")) {
266+
binary |= RE2JS.CASE_INSENSITIVE;
267+
}
268+
if (flags.includes("s")) {
269+
binary |= RE2JS.DOTALL;
270+
}
271+
if (flags.includes("m")) {
272+
binary |= RE2JS.MULTILINE;
273+
}
274+
return binary;
275+
};
276+
// jsep parses ruleIf into an AST, handling &&, ||, () and operator precedence.
277+
const walk = (node: jsep.Expression): boolean => {
278+
if (node.type === "BinaryExpression") {
279+
const n = node as jsep.BinaryExpression;
280+
if (n.operator === "&&") return walk(n.left) && walk(n.right);
281+
if (n.operator === "||") return walk(n.left) || walk(n.right);
282+
if (n.operator === "=~" || n.operator === "!~") {
283+
assert(n.left.type === "Literal", `Not a Literal: ${JSON.stringify(n.left)}`);
284+
assert(n.right.type === "Literal", `Not a Literal: ${JSON.stringify(n.right)}`);
285+
const leftStr = n.left as jsep.Literal;
286+
const rightStr = n.right as jsep.Literal;
287+
if (leftStr.value === null)
288+
return n.operator === "!~"; // null =~ /p/ → false; null !~ /p/ → true
289+
if (rightStr.value === null)
290+
return false;
291+
let regexStr: string = rightStr.raw;
292+
regexStr = this.stripQuotes(regexStr);
293+
294+
const regex = /\/(?<pattern>.*)\/(?<flags>[igmsuy]*)/;
295+
const _rhs = regexStr.replace(regex, (_: string, pattern: string, flags: string) => {
296+
const flagsBinary = flagsToBinary(flags);
297+
return `RE2JS.compile(${JSON.stringify(pattern)}, ${flagsBinary})`;
298+
});
299+
300+
const _operator = n.operator === "=~" ? "!=" : "=="; // =~ -> !=; !~ -> ==
301+
302+
const evalStr = `${leftStr.raw}.matchRE2JS(${_rhs}) ${_operator} null`;
303+
304+
let res;
305+
try {
306+
(globalThis as any).RE2JS = RE2JS;
307+
res = (0, eval)(evalStr); // indirect eval
308+
delete (globalThis as any).RE2JS;
309+
} catch (error) {
310+
console.error(error);
311+
const assertMsg = [
312+
"Error attempting to evaluate the following rules:",
313+
" rules:",
314+
` - if: '${Utils._nodeToAtom(node, envs)}'`,
315+
"as",
316+
"```javascript",
317+
`${evalStr}`,
318+
"```",
319+
];
320+
assert(false, assertMsg.join("\n"));
321+
}
322+
return Boolean(res);
323+
}
324+
}
325+
// assert (node.type === "Literal", `not a Literal: ${JSON.stringify(node)}`)
326+
const res = Utils._evaluateRuleIf(Utils._nodeToAtom(node, envs), envs);
327+
// console.log(`${JSON.stringify(node)} -> ${res}`)
328+
return res;
329+
};
330+
331+
return walk(jsep(evalStr));
332+
}
333+
334+
static _evaluateRuleIf (ruleIf: string | undefined, envs: {[key: string]: string}): boolean {
335+
if (ruleIf === undefined) return true;
336+
assert(!/\$\{\w+\}/.test(ruleIf), chalk`rules:rule if invalid expression syntax: {blueBright ${ruleIf}}\nuse {green $VAR} not {red \${VAR\}} in rules:if`);
337+
let evalStr = ruleIf;
220338

221339
const flagsToBinary = (flags: string): number => {
222340
let binary = 0;
@@ -232,7 +350,6 @@ export class Utils {
232350
return binary;
233351
};
234352

235-
// Expand all variables
236353
evalStr = this.expandTextWith(evalStr, {
237354
unescape: JSON.stringify("$"),
238355
variable: (name) => JSON.stringify(envs[name] ?? null).replaceAll("\\\\", "\\"),
@@ -246,10 +363,10 @@ export class Utils {
246363
let _operator;
247364
switch (operator) {
248365
case "=~":
249-
_operator = "!=";
366+
_operator = "!="; // Matches are found (!= null)
250367
break;
251368
case "!~":
252-
_operator = "==";
369+
_operator = "=="; // Matches are not found (== null)
253370
break;
254371
default:
255372
throw operator;
@@ -274,10 +391,10 @@ export class Utils {
274391
let _operator;
275392
switch (operator) {
276393
case "=~":
277-
_operator = "!=";
394+
_operator = "!="; // Matches are found (!= null)
278395
break;
279396
case "!~":
280-
_operator = "==";
397+
_operator = "=="; // Matches are not found (== null)
281398
break;
282399
default:
283400
throw operator;

0 commit comments

Comments
 (0)