querystring: using toString for objects on querystring.escape

This commit fixes an inconsistency in querystring.escape objects handling
compared to native encodeURIComponent function.

Fixes: https://github.com/nodejs/node/issues/5309
PR-URL: https://github.com/nodejs/node/pull/5341
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

lib/querystring.js

@@ -90,8 +90,12 @@ for (var i = 0; i < 256; ++i)
 QueryString.escape = function(str) {
   // replaces encodeURIComponent
   // http://www.ecma-international.org/ecma-262/5.1/#sec-15.1.3.4
-  if (typeof str !== 'string')
+  if (typeof str !== 'string') {
-    str += '';
+    if (typeof str === 'object')
+      str = String(str);
+    else
+      str += '';
+  }
   var out = '';
   var lastPos = 0;
 

test/parallel/test-querystring-escape.js

@@ -0,0 +1,24 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+const qs = require('querystring');
+
+assert.deepEqual(qs.escape(5), '5');
+assert.deepEqual(qs.escape('test'), 'test');
+assert.deepEqual(qs.escape({}), '%5Bobject%20Object%5D');
+assert.deepEqual(qs.escape([5, 10]), '5%2C10');
+
+// using toString for objects
+assert.strictEqual(
+  qs.escape({test: 5, toString: () => 'test', valueOf: () => 10 }),
+  'test'
+);
+
+// toString is not callable, must throw an error
+assert.throws(() => qs.escape({toString: 5}));
+
+// should use valueOf instead of non-callable toString
+assert.strictEqual(qs.escape({toString: 5, valueOf: () => 'test'}), 'test');
+
+assert.throws(() => qs.escape(Symbol('test')));