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

Routes with %2F not routing correctly #3592

Closed
SeriousKen opened this issue Jun 13, 2018 · 9 comments
Closed

Routes with %2F not routing correctly #3592

SeriousKen opened this issue Jun 13, 2018 · 9 comments

Comments

@SeriousKen
Copy link

Expected behavior

When a route of /calendar/:academicYear is created the URL /calendar/2018%2F2019 should route to a calendar with the academicYear parameter 2018/2019

Actual behavior

This results in a 404 as the routing seems to treat the %2F as an actual slash, presumably it is decoding the URL before splitting

https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters

Reproduce steps

Create the following page and try the above route

title = "calendar"
url = "/calendar/:academicYear"
is_hidden = 0
==
<?php
function onStart()
{
    $this['academicYear'] = $this->param('academicYear');
}
?>
==
{{ academicYear }}
October build

Build 436

@LukeTowers
Copy link
Contributor

@SeriousKen try throwing some debug statements in https://github.com/octobercms/october/blob/master/modules/cms/classes/Router.php to figure out when the URL gets converted

@SeriousKen
Copy link
Author

@LukeTowers the URL is decoded before it gets passed to the run method of the CmsController so it looks like it's the Laravel router itself that's doing the decoding. A search on Laravel's Gitub issues shows this to be the case, but there seems to be disagreement as to whether the current behaviour is correct or not.

@LukeTowers
Copy link
Contributor

@SeriousKen make a PR to octobercms/library implementing a solution along the lines of laravel/framework#4338 but make the behaviour configurable with a config.php entry in octobercms/october and set the default to be the current behaviour

@bennothommo
Copy link
Contributor

Closing as it has been over a month since any activity on this occurred and we are trying to figure out what issues are still relevant. If this is still something that you would like to see through to fruition please respond and we can get the ball rolling.

@LukeTowers
Copy link
Contributor

@SeriousKen I just ran into this issue myself while working on the image resizer PR. What did you end up doing to get around it?

@Samuell1
Copy link
Member

Samuell1 commented Aug 9, 2020

@LukeTowers Isnt better to split params to 2 separate and call them and merge them together in code? Like /calendar/2018/2019 then get firstyear and secondyear

And its not issue in laravel but symfony has this https://symfony.com/doc/current/routing.html#slash-characters-in-route-parameters

Another solution is that replace slashes with for example other character and them in code replace back to slash. Or maybe like said here guy encoded it twice laravel/framework#22125 (comment)

@LukeTowers
Copy link
Contributor

@Samuell1 in my case I'm trying to pass a URL as a parameter, which would require the use of a slash character. Double encoding it is a good idea though.

@LukeTowers
Copy link
Contributor

@Samuell1 also the symfony issue is different I believe, I think if you passed a URL encoded / to a route param in symfony it would work fine because it doesn't try to decode the path before parsing it like Laravel does.

@LukeTowers
Copy link
Contributor

Since changing this behaviour could cause breaking changes, we're going to leave it as it is and say that our official recommendation is to double encode any URL params that require slashes in them (note that Laravel's official recommendation is different, see https://github.com/laravel/docs/pull/4785/files).

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

No branches or pull requests

4 participants