Merge pull request #448 from stelace/fix-invalid-cron-pattern

Improve detection of incorrect cron pattern

lib/cron.js

@@ -413,6 +413,14 @@
 			var i = 0;
 			var len = timeUnits.length;
 
+			// seconds are optional
+			if (split.length < timeUnits.length - 1) {
+				throw new Error('Too few fields');
+			}
+			if (split.length > timeUnits.length) {
+				throw new Error('Too many fields');
+			}
+
 			for (; i < timeUnits.length; i++) {
 				// If the split source string doesn't contain all digits,
 				// assume defaults for first n missing digits.
@@ -429,6 +437,14 @@
 			var low = constraints[0];
 			var high = constraints[1];
 
+			var fields = field.split(',');
+			fields.forEach(function(field) {
+				var wildcardIndex = field.indexOf('*');
+				if (wildcardIndex !== -1 && wildcardIndex !== 0) {
+					throw new Error('Field (' + field + ') has an invalid wildcard expression');
+				}
+			});
+
 			// * is a shortcut to [lower-upper] range
 			field = field.replace(/\*/g, low + '-' + high);
 
@@ -438,7 +454,24 @@
 			for (var i = 0; i < allRanges.length; i++) {
 				if (allRanges[i].match(rangePattern)) {
 					allRanges[i].replace(rangePattern, function($0, lower, upper, step) {
+						if (step === '0') {
+							throw new Error('Field (' + field + ') has a step of zero');
+						}
 						step = parseInt(step) || 1;
+
+						if (upper && lower > upper) {
+							throw new Error('Field (' + field + ') has an invalid range');
+						}
+
+						const outOfRangeError =
+							lower < low ||
+							(upper && upper > high) ||
+							(!upper && lower > high);
+
+						if (outOfRangeError) {
+							throw new Error('Field (' + field + ') value is out of range');
+						}
+
 						// Positive integer higher than constraints[0]
 						lower = Math.min(Math.max(low, ~~Math.abs(lower)), high);
 

tests/cron.test.js

@@ -1,3 +1,4 @@
+/* eslint-disable no-new */
 const sinon = require('sinon');
 const cron = require('../lib/cron');
 describe('cron', () => {
@@ -486,32 +487,27 @@ describe('cron', () => {
 	});
 
 	it('should not get into an infinite loop on invalid times', function() {
-		const clock = sinon.useFakeTimers();
-
-		const invalid1 = new cron.CronJob(
-			'* 60 * * * *',
-			function() {
-				expect.ok(true);
-			},
-			null,
-			true
-		);
-		var invalid2 = new cron.CronJob(
-			'* * 24 * * *',
-			function() {
-				expect.ok(true);
-			},
-			null,
-			true
-		);
-
-		clock.tick(1000);
-
-		// assert that it gets here
-		invalid1.stop();
-		invalid2.stop();
+		expect(function() {
+			new cron.CronJob(
+				'* 60 * * * *',
+				function() {
+					expect.ok(true);
+				},
+				null,
+				true
+			);
+		}).toThrow();
 
-		clock.restore();
+		expect(function() {
+			new cron.CronJob(
+				'* * 24 * * *',
+				function() {
+					expect.ok(true);
+				},
+				null,
+				true
+			);
+		}).toThrow();
 	});
 
 	it('should test start of month', function() {
@@ -822,10 +818,10 @@ describe('cron', () => {
 			expect(callback).toHaveBeenCalledTimes(1);
 		});
 
-		it('should be able to detect out of range days of month and fix them', function() {
-			var ct = new cron.CronTime('* * 32 FEB *');
-			expect(ct.dayOfMonth['32']).toBe(undefined);
-			expect(ct.dayOfMonth['2']).toBe(true);
+		it('should be able to detect out of range days of month', function() {
+			expect(function() {
+				new cron.CronTime('* * 32 FEB *');
+			}).toThrow();
 		});
 	});
 });

tests/crontime.test.js

@@ -64,9 +64,9 @@ describe('crontime', function() {
 		}).not.toThrow();
 	});
 
-	it('should test all hyphens (0-10 0-10 0-10 0-10 0-10 0-1)', function() {
+	it('should test all hyphens (0-10 0-10 1-10 1-10 0-6 0-1)', function() {
 		expect(function() {
-			new cron.CronTime('0-10 0-10 0-10 0-10 0-10 0-1');
+			new cron.CronTime('0-10 0-10 1-10 1-10 0-6 0-1');
 		}).not.toThrow();
 	});
 
@@ -82,9 +82,9 @@ describe('crontime', function() {
 		}).not.toThrow();
 	});
 
-	it('should test all commas (0,10 0,10 0,10 0,10 0,10 0,1)', function() {
+	it('should test all commas (0,10 0,10 1,10 1,10 0,6 0,1)', function() {
 		expect(function() {
-			new cron.CronTime('0,10 0,10 0,10 0,10 0,10 0,1');
+			new cron.CronTime('0,10 0,10 1,10 1,10 0,6 0,1');
 		}).not.toThrow();
 	});
 
@@ -118,6 +118,42 @@ describe('crontime', function() {
 		}).toThrow();
 	});
 
+	it('should test too few fields', function() {
+		expect(function() {
+			new cron.CronTime('* * * *', null, null);
+		}).toThrow();
+	});
+
+	it('should test too many fields', function() {
+		expect(function() {
+			new cron.CronTime('* * * * * * *', null, null);
+		}).toThrow();
+	});
+
+	it('should test out of range values', function() {
+		expect(function() {
+			new cron.CronTime('* * * * 1234', null, null);
+		}).toThrow();
+	});
+
+	it('should test invalid wildcard expression', function() {
+		expect(function() {
+			new cron.CronTime('* * * * 0*');
+		}).toThrow();
+	});
+
+	it('should test invalid step', function() {
+		expect(function() {
+			new cron.CronTime('* * * 1/0 *');
+		}).toThrow();
+	});
+
+	it('should test invalid range', function() {
+		expect(function() {
+			new cron.CronTime('* 2-1 * * *');
+		}).toThrow();
+	});
+
 	it('should test Date', function() {
 		const d = new Date();
 		const ct = new cron.CronTime(d);