Skip to content

MethodNotAllowedException is not caught #358

Closed
marcospassos opened this Issue Jan 13, 2013 · 32 comments

10 participants

@marcospassos

Hi all,

I've the following configuration:

product_option_create:
    pattern: /create.{_format}
    defaults:
        _format: ~
        _controller: colibri_product.controller.option:createAction
    requirements:
        _method: POST
twig:
    exception_controller: 'FOS\RestBundle\Controller\ExceptionController::showAction'

Accessing option/create.json, results in:
Untitled

There is some way to handle this exception properly?

I was expecting something like:

{
    "status":"error",
    "status_code":405,
    "status_text":"Method Not Allowed",
    "current_content":"",
    "message":"Method Not Allowed (Allow: POST)"
}

Thanks in advance.

@baldurrensch

That makes perfect sense. You are GETting a URL in the browser, but the requirement is to have it be a POST. I think the requirement should be GET in your configuration, that's what would make sense for a "show" action.

@marcospassos

Yes, it was just for testing. I would like to handle correctly the MethodNotAllowedHttpException. I've just updated my example.

@baldurrensch

As far as I know, the bundle does not provide that. It should be fairly easy to write an event listener though that formats error messages in such a way. I would at that point consider implementing that with https://github.com/blongden/vnd.error

@marcospassos

@lsmith77 Enable the ExceptionController is the same of put the following code?

twig:
    exception_controller: 'FOS\RestBundle\Controller\ExceptionController::showAction'

If so, I've enabled.

@lsmith77
FriendsOfSymfony member

yes .. it should be enabled this way. in this case i can only imagine that the failure happens so early in the request that the exception controller isnt called for some reason ..

@marcospassos

Yes, but do you agree this is a fundamental error in a REST application? Even whether it requires a event listener, I think this should be handled for a good understanding of final user.

I can work on this whether you agree.

@lsmith77
FriendsOfSymfony member

yeah .. i would hope that we can ensure that everything goes through the exception controller. if you could figure out why this isnt happening that would be awesome.

@marcospassos

Wow, it's more complicated than I've imagined. It seems like a Symfony limitation or bug (in our case a serious security case).

Explanation:

The Symfony router cache system generates a cached UrlMatcher ( appdevUrlMatcher ) that has a method match($pathinfo) which verifies which cached URL matches with the current URL (including the _method) and returns the preg_match result to be merged with the context parameters. The preg_match result has all variables declared in the routes configurations populated with the URL data. The problem is when the parameter _method of route does not match with the request type, even then nothing is returned because a MethodNotAllowedHttpException is thrown.

#app/cache/dev/appdevUrlMatcher.php
public function match($pathinfo)
{
    if (0 === strpos($pathinfo, '/product/option/show') && preg_match('#^/product/option/show(?:\\.(?P<_format>json))?$#s', $pathinfo, $matches)) {
        if ($this->context->getMethod() != 'POST') {
            // print_r($matches);
            // Array ( [0] => /product/option/show.json [_format] => json [1] => json )
            $allow[] = 'POST';
            goto not_product_option_show;
        }

        return $this->mergeDefaults(array_replace($matches, array('_route' => 'product_option_show')), array (  '_format' => NULL,  '_controller' => 'product.controller.option:showAction',));
    }
    not_product_option_show:

    throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); //
}

Thus, the method getRequestFormat returns html once the Request->format property is null.

#Symfony\Component\HttpFoundation\Request
public function getRequestFormat($default = 'html')
{
    #echo $this->format; // returns 'html'
    if (null === $this->format) {
        $this->format = $this->get('_format', $default);
    }

    return $this->format;
}

As the format is html, the method ExceptionController->showAction understands that is a case of templating format and renders a html page error.

public function showAction(Request $request, FlattenException $exception, DebugLoggerInterface $logger = null, $format = 'html')
{
   ...
    if (!$viewHandler->isFormatTemplating($format)) {
        $parameters = $this->createExceptionWrapper($parameters);
    }

    $view = View::create($parameters, $code, $exception->getHeaders());
    $view->setFormat($format);

    if ($viewHandler->isFormatTemplating($format)) {
        $view->setTemplate($this->findTemplate($format, $code));
    }

    $response = $viewHandler->handle($view);
    ...
}

I'll report this case on Symfony's page because it may cause some vulnerabilities issues as happened to us.

@lsmith77
FriendsOfSymfony member

thx for debugging this .. indeed tricky. it makes sense to have a default format i guess but we would need to have a quick way to find out if its explicitly html or if its just the default so that we can then decide if we should trigger some logic to try and determine a more appropriate format.

@marcospassos

I can't figure out how we can do that without duplicate a lot of code. Maybe a possible solution is to pass the $matches embedded in the MethodNotAllowedException, but it requires their approval.

@lsmith77
FriendsOfSymfony member

well what i was talking about would require a change in the core Request class

@marcospassos

@lsmith77 My team is facing other problem here. As I already have said, just one part of our app is a REST API. Errors like NotFoundHttpException are not caught properly because the routing context does not even have a format yet. But, would be nice if just in /api/ the errors was rendered properly (catching all Exceptions). Maybe a configuration based on a route could be a good solution, requiring just a small modification in the ExceptionController:

exception:
    coverage: [{path: /api, templating: false}] # templating: true, false or auto

What do you think?

@lsmith77
FriendsOfSymfony member

i think for your case it would be better to simply extend the ExceptionController and add such a check. if it becomes clear that this is an unresolveable issues on the core then we might indeed have to add something like this. but i dont want to start adding band aids over core issues by replicating the routing partially.

@marcospassos

No problems, I just didn't see any alternative to do this trick without create other Kernel. As we will develop some solution, I've thought in discuss about, in order to do something reusable that we could share with the other users.

@lsmith77
FriendsOfSymfony member

well we do need to keep an eye on this topic and if necessary we would indeed need to add some logic to handle this .. but i would like to refrain from doing it just now. that being said .. if you want to do this inside a PR to collaborate with others that is all good too ..

@marcospassos

Ok :)

@Tobion
FriendsOfSymfony member
Tobion commented Oct 7, 2014

This problem is resolved. It now returns the the correct error response format for 405.
The only time when it appears that the 405 is not caught, is when an exception is thrown inside the exception controller. Then the symfony http kernel will ignore this exception in the from the exceptioncontroller and instead render the origin exception (which is for example the HttpMethodNotAllowException).
But this situation should not occur anymore and if so, is a caused by developer error.

@Tobion Tobion closed this Oct 7, 2014
@kminh
kminh commented Jan 16, 2015

I'm having the exact same issue using Symfony 2.6.1 and FOSRestBundle 1.4.2.

My configuration is as followed:

fos_rest:
    access_denied_listener: true
    view:
        view_response_listener: 'force'
        formats:
            json: true
        mime_types:
            json: ['application/json; charset=UTF-8', 'application/json']
    routing_loader:
        default_format: json
        include_format: false

I doubt that I'm doing something wrong, because all I have done is setting up some empty controllers to test, using this documentation: https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Resources/doc/5-automatic-route-generation_single-restful-controller.rst

The response for a 405 response code is always a standard Symfony exception page.

I have also taken a look at https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Resources/doc/4-exception-controller-support.rst, but the documentation seems outdated as there are neither enabled nor exception configuration key under fos_rest.exception.

Thanks for any insight.

@lsmith77
FriendsOfSymfony member

I have added a feature that enables you to set a different fallback format for exceptions. this is useful especially when one does not want to set a fallback format to ensure that unsupported formats get a 406 returned. previously though this often meant that the response would then be html. with this new setting its possible to set a specific format for exceptions in this case.

@renatomefidf

I have the same issue using version 1.4.2 of fosrestbundle and symfony 2.6.3

@lsmith77 can you show how to implement your solution?

@lsmith77
FriendsOfSymfony member

see #940 .. its part of 1.5.0-RC4

@gmansilla

I'm having the same issue.

After reading this response I went to Exception to controller to check by myself the error code being returned. The error code is a 404, which is wrong (maybe because of this?).

@Ener-Getick Ener-Getick reopened this Jan 20, 2016
@Ener-Getick
FriendsOfSymfony member

@gmansilla can you provide more information please?

Like the content returned, your configuration and everything which may be useful.

@gmansilla

Aight, here are the relevant parts

//routing.yml
product:
pattern: /product/{product_id}/{_format}
defaults: { _controller: AppBundle:Product:get, _format: json }
requirements:
    _method: GET



//config.yml
# FOSRestBundle
fos_rest:
    allowed_methods_listener: true
    routing_loader:
    default_format: 'json'
format_listener: true
body_listener:
    decoders:
        json: fos_rest.decoder.json
        xml: fos_rest.decoder.xml
view:
    formats:
        json: true
        xml: true
        rss: false
        html: false
    view_response_listener: 'force'
exception:
    enabled: true
    exception_controller: 'FOS\RestBundle\Controller\ExceptionController::showAction'
    codes:
        'Symfony\Component\HttpKernel\Exception\NotFoundHttpException': 404
        'Symfony\Component\Routing\Exception\ResourceNotFoundException': 404
        'Doctrine\ORM\OptimisticLockException': HTTP_CONFLICT
    messages:
        'Symfony\Component\Routing\Exception\ResourceNotFoundException': false    


//Controller action
/**
 * @Rest\View()
 */
public function getAction($productId) {
    $product = $something->getProduct($productId);

    if (!$product instanceof Product) {
        throw new NotFoundHttpException("Product not found");
    }

    return $product;
}

Using postman to simulate a POST to the above action returns

An Exception was thrown while handling: No route found for "POST /product/"

screen shot 2016-01-20 at 5 49 58 pm

I aso put a var_dump($code) after this line and the code at that point is 404

@Ener-Getick
FriendsOfSymfony member

We really need to improve this error message and maybe log it...
Can you dump the exception thrown here please?

@gmansilla

Here

Do you want me to paste the whole thing here? (1254 lines though)

@Ener-Getick
FriendsOfSymfony member

Did you dump $e or $exception ? It doesn't seem to be the right exception :x

@gmansilla

Oh, ok. You're right. I var_dumped $exception instead of $e.

The thing is, when I var_dump($e) I get a timeout. However, if I check the message (var_dump($e->getMessage()) I get this message

string(76) "The template "TwigBundle:Exception:exception_full.html.twig" does not exist."

Which makes sense because I am not using Twig. Do you want me to debug something else on $e?

Another thing I'd like to mention is that my composer entry for this bundle is:

"friendsofsymfony/rest-bundle": "~1.0",

and after taking a look at composer.lock I see this entry

"version": "1.7.4",

meaning I am var_dumping the exception thrown in this line

@xabbuh
FriendsOfSymfony member
xabbuh commented Jan 31, 2016

I don't understand why that doesn't lead to an internal server error then as such a response is created and returned in that code path. But to handle this gracefully, you will need to replace the exception controller with a custom implementation if the TwigBundle is disabled (luckily that will be a bit easier in 2.0 thanks to #1288).

@h00dstoker

obs: The name of route(s) should to be unique.

@gmansilla

If anyone has the same issue in the future, this is what I did:

I created a custom Exception controller that extends this bundle's controller and then override the showAction method.

namespace AppBundle\Controller;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Log\DebugLoggerInterface;

class ExceptionController extends \FOS\RestBundle\Controller\ExceptionController 
{

    public function showAction(Request $request, $exception, DebugLoggerInterface $logger = null)
    {

        if ($exception->getClass() == 'Symfony\Component\HttpKernel\Exception\NotFoundHttpException') {
            return new Response("Not found", 404);
        }
        return parent::showAction($request, $exception, $logger);

    }
}

Don't forget to change the configuration setttings (config.yml)

exception:
    enabled: true
    exception_controller: 'AppBundle\Controller\ExceptionController::showAction'
@lsmith77 lsmith77 changed the title from MethodNotAllowedException is not caught to MethodNotAllowedException is not caught Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.