FriendsOfSymfony / FOSRestBundle

This Bundle provides various tools to rapidly develop RESTful API's with Symfony
http://symfony.com/doc/master/bundles/FOSRestBundle/index.html
MIT License
2.79k stars 703 forks source link

Disable CSRF on a "context" basis #1121

Open jbenezech opened 9 years ago

jbenezech commented 9 years ago

We'd like to use the same controllers/forms for both our web client interface and our REST api that serves mobile apps. Application users may be using the mobile app or the web client and authenticate with the same credentials. I understand we can disable CSRF protection on a per-user basis, assigning a specific role, but this doesn't seem to suit our scenario.

Would it be possible to extend this functionality to a per-context basis, maybe using request headers ? Is there today a workaround for that ?

lsmith77 commented 9 years ago

maybe in 2.0 as part of https://github.com/FriendsOfSymfony/FOSRestBundle/pull/955

alfonmga commented 8 years ago

Hey @jbenezech that scenario looks insteresting, you solved it?

jbenezech commented 8 years ago

@alfonsomga , as a temporary solution, we've solved that by registering a form type extension which disables the csrf protection based on the request path (/api)

Ma27 commented 8 years ago

I'd add a new configuration parameter that disables the extension if the headers are not correct:

fos_rest:
    disable_csrf:
        role: ROLE_API (defaultNull) (moved with "headers" into one section because of readability)
        headers:
            Accept:
                - text/html (can be string or array if many types should be accepted)

if you'd be fine with that approach, I'd work on it when having time :)

/cc @lsmith77 @Ener-Getick

xabbuh commented 8 years ago

Is that a good idea? Wouldn't that make it possible to run CSRF attacks through, for example, AJAX requests?

Ma27 commented 8 years ago

the reason of that extension is to disable csrf protection on for the api, so if it is not a good idea, the whole approach including the solution of the issue reporter would be a bad idea or am I completly wrong?

xabbuh commented 8 years ago

Sure, but disabling it based on headers makes it possible to disable it everywhere from the outside (and not only for the API). This would work quite well for API-only applications but what if my application had an API and a regular user interface?

lsmith77 commented 8 years ago

It makes sense to disable based on role as long as the auth data is not available to anyone browsing the site.

GuilhemN commented 8 years ago

I think that we can create a voter based on a request matcher.

GuilhemN commented 8 years ago

I see something like that:

class ApiVoter extends Voter
{
    private $requestMatcher;
    private $requestStack;

    public function __construct(RequestMatcherInterface $requestMatcher, RequestStack $requestStack) {
        $this->requestMatcher = $requestMatcher;
        $this->requestStack = $requestStack;
    }

    protected function supports($attribute, $object) {
        return $object === null && $attribute === 'ON_API';
    }

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token) {
        return $this->requestMatcher->matches($this->requestStack->getCurrentRequest();
    }
}

We would need a new configuration section for the RequestMatcher and it would also be nice to create a ChainRequestMatcher to avoid having to inject multiple RequestMatcherInterface.