-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] ensure the 'route' index is set before attempting to use it #23238
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
Conversation
utests are missing |
They are not missing, they are not written to test this. I take it from that you wish a unit test to assert this. I will write a test for it and also fix the base branch to 2.8 |
Status: needs work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning false
is indeed what needs to be done here as we are in the case of a route name. Can you add some tests?
Sure, I will try to get around to it tonight. |
@fabian Test added to assert not passing the Testing src/Symfony/Component/Security/Http/Tests
............................................................... 63 / 245 ( 25%)
............................................................... 126 / 245 ( 51%)
.........................................................E..... 189 / 245 ( 77%)
......................................................S. 245 / 245 (100%)
Time: 577 ms, Memory: 14.00MB
There was 1 error:
1) Symfony\Component\Security\Http\Tests\HttpUtilsTest::testCheckPathWithoutRouteParam
Undefined index: _route
/Users/gavin/Sites/symfony/src/Symfony/Component/Security/Http/HttpUtils.php:115
/Users/gavin/Sites/symfony/src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php:235
/Users/gavin/Sites/symfony/vendor/symfony/phpunit-bridge/bin/.phpunit/phpunit-5.7/phpunit:5 Test before I made it green with the change. |
Should be merged into the |
Thank you @gsdevme. |
…ng to use it (gsdevme) This PR was submitted for the 2.8 branch but it was merged into the 2.7 branch instead (closes #23238). Discussion ---------- [Security] ensure the 'route' index is set before attempting to use it | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | ``` // matching a request is more powerful than matching a URL path + context, so try that first if ($this->urlMatcher instanceof RequestMatcherInterface) { $parameters = $this->urlMatcher->matchRequest($request); } else { $parameters = $this->urlMatcher->match($request->getPathInfo()); } return $path === $parameters['_route']; ``` Hi the issue here is the code is assuming a `_route` has been returned from the `match()` method.. however there is nothing to suggest that is always the case. For example if I just want to return a controller that is perhaps not added as an actual route I can & it works.. Although this will generate a notice warning. **In terms of what happens if the `_route` is not defined should it return `false?` or actually perform a similar condition as `return $path === rawurldecode($request->getPathInfo());` ** I have an implementation of a router that is just returning a controller path and its arguments without a `_route` which works aside from this notice. Commits ------- 7ae578c fix(security): ensure the 'route' index is set before attempting to use it
Hi the issue here is the code is assuming a
_route
has been returned from thematch()
method.. however there is nothing to suggest that is always the case. For example if I just want to return a controller that is perhaps not added as an actual route I can & it works.. Although this will generate a notice warning.**In terms of what happens if the
_route
is not defined should it returnfalse?
or actually perform a similar condition asreturn $path === rawurldecode($request->getPathInfo());
**I have an implementation of a router that is just returning a controller path and its arguments without a
_route
which works aside from this notice.