Skip to content

Commit ada9aa0

Browse files
nicolas-grekasnicolas-grekas
committed
bug #31240 Fix url matcher edge cases with trailing slash (arjenm)
This PR was squashed before being merged into the 4.2 branch (closes #31240). Discussion ---------- Fix url matcher edge cases with trailing slash | Q | A | ------------- | --- | Branch? | master / 4.2 (not sure whether this should've been against 4.2) | Bug fix? | yes | New feature? | no | BC breaks? | no (I think... if so, in obscure route configurations like the ones that broke in 4.2.7 ;) ) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #30721 | License | MIT As stated in #30721 the "redirecting" (compiled) UrlMatcher ignored host-requirements when doing a 'matched url except for trailing slash difference'-redirect. With routes like this, you'd get 404's rather than 200's on the second routes. ```yaml host1.withTrail: path: /foo/ # host2/foo would become host2/foo/ due to this partial path-match host: host1 host2.withoutTrail: # A request for host2/foo should match this route path: /foo # host2/foo/ does not match this path host: host2 ``` This was caused by too eagerly testing whether a route could've worked with an additional trailing slash. If it could be, that would result in an attempt to redirect _before_ testing the host. The original url would get the additional slash and prior to actually redirecting, it'd be retested against the available routes. _That new url_ would actually match no routes, since now the host check for the first routes would actually be executed and fail the match for those routes. The adjusted path in the route does not match the second sets of routes... This PR moves the trailing-slash check to after the host checks. I've added several tests with variants on these edge cases. *Note:* This is a bug in 4.2 (and 4.3). I fixed it against master. It appears 4.2's PhpMatcherTrait was renamed to CompiledUrlMatcherTrait in 4.3. But that made it loose its history. So I'm not sure how to proceed from here. I can rebase it on 4.2 and do the change in PhpMatcherTrait, but that would probably fail to merge in master? I'm not nearly proficient enough in Symfony PR's to know how to solve all this ;) @nicolas-grekas also asked for these or similar tests to be added to 3.4 as 'proof' those routes worked in that version as well. I have not (yet) done so. The tests did work on the non-redirecting UrlMatcher without change (i.e. just running UrlMatcherTest), which implies - to me at least - the routes where properly defined and should not result in 404's simply because a RedirectableUrlMatcherInterface _can_ do redirects. Actually, since I needed to change UrlMatcher as well, I'm not convinced these bugs where totally absent in 3.4. They didn't occur with the compiled version, since the host check was the very first if-statement in that humongous if-tree it generated. PS, the branch name is a bit dramatic ;) Commits ------- 4fcfa9d Fix url matcher edge cases with trailing slash
2 parents 5d8f5fc + 4fcfa9d commit ada9aa0

File tree

3 files changed

+169
-19
lines changed

3 files changed

+169
-19
lines changed

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

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@
1515
use Symfony\Component\Routing\Exception\NoConfigurationException;
1616
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
1717
use Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface;
18+
use Symfony\Component\Routing\RequestContext;
1819

1920
/**
2021
* @author Nicolas Grekas <p@tchwork.com>
2122
*
2223
* @internal
24+
*
25+
* @property RequestContext $context
2326
*/
2427
trait PhpMatcherTrait
2528
{
@@ -89,13 +92,6 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
8992
continue;
9093
}
9194

92-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
93-
if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) {
94-
return $allow = $allowSchemes = [];
95-
}
96-
continue;
97-
}
98-
9995
if ($requiredHost) {
10096
if ('#' !== $requiredHost[0] ? $requiredHost !== $host : !preg_match($requiredHost, $host, $hostMatches)) {
10197
continue;
@@ -106,13 +102,21 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
106102
}
107103
}
108104

105+
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
106+
if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) {
107+
return $allow = $allowSchemes = [];
108+
}
109+
continue;
110+
}
111+
109112
$hasRequiredScheme = !$requiredSchemes || isset($requiredSchemes[$context->getScheme()]);
110113
if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
111114
if ($hasRequiredScheme) {
112115
$allow += $requiredMethods;
113116
}
114117
continue;
115118
}
119+
116120
if (!$hasRequiredScheme) {
117121
$allowSchemes += $requiredSchemes;
118122
continue;

‎src/Symfony/Component/Routing/Matcher/UrlMatcher.php

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ class UrlMatcher implements UrlMatcherInterface, RequestMatcherInterface
3232
const REQUIREMENT_MISMATCH = 1;
3333
const ROUTE_MATCH = 2;
3434

35+
/** @var RequestContext */
3536
protected $context;
3637

3738
/**
@@ -166,14 +167,6 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
166167
}
167168
}
168169

169-
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
170-
if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
171-
return $this->allow = $this->allowSchemes = [];
172-
}
173-
174-
continue;
175-
}
176-
177170
$hostMatches = [];
178171
if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) {
179172
continue;
@@ -185,6 +178,14 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
185178
continue;
186179
}
187180

181+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
182+
if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
183+
return $this->allow = $this->allowSchemes = [];
184+
}
185+
186+
continue;
187+
}
188+
188189
$hasRequiredScheme = !$route->getSchemes() || $route->hasScheme($this->context->getScheme());
189190
if ($requiredMethods) {
190191
if (!\in_array($method, $requiredMethods)) {

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

Lines changed: 149 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,8 @@ public function testMethodNotAllowedAggregatesAllowedMethods()
8585
}
8686
}
8787

88-
public function testMatch()
88+
public function testPatternMatchAndParameterReturn()
8989
{
90-
// test the patterns are matched and parameters are returned
9190
$collection = new RouteCollection();
9291
$collection->add('foo', new Route('/foo/{bar}'));
9392
$matcher = $this->getUrlMatcher($collection);
@@ -96,14 +95,21 @@ public function testMatch()
9695
$this->fail();
9796
} catch (ResourceNotFoundException $e) {
9897
}
98+
9999
$this->assertEquals(['_route' => 'foo', 'bar' => 'baz'], $matcher->match('/foo/baz'));
100+
}
100101

102+
public function testDefaultsAreMerged()
103+
{
101104
// test that defaults are merged
102105
$collection = new RouteCollection();
103106
$collection->add('foo', new Route('/foo/{bar}', ['def' => 'test']));
104107
$matcher = $this->getUrlMatcher($collection);
105108
$this->assertEquals(['_route' => 'foo', 'bar' => 'baz', 'def' => 'test'], $matcher->match('/foo/baz'));
109+
}
106110

111+
public function testMethodIsIgnoredIfNoMethodGiven()
112+
{
107113
// test that route "method" is ignored if no method is given in the context
108114
$collection = new RouteCollection();
109115
$collection->add('foo', new Route('/foo', [], [], [], '', [], ['get', 'head']));
@@ -123,8 +129,10 @@ public function testMatch()
123129
$this->assertInternalType('array', $matcher->match('/foo'));
124130
$matcher = $this->getUrlMatcher($collection, new RequestContext('', 'head'));
125131
$this->assertInternalType('array', $matcher->match('/foo'));
132+
}
126133

127-
// route with an optional variable as the first segment
134+
public function testRouteWithOptionalVariableAsFirstSegment()
135+
{
128136
$collection = new RouteCollection();
129137
$collection->add('bar', new Route('/{bar}/foo', ['bar' => 'bar'], ['bar' => 'foo|bar']));
130138
$matcher = $this->getUrlMatcher($collection);
@@ -136,8 +144,10 @@ public function testMatch()
136144
$matcher = $this->getUrlMatcher($collection);
137145
$this->assertEquals(['_route' => 'bar', 'bar' => 'foo'], $matcher->match('/foo'));
138146
$this->assertEquals(['_route' => 'bar', 'bar' => 'bar'], $matcher->match('/'));
147+
}
139148

140-
// route with only optional variables
149+
public function testRouteWithOnlyOptionalVariables()
150+
{
141151
$collection = new RouteCollection();
142152
$collection->add('bar', new Route('/{foo}/{bar}', ['foo' => 'foo', 'bar' => 'bar'], []));
143153
$matcher = $this->getUrlMatcher($collection);
@@ -512,6 +522,141 @@ public function testWithHostOnRouteCollection()
512522
$this->assertEquals(['foo' => 'bar', '_route' => 'bar', 'locale' => 'en'], $matcher->match('/bar/bar'));
513523
}
514524

525+
public function testVariationInTrailingSlashWithHosts()
526+
{
527+
$coll = new RouteCollection();
528+
$coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com'));
529+
$coll->add('bar', new Route('/foo', [], [], [], 'bar.example.com'));
530+
531+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
532+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
533+
534+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
535+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
536+
}
537+
538+
public function testVariationInTrailingSlashWithHostsInReverse()
539+
{
540+
// The order should not matter
541+
$coll = new RouteCollection();
542+
$coll->add('bar', new Route('/foo', [], [], [], 'bar.example.com'));
543+
$coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com'));
544+
545+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
546+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
547+
548+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
549+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
550+
}
551+
552+
public function testVariationInTrailingSlashWithHostsAndVariable()
553+
{
554+
$coll = new RouteCollection();
555+
$coll->add('foo', new Route('/{foo}/', [], [], [], 'foo.example.com'));
556+
$coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com'));
557+
558+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
559+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
560+
561+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
562+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
563+
}
564+
565+
public function testVariationInTrailingSlashWithHostsAndVariableInReverse()
566+
{
567+
// The order should not matter
568+
$coll = new RouteCollection();
569+
$coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com'));
570+
$coll->add('foo', new Route('/{foo}/', [], [], [], 'foo.example.com'));
571+
572+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
573+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
574+
575+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
576+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
577+
}
578+
579+
public function testVariationInTrailingSlashWithMethods()
580+
{
581+
$coll = new RouteCollection();
582+
$coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST']));
583+
$coll->add('bar', new Route('/foo', [], [], [], '', [], ['GET']));
584+
585+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
586+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
587+
588+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
589+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
590+
}
591+
592+
public function testVariationInTrailingSlashWithMethodsInReverse()
593+
{
594+
// The order should not matter
595+
$coll = new RouteCollection();
596+
$coll->add('bar', new Route('/foo', [], [], [], '', [], ['GET']));
597+
$coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST']));
598+
599+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
600+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
601+
602+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
603+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
604+
}
605+
606+
public function testVariableVariationInTrailingSlashWithMethods()
607+
{
608+
$coll = new RouteCollection();
609+
$coll->add('foo', new Route('/{foo}/', [], [], [], '', [], ['POST']));
610+
$coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET']));
611+
612+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
613+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
614+
615+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
616+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
617+
}
618+
619+
public function testVariableVariationInTrailingSlashWithMethodsInReverse()
620+
{
621+
// The order should not matter
622+
$coll = new RouteCollection();
623+
$coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET']));
624+
$coll->add('foo', new Route('/{foo}/', [], [], [], '', [], ['POST']));
625+
626+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
627+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
628+
629+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
630+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
631+
}
632+
633+
public function testMixOfStaticAndVariableVariationInTrailingSlashWithHosts()
634+
{
635+
$coll = new RouteCollection();
636+
$coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com'));
637+
$coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com'));
638+
639+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
640+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
641+
642+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
643+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
644+
}
645+
646+
public function testMixOfStaticAndVariableVariationInTrailingSlashWithMethods()
647+
{
648+
$coll = new RouteCollection();
649+
$coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST']));
650+
$coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET']));
651+
652+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
653+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
654+
655+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
656+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
657+
$this->assertEquals(['foo' => 'foo', '_route' => 'bar'], $matcher->match('/foo'));
658+
}
659+
515660
/**
516661
* @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
517662
*/

0 commit comments

Comments
 (0)