Opt-in fix for double decoded query params using FlowRouter.decodeQueryParamsOnce = true flag#92
Merged
dr-dimitru merged 2 commits intoveliovgroup:masterfrom Jun 3, 2022
Merged
Conversation
…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.
|
Can we get this merged anytime soon? |
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`*
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
encodeURIComponenton 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:
The change takes place only on a single line.
Basically, the justification here is that
context.querystringis inappropriately decoded one additional time by thepagelibrary, whereascontext.pathis not, and it still contains the query string, which I was able to parse out using theURLclass. 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 theURLclass 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!