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

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

Closed
csga5000 opened this issue Jan 11, 2019 · 8 comments
Closed

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

csga5000 opened this issue Jan 11, 2019 · 8 comments

Comments

@csga5000
Copy link

  • Laravel Version: Any
  • PHP Version: Any
  • Database Driver & Version: Any

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
Copy link
Author

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
Copy link
Author

csga5000 commented Jan 11, 2019

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
Copy link
Contributor

ahinkle commented Jan 11, 2019

Duplicate of #22125

@driesvints
Copy link
Member

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
Copy link
Author

csga5000 commented Jan 11, 2019

@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
Copy link
Author

csga5000 commented Jan 11, 2019

@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
Copy link
Contributor

ahinkle commented Jan 14, 2019

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
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants