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

allow for leading whitespace in JSON #256

Merged
merged 1 commit into from Sep 7, 2016
Merged

Conversation

@wraithgar
Copy link
Contributor

wraithgar commented Aug 26, 2016

Some oauth providers (specifically AT&T) return their JSON payloads with lots of leading whitespace, which fails the current test of payload[0] === '{' and causes the response from get to be passed through Querystring.parse instead.

This PR is an attempt to allow for situations like this, and still properly catch these payloads as JSON.

I tried looking through the tests to see where I could add a case to ensure this works but didn't see anywhere that stood out to me. I'm more than happy to attempt that if given a little more direction on this.

@@ -558,7 +558,7 @@ internals.encode = function (string) {
internals.parse = function (payload) {

payload = Buffer.isBuffer(payload) ? payload.toString() : payload;
if (payload[0] === '{') {
if (/^\s*{/.test(payload)) {

This comment has been minimized.

Copy link
@nlf

nlf Aug 26, 2016

Member

just curious, but is calling payload.trim() to remove leading and trailing spaces enough to avoid the regex here?

can anyone think of any edge cases where maintaining leading or trailing whitespace would be necessary?

This comment has been minimized.

Copy link
@wraithgar

wraithgar Aug 26, 2016

Author Contributor

You could technically be clobbering Querystring params and/or values with whitespace if the test failed and it fell through to Querystring.parse. Obviously that's super edge case and I've never personally seen query params with whitespace in their names, but that's anecdotal and this felt safer.

This comment has been minimized.

Copy link
@kamronbatman

kamronbatman Aug 30, 2016

Contributor

Considering trim() is nondestructive to the original reference and JSON.parse() ignores leading/trailing whitespaces, then I don't see the possible clobbering. Why wouldn't something like this work?

payload = Buffer.isBuffer(payload) ? payload.toString() : payload;
if (payload.trim()[0] === '{') {

This comment has been minimized.

Copy link
@wraithgar

wraithgar Aug 30, 2016

Author Contributor

Yes you're right. For some reason I was assuming nlf was talking about using it destructively a la payload = payload.trim(). No clue why I thought that. I'll switch it to use trim instead.

@kamronbatman

This comment has been minimized.

Copy link
Contributor

kamronbatman commented Sep 6, 2016

@nlf I think this can be merged, what do you think?

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Sep 6, 2016

looks good to me

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Sep 7, 2016

Thank you so much! Yeah the JSON detection is definitely not the best.

@ldesplat ldesplat merged commit 3720640 into hapijs:master Sep 7, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldesplat ldesplat modified the milestones: 8.1.1, 8.1.2 Sep 7, 2016
@wraithgar wraithgar deleted the wraithgar:json_whitespace branch Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.