From c56a89013c16c3c6afe03859b2007377ea9bd808 Mon Sep 17 00:00:00 2001
From: Timothy Gu <timothygu99@gmail.com>
Date: Mon, 10 Jul 2017 13:58:26 +0800
Subject: [PATCH] querystring: fix up lastPos usage

Use lastPos ONLY for tracking what has been .slice()'d, never as an
indication of if key/value has been seen, since lastPos is updated on
seeing + as well.

PR-URL: https://github.com/nodejs/node/pull/14151
Fixes: https://github.com/nodejs/node/issues/13773
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
---
 lib/querystring.js                | 22 ++++++++++------------
 test/fixtures/url-searchparams.js |  9 +++++++++
 test/parallel/test-querystring.js | 15 +++++++++++++++
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/lib/querystring.js b/lib/querystring.js
index dd4d0a13e00c05..0eaaca57b08117 100644
--- a/lib/querystring.js
+++ b/lib/querystring.js
@@ -308,9 +308,7 @@ function parse(qs, sep, eq, options) {
           if (lastPos < end) {
             // Treat the substring as part of the key instead of the value
             key += qs.slice(lastPos, end);
-            if (keyEncoded)
-              key = decodeStr(key, decode);
-          } else {
+          } else if (key.length === 0) {
             // We saw an empty substring between separators
             if (--pairs === 0)
               return obj;
@@ -319,15 +317,15 @@ function parse(qs, sep, eq, options) {
             continue;
           }
         } else {
-          if (lastPos < end) {
+          if (lastPos < end)
             value += qs.slice(lastPos, end);
-            if (valEncoded)
-              value = decodeStr(value, decode);
-          }
-          if (keyEncoded)
-            key = decodeStr(key, decode);
         }
 
+        if (key.length > 0 && keyEncoded)
+          key = decodeStr(key, decode);
+        if (value.length > 0 && valEncoded)
+          value = decodeStr(value, decode);
+
         // Use a key array lookup instead of using hasOwnProperty(), which is
         // slower
         if (keys.indexOf(key) === -1) {
@@ -422,13 +420,13 @@ function parse(qs, sep, eq, options) {
       key += qs.slice(lastPos);
     else if (sepIdx < sepLen)
       value += qs.slice(lastPos);
-  } else if (eqIdx === 0) {
+  } else if (eqIdx === 0 && key.length === 0) {
     // We ended on an empty substring
     return obj;
   }
-  if (keyEncoded)
+  if (key.length > 0 && keyEncoded)
     key = decodeStr(key, decode);
-  if (valEncoded)
+  if (value.length > 0 && valEncoded)
     value = decodeStr(value, decode);
   // Use a key array lookup instead of using hasOwnProperty(), which is slower
   if (keys.indexOf(key) === -1) {
diff --git a/test/fixtures/url-searchparams.js b/test/fixtures/url-searchparams.js
index 3b186fc97bc38b..918af678bc563e 100644
--- a/test/fixtures/url-searchparams.js
+++ b/test/fixtures/url-searchparams.js
@@ -44,6 +44,7 @@ module.exports = [
   ['=', '=', [['', '']]],
   ['+', '+=', [[' ', '']]],
   ['+=', '+=', [[' ', '']]],
+  ['+&', '+=', [[' ', '']]],
   ['=+', '=+', [['', ' ']]],
   ['+=&', '+=', [[' ', '']]],
   ['a&&b', 'a=&b=', [['a', ''], ['b', '']]],
@@ -56,6 +57,14 @@ module.exports = [
   ['a=&a=value&a=', 'a=&a=value&a=', [['a', ''], ['a', 'value'], ['a', '']]],
   ['foo%20bar=baz%20quux', 'foo+bar=baz+quux', [['foo bar', 'baz quux']]],
   ['+foo=+bar', '+foo=+bar', [[' foo', ' bar']]],
+  ['a+', 'a+=', [['a ', '']]],
+  ['=a+', '=a+', [['', 'a ']]],
+  ['a+&', 'a+=', [['a ', '']]],
+  ['=a+&', '=a+', [['', 'a ']]],
+  ['%20+', '++=', [['  ', '']]],
+  ['=%20+', '=++', [['', '  ']]],
+  ['%20+&', '++=', [['  ', '']]],
+  ['=%20+&', '=++', [['', '  ']]],
   [
     // fake percent encoding
     'foo=%©ar&baz=%A©uux&xyzzy=%©ud',
diff --git a/test/parallel/test-querystring.js b/test/parallel/test-querystring.js
index 10f166562ba6c1..69fee2d6af168f 100644
--- a/test/parallel/test-querystring.js
+++ b/test/parallel/test-querystring.js
@@ -78,9 +78,16 @@ const qsTestCases = [
   ['a=b&=c&d=e', 'a=b&=c&d=e', { a: 'b', '': 'c', d: 'e' }],
   ['a=b&=&c=d', 'a=b&=&c=d', { a: 'b', '': '', c: 'd' }],
   ['&&foo=bar&&', 'foo=bar', { foo: 'bar' }],
+  ['&', '', {}],
   ['&&&&', '', {}],
   ['&=&', '=', { '': '' }],
   ['&=&=', '=&=', { '': [ '', '' ]}],
+  ['=', '=', { '': '' }],
+  ['+', '%20=', { ' ': '' }],
+  ['+=', '%20=', { ' ': '' }],
+  ['+&', '%20=', { ' ': '' }],
+  ['=+', '=%20', { '': ' ' }],
+  ['+=&', '%20=', { ' ': '' }],
   ['a&&b', 'a=&b=', { 'a': '', 'b': '' }],
   ['a=a&&b=b', 'a=a&b=b', { 'a': 'a', 'b': 'b' }],
   ['&a', 'a=', { 'a': '' }],
@@ -91,6 +98,14 @@ const qsTestCases = [
   ['a=&a=value&a=', 'a=&a=value&a=', { a: [ '', 'value', '' ] }],
   ['foo+bar=baz+quux', 'foo%20bar=baz%20quux', { 'foo bar': 'baz quux' }],
   ['+foo=+bar', '%20foo=%20bar', { ' foo': ' bar' }],
+  ['a+', 'a%20=', { 'a ': '' }],
+  ['=a+', '=a%20', { '': 'a ' }],
+  ['a+&', 'a%20=', { 'a ': '' }],
+  ['=a+&', '=a%20', { '': 'a ' }],
+  ['%20+', '%20%20=', { '  ': '' }],
+  ['=%20+', '=%20%20', { '': '  ' }],
+  ['%20+&', '%20%20=', { '  ': '' }],
+  ['=%20+&', '=%20%20', { '': '  ' }],
   [null, '', {}],
   [undefined, '', {}]
 ];