From f3c1fad50a54395bb61f828841399fa98989684b Mon Sep 17 00:00:00 2001 From: milnerTill Date: Mon, 20 Apr 2026 11:35:35 -0300 Subject: [PATCH 1/2] add support to HS256 algorithm --- app/Access/Controllers/OidcController.php | 17 +++++ ...idcHttpBasicWithClientIdOptionProvider.php | 27 +++++++ app/Access/Oidc/OidcIdToken.php | 6 +- app/Access/Oidc/OidcJwtWithClaims.php | 73 +++++++++++-------- app/Access/Oidc/OidcProviderSettings.php | 2 +- app/Access/Oidc/OidcService.php | 5 +- lang/en/errors.php | 1 + lang/es/errors.php | 1 + lang/es_AR/errors.php | 1 + package-lock.json | 2 +- 10 files changed, 97 insertions(+), 38 deletions(-) create mode 100644 app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php index 654ed692880..20313060212 100644 --- a/app/Access/Controllers/OidcController.php +++ b/app/Access/Controllers/OidcController.php @@ -55,6 +55,7 @@ public function callback(Request $request) } try { + $this->_catchCustomErrorAndFail($request); $this->oidcService->processAuthorizeResponse($request->query('code')); } catch (OidcException $oidcException) { $this->showErrorNotification($oidcException->getMessage()); @@ -72,4 +73,20 @@ public function logout() { return redirect($this->oidcService->logout()); } + + private function _catchCustomErrorAndFail(Request $request):void { + $_errorCode = $request->query('error'); + if ($_errorCode) { + if ($_errorCode === 'need_auth') { + if ( null === ($errorMsg = $request->query('error_description')) ){ + $customError = trans('errors.oidc_need_auth', ['system' => config('oidc.name')]); + }else { + $customError = $errorMsg; + } + } + throw new OidcException($customError); + } + } + + } diff --git a/app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php b/app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php new file mode 100644 index 00000000000..b5aca63f31b --- /dev/null +++ b/app/Access/Oidc/OidcHttpBasicWithClientIdOptionProvider.php @@ -0,0 +1,27 @@ +validateTokenClaims($clientId); + parent::validateCommonTokenDetails($settings); + $this->validateTokenClaims($settings->clientId); return true; } diff --git a/app/Access/Oidc/OidcJwtWithClaims.php b/app/Access/Oidc/OidcJwtWithClaims.php index 9d7eeead1a9..b3da04ee516 100644 --- a/app/Access/Oidc/OidcJwtWithClaims.php +++ b/app/Access/Oidc/OidcJwtWithClaims.php @@ -9,7 +9,9 @@ class OidcJwtWithClaims implements ProvidesClaims protected string $signature; protected string $issuer; protected array $tokenParts = []; - + protected array $acceptedSignatures = [self::hs256Signature, self::rs256Signature]; + private const hs256Signature = 'HS256' + , rs256Signature = 'RS256'; /** * @var array[]|string[] */ @@ -59,11 +61,11 @@ protected function base64UrlDecode(string $encoded): string * * @throws OidcInvalidTokenException */ - public function validateCommonTokenDetails(string $clientId): bool + public function validateCommonTokenDetails(OidcProviderSettings $settings): bool { $this->validateTokenStructure(); - $this->validateTokenSignature(); - $this->validateCommonClaims($clientId); + $this->validateTokenSignature($settings); + $this->validateCommonClaims($settings->clientId); return true; } @@ -102,12 +104,12 @@ public function replaceClaims(array $claims): void protected function validateTokenStructure(): void { foreach (['header', 'payload'] as $prop) { - if (empty($this->$prop)) { + if (empty($this->$prop) || !is_array($this->$prop)) { throw new OidcInvalidTokenException("Could not parse out a valid {$prop} within the provided token"); } } - if (empty($this->signature)) { + if (empty($this->signature) || !is_string($this->signature)) { throw new OidcInvalidTokenException('Could not parse out a valid signature within the provided token'); } } @@ -117,31 +119,42 @@ protected function validateTokenStructure(): void * * @throws OidcInvalidTokenException */ - protected function validateTokenSignature(): void - { - if ($this->header['alg'] !== 'RS256') { - throw new OidcInvalidTokenException("Only RS256 signature validation is supported. Token reports using {$this->header['alg']}"); + protected function validateTokenSignature(OidcProviderSettings $settings): void { + $validSignatures = implode(', ',$this->acceptedSignatures); + switch ($this->header['alg']) { + case self::rs256Signature: + $parsedKeys = array_map(function ($key) { + try { + return new OidcJwtSigningKey($key); + } catch (OidcInvalidKeyException $e) { + throw new OidcInvalidTokenException('Failed to read signing key with error: ' . $e->getMessage()); + } + }, $this->keys); + + $parsedKeys = array_filter($parsedKeys); + + $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1]; + /** @var OidcJwtSigningKey $parsedKey */ + foreach ($parsedKeys as $parsedKey) { + if ($parsedKey->verify($contentToSign, $this->signature)) { + return; + } + } + + throw new OidcInvalidTokenException('Token signature could not be validated using the provided keys'); + case self::hs256Signature: + $secret = $settings->clientSecret; + $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1]; + $expectedSignature = hash_hmac('sha256', $contentToSign, $secret, true); + + if (hash_equals($expectedSignature, $this->signature)) { + return; + } + + throw new OidcInvalidTokenException('Token signature could not be validated using the provided secret'); + default: + throw new OidcInvalidTokenException("Only $validSignatures signatures validation are supported. Token reports using {$this->header['alg']}"); } - - $parsedKeys = array_map(function ($key) { - try { - return new OidcJwtSigningKey($key); - } catch (OidcInvalidKeyException $e) { - throw new OidcInvalidTokenException('Failed to read signing key with error: ' . $e->getMessage()); - } - }, $this->keys); - - $parsedKeys = array_filter($parsedKeys); - - $contentToSign = $this->tokenParts[0] . '.' . $this->tokenParts[1]; - /** @var OidcJwtSigningKey $parsedKey */ - foreach ($parsedKeys as $parsedKey) { - if ($parsedKey->verify($contentToSign, $this->signature)) { - return; - } - } - - throw new OidcInvalidTokenException('Token signature could not be validated using the provided keys'); } /** diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php index 71c3b573421..573d2beefe5 100644 --- a/app/Access/Oidc/OidcProviderSettings.php +++ b/app/Access/Oidc/OidcProviderSettings.php @@ -74,7 +74,7 @@ public function validate(): void { $this->validateInitial(); - $required = ['keys', 'tokenEndpoint', 'authorizationEndpoint']; + $required = ['tokenEndpoint', 'authorizationEndpoint']; foreach ($required as $prop) { if (empty($this->$prop)) { throw new InvalidArgumentException("Missing required configuration \"{$prop}\" value"); diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index a84bd320513..60eb0a4d1fd 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -14,7 +14,6 @@ use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\User; use Illuminate\Support\Facades\Cache; -use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider; use League\OAuth2\Client\Provider\Exception\IdentityProviderException; /** @@ -140,7 +139,7 @@ protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvide 'redirectUri' => url('/oidc/callback'), ], [ 'httpClient' => $this->http->buildClient(5), - 'optionProvider' => new HttpBasicAuthOptionProvider(), + 'optionProvider' => new OidcHttpBasicWithClientIdOptionProvider(), ]); foreach ($this->getAdditionalScopes() as $scope) { @@ -199,7 +198,7 @@ protected function processAccessTokenCallback(OidcAccessToken $accessToken, Oidc } try { - $idToken->validate($settings->clientId); + $idToken->validate($settings); } catch (OidcInvalidTokenException $exception) { throw new OidcException("ID token validation failed with error: {$exception->getMessage()}"); } diff --git a/lang/en/errors.php b/lang/en/errors.php index 20537d59f0c..89225c9fe45 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -26,6 +26,7 @@ 'oidc_already_logged_in' => 'Already logged in', 'oidc_no_email_address' => 'Could not find an email address, for this user, in the data provided by the external authentication system', 'oidc_fail_authed' => 'Login using :system failed, system did not provide successful authorization', + 'oidc_need_auth' => 'You need to be authenticated to perform this action.', 'social_no_action_defined' => 'No action defined', 'social_login_bad_response' => "Error received during :socialAccount login: \n:error", 'social_account_in_use' => 'This :socialAccount account is already in use, Try logging in via the :socialAccount option.', diff --git a/lang/es/errors.php b/lang/es/errors.php index 25f82a5d872..fd16c3163e2 100644 --- a/lang/es/errors.php +++ b/lang/es/errors.php @@ -26,6 +26,7 @@ 'oidc_already_logged_in' => 'Ya tenías la sesión iniciada', 'oidc_no_email_address' => 'No se pudo encontrar una dirección de correo electrónico, para este usuario, en los datos proporcionados por el sistema de autenticación externo', 'oidc_fail_authed' => 'El inicio de sesión con :system falló, el sistema no proporcionó una autorización correcta', + 'oidc_need_auth' => 'Necesita estar autenticado para realizar esta acción.', 'social_no_action_defined' => 'Acción no definida', 'social_login_bad_response' => "Se ha recibido un error durante el acceso con :socialAccount error: \n:error", 'social_account_in_use' => 'la cuenta :socialAccount ya se encuentra en uso, intente acceder a través de la opción :socialAccount .', diff --git a/lang/es_AR/errors.php b/lang/es_AR/errors.php index 6f6b25bcf98..c24fde88cfd 100644 --- a/lang/es_AR/errors.php +++ b/lang/es_AR/errors.php @@ -26,6 +26,7 @@ 'oidc_already_logged_in' => 'Ya está conectado', 'oidc_no_email_address' => 'No se pudo encontrar una dirección de correo electrónico para este usuario en los datos proporcionados por el sistema de autenticación externo', 'oidc_fail_authed' => 'El inicio de sesión con :system falló, el sistema no proporcionó una autorización correcta', + 'oidc_need_auth' => 'Necesita estar autenticado para realizar esta acción.', 'social_no_action_defined' => 'Acción no definida', 'social_login_bad_response' => "SE recibió un Error durante el acceso con :socialAccount : \n:error", 'social_account_in_use' => 'la cuenta :socialAccount ya se encuentra en uso, intente loguearse a través de la opcón :socialAccount .', diff --git a/package-lock.json b/package-lock.json index d239426ec2c..97bb4b7f2fe 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "bookstack", + "name": "openId-HS256-support", "lockfileVersion": 3, "requires": true, "packages": { From 844c79ae72f92bdcf8a72df756f017e1ee5b3858 Mon Sep 17 00:00:00 2001 From: milnerTill Date: Mon, 20 Apr 2026 13:38:17 -0300 Subject: [PATCH 2/2] improved error handling and tests added --- app/Access/Controllers/OidcController.php | 29 +++++++++------- lang/en/errors.php | 1 - lang/es/errors.php | 1 - lang/es_AR/errors.php | 1 - package-lock.json | 2 +- tests/Helpers/OidcJwtHelper.php | 36 ++++++++++++++++++++ tests/Unit/OidcIdTokenTest.php | 41 +++++++++++++++++------ 7 files changed, 83 insertions(+), 28 deletions(-) diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php index 20313060212..b9cd57243b4 100644 --- a/app/Access/Controllers/OidcController.php +++ b/app/Access/Controllers/OidcController.php @@ -55,7 +55,7 @@ public function callback(Request $request) } try { - $this->_catchCustomErrorAndFail($request); + $this->throwIfAuthorizationError($request); $this->oidcService->processAuthorizeResponse($request->query('code')); } catch (OidcException $oidcException) { $this->showErrorNotification($oidcException->getMessage()); @@ -74,19 +74,22 @@ public function logout() return redirect($this->oidcService->logout()); } - private function _catchCustomErrorAndFail(Request $request):void { - $_errorCode = $request->query('error'); - if ($_errorCode) { - if ($_errorCode === 'need_auth') { - if ( null === ($errorMsg = $request->query('error_description')) ){ - $customError = trans('errors.oidc_need_auth', ['system' => config('oidc.name')]); - }else { - $customError = $errorMsg; - } - } - throw new OidcException($customError); + /** + * + * @throws OidcException + */ + private function throwIfAuthorizationError(Request $request): void + { + $errorCode = $request->query('error'); + if (!$errorCode) { + return; } - } + $errorDescription = $request->query('error_description'); + if ($errorDescription) { + throw new OidcException($errorDescription); + } + throw new OidcException(trans('errors.oidc_fail_authed', ['system' => config('oidc.name')])); + } } diff --git a/lang/en/errors.php b/lang/en/errors.php index 89225c9fe45..20537d59f0c 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -26,7 +26,6 @@ 'oidc_already_logged_in' => 'Already logged in', 'oidc_no_email_address' => 'Could not find an email address, for this user, in the data provided by the external authentication system', 'oidc_fail_authed' => 'Login using :system failed, system did not provide successful authorization', - 'oidc_need_auth' => 'You need to be authenticated to perform this action.', 'social_no_action_defined' => 'No action defined', 'social_login_bad_response' => "Error received during :socialAccount login: \n:error", 'social_account_in_use' => 'This :socialAccount account is already in use, Try logging in via the :socialAccount option.', diff --git a/lang/es/errors.php b/lang/es/errors.php index fd16c3163e2..25f82a5d872 100644 --- a/lang/es/errors.php +++ b/lang/es/errors.php @@ -26,7 +26,6 @@ 'oidc_already_logged_in' => 'Ya tenías la sesión iniciada', 'oidc_no_email_address' => 'No se pudo encontrar una dirección de correo electrónico, para este usuario, en los datos proporcionados por el sistema de autenticación externo', 'oidc_fail_authed' => 'El inicio de sesión con :system falló, el sistema no proporcionó una autorización correcta', - 'oidc_need_auth' => 'Necesita estar autenticado para realizar esta acción.', 'social_no_action_defined' => 'Acción no definida', 'social_login_bad_response' => "Se ha recibido un error durante el acceso con :socialAccount error: \n:error", 'social_account_in_use' => 'la cuenta :socialAccount ya se encuentra en uso, intente acceder a través de la opción :socialAccount .', diff --git a/lang/es_AR/errors.php b/lang/es_AR/errors.php index c24fde88cfd..6f6b25bcf98 100644 --- a/lang/es_AR/errors.php +++ b/lang/es_AR/errors.php @@ -26,7 +26,6 @@ 'oidc_already_logged_in' => 'Ya está conectado', 'oidc_no_email_address' => 'No se pudo encontrar una dirección de correo electrónico para este usuario en los datos proporcionados por el sistema de autenticación externo', 'oidc_fail_authed' => 'El inicio de sesión con :system falló, el sistema no proporcionó una autorización correcta', - 'oidc_need_auth' => 'Necesita estar autenticado para realizar esta acción.', 'social_no_action_defined' => 'Acción no definida', 'social_login_bad_response' => "SE recibió un Error durante el acceso con :socialAccount : \n:error", 'social_account_in_use' => 'la cuenta :socialAccount ya se encuentra en uso, intente loguearse a través de la opcón :socialAccount .', diff --git a/package-lock.json b/package-lock.json index 97bb4b7f2fe..d239426ec2c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "openId-HS256-support", + "name": "bookstack", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/tests/Helpers/OidcJwtHelper.php b/tests/Helpers/OidcJwtHelper.php index 55a34d4dc96..97841a68896 100644 --- a/tests/Helpers/OidcJwtHelper.php +++ b/tests/Helpers/OidcJwtHelper.php @@ -2,6 +2,7 @@ namespace Tests\Helpers; +use BookStack\Access\Oidc\OidcProviderSettings; use phpseclib3\Crypt\RSA; /** @@ -119,6 +120,41 @@ public static function privatePemKey(): string -----END PRIVATE KEY-----'; } + public static function defaultSecret(): string + { + return 'test-client-secret-for-hs256'; + } + + /** + * Build a minimal OidcProviderSettings for use in token validation tests. + */ + public static function defaultProviderSettings(array $overrides = []): OidcProviderSettings + { + return new OidcProviderSettings(array_merge([ + 'issuer' => static::defaultIssuer(), + 'clientId' => static::defaultClientId(), + 'clientSecret' => static::defaultSecret(), + ], $overrides)); + } + + /** + * Build an HS256-signed ID token using the given secret. + */ + public static function hs256IdToken(string $secret, array $payloadOverrides = []): string + { + $payload = array_merge(static::defaultPayload(), $payloadOverrides); + $header = ['alg' => 'HS256', 'typ' => 'JWT']; + + $top = implode('.', [ + static::base64UrlEncode(json_encode($header)), + static::base64UrlEncode(json_encode($payload)), + ]); + + $signature = hash_hmac('sha256', $top, $secret, true); + + return $top . '.' . static::base64UrlEncode($signature); + } + public static function publicJwkKeyArray(): array { return [ diff --git a/tests/Unit/OidcIdTokenTest.php b/tests/Unit/OidcIdTokenTest.php index 73932326694..0eda9e937f9 100644 --- a/tests/Unit/OidcIdTokenTest.php +++ b/tests/Unit/OidcIdTokenTest.php @@ -15,7 +15,7 @@ public function test_valid_token_passes_validation() OidcJwtHelper::publicJwkKeyArray(), ]); - $this->assertTrue($token->validate('xxyyzz.aaa.bbccdd.123')); + $this->assertTrue($token->validate(OidcJwtHelper::defaultProviderSettings())); } public function test_get_claim_returns_value_if_existing() @@ -56,7 +56,7 @@ public function test_token_structure_error_cases() $err = null; try { - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } catch (\Exception $exception) { $err = $exception; } @@ -71,7 +71,7 @@ public function test_error_thrown_if_token_signature_not_validated_from_no_keys( $token = new OidcIdToken(OidcJwtHelper::idToken(), OidcJwtHelper::defaultIssuer(), []); $this->expectException(OidcInvalidTokenException::class); $this->expectExceptionMessage('Token signature could not be validated using the provided keys'); - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } public function test_error_thrown_if_token_signature_not_validated_from_non_matching_key() @@ -83,7 +83,7 @@ public function test_error_thrown_if_token_signature_not_validated_from_non_matc ]); $this->expectException(OidcInvalidTokenException::class); $this->expectExceptionMessage('Token signature could not be validated using the provided keys'); - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } public function test_error_thrown_if_invalid_key_provided() @@ -91,15 +91,34 @@ public function test_error_thrown_if_invalid_key_provided() $token = new OidcIdToken(OidcJwtHelper::idToken(), OidcJwtHelper::defaultIssuer(), ['url://example.com']); $this->expectException(OidcInvalidTokenException::class); $this->expectExceptionMessage('Unexpected type of key value provided'); - $token->validate('abc'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } - public function test_error_thrown_if_token_algorithm_is_not_rs256() + public function test_error_thrown_if_token_algorithm_is_not_supported() { - $token = new OidcIdToken(OidcJwtHelper::idToken([], ['alg' => 'HS256']), OidcJwtHelper::defaultIssuer(), []); + $token = new OidcIdToken(OidcJwtHelper::idToken([], ['alg' => 'ES256']), OidcJwtHelper::defaultIssuer(), []); $this->expectException(OidcInvalidTokenException::class); - $this->expectExceptionMessage('Only RS256 signature validation is supported. Token reports using HS256'); - $token->validate('abc'); + $this->expectExceptionMessage('Only HS256, RS256 signatures validation are supported. Token reports using ES256'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); + } + + public function test_hs256_token_passes_validation_with_correct_secret() + { + $secret = OidcJwtHelper::defaultSecret(); + $token = new OidcIdToken(OidcJwtHelper::hs256IdToken($secret), OidcJwtHelper::defaultIssuer(), []); + $settings = OidcJwtHelper::defaultProviderSettings(['clientSecret' => $secret]); + + $this->assertTrue($token->validate($settings)); + } + + public function test_hs256_token_fails_validation_with_wrong_secret() + { + $token = new OidcIdToken(OidcJwtHelper::hs256IdToken('correct-secret'), OidcJwtHelper::defaultIssuer(), []); + $settings = OidcJwtHelper::defaultProviderSettings(['clientSecret' => 'wrong-secret']); + + $this->expectException(OidcInvalidTokenException::class); + $this->expectExceptionMessage('Token signature could not be validated using the provided secret'); + $token->validate($settings); } public function test_token_claim_error_cases() @@ -141,7 +160,7 @@ public function test_token_claim_error_cases() $err = null; try { - $token->validate('xxyyzz.aaa.bbccdd.123'); + $token->validate(OidcJwtHelper::defaultProviderSettings()); } catch (\Exception $exception) { $err = $exception; } @@ -160,7 +179,7 @@ public function test_keys_can_be_a_local_file_reference_to_pem_key() $testFilePath, ]); - $this->assertTrue($token->validate('xxyyzz.aaa.bbccdd.123')); + $this->assertTrue($token->validate(OidcJwtHelper::defaultProviderSettings())); unlink($testFilePath); } }