Skip to content

Commit

Permalink
bug #33244 [Router] Fix TraceableUrlMatcher behaviour with trailing s…
Browse files Browse the repository at this point in the history 
…lash (Xavier Leune)

This PR was merged into the 3.4 branch.

Discussion
----------

[Router] Fix TraceableUrlMatcher behaviour with trailing slash

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #32149
| License       | MIT
| Doc PR        | ¤
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->
This pull requests fixes the bug #32149. This issue was about TraceableUrlMatcher having a wrong behaviour regarding trailing slashes (according to UrlMatcher and documentation).

With this pull requests, the test class TraceableUrlMatcherTest now extends UrlMatcherTest, to prevent such behaviour digression.

Thanks @nicolas-grekas for his feedback on the issue #32149

Commits
-------

fd1cb44 [Router] Fix TraceableUrlMatcher behaviour with trailing slash
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Aug 20, 2019
2 parents 727d431 + fd1cb44 commit 9672f4c
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 33 deletions.
89 changes: 58 additions & 31 deletions src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php
View file
Expand Up @@ -52,10 +52,41 @@ public function getTracesForRequest(Request $request)

protected function matchCollection($pathinfo, RouteCollection $routes)
{
// HEAD and GET are equivalent as per RFC
if ('HEAD' === $method = $this->context->getMethod()) {
$method = 'GET';
}
$supportsTrailingSlash = '/' !== $pathinfo && '' !== $pathinfo && $this instanceof RedirectableUrlMatcherInterface;

foreach ($routes as $name => $route) {
$compiledRoute = $route->compile();
$staticPrefix = $compiledRoute->getStaticPrefix();
$requiredMethods = $route->getMethods();

// check the static prefix of the URL first. Only use the more expensive preg_match when it matches
if ('' === $staticPrefix || 0 === strpos($pathinfo, $staticPrefix)) {
// no-op
} elseif (!$supportsTrailingSlash || ($requiredMethods && !\in_array('GET', $requiredMethods)) || 'GET' !== $method) {
$this->addTrace(sprintf('Path "%s" does not match', $route->getPath()), self::ROUTE_DOES_NOT_MATCH, $name, $route);
continue;
} elseif ('/' === substr($staticPrefix, -1) && substr($staticPrefix, 0, -1) === $pathinfo) {
$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);

return $this->allow = [];
} else {
$this->addTrace(sprintf('Path "%s" does not match', $route->getPath()), self::ROUTE_DOES_NOT_MATCH, $name, $route);
continue;
}
$regex = $compiledRoute->getRegex();

if ($supportsTrailingSlash && $pos = strpos($regex, '/$')) {
$regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2);
$hasTrailingSlash = true;
} else {
$hasTrailingSlash = false;
}

if (!preg_match($compiledRoute->getRegex(), $pathinfo, $matches)) {
if (!preg_match($regex, $pathinfo, $matches)) {
// does it match without any requirements?
$r = new Route($route->getPath(), $route->getDefaults(), [], $route->getOptions());
$cr = $r->compile();
Expand All @@ -79,53 +110,49 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
continue;
}

// check host requirement
if ($hasTrailingSlash && '/' !== substr($pathinfo, -1)) {
if ((!$requiredMethods || \in_array('GET', $requiredMethods)) && 'GET' === $method) {
$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);

return $this->allow = [];
}
$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);
continue;
}

$hostMatches = [];
if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) {
$this->addTrace(sprintf('Host "%s" does not match the requirement ("%s")', $this->context->getHost(), $route->getHost()), self::ROUTE_ALMOST_MATCHES, $name, $route);

continue;
}

// check HTTP method requirement
if ($requiredMethods = $route->getMethods()) {
// HEAD and GET are equivalent as per RFC
if ('HEAD' === $method = $this->context->getMethod()) {
$method = 'GET';
}

if (!\in_array($method, $requiredMethods)) {
$this->allow = array_merge($this->allow, $requiredMethods);

$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);
$status = $this->handleRouteRequirements($pathinfo, $name, $route);

continue;
if (self::REQUIREMENT_MISMATCH === $status[0]) {
if ($route->getCondition()) {
$this->addTrace(sprintf('Condition "%s" does not evaluate to "true"', $route->getCondition()), self::ROUTE_ALMOST_MATCHES, $name, $route);
} else {
$this->addTrace(sprintf('Scheme "%s" does not match any of the required schemes (%s); the user will be redirected to first required scheme', $this->getContext()->getScheme(), implode(', ', $route->getSchemes())), self::ROUTE_ALMOST_MATCHES, $name, $route);
}
}

// check condition
if ($condition = $route->getCondition()) {
if (!$this->getExpressionLanguage()->evaluate($condition, ['context' => $this->context, 'request' => $this->request ?: $this->createRequest($pathinfo)])) {
$this->addTrace(sprintf('Condition "%s" does not evaluate to "true"', $condition), self::ROUTE_ALMOST_MATCHES, $name, $route);

continue;
}
continue;
}

// check HTTP scheme requirement
if ($requiredSchemes = $route->getSchemes()) {
$scheme = $this->context->getScheme();

if (!$route->hasScheme($scheme)) {
$this->addTrace(sprintf('Scheme "%s" does not match any of the required schemes (%s); the user will be redirected to first required scheme', $scheme, implode(', ', $requiredSchemes)), self::ROUTE_ALMOST_MATCHES, $name, $route);
// check HTTP method requirement
if ($requiredMethods) {
if (!\in_array($method, $requiredMethods)) {
if (self::REQUIREMENT_MATCH === $status[0]) {
$this->allow = array_merge($this->allow, $requiredMethods);
}
$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);

return true;
continue;
}
}

$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);

return true;
return $this->getAttributes($route, $name, array_replace($matches, $hostMatches, isset($status[1]) ? $status[1] : []));
}

return [];
Expand Down
8 changes: 6 additions & 2 deletions src/Symfony/Component/Routing/Tests/Matcher/TraceableUrlMatcherTest.php
View file
Expand Up @@ -11,14 +11,13 @@

namespace Symfony\Component\Routing\Tests\Matcher;

use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Matcher\TraceableUrlMatcher;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;

class TraceableUrlMatcherTest extends TestCase
class TraceableUrlMatcherTest extends UrlMatcherTest
{
public function test()
{
Expand Down Expand Up @@ -119,4 +118,9 @@ public function testRoutesWithConditions()
$traces = $matcher->getTracesForRequest($matchingRequest);
$this->assertEquals('Route matches!', $traces[0]['log']);
}

protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
{
return new TraceableUrlMatcher($routes, $context ?: new RequestContext());
}
}

0 comments on commit 9672f4c

Please sign in to comment.