From bb9e0d0847c92ae86994f134e3cb3ae73aeb3e12 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Wed, 22 Apr 2026 15:52:19 +0200 Subject: [PATCH 1/2] Remove SAML request from URL in wayf Prior to this change, users would bookmark the wayf. This caused expired/old saml requests from being handled by EB. EB does not have issues with that, but the SP might. This change prevents users from bookmarking SAML requests. Resolves #1703 --- .../skeune/wayf/wayf.bookmarkableUrl.spec.js | 22 +++++++++++++++++++ theme/base/javascripts/wayf.js | 3 ++- .../javascripts/wayf/hideBookmarkableUrl.js | 17 ++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/cypress/integration/skeune/wayf/wayf.bookmarkableUrl.spec.js create mode 100644 theme/base/javascripts/wayf/hideBookmarkableUrl.js diff --git a/tests/e2e/cypress/integration/skeune/wayf/wayf.bookmarkableUrl.spec.js b/tests/e2e/cypress/integration/skeune/wayf/wayf.bookmarkableUrl.spec.js new file mode 100644 index 0000000000..4ae816af0f --- /dev/null +++ b/tests/e2e/cypress/integration/skeune/wayf/wayf.bookmarkableUrl.spec.js @@ -0,0 +1,22 @@ +context('hideBookmarkableUrl', () => { + describe('When the URL contains a SAMLRequest parameter', () => { + it('replaces the URL with ?feedback=bookmark', () => { + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?SAMLRequest=somevalue'); + cy.url().should('not.include', 'SAMLRequest'); + cy.url().should('include', 'feedback=bookmark'); + }); + }); + + describe('When the URL does not contain a SAMLRequest parameter', () => { + it('leaves the URL unchanged', () => { + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf'); + cy.url().should('not.include', 'feedback=bookmark'); + }); + + it('leaves other query parameters intact', () => { + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?connectedIdps=5'); + cy.url().should('not.include', 'feedback=bookmark'); + cy.url().should('include', 'connectedIdps=5'); + }); + }); +}); diff --git a/theme/base/javascripts/wayf.js b/theme/base/javascripts/wayf.js index 461f83b3d1..58660d2647 100644 --- a/theme/base/javascripts/wayf.js +++ b/theme/base/javascripts/wayf.js @@ -1,7 +1,8 @@ import {initializePage} from './page'; import {wayfCallbackAfterLoad} from './handlers'; import {wayfPageSelector} from './selectors'; +import {hideBookmarkableUrl} from './wayf/hideBookmarkableUrl'; export function initializeWayf() { - initializePage(wayfPageSelector, wayfCallbackAfterLoad); + initializePage(wayfPageSelector, wayfCallbackAfterLoad, hideBookmarkableUrl); } diff --git a/theme/base/javascripts/wayf/hideBookmarkableUrl.js b/theme/base/javascripts/wayf/hideBookmarkableUrl.js new file mode 100644 index 0000000000..ae3d29c369 --- /dev/null +++ b/theme/base/javascripts/wayf/hideBookmarkableUrl.js @@ -0,0 +1,17 @@ +/** + * Replace SAML query parameters in the URL bar with ?feedback=bookmark to prevent broken bookmarks + */ +export const hideBookmarkableUrl = () => { + if (!window.history || !window.history.replaceState) { + return; + } + + const url = new URL(window.location.href); + if (!url.searchParams.has('SAMLRequest')) { + return; + } + + const cleanUrl = new URL(window.location.pathname, window.location.origin); + cleanUrl.searchParams.set('feedback', 'bookmark'); + window.history.replaceState(null, '', cleanUrl.toString()); +}; From 5ff86618c3630d48fc2b983b62ef5b157d66ffa9 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Thu, 23 Apr 2026 11:52:35 +0200 Subject: [PATCH 2/2] Restore back button flow Prior to this change, when the user would click the wrong IdP in the wayf, then used the back button, an error page would be shown, because the saml request was gone from the url. This change stores the saml request in the session when visiting the wayf. This way, when the wayf is visited without a request, the last used request will be used. Fixes #1973 --- config/reference.php | 14 ++- .../Controller/IdentityProviderController.php | 22 ++++ .../Features/HideBookmarkableUrl.feature | 24 ++++ .../IdentityProviderControllerTest.php | 117 ++++++++++++++++++ 4 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 src/OpenConext/EngineBlockFunctionalTestingBundle/Features/HideBookmarkableUrl.feature create mode 100644 tests/unit/OpenConext/EngineBlockBundle/Controller/IdentityProviderControllerTest.php diff --git a/config/reference.php b/config/reference.php index 0fcf803aa0..c7a371fb94 100644 --- a/config/reference.php +++ b/config/reference.php @@ -1466,8 +1466,6 @@ * monolog?: MonologConfig, * doctrine?: DoctrineConfig, * doctrine_migrations?: DoctrineMigrationsConfig, - * debug?: DebugConfig, - * web_profiler?: WebProfilerConfig, * "when@ci"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, @@ -1494,6 +1492,17 @@ * debug?: DebugConfig, * web_profiler?: WebProfilerConfig, * }, + * "when@prod"?: array{ + * imports?: ImportsConfig, + * parameters?: ParametersConfig, + * services?: ServicesConfig, + * framework?: FrameworkConfig, + * security?: SecurityConfig, + * twig?: TwigConfig, + * monolog?: MonologConfig, + * doctrine?: DoctrineConfig, + * doctrine_migrations?: DoctrineMigrationsConfig, + * }, * "when@test"?: array{ * imports?: ImportsConfig, * parameters?: ParametersConfig, @@ -1592,6 +1601,7 @@ public static function config(array $config): array * @psalm-type RoutesConfig = array{ * "when@ci"?: array, * "when@dev"?: array, + * "when@prod"?: array, * "when@test"?: array, * ... * } diff --git a/src/OpenConext/EngineBlockBundle/Controller/IdentityProviderController.php b/src/OpenConext/EngineBlockBundle/Controller/IdentityProviderController.php index 57e10014fb..7777078ee8 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/IdentityProviderController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/IdentityProviderController.php @@ -26,6 +26,7 @@ use OpenConext\EngineBlockBridge\ResponseFactory; use OpenConext\EngineBlockBundle\Configuration\FeatureConfigurationInterface; use Psr\Log\LoggerInterface; +use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; @@ -122,9 +123,16 @@ public function __construct( #[Route(path: '/authentication/idp/single-sign-on/{idpHash}', name: 'authentication_idp_sso_idphash', methods: ['GET', 'POST'])] public function singleSignOnAction(Request $request, ?string $keyId = null, ?string $idpHash = null) { + $storedRequest = $this->getStoredSessionRequest($request); + if ($storedRequest !== null) { + return new RedirectResponse($storedRequest); + } + $this->requestValidator->isValid($request); $this->bindingValidator->isValid($request); + $this->storeRequestToSession($request); + $cortoAdapter = new EngineBlock_Corto_Adapter(); if ($keyId !== null) { @@ -136,6 +144,20 @@ public function singleSignOnAction(Request $request, ?string $keyId = null, ?str return ResponseFactory::fromEngineBlockResponse($this->engineBlockApplicationSingleton->getHttpResponse()); } + private function storeRequestToSession(Request $request): void + { + $request->getSession()->set('eb_last_sso_request_url', $request->getUri()); + } + + private function getStoredSessionRequest(Request $request): ?string + { + if (!$request->isMethod('GET') || $request->query->has('SAMLRequest')) { + return null; + } + + return $request->getSession()->remove('eb_last_sso_request_url'); + } + /** * @return \Symfony\Component\HttpFoundation\RedirectResponse|\Symfony\Component\HttpFoundation\Response * @throws NotFoundHttpException If the IdP-initiated flow has been disabled by config diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/HideBookmarkableUrl.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/HideBookmarkableUrl.feature new file mode 100644 index 0000000000..d9e6fd7f6c --- /dev/null +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/HideBookmarkableUrl.feature @@ -0,0 +1,24 @@ +Feature: + In order to prevent users from saving a broken SAML URL as a bookmark + As EngineBlock + I want to hide the SAMLRequest from the browser's address bar + while still recovering gracefully when the user presses the Back button + + Background: + Given an EngineBlock instance on "dev.openconext.local" + And no registered SPs + And no registered Idps + And an Identity Provider named "Dummy Idp" + And an Identity Provider named "Second Idp" + And a Service Provider named "Dummy SP" + + Scenario: Pressing Back after visiting the WAYF restores the SAML request from the session + When I log in at "Dummy SP" + And I go to Engineblock URL "/authentication/idp/single-sign-on" + Then I should see "Dummy Idp" + + Scenario: A GET to the SSO endpoint without an active session shows an error + When I log in at "Dummy SP" + And I lose my session + And I go to Engineblock URL "/authentication/idp/single-sign-on" + Then I should see "The parameter \"SAMLRequest\" is missing on the SAML SSO request" diff --git a/tests/unit/OpenConext/EngineBlockBundle/Controller/IdentityProviderControllerTest.php b/tests/unit/OpenConext/EngineBlockBundle/Controller/IdentityProviderControllerTest.php new file mode 100644 index 0000000000..2cc28fa539 --- /dev/null +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/IdentityProviderControllerTest.php @@ -0,0 +1,117 @@ +controller = new IdentityProviderController( + Mockery::mock(EngineBlock_ApplicationSingleton::class), + Mockery::mock(Environment::class), + Mockery::mock(\Psr\Log\LoggerInterface::class), + Mockery::mock(RequestAccessMailer::class), + Mockery::mock(RequestValidator::class), + Mockery::mock(RequestValidator::class), + Mockery::mock(RequestValidator::class), + Mockery::mock(AuthenticationStateHelperInterface::class), + Mockery::mock(FeatureConfigurationInterface::class) + ); + } + + #[Test] + public function a_get_without_saml_request_redirects_to_the_stored_session_url(): void + { + $session = new Session(new MockArraySessionStorage()); + $session->set('eb_last_sso_request_url', 'https://engine.example.com/authentication/idp/single-sign-on?SAMLRequest=abc'); + + $request = Request::create('https://engine.example.com/authentication/idp/single-sign-on'); + $request->setSession($session); + + $response = $this->controller->singleSignOnAction($request); + + $this->assertInstanceOf(RedirectResponse::class, $response); + $this->assertSame( + 'https://engine.example.com/authentication/idp/single-sign-on?SAMLRequest=abc', + $response->getTargetUrl() + ); + } + + #[Test] + public function the_stored_session_url_is_consumed_on_redirect_so_it_cannot_be_reused(): void + { + $session = new Session(new MockArraySessionStorage()); + $session->set('eb_last_sso_request_url', 'https://engine.example.com/authentication/idp/single-sign-on?SAMLRequest=abc'); + + $request = Request::create('https://engine.example.com/authentication/idp/single-sign-on'); + $request->setSession($session); + + $this->controller->singleSignOnAction($request); + + $this->assertNull($session->get('eb_last_sso_request_url')); + } + + #[Test] + public function a_get_without_saml_request_and_no_stored_session_url_throws_missing_parameter_exception(): void + { + $this->expectException(MissingParameterException::class); + + $requestValidator = Mockery::mock(RequestValidator::class); + $requestValidator->shouldReceive('isValid')->andThrow(new MissingParameterException('The parameter "SAMLRequest" is missing')); + + $controller = new IdentityProviderController( + Mockery::mock(EngineBlock_ApplicationSingleton::class), + Mockery::mock(Environment::class), + Mockery::mock(\Psr\Log\LoggerInterface::class), + Mockery::mock(RequestAccessMailer::class), + $requestValidator, + Mockery::mock(RequestValidator::class), + Mockery::mock(RequestValidator::class), + Mockery::mock(AuthenticationStateHelperInterface::class), + Mockery::mock(FeatureConfigurationInterface::class) + ); + + $session = new Session(new MockArraySessionStorage()); + $request = Request::create('https://engine.example.com/authentication/idp/single-sign-on'); + $request->setSession($session); + + $controller->singleSignOnAction($request); + } +}