support passing most qs options to restify.queryParser (#1209)

Fixes #925

CHANGES.md

@@ -1,5 +1,12 @@
 # restify Changelog
 
+## 4.2.0
+
+- #925 Support passing (most) [qs](https://github.com/ljharb/qs#readme) options
+  to the `restify.queryParser` plugin. Update the "qs" dependency to latest (v6)
+  while maintaining backward compatibility (see notes in the API doc and
+  "test/query.test.js" on `allowDots` and `plainObjects`).
+
 ## 4.1.1
 - update negotiator (NSP advisory #106) and lru-cache (bug fix).
 

docs/index.restdown

@@ -929,14 +929,41 @@ interpreted in seconds, to allow for clock skew.
 
 ### QueryParser
 
-    server.use(restify.queryParser());
+    server.use(restify.queryParser([OPTIONS]));
+
+Parses the HTTP query string (e.g., `/foo?id=bar&name=mark`) to an object on
+`req.query`, e.g. `{id: 'bar', name: 'mark'}`.
+
+By default, query params are merged into `req.params` (which may include
+parameters from `restify.bodyParser`). This can be disabled by the `mapParams`
+option:
+
+    server.use(restify.queryParser({mapParams: false}));
+
+QueryParser options are as follows. All but `mapParams` are directly passed to
+the underlying ["qs" module](https://github.com/ljharb/qs#readme).
+
+|| **Option**         || **Type** || **Default** || **Description** ||
+|| mapParams          || bool || true    || Whether to merge req.query into req.params. ||
+|| allowDots          || bool || true \* || Transform `?foo.bar=baz` to a nested object: `{foo: {bar: 'baz'}}`. ||
+|| arrayLimit         || num  || 20      || Only transform `?a[$index]=b` to an array if `$index` is less than `arrayLimit`. ||
+|| depth              || num  || 5       || The depth limit for parsing nested objects, e.g. `?a[b][c][d][e][f][g][h][i]=j`. ||
+|| parameterLimit     || num  || 1000    || Maximum number of query params parsed. Additional params are silently dropped. ||
+|| parseArrays        || bool || true    || Whether to parse `?a[]=b&a[1]=c` to an array, e.g. `{a: ['b', 'c']}`. ||
+|| plainObjects       || bool || true \* || Whether `req.query` is a "plain" object -- does not inherit from `Object`. This can be used to allow query params whose names collide with Object methods, e.g. `?hasOwnProperty=blah`. ||
+|| strictNullHandling || bool || false   || If true, `?a&b=` results in `{a: null, b: ''}`. Otherwise, `{a: '', b: ''}`. ||
+
+\*: The `allowDots` and `plainObjects` options are `true` by default in restify
+4.x because of an accident. Restify 4.0 updated from qs@2 to qs@3, which changed
+default behavior to this. Subsequent qs major versions reverted that behaviour.
+The restify 4.x stream will maintain this default, but all other major versions
+of restify (those before v4) and the `queryParser` plugin in [email protected]
+and later (restify 5.x no longer bundles the plugins) default to
+`allowDots=false` and `plainObjects=false`. The recommended usage (for new
+code or to maintain compat across restify major versions is):
 
-Parses the HTTP query string (i.e., `/foo?id=bar&name=mark`).  If you use this,
-the parsed content will always be available in `req.query`, additionally params
-are merged into `req.params`.  You can disable by passing in `mapParams: false`
-in the options object:
+    server.use(restify.queryParser({allowDots: false, plainObjects: false}));
 
-    server.use(restify.queryParser({ mapParams: false }));
 
 ### JSONP
 

lib/plugins/query.js

@@ -5,6 +5,23 @@
 var qs = require('qs');
 var assert = require('assert-plus');
 
+var EXPOSED_QS_OPTIONS = {
+    allowDots: assert.optionalBool,
+    arrayLimit: assert.optionalNumber,
+    depth: assert.optionalNumber,
+    parameterLimit: assert.optionalNumber,
+    parseArrays: assert.optionalBool,
+    plainObjects: assert.optionalBool,
+    strictNullHandling: assert.optionalBool
+
+    /*
+     * Exclusions (`qs.parse` options that restify does NOT expose):
+     * - `allowPrototypes`: It is strongly suggested against in qs docs.
+     * - `decoder`
+     * - `delimiter`: For query string parsing we shouldn't support anything
+     *   but the default '&'.
+     */
+};
 
 /**
  * Returns a plugin that will parse the query string, and merge the results
@@ -22,14 +39,33 @@ function queryParser(options) {
     }
     assert.object(options, 'options');
 
+    /*
+     * Releases of restify 4.x up to 4.1.1 used qs@3 which effectively defaulted
+     * to `plainObjects=true` and `allowDots=true`. To maintain backward
+     * compatibility for the restify 4.x stream while using the latest qs
+     * version, we need to maintain those defaults. Note that restify-plugins
+     * changes back to the pre-restify-4.x behaviour. See test/query.test.js
+     * for more details.
+     */
+    var qsOptions = {
+        plainObjects: true,
+        allowDots: true
+    };
+    Object.keys(EXPOSED_QS_OPTIONS).forEach(function (k) {
+        EXPOSED_QS_OPTIONS[k](options[k], k); // assert type of this option
+
+        if (options.hasOwnProperty(k)) {
+            qsOptions[k] = options[k];
+        }
+    });
 
     function parseQueryString(req, res, next) {
         if (!req.getQuery()) {
             req.query = {};
             return (next());
         }
 
-        req.query = qs.parse(req.getQuery());
+        req.query = qs.parse(req.getQuery(), qsOptions);
 
         if (options.mapParams !== false) {
             Object.keys(req.query).forEach(function (k) {

package.json

@@ -41,7 +41,7 @@
   "name": "restify",
   "homepage": "http://restifyjs.com",
   "description": "REST framework",
-  "version": "4.1.1",
+  "version": "4.2.0",
   "repository": {
     "type": "git",
     "url": "git://github.com/restify/node-restify.git"
@@ -70,7 +70,7 @@
     "negotiator": "^0.6.1",
     "node-uuid": "^1.4.1",
     "once": "^1.3.0",
-    "qs": "^3.1.0",
+    "qs": "^6.2.1",
     "semver": "^4.3.3",
     "spdy": "^3.3.3",
     "tunnel-agent": "^0.4.0",

test/plugins.test.js

@@ -147,57 +147,6 @@ test('authorization basic invalid', function (t) {
 });
 
 
-test('query ok', function (t) {
-    SERVER.get('/query/:id', function (req, res, next) {
-        t.equal(req.params.id, 'foo');
-        t.equal(req.params.name, 'markc');
-        t.equal(req.params.name, 'markc');
-        res.send();
-        next();
-    });
-
-    CLIENT.get('/query/foo?id=bar&name=markc', function (err, _, res) {
-        t.ifError(err);
-        t.equal(res.statusCode, 200);
-        t.end();
-    });
-});
-
-
-test('GH-124 query ok no query string', function (t) {
-    SERVER.get('/query/:id', function (req, res, next) {
-        t.equal(req.getQuery(), '');
-        res.send();
-        next();
-    });
-
-    CLIENT.get('/query/foo', function (err, _, res) {
-        t.ifError(err);
-        t.equal(res.statusCode, 200);
-        t.end();
-    });
-});
-
-
-test('query object', function (t) {
-    SERVER.get('/query/:id', function (req, res, next) {
-        t.equal(req.params.id, 'foo');
-        t.ok(req.params.name);
-        t.equal(req.params.name.first, 'mark');
-        t.equal(req.query.name.last, 'cavage');
-        res.send();
-        next();
-    });
-
-    var p = '/query/foo?name[first]=mark&name[last]=cavage';
-    CLIENT.get(p, function (err, _, res) {
-        t.ifError(err);
-        t.equal(res.statusCode, 200);
-        t.end();
-    });
-});
-
-
 test('body url-encoded ok', function (t) {
     SERVER.post('/bodyurl/:id',
         restify.bodyParser(),