Skip to content
This repository has been archived by the owner on Nov 25, 2020. It is now read-only.

[Twig] Fixed missing query string encoding #119

Closed
wants to merge 1 commit into from
Closed

[Twig] Fixed missing query string encoding #119

wants to merge 1 commit into from

Conversation

cleentfaar
Copy link

Query parameters were not being encoded.

Reproduce: have your current url contain a parameter with a forward slash. E.g. something like /foo?param1=/this/is/weird/. As long as it's not a route parameter.

Appearantly the twig extension assumes the URL (_route) in the current request is always safe for templates, but that is not the case.

@stof
Copy link
Contributor

stof commented Aug 19, 2015

The query string can actually contain unencoded slashes.

The Symfony router already takes care of the proper URL encoding

@cleentfaar
Copy link
Author

That's strange, could you explain then why the router did not encode the query-string in my example? Could it be because it's not part of the actual route?

For what it's worth; can you reproduce this issue?

@stof
Copy link
Contributor

stof commented Aug 21, 2015

@cleentfaar because it is perfectly valid to have unencoded slashes in the query string (other chars need encoding and they are encoded)

@cleentfaar
Copy link
Author

Hm perhaps it just seems like this is the issue, I will dig a little deeper and report my findings here. Thanks for the heads-up.

@fieg
Copy link

fieg commented Aug 21, 2015

because it is perfectly valid to have unencoded slashes in the query string

@stof do you have some reference that states this? Only thing I could find was someone stating otherwise: http://stackoverflow.com/questions/1856785/characters-allowed-in-a-url#comment14982015_1856809

@stof
Copy link
Contributor

stof commented Aug 21, 2015

@fieg look at the 3.4 Query section of the RFC: http://www.ietf.org/rfc/rfc3986.txt

@stof
Copy link
Contributor

stof commented Aug 21, 2015

btw, unencoded question marks are also valid

@fieg
Copy link

fieg commented Aug 21, 2015

👍

@cleentfaar
Copy link
Author

Closing as it seems I spoke too soon; something else is causing this issue for us so we will solve it there. Thanks again @stof.

@cleentfaar cleentfaar closed this Aug 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants