From ed14d7942d6b640a4d28940f11aa58e92bbf7b18 Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Wed, 27 Mar 2024 20:44:54 +1300 Subject: [PATCH] Set the ACS base URL (including domain) correctly As a work-around for a problem where ACS was read as e.g. domain/acs instead of domain/path/acs, explicitly setting the path (via the base url) was done. But we need the domain to match the actual loaded domain, not the service provider entity ID. It is important since there is already another major version of this saml module that no changes that would require existing users to make changes to their code be introduced (requiring a major version). This is the reason we're now relying on Director::absoluteBaseUrl instead of letting the SAML library do what it does (in cases where the workaround is not needed). When using Director::absoluteBaseUrl instead of letting the SAML library build it's own interpretation, the use of other Util class options become redundant - they're not used anyway, so there is no point in having any further/alternative options. A hook point is introduced to further manipulate things like that should anyone have the need. --- src/Control/SAMLController.php | 9 ++++++--- src/Services/SAMLConfiguration.php | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Control/SAMLController.php b/src/Control/SAMLController.php index 62f4f39..1999c39 100644 --- a/src/Control/SAMLController.php +++ b/src/Control/SAMLController.php @@ -70,9 +70,12 @@ public function acs() $uniqueErrorId = uniqid('SAML-'); // Force php-saml module to use the current absolute base URL (e.g. https://www.example.com/saml). This avoids - // errors that we otherwise get when having a multi-directory ACS URL like /saml/acs). + // errors that we otherwise get when having a multi-directory ACS URL (like /saml/acs). // See https://github.com/onelogin/php-saml/issues/249 - Utils::setBaseURL(Controller::join_links($auth->getSettings()->getSPData()['entityId'], 'saml')); + Utils::setBaseURL(Controller::join_links(Director::absoluteBaseURL(), 'saml')); + + // Hook point to allow extensions to further modify or unset any of the above base url coersion + $this->extend('onBeforeAcs', $uniqueErrorId); // Attempt to process the SAML response. If there are errors during this, log them and redirect to the generic // error page. Note: This does not necessarily include all SAML errors (e.g. we still need to confirm if the @@ -269,7 +272,7 @@ protected function getRedirect() if ($relayState && Director::is_site_url($relayState)) { return $this->redirect($relayState); } - + // Spoofing attack, redirect to homepage instead of spoofing url if ($this->getRequest()->getSession()->get('BackURL') && !Director::is_site_url($this->getRequest()->getSession()->get('BackURL'))) { diff --git a/src/Services/SAMLConfiguration.php b/src/Services/SAMLConfiguration.php index 209ee00..35076c8 100644 --- a/src/Services/SAMLConfiguration.php +++ b/src/Services/SAMLConfiguration.php @@ -144,6 +144,7 @@ public function asArray() $spEntityId = Injector::inst()->convertServiceProperty($sp['entityId']); $extraAcsBaseUrl = (array)$config->get('extra_acs_base'); $currentBaseUrl = Director::absoluteBaseURL(); + $count = count($extraAcsBaseUrl); $acsBaseUrl = in_array($currentBaseUrl, $extraAcsBaseUrl) ? $currentBaseUrl : $spEntityId; $spX509Cert = Injector::inst()->convertServiceProperty($sp['x509cert']);