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>

2 files changed, 30 insertions, 2 deletions

diff --git a/lib/querystring.js b/lib/querystring.js
index ad1b5861a0..bacfc4bd70 100644
--- a/lib/querystring.js
+++ b/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')
-    str += '';
+  if (typeof str !== 'string') {
+    if (typeof str === 'object')
+      str = String(str);
+    else
+      str += '';
+  }
   var out = '';
   var lastPos = 0;
 
diff --git a/test/parallel/test-querystring-escape.js b/test/parallel/test-querystring-escape.js
new file mode 100644
index 0000000000..e15ce3266d
--- /dev/null
+++ b/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')));