Clean up validation of resetSeconds and creditLimit

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Divvy Changelog
 
+## v1.2.0 (2018-04-27)
+
+* Protocol Documentation: Clarify the valid range of values for `creditLimit` and `resetSeconds`.
+* Validation: Moved validation of `resetSeconds` from rule evaluation time to configuration parse time, and added validation for `creditLimit`.
+
 ## v1.1.0 (2017-08-15)
 
 * Feature: Enable Prometheus metric scraping by exporting `HTTP_SERVICE_PORT` and `PROMETHEUS_METRICS_PATH` environment variables.

README.md

@@ -212,8 +212,8 @@ Configuration is expressed as a sequence of *buckets*. Buckets have the followin
 * `operation`: Zero or more key-value pairs which must be found in the incoming `HIT` request.
   * The special value `*` may be used here to express, "key must be present, but any value can match".
   * Glob keys are supported in the interest of specifying limits across subpaths, such as `/v1/billing/*`.
-* `creditLimit`: The number of hits that are allowed in the quota period.
-* `resetSeconds`: The quota period; reset the counter and refresh quota after this many seconds.
+* `creditLimit`: The number of hits that are allowed in the quota period. Must be >= 0.
+* `resetSeconds`: The quota period; reset the counter and refresh quota after this many seconds. Must be > 0.
 
 Bucket order is signficant: Quota is determined for a `HIT` request by finding the first bucket where all key/value pairs required by the bucket's `operation` match the request. Additional key/value pairs in the request *may* be ignored.
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@button/divvy",
-  "version": "1.1.0",
+  "version": "1.2.0",
   "description": "Redis-backed rate limit service.",
   "main": "index.js",
   "directories": {

src/backend.js

@@ -38,10 +38,6 @@ class Backend {
       return Promise.reject(new Error('hit(): Backend not initialized.'));
     }
 
-    if (resetSeconds < 1) {
-      return Promise.reject(new Error('hit(): bad value for resetSeconds'));
-    }
-
     if (creditLimit <= 0) {
       return Promise.resolve({
         isAllowed: false,

src/config.js

@@ -18,7 +18,6 @@ function isGlobValue(v) {
 }
 
 class Config {
-
   constructor() {
     this.rules = [];
   }
@@ -45,11 +44,14 @@ class Config {
     for (const rulegroupString of Object.keys(rawConfig)) {
       const rulegroupConfig = rawConfig[rulegroupString];
 
+      // These fields are required and will be validated within addRule
       const operation = Config.stringToOperation(rulegroupString);
-      const creditLimit = parseInt(rulegroupConfig.creditLimit, 10) || 0;
-      const resetSeconds = parseInt(rulegroupConfig.resetSeconds, 10) || 0;
+      const creditLimit = parseInt(rulegroupConfig.creditLimit, 10);
+      const resetSeconds = parseInt(rulegroupConfig.resetSeconds, 10);
+
+      // Optional fields.
       const actorField = rulegroupConfig.actorField || '';
-      const comment = rulegroupConfig.comment;
+      const comment = rulegroupConfig.comment || '';
 
       config.addRule(operation, creditLimit, resetSeconds, actorField, comment);
     }
@@ -82,11 +84,20 @@ class Config {
    */
   addRule(operation, creditLimit, resetSeconds, actorField, comment) {
     const foundRule = this.findRule(operation);
+
     if (foundRule !== null) {
       throw new Error(
         `Unreachable rule for operation=${operation}; masked by operation=${foundRule.operation}`);
     }
 
+    if (isNaN(creditLimit) || creditLimit < 0) {
+      throw new Error(`Invalid creditLimit for operation=${operation} (${creditLimit})`);
+    }
+
+    if (isNaN(resetSeconds) || resetSeconds < 1) {
+      throw new Error(`Invalid resetSeconds for operation=${operation} (${resetSeconds})`);
+    }
+
     const rule = {
       operation,
       creditLimit,

tests/config-test.js

@@ -89,6 +89,31 @@ describe('src/config', function () {
       }, /Unreachable rule/);
     });
 
+    it('with a rule containing an invalid creditLimit', function () {
+      const config = new Config();
+      config.addRule({ service: 'myservice', method: 'GET' }, 0, 60);
+      assert.throws(() => {
+        config.addRule({ service: 'myservice', method: 'POST' }, -1, 60);
+      }, /Invalid creditLimit/);
+      assert.throws(() => {
+        config.addRule({ service: 'myservice', method: 'PATCH' }, 'seven', 60);
+      }, /Invalid creditLimit/);
+    });
+
+    it('with a rule where resetSeconds < 1', function () {
+      const config = new Config();
+      config.addRule({ service: 'myservice', method: 'GET' }, 20, 1);
+      assert.throws(() => {
+        config.addRule({ service: 'myservice', method: 'POST' }, 70, 0);
+      }, /Invalid resetSeconds/);
+      assert.throws(() => {
+        config.addRule({ service: 'myservice', method: 'POST' }, 70, -20);
+      }, /Invalid resetSeconds/);
+      assert.throws(() => {
+        config.addRule({ service: 'myservice', method: 'POST' }, 10, 'fish');
+      }, /Invalid resetSeconds/);
+    });
+
     it('handles simple glob keys', function () {
       const config = new Config();