10000 bug #25427 Preserve percent-encoding in URLs when performing redirect… · symfony/symfony@01229fb · GitHub
[go: up one dir, main page]
Skip to content

Commit 01229fb

Browse files
fabpotfabpot
committed
bug #25427 Preserve percent-encoding in URLs when performing redirects 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
2 parents 9ff3776 + 8146510 commit 01229fb

File tree

Expand file tree

6 files changed

+97
-15
lines changed

6 files changed

+97
-15
lines changed

‎src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,10 @@ private function generateMatchMethod($supportsRedirections)
9696
$code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n");
9797

9898
return <<<EOF
99-
public function match(\$pathinfo)
99+
public function match(\$rawPathinfo)
100100
{
101101
\$allow = array();
102-
\$pathinfo = rawurldecode(\$pathinfo);
102+
\$pathinfo = rawurldecode(\$rawPathinfo);
103103
\$context = \$this->context;
104104
\$request = \$this->request;
105105
@@ -284,7 +284,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
284284
if ($hasTrailingSlash) {
285285
$code .= <<<EOF
286286
if (substr(\$pathinfo, -1) !== '/') {
287-
return \$this->redirect(\$pathinfo.'/', '$name');
287+
return \$this->redirect(\$rawPathinfo.'/', '$name');
288288
}
289289
290290
@@ -299,7 +299,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
299299
$code .= <<<EOF
300300
\$requiredSchemes = $schemes;
301301
if (!isset(\$requiredSchemes[\$this->context->getScheme()])) {
302-
return \$this->redirect(\$pathinfo, '$name', key(\$requiredSchemes));
302+
return \$this->redirect(\$rawPathinfo, '$name', key(\$requiredSchemes));
303303
}
304304
305305

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ public function __construct(RequestContext $context)
1515
$this->context = $context;
1616
}
1717

18-
public function match($pathinfo)
18+
public function match($rawPathinfo)
1919
{
2020
$allow = array();
21-
$pathinfo = rawurldecode($pathinfo);
21+
$pathinfo = rawurldecode($rawPathinfo);
2222
$context = $this->context;
2323
$request = $this->request;
2424

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ public function __construct(RequestContext $context)
1515
$this->context = $context;
1616
}
1717

18-
public function match($pathinfo)
18+
public function match($rawPathinfo)
1919
{
2020
$allow = array();
21-
$pathinfo = rawurldecode($pathinfo);
21+
$pathinfo = rawurldecode($rawPathinfo);
2222
$context = $this->context;
2323
$request = $this->request;
2424

@@ -67,7 +67,7 @@ public function match($pathinfo)
6767
// baz3
6868
if ('/test/baz3' === rtrim($pathinfo, '/')) {
6969
if (substr($pathinfo, -1) !== '/') {
70-
return $this->redirect($pathinfo.'/', 'baz3');
70+
return $this->redirect($rawPathinfo.'/', 'baz3');
7171
}
7272

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

8484
return $this->mergeDefaults(array_replace($matches, array('_route' => 'baz4')), array ());
@@ -171,7 +171,7 @@ public function match($pathinfo)
171171
// hey
172172
if ('/multi/hey' === rtrim($pathinfo, '/')) {
173173
if (substr($pathinfo, -1) !== '/') {
174-
return $this->redirect($pathinfo.'/', 'hey');
174+
return $this->redirect($rawPathinfo.'/', 'hey');
175175
}
176176

177177
return array('_route' => 'hey');
@@ -318,7 +318,7 @@ public function match($pathinfo)
318318
if ('/secure' === $pathinfo) {
319319
$requiredSchemes = array ( 'https' => 0,);
320320
if (!isset($requiredSchemes[$this->context->getScheme()])) {
321-
return $this->redirect($pathinfo, 'secure', key($requiredSchemes));
321+
return $this->redirect($rawPathinfo, 'secure', key($requiredSchemes));
322322
}
323323

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

334334
return array('_route' => 'nonsecure');

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ public function __construct(RequestContext $context)
1515
$this->context = $context;
1616
}
1717

18-
public function match($pathinfo)
18+
public function match($rawPathinfo)
1919
{
2020
$allow = array();
21-
$pathinfo = rawurldecode($pathinfo);
21+
$pathinfo = rawurldecode($rawPathinfo);
2222
$context = $this->context;
2323
$request = $this->request;
2424

‎src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,39 @@
1313

1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper;
16+
use Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface;
17+
use Symfony\Component\Routing\Matcher\UrlMatcher;
18+
use Symfony\Component\Routing\RequestContext;
1619
use Symfony\Component\Routing\Route;
1720
use Symfony\Component\Routing\RouteCollection;
1821

1922
class PhpMatcherDumperTest extends TestCase
2023
{
24+
/**
25+
* @var string
26+
*/
27+
private $matcherClass;
28+
29+
/**
30+
* @var string
31+
*/
32+
private $dumpPath;
33+
34+
protected function setUp()
35+
{
36+
parent::setUp();
37+
38+
$this->matcherClass = uniqid('ProjectUrlMatcher');
39+
$this->dumpPath = sys_get_temp_dir().DIRECTORY_SEPARATOR.'php_matcher.'.$this->matcherClass.'.php';
40+
}
41+
42+
protected function tearDown()
43+
{
44+
parent::tearDown();
45+
46+
@unlink($this->dumpPath);
47+
}
48+
2149
/**
2250
* @expectedException \LogicException
2351
*/
@@ -36,6 +64,23 @@ public function testDumpWhenSchemeIsUsedWithoutAProperDumper()
3664
$dumper->dump();
3765
}
3866

67+
public function testRedirectPreservesUrlEncoding()
68+
{
69+
$collection = new RouteCollection();
70+
$collection->add('foo', new Route('/foo:bar/'));
71+
72+
$class = $this->generateDumpedMatcher($collection, true);
73+
74+
$matcher = $this->getMockBuilder($class)
75+
->setMethods(array('redirect'))
76+
->setConstructorArgs(array(new RequestContext()))
77+
->getMock();
78+
79+
$matcher->expects($this->once())->method('redirect')->with('/foo%3Abar/', 'foo');
80+
81+
$matcher->match('/foo%3Abar');
82+
}
83+
3984
/**
4085
* @dataProvider getRouteCollections
4186
*/
@@ -285,4 +330,31 @@ public function getRouteCollections()
285330
array($rootprefixCollection, 'url_matcher3.php', array()),
286331
);
287332
}
333+
334+
/**
335+
* @param $dumper
336+
*/
337+
private function generateDumpedMatcher(RouteCollection $collection, $redirectableStub = false)
338+
{
339+
$options = array('class' => $this->matcherClass);
340+
341+
if ($redirectableStub) {
342+
$options['base_class'] = '\Symfony\Component\Routing\Tests\Matcher\Dumper\RedirectableUrlMatcherStub';
343+
}
344+
345+
$dumper = new PhpMatcherDumper($collection);
346+
$code = $dumper->dump($options);
347+
348+
file_put_contents($this->dumpPath, $code);
349+
include $this->dumpPath;
350+
351+
return $this->matcherClass;
352+
}
353+
}
354+
355+
abstract class RedirectableUrlMatcherStub extends UrlMatcher implements RedirectableUrlMatcherInterface
356+
{
357+
public function redirect($path, $route, $scheme = null)
358+
{
359+
}
288360
}

‎src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,14 @@ public function testNoSchemaRedirectIfOnOfMultipleSchemesMatches()
6969
;
7070
$matcher->match('/foo');
7171
}
72+
73+
public function testRedirectPreservesUrlEncoding()
74+
{
75+
$coll = new RouteCollection();
76+
$coll->add('foo', new Route('/foo:bar/'));
77+
78+
$matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, new RequestContext()));
79+
$matcher->expects($this->once())->method('redirect')->with('/foo%3Abar/');
80+
$matcher->match('/foo%3Abar');
81+
}
7282
}

0 commit comments

Comments
 (0)
0