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

Is it valid not to throw error in querystring.encode? #3702

Closed
hakatashi opened this issue Nov 7, 2015 · 2 comments · Fixed by #5858
Closed

Is it valid not to throw error in querystring.encode? #3702

hakatashi opened this issue Nov 7, 2015 · 2 comments · Fixed by #5858
Labels
querystring Issues and PRs related to the built-in querystring module.

Comments

@hakatashi
Copy link

Through reading source code for querystring.escape, I found that querystring.escape doesn't throw any error since #847 merge, even when invalid surrogate pairs are supplied.

Real world example:

vagrant@vagrant-ubuntu-trusty-64:~$ nvm use 0.12
Now using node v0.12.7 (npm v2.11.3)
vagrant@vagrant-ubuntu-trusty-64:~$ node
> var qs = require('querystring')
undefined
> qs.stringify({foo: '\udc00'})
URIError: URI malformed
    at encodeURIComponent (native)
    at QueryString.escape (querystring.js:118:10)
    at Object.QueryString.stringify.QueryString.encode (querystring.js:154:26)
    at repl:1:4
    at REPLServer.defaultEval (repl.js:132:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:279:12)
    at REPLServer.emit (events.js:107:17)
    at REPLServer.Interface._onLine (readline.js:214:10)
>
(^C again to quit)
>
vagrant@vagrant-ubuntu-trusty-64:~$ nvm use 5
Now using node v5.0.0 (npm v3.3.6)
vagrant@vagrant-ubuntu-trusty-64:~$ node
> const qs = require('querystring')
undefined
> qs.stringify({foo: '\udc00'})
'foo=%F0%90%80%80'

Since I couldn't find any docs or comments about the behavior, I wonder if this is an intentional change or a regression. In either case, it should be included in its test.

@mscdex mscdex added the querystring Issues and PRs related to the built-in querystring module. label Nov 8, 2015
@Fishrock123
Copy link
Contributor

I guess ping @mscdex since he made that change?

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

@mscdex ... ping.

mscdex added a commit to mscdex/io.js that referenced this issue Mar 26, 2016
Fixes: nodejs#3702
PR-URL: nodejs#5858
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue Mar 30, 2016
Fixes: #3702
PR-URL: #5858
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue Mar 31, 2016
Fixes: #3702
PR-URL: #5858
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
mscdex added a commit to mscdex/io.js that referenced this issue Apr 8, 2016
Fixes: nodejs#3702
PR-URL: nodejs#5858
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 11, 2016
Fixes: #3702
PR-URL: #5858
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants