fix: verify the oauth2 state before the error parameter

src/Security/OAuth2/MyEpClient.php

@@ -61,30 +61,15 @@ class MyEpClient
         $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);
-        }
-
         $state = $request->query->get('state');
 
+        // Before anything else, including the error parameter: RFC 6749 4.1.2.1 asks for the
+        // state to be verified on an error response too, and there is a practical reason to.
+        // The session cookie is SameSite=lax, so a top-level navigation to this route carries
+        // the victim's cookie, and the two removes above have already consumed their pending
+        // flow. Refusing here means a callback that cannot prove the state no longer destroys
+        // a login it did not start.
+        //
         // 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.
@@ -97,6 +82,25 @@ class MyEpClient
             throw new AuthorizationRequestException('Missing state or mismatch', 400, $request);
         }
 
+        // Now that the callback is known to belong to this flow: 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. Not logged — this is no failure of ours, and the
+        // caller decides what to make of it from the exception.
+        $error = $request->query->get('error');
+
+        if (true === is_string($error) && '' !== $error) {
+            $description = $request->query->get('error_description');
+            $description = is_string($description) && '' !== $description ? $description : null;
+
+            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);
+        }
+
         // 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.

tests/Security/OAuth2/MyEpClientTest.php

@@ -32,7 +32,7 @@ class MyEpClientTest extends TestCase
 
         // 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)), '+/', '-_'), '=');
+        $expectedChallenge = rtrim(strtr(base64_encode(hash('sha256', $verifier, true)), '+/', '-_'), '=');
         $this->assertSame($expectedChallenge, $query['code_challenge']);
     }
 
@@ -59,6 +59,25 @@ class MyEpClientTest extends TestCase
         $this->assertFalse($request->getSession()->has('oauth2pkce'));
     }
 
+    public function testDeniedCallbackWithAForeignStateIsRefusedAsAMismatch(): void
+    {
+        $request = $this->createRequest([
+            'state' => 'other-state',
+            'error' => 'access_denied',
+        ]);
+        $request->getSession()->set('oauth2state', 'expected-state');
+        $request->getSession()->set('oauth2pkce', 'verifier');
+
+        // the state is checked before the error parameter: a callback that cannot prove it
+        // belongs to this flow must not be able to report a denial into it. Otherwise a
+        // link to this route would consume the pending state of whoever follows it - the
+        // session cookie is SameSite=lax, so it rides along on a top-level navigation.
+        $this->expectException(AuthorizationRequestException::class);
+        $this->expectExceptionMessage('Missing state or mismatch');
+
+        $this->createClient()->fetchAccessToken($request);
+    }
+
     public function testCallbackIsRefusedWhenTheSessionHoldsNoVerifier(): void
     {
         $request = $this->createRequest(['code' => 'a-code', 'state' => 'expected-state']);