Query parameters get destroyed by being decoded twice

[perbergland]

The use case is for a roundtrip to a login page that then is supposed to take you back to the original page including its query parameters.

You will then have a returnUrl query parameter which has doubly-encoded query parameters which are not handled properly because flow-router seems to decode the query parameters twice when it is vital that they are only decoded once.

Example:

Original url:
http://localhost:3000/projects?param1=abc&redirectUrl=https%3A%2F%2Fwww.company.com%2Fabc123

Redirected to
http://localhost:3000/login?returnUrl=%2Fprojects%3Fparam1%3Dabc%26redirectUrl%3Dhttps%253A%252F%252Fwww.company.com%252Fabc123

The queryParams provided by FlowRouter to the route registered for /login will now be
{returnUrl: "/projects?param1=abc", redirectUrl: "https://www.company.com/abc123"}

i.e. the entire redirectUrl part of the returnUrl is lost.

The hack fix is to use qs directly instead of the queryParam provided by FlowRouter

qs.parse(window.location.search, {
ignoreQueryPrefix: true,
});

which gives the correct result:

{returnUrl: "/projects?param1=abc&redirectUrl=https%3A%2F%2Fwww.company.com%2Fabc123"}

This happens when using kadira:flowrouter and flowrouter-extra@3.7.4, regardless of using the latest qs version (6.9.3) or some earlier version like 6.5.1.

This is for Meteor 1.10.1

[dr-dimitru]

Hello @perbergland ,

Sadly this is a flaw of this package inherited from original FlowRouter.
Although that will be an easy fix, I'm not sure if it can be done without breaking current integrations.

Saying that this will require major version bump.
I already have plans for v4 to keep current API and write new codebase from the scratch for top performance and compatibility (for example now we can not even update dependencies to the latest releases).
Before v4 is released you have next options:

1. Fork this package and use it as local package placing it under /packages directory;
2. Use qs.parse directly as you suggested in OP;
3. Send a PR to enable/disable double-decoding conditionally with flag or settings.

Hope that helps.
Let's keep this issue open until it fixed by further releases.

[perbergland]

Ah OK that’s what I thought - "bug compatibility" is a bitch…

Could you point to the code that would need to change and then I’ll try to look into introducing some kind of configuration?

Caveat: I’ve never modified/published either an npm or meteor package before :)

[dr-dimitru]

Hello @perbergland

I’ve never modified/published either an npm or meteor package before

Hope you're excited to make a first step. Please, get familiar with our simple contribution guidelines.

Basically you want to fork this branch (as it's a fork of original Flow-Router itself) to your own so called "feature branch". After you finish modifications you going to submit a pull request (PR).

Before you submit a PR please make sure nothing is broken by running tests.

This is the piece if code where we want to set the new rule/option. My suggestion is to introduce flag avoidDoubleURIEncoding (you're welcome to suggest better name) and use it in this if(){}else{} condition:

        if (!this. avoidDoubleURIEncoding && this._specialChars.includes(paramArr[i])){
          _param += encodeURIComponent(encodeURIComponent(paramArr[i]));
        } else {
          try {
            _param += encodeURIComponent(paramArr[i]);
          } catch (e) {
            _param += paramArr[i];
          }
        }

[michaelcbrook]

@dr-dimitru and @perbergland, I also came across this bug and took it upon myself to follow the above recommendation and create an opt-in fix for it. Unfortunately, the above code didn't fix the issue, but the PR I submitted fixes it and passes all the tests. To use it, you would just apply:

FlowRouter.decodeQueryParamsOnce = true;

It's been over a year since this issue was first discussed, but hopefully this helps someone else struggling with the same issue!

[perbergland]

🎉🎉🎉🎉🙏🙏