Merge pull request #80 from github/thyeggman/cron-parsing

Improve and fix cron parsing

actions-languageservice/src/validate.test.ts

@@ -186,4 +186,34 @@ jobs:
       }
     } as Diagnostic);
   });
+
+  it("invalid cron string", async () => {
+    const result = await validate(
+      createDocument(
+        "wf.yaml",
+        `on:
+  schedule:
+    - cron: '0 0 * *'
+jobs:
+  build:
+    runs-on: ubuntu-latest`
+      ),
+      {valueProviderConfig: defaultValueProviders}
+    );
+
+    expect(result.length).toBe(1);
+    expect(result[0]).toEqual({
+      message: "Invalid cron string",
+      range: {
+        end: {
+          character: 21,
+          line: 2
+        },
+        start: {
+          character: 12,
+          line: 2
+        }
+      }
+    } as Diagnostic);
+  });
 });

actions-workflow-parser/src/model/converter/cron.test.ts

@@ -0,0 +1,63 @@
+import { isValidCron } from "./cron"
+
+describe("isValidCron", () => {
+  const valid = [
+    ["0 0 * * *", "every day at midnight"],
+    ["0 000 001 * *", "accepts leading zeros"],
+    ["15 * * * *", "accepts numbers in range"],
+    ["2,10 4,5 * * *", "accepts comma separated values"],
+    ["30 4-6 * * *", "accepts range"],
+    ["0 4-4 * * *", "accepts range with two equal values"],
+    ["20/15 * * * *", "accepts step with numerical values"],
+    ["30 5,17 * * *", "accepts numbers and ranges"],
+    ["28 */4 * * *", "accepts step with * and numerical value"],
+    ["28 5,*/4 * * *", "accepts comma separated value with step"],
+    ["28 5,*/4,6-8 * * *", "accepts comma separated value with step and range"],
+    ["0 0 * * SUN", "accepts day of week short name"],
+    ["0 0 * * SUN-TUE", "accepts day of week short name range"],
+    ["0 0 * * SUN-2", "accepts day of week range combined with number"],
+    ["0 2-4/5 * * *", "accepts range with step"],
+    ["0   0  *  *               *", "accepts multiple spaces"],
+  ]
+
+  for (const [cron, reason] of valid) {
+    it(`${cron} should be valid: ${reason}`, () => {
+      expect(isValidCron(cron)).toBe(true)
+    })
+  }
+
+  const invalid = [
+    ["0 0 * *", "too few parts"],
+    ["0 0 * * * * *", "too many parts"],
+    ["0 -1 * * *", "should not accept negative numbers"],
+    ["0 1- * * *", "should not accept trailing -"],
+    ["0 /1 * * *", "should not accept leading / (empty value)"],
+    ["0 1/ * * *", "should not accept trailing / (empty value)"],
+    ["0 ,1 * * *", "should not accept leading , (empty value)"],
+    ["0 1, * * *", "should not accept trailing , (empty value)"],
+    ["0 5--5 * * *", "should not accept multiple -"],
+    ["0 *//5 * * *", "should not accept multiple /"],
+    ["0 */* * * *", "step start and size may not both be *"],
+    ["0 5/* * * *", "steps size may not be *"],
+    ["0 *-4 5-* *-* *", "range may not contain *"],
+    ["0 *//5 * * *", "should not accept multiple /"],
+    ["0 ,, * * *", "should not accept multiple ,"],
+    [", , , , ,", "comma is not a valid part"],
+    ["0 ** * * *", "should not accept multiple *"],
+    ["0 0 * * BUN", "invalid short name"],
+    ["0 0 * SUN JAN", "short name in incorrect position"],
+    ["0 0 * * FRI-TUE", "should not accept short name range with start > end"],
+    ["0 12-4 * * *", "should not accept nuerical range with start > end"],
+    ["0 */0 * * *", "step size may not be 0"],
+    ["0 2/4-5 * * *", "step size may not be a range"],
+    ["0 2-4-6 * * *", "range may not contain multiple -"],
+    ["0 2/4/6 * * *", "step may not contain multiple /"],
+    ["0 * * */FEB */TUE", "step size may not be a short name"],
+  ]
+
+  for (const [cron, reason] of invalid) {
+    it(`${cron} should be invalid: ${reason}`, () => {
+      expect(isValidCron(cron)).toBe(false)
+    })
+  }
+})

actions-workflow-parser/src/model/converter/cron.ts

@@ -0,0 +1,118 @@
+const MONTHS = {
+  jan: 1,
+  feb: 2,
+  mar: 3,
+  apr: 4,
+  may: 5,
+  jun: 6,
+  jul: 7,
+  aug: 8,
+  sep: 9,
+  oct: 10,
+  nov: 11,
+  dec: 12,
+}
+
+const DAYS = {
+  sun: 0,
+  mon: 1,
+  tue: 2,
+  wed: 3,
+  thu: 4,
+  fri: 5,
+  sat: 6,
+}
+
+// TODO: make this parseCron
+export function isValidCron(cron: string): boolean {
+  // https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule
+
+  const parts = cron.split(/ +/)
+  if (parts.length != 5) {
+    return false
+  }
+
+  const [minutes, hours, dom, months, dow] = parts
+
+  return (
+    validateRange(minutes, { min: 0, max: 59 }) &&
+    validateRange(hours, { min: 0, max: 23 }) &&
+    validateRange(dom, { min: 1, max: 31 }) &&
+    validateRange(months, { min: 1, max: 12, names: MONTHS }) &&
+    validateRange(dow, { min: 0, max: 6, names: DAYS })
+  )
+}
+
+type Range = {
+  min: number
+  max: number
+  names?: Record<string, number>
+}
+
+function validateRange(
+  value: string,
+  range: Range,
+  allowSeparators = true
+): boolean {
+  if (range.names && range.names[value.toLowerCase()] !== undefined) {
+    return true
+  }
+
+  if (value === "*") {
+    return true
+  }
+
+  // Operator precedence: , > / > -
+  if (value.includes(",")) {
+    if (!allowSeparators) {
+      return false
+    }
+    return value.split(",").every((v) => v && validateRange(v, range))
+  }
+
+  if (value.includes("/")) {
+    if (!allowSeparators) {
+      return false
+    }
+
+    const [start, step, ...rest] = value.split("/")
+    const stepNumber = +step
+    if (rest.length > 0 || isNaN(stepNumber) || stepNumber <= 0 || !start || !step) {
+      return false
+    }
+
+    // Separators are only allowed in the part before the `/`, e.g. `1-5/2`
+    return (
+      validateRange(start, range) && validateRange(step, range, false)
+    )
+  }
+
+  if (value.includes("-")) {
+    if (!allowSeparators) {
+      return false
+    }
+    const [start, end, ...rest] = value.split("-")
+    if (rest.length > 0 || !start || !end) {
+      return false
+    }
+
+    // Convert name to integers so we can make sure end >= start
+    const startNumber = convertToNumber(range, start)
+    const endNumber = convertToNumber(range, end)
+    return (
+      validateRange(start, range, false) && validateRange(end, range, false) && endNumber >= startNumber
+    )
+  }
+
+  const number = +value
+  return !isNaN(number) && number >= range.min && number <= range.max
+}
+
+function convertToNumber(range: Range, value: string): number {
+  if (range.names && range.names[value.toLowerCase()] !== undefined) {
+    return +range.names[value.toLowerCase()]
+  }
+  else {
+    return +value
+  }
+}

actions-workflow-parser/src/model/converter/events.ts

@@ -13,6 +13,7 @@ import {
   TypesFilterConfig,
   WorkflowFilterConfig
 } from "../workflow-template";
+import {isValidCron} from "./cron"
 import {convertStringList} from "./string-list";
 import {convertEventWorkflowDispatchInputs} from "./workflow-dispatch";
 
@@ -144,8 +145,8 @@ function convertSchedule(context: TemplateContext, token: SequenceToken): Schedu
       if (scheduleKey.value == "cron") {
         const cron = schedule.value.assertString(`schedule cron`);
         // Validate the cron string
-        if (!cron.value.match(/((((\d+,)+\d+|(\d+(\/|-)\d+)|\d+|\*) ?){5,7})/)) {
-          context.error(cron, "Invalid cron string");
+        if (!isValidCron(cron.value)) {
+          context.error(cron, "Invalid cron string")
         }
         result.push({cron: cron.value});
       } else {