More validation to ensure expression does not contain any unexpected characters (#331)

Author: bradymholt

src/cronParser.ts

@@ -26,7 +26,7 @@ export class CronParser {
    */
   parse(): string[] {
     let parsed: string[];
-    
+
     var expression = this.expression ?? '';
 
     if (expression.startsWith('@')) {
@@ -316,12 +316,22 @@ export class CronParser {
   }
 
   protected validate(parsed: string[]) {
-    this.assertNoInvalidCharacters("DOW", parsed[5]);
-    this.assertNoInvalidCharacters("DOM", parsed[3]);
-    this.validateRange(parsed);
+    const standardCronPartCharacters = "0-9,\\-*\/";
+    this.validateOnlyExpectedCharactersFound(parsed[0], standardCronPartCharacters);
+    this.validateOnlyExpectedCharactersFound(parsed[1], standardCronPartCharacters);
+    this.validateOnlyExpectedCharactersFound(parsed[2], standardCronPartCharacters);
+    // DOM
+    this.validateOnlyExpectedCharactersFound(parsed[3], "0-9,\\-*\/LW");
+    this.validateOnlyExpectedCharactersFound(parsed[4], standardCronPartCharacters);
+    // DOW
+    this.validateOnlyExpectedCharactersFound(parsed[5], "0-9,\\-*\/L#");
+    this.validateOnlyExpectedCharactersFound(parsed[6], standardCronPartCharacters);
+
+    this.validateAnyRanges(parsed);
+
   }
 
-  protected validateRange(parsed: string[]) {
+  protected validateAnyRanges(parsed: string[]) {
     RangeValidator.secondRange(parsed[0]);
     RangeValidator.minuteRange(parsed[1]);
     RangeValidator.hourRange(parsed[2]);
@@ -330,11 +340,12 @@ export class CronParser {
     RangeValidator.dayOfWeekRange(parsed[5], this.dayOfWeekStartIndexZero);
   }
 
-  protected assertNoInvalidCharacters(partDescription: string, expression: string) {
-    // No characters other than 'L' or 'W' should remain after normalization
-    let invalidChars = expression.match(/[A-KM-VX-Z]+/gi);
+  protected validateOnlyExpectedCharactersFound(cronPart: string, allowedCharsExpression: string) {
+    // Write code that will ensure the expression string only contains numbers or any of the following characters , - * /
+    // If any other characters are found, it is an error.
+    let invalidChars = cronPart.match(new RegExp(`[^${allowedCharsExpression}]+`, "gi"));
     if (invalidChars && invalidChars.length) {
-      throw new Error(`${partDescription} part contains invalid values: '${invalidChars.toString()}'`);
+      throw new Error(`Expression contains invalid values: '${invalidChars.toString()}'`);
     }
   }
 }

src/expressionDescriptor.ts

@@ -653,7 +653,7 @@ export class ExpressionDescriptor {
   protected formatTime(hourExpression: string, minuteExpression: string, secondExpression: string) {
     let hourOffset: number = 0;
     let minuteOffset: number = 0;
-    
+
     if(this.options.tzOffset) {
       hourOffset = this.options.tzOffset > 0 ? Math.floor(this.options.tzOffset) : Math.ceil(this.options.tzOffset);
 
@@ -674,7 +674,7 @@ export class ExpressionDescriptor {
       minute += 60;
       hour -= 1;
     }
-    
+
     if (hour >= 24) {
       hour = hour - 24;
     } else if (hour < 0) {

test/cronParser.ts

@@ -25,7 +25,21 @@ describe("CronParser", function () {
     it("should error if DOW part is not valid", function () {
       assert.throws(function () {
         new CronParser("* * * * MO").parse();
-      }, `DOW part contains invalid values: 'MO'`);
+      }, `Expression contains invalid values: 'MO'`);
+    });
+
+     it("does not allow unexpected characters or statements in any part", function () {
+      const maliciousStatement = "\nDROP\tDATABASE\tusers;";
+
+      for(let i = 0; i <= 6; i++) {
+        const cleanCronParts = ["*", "*", "*", "*", "*", "*", "*"];
+        cleanCronParts[i] = maliciousStatement;
+        const cronToTest = cleanCronParts.join(" ");
+        assert.throws(function () {
+          new CronParser(cronToTest).parse();
+        }, `Expression contains invalid values:`);
+      }
+
     });
 
     it("should parse cron with multiple spaces between parts", function () {

test/cronstrue.ts

@@ -729,6 +729,12 @@ describe("Cronstrue", function () {
       }, "Error: Expression has only 1 part. At least 5 parts are required.");
     });
 
+    it("too many parts", function () {
+      assert.throws(function () {
+        cronstrue.toString("* * * * * * * *");
+      }, "Expression has 8 parts; too many!");
+    });
+
     it("empty expression", function () {
       assert.throws(function () {
         cronstrue.toString("");