diff --git a/src/Cas/ServiceValidator.php b/src/Cas/ServiceValidator.php index f6dd8897..ed7c79e6 100644 --- a/src/Cas/ServiceValidator.php +++ b/src/Cas/ServiceValidator.php @@ -29,53 +29,70 @@ public function __construct(Configuration $mainConfig) /** * Check that the $service is allowed, and if so return the configuration to use. - * @param string $service The service url. Assume to already be url decoded + * + * @param string $service The service url. Assume to already be url decoded + * * @return Configuration|null Return the configuration to use for this service, or null if service is not allowed + * @throws \ErrorException */ public function checkServiceURL(string $service): ?Configuration { $isValidService = false; $legalUrl = 'undefined'; $configOverride = null; - foreach ($this->mainConfig->getOptionalArray('legal_service_urls', []) as $index => $value) { + $legalServiceUrlsConfig = $this->mainConfig->getOptionalArray('legal_service_urls', []); + + foreach ($legalServiceUrlsConfig as $index => $value) { // Support two styles: 0 => 'https://example' and 'https://example' => [ extra config ] - if (is_int($index)) { - $legalUrl = $value; - $configOverride = null; - } else { - $legalUrl = $index; - $configOverride = $value; - } + $legalUrl = \is_int($index) ? $value : $index; if (empty($legalUrl)) { Logger::warning("Ignoring empty CAS legal service url '$legalUrl'."); continue; } - if (!ctype_alnum($legalUrl[0])) { - // Probably a regex. Suppress errors incase the format is invalid - $result = @preg_match($legalUrl, $service); - if ($result === 1) { - $isValidService = true; - break; - } elseif ($result === false) { - Logger::warning("Invalid CAS legal service url '$legalUrl'. Error " . preg_last_error()); + + $configOverride = \is_int($index) ? null : $value; + + // URL String + if (str_starts_with($service, $legalUrl)) { + $isValidService = true; + break; + } + + // Regex + // Since "If the regex pattern passed does not compile to a valid regex, an E_WARNING is emitted. " + // we will throw an exception if the warning is emitted and use try-catch to handle it + set_error_handler(static function ($severity, $message, $file, $line) { + throw new \ErrorException($message, $severity, $severity, $file, $line); + }, E_WARNING); + + try { + $result = preg_match($legalUrl, $service); + if ($result !== 1) { + throw new \RuntimeException('Service URL does not match legal service URL.'); } - } elseif (strpos($service, $legalUrl) === 0) { $isValidService = true; break; + } catch (\Exception $e) { + // do nothing + Logger::warning("Invalid CAS legal service url '$legalUrl'. Error " . preg_last_error()); + } finally { + restore_error_handler(); } } - if ($isValidService) { - $serviceConfig = $this->mainConfig->toArray(); - // Return contextual information about which url rule triggered the validation - $serviceConfig['casService'] = [ - 'matchingUrl' => $legalUrl, - 'serviceUrl' => $service, - ]; - if ($configOverride) { - $serviceConfig = array_merge($serviceConfig, $configOverride); - } - return Configuration::loadFromArray($serviceConfig); + + if (!$isValidService) { + return null; + } + + $serviceConfig = $this->mainConfig->toArray(); + // Return contextual information about which url rule triggered the validation + $serviceConfig['casService'] = [ + 'matchingUrl' => $legalUrl, + 'serviceUrl' => $service, + ]; + if ($configOverride) { + $serviceConfig = array_merge($serviceConfig, $configOverride); } - return null; + return Configuration::loadFromArray($serviceConfig); } } diff --git a/src/Cas/Ticket/SQLTicketStore.php b/src/Cas/Ticket/SQLTicketStore.php index 5ab0d844..0603ed8c 100644 --- a/src/Cas/Ticket/SQLTicketStore.php +++ b/src/Cas/Ticket/SQLTicketStore.php @@ -321,9 +321,11 @@ private function get(string $key): ?array /** - * @param string $key - * @param array $value - * @param int|null $expire + * @param string $key + * @param array $value + * @param int|null $expire + * + * @throws Exception */ private function set(string $key, array $value, int $expire = null): void { diff --git a/src/Controller/Cas10Controller.php b/src/Controller/Cas10Controller.php index 0de3e102..81c7f4c7 100644 --- a/src/Controller/Cas10Controller.php +++ b/src/Controller/Cas10Controller.php @@ -144,15 +144,8 @@ public function validate( ); } - // Get the username field - $usernameField = $this->casConfig->getOptionalValue('attrname', 'eduPersonPrincipalName'); - // Fail if the username is not present in the ticket if (empty($serviceTicket['userName'])) { - Logger::error( - 'casserver:validate: internal server error. Missing user name attribute: ' - . var_export($usernameField, true), - ); return new Response( $this->cas10Protocol->getValidateFailureResponse(), Response::HTTP_BAD_REQUEST, @@ -169,9 +162,9 @@ public function validate( /** * Used by the unit tests * - * @return mixed + * @return TicketStore */ - public function getTicketStore(): mixed + public function getTicketStore(): TicketStore { return $this->ticketStore; } diff --git a/src/Controller/Cas20Controller.php b/src/Controller/Cas20Controller.php index 3da28a3d..eb903f4e 100644 --- a/src/Controller/Cas20Controller.php +++ b/src/Controller/Cas20Controller.php @@ -88,7 +88,7 @@ public function __construct( */ public function serviceValidate( Request $request, - #[MapQueryParameter] string $TARGET = null, + #[MapQueryParameter] ?string $TARGET = null, #[MapQueryParameter] bool $renew = false, #[MapQueryParameter] ?string $ticket = null, #[MapQueryParameter] ?string $service = null, @@ -124,7 +124,7 @@ public function proxy( $legal_target_service_urls = $this->casConfig->getOptionalValue('legal_target_service_urls', []); // Fail if $message = match (true) { - // targetService pareameter is not defined + // targetService parameter is not defined $targetService === null => 'Missing target service parameter [targetService]', // pgt parameter is not defined $pgt === null => 'Missing proxy granting ticket parameter: [pgt]', @@ -226,9 +226,9 @@ public function proxyValidate( /** * Used by the unit tests * - * @return mixed + * @return TicketStore */ - public function getTicketStore(): mixed + public function getTicketStore(): TicketStore { return $this->ticketStore; } diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 6d5f7316..05bbda62 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -134,13 +134,13 @@ public function login( Request $request, #[MapQueryParameter] bool $renew = false, #[MapQueryParameter] bool $gateway = false, - #[MapQueryParameter] string $service = null, - #[MapQueryParameter] string $TARGET = null, - #[MapQueryParameter] string $scope = null, - #[MapQueryParameter] string $language = null, - #[MapQueryParameter] string $entityId = null, - #[MapQueryParameter] string $debugMode = null, - #[MapQueryParameter] string $method = null, + #[MapQueryParameter] ?string $service = null, + #[MapQueryParameter] ?string $TARGET = null, + #[MapQueryParameter] ?string $scope = null, + #[MapQueryParameter] ?string $language = null, + #[MapQueryParameter] ?string $entityId = null, + #[MapQueryParameter] ?string $debugMode = null, + #[MapQueryParameter] ?string $method = null, ): RedirectResponse|XmlResponse|null { $forceAuthn = $renew; $serviceUrl = $service ?? $TARGET ?? null; @@ -421,9 +421,9 @@ public function getSession(): ?Session } /** - * @return mixed + * @return TicketStore */ - public function getTicketStore(): mixed + public function getTicketStore(): TicketStore { return $this->ticketStore; } diff --git a/src/Controller/LogoutController.php b/src/Controller/LogoutController.php index 11b3ad2b..1ee868dc 100644 --- a/src/Controller/LogoutController.php +++ b/src/Controller/LogoutController.php @@ -127,9 +127,9 @@ public function logout( } /** - * @return mixed + * @return TicketStore */ - public function getTicketStore(): mixed + public function getTicketStore(): TicketStore { return $this->ticketStore; }