Skip to content

Commit

Permalink
bug #26100 [Routing] Throw 405 instead of 404 when redirect is not po…
Browse files Browse the repository at this point in the history 
…ssible (nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[Routing] Throw 405 instead of 404 when redirect is not possible

| 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        | -

Finishes #25962.

Commits
-------

9284281 [Routing] Throw 405 instead of 404 when redirect is not possible
  • Loading branch information
@fabpot
fabpot committed Feb 12, 2018
2 parents 7218310 + 9284281 commit aa3a04a
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 24 deletions.
55 changes: 31 additions & 24 deletions src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Expand Up @@ -217,7 +217,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$methods[] = 'HEAD';
}

$supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('HEAD', $methods));
$supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('GET', $methods));

if (!count($compiledRoute->getPathVariables()) && false !== preg_match('#^(.)\^(?P<url>.*?)\$\1#', $compiledRoute->getRegex(), $m)) {
if ($supportsTrailingSlash && '/' === substr($m['url'], -1)) {
Expand Down Expand Up @@ -258,34 +258,13 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
EOF;

$gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name);
if ($methods) {
if (1 === count($methods)) {
$code .= <<<EOF
if (\$this->context->getMethod() != '$methods[0]') {
\$allow[] = '$methods[0]';
goto $gotoname;
}
EOF;
} else {
$methods = implode("', '", $methods);
$code .= <<<EOF
if (!in_array(\$this->context->getMethod(), array('$methods'))) {
\$allow = array_merge(\$allow, array('$methods'));
goto $gotoname;
}
EOF;
}
}

if ($hasTrailingSlash) {
$code .= <<<EOF
if ('/' === substr(\$pathinfo, -1)) {
// no-op
} elseif (!in_array(\$this->context->getMethod(), array('HEAD', 'GET'))) {
\$allow[] = 'GET';
goto $gotoname;
} else {
return \$this->redirect(\$rawPathinfo.'/', '$name');
Expand All @@ -303,13 +282,41 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$code .= <<<EOF
\$requiredSchemes = $schemes;
if (!isset(\$requiredSchemes[\$this->context->getScheme()])) {
if (!in_array(\$this->context->getMethod(), array('HEAD', 'GET'))) {
\$allow[] = 'GET';
goto $gotoname;
}
return \$this->redirect(\$rawPathinfo, '$name', key(\$requiredSchemes));
}
EOF;
}

if ($methods) {
if (1 === count($methods)) {
$code .= <<<EOF
if (\$this->context->getMethod() != '$methods[0]') {
\$allow[] = '$methods[0]';
goto $gotoname;
}
EOF;
} else {
$methods = implode("', '", $methods);
$code .= <<<EOF
if (!in_array(\$this->context->getMethod(), array('$methods'))) {
\$allow = array_merge(\$allow, array('$methods'));
goto $gotoname;
}
EOF;
}
}

// optimize parameters array
if ($matches || $hostMatches) {
$vars = array();
Expand All @@ -333,7 +340,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
}
$code .= " }\n";

if ($methods || $hasTrailingSlash) {
if ($hasTrailingSlash || $schemes || $methods) {
$code .= " $gotoname:\n";
}

Expand Down
15 changes: 15 additions & 0 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
View file
Expand Up @@ -69,6 +69,7 @@ public function match($rawPathinfo)
if ('/' === substr($pathinfo, -1)) {
// no-op
} elseif (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
$allow[] = 'GET';
goto not_baz3;
} else {
return $this->redirect($rawPathinfo.'/', 'baz3');
Expand All @@ -85,6 +86,7 @@ public function match($rawPathinfo)
if ('/' === substr($pathinfo, -1)) {
// no-op
} elseif (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
$allow[] = 'GET';
goto not_baz4;
} else {
return $this->redirect($rawPathinfo.'/', 'baz4');
Expand Down Expand Up @@ -183,6 +185,7 @@ public function match($rawPathinfo)
if ('/' === substr($pathinfo, -1)) {
// no-op
} elseif (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
$allow[] = 'GET';
goto not_hey;
} else {
return $this->redirect($rawPathinfo.'/', 'hey');
Expand Down Expand Up @@ -333,21 +336,33 @@ public function match($rawPathinfo)
if ('/secure' === $pathinfo) {
$requiredSchemes = array ( 'https' => 0,);
if (!isset($requiredSchemes[$this->context->getScheme()])) {
if (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
$allow[] = 'GET';
goto not_secure;
}

return $this->redirect($rawPathinfo, 'secure', key($requiredSchemes));
}

return array('_route' => 'secure');
}
not_secure:

// nonsecure
if ('/nonsecure' === $pathinfo) {
$requiredSchemes = array ( 'http' => 0,);
if (!isset($requiredSchemes[$this->context->getScheme()])) {
if (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
$allow[] = 'GET';
goto not_nonsecure;
}

return $this->redirect($rawPathinfo, 'nonsecure', key($requiredSchemes));
}

return array('_route' => 'nonsecure');
}
not_nonsecure:

throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/Routing/Tests/Matcher/DumpedRedirectableUrlMatcherTest.php
View file
Expand Up @@ -19,6 +19,14 @@

class DumpedRedirectableUrlMatcherTest extends RedirectableUrlMatcherTest
{
/**
* @expectedException \Symfony\Component\Routing\Exception\MethodNotAllowedException
*/
public function testRedirectWhenNoSlashForNonSafeMethod()
{
parent::testRedirectWhenNoSlashForNonSafeMethod();
}

protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
{
static $i = 0;
Expand Down

0 comments on commit aa3a04a

Please sign in to comment.