Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
bug #25427 Preserve percent-encoding in URLs when performing redirect…
…s in the UrlMatcher (mpdude)

This PR was squashed before being merged into the 2.7 branch (closes #25427).

Discussion
----------

Preserve percent-encoding in URLs when performing redirects in the UrlMatcher

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

While investigating #25373, I found that when the *dumped* `UrlMatcher` performs redirections due to missing trailing slashes on URLs, it does so using an url*de*coded URL.

This is wrong, as it may lead to wrong interpretations of URLs upon the next request. For example, think of an URL that contains `%23` in the middle of the path info. Upon redirection, this will be turned into `#` with an obvious effect.

Commits
-------

8146510 Preserve percent-encoding in URLs when performing redirects in the UrlMatcher
  • Loading branch information
fabpot committed Dec 14, 2017
2 parents 9ff3776 + 8146510 commit 01229fb
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 15 deletions.
Expand Up @@ -96,10 +96,10 @@ private function generateMatchMethod($supportsRedirections)
$code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n");

return <<<EOF
public function match(\$pathinfo)
public function match(\$rawPathinfo)
{
\$allow = array();
\$pathinfo = rawurldecode(\$pathinfo);
\$pathinfo = rawurldecode(\$rawPathinfo);
\$context = \$this->context;
\$request = \$this->request;
Expand Down Expand Up @@ -284,7 +284,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
if ($hasTrailingSlash) {
$code .= <<<EOF
if (substr(\$pathinfo, -1) !== '/') {
return \$this->redirect(\$pathinfo.'/', '$name');
return \$this->redirect(\$rawPathinfo.'/', '$name');
}
Expand All @@ -299,7 +299,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$code .= <<<EOF
\$requiredSchemes = $schemes;
if (!isset(\$requiredSchemes[\$this->context->getScheme()])) {
return \$this->redirect(\$pathinfo, '$name', key(\$requiredSchemes));
return \$this->redirect(\$rawPathinfo, '$name', key(\$requiredSchemes));
}
Expand Down
Expand Up @@ -15,10 +15,10 @@ public function __construct(RequestContext $context)
$this->context = $context;
}

public function match($pathinfo)
public function match($rawPathinfo)
{
$allow = array();
$pathinfo = rawurldecode($pathinfo);
$pathinfo = rawurldecode($rawPathinfo);
$context = $this->context;
$request = $this->request;

Expand Down
Expand Up @@ -15,10 +15,10 @@ public function __construct(RequestContext $context)
$this->context = $context;
}

public function match($pathinfo)
public function match($rawPathinfo)
{
$allow = array();
$pathinfo = rawurldecode($pathinfo);
$pathinfo = rawurldecode($rawPathinfo);
$context = $this->context;
$request = $this->request;

Expand Down Expand Up @@ -67,7 +67,7 @@ public function match($pathinfo)
// baz3
if ('/test/baz3' === rtrim($pathinfo, '/')) {
if (substr($pathinfo, -1) !== '/') {
return $this->redirect($pathinfo.'/', 'baz3');
return $this->redirect($rawPathinfo.'/', 'baz3');
}

return array('_route' => 'baz3');
Expand All @@ -78,7 +78,7 @@ public function match($pathinfo)
// baz4
if (preg_match('#^/test/(?P<foo>[^/]++)/?$#s', $pathinfo, $matches)) {
if (substr($pathinfo, -1) !== '/') {
return $this->redirect($pathinfo.'/', 'baz4');
return $this->redirect($rawPathinfo.'/', 'baz4');
}

return $this->mergeDefaults(array_replace($matches, array('_route' => 'baz4')), array ());
Expand Down Expand Up @@ -171,7 +171,7 @@ public function match($pathinfo)
// hey
if ('/multi/hey' === rtrim($pathinfo, '/')) {
if (substr($pathinfo, -1) !== '/') {
return $this->redirect($pathinfo.'/', 'hey');
return $this->redirect($rawPathinfo.'/', 'hey');
}

return array('_route' => 'hey');
Expand Down Expand Up @@ -318,7 +318,7 @@ public function match($pathinfo)
if ('/secure' === $pathinfo) {
$requiredSchemes = array ( 'https' => 0,);
if (!isset($requiredSchemes[$this->context->getScheme()])) {
return $this->redirect($pathinfo, 'secure', key($requiredSchemes));
return $this->redirect($rawPathinfo, 'secure', key($requiredSchemes));
}

return array('_route' => 'secure');
Expand All @@ -328,7 +328,7 @@ public function match($pathinfo)
if ('/nonsecure' === $pathinfo) {
$requiredSchemes = array ( 'http' => 0,);
if (!isset($requiredSchemes[$this->context->getScheme()])) {
return $this->redirect($pathinfo, 'nonsecure', key($requiredSchemes));
return $this->redirect($rawPathinfo, 'nonsecure', key($requiredSchemes));
}

return array('_route' => 'nonsecure');
Expand Down
Expand Up @@ -15,10 +15,10 @@ public function __construct(RequestContext $context)
$this->context = $context;
}

public function match($pathinfo)
public function match($rawPathinfo)
{
$allow = array();
$pathinfo = rawurldecode($pathinfo);
$pathinfo = rawurldecode($rawPathinfo);
$context = $this->context;
$request = $this->request;

Expand Down
Expand Up @@ -13,11 +13,39 @@

use PHPUnit\Framework\TestCase;
use Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper;
use Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface;
use Symfony\Component\Routing\Matcher\UrlMatcher;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;

class PhpMatcherDumperTest extends TestCase
{
/**
* @var string
*/
private $matcherClass;

/**
* @var string
*/
private $dumpPath;

protected function setUp()
{
parent::setUp();

$this->matcherClass = uniqid('ProjectUrlMatcher');
$this->dumpPath = sys_get_temp_dir().DIRECTORY_SEPARATOR.'php_matcher.'.$this->matcherClass.'.php';
}

protected function tearDown()
{
parent::tearDown();

@unlink($this->dumpPath);
}

/**
* @expectedException \LogicException
*/
Expand All @@ -36,6 +64,23 @@ public function testDumpWhenSchemeIsUsedWithoutAProperDumper()
$dumper->dump();
}

public function testRedirectPreservesUrlEncoding()
{
$collection = new RouteCollection();
$collection->add('foo', new Route('/foo:bar/'));

$class = $this->generateDumpedMatcher($collection, true);

$matcher = $this->getMockBuilder($class)
->setMethods(array('redirect'))
->setConstructorArgs(array(new RequestContext()))
->getMock();

$matcher->expects($this->once())->method('redirect')->with('/foo%3Abar/', 'foo');

$matcher->match('/foo%3Abar');
}

/**
* @dataProvider getRouteCollections
*/
Expand Down Expand Up @@ -285,4 +330,31 @@ public function getRouteCollections()
array($rootprefixCollection, 'url_matcher3.php', array()),
);
}

/**
* @param $dumper
*/
private function generateDumpedMatcher(RouteCollection $collection, $redirectableStub = false)
{
$options = array('class' => $this->matcherClass);

if ($redirectableStub) {
$options['base_class'] = '\Symfony\Component\Routing\Tests\Matcher\Dumper\RedirectableUrlMatcherStub';
}

$dumper = new PhpMatcherDumper($collection);
$code = $dumper->dump($options);

file_put_contents($this->dumpPath, $code);
include $this->dumpPath;

return $this->matcherClass;
}
}

abstract class RedirectableUrlMatcherStub extends UrlMatcher implements RedirectableUrlMatcherInterface
{
public function redirect($path, $route, $scheme = null)
{
}
}
Expand Up @@ -69,4 +69,14 @@ public function testNoSchemaRedirectIfOnOfMultipleSchemesMatches()
;
$matcher->match('/foo');
}

public function testRedirectPreservesUrlEncoding()
{
$coll = new RouteCollection();
$coll->add('foo', new Route('/foo:bar/'));

$matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, new RequestContext()));
$matcher->expects($this->once())->method('redirect')->with('/foo%3Abar/');
$matcher->match('/foo%3Abar');
}
}

0 comments on commit 01229fb

Please sign in to comment.