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

[Security] ensure the 'route' index is set before attempting to use it #23238

Closed
wants to merge 1 commit into from

Conversation

@gsdevme
Copy link
Contributor

gsdevme commented Jun 20, 2017

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.

@fabpot fabpot changed the base branch from master to 2.7 Jun 20, 2017
@fabpot fabpot changed the base branch from 2.7 to 2.8 Jun 20, 2017
@fabpot fabpot changed the base branch from 2.8 to master Jun 20, 2017
@keradus

This comment has been minimized.

Copy link
Member

keradus commented Jun 21, 2017

utests are missing

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jun 21, 2017

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

@gsdevme gsdevme changed the base branch from master to 2.8 Jun 21, 2017
@gsdevme gsdevme changed the base branch from 2.8 to master Jun 21, 2017
@gsdevme gsdevme force-pushed the gsdevme:patch-1 branch from b47a0a0 to 0b08001 Jun 21, 2017
@gsdevme gsdevme changed the base branch from master to 2.8 Jun 21, 2017
@nicolas-grekas nicolas-grekas changed the title fix(security): ensure the 'route' index is set before attempting to u… fix(security): ensure the 'route' index is set before attempting to use it Jul 3, 2017
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jul 3, 2017

Status: needs work

@fabpot fabpot changed the title fix(security): ensure the 'route' index is set before attempting to use it [Security] ensure the 'route' index is set before attempting to use it Jul 19, 2017
@fabpot
fabpot approved these changes Jul 19, 2017
Copy link
Member

fabpot left a comment

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?

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 19, 2017

Sure, I will try to get around to it tonight.

@xabbuh
xabbuh approved these changes Jul 19, 2017
@gsdevme gsdevme force-pushed the gsdevme:patch-1 branch from 0b08001 to 601274d Jul 19, 2017
@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 19, 2017

@fabian Test added to assert not passing the _route doesn't cause issues and returns false.

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.

@gsdevme gsdevme force-pushed the gsdevme:patch-1 branch from 601274d to 3c04ca6 Jul 19, 2017
@gsdevme gsdevme force-pushed the gsdevme:patch-1 branch from 3c04ca6 to 316e9c7 Jul 19, 2017
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jul 20, 2017

Should be merged into the 2.7 branch, right?

@fabpot
fabpot approved these changes Jul 20, 2017
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jul 20, 2017

Thank you @gsdevme.

fabpot added a commit that referenced this pull request Jul 20, 2017
…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
@fabpot fabpot closed this Jul 20, 2017
@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 20, 2017

@gsdevme gsdevme deleted the gsdevme:patch-1 branch Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.