From 99234ed33f6f504de90d92ad27cd4fc388bfe447 Mon Sep 17 00:00:00 2001
From: Rob Allen <rob@akrabat.com>
Date: Mon, 4 Jan 2021 13:55:23 +0000
Subject: [PATCH] Error when the token is in the body of GET request

Methods such as GET should not be used for state change operations
(RFC2616). If the body of a GET request has the token in it, then treat
that as an error.

Thanks to Xhelal Likaj (https://github.com/xhlika) for the report.
---
 src/Guard.php       | 21 +++++++++++++--------
 tests/GuardTest.php | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/src/Guard.php b/src/Guard.php
index 4936459..c4eda33 100644
--- a/src/Guard.php
+++ b/src/Guard.php
@@ -405,16 +405,16 @@ protected function appendTokenToRequest(ServerRequestInterface $request, array $
      */
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
-        if (in_array($request->getMethod(), ['POST', 'PUT', 'DELETE', 'PATCH'])) {
-            $body = $request->getParsedBody();
-            $name = null;
-            $value = null;
+        $body = $request->getParsedBody();
+        $name = null;
+        $value = null;
 
-            if (is_array($body)) {
-                $name = $body[$this->getTokenNameKey()] ?? null;
-                $value = $body[$this->getTokenValueKey()] ?? null;
-            }
+        if (is_array($body)) {
+            $name = $body[$this->getTokenNameKey()] ?? null;
+            $value = $body[$this->getTokenValueKey()] ?? null;
+        }
 
+        if (in_array($request->getMethod(), ['POST', 'PUT', 'DELETE', 'PATCH'])) {
             $isValid = $this->validateToken((string) $name, (string) $value);
             if ($isValid && !$this->persistentTokenMode) {
                 // successfully validated token, so delete it if not in persistentTokenMode
@@ -425,6 +425,11 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
                 $request = $this->appendNewTokenToRequest($request);
                 return $this->handleFailure($request, $handler);
             }
+        } else {
+            // Method is GET/OPTIONS/HEAD/etc, so do not accept the token in the body of this request
+            if ($name !== null) {
+                return $this->handleFailure($request, $handler);
+            }
         }
 
         if (!$this->persistentTokenMode || !$this->loadLastKeyPair()) {
diff --git a/tests/GuardTest.php b/tests/GuardTest.php
index bd410c6..06e6e37 100644
--- a/tests/GuardTest.php
+++ b/tests/GuardTest.php
@@ -318,6 +318,43 @@ public function testTokenIsRemovedFromStorageWhenPersistentModeIsOff()
         self::assertArrayNotHasKey('test_name', $storage);
     }
 
+    public function testTokenInBodyofGetIsInvalid()
+    {
+        $storage = [
+            'test_name' => 'test_value123',
+        ];
+
+        // we set up a failure handler that we expect to be called because a GET cannot have a token
+        $self = $this;
+        $failureHandlerCalled = 0;
+        $failureHandler = function () use ($self, &$failureHandlerCalled) {
+            $failureHandlerCalled++;
+            $responseProphecy = $self->prophesize(ResponseInterface::class);
+            return $responseProphecy->reveal();
+        };
+
+        $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class);
+
+        $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, $failureHandler);
+
+        $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class);
+
+        $requestProphecy = $this->prophesize(ServerRequestInterface::class);
+        $requestProphecy
+            ->getMethod()
+            ->willReturn('GET')
+            ->shouldBeCalledOnce();
+        $requestProphecy
+            ->getParsedBody()
+            ->willReturn([
+                'test_name' => 'test_name',
+                'test_value' => 'test_value123',
+            ])
+            ->shouldBeCalledOnce();
+
+        $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal());
+        self::assertSame(1, $failureHandlerCalled);
+    }
 
     public function testProcessAppendsNewTokensWhenPersistentTokenModeIsOff()
     {
@@ -328,6 +365,7 @@ public function testProcessAppendsNewTokensWhenPersistentTokenModeIsOff()
         $responseProphecy = $this->prophesize(ResponseInterface::class);
 
         $requestProphecy = $this->prophesize(ServerRequestInterface::class);
+        $requestProphecy->getParsedBody()->willReturn(null)->shouldBeCalledOnce();
         $requestProphecy
             ->getMethod()
             ->willReturn('GET')
@@ -359,6 +397,7 @@ public function testProcessAppendsNewTokensWhenPersistentTokenModeIsOn()
         $responseProphecy = $this->prophesize(ResponseInterface::class);
 
         $requestProphecy = $this->prophesize(ServerRequestInterface::class);
+        $requestProphecy->getParsedBody()->willReturn(null)->shouldBeCalledOnce();
         $requestProphecy
             ->getMethod()
             ->willReturn('GET')