Merge commit from fork

* Support Cas v3 logout and validate logout url

* Remove separate loggedout controller to avoid needing to duplicate url validation logic

* Address PR feedback

Author: pradtke

docker/ssp/config-override.php

@@ -6,6 +6,7 @@
 $config['module.enable']['casserver'] = true;
 // Have preprod warning enabled (though it may not be installed) to ease authproc redirect testing
 $config['module.enable']['preprodwarning'] = true;
+$config['trusted.url.domains'] = [ 'main-config.example.com'];
 $config = [
         'secretsalt' => 'testsalt',
         'logging.level' => 7,

phpstan-baseline.neon

@@ -310,11 +310,6 @@ parameters:
 			count: 1
 			path: src/Controller/LoginController.php
 
-		-
-			message: "#^Property SimpleSAML\\\\Module\\\\casserver\\\\Controller\\\\LogoutController\\:\\:\\$sspConfig is never read, only written\\.$#"
-			count: 1
-			path: src/Controller/LogoutController.php
-
 		-
 			message: "#^Access to an undefined property DOMNode\\:\\:\\$value\\.$#"
 			count: 1

routing/routes/routes.php

@@ -37,8 +37,9 @@
         ->methods(['POST']);
     $routes->add(RoutesEnum::Logout->name, RoutesEnum::Logout->value)
         ->controller([LogoutController::class, 'logout']);
+    // This path was combined into the main logout
     $routes->add(RoutesEnum::LoggedOut->name, RoutesEnum::LoggedOut->value)
-        ->controller([LoggedOutController::class, 'main']);
+        ->controller([LogoutController::class, 'logout']);
     $routes->add(RoutesEnum::Login->name, RoutesEnum::Login->value)
         ->controller([LoginController::class, 'login']);
     $routes->add(RoutesEnum::LoggedIn->name, RoutesEnum::LoggedIn->value)
@@ -61,8 +62,9 @@
         ->methods(['POST']);
     $routes->add(LegacyRoutesEnum::LegacyLogout->name, LegacyRoutesEnum::LegacyLogout->value)
         ->controller([LogoutController::class, 'logout']);
+    // This path was combined into the main logout
     $routes->add(LegacyRoutesEnum::LegacyLoggedOut->name, LegacyRoutesEnum::LegacyLoggedOut->value)
-        ->controller([LoggedOutController::class, 'main']);
+        ->controller([LogoutController::class, 'logout']);
     $routes->add(LegacyRoutesEnum::LegacyLogin->name, LegacyRoutesEnum::LegacyLogin->value)
         ->controller([LoginController::class, 'login']);
     $routes->add(LegacyRoutesEnum::LegacyLoggedIn->name, LegacyRoutesEnum::LegacyLoggedIn->value)

src/Controller/LoggedOutController.php

@@ -11,10 +11,12 @@
 use SimpleSAML\Logger;
 use SimpleSAML\Module;
 use SimpleSAML\Module\casserver\Cas\Factories\TicketFactory;
+use SimpleSAML\Module\casserver\Cas\ServiceValidator;
 use SimpleSAML\Module\casserver\Cas\Ticket\TicketStore;
 use SimpleSAML\Module\casserver\Controller\Traits\UrlTrait;
 use SimpleSAML\Session;
 use SimpleSAML\Utils;
+use SimpleSAML\XHTML\Template;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Attribute\AsController;
 use Symfony\Component\HttpKernel\Attribute\MapQueryParameter;
@@ -41,6 +43,7 @@ class LogoutController
 
     /** @var \SimpleSAML\Module\casserver\Cas\Ticket\TicketStore */
     protected TicketStore $ticketStore;
+    private ServiceValidator $serviceValidator;
 
 
     /**
@@ -63,6 +66,8 @@ public function __construct(
         // argument in order to facilitate testin.
         $this->casConfig = ($casConfig === null || $casConfig === $sspConfig)
             ? Configuration::getConfig('module_casserver.php') : $casConfig;
+        $this->serviceValidator = new ServiceValidator($this->casConfig);
+
         $this->authSource = $source ?? new Simple($this->casConfig->getValue('authsource'));
         $this->httpUtils = $httpUtils ?? new Utils\HTTP();
 
@@ -82,49 +87,68 @@ public function __construct(
      * @param \Symfony\Component\HttpFoundation\Request $request
      * @param string|null $url
      *
-     * @return \SimpleSAML\HTTP\RunnableResponse
+     * @return \SimpleSAML\XHTML\Template|\SimpleSAML\HTTP\RunnableResponse
      */
     public function logout(
         Request $request,
         #[MapQueryParameter] ?string $url = null,
-    ): RunnableResponse {
+        #[MapQueryParameter] ?string $service = null,
+    ): Template|RunnableResponse {
         if (!$this->casConfig->getOptionalValue('enable_logout', false)) {
             $this->handleExceptionThrown('Logout not allowed');
         }
 
-        // Skip Logout Page configuration
-        $skipLogoutPage = $this->casConfig->getOptionalValue('skip_logout_page', false);
-
-        if ($skipLogoutPage && $url === null) {
-            $this->handleExceptionThrown('Required URL query parameter [url] not provided. (CAS Server)');
+        // note: casv3 says to ignore the casv2 url parameter, however deployments will see a mix of cas v2 and
+        // cas v3 clients so we support both.  casv3 makes a query parameter optional
+        $isCasV3 = empty($url);
+        $url = $isCasV3 ? $service : $url;
+
+        // Validate the return $url is valid
+        if (!is_null($url)) {
+            $isValidReturnUrl = !is_null($this->serviceValidator->checkServiceURL($this->sanitize($url)));
+            if (!$isValidReturnUrl) {
+                try {
+                    $url = $this->httpUtils->checkURLAllowed($url);
+                    $isValidReturnUrl = true;
+                } catch (\Exception $e) {
+                    Logger::info('Invalid cas logout url ' . $e->getMessage());
+                    $isValidReturnUrl = false;
+                }
+            }
+            if (!$isValidReturnUrl) {
+                // Protocol does not define behavior if invalid logout url sent
+                // act like no url sent and show logout page
+                Logger::info("Invalid logout url '$url'. Ignoring");
+                $url = null;
+            }
         }
 
-        // Construct the logout redirect url
-        if ($skipLogoutPage) {
-            $logoutRedirectUrl = $url;
-            $params = [];
-        } else {
-            $logoutRedirectUrl = Module::getModuleURL('casserver/loggedOut');
-            $params =  $url === null ? []
-                : ['url' => $url];
-        }
+        // Skip Logout Page configuration
+        $skipLogoutPage = !is_null($url) && ($isCasV3 || $this->casConfig->getOptionalValue('skip_logout_page', false));
+
 
         // Delete the ticket from the session
         $session = $this->getSession();
         if ($session !== null) {
             $this->ticketStore->deleteTicket($session->getSessionId());
         }
 
-        // Redirect
-        if (!$this->authSource->isAuthenticated()) {
-            return new RunnableResponse([$this->httpUtils, 'redirectTrustedURL'], [$logoutRedirectUrl, $params]);
+        if ($this->authSource->isAuthenticated()) {
+            // Logout and come back here to handle the logout
+            return new RunnableResponse(
+                [$this->authSource, 'logout'],
+                [$this->httpUtils->getSelfURL()],
+            );
+        } elseif ($skipLogoutPage) {
+            $params = [];
+            return new RunnableResponse([$this->httpUtils, 'redirectTrustedURL'], [$url, $params]);
+        } else {
+            $t = new Template($this->sspConfig, 'casserver:loggedOut.twig');
+            if ($url) {
+                $t->data['url'] = $url;
+            }
+            return $t;
         }
-
-        // Logout and redirect
-        return new RunnableResponse(
-            [$this->authSource, 'logout'],
-            [$logoutRedirectUrl],
-        );
     }
 
     /**