Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

gsdevme
Copy link
Contributor

@gsdevme 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 June 20, 2017 17:33
@fabpot fabpot changed the base branch from 2.7 to 2.8 June 20, 2017 17:33
@fabpot fabpot changed the base branch from 2.8 to master June 20, 2017 17:34
@keradus
Copy link
Member

keradus commented Jun 21, 2017

utests are missing

@gsdevme
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 June 21, 2017 07:22
@gsdevme gsdevme changed the base branch from 2.8 to master June 21, 2017 07:23
@gsdevme gsdevme changed the base branch from master to 2.8 June 21, 2017 07:44
@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 nicolas-grekas added this to the 2.8 milestone Jul 3, 2017
@nicolas-grekas
Copy link
Member

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

@fabpot fabpot left a 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?

@gsdevme
Copy link
Contributor Author

gsdevme commented Jul 19, 2017

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

@gsdevme
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.

@xabbuh
Copy link
Member

xabbuh commented Jul 20, 2017

Should be merged into the 2.7 branch, right?

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

gsdevme commented Jul 20, 2017 via email

@gsdevme gsdevme deleted the patch-1 branch July 20, 2017 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants