Skip to content

Commit

Permalink
bug #23238 [Security] ensure the 'route' index is set before attempti…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
fabpot committed Jul 20, 2017
2 parents f4172b0 + 7ae578c commit f4fffc0
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/HttpUtils.php
Expand Up @@ -108,7 +108,7 @@ public function checkRequestPath(Request $request, $path)
$parameters = $this->urlMatcher->match($request->getPathInfo());
}

return $path === $parameters['_route'];
return isset($parameters['_route']) && $path === $parameters['_route'];
} catch (MethodNotAllowedException $e) {
return false;
} catch (ResourceNotFoundException $e) {
Expand Down
13 changes: 13 additions & 0 deletions src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php
Expand Up @@ -221,6 +221,19 @@ public function testCheckRequestPathWithUrlMatcherLoadingException()
$utils->checkRequestPath($this->getRequest(), 'foobar');
}

public function testCheckPathWithoutRouteParam()
{
$urlMatcher = $this->getMockBuilder('Symfony\Component\Routing\Matcher\UrlMatcherInterface')->getMock();
$urlMatcher
->expects($this->any())
->method('match')
->willReturn(array('_controller' => 'PathController'))
;

$utils = new HttpUtils(null, $urlMatcher);
$this->assertFalse($utils->checkRequestPath($this->getRequest(), 'path/index.html'));
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage Matcher must either implement UrlMatcherInterface or RequestMatcherInterface
Expand Down

0 comments on commit f4fffc0

Please sign in to comment.