Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decodeUri the path breaks encoding #551

Closed
aligon opened this issue Dec 5, 2017 · 4 comments
Closed

decodeUri the path breaks encoding #551

aligon opened this issue Dec 5, 2017 · 4 comments

Comments

@aligon
Copy link

aligon commented Dec 5, 2017

I'm given an ID that I need to put in the url, so I call encodeURIComponent on it to make it sure its safe, then decodeURIComponent to get the original ID back. The IDs are coming from a server that I can't control.

One of the IDs has a %20 in it, when I encodeURIComponent it becomes %2520. When that route is pushed to history the pathname is set calling decodeURI on the path which turns it back to %20. Calling decodeURIComponent on it then returns instead of the original %20 so it no longer matches the id.

@kevinliang
Copy link

kevinliang commented Dec 19, 2017

I too am having the same issue. It looks like the bug is happening because of this line: https://github.com/ReactTraining/history/blob/master/modules/LocationUtils.js#L35 . Any chance we can fix this? Right now there is no front end solution to mitigate for this. Thanks!

@knicklabs
Copy link

I'm running into the same issue here and I imagine many others are too whether they know it or not. There's a real legitimate need to maintain encoding as we may need to escape user input. A common scenario is when passing along a user-entered search query that contains an ampersand. For example, history.push('/search?query=this%26that') becomes /search?query=this&that which surprisingly changes the query to to "this" and adds a new empty query parameter for "that". This is a very surprising side effect I'd not expect from this library. I generally would not expect this library to change the URL I provide to history.push().

@prencher
Copy link

@mjackson Looking for some guidance or a fix here. Also happy to contribute a fix if you can provide guidance on how it should be addressed.

@mjackson
Copy link
Member

I'm pretty sure this is a dup of #505. Let's please only discuss in one spot, thanks!

(Happy to re-open if it's not a duplicate)

@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants