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

PatternUtils.formatPattern replaces encoded space (%20) with + #2407

Closed
coopy opened this issue Oct 28, 2015 · 8 comments
Closed

PatternUtils.formatPattern replaces encoded space (%20) with + #2407

coopy opened this issue Oct 28, 2015 · 8 comments
Labels
Milestone

Comments

@coopy
Copy link

coopy commented Oct 28, 2015

This line causes a URL parameter to be encoded in a non-reversible way: https://github.com/rackt/react-router/blob/v1.0.0-rc3/modules/PatternUtils.js#L170

We have an IIS server that doesn't correctly resolve URLs because of this behavior.

The change was introduced here: d955d7a

I'd like to know the reason that this behavior was introduced, as it's somewhat unexpected. To quote @mjackson:

Apparently + is a special character in query strings, but not in the path portion of the URL.
#716 (comment)

I've not been able to find conclusive evidence in the URI spec, but considering that encoding and decoding should be symmetrical operations, this seems wrong. There is some discussion in this Stackoverflow thread.

Example:

var placeName = 'Laguna Beach';
var uri = formatPattern('/place/:placeName', { placeName: placeName });
decodeURI(uri);
// -> "/place/Laguna+Beach"
// Expected decoded URI would be "/place/Laguna Beach".
@taion
Copy link
Contributor

taion commented Oct 29, 2015

Good catch. Do you want to put together a PR?

@taion taion added the bug label Oct 29, 2015
@taion taion added this to the v1.0.0-final milestone Oct 29, 2015
@coopy
Copy link
Author

coopy commented Oct 29, 2015

Sure, not a problem.

@taion
Copy link
Contributor

taion commented Oct 29, 2015

It's interesting that we only call formatPattern from <Redirect>. You'd think a more consistent API to handle URL parameters in <Redirect> would be to just accept a function of params => new path.

@coopy
Copy link
Author

coopy commented Oct 29, 2015

In our application, we call formatPattern ourselves for reasons outlined in #1850.

@mjackson
Copy link
Member

You're right, we should be using %20 in the pathname portion of the URL. We already have a test that appears to be testing the wrong thing.

I'd like to use + instead of %20 in the query string. From the URI spec:

Within the query string, the plus sign is reserved as shorthand notation for a space. Therefore, real plus signs must be encoded. This method was used to make query URIs easier to pass in systems which did not allow spaces.

But that logic belongs in the history package (i.e. useQueries), not here.

@Download
Copy link
Contributor

I have a route like this:

<Route path="near/:city" component={StoresNear} />

In StoresNear, I print the :city parameter, like this:

<h2>Stores near {this.props.params.city}</h2>

I enter this URL into the address bar of the browser:

http://localhost:8080/near/My+City

I then get this header:

Stores near My+City

If I change the URL to use a percent-encoded space

http://localhost:8080/near/My%20City

it works:

Stores near My City

Question:
Is this expected behavior? I have been reading this bug report, as well as this old one and this more recent one, but I can't figure it out.

I didn't want to create a new issue so I hope you guys don't mind me reviving this one.

To me it feels like a bug that the plus symbol does not get decoded, but it might be a feature? I don't get it though because afaik, all servers will decode it to space...

@taion
Copy link
Contributor

taion commented Dec 24, 2015

This is expected. + only gets decoded in query strings, not in URLs proper.

@Download
Copy link
Contributor

@taion Ok, thanks for clearing that up. And Merry Christmas :)

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

No branches or pull requests

4 participants