fix: add cron validation to jobVerifier (#457)

package.json

@@ -45,7 +45,7 @@
     "bluebird": "^3.7.2",
     "body-parser": "^1.19.0",
     "copy-dir": "^0.3.0",
-    "cron": "1.7.2",
+    "cron": "^1.8.1",
     "dockerode": "^2.5.8",
     "express": "^4.17.1",
     "express-ajv-swagger-validation": "^1.1.1",

src/jobs/helpers/jobVerifier.js

@@ -1,5 +1,19 @@
 'use strict';
 const testsManager = require('../../tests/models/manager');
+const CronTime = require('cron').CronTime;
+
+/**
+ * Validates a cron expression and returns error message if the expression is invalid
+ * @param {string} exp
+ * @returns {(string|undefined)} Error message if the expression is invalid
+ */
+function verifyCronExpression(exp) {
+    try {
+        const ct = new CronTime(exp);
+    } catch (err) {
+        return err.message;
+    }
+}
 
 module.exports.verifyJobBody = (req, res, next) => {
     let errorToThrow;
@@ -12,6 +26,13 @@ module.exports.verifyJobBody = (req, res, next) => {
         errorToThrow = new Error('It is impossible to disable job without cron_expression');
         errorToThrow.statusCode = 400;
     }
+    if (jobBody.cron_expression) {
+        const message = verifyCronExpression(jobBody.cron_expression);
+        if (message) {
+            errorToThrow = new Error(`Unsupported cron_expression. ${message}`);
+            errorToThrow.statusCode = 400;
+        }
+    }
     next(errorToThrow);
 };
 

tests/integration-tests/jobs/updateJob-test.js

@@ -130,7 +130,7 @@ describe('Update scheduled job', function () {
                 duration: 1,
                 environment: 'test',
                 run_immediately: false,
-                cron_expression: '* ' + date.getHours() + ' * * * *'
+                cron_expression: '* ' + date.getHours() + ' * * *'
             };
             const createJobResponse = await schedulerRequestCreator.createJob(validBody, {
                 'Content-Type': 'application/json'
@@ -174,7 +174,7 @@ describe('Update scheduled job', function () {
                 type: 'load_test',
                 environment: 'test',
                 run_immediately: false,
-                cron_expression: '* ' + date.getHours() + ' * * * *',
+                cron_expression: '* ' + date.getHours() + ' * * *',
                 webhooks: [],
                 emails: ['b@emails.com']
             };
@@ -188,7 +188,7 @@ describe('Update scheduled job', function () {
             const date = new Date();
 
             date.setHours(date.getHours() + 3);
-            updatedCronExpression = `* ${date.getHours()} * * * * *`;
+            updatedCronExpression = `* ${date.getHours()} * * *`;
 
             const updateJobResponse = await schedulerRequestCreator.updateJob(jobId,
                 {

tests/unit-tests/jobs/helpers/jobVerifier-test.js

@@ -75,13 +75,13 @@ describe('Jobs verifier tests', function () {
         });
 
         it('Run immediately is true and cron expression exist and enabled is false, should pass', async () => {
-            req = { body: { run_immediately: true, cron_expression: '* * *', enabled: false } };
+            req = { body: { run_immediately: true, cron_expression: '* * * * *', enabled: false } };
             await jobVerifier.verifyJobBody(req, res, nextStub);
             should(nextStub.args[0][0]).eql(undefined);
         });
 
         it('Run immediately is true and cron expression exist and enabled is true, should pass', async () => {
-            req = { body: { run_immediately: true, cron_expression: '* * *', enabled: true } };
+            req = { body: { run_immediately: true, cron_expression: '* * * * *', enabled: true } };
             await jobVerifier.verifyJobBody(req, res, nextStub);
             should(nextStub.args[0][0]).eql(undefined);
         });
@@ -94,12 +94,12 @@ describe('Jobs verifier tests', function () {
         });
 
         it('Run immediately is false and cron expression exist, should pass', async () => {
-            req = { body: { run_immediately: false, cron_expression: '* * *' } };
+            req = { body: { run_immediately: false, cron_expression: '* * * * *' } };
             await jobVerifier.verifyJobBody(req, res, nextStub);
             should(nextStub.args[0][0]).eql(undefined);
         });
         it('Run immediately does not exits and cron expression exist, should pass', async () => {
-            req = { body: { cron_expression: '* * *' } };
+            req = { body: { cron_expression: '* * * * *' } };
             await jobVerifier.verifyJobBody(req, res, nextStub);
             should(nextStub.args[0][0]).eql(undefined);
         });
@@ -124,5 +124,32 @@ describe('Jobs verifier tests', function () {
             await jobVerifier.verifyJobBody(req, res, nextStub);
             should(nextStub.args[0][0]).eql(undefined);
         });
+
+        it('Run immediately is false and cron expression exists, valid cron expression, should pass', async () => {
+            req = { body: { run_immediately: false, cron_expression: '0 20 0 7 OCT *' } };
+            await jobVerifier.verifyJobBody(req, res, nextStub);
+            should(nextStub.args[0][0]).eql(undefined);
+        });
+
+        it('Run immediately is false and cron expression exists, cron expression w/ unsupported character, should fail', async () => {
+            req = { body: { run_immediately: false, cron_expression: '0 20 0 7 OCT ? 2020' } };
+            await jobVerifier.verifyJobBody(req, res, nextStub);
+            should(nextStub.args[0][0].message).startWith('Unsupported cron_expression. ');
+            should(nextStub.args[0][0].statusCode).eql(400);
+        });
+
+        it('Run immediately is false and cron expression exists, cron expression w/ too many fields, should fail', async () => {
+            req = { body: { run_immediately: false, cron_expression: '0 20 0 7 OCT * 2020' } };
+            await jobVerifier.verifyJobBody(req, res, nextStub);
+            should(nextStub.args[0][0].message).startWith('Unsupported cron_expression. ');
+            should(nextStub.args[0][0].statusCode).eql(400);
+        });
+
+        it('Run immediately is false and cron expression exists, cron expression w/ not enough fields, should fail', async () => {
+            req = { body: { run_immediately: false, cron_expression: '* * *' } };
+            await jobVerifier.verifyJobBody(req, res, nextStub);
+            should(nextStub.args[0][0].message).startWith('Unsupported cron_expression. ');
+            should(nextStub.args[0][0].statusCode).eql(400);
+        });
     });
 });

ui/package.json

@@ -80,6 +80,7 @@
     "axios": "^0.19.2",
     "babel-runtime": "^6.26.0",
     "classnames": "^2.2.5",
+    "cron-validator": "^1.1.1",
     "cronstrue": "^1.100.0",
     "dateformat": "^3.0.3",
     "font-awesome": "^4.7.0",

ui/src/features/components/JobForm/index.js

@@ -556,7 +556,7 @@ class Form extends React.Component {
                   onChange={(evt) => this.onChangeProperty(oneItem.name, evt.target.value)} />
               </ErrorWrapper>
             </TitleInput>
-            {oneItem.name === 'cron_expression' && <CronViewer value={cron_expression} />}
+            {oneItem.name === 'cron_expression' && !this.state.errors['cron_expression'] && <CronViewer value={cron_expression} />}
           </div>
 
         );

ui/src/features/components/JobForm/validator.js

@@ -1,5 +1,5 @@
 import { isUndefined } from 'lodash'
-import cronstrue from 'cronstrue';
+import { isValidCron } from 'cron-validator';
 import { testTypes } from './constants';
 
 import React from 'react';
@@ -81,12 +81,8 @@ function rampToValidator (value, allState) {
 }
 
 function cronValidator (value, allState) {
-  if (value) {
-    try {
-      cronstrue.toString(value)
-    } catch (err) {
-      return 'illegal cron input'
-    }
+  if (value && !isValidCron(value, { alias: true, seconds: true })) {
+    return 'illegal cron input'
   }
 }