feat: harden the myep oauth2 client

.env

@@ -49,6 +49,11 @@ MAILING_MAILER_DSN=null://null
 
 APP_BASE_URI=https://myep-team.ddev.site
 
+# Regex matched against the Host header; requests for any other host are refused. The
+# OAuth2 redirect_uri is generated from the request, so this is what pins it. Production
+# sets its own value outside the repository, like APP_BASE_URI above.
+APP_TRUSTED_HOSTS=^myep-team\.ddev\.site$
+
 APP_BPN_USER=
 APP_BPN_PASSWORD=
 APP_BPN_IP=

.env.test

@@ -4,3 +4,5 @@ APP_SECRET='$ecretf0rt3st'
 SYMFONY_DEPRECATIONS_HELPER=999999
 PANTHER_APP_ENV=panther
 PANTHER_ERROR_SCREENSHOT_DIR=./var/error-screenshots
+# the test client requests http://localhost/
+APP_TRUSTED_HOSTS='^localhost$'

config/packages/framework.yaml

@@ -5,6 +5,11 @@ framework:
     http_method_override: false
     handle_all_throwables: true
 
+    # The OAuth2 redirect_uri handed to MyE&P is generated from the incoming request, so an
+    # unvalidated Host header would let a crafted request point the authorization code
+    # somewhere else. Regex patterns, matched against the host without the scheme or port.
+    trusted_hosts: ['%env(APP_TRUSTED_HOSTS)%']
+
     # Enables session support. Note that the session will ONLY be started if you read or write from it.
     # Remove or comment this section to explicitly disable session support.
     session:

src/Controller/Security/OAuth2Controller.php

@@ -22,10 +22,9 @@ class OAuth2Controller extends AbstractController
     {
         $this->denyUnlessFeatureIsActive();
 
-        $provider = $this->client->getProvider();
-        $url = $provider->getAuthorizationUrl();
-        $state = $provider->getState();
-        $request->getSession()->set('oauth2state', $state);
+        // the state and the PKCE verifier belong to the client, which is what consumes
+        // them again on the callback
+        $url = $this->client->createAuthorizationUrl($request);
 
         return $this->redirect($url);
     }

src/Security/MyEpAuthenticator.php

@@ -6,6 +6,7 @@ use App\BusProNet\UserDataHandler;
 use App\Entity\Teamer;
 use App\Entity\User;
 use App\RequiredTeamerCheck\RequiredTeamerCheckRegistry;
+use App\Security\OAuth2\AuthorizationDeniedException;
 use App\Security\OAuth2\AuthorizationRequestException;
 use App\Security\OAuth2\MyEpClient;
 use Doctrine\ORM\EntityManagerInterface;
@@ -34,6 +35,12 @@ class MyEpAuthenticator extends AbstractAuthenticator
      * The roles that entitle someone to log in here at all. Anything else MyE&P reports
      * is dropped rather than stored, so that no role this application assigns a meaning
      * to can be set from the outside.
+     *
+     * This list exists a second time on MyE&P, as the required_roles of this application's
+     * oauth2_client row: MyE&P refuses the authorization request outright when the account
+     * holds none of them, and explains why on its own page. The two are one policy written
+     * twice and must be changed together — widening only one either strands a user at MyE&P
+     * with no explanation this side can give, or lets one through to be refused here.
      */
     private const ELIGIBLE_ROLES = ['ROLE_TEAM_ADMIN', 'ROLE_TEAMER', 'ROLE_MANAGER', 'ROLE_HOUSE_MANAGER'];
 
@@ -63,11 +70,18 @@ class MyEpAuthenticator extends AbstractAuthenticator
     {
         try {
             $accessToken = $this->client->fetchAccessToken($request);
+        } catch (AuthorizationDeniedException $e) {
+            // MyE&P said why it sent no code, so this is not a failure to report as one
+            $this->logger->info('Login via MyE&P was denied', [
+                'error' => $e->getError(),
+                'error_description' => $e->getErrorDescription(),
+            ]);
+            throw new CustomUserMessageAuthenticationException('Login via MyE&P wurde abgebrochen');
         } catch (AuthorizationRequestException|IdentityProviderException $e) {
             $this->logger->error('Login via MyE&P failed due to unobtainable access token', [
                 'exception' => $e,
             ]);
-            throw new CustomUserMessageAuthenticationException('Invalid token');
+            throw new CustomUserMessageAuthenticationException('Login via MyE&P nicht möglich');
         }
 
         try {

src/Security/OAuth2/AuthorizationDeniedException.php

@@ -0,0 +1,34 @@
+<?php
+
+namespace App\Security\OAuth2;
+
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * MyE&P redirected back with an `error` instead of a code — the person cancelled, or the
+ * authorization was refused.
+ *
+ * A subclass of AuthorizationRequestException so that a caller which does not care why the
+ * callback carried no code still catches it, and one that does can tell this apart from a
+ * malformed or replayed callback and say so.
+ */
+class AuthorizationDeniedException extends AuthorizationRequestException
+{
+    public function __construct(
+        private readonly string $error,
+        private readonly ?string $errorDescription,
+        Request $request,
+    ) {
+        parent::__construct(sprintf('Authorization denied: %s', $error), 400, $request);
+    }
+
+    public function getError(): string
+    {
+        return $this->error;
+    }
+
+    public function getErrorDescription(): ?string
+    {
+        return $this->errorDescription;
+    }
+}

src/Security/OAuth2/MyEpClient.php

@@ -13,41 +13,104 @@ use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 
 class MyEpClient
 {
+    private const SESSION_KEY_STATE = 'oauth2state';
+    private const SESSION_KEY_PKCE = 'oauth2pkce';
+
     private array $config;
 
     public function __construct(
         private readonly UrlGeneratorInterface $urlGenerator,
         private readonly LoggerInterface $logger,
+        // the client secret rides in here, marked sensitive so it is redacted from stack
+        // traces rather than printed in full on an exception page or in a log
+        #[\SensitiveParameter]
         array $options,
     ) {
         $this->config = $this->resolveConfig($options);
     }
 
+    /**
+     * Starts the authorization code flow and stores what the callback has to prove.
+     *
+     * The state and the PKCE verifier are generated by getAuthorizationUrl() and are only
+     * readable afterwards, so both are taken from the provider rather than built here.
+     */
+    public function createAuthorizationUrl(Request $request): string
+    {
+        $provider = $this->getProvider();
+        $url = $provider->getAuthorizationUrl();
+
+        $session = $request->getSession();
+        $session->set(self::SESSION_KEY_STATE, $provider->getState());
+        $session->set(self::SESSION_KEY_PKCE, $provider->getPkceCode());
+
+        return $url;
+    }
+
     /**
      * @throws IdentityProviderException
+     * @throws AuthorizationDeniedException
      * @throws AuthorizationRequestException
      */
     public function fetchAccessToken(Request $request): AccessTokenInterface
     {
+        $session = $request->getSession();
+
+        // Both are single-use, so they are consumed before anything can fail: a callback
+        // that throws must not leave a state behind that a second attempt could replay.
+        $expectedState = $session->remove(self::SESSION_KEY_STATE);
+        $pkceCode = $session->remove(self::SESSION_KEY_PKCE);
+
+        // Before anything else: MyE&P has said why it is not sending a code, and every
+        // later check would report the wrong cause. The role gate renders on MyE&P rather
+        // than redirecting, but a cancelled or refused authorization arrives here.
+        $error = $request->query->get('error');
+
+        if (true === is_string($error) && '' !== $error) {
+            $description = $request->query->get('error_description');
+            $description = is_string($description) && '' !== $description ? $description : null;
+
+            $this->logger->error('OAuth2 login request denied', [
+                'error' => $error,
+                'error_description' => $description,
+            ]);
+
+            throw new AuthorizationDeniedException($error, $description, $request);
+        }
+
         if (null === $code = $request->query->get('code')) {
             $this->logger->error('OAuth2 login request missing code');
             throw new AuthorizationRequestException('Missing code', 400, $request);
         }
 
-        $session = $request->getSession();
+        $state = $request->query->get('state');
 
+        // hash_equals() rather than !==, the state being the CSRF secret of the flow. The
+        // string checks come first: a callback with no state, or a session that never held
+        // one, must fail here rather than reach a comparison with null.
         if (
-            null === $request->query->get('state')
-            || $request->query->get('state') !== $session->get('oauth2state')
+            false === is_string($state)
+            || false === is_string($expectedState)
+            || false === hash_equals($expectedState, $state)
         ) {
-            $session->remove('oauth2state');
             $this->logger->error('OAuth2 login request missing state or mismatch');
             throw new AuthorizationRequestException('Missing state or mismatch', 400, $request);
         }
 
-        $session->remove('oauth2state');
+        // A challenge went out with the authorization request, so MyE&P rejects an exchange
+        // without the verifier. Refused here instead, where the reason is still known — the
+        // usual cause is a session replaced between the two legs of the flow.
+        if (false === is_string($pkceCode) || '' === $pkceCode) {
+            $this->logger->error('OAuth2 login request without the PKCE verifier of its session');
+            throw new AuthorizationRequestException('Missing PKCE verifier', 400, $request);
+        }
 
-        return $this->getProvider()->getAccessToken('authorization_code', [
+        $provider = $this->getProvider();
+
+        // replays the verifier whose challenge was sent with the authorization request
+        $provider->setPkceCode($pkceCode);
+
+        return $provider->getAccessToken('authorization_code', [
             'code' => $code,
         ]);
     }
@@ -68,6 +131,10 @@ class MyEpClient
             'urlResourceOwnerDetails' => $this->config['myep_oauth2_url_resource_owner_details'],
             'scopes' => $this->config['myep_oauth2_scopes'],
             'scopeSeparator' => ' ',
+            // The redirect URI above is generated from the incoming request, so it is only
+            // as trustworthy as the Host header. PKCE binds the authorization code to this
+            // flow by a second, header-independent means.
+            'pkceMethod' => AbstractProvider::PKCE_METHOD_S256,
         ]);
     }
 

tests/Security/OAuth2/MyEpClientTest.php

@@ -0,0 +1,147 @@
+<?php
+
+namespace App\Tests\Security\OAuth2;
+
+use App\Security\OAuth2\AuthorizationDeniedException;
+use App\Security\OAuth2\AuthorizationRequestException;
+use App\Security\OAuth2\MyEpClient;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Session\Session;
+use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+
+class MyEpClientTest extends TestCase
+{
+    public function testAuthorizationUrlCarriesAnS256ChallengeAndStoresItsVerifier(): void
+    {
+        $request = $this->createRequest();
+
+        $url = $this->createClient()->createAuthorizationUrl($request);
+
+        parse_str((string) parse_url($url, \PHP_URL_QUERY), $query);
+
+        $this->assertSame('S256', $query['code_challenge_method']);
+
+        $session = $request->getSession();
+        $verifier = $session->get('oauth2pkce');
+
+        $this->assertIsString($verifier);
+        $this->assertSame($session->get('oauth2state'), $query['state']);
+
+        // the challenge is what MyE&P stores; the verifier replayed on the token request
+        // has to hash to it, or the exchange is refused
+        $expectedChallenge = trim(strtr(base64_encode(hash('sha256', $verifier, true)), '+/', '-_'), '=');
+        $this->assertSame($expectedChallenge, $query['code_challenge']);
+    }
+
+    public function testDeniedCallbackIsReportedAsADenialAndConsumesTheStoredState(): void
+    {
+        $request = $this->createRequest([
+            'state' => 'expected-state',
+            'error' => 'access_denied',
+            'error_description' => 'The user denied the request',
+        ]);
+        $request->getSession()->set('oauth2state', 'expected-state');
+        $request->getSession()->set('oauth2pkce', 'verifier');
+
+        try {
+            $this->createClient()->fetchAccessToken($request);
+            $this->fail('Expected an AuthorizationDeniedException');
+        } catch (AuthorizationDeniedException $e) {
+            // MyE&P has named the cause; reporting this as a missing code would not
+            $this->assertSame('access_denied', $e->getError());
+            $this->assertSame('The user denied the request', $e->getErrorDescription());
+        }
+
+        $this->assertFalse($request->getSession()->has('oauth2state'));
+        $this->assertFalse($request->getSession()->has('oauth2pkce'));
+    }
+
+    public function testCallbackIsRefusedWhenTheSessionHoldsNoVerifier(): void
+    {
+        $request = $this->createRequest(['code' => 'a-code', 'state' => 'expected-state']);
+        $request->getSession()->set('oauth2state', 'expected-state');
+
+        $this->expectException(AuthorizationRequestException::class);
+        $this->expectExceptionMessage('Missing PKCE verifier');
+
+        $this->createClient()->fetchAccessToken($request);
+    }
+
+    public function testCallbackWithoutCodeConsumesTheStoredState(): void
+    {
+        $request = $this->createRequest(['state' => 'expected-state']);
+        $request->getSession()->set('oauth2state', 'expected-state');
+        $request->getSession()->set('oauth2pkce', 'verifier');
+
+        try {
+            $this->createClient()->fetchAccessToken($request);
+            $this->fail('Expected an AuthorizationRequestException');
+        } catch (AuthorizationRequestException $e) {
+            $this->assertSame('Missing code', $e->getMessage());
+        }
+
+        // the state is single-use: a callback that fails must not leave one behind that a
+        // later attempt could still replay
+        $this->assertFalse($request->getSession()->has('oauth2state'));
+        $this->assertFalse($request->getSession()->has('oauth2pkce'));
+    }
+
+    public function testCallbackWithAMismatchedStateConsumesTheStoredState(): void
+    {
+        $request = $this->createRequest(['code' => 'a-code', 'state' => 'other-state']);
+        $request->getSession()->set('oauth2state', 'expected-state');
+        $request->getSession()->set('oauth2pkce', 'verifier');
+
+        $this->expectException(AuthorizationRequestException::class);
+        $this->expectExceptionMessage('Missing state or mismatch');
+
+        try {
+            $this->createClient()->fetchAccessToken($request);
+        } finally {
+            $this->assertFalse($request->getSession()->has('oauth2state'));
+            $this->assertFalse($request->getSession()->has('oauth2pkce'));
+        }
+    }
+
+    public function testCallbackIsRefusedWhenTheSessionHoldsNoState(): void
+    {
+        $request = $this->createRequest(['code' => 'a-code', 'state' => 'any-state']);
+
+        $this->expectException(AuthorizationRequestException::class);
+        $this->expectExceptionMessage('Missing state or mismatch');
+
+        $this->createClient()->fetchAccessToken($request);
+    }
+
+    private function createClient(): MyEpClient
+    {
+        $urlGenerator = $this->createMock(UrlGeneratorInterface::class);
+        $urlGenerator
+            ->method('generate')
+            ->willReturn('https://myep-team.example.org/myep-auth/check')
+        ;
+
+        return new MyEpClient($urlGenerator, $this->createMock(LoggerInterface::class), [
+            'myep_oauth2_client_id' => 'client-id',
+            'myep_oauth2_client_secret' => 'client-secret',
+            'myep_oauth2_url_authorize' => 'https://my.example.org/authorize',
+            'myep_oauth2_url_access_token' => 'https://my.example.org/token',
+            'myep_oauth2_url_resource_owner_details' => 'https://my.example.org/api/userinfo',
+            'myep_oauth2_scopes' => ['email', 'id', 'roles', 'profile'],
+        ]);
+    }
+
+    /**
+     * @param array<string, string> $query
+     */
+    private function createRequest(array $query = []): Request
+    {
+        $request = new Request($query);
+        $request->setSession(new Session(new MockArraySessionStorage()));
+
+        return $request;
+    }
+}