Merge branch 'master' into features/allow-client_secret_basic-on-requ…

…est-token * master: fix: protected responseContentType to allow overloading of fetchUrl function (#446) test: unit tests for verifyJWTClaims and different aud claims (#443) Fix TypeError in `verifyJWTClaims` (#442) release: v1.0.2 (#439) test: add unit test for SERVER_PORT type cast (#438) fix: bring back #404 (#437) release: v1.0.1 (#432) fix: protected $responseCode to allow proper overloading of fetchURL() (#433) chore(deps-dev): update yoast/phpunit-polyfills requirement from ^1.0 to ^2.0 (#430) chore(deps): update phpseclib/phpseclib requirement from ~3.0 to ^3.0.7 ci: run GitHub workflows on pull requests and pushes to master (#431) chore: enable dependabot for composer (#429) fix: handle JWT decode of non JWT tokens (#428) fix: method signatures after 1.0 release (#427)
jumbojett · Nov 7, 2024 · 48d38b3 · 48d38b3
2 parents c9ee737 + 9b64e0d
commit 48d38b3
Show file tree

Hide file tree

Showing 6 changed files with 151 additions and 30 deletions.
diff --git a/.github/dependabot.yml b/.github/dependabot.yml
@@ -11,3 +11,9 @@ updates:
     directory: "/"
     schedule:
       interval: "weekly"
+
+  # Maintain dependencies for composer
+  - package-ecosystem: "composer"
+    directory: "/"
+    schedule:
+      interval: "weekly"
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -1,7 +1,13 @@
 ---
 name: build
 
-on: [push, pull_request]
+on:
+  push:
+    branches:
+      - master
+  pull_request:
+    branches:
+      - master
 
 env:
     DEFAULT_COMPOSER_FLAGS: "--prefer-dist --no-interaction --no-progress --optimize-autoloader --ansi"
@@ -35,4 +41,4 @@ jobs:
             - name: Install dependencies
               run: composer update $DEFAULT_COMPOSER_FLAGS
             - name: Run unit tests
-              run: vendor/bin/phpunit --verbose --colors=always tests
+              run: vendor/bin/phpunit --colors=always tests
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,10 +4,16 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-[unreleased]
-- Updated CI to also test on PHP 8.3 #407
-- Updated readme PHP requirement to PHP 7.0+ #407
-- Added dependabot for GitHub Actions #407
+## [1.0.1] - 2024-09-13
+
+### Fixed
+- Cast `$_SERVER['SERVER_PORT']` to integer to prevent adding 80 or 443 port to redirect URL. #437
+
+## [1.0.1] - 2024-09-05
+
+### Fixed
+- Fix JWT decode of non JWT tokens #428
+- Fix method signatures #427
 - Cast `$_SERVER['SERVER_PORT']` to integer to prevent adding 80 or 443 port to redirect URL. #403
 - Check subject when verifying JWT #406
 - Removed duplicate check on jwks_uri and only check if jwks_uri exists when needed #373

diff --git a/composer.json b/composer.json
@@ -6,11 +6,12 @@
         "php": ">=7.0",
         "ext-json": "*",
         "ext-curl": "*",
-        "phpseclib/phpseclib": "~3.0"
+        "phpseclib/phpseclib": "^3.0.7"
     },
     "require-dev": {
+        "phpunit/phpunit": "<10",
         "roave/security-advisories": "dev-latest",
-        "yoast/phpunit-polyfills": "^1.0"
+        "yoast/phpunit-polyfills": "^2.0"
     },
 	"replace": {
 		"jumbojett/openid-connect-php": "<=0.9.10"

diff --git a/src/OpenIDConnectClient.php b/src/OpenIDConnectClient.php
@@ -3,7 +3,7 @@
  *
  * Copyright MITRE 2020
  *
- * OpenIDConnectClient for PHP5
+ * OpenIDConnectClient for PHP7+
  * Author: Michael Jett <mjett@mitre.org>
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -25,7 +25,6 @@
 
 use Error;
 use Exception;
-use phpseclib3\Crypt\PublicKeyLoader;
 use phpseclib3\Crypt\RSA;
 use phpseclib3\Math\BigInteger;
 use stdClass;
@@ -145,12 +144,12 @@ class OpenIDConnectClient
     /**
      * @var int|null Response code from the server
      */
-    private $responseCode;
+    protected $responseCode;
 
     /**
      * @var string|null Content type from the server
      */
-    private $responseContentType;
+    protected $responseContentType;
 
     /**
      * @var array holds response types
@@ -380,7 +379,7 @@ public function authenticate(): bool
             $accessToken = $_REQUEST['access_token'] ?? null;
 
             // Do an OpenID Connect session check
-	    if (!isset($_REQUEST['state']) || ($_REQUEST['state'] !== $this->getState())) {            
+    	    if (!isset($_REQUEST['state']) || ($_REQUEST['state'] !== $this->getState())) {
                 throw new OpenIDConnectClientException('Unable to determine state');
             }
 
@@ -691,6 +690,7 @@ public function getRedirectURL(): string
         if (isset($_SERVER['HTTP_X_FORWARDED_PORT'])) {
             $port = (int)$_SERVER['HTTP_X_FORWARDED_PORT'];
         } elseif (isset($_SERVER['SERVER_PORT'])) {
+            # keep this case - even if some tool claim it is unnecessary
             $port = (int)$_SERVER['SERVER_PORT'];
         } elseif ($protocol === 'https') {
             $port = 443;
@@ -1212,8 +1212,10 @@ protected function verifyJWTClaims($claims, string $accessToken = null): bool
             $len = ((int)$bit)/16;
             $expected_at_hash = $this->urlEncode(substr(hash('sha'.$bit, $accessToken, true), 0, $len));
         }
+        $auds = $claims->aud;
+        $auds = is_array( $auds ) ? $auds : [ $auds ];
         return (($this->validateIssuer($claims->iss))
-            && (($claims->aud === $this->clientID) || in_array($this->clientID, $claims->aud, true))
+            && (in_array($this->clientID, $auds, true))
             && ($claims->sub === $this->getIdTokenPayload()->sub)
             && (!isset($claims->nonce) || $claims->nonce === $this->getNonce())
             && ( !isset($claims->exp) || ((is_int($claims->exp)) && ($claims->exp >= time() - $this->leeway)))
@@ -1232,12 +1234,11 @@ protected function urlEncode(string $str): string
     /**
      * @param string $jwt encoded JWT
      * @param int $section the section we would like to decode
-     * @return object
+     * @return object|string|null
      */
-    protected function decodeJWT(string $jwt, int $section = 0): stdClass {
-
+    protected function decodeJWT(string $jwt, int $section = 0) {
         $parts = explode('.', $jwt);
-        return json_decode(base64url_decode($parts[$section]), false);
+        return json_decode(base64url_decode($parts[$section] ?? ''), false);
     }
 
     /**
@@ -1699,7 +1700,10 @@ public function revokeToken(string $token, string $token_type_hint = '', string
         return json_decode($this->fetchURL($revocation_endpoint, $post_params, $headers), false);
     }
 
-    public function getClientName(): string
+    /**
+     * @return string|null
+     */
+    public function getClientName()
     {
         return $this->clientName;
     }
@@ -1709,14 +1713,14 @@ public function setClientName(string $clientName) {
     }
 
     /**
-     * @return string
+     * @return string|null
      */
     public function getClientID() {
         return $this->clientID;
     }
 
     /**
-     * @return string
+     * @return string|null
      */
     public function getClientSecret() {
         return $this->clientSecret;
@@ -1731,17 +1735,30 @@ public function setAccessToken(string $accessToken) {
         $this->accessToken = $accessToken;
     }
 
-    public function getAccessToken(): string
+    /**
+     * @return string|null
+     */
+    public function getAccessToken()
     {
         return $this->accessToken;
     }
 
-    public function getRefreshToken(): string
+    /**
+     * @return string|null
+     */
+    public function getRefreshToken()
     {
         return $this->refreshToken;
     }
 
-    public function getIdToken(): string
+    public function setIdToken(string $idToken) {
+        $this->idToken = $idToken;
+    }
+
+    /**
+     * @return string|null
+     */
+    public function getIdToken()
     {
         return $this->idToken;
     }
@@ -1754,21 +1771,21 @@ public function getAccessTokenHeader() {
     }
 
     /**
-     * @return object
+     * @return object|string|null
      */
     public function getAccessTokenPayload() {
         return $this->decodeJWT($this->accessToken, 1);
     }
 
     /**
-     * @return object
+     * @return object|string|null
      */
     public function getIdTokenHeader() {
         return $this->decodeJWT($this->idToken);
     }
 
     /**
-     * @return object
+     * @return object|string|null
      */
     public function getIdTokenPayload() {
         return $this->decodeJWT($this->idToken, 1);

diff --git a/tests/OpenIDConnectClientTest.php b/tests/OpenIDConnectClientTest.php
@@ -7,9 +7,90 @@
 
 class OpenIDConnectClientTest extends TestCase
 {
-    /**
-     * @return void
-     */
+    public function testValidateClaims()
+    {
+        $client = new class extends OpenIDConnectClient {
+            public function testVerifyJWTClaims($claims): bool
+            {
+                return $this->verifyJWTClaims($claims);
+            }
+            public function getIdTokenPayload()
+            {
+                return (object)[
+                    'sub' => 'sub'
+                ];
+            }
+        };
+        $client->setClientID('client-id');
+        $client->setIssuer('issuer');
+        $client->setIdToken('');
+
+        # simple aud
+        $valid = $client->testVerifyJWTClaims((object)[
+            'aud' => 'client-id',
+            'iss' => 'issuer',
+            'sub' => 'sub',
+        ]);
+        self::assertTrue($valid);
+
+        # array aud
+        $valid = $client->testVerifyJWTClaims((object)[
+            'aud' => ['client-id'],
+            'iss' => 'issuer',
+            'sub' => 'sub',
+        ]);
+        self::assertTrue($valid);
+
+        # aud not matching
+        $valid = $client->testVerifyJWTClaims((object)[
+            'aud' => ['ipsum'],
+            'iss' => 'issuer',
+            'sub' => 'sub',
+        ]);
+        self::assertFalse($valid);
+    }
+    public function testJWTDecode()
+    {
+        $client = new OpenIDConnectClient();
+        # access token
+        $client->setAccessToken('');
+        $header = $client->getAccessTokenHeader();
+        self::assertEquals('', $header);
+        $payload = $client->getAccessTokenPayload();
+        self::assertEquals('', $payload);
+
+        # id token
+        $client->setIdToken('');
+        $header = $client->getIdTokenHeader();
+        self::assertEquals('', $header);
+        $payload = $client->getIdTokenPayload();
+        self::assertEquals('', $payload);
+    }
+
+    public function testGetNull()
+    {
+        $client = new OpenIDConnectClient();
+        self::assertNull($client->getAccessToken());
+        self::assertNull($client->getRefreshToken());
+        self::assertNull($client->getIdToken());
+        self::assertNull($client->getClientName());
+        self::assertNull($client->getClientID());
+        self::assertNull($client->getClientSecret());
+        self::assertNull($client->getCertPath());
+    }
+
+    public function testResponseTypes()
+    {
+        $client = new OpenIDConnectClient();
+        self::assertEquals([], $client->getResponseTypes());
+
+        $client->setResponseTypes('foo');
+        self::assertEquals(['foo'], $client->getResponseTypes());
+
+        $client->setResponseTypes(['bar', 'ipsum']);
+        self::assertEquals(['foo', 'bar', 'ipsum'], $client->getResponseTypes());
+    }
+
     public function testGetRedirectURL()
     {
         $client = new OpenIDConnectClient();
@@ -18,7 +99,11 @@ public function testGetRedirectURL()
 
         $_SERVER['SERVER_NAME'] = 'domain.test';
         $_SERVER['REQUEST_URI'] = '/path/index.php?foo=bar&baz#fragment';
+        $_SERVER['SERVER_PORT'] = '443';
         self::assertSame('http://domain.test/path/index.php', $client->getRedirectURL());
+
+        $_SERVER['SERVER_PORT'] = '8888';
+        self::assertSame('http://domain.test:8888/path/index.php', $client->getRedirectURL());
     }
 
     public function testAuthenticateDoesNotThrowExceptionIfClaimsIsMissingNonce()