Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

querystring.parse decodes incorrect in a specific case #13773

Closed
zkd8907 opened this issue Jun 19, 2017 · 5 comments
Closed

querystring.parse decodes incorrect in a specific case #13773

zkd8907 opened this issue Jun 19, 2017 · 5 comments
Labels
confirmed-bug Issues with confirmed bugs. querystring Issues and PRs related to the built-in querystring module.

Comments

@zkd8907
Copy link

zkd8907 commented Jun 19, 2017

  • Version: 8.0.0/8.1.2
  • Platform: Windows 10 x64/Linux x64

At Node 7.* or before, the result of require('querystring').parse('a=%20+&') is { a: ' ' }. However, at Node 8.0.0 or 8.1.2, the result of require('querystring').parse('a=%20+&') is { a: '%20 ' }. %20 doesn't decode when it is before +.

@vsemozhetbyt vsemozhetbyt added querystring Issues and PRs related to the built-in querystring module. v8.x zlib Issues and PRs related to the zlib subsystem. and removed zlib Issues and PRs related to the zlib subsystem. labels Jun 19, 2017
@targos
Copy link
Member

targos commented Jun 19, 2017

That behavior changed in #11234

/cc @mscdex

@TimothyGu
Copy link
Member

Aside from the issue, it is generally recommended that you use the new URLSearchParams class that parses query strings the exact same way browsers do, which is also faster than querystring module in almost all cases.

@addaleax addaleax removed the v8.x label Jun 30, 2017
@jsilveira
Copy link

After 5 hours of struggling with a an intermittent issue in the encoding of some of the POSTs received from our website, we tracked down the issue to this change in handling trailing spaces. The "random" nature of the issue was caused by the fact that some people submitted the form input with a trailing space.

So, is this a bug or an expected "changed behaviour" ???

@TimothyGu
Copy link
Member

There are a lot more problematic cases

// str    v8.x     v6.x         expected
'af+'     {}       {}            { 'af ': '' }
'af+&'    {}       { 'af ': '' } { 'af ': '' }     REGRESSION
'%20+'    {}       {}            { '  ': '' }
'%20+&'   {}       { '  ': '' }  { '  ': '' }      REGRESSION
'+'       {}       {}            { ' ': '' }
'+&'      {}       { ' ': '' }   { ' ': '' }       REGRESSION

@TimothyGu
Copy link
Member

@jsilveira It is a bug.

TimothyGu added a commit to TimothyGu/node that referenced this issue Jul 10, 2017
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.

Fixes: nodejs#13773
@targos targos added the confirmed-bug Issues with confirmed bugs. label Jul 10, 2017
addaleax pushed a commit that referenced this issue Jul 12, 2017
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: #14151
Fixes: #13773
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
addaleax pushed a commit that referenced this issue Jul 18, 2017
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: #14151
Fixes: #13773
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
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: #14151
Fixes: #13773
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants