search

veliovgroup / flow-router

🚦 Carefully extended flow-router for Meteor
https://packosphere.com/ostrio/flow-router-extra
BSD 3-Clause "New" or "Revised" License
201 stars 30 forks source link

Opt-in fix for double decoded query params using FlowRouter.decodeQueryParamsOnce = true flag #92

Closed michaelcbrook closed 2 years ago

michaelcbrook commented 2 years ago

Hey there,

I've created this lightweight PR to fix a long-standing bug that has existed for a while that causes query params to get decoded twice, despite using encodeURIComponent on the query param values first. This issue has been discussed at length here but never fixed (the suggested code change in the conversation didn't fix it either). The proposal that was discussed was to introduce a flag that could be turned on to opt-in to this bug fix because there was concern that fixing this bug could break existing legacy applications. So that's what I did.

To apply this fix to FlowRouter, you would just use this flag:

FlowRouter.decodeQueryParamsOnce = true;

The change takes place only on a single line.

// OLD:
const queryParams = this._qs.parse(context.querystring);

// NEW:
const queryParams = this._qs.parse((this.decodeQueryParamsOnce) ? (new URL(context.path, "http://example.com")).searchParams.toString() : context.querystring);

Basically, the justification here is that context.querystring is inappropriately decoded one additional time by the page library, whereas context.path is not, and it still contains the query string, which I was able to parse out using the URL class. I could have parsed it out manually, but since we don't know if there's a hash in the URL or other calamities that could occur in a URL, I just used the URL class to parse out the query string predictably and reliably. Since it requires a base, I give it "example.com", but this part is completely ignored and unused so therefore it's irrelevant to the output.

If this flag is not enabled, there is no change to the code as it ran before, so this will not break any existing or legacy apps.

I ran all the tests with the flag on and off, and all tests passed.

The new flag has been fully documented in all the appropriate docs and READMEs as well. The docs also offer some examples of what the results look like with this flag turned off and on.

Links to discussions about this bug (even way back to kadira):

https://github.com/veliovgroup/flow-router/issues/78 https://github.com/kadirahq/flow-router/issues/465

Side note:

This doesn't appear to be the only place this bug occurs. I discovered that path params share this bug as well and get prematurely decoded even prior to regex matching, but I wasn't able to come up with a fix for that one. That appears to be a whole different beast. And I can't confirm whether or not this bug exists in hashbangs. I did not check that myself. But this fixes at least query params, which are the most critical in my mind and are what we've struggled with the most.

These problems seem to stem largely from the page package. I found these similar issues as well:

https://github.com/visionmedia/page.js/issues/216 https://github.com/visionmedia/page.js/issues/187 https://github.com/visionmedia/page.js/issues/306

Thank you for maintaining such an important package! I know it's not easy, but your efforts are appreciated, and I hope I've helped make this PR as plug-and-play as possible. Thank you!

perbergland commented 2 years ago

Can we get this merged anytime soon?