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

Fragments are double encoded #321

Closed
leofeyer opened this Issue Feb 7, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@leofeyer
Copy link
Member

leofeyer commented Feb 7, 2019

Affected version(s)

Contao 4.4+

Description

The changes from d13b832 (see #7829) cause problems. We first urldecode here:

// URL decode here (see #6232)
$strRequest = rawurldecode($strRequest);

And then again here:

\Input::setGet(urldecode($arrFragments[$i]), urldecode($arrFragments[$i+1]), true);

Correct: path: /foo/email/a+b@example.com -> Input::get('email'): a b@example.com
Wrong: path: /foo/email/a%2Bb@example.com -> Input::get('email'): a b@example.com
Wrong: path: /foo/email/a%252Bb@example.com -> Input::get('email'): a+b@example.com

@aschempp @ausi @backbone87 /cc

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Feb 7, 2019

It looks like rawurldecode was added first (in contao/core#6232) and that fix actually removed urldecode from the fragments (contao/core@33cee3b). So not sure why contao/core#7829 was ever necessary/relevant 🤔

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Feb 14, 2019

As discussed on Mumble this should work correctly:

  1. Split the request into fragments by /
  2. urldecode() all fragments
  3. when building the alias replace / by %2F for the affected fragments

@leofeyer leofeyer added defect and removed up for discussion labels Feb 14, 2019

@leofeyer leofeyer added this to the 4.4.35 milestone Feb 14, 2019

aschempp added a commit to aschempp/contao that referenced this issue Feb 15, 2019

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Feb 15, 2019

Unfortunately, it's a little more complicated. Symfony apparently fixed a similar issue a while ago (https://github.com/symfony/symfony/blame/master/src/Symfony/Component/Routing/CHANGELOG.md#L234) and use rawurldecode. They also do decode slashes in the path.

In regards to Contao, this means changing this in Contao 4.4 is not really useful, because in Contao 4.7 we use Symfony and therefore can't fix it the same way. I guess we need to discuss this again.

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Feb 17, 2019

Couldn’t we use the fix from #330 and replace urldecode with rawurldecode?
Not supporting the + char as a replacement for space shouldn’t be a problem if we use rawurlencode when outputting the URL IMO. And in Contao 4.7 we cannot support the + anyways because of Symfony, right?

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Feb 17, 2019

Not really. Symfony decodes the whole path, which means we should keep the first rawurldecode and remove it from the Input::setGet. It will also mean that an encoded slash will become a slash (and therefore a fragment), but that's how Symfony behaves… although it is also how Contao always behaved (since that line was added)

@leofeyer leofeyer assigned leofeyer and unassigned ausi Feb 17, 2019

leofeyer added a commit that referenced this issue Feb 19, 2019

@leofeyer

This comment has been minimized.

Copy link
Member Author

leofeyer commented Feb 19, 2019

Fixed in 3327de1 then.

@leofeyer leofeyer closed this Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment