Skip to content

Conversation

michaelcbrook
Copy link

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):

#78
kadirahq#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:

visionmedia/page.js#216
visionmedia/page.js#187
visionmedia/page.js#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!

…ppropriately get decoded only once instead of twice as one would expect. This has technically been a long-standing bug, but the opt-in flag was chosen to be created so we don't break legacy apps.
@perbergland
Copy link

Can we get this merged anytime soon?

@dr-dimitru dr-dimitru merged commit 512f7e4 into veliovgroup:master Jun 3, 2022
dr-dimitru added a commit that referenced this pull request Jun 8, 2022
__Major Changes:__

- ⚠️ It is recommended to set `decodeQueryParamsOnce` to `true`, fixing #78, pr #92, thanks to @michaelcbrook
- ⚠️ By default use `{decodeQueryParamsOnce : true}` in tests
- ⚠️ Deprecated `staringatlights:inject-data`, in favor of `communitypackages:inject-data`
- ⚠️ Deprecated `staringatlights:fast-render`, in favor of `communitypackages:fast-render`

__Changes:__

- 👷‍♂️ Merged #92, thanks to @michaelcbrook and @Pitchlyapp, fix #78
- 👨‍💻 Fix #93, thanks to @drone1
- 🤝 Support and compatibility with `communitypackages:inject-data`
- 🤝 Support and compatibility with `communitypackages:fast-render`
- 🤝 Compatibility with `meteor@2.7.3`

__Other changes:__

- 📔 Overall documentation update and minor refinement
- 👨‍🔬 Update test-suite to cover #93
- 🧹 Overall codebase cleanup
- 👷‍♂️ Removed unnecessary "monkey patching" around `Blaze.remove()`, thanks to @jankapunkt
  - 🔗 Related: meteor/blaze#372
  - 🔗 Related: meteor/blaze#375

__Dependencies:__

- 📦 `qs@6.10.3`, *was `v6.5.2`*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants