CVE-2012-6431

@@ -70,7 +70,10 @@ public function onKernelRequest(GetResponseEvent $event)
 
         // add attributes based on the path info (routing)
         try {
-            $parameters = $this->router->match($request->getPathInfo());
+            // The path is returned in decoded form from the request, so we need to
+            // encode it again as the router applies its own decoding. This prevents
+            // double-decoding.
+            $parameters = $this->router->match(urlencode($request->getPathInfo()));
 
             if (null !== $this->logger) {
                 $this->logger->info(sprintf('Matched route "%s" (parameters: %s)', $parameters['_route'], $this->parametersToString($parameters)));

@@ -107,7 +107,7 @@ public function checkRequestPath(Request $request, $path)
     {
         if ('/' !== $path[0]) {
             try {
-                $parameters = $this->router->match($request->getPathInfo());
+                $parameters = $this->router->match(urlencode($request->getPathInfo()));
 
                 return $path === $parameters['_route'];
             } catch (MethodNotAllowedException $e) {
@@ -129,7 +129,7 @@ private function resetLocale(Request $request)
         }
 
         try {
-            $parameters = $this->router->match($request->getPathInfo());
+            $parameters = $this->router->match(urlencode($request->getPathInfo()));
 
             if (isset($parameters['_locale'])) {
                 $context->setParameter('_locale', $parameters['_locale']);

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

@@ -130,7 +130,10 @@ private function addProfilerSection(ArrayNodeDefinition $rootNode)
                             ->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()

@@ -154,7 +154,11 @@ private function addAccessControlSection(ArrayNodeDefinition $rootNode)
                     ->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')

@@ -127,7 +127,7 @@ public function matches(Request $request)
         if (null !== $this->path) {
             $path = str_replace('#', '\\#', $this->path);
 
-            if (!preg_match('#'.$path.'#', $request->getPathInfo())) {
+            if (!preg_match('#'.$path.'#', rawurldecode($request->getPathInfo()))) {
                 return false;
             }
         }

@@ -16,6 +16,14 @@
 /**
  * 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 <fabien@symfony.com>
  *
  * @api
@@ -568,9 +576,10 @@ public function getScriptName()
      *
      *  * 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 @@ public function getPathInfo()
      *
      * 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 @@ public function getBasePath()
      * 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 @@ public function getHttpHost()
     /**
      * Returns the requested URI.
      *
-     * @return string
+     * @return string The raw URI (i.e. not urldecoded)
      *
      * @api
      */
@@ -1317,7 +1327,7 @@ protected function prepareBaseUrl()
         }
 
         $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 @@ protected function preparePathInfo()
             return $requestUri;
         }
 
-        return rawurldecode((string) $pathInfo);
+        return (string) $pathInfo;
     }
 
     /**

@@ -150,6 +150,14 @@ public function testPathWithLocaleIsNotSupported()
         $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();

@@ -315,6 +315,27 @@ public function testGetUri()
         $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 @@ public function getBaseUrlData()
                 '/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',

@@ -62,7 +62,7 @@ private function addMatcher($supportsRedirections)
     public function match(\$pathinfo)
     {
         \$allow = array();
-        \$pathinfo = urldecode(\$pathinfo);
+        \$pathinfo = rawurldecode(\$pathinfo);
 
 $code
 

@@ -43,8 +43,6 @@ public function getTraces($pathinfo)
 
     protected function matchCollection($pathinfo, RouteCollection $routes)
     {
-        $pathinfo = urldecode($pathinfo);
-
         foreach ($routes as $name => $route) {
             if ($route instanceof RouteCollection) {
                 if (!$ret = $this->matchCollection($pathinfo, $route)) {

@@ -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
      *

@@ -72,15 +72,22 @@ public function getContext()
     }
 
     /**
-     * {@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
      */
     public function match($pathinfo)
     {
         $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 @@ protected function mergeDefaults($params, $defaults)
         $parameters = $defaults;
         foreach ($params as $key => $value) {
             if (!is_int($key)) {
-                $parameters[$key] = rawurldecode($value);
+                $parameters[$key] = $value;
             }
         }
 

@@ -23,7 +23,7 @@ public function __construct(RequestContext $context)
     public function match($pathinfo)
     {
         $allow = array();
-        $pathinfo = urldecode($pathinfo);
+        $pathinfo = rawurldecode($pathinfo);
 
         // foo
         if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#xs', $pathinfo, $matches)) {

@@ -23,7 +23,7 @@ public function __construct(RequestContext $context)
     public function match($pathinfo)
     {
         $allow = array();
-        $pathinfo = urldecode($pathinfo);
+        $pathinfo = rawurldecode($pathinfo);
 
         // foo
         if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#xs', $pathinfo, $matches)) {

@@ -157,8 +157,8 @@ public function testMatchNonAlpha()
         $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 @@ public function testSchemeRequirement()
         $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'));
+    }
 }