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: using toString for objects on querystring.escape #5341

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@silentroach
Copy link
Contributor

commented Feb 20, 2016

querystring.encode unexpected behavior on objects, see #5309

here is specs for encodeURIComponent - http://www.ecma-international.org/ecma-262/5.1/#sec-15.1.3.4, so object must be casted to string via toString, not via valueOf.

const qs = require('querystring');

const testObject = { toString: () => 'test', valueOf: () => 5 };

console.log('expected', encodeURIComponent(testObject));   // 'test'
console.log('actual',   qs.escape(testObject));            // 5

Here is why:

> '' + {toString: () => 'test', valueOf: () => 5}
5

String({toString: () => 'test', valueOf: () => 5})
'test'

@silentroach silentroach changed the title using toString for objects on querystring.escape (refs #5309) querystring: using toString for objects on querystring.escape Feb 20, 2016

@mscdex mscdex added the querystring label Feb 20, 2016

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2016

I just ran some quick benchmarks and it seems that using the String() constructor is currently significantly slower on objects than simply calling its .toString(). It would need further benchmarking, but perhaps putting an extra conditional to avoid that particular case might help things.

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Feb 20, 2016

@silentroach

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2016

@mscdex rewritten with typecheck

@jacobp100

This comment has been minimized.

Copy link

commented Feb 20, 2016

Not quite.

const x = Symbol('test');
'' + x; // throws
String(x) // 'Symbol(test)'
@silentroach

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2016

@jacobp100 the same with encodeURIComponent:

> encodeURIComponent(Symbol('test'))
TypeError: Cannot convert a Symbol value to a string

> '' + Symbol('test')
TypeError: Cannot convert a Symbol value to a string

so the current solution is better than String constructor. Thank you for additional test case :)

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Feb 20, 2016

@jacobp100

This comment has been minimized.

Copy link

commented Feb 20, 2016

You’re correct, good catch. Just checked, per spec, ToString should throw for symbols.

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Feb 20, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 21, 2016

LGTM

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

Are there any cases we can test where this would make a difference even if you are not setting toString()/valueOf()?

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

@Fishrock123 What do you mean? There will always be a toString() of some kind, whether you explicitly set one or not (if you don't, Object.prototype.toString() will get used).

@jacobp100

This comment has been minimized.

Copy link

commented Feb 23, 2016

Nah, he's right. If toString is not callable, then you need to try valueOf. If that's also not callable, you need to throw an error.

@silentroach silentroach force-pushed the silentroach:querystring-escape branch 4 times, most recently Feb 23, 2016

@silentroach

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2016

Fixed. + more tests

Seems like it is better to use String constructor instead all of these checks. What do you think about last commit, @mscdex?

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Feb 23, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

@mscdex @nodejs/ctc ... ping

@mscdex

View changes

lib/querystring.js Outdated
if (typeof str !== 'string') {
if (typeof str === 'object')
str = String(str);
else str += '';

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 24, 2016

Contributor

This should be moved to the next line.

This comment has been minimized.

Copy link
@silentroach

silentroach Mar 24, 2016

Author Contributor

but why? is there any reason to add empty string to str if it is already a string?

please check out tests below, maybe I forgot some cases?

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 24, 2016

Contributor

No, I meant this as a style nit: moving the str += ''; to the next line instead of on the same line as the else.

This comment has been minimized.

Copy link
@silentroach

silentroach Mar 24, 2016

Author Contributor

oh. will fix in 5 minutes, thank you

@silentroach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

@mscdex your variant will fail for this test

// toString is not callable, must throw an error
assert.throws(() => qs.escape({toString: 5}));

and in my version there is no problem with encoding symbols - it will throw as with encodeURIComponent

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

LGTM with one minor nit

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Mar 24, 2016

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

One failure in CI that looks unrelated.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

@mscdex ... any further comments on this one?

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 4, 2016

@silentroach ... can you update the commit log to follow our style guidelines here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2016

@jasnell Nope, other than commit message needs to be formatted correctly

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Apr 5, 2016

@silentroach silentroach force-pushed the silentroach:querystring-escape branch Apr 5, 2016

@silentroach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2016

@jasnell, @mscdex fixed. Is it ok now?

querystring: using toString for objects on querystring.escape
This commit fixes an inconsistency in querystring.escape objects handling
compared to native encodeURIComponent function.

Fixes: #5309
PR-URL: #5341

@silentroach silentroach force-pushed the silentroach:querystring-escape branch to 7247aca Apr 6, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Yep, LGTM

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

New CI, just to be extra careful ;-) https://ci.nodejs.org/job/node-test-pull-request/2244/

jasnell added a commit that referenced this pull request Apr 12, 2016

querystring: using toString for objects on querystring.escape
This commit fixes an inconsistency in querystring.escape objects handling
compared to native encodeURIComponent function.

Fixes: #5309
PR-URL: #5341
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Landed in 5dafb43

@jasnell jasnell closed this Apr 12, 2016

@silentroach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2016

Thank you :}

jasnell added a commit that referenced this pull request Apr 26, 2016

querystring: using toString for objects on querystring.escape
This commit fixes an inconsistency in querystring.escape objects handling
compared to native encodeURIComponent function.

Fixes: #5309
PR-URL: #5341
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
You can’t perform that action at this time.