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

Query parameters with '//' in them get collapsed to '/' #102

Closed drone1 closed 7 months ago

drone1 commented 1 year ago

Google OAuth redirects to web apps with the scope included on the query string.

Example URL:

http://www.test.com/oauth-redirect?code=xxx&scope=https://www.googleapis.com/auth/gmail.modify

However, this line corrupts the query string, collapsing https:// to https:/:

https://github.com/veliovgroup/flow-router/blob/6961ad112968c8ed2c6a00e22ef9ce6581e1d4ac/client/router.js#L478

dr-dimitru commented 1 year ago

@drone1 I'm sure there was a reason for this .replace() call. Do you want to send a PR providing an option to disable this behavior?

drone1 commented 1 year ago

probably to remove erroneous double-slashes from the url. which may still be needed. but perhaps not for the query string.

On Sun 29. Jan 2023 at 19:50, dr.dimitru @.***> wrote:

@drone1 https://github.com/drone1 I'm sure there was a reason for this .replace() call. Do you want to send a PR providing an option to disable this behavior?

— Reply to this email directly, view it on GitHub https://github.com/veliovgroup/flow-router/issues/102#issuecomment-1407741311, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPDTHHMQQ2ID6VVGG33WU23WJANCNFSM6AAAAAAUGTRVJA . You are receiving this because you were mentioned.Message ID: @.***>

dr-dimitru commented 1 year ago

@drone1 then we would need to exclude https?:\/\/ from this replacement

drone1 commented 1 year ago

Sadly I don't have time to submit a PR, but hopefully this code can get someone else started:

const pathParts = path.split('?')
let fixedPath = pathParts[0].replace(/\/\/+/g, '/')
if (pathParts.length > 1) {
   fixedPath += `?${pathParts[1]}`
}
original.call(self, fixedPath, state, dispatch, push);