Skip to content

Allow disabling urldecoding before route parsing. - #16110

Merged
markstory merged 1 commit into
4.xfrom
issue-15269
Nov 15, 2021
Merged

markstory merged 1 commit into
4.xfrom
issue-15269

Conversation

@ADmad

@ADmad ADmad commented Nov 13, 2021

Copy link
Copy Markdown
Member

Closes #15269

@ADmad ADmad added this to the 4.3.2 milestone Nov 13, 2021
@ADmad
ADmad force-pushed the issue-15269 branch 2 times, most recently from 86e0501 to 76536e1 Compare November 13, 2021 18:03
$expected = [
'controller' => 'Products',
'action' => 'test',
'pass' => ['xx/yy'],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So an unmatched passed parameter will always be decoded once while a matched one will not?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels off. We could re-urlencode the pass values, or disable the urldecoding there as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This decoding is due to the rawurldecode() in _parseArgs() which is called only for the part that matches the trailing * in the route template. As you can see in next test, no decoding is done for the value matching the named placeholder {slug}.

Also as per the issue #15269, he expects the decoded value xx/yy and not xx%2Fyy in passed args.

So maybe this special casing for * is intentional? Or perhaps this decoding should be done for ** instead of *?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe this special casing for * is intentional? Or perhaps this decoding should be done for ** instead of *?

I think the ** intentionally doesn't decode as it is intended to preserve as much as possible of the path. I think having the * use the route's urldecode setting instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the * use the route's urldecode setting instead.

Okay, I will update accordingly.

@ADmad
ADmad force-pushed the issue-15269 branch 2 times, most recently from 5fe50cb to 1b585e7 Compare November 15, 2021 13:39
@markstory
markstory merged commit 64d5c69 into 4.x Nov 15, 2021
@markstory
markstory deleted the issue-15269 branch November 15, 2021 16:08
markstory added a commit that referenced this pull request Jul 12, 2026
Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
markstory added a commit that referenced this pull request Jul 12, 2026
Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
markstory added a commit that referenced this pull request Jul 14, 2026
Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
markstory added a commit that referenced this pull request Jul 14, 2026
Handle %2f in routing more consistently

Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
markstory added a commit that referenced this pull request Jul 14, 2026
Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
markstory added a commit that referenced this pull request Jul 14, 2026
Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
markstory added a commit that referenced this pull request Jul 15, 2026
Handle %2f in routing more consistently

Currently path prefix matching does not treat %2f as /, and `Route` has
an option for it. In the past we've made changes in this area
(#18050, #16110) to make urldecoding optional, and to intentionally
decode urlencoding in path segments to support non-ascii applications.

We got a report on the security list for a potential issue where *if*
an application enforced authorization within path prefixed scopes, and
had fallback routes enabled, then one could potentially bypass the scoped
middleware and hit the fallback routes which inconsistently handle %2f.

These changes align the behavior of urldecoding between RouteCollection
and Route with a new shared internal function. I thought a function was
better than exposing a static method on a public class.

Thanks to Rotem Reiss for reporting this issue.
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

Successfully merging this pull request may close these issues.

Route.php urlencodes params once but decodes twice, causing params like xx%2Fyy to fail.

3 participants