8 files changed, 108 insertions(+), 55 deletions(-)

M CHANGES.md
M src/EdDSA.php
M src/HS256.php
M src/Json.php
M src/Jwt.php
M src/RS256.php
M tests/RS256Test.php
A tests/TestRS256.php

@@ 1,5 1,14 @@
 # ChangeLog
 
+## 1.1.0 (...)
+- make static code analyzers happy
+- move header check until after the signature has been verified
+- remove public `Jwt::setDateTime()`, was only used for testing and nobody was
+  supposed to use it anyway, if they do/did that is a security issue, and their
+  code MUST break
+- actual algorithm classes have now a `getAlgorithm()` method exposing the
+  JWT algorithm instead of a `const`
+
 ## 1.0.1 (2019-08-20)
 - switch to `paragonie/sodium_compat` for Composer installations
 - add benchmarks for signature validation

@@ 30,9 30,6 @@ use fkooman\Jwt\Keys\EdDSA\SecretKey;
 
 class EdDSA extends Jwt
 {
-    /** @var string */
-    const JWT_ALGORITHM = 'EdDSA';
-
     /** @var Keys\EdDSA\PublicKey */
     private $publicKey;
 


@@ 50,6 47,14 @@ class EdDSA extends Jwt
     }
 
     /**
+     * @return string
+     */
+    protected function getAlgorithm()
+    {
+        return 'EdDSA';
+    }
+
+    /**
      * @param string $inputStr
      *
      * @return string

@@ 28,9 28,6 @@ use fkooman\Jwt\Keys\HS256\SecretKey;
 
 class HS256 extends Jwt
 {
-    /** @var string */
-    const JWT_ALGORITHM = 'HS256';
-
     /** @var Keys\HS256\SecretKey */
     private $secretKey;
 


@@ 43,6 40,14 @@ class HS256 extends Jwt
     }
 
     /**
+     * @return string
+     */
+    protected function getAlgorithm()
+    {
+        return 'HS256';
+    }
+
+    /**
      * @param string $inputStr
      *
      * @return string

@@ 29,8 29,6 @@ use fkooman\Jwt\Exception\JsonException;
 class Json
 {
     /**
-     * @param array $jsonData
-     *
      * @return string
      */
     public static function encode(array $jsonData)


@@ 38,12 36,7 @@ class Json
         $jsonString = \json_encode($jsonData);
         // 5.5.0 	The return value on failure was changed from null string to FALSE.
         if (false === $jsonString || 'null' === $jsonString) {
-            throw new JsonException(
-                \sprintf(
-                    'unable to encode JSON, error code "%d"',
-                    \json_last_error()
-                )
-            );
+            throw new JsonException(\sprintf('unable to encode JSON, error code "%d"', \json_last_error()));
         }
 
         return $jsonString;


@@ 59,12 52,7 @@ class Json
         /** @psalm-suppress MixedAssignment */
         $jsonData = \json_decode($jsonString, true);
         if (null === $jsonData && JSON_ERROR_NONE !== \json_last_error()) {
-            throw new JsonException(
-                \sprintf(
-                    'unable to decode JSON, error code "%d"',
-                    \json_last_error()
-                )
-            );
+            throw new JsonException(\sprintf('unable to decode JSON, error code "%d"', \json_last_error()));
         }
 
         if (!\is_array($jsonData)) {

@@ 41,19 41,6 @@ abstract class Jwt
     protected $keyId = null;
 
     /**
-     * Override the "DateTime" for unit testing. Do NOT use this in your
-     * application.
-     *
-     * @param \DateTime $dateTime
-     *
-     * @return void
-     */
-    public function setDateTime(DateTime $dateTime)
-    {
-        $this->dateTime = $dateTime;
-    }
-
-    /**
      * @param string $keyId
      *
      * @return void


@@ 64,14 51,12 @@ abstract class Jwt
     }
 
     /**
-     * @param array $jsonData
-     *
      * @return string
      */
     public function encode(array $jsonData)
     {
         $headerData = [
-            'alg' => static::JWT_ALGORITHM,
+            'alg' => $this->getAlgorithm(),
             'typ' => 'JWT',
         ];
 


@@ 94,10 79,17 @@ abstract class Jwt
     public function decode($jwtStr)
     {
         $jwtParts = self::parseToken($jwtStr);
-        self::validateHeader($jwtParts[0]);
         if (false === $this->verify($jwtParts[0].'.'.$jwtParts[1], Base64UrlSafe::decode($jwtParts[2]))) {
             throw new JwtException('invalid signature');
         }
+
+        // as we do not need any information from the header BEFORE checking
+        // the signature, we only verify it AFTER checking the signature.
+        // --> verify signature before parsing best-practice.
+        $headerData = Json::decode(Base64UrlSafe::decode($jwtParts[0]));
+        $this->checkHeader($headerData);
+
+        // verify payload
         $payloadData = Json::decode(Base64UrlSafe::decode($jwtParts[1]));
         $this->checkToken($payloadData);
 


@@ 112,7 104,7 @@ abstract class Jwt
     public static function extractKeyId($jwtStr)
     {
         $jwtParts = self::parseToken($jwtStr);
-        $jwtHeaderData = self::validateHeader($jwtParts[0]);
+        $jwtHeaderData = Json::decode(Base64UrlSafe::decode($jwtParts[0]));
         if (!\array_key_exists('kid', $jwtHeaderData)) {
             return null;
         }


@@ 124,6 116,11 @@ abstract class Jwt
     }
 
     /**
+     * @return string
+     */
+    abstract protected function getAlgorithm();
+
+    /**
      * @param string $inputStr
      *
      * @return string


@@ 154,36 151,38 @@ abstract class Jwt
     }
 
     /**
-     * @param string $jwtHeaderStr
+     * Make sure we have an "alg" with the correct value and that "crit" is
+     * not set.
      *
-     * @return array
+     * @param array<mixed> $headerData
+     *
+     * @return void
      */
-    private static function validateHeader($jwtHeaderStr)
+    private function checkHeader(array $headerData)
     {
-        $jwtHeaderData = Json::decode(Base64UrlSafe::decode($jwtHeaderStr));
-        if (!\array_key_exists('alg', $jwtHeaderData)) {
+        if (!\array_key_exists('alg', $headerData)) {
             throw new JwtException('"alg" header key missing');
         }
-        if (static::JWT_ALGORITHM !== $jwtHeaderData['alg']) {
+        if ($this->getAlgorithm() !== $headerData['alg']) {
             throw new JwtException('unexpected "alg" value');
         }
-        if (\array_key_exists('crit', $jwtHeaderData)) {
+        if (\array_key_exists('crit', $headerData)) {
             throw new JwtException('"crit" header key not supported');
         }
-
-        return $jwtHeaderData;
     }
 
     /**
      * Verify the "exp" and "nbf" keys iff they are set.
      *
-     * @param array $payloadData
+     * @param array<mixed> $payloadData
      *
      * @return void
      */
     private function checkToken(array $payloadData)
     {
-        $dateTime = null !== $this->dateTime ? $this->dateTime : new DateTime();
+        if (null === $dateTime = $this->dateTime) {
+            $dateTime = new DateTime();
+        }
 
         // exp
         if (\array_key_exists('exp', $payloadData)) {

@@ 31,9 31,6 @@ use RuntimeException;
 
 class RS256 extends Jwt
 {
-    /** @var string */
-    const JWT_ALGORITHM = 'RS256';
-
     /** @var Keys\RS256\PublicKey */
     private $publicKey;
 


@@ 51,6 48,14 @@ class RS256 extends Jwt
     }
 
     /**
+     * @return string
+     */
+    protected function getAlgorithm()
+    {
+        return 'RS256';
+    }
+
+    /**
      * @param string $inputStr
      *
      * @return string

@@ 62,10 62,10 @@ s4cKDvb+zYNNvg2/u7KgD6vXMqmxIj3Gi8zhTP4qN2ro69YCImCHtWXXubUtvq16
 j/fxj8hQmv2KnPKtsMrGHQRso2a+NGAvHGe3N+0fyrJ+E/ANa3EpsbydmAMcneS8
 WwIDAQAB
 -----END PUBLIC KEY-----');
-        $r = new RS256(
-            $publicKey
+        $r = new TestRS256(
+            $publicKey,
+            new DateTime('@1534756800')
         );
-        $r->setDateTime(new DateTime('@1534756800'));
         $jwtStr = 'eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9.eyJpc3MiOiJodHRwczpcL1wvYXV0aC5kYXRhcG9ydGVuLm5vIiwiYXVkIjoiNjVlMGE2MDktNzcwZC00ODk5LTlhMTYtYzUwMDkxNTQyZTE2Iiwic3ViIjoiNTVkZTdkNzEtNGEyNS00MTAzLThlNDMtMzVkZjhjMmQ0NzJhIiwiaWF0IjoxNTM0NzUzMjgzLCJleHAiOjE1MzQ3NTY4ODMsImF1dGhfdGltZSI6MTUzNDc1MzI4MX0.i3OLSrRl3hiEHoH7X7aceOHI7-UVj-G9L554hz1cC1jcCgsWlFTILHvDTKA6Qt2wy4gSE6TMotnjuJePt5ZnMllwwESIyCdSF3YQjF-A8Fz-DOKP24iyVmPgYuFMZ_m8gqKn0TaVTEcy5MOPncvPj53v0Zhr8VyxBY39qA9Gbbzvhhns72lWuhePNx6QLxoeEQx3UVQd6fNlXRj5cmgGGUOYNZ-_wDFmGbigC2mBlFQvs7Hhu6wAB2LLN16Fcc2Q6rXJ6CXJVuZQDqulLvxNGnOSrTOQxPTG1b8tbEdN1skhphqVDBSh0ZP1bnTwNhaB98IdKjkU2DTFqsKSCmrAmg';
         $payloadData = [
             'iss' => 'https://auth.dataporten.no',

@@ 0,0 1,42 @@
+<?php
+
+/*
+ * Copyright (c) 2019 François Kooman <fkooman@tuxed.net>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+namespace fkooman\Jwt\Tests;
+
+use DateTime;
+use fkooman\Jwt\Keys\RS256\PublicKey;
+use fkooman\Jwt\RS256;
+
+/*
+ * We have a TestRS256 in order to override DateTime for not failing on expired
+ * tokens.
+ */
+class TestRS256 extends RS256
+{
+    public function __construct(PublicKey $publicKey, DateTime $dateTime)
+    {
+        parent::__construct($publicKey);
+        $this->dateTime = $dateTime;
+    }
+}