From 4fcfa9d85c2e0f554b699a4a3125f399a7dbef0b Mon Sep 17 00:00:00 2001 From: Arjen van der Meijden Date: Wed, 24 Apr 2019 13:18:39 +0200 Subject: [PATCH] Fix url matcher edge cases with trailing slash --- .../Matcher/Dumper/PhpMatcherTrait.php | 18 +- .../Component/Routing/Matcher/UrlMatcher.php | 17 +- .../Routing/Tests/Matcher/UrlMatcherTest.php | 155 +++++++++++++++++- 3 files changed, 170 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php index b4b4256385..42cd41a915 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php @@ -15,11 +15,14 @@ use Symfony\Component\Routing\Exception\MethodNotAllowedException; use Symfony\Component\Routing\Exception\NoConfigurationException; use Symfony\Component\Routing\Exception\ResourceNotFoundException; use Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface; +use Symfony\Component\Routing\RequestContext; /** * @author Nicolas Grekas * * @internal + * + * @property RequestContext $context */ trait PhpMatcherTrait { @@ -89,13 +92,6 @@ trait PhpMatcherTrait continue; } - if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) { - if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) { - return $allow = $allowSchemes = []; - } - continue; - } - if ($requiredHost) { if ('#' !== $requiredHost[0] ? $requiredHost !== $host : !preg_match($requiredHost, $host, $hostMatches)) { continue; @@ -106,6 +102,13 @@ trait PhpMatcherTrait } } + if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) { + if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) { + return $allow = $allowSchemes = []; + } + continue; + } + $hasRequiredScheme = !$requiredSchemes || isset($requiredSchemes[$context->getScheme()]); if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) { if ($hasRequiredScheme) { @@ -113,6 +116,7 @@ trait PhpMatcherTrait } continue; } + if (!$hasRequiredScheme) { $allowSchemes += $requiredSchemes; continue; diff --git a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php index 536ed732ad..7b2662a2a3 100644 --- a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php +++ b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php @@ -32,6 +32,7 @@ class UrlMatcher implements UrlMatcherInterface, RequestMatcherInterface const REQUIREMENT_MISMATCH = 1; const ROUTE_MATCH = 2; + /** @var RequestContext */ protected $context; /** @@ -166,14 +167,6 @@ class UrlMatcher implements UrlMatcherInterface, RequestMatcherInterface } } - if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) { - if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) { - return $this->allow = $this->allowSchemes = []; - } - - continue; - } - $hostMatches = []; if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) { continue; @@ -185,6 +178,14 @@ class UrlMatcher implements UrlMatcherInterface, RequestMatcherInterface continue; } + if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) { + if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) { + return $this->allow = $this->allowSchemes = []; + } + + continue; + } + $hasRequiredScheme = !$route->getSchemes() || $route->hasScheme($this->context->getScheme()); if ($requiredMethods) { if (!\in_array($method, $requiredMethods)) { diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php index ac3816f893..8f99f7b8e9 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php @@ -85,9 +85,8 @@ class UrlMatcherTest extends TestCase } } - public function testMatch() + public function testPatternMatchAndParameterReturn() { - // test the patterns are matched and parameters are returned $collection = new RouteCollection(); $collection->add('foo', new Route('/foo/{bar}')); $matcher = $this->getUrlMatcher($collection); @@ -96,14 +95,21 @@ class UrlMatcherTest extends TestCase $this->fail(); } catch (ResourceNotFoundException $e) { } - $this->assertEquals(['_route' => 'foo', 'bar' => 'baz'], $matcher->match('/foo/baz')); + $this->assertEquals(['_route' => 'foo', 'bar' => 'baz'], $matcher->match('/foo/baz')); + } + + public function testDefaultsAreMerged() + { // test that defaults are merged $collection = new RouteCollection(); $collection->add('foo', new Route('/foo/{bar}', ['def' => 'test'])); $matcher = $this->getUrlMatcher($collection); $this->assertEquals(['_route' => 'foo', 'bar' => 'baz', 'def' => 'test'], $matcher->match('/foo/baz')); + } + public function testMethodIsIgnoredIfNoMethodGiven() + { // test that route "method" is ignored if no method is given in the context $collection = new RouteCollection(); $collection->add('foo', new Route('/foo', [], [], [], '', [], ['get', 'head'])); @@ -123,8 +129,10 @@ class UrlMatcherTest extends TestCase $this->assertInternalType('array', $matcher->match('/foo')); $matcher = $this->getUrlMatcher($collection, new RequestContext('', 'head')); $this->assertInternalType('array', $matcher->match('/foo')); + } - // route with an optional variable as the first segment + public function testRouteWithOptionalVariableAsFirstSegment() + { $collection = new RouteCollection(); $collection->add('bar', new Route('/{bar}/foo', ['bar' => 'bar'], ['bar' => 'foo|bar'])); $matcher = $this->getUrlMatcher($collection); @@ -136,8 +144,10 @@ class UrlMatcherTest extends TestCase $matcher = $this->getUrlMatcher($collection); $this->assertEquals(['_route' => 'bar', 'bar' => 'foo'], $matcher->match('/foo')); $this->assertEquals(['_route' => 'bar', 'bar' => 'bar'], $matcher->match('/')); + } - // route with only optional variables + public function testRouteWithOnlyOptionalVariables() + { $collection = new RouteCollection(); $collection->add('bar', new Route('/{foo}/{bar}', ['foo' => 'foo', 'bar' => 'bar'], [])); $matcher = $this->getUrlMatcher($collection); @@ -512,6 +522,141 @@ class UrlMatcherTest extends TestCase $this->assertEquals(['foo' => 'bar', '_route' => 'bar', 'locale' => 'en'], $matcher->match('/bar/bar')); } + public function testVariationInTrailingSlashWithHosts() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com')); + $coll->add('bar', new Route('/foo', [], [], [], 'bar.example.com')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com')); + $this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com')); + $this->assertEquals(['_route' => 'bar'], $matcher->match('/foo')); + } + + public function testVariationInTrailingSlashWithHostsInReverse() + { + // The order should not matter + $coll = new RouteCollection(); + $coll->add('bar', new Route('/foo', [], [], [], 'bar.example.com')); + $coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com')); + $this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com')); + $this->assertEquals(['_route' => 'bar'], $matcher->match('/foo')); + } + + public function testVariationInTrailingSlashWithHostsAndVariable() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/{foo}/', [], [], [], 'foo.example.com')); + $coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com')); + $this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com')); + $this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar')); + } + + public function testVariationInTrailingSlashWithHostsAndVariableInReverse() + { + // The order should not matter + $coll = new RouteCollection(); + $coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com')); + $coll->add('foo', new Route('/{foo}/', [], [], [], 'foo.example.com')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com')); + $this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com')); + $this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar')); + } + + public function testVariationInTrailingSlashWithMethods() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST'])); + $coll->add('bar', new Route('/foo', [], [], [], '', [], ['GET'])); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST')); + $this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET')); + $this->assertEquals(['_route' => 'bar'], $matcher->match('/foo')); + } + + public function testVariationInTrailingSlashWithMethodsInReverse() + { + // The order should not matter + $coll = new RouteCollection(); + $coll->add('bar', new Route('/foo', [], [], [], '', [], ['GET'])); + $coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST'])); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST')); + $this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET')); + $this->assertEquals(['_route' => 'bar'], $matcher->match('/foo')); + } + + public function testVariableVariationInTrailingSlashWithMethods() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/{foo}/', [], [], [], '', [], ['POST'])); + $coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET'])); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST')); + $this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET')); + $this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar')); + } + + public function testVariableVariationInTrailingSlashWithMethodsInReverse() + { + // The order should not matter + $coll = new RouteCollection(); + $coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET'])); + $coll->add('foo', new Route('/{foo}/', [], [], [], '', [], ['POST'])); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST')); + $this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET')); + $this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar')); + } + + public function testMixOfStaticAndVariableVariationInTrailingSlashWithHosts() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com')); + $coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com')); + $this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com')); + $this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar')); + } + + public function testMixOfStaticAndVariableVariationInTrailingSlashWithMethods() + { + $coll = new RouteCollection(); + $coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST'])); + $coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET'])); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST')); + $this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/')); + + $matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET')); + $this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar')); + $this->assertEquals(['foo' => 'foo', '_route' => 'bar'], $matcher->match('/foo')); + } + /** * @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException */