Skip to content

Commit

Permalink
Fix BackURL redirect with strict or lax session cookie security.
Browse files Browse the repository at this point in the history
See #55 PR for more context.

This is merely a cherry pick / copy of that commit but for the 2 branch so in theory should work in SS 4.x
  • Loading branch information
jareddreyerss authored and satrun77 committed Feb 28, 2024
1 parent a55be58 commit 43fd2cb
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 1 deletion.
10 changes: 10 additions & 0 deletions src/Control/SAMLController.php
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,16 @@ protected function getRedirect()
return $this->redirect($this->getRequest()->getSession()->get('BackURL'));
}

// In SAMLHelper, we use RelayState to convey BackURL because in a HTTP POST flow
// with lax or strict cookie security the session will not be there for us. RelayState
// will be reflected back in the acs POST request.
// Note if only assertion is signed, RelayState cannot be trusted. Prevent open relay
// as in https://github.com/SAML-Toolkits/php-saml#avoiding-open-redirect-attacks
$relayState = $this->owner->getRequest()->postVar('RelayState');
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'))) {
Expand Down
3 changes: 2 additions & 1 deletion src/Helpers/SAMLHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ public function redirect(RequestHandler $requestHandler = null, HTTPRequest $req
$additionalGetQueryParams = $this->getAdditionalGETQueryParameters();

try {
$auth->login(Director::absoluteBaseURL() . 'saml/', $additionalGetQueryParams);
/** Use RelayState to convey BackURL (will be handled in {@see SAMLController}). */
$auth->login($backURL, $additionalGetQueryParams);
} catch (Exception $e) {
/** @var LoggerInterface $logger */
$logger = Injector::inst()->get(LoggerInterface::class);
Expand Down

0 comments on commit 43fd2cb

Please sign in to comment.