api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.39k stars 848 forks source link

Content Negotiation should be performed before Authentication / FirewallListener #3577

Closed kralos closed 3 years ago

kralos commented 4 years ago

API Platform version(s) affected: 2.5.5

Description
Currently, AddFormatListener (priority 7) is executed after FirewallListener (priority 8). See registered kernel.request event listeners. This means the Security system cannot rely on Symfony\HttpFoundation\Request::getRequestFormat() as it has not yet been initialized.

When setting a response for the kernel.request event, the propagation is stopped. This means listeners with lower priority won't be executed.

How to reproduce
What if we had a Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface which relied on the request format to decide if it should return a HTTP 302 (see https://tools.ietf.org/html/rfc6749#section-4.1) redirect a user to login. But also supports https://tools.ietf.org/html/rfc6750#section-3 - notify an API client they must authenticate with a Bearer token?

    public function start(
        HttpFoundation\Request $request,
        AuthenticationException $authException = null
    ): HttpFoundation\Response {
        if ($request->getRequestFormat() === 'html') {
            /**
             * @see https://tools.ietf.org/html/rfc6749#section-4.1.1
             */
            $state = sha1(openssl_random_pseudo_bytes(1000));
            $stateKey = AuthorizationCodeAuthenticator::getStateKey($state);
            $this->cache->save($stateKey, $state, 300);

            $uri = $this->router->generate('auth_client_authorization_v2_authorize', [
                'response_type' => 'code',
                'client_id' => $this->oAuthClientId,
                'state' => $state,
            ], UrlGeneratorInterface::ABSOLUTE_URL);

            return new HttpFoundation\RedirectResponse($uri);
        } else {
            /**
             * @see https://tools.ietf.org/html/rfc6750#section-3
             */
            $data = [
                'message' => 'Authentication Required',
            ];
            $wwwAuthenticateHeader = sprintf(
                'Bearer realm="%s"',
                $this->realm
            );
            if (
                $request->headers->has('Authorization')
                && 'Bearer' === substr($request->headers->get('Authorization'), 0, 6)
            ) {
                $wwwAuthenticateHeader .= ', error="invalid_token"';
            }
            return new HttpFoundation\JsonResponse($data, HttpFoundation\Response::HTTP_UNAUTHORIZED, [
                'WWW-Authenticate' => $wwwAuthenticateHeader,
            ]);
        }
    }

Possible Solution
Move AddFormatListener to a priority > 8

Additional Context

https://github.com/api-platform/core/commit/3b3f10df67fd3c96960cd4f57150f6240c0bff14#r39354527

but we must double check that it has no bad side effects in term of security before changing this.

The side effects of running ApiPlatform\Core\EventListener\AddFormatListener are:

When priority is < 8 the Request::$format is left null (unless manipulated outside of api-platform/core by another higher priority listener).

Since event dispatcher priorities are not strictly defined by Symfony, we should look to other projects for consensus on a good priority to use for AddFormatListener.

We should start a list of notable bundles that register kernel.request event listeners which might affect the chosen priority for AddFormatListener. Interoperability with other libraries is an important consideration.

Example order of event listeners in a standard symfony 5 project:

Listener Priority
Symfony\Component\HttpKernel\EventListener\DebugHandlersListener::configure() 2048
Symfony\Component\HttpKernel\EventListener\ValidateRequestListener::onKernelRequest() 256
Symfony\Component\HttpKernel\EventListener\SessionListener::onKernelRequest() 128
Symfony\Component\HttpKernel\EventListener\LocaleListener::setDefaultLocale() 100
Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest() 32
Symfony\Component\HttpKernel\EventListener\LocaleListener::onKernelRequest() 16
Symfony\Component\HttpKernel\EventListener\LocaleAwareListener::onKernelRequest() 15
Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener::configureLogoutUrlGenerator() 8
Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener::onKernelRequest() 8
Sentry\SentryBundle\EventListener\RequestListener::onKernelRequest() 1
Sentry\SentryBundle\EventListener\SubRequestListener::onKernelRequest() 1
kralos commented 4 years ago

FosRestBundle uses priority 34 - https://symfony.com/doc/master/bundles/FOSRestBundle/3-listener-support.html#priorities

soyuka commented 4 years ago

I definitely agree on the fix here. Would you be able to open a PR?

kralos commented 4 years ago

Sorry, finally got around to this. I had a play with priorities but since AddFormatListener is only supposed to act if within an api-platform route (check for things like $request->attributes->get('_api_resource_class')) we need to be after Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest() (32)

I settled on 28 leaving a little room between router and controller name resolution:

$ APP_ENV=test tests/Fixtures/app/console -v debug:event-dispatcher kernel.request

Registered Listeners for "kernel.request" Event
===============================================

 ------- ------------------------------------------------------------------------------------------------------- ---------- 
  Order   Callable                                                                                                Priority  
 ------- ------------------------------------------------------------------------------------------------------- ---------- 
  #1      Symfony\Component\HttpKernel\EventListener\DebugHandlersListener::configure()                           2048      
  #2      Symfony\Component\HttpKernel\EventListener\ValidateRequestListener::onKernelRequest()                   256       
  #3      Symfony\Component\HttpKernel\EventListener\TestSessionListener::onKernelRequest()                       192       
  #4      Symfony\Component\HttpKernel\EventListener\SessionListener::onKernelRequest()                           128       
  #5      Symfony\Component\HttpKernel\EventListener\LocaleListener::setDefaultLocale()                           100       
  #6      Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest()                            32        
  #7      ApiPlatform\Core\EventListener\AddFormatListener::onKernelRequest()                                     28        
  #8      Symfony\Bundle\FrameworkBundle\EventListener\ResolveControllerNameSubscriber::resolveControllerName()   24        
  #9      ApiPlatform\Core\Filter\QueryParameterValidateListener::onKernelRequest()                               16        
  #10     Symfony\Component\HttpKernel\EventListener\LocaleListener::onKernelRequest()                            16        
  #11     Symfony\Component\HttpKernel\EventListener\LocaleAwareListener::onKernelRequest()                       15        
  #12     Symfony\Bundle\SecurityBundle\EventListener\FirewallListener::configureLogoutUrlGenerator()             8         
  #13     Symfony\Bundle\SecurityBundle\EventListener\FirewallListener::onKernelRequest()                         8         
  #14     ApiPlatform\Core\JsonApi\EventListener\TransformPaginationParametersListener::onKernelRequest()         5         
  #15     ApiPlatform\Core\JsonApi\EventListener\TransformSortingParametersListener::onKernelRequest()            5         
  #16     ApiPlatform\Core\JsonApi\EventListener\TransformFieldsetsParametersListener::onKernelRequest()          5         
  #17     ApiPlatform\Core\JsonApi\EventListener\TransformFilteringParametersListener::onKernelRequest()          5         
  #18     ApiPlatform\Core\EventListener\ReadListener::onKernelRequest()                                          4         
  #19     ApiPlatform\Core\Security\EventListener\DenyAccessListener::onSecurity()                                3         
  #20     ApiPlatform\Core\EventListener\DeserializeListener::onKernelRequest()                                   2         
  #21     ApiPlatform\Core\Security\EventListener\DenyAccessListener::onSecurityPostDenormalize()                 1         
  #22     ApiPlatform\Core\Bridge\Symfony\Bundle\EventListener\SwaggerUiListener::onKernelRequest()               0         
 ------- ------------------------------------------------------------------------------------------------------- ----------