search

laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.46k stars 11k forks source link

[Bug] Failure to properly parse all valid URLs #27133

Closed csga5000 closed 5 years ago

csga5000 commented 5 years ago

Description:

When doing routing based on the URL Laravel parses hex values in the URL which should not happen. Other frameworks like Symfony don't have this issue. This means that your routing system isn't using the "URL" for routing you're "decoding" the URL, so it's routing is done on the string produced when the the entire URL has been decoded (which makes no sense, why would you ever want to urldecode an entire URL rather than it's individual parameters separately?).

Steps To Reproduce:

Pass url encoded strings are parameters in a URL and your route will note match

Ex:

Route::get('pdf_header/{patient}/{mrn}', 'TranscriptionController@pdf_header')

Navigate to http://app.com/pdf_header/johnsmith/123%2F123

csga5000 commented 5 years ago

My case is a bit complicated, but very common examples may include when putting blog post titles in URL's or something for SEO. I've seen other places (stack overflow, closed issues) where a lot of users have come across this issue as well

Blog ex: blog.com/2018/10/01/Why+laravel%E2s+routing+should+allow+urlencoded%2Fhexencoded+data

Which is the URL format for "Why laravel’s routing should allow urlencoded/hexencoded data" as the final param of http://blog.com/2018/10/01/{NAME}

Route::get('{year}/{month}/{day}/{postname}', 'TranscriptionController@pdf_header');
csga5000 commented 5 years ago

It also seems to partially urldecode incorrectly.

If we use our example from above but remove the /, then the arguments our action would get would be the following:

year = '2018'
month = '10'
day = '01'
postname = 'Why+laravel’s+routing+should+allow+urlencodedhexencoded+data'

I have no idea why but it appears to have parsed the hex codes but not the encoded spaces.

Easily fixed though:

$postname = urldecode($postname);

That replaces the + with ' ' properly.

ahinkle commented 5 years ago

Duplicate of #22125

driesvints commented 5 years ago

The encoded %2F problem you're showing above is indeed a duplicate of the issue linked above. At the moment the behavior should be the same as in Symfony. As noted in the issue linked above we won't be changing this any time soon.

There is a documented way to circumvent this though: https://laravel.com/docs/5.7/routing#parameters-encoded-forward-slashes

csga5000 commented 5 years ago

@driesvints It's a duplicate of a couple issues all of them are "closed", and many don't seem to be getting any attention despite dozens of 👍 . The one you linked has 32 👍 on the statement that it shouldn't be intended behavior.

This is an annoying issue that confuses developers all the time so they end up spending an hour or two thinking "What the heck is going on" until they realize "Oh laravel is being dumb"

This also breaks base64 encoded parameters because base64 uses / (of course you would anticipate making it URL safe before putting it in a param.

Your work around didn't work for me. Admittedly maybe that's because I'm on laravel 5.3 but I don't have time to upgrade the project right now just to apply a workaround - and really I think this is about making your library make "sense" and not make developers scratch there heads over something stupid before traversing docs and issue stuff.

csga5000 commented 5 years ago

@driesvints Also, your "work around" supposedly only works for the last argument. There's no reason there should be these kind of stupid and counter-intuitive restrictions! As a developer you should be-able to trust that url encoding data will prevent it from effecting how the URL is resolved. Can you imagine if Apache forced you to always treat %2F as /?

ahinkle commented 5 years ago

If you “don’t have time” to upgrade your project to the latest security and patches that have solutions to your problem; that’s a problem within itself.

I would recommend using https://laravelshift.com/ to upgrade your project to the latest version. It’s quick and painless.

csga5000 commented 5 years ago

And ignore any other good point made here. It's not a great solution and actually does not solve my problem because I'm using two arugments, and is there even a guarantee the version is why it's not working?