From 48e3f09f6b87c0c2cb04f779c437609f3db018b3 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 17 Jul 2023 12:12:28 +0200 Subject: [PATCH 1/6] added method to validate regex expressions --- src/file/validator.ts | 39 +++++++++++++++++++++++++++++++++++++++ src/runner.ts | 12 ++++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/file/validator.ts b/src/file/validator.ts index b4c2328..ccae618 100644 --- a/src/file/validator.ts +++ b/src/file/validator.ts @@ -20,3 +20,42 @@ export const schema = Joi.object().keys({ .optional() .allow(null), }); + +export const isRegexValid = (regex: string): boolean => { + try { + new RegExp(regex); + return true; + } catch (e) { + console.error(e); + return false; + } +}; + +/** Evaluate if the regex expression inside a configuration are valid. + * @returns a tuple of type [boolean, string]. If the boolean is false, the string will contain an error message + * @example + * const [result, error] = validateRegularExpressions(myConfig); + * if (!result) { + * throw new Error(error); + * } else { + * runExpression(myConfig); + * } + */ +export const validateRegularExpressions = (config: ConfigurationFile): [true] | [false, string] => { + config.rules.forEach((rule) => { + rule.condition.include.forEach((condition) => { + if (!isRegexValid(condition)) { + return [false, `Include condition '${condition}' is not a valid regex`]; + } + }); + if (rule.condition.exclude) { + rule.condition.exclude.forEach((condition) => { + if (!isRegexValid(condition)) { + return [false, `Exclude condition '${condition}' is not a valid regex`]; + } + }); + } + }); + + return [true]; +}; diff --git a/src/runner.ts b/src/runner.ts index 76626e2..2b30599 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -3,7 +3,7 @@ import { parse } from "yaml"; import { Inputs } from "."; import { ConfigurationFile } from "./file/types"; -import { schema } from "./file/validator"; +import { schema, validateRegularExpressions } from "./file/validator"; import { PullRequestApi } from "./github/pullRequest"; import { ActionLogger } from "./github/types"; @@ -22,7 +22,15 @@ export class ActionRunner { this.logger.info(`Obtained config at ${configLocation}`); - return validate(config, schema, { message: "Configuration file is invalid" }); + const configFile = validate(config, schema, { message: "Configuration file is invalid" }); + + const [result, error] = validateRegularExpressions(configFile); + if (!result) { + this.logger.error(error); + throw new Error("Regular expression is invalid. Check the logs"); + } + + return configFile; } async runAction(inputs: Omit): Promise { From 033840d4827882ea7518f5f37f5c8046c7be1cc6 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 17 Jul 2023 12:33:19 +0200 Subject: [PATCH 2/6] fixed order of the validator This will now have it in a more ordered way --- src/file/validator.ts | 39 ++++++++++++++++++++++----------------- src/runner.ts | 2 +- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/file/validator.ts b/src/file/validator.ts index ccae618..19b9af2 100644 --- a/src/file/validator.ts +++ b/src/file/validator.ts @@ -1,5 +1,6 @@ import Joi from "joi"; +import { ActionLogger } from "../github/types"; import { ConfigurationFile, Rule } from "./types"; const ruleSchema = Joi.object().keys({ @@ -21,16 +22,6 @@ export const schema = Joi.object().keys({ .allow(null), }); -export const isRegexValid = (regex: string): boolean => { - try { - new RegExp(regex); - return true; - } catch (e) { - console.error(e); - return false; - } -}; - /** Evaluate if the regex expression inside a configuration are valid. * @returns a tuple of type [boolean, string]. If the boolean is false, the string will contain an error message * @example @@ -41,21 +32,35 @@ export const isRegexValid = (regex: string): boolean => { * runExpression(myConfig); * } */ -export const validateRegularExpressions = (config: ConfigurationFile): [true] | [false, string] => { - config.rules.forEach((rule) => { - rule.condition.include.forEach((condition) => { +export const validateRegularExpressions = ( + config: ConfigurationFile, + logger: ActionLogger, +): [true] | [false, string] => { + /** Regex evaluator */ + const isRegexValid = (regex: string): boolean => { + try { + new RegExp(regex); + return true; + } catch (e) { + logger.error(e as Error); + return false; + } + }; + + for (const rule of config.rules) { + for (const condition of rule.condition.include) { if (!isRegexValid(condition)) { return [false, `Include condition '${condition}' is not a valid regex`]; } - }); + } if (rule.condition.exclude) { - rule.condition.exclude.forEach((condition) => { + for (const condition of rule.condition.exclude) { if (!isRegexValid(condition)) { return [false, `Exclude condition '${condition}' is not a valid regex`]; } - }); + } } - }); + } return [true]; }; diff --git a/src/runner.ts b/src/runner.ts index 2b30599..428b7c8 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -24,7 +24,7 @@ export class ActionRunner { const configFile = validate(config, schema, { message: "Configuration file is invalid" }); - const [result, error] = validateRegularExpressions(configFile); + const [result, error] = validateRegularExpressions(configFile, this.logger); if (!result) { this.logger.error(error); throw new Error("Regular expression is invalid. Check the logs"); From ec6dab02cf354a938d21b3913818f027053bfe83 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 17 Jul 2023 12:33:37 +0200 Subject: [PATCH 3/6] fixed wrong evaluation in tests --- src/test/runner/config.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/runner/config.test.ts b/src/test/runner/config.test.ts index abff0ef..f7f609b 100644 --- a/src/test/runner/config.test.ts +++ b/src/test/runner/config.test.ts @@ -11,7 +11,7 @@ import { TestLogger } from "../logger"; describe("Config Parsing", () => { let api: MockProxy; let runner: ActionRunner; - let logger: ActionLogger; + let logger: TestLogger; beforeEach(() => { logger = new TestLogger(); api = mock(); @@ -28,7 +28,7 @@ describe("Config Parsing", () => { - 'example' `); const config = await runner.getConfigFile(""); - expect(config.preventReviewRequests).toBeNull; + expect(config.preventReviewRequests).toBeUndefined(); }); test("should call GitHub api with path", async () => { @@ -86,7 +86,7 @@ describe("Config Parsing", () => { - 'example' `); const config = await runner.getConfigFile(""); - expect(config.preventReviewRequests).toBeNull; + expect(config.preventReviewRequests).toBeUndefined(); }); }); @@ -137,7 +137,7 @@ describe("Config Parsing", () => { - '.*' `); const config = await runner.getConfigFile(""); - expect(config.rules[0].condition.exclude).toBeNull; + expect(config.rules[0].condition.exclude).toBeUndefined(); }); }); }); From ec489dd6c8688d21fe8a740393045cffc80b6aed Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 17 Jul 2023 12:33:54 +0200 Subject: [PATCH 4/6] added test to verify the validator works --- src/test/runner/config.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/test/runner/config.test.ts b/src/test/runner/config.test.ts index f7f609b..4a4044a 100644 --- a/src/test/runner/config.test.ts +++ b/src/test/runner/config.test.ts @@ -36,6 +36,21 @@ describe("Config Parsing", () => { expect(api.getConfigFile).toHaveBeenCalledWith("example-location"); }); + describe("regular expressions validator", () => { + test("should fail with invalid regular expression", async () => { + const invalidRegex = "(?("; + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '${invalidRegex}' + `); + await expect(runner.getConfigFile("")).rejects.toThrowError("Regular expression is invalid. Check the logs"); + expect(logger.logHistory).toContainEqual(`Include condition '${invalidRegex}' is not a valid regex`); + }); + }); + describe("preventReviewRequests field", () => { test("should get team", async () => { api.getConfigFile.mockResolvedValue(` From e42f359ed883156d8e6735cf17f3a10908a8745d Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 17 Jul 2023 12:34:14 +0200 Subject: [PATCH 5/6] removed unused import --- src/test/runner/config.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/runner/config.test.ts b/src/test/runner/config.test.ts index 4a4044a..94977c7 100644 --- a/src/test/runner/config.test.ts +++ b/src/test/runner/config.test.ts @@ -4,7 +4,6 @@ import { mock, MockProxy } from "jest-mock-extended"; import { PullRequestApi } from "../../github/pullRequest"; -import { ActionLogger } from "../../github/types"; import { ActionRunner } from "../../runner"; import { TestLogger } from "../logger"; From 1240d9a2faccb2766e77251842d1330a292ff4ae Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 17 Jul 2023 13:55:16 +0200 Subject: [PATCH 6/6] enhance the error log --- src/runner.ts | 3 +-- src/test/runner/config.test.ts | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 428b7c8..034f39d 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -26,8 +26,7 @@ export class ActionRunner { const [result, error] = validateRegularExpressions(configFile, this.logger); if (!result) { - this.logger.error(error); - throw new Error("Regular expression is invalid. Check the logs"); + throw new Error(`Regular expression is invalid: ${error}`); } return configFile; diff --git a/src/test/runner/config.test.ts b/src/test/runner/config.test.ts index 94977c7..55c1306 100644 --- a/src/test/runner/config.test.ts +++ b/src/test/runner/config.test.ts @@ -45,8 +45,10 @@ describe("Config Parsing", () => { include: - '${invalidRegex}' `); - await expect(runner.getConfigFile("")).rejects.toThrowError("Regular expression is invalid. Check the logs"); - expect(logger.logHistory).toContainEqual(`Include condition '${invalidRegex}' is not a valid regex`); + await expect(runner.getConfigFile("")).rejects.toThrowError( + `Regular expression is invalid: Include condition '${invalidRegex}' is not a valid regex`, + ); + expect(logger.logHistory).toContainEqual(`Invalid regular expression: /${invalidRegex}/: Invalid group`); }); });