Skip to content

Commit ee0b8c2

Browse files
ANGkeithANGkeith
authored
fix: handle more edge cases in evaluateRuleIf (#1340)
1 parent 4bca73f commit ee0b8c2

File tree

3 files changed

+174
-16
lines changed

3 files changed

+174
-16
lines changed

‎src/utils.ts‎

Lines changed: 62 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -101,11 +101,7 @@ export class Utils {
101101
} else {
102102
const name = var1 || var2;
103103
assert(name, "unexpected unset capture group");
104-
let value = `${expandWith.variable(name)}`;
105-
if (value.startsWith("\"/") && value.endsWith("/\"")) {
106-
value = value.substring(1).slice(0, -1);
107-
}
108-
return `${value}`;
104+
return `${expandWith.variable(name)}`;
109105
}
110106
}
111107
);
@@ -205,20 +201,74 @@ export class Utils {
205201
unescape: JSON.stringify("$"),
206202
variable: (name) => JSON.stringify(envs[name] ?? null).replaceAll("\\\\", "\\"),
207203
});
204+
const expandedEvalStr = evalStr;
205+
206+
// Scenario when RHS is a <regex>
207+
// https://regexr.com/85sjo
208+
const pattern1 = /\s*(?<operator>(?:=~)|(?:!~))\s*(?<rhs>\/.*?\/)(?<flags>[igmsuy]*)(\s|$|\))/g;
209+
evalStr = evalStr.replace(pattern1, (_, operator, rhs, flags, remainingTokens) => {
210+
let _operator;
211+
switch (operator) {
212+
case "=~":
213+
_operator = "!=";
214+
break;
215+
case "!~":
216+
_operator = "==";
217+
break;
218+
default:
219+
throw operator;
220+
}
221+
return `.match(${rhs}${flags})${remainingTokens} ${_operator} null`;
222+
});
208223

209-
// Convert =~ to match function
210-
evalStr = evalStr.replace(/\s*=~\s*(\/.*?\/[igmsuy]*)(?:\s|$)/g, ".match($1) != null");
211-
evalStr = evalStr.replace(/\s*=~\s(.+?)(\)*?)(?:\s|$)/g, ".match(new RegExp($1)) != null$2"); // Without forward slashes
224+
// Scenario when RHS is surrounded by double-quotes
225+
// https://regexr.com/85t0g
226+
const pattern2 = /\s*(?<operator>(?:=~)|(?:!~))\s*"(?<rhs>[^"\\]*(?:\\.[^"\\]*)*)"/g;
227+
evalStr = evalStr.replace(pattern2, (_, operator, rhs) => {
228+
let _operator;
229+
switch (operator) {
230+
case "=~":
231+
_operator = "!=";
232+
break;
233+
case "!~":
234+
_operator = "==";
235+
break;
236+
default:
237+
throw operator;
238+
}
212239

213-
// Convert !~ to match function
214-
evalStr = evalStr.replace(/\s*!~\s*(\/.*?\/[igmsuy]*)(?:\s|$)/g, ".match($1) == null");
215-
evalStr = evalStr.replace(/\s*!~\s(.+?)(\)*?)(?:\s|$)/g, ".match(new RegExp($1)) == null$2"); // Without forward slashes
240+
if (!/\/(.*)\/([\w]*)/.test(rhs)) {
241+
throw Error(`RHS (${rhs}) must be a regex pattern. Do not rely on this behavior!
242+
Refer to https://docs.gitlab.com/ee/ci/jobs/job_rules.html#unexpected-behavior-from-regular-expression-matching-with- for more info...`);
243+
}
244+
const regex = /\/(?<pattern>.*)\/(?<flags>[igmsuy]*)/;
245+
const _rhs = rhs.replace(regex, (_: string, pattern: string, flags: string) => {
246+
const _pattern = pattern.replace(/(?<!\\)\//g, "\\/"); // escape potentially unescaped `/` that's in the pattern
247+
return `/${_pattern}/${flags}`;
248+
});
249+
return `.match(new RegExp(${_rhs})) ${_operator} null`;
250+
});
216251

217252
// Convert all null.match functions to false
218253
evalStr = evalStr.replace(/null.match\(.+?\) != null/g, "false");
219254
evalStr = evalStr.replace(/null.match\(.+?\) == null/g, "false");
220255

221-
return Boolean(eval(evalStr));
256+
let res;
257+
try {
258+
res = eval(evalStr);
259+
} catch (err) {
260+
console.log(`
261+
Error attempting to evaluate the following rules:
262+
rules:
263+
- if: '${expandedEvalStr}'
264+
as
265+
\`\`\`javascript
266+
${evalStr}
267+
\`\`\`
268+
`);
269+
throw err;
270+
}
271+
return Boolean(res);
222272
}
223273

224274
static evaluateRuleExist (cwd: string, ruleExists: string[] | undefined): boolean {

‎tests/rules-regex.test.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
2+
import {Utils} from "../src/utils";
3+
import {GitData} from "../src/git-data";
4+
import {WriteStreamsMock} from "../src/write-streams";
5+
6+
let writeStreams;
7+
let gitData: GitData;
8+
//
9+
beforeEach(async () => {
10+
writeStreams = new WriteStreamsMock();
11+
gitData = await GitData.init("tests", writeStreams);
12+
jest.clearAllMocks();
13+
});
14+
15+
/* eslint-disable @typescript-eslint/quotes */
16+
const tests = [
17+
{
18+
rule: '"foo" !~ /foo/',
19+
jsExpression: '"foo".match(/foo/) == null',
20+
evalResult: false,
21+
},
22+
{
23+
rule: '"foo" =~ /foo/',
24+
jsExpression: '"foo".match(/foo/) != null',
25+
evalResult: true,
26+
},
27+
{
28+
rule: '"foo"=~ /foo/',
29+
jsExpression: '"foo".match(/foo/) != null',
30+
evalResult: true,
31+
},
32+
{
33+
rule: '"foo"=~/foo/',
34+
jsExpression: '"foo".match(/foo/) != null',
35+
evalResult: true,
36+
},
37+
{
38+
rule: '"foo"=~ /foo/',
39+
jsExpression: '"foo".match(/foo/) != null',
40+
evalResult: true,
41+
},
42+
{
43+
rule: '"foo" =~ "/foo/"',
44+
jsExpression: '"foo".match(new RegExp(/foo/)) != null',
45+
evalResult: true,
46+
},
47+
{
48+
rule: '"test/url" =~ "/test/ur/"',
49+
jsExpression: '"test/url".match(new RegExp(/test\\/ur/)) != null',
50+
evalResult: true,
51+
},
52+
{
53+
rule: '"test/url" =~ "/test\\/ur/"',
54+
jsExpression: '"test/url".match(new RegExp(/test\\/ur/)) != null',
55+
evalResult: true,
56+
},
57+
{
58+
rule: '"test/url" =~ /test/ur/',
59+
expectErr: true,
60+
},
61+
{
62+
rule: '"master" =~ /master$/',
63+
jsExpression: '"master".match(/master$/) != null',
64+
evalResult: true,
65+
},
66+
{
67+
rule: '"23" =~ "1234"',
68+
expectErr: true,
69+
},
70+
{
71+
rule: '"23" =~ /1234/',
72+
jsExpression: '"23".match(/1234/) != null',
73+
evalResult: false,
74+
},
75+
];
76+
/* eslint-enable @typescript-eslint/quotes */
77+
78+
describe("gitlab rules regex", () => {
79+
tests.filter(t => !t.expectErr)
80+
.forEach((t) => {
81+
test(`- if: '${t.rule}'\n\t => ${t.evalResult}`, async () => {
82+
const rules = [ {if: t.rule} ];
83+
const evalSpy = jest.spyOn(global, "eval");
84+
const evaluateRuleIfSpy = jest.spyOn(Utils, "evaluateRuleIf");
85+
86+
Utils.getRulesResult({cwd: "", rules, variables: {}}, gitData);
87+
expect(evalSpy).toHaveBeenCalledWith(t.jsExpression);
88+
expect(evaluateRuleIfSpy).toHaveReturnedWith(t.evalResult);
89+
});
90+
});
91+
});
92+
93+
describe("gitlab rules regex [invalid]", () => {
94+
tests.filter(t => t.expectErr)
95+
.forEach((t) => {
96+
test(`- if: '${t.rule}'\n\t => error`, async () => {
97+
const rules = [ {if: t.rule} ];
98+
99+
try {
100+
Utils.getRulesResult({cwd: "", rules, variables: {}}, gitData);
101+
} catch (e) {
102+
return;
103+
}
104+
105+
throw new Error("Error is expected but not thrown/caught");
106+
});
107+
});
108+
});

‎tests/rules.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -264,16 +264,16 @@ test("https://github.com/firecow/gitlab-ci-local/issues/350", () => {
264264
test("https://github.com/firecow/gitlab-ci-local/issues/300", () => {
265265
let rules, rulesResult, variables;
266266
rules = [
267-
{if: "$VAR1 && (($VAR3 =~ /ci-skip-job-/ && $VAR2 =~ $VAR3) || ($VAR3 =~ /ci-skip-stage-/ && $VAR2 =~ $VAR3))", when: "manual"},
267+
{if: "$VAR1 && (($VAR2 =~ /ci-skip-job-/ && $VAR2 =~ $VAR3) || ($VAR2 =~ /ci-skip-stage-/ && $VAR2 =~ $VAR3))", when: "manual"},
268268
];
269-
variables = {VAR1: "val", VAR2: "ci-skip-job-", VAR3: "ci-skip-job-"};
269+
variables = {VAR1: "val", VAR2: "ci-skip-job-", VAR3: "/ci-skip-job-/"};
270270
rulesResult = Utils.getRulesResult({cwd: "", rules, variables}, gitData);
271271
expect(rulesResult).toEqual({when: "manual", allowFailure: false, variables: undefined});
272272

273273
rules = [
274-
{if: "$VAR1 && (($VAR3 =~ /ci-skip-job-/ && $VAR2 =~ $VAR3) || ($VAR3 =~ /ci-skip-stage-/ && $VAR2 =~ $VAR3))", when: "manual"},
274+
{if: "$VAR1 && (($VAR2 =~ /ci-skip-job-/ && $VAR2 =~ $VAR3) || ($VAR2 =~ /ci-skip-stage-/ && $VAR2 =~ $VAR3))", when: "manual"},
275275
];
276-
variables = {VAR1: "val", VAR2: "ci-skip-stage-", VAR3: "ci-skip-stage-"};
276+
variables = {VAR1: "val", VAR2: "ci-skip-stage-", VAR3: "/ci-skip-stage-/"};
277277
rulesResult = Utils.getRulesResult({cwd: "", rules, variables}, gitData);
278278
expect(rulesResult).toEqual({when: "manual", allowFailure: false, variables: undefined});
279279
});

0 commit comments

Comments
 (0)