8000 [Routing] Throw 405 instead of 404 when redirect is not possible by nicolas-grekas · Pull Request #26100 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 12, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 31 additions & 24 deletions src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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';
Copy link
Contributor
@Tobion Tobion Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know that GET is actually allowed? If we do a POST /foo on a PUT /foo/ route, it will say method not allowed but GET is allowed? We don't know if GET is allowed at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is: the redirect handles the GET

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you need to check if get is in the allowed methods of the route first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if not we can mark the request with 307 response in master and here we just continue matching

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET is allowed: it triggers a redirect. There is nothing else to consider on this very path without slash. The verbs allowed on the route apply only when there is a slash.

Copy link
Member Author
@nicolas-grekas nicolas-grekas Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retry the same request with GET and he could still receive a 404

Why? to my understanding, it would then get a 301/302, because of this very "if" which will be hit again.

Copy link
Member Author
@nicolas-grekas nicolas-grekas Feb 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tobion note that if you're fine with the 404, but not with the 405, I'm OK to remove this line. It's only a minor thing on the HTTP side, and I doubt it will make any real difference in practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @Tobion let's agree on this and move on :)

Copy link
Contributor
@Tobion Tobion Feb 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you replace HEAD with GET in $supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('HEAD', $methods)); this works for PhpMatcherDumper. But the logic in RedirectableUrlMatcher is broken. See below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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