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.
slimphp · Jan 7, 2021 · 99234ed · 99234ed
1 parent d641663
commit 99234ed
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 8 deletions.
diff --git 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
@@ -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')