From 55014a6841bec50046e8329a4835c160ac31a496 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Mon, 19 Mar 2012 15:59:52 +0100 Subject: [PATCH] [Routing] Request methods always return a raw path, fix the matcher to decode only once sq --- CHANGELOG-2.1.md | 7 +++++ .../DependencyInjection/Configuration.php | 5 +++- .../DependencyInjection/MainConfiguration.php | 6 +++- .../Component/HttpFoundation/Request.php | 28 +++++++++++++------ .../HttpFoundation/RequestMatcher.php | 2 +- .../Tests/RequestMatcherTest.php | 8 ++++++ .../HttpFoundation/Tests/RequestTest.php | 25 +++++++++++++++-- .../Matcher/Dumper/PhpMatcherDumper.php | 2 +- .../Routing/Matcher/TraceableUrlMatcher.php | 2 -- .../Component/Routing/Matcher/UrlMatcher.php | 13 +++++++-- .../Routing/Matcher/UrlMatcherInterface.php | 2 +- .../Tests/Fixtures/dumper/url_matcher1.php | 2 +- .../Tests/Fixtures/dumper/url_matcher2.php | 2 +- .../Routing/Tests/Matcher/UrlMatcherTest.php | 13 +++++++-- 14 files changed, 92 insertions(+), 25 deletions(-) diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index c0489a73ef..e535ccce20 100644 --- a/CHANGELOG-2.1.md +++ b/CHANGELOG-2.1.md @@ -329,6 +329,10 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * Request::getClientIp() method doesn't take a parameter anymore but bases itself on the trustProxy parameter. * Added isMethod() to Request object. + * [BC BREAK] The methods `getPathInfo()`, `getBaseUrl()` and `getBasePath()` of + a `Request` now all return a raw value (vs a urldecoded value before). Any call + to one of these methods must be checked and wrapped in a `rawurldecode()` if + needed. ### HttpKernel @@ -356,6 +360,9 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * added a TraceableUrlMatcher * added the possibility to define options, default values and requirements for placeholders in prefix, including imported routes * added RouterInterface::getRouteCollection + * [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were + decoded twice before. Note that the `urldecode()` calls have been change for a + single `rawurldecode()` in order to support `+` for input paths. ### Security diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index e6fe6f599f..96d01f9918 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php @@ -130,7 +130,10 @@ class Configuration implements ConfigurationInterface ->performNoDeepMerging() ->children() ->scalarNode('ip')->end() - ->scalarNode('path')->end() + ->scalarNode('path') + ->setInfo('use the urldecoded format') + ->setExample('^/path to resource/') + ->end() ->scalarNode('service')->end() ->end() ->end() diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php index d5f60c117d..607ebee527 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php @@ -154,7 +154,11 @@ class MainConfiguration implements ConfigurationInterface ->prototype('array') ->children() ->scalarNode('requires_channel')->defaultNull()->end() - ->scalarNode('path')->defaultNull()->end() + ->scalarNode('path') + ->defaultNull() + ->setInfo('use the urldecoded format') + ->setExample('^/path to resource/') + ->end() ->scalarNode('host')->defaultNull()->end() ->scalarNode('ip')->defaultNull()->end() ->arrayNode('methods') diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 09237f6d33..b4fd7ace51 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -16,6 +16,14 @@ use Symfony\Component\HttpFoundation\Session\SessionInterface; /** * Request represents an HTTP request. * + * The methods dealing with URL accept / return a raw path (% encoded): + * * getBasePath + * * getBaseUrl + * * getPathInfo + * * getRequestUri + * * getUri + * * getUriForPath + * * @author Fabien Potencier * * @api @@ -568,9 +576,10 @@ class Request * * * http://localhost/mysite returns an empty string * * http://localhost/mysite/about returns '/about' + * * htpp://localhost/mysite/enco%20ded returns '/enco%20ded' * * http://localhost/mysite/about?var=1 returns '/about' * - * @return string + * @return string The raw path (i.e. not urldecoded) * * @api */ @@ -588,11 +597,12 @@ class Request * * Suppose that an index.php file instantiates this request object: * - * * http://localhost/index.php returns an empty string - * * http://localhost/index.php/page returns an empty string - * * http://localhost/web/index.php return '/web' + * * http://localhost/index.php returns an empty string + * * http://localhost/index.php/page returns an empty string + * * http://localhost/web/index.php returns '/web' + * * http://localhost/we%20b/index.php returns '/we%20b' * - * @return string + * @return string The raw path (i.e. not urldecoded) * * @api */ @@ -613,7 +623,7 @@ class Request * This is similar to getBasePath(), except that it also includes the * script filename (e.g. index.php) if one exists. * - * @return string + * @return string The raw url (i.e. not urldecoded) * * @api */ @@ -698,7 +708,7 @@ class Request /** * Returns the requested URI. * - * @return string + * @return string The raw URI (i.e. not urldecoded) * * @api */ @@ -1317,7 +1327,7 @@ class Request } $basename = basename($baseUrl); - if (empty($basename) || !strpos(urldecode($truncatedRequestUri), $basename)) { + if (empty($basename) || !strpos(rawurldecode($truncatedRequestUri), $basename)) { // no match whatsoever; set it blank return ''; } @@ -1385,7 +1395,7 @@ class Request return $requestUri; } - return rawurldecode((string) $pathInfo); + return (string) $pathInfo; } /** diff --git a/src/Symfony/Component/HttpFoundation/RequestMatcher.php b/src/Symfony/Component/HttpFoundation/RequestMatcher.php index 0ca082d709..09c548c065 100644 --- a/src/Symfony/Component/HttpFoundation/RequestMatcher.php +++ b/src/Symfony/Component/HttpFoundation/RequestMatcher.php @@ -127,7 +127,7 @@ class RequestMatcher implements RequestMatcherInterface if (null !== $this->path) { $path = str_replace('#', '\\#', $this->path); - if (!preg_match('#'.$path.'#', $request->getPathInfo())) { + if (!preg_match('#'.$path.'#', rawurldecode($request->getPathInfo()))) { return false; } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestMatcherTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestMatcherTest.php index 20d989860c..069d8b4af2 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/RequestMatcherTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/RequestMatcherTest.php @@ -150,6 +150,14 @@ class RequestMatcherTest extends \PHPUnit_Framework_TestCase $this->assertFalse($matcher->matches($request)); } + public function testPathWithEncodedCharacters() + { + $matcher = new RequestMatcher(); + $request = Request::create('/admin/fo%20o'); + $matcher->matchPath('^/admin/fo o*$'); + $this->assertTrue($matcher->matches($request)); + } + public function testAttributes() { $matcher = new RequestMatcher(); diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php index 29f678a108..84f19a26c9 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php @@ -315,6 +315,27 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), $server); $this->assertEquals('http://servername/path/info?query=string', $request->getUri(), '->getUri() with rewrite, default port without HOST_HEADER'); + + // With encoded characters + + $server = array( + 'HTTP_HOST' => 'hostname:8080', + 'SERVER_NAME' => 'servername', + 'SERVER_PORT' => '8080', + 'QUERY_STRING' => 'query=string', + 'REQUEST_URI' => '/ba%20se/index_dev.php/foo%20bar/in+fo?query=string', + 'SCRIPT_NAME' => '/ba se/index_dev.php', + 'PATH_TRANSLATED' => 'redirect:/index.php/foo bar/in+fo', + 'PHP_SELF' => '/ba se/index_dev.php/path/info', + 'SCRIPT_FILENAME' => '/some/where/ba se/index_dev.php', + ); + + $request->initialize(array(), array(), array(), array(), array(), $server); + + $this->assertEquals( + 'http://hostname:8080/ba%20se/index_dev.php/foo%20bar/in+fo?query=string', + $request->getUri() + ); } /** @@ -984,14 +1005,14 @@ class RequestTest extends \PHPUnit_Framework_TestCase '/home', ), array( - '/foo%20bar/app.php/home%2Fbaz', + '/foo%20bar/app.php/home%3Dbaz', array( 'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/app.php', 'SCRIPT_NAME' => '/foo bar/app.php', 'PHP_SELF' => '/foo bar/app.php', ), '/foo%20bar/app.php', - '/home%2Fbaz', + '/home%3Dbaz', ), array( '/foo/bar+baz', diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index 9df0176407..571469005e 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php @@ -62,7 +62,7 @@ class PhpMatcherDumper extends MatcherDumper public function match(\$pathinfo) { \$allow = array(); - \$pathinfo = urldecode(\$pathinfo); + \$pathinfo = rawurldecode(\$pathinfo); $code diff --git a/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php b/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php index df8e89ee2c..1e2491c488 100644 --- a/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php +++ b/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php @@ -43,8 +43,6 @@ class TraceableUrlMatcher extends UrlMatcher protected function matchCollection($pathinfo, RouteCollection $routes) { - $pathinfo = urldecode($pathinfo); - foreach ($routes as $name => $route) { if ($route instanceof RouteCollection) { if (!$ret = $this->matchCollection($pathinfo, $route)) { diff --git a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php index eaae027392..ded89e637b 100644 --- a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php +++ b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php @@ -72,7 +72,14 @@ class UrlMatcher implements UrlMatcherInterface } /** - * {@inheritDoc} + * Tries to match a URL with a set of routes. + * + * @param string $pathinfo The path info to be parsed (raw format, i.e. not urldecoded) + * + * @return array An array of parameters + * + * @throws ResourceNotFoundException If the resource could not be found + * @throws MethodNotAllowedException If the resource was found but the request method is not allowed * * @api */ @@ -80,7 +87,7 @@ class UrlMatcher implements UrlMatcherInterface { $this->allow = array(); - if ($ret = $this->matchCollection(urldecode($pathinfo), $this->routes)) { + if ($ret = $this->matchCollection(rawurldecode($pathinfo), $this->routes)) { return $ret; } @@ -177,7 +184,7 @@ class UrlMatcher implements UrlMatcherInterface $parameters = $defaults; foreach ($params as $key => $value) { if (!is_int($key)) { - $parameters[$key] = rawurldecode($value); + $parameters[$key] = $value; } } diff --git a/src/Symfony/Component/Routing/Matcher/UrlMatcherInterface.php b/src/Symfony/Component/Routing/Matcher/UrlMatcherInterface.php index ce998ead60..58c5688ad9 100644 --- a/src/Symfony/Component/Routing/Matcher/UrlMatcherInterface.php +++ b/src/Symfony/Component/Routing/Matcher/UrlMatcherInterface.php @@ -30,7 +30,7 @@ interface UrlMatcherInterface extends RequestContextAwareInterface * If the matcher can not find information, it must throw one of the exceptions documented * below. * - * @param string $pathinfo The path info to be parsed + * @param string $pathinfo The path info to be parsed (raw format, i.e. not urldecoded) * * @return array An array of parameters * diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php index 6367119def..bc092a01c1 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php @@ -23,7 +23,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher public function match($pathinfo) { $allow = array(); - $pathinfo = urldecode($pathinfo); + $pathinfo = rawurldecode($pathinfo); // foo if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?Pbaz|symfony)$#xs', $pathinfo, $matches)) { diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php index 7a4d5b12a2..05cccf6158 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php @@ -23,7 +23,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec public function match($pathinfo) { $allow = array(); - $pathinfo = urldecode($pathinfo); + $pathinfo = rawurldecode($pathinfo); // foo if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?Pbaz|symfony)$#xs', $pathinfo, $matches)) { diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php index 14db7171cb..1d782b889d 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php @@ -157,8 +157,8 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase $collection->add('foo', new Route('/{foo}/bar', array(), array('foo' => '['.preg_quote($chars).']+'))); $matcher = new UrlMatcher($collection, new RequestContext(), array()); - $this->assertEquals(array('_route' => 'foo', 'foo' => $chars), $matcher->match('/'.urlencode($chars).'/bar')); - $this->assertEquals(array('_route' => 'foo', 'foo' => $chars), $matcher->match('/'.strtr($chars, array('%' => '%25', '+' => '%2B')).'/bar')); + $this->assertEquals(array('_route' => 'foo', 'foo' => $chars), $matcher->match('/'.rawurlencode($chars).'/bar')); + $this->assertEquals(array('_route' => 'foo', 'foo' => $chars), $matcher->match('/'.strtr($chars, array('%' => '%25')).'/bar')); } public function testMatchWithDotMetacharacterInRequirements() @@ -216,4 +216,13 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase $matcher = new UrlMatcher($coll, new RequestContext()); $matcher->match('/foo'); } + + public function testDecodeOnce() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/foo/{foo}')); + + $matcher = new UrlMatcher($coll, new RequestContext()); + $this->assertEquals(array('foo' => 'bar%23', '_route' => 'foo'), $matcher->match('/foo/bar%2523')); + } }