From 8e9d141f5c9b30e8271e2637134ae237db1f6faa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 11:44:18 +0000 Subject: [PATCH 1/4] Initial plan From 94a7985c0622f189f0672addf8a2ddc1ceabafd7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 11:56:08 +0000 Subject: [PATCH 2/4] Fix open redirect vulnerabilities by validating _redirect parameter Co-authored-by: dpfaffenbauer <5981845+dpfaffenbauer@users.noreply.github.com> --- .../Controller/CartController.php | 14 ++++- .../Controller/CustomerController.php | 8 ++- .../Controller/FrontendController.php | 46 ++++++++++++++++ .../Controller/RegisterController.php | 7 ++- .../Controller/StorageListController.php | 53 ++++++++++++++++++- 5 files changed, 123 insertions(+), 5 deletions(-) diff --git a/src/CoreShop/Bundle/FrontendBundle/Controller/CartController.php b/src/CoreShop/Bundle/FrontendBundle/Controller/CartController.php index c0ee90d66f..54014adf7a 100644 --- a/src/CoreShop/Bundle/FrontendBundle/Controller/CartController.php +++ b/src/CoreShop/Bundle/FrontendBundle/Controller/CartController.php @@ -226,7 +226,12 @@ public function addItemAction(Request $request): Response $this->denyAccessUnlessGranted('CORESHOP_CART'); $this->denyAccessUnlessGranted('CORESHOP_CART_ADD_ITEM'); - $redirect = $this->getParameterFromRequest($request, '_redirect', $this->generateUrl('coreshop_index')); + $defaultRedirectGet = $this->generateUrl('coreshop_index'); + $redirect = $this->validateRedirectUrl( + $request, + (string) $this->getParameterFromRequest($request, '_redirect', $defaultRedirectGet), + $defaultRedirectGet, + ); $product = $this->container->get('coreshop.repository.stack.purchasable')->find($this->getParameterFromRequest($request, 'product')); @@ -249,7 +254,12 @@ public function addItemAction(Request $request): Response $form = $this->container->get('form.factory')->createNamed('coreshop-' . $product->getId(), AddToCartType::class, $addToCart); if ($request->isMethod('POST')) { - $redirect = $this->getParameterFromRequest($request, '_redirect', $this->generateUrl('coreshop_cart_summary')); + $defaultRedirectPost = $this->generateUrl('coreshop_cart_summary'); + $redirect = $this->validateRedirectUrl( + $request, + (string) $this->getParameterFromRequest($request, '_redirect', $defaultRedirectPost), + $defaultRedirectPost, + ); $form->handleRequest($request); diff --git a/src/CoreShop/Bundle/FrontendBundle/Controller/CustomerController.php b/src/CoreShop/Bundle/FrontendBundle/Controller/CustomerController.php index 05e663e091..db8b01d105 100644 --- a/src/CoreShop/Bundle/FrontendBundle/Controller/CustomerController.php +++ b/src/CoreShop/Bundle/FrontendBundle/Controller/CustomerController.php @@ -194,8 +194,14 @@ public function addressAction(Request $request): Response $this->fireEvent($request, $address, sprintf('%s.%s.%s_post', 'coreshop', 'address', $eventType)); $this->addFlash('success', $this->container->get('translator')->trans(sprintf('coreshop.ui.customer.address_successfully_%s', $eventType === 'add' ? 'added' : 'updated'))); + $defaultRedirect = $this->generateUrl('coreshop_customer_addresses'); + return $this->redirect( - $this->getParameterFromRequest($request, '_redirect', $this->generateUrl('coreshop_customer_addresses')), + $this->validateRedirectUrl( + $request, + (string) $this->getParameterFromRequest($request, '_redirect', $defaultRedirect), + $defaultRedirect, + ), ); } } diff --git a/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php b/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php index 8b067b0058..62528c7ae4 100644 --- a/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php +++ b/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php @@ -73,4 +73,50 @@ protected function getParameterFromRequest(Request $request, string $key, $defau return $default; } + + /** + * Validates a redirect URL to prevent open redirects. + * + * Only allows: + * - Relative URLs (starting with "/" but not "//") + * - URLs on the same host as the current request + * + * @param Request $request The current request to validate against + * @param string $url The URL to validate + * @param string $default The default URL to return if validation fails + * + * @return string The validated URL or the default if invalid + */ + protected function validateRedirectUrl(Request $request, string $url, string $default): string + { + // Empty URL, use default + if ('' === $url) { + return $default; + } + + // Check for protocol-relative URLs (//example.com) which could be used for open redirects + if (str_starts_with($url, '//')) { + return $default; + } + + // Relative URLs (starting with /) are safe + if (str_starts_with($url, '/')) { + return $url; + } + + // For absolute URLs, verify the host matches the current request + $parsedUrl = parse_url($url); + + // If parsing failed or no host is present, use default + if (false === $parsedUrl || !isset($parsedUrl['host'])) { + return $default; + } + + // Check if the host matches the current request host + if (strtolower($parsedUrl['host']) === strtolower($request->getHost())) { + return $url; + } + + return $default; + } } diff --git a/src/CoreShop/Bundle/FrontendBundle/Controller/RegisterController.php b/src/CoreShop/Bundle/FrontendBundle/Controller/RegisterController.php index 3db01284a2..caca6b4dc4 100644 --- a/src/CoreShop/Bundle/FrontendBundle/Controller/RegisterController.php +++ b/src/CoreShop/Bundle/FrontendBundle/Controller/RegisterController.php @@ -50,7 +50,12 @@ public function registerAction(Request $request): Response $form = $this->container->get('form.factory')->createNamed('customer', CustomerRegistrationType::class, $this->container->get('coreshop.factory.customer')->createNew()); - $redirect = $this->getParameterFromRequest($request, '_redirect', $this->generateUrl('coreshop_customer_profile')); + $defaultRedirect = $this->generateUrl('coreshop_customer_profile'); + $redirect = $this->validateRedirectUrl( + $request, + (string) $this->getParameterFromRequest($request, '_redirect', $defaultRedirect), + $defaultRedirect, + ); if (in_array($request->getMethod(), ['POST', 'PUT', 'PATCH'], true)) { $form = $form->handleRequest($request); diff --git a/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php b/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php index 2c836cba52..220f9455cb 100644 --- a/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php +++ b/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php @@ -85,7 +85,12 @@ public function addItemAction(Request $request): Response $this->denyAccessUnlessGranted($privilege); $this->denyAccessUnlessGranted($privilegeAdd); - $redirect = $this->getParameterFromRequest($request, '_redirect', $this->generateUrl($this->summaryRoute)); + $defaultRedirect = $this->generateUrl($this->summaryRoute); + $redirect = $this->validateRedirectUrl( + $request, + (string) $this->getParameterFromRequest($request, '_redirect', $defaultRedirect), + $defaultRedirect, + ); $product = $this->productRepository->find($this->getParameterFromRequest($request, 'product')); $storageList = $this->context->getStorageList(); @@ -289,4 +294,50 @@ protected function getParameterFromRequest(Request $request, string $key, $defau return $default; } + + /** + * Validates a redirect URL to prevent open redirects. + * + * Only allows: + * - Relative URLs (starting with "/" but not "//") + * - URLs on the same host as the current request + * + * @param Request $request The current request to validate against + * @param string $url The URL to validate + * @param string $default The default URL to return if validation fails + * + * @return string The validated URL or the default if invalid + */ + protected function validateRedirectUrl(Request $request, string $url, string $default): string + { + // Empty URL, use default + if ('' === $url) { + return $default; + } + + // Check for protocol-relative URLs (//example.com) which could be used for open redirects + if (str_starts_with($url, '//')) { + return $default; + } + + // Relative URLs (starting with /) are safe + if (str_starts_with($url, '/')) { + return $url; + } + + // For absolute URLs, verify the host matches the current request + $parsedUrl = parse_url($url); + + // If parsing failed or no host is present, use default + if (false === $parsedUrl || !isset($parsedUrl['host'])) { + return $default; + } + + // Check if the host matches the current request host + if (strtolower($parsedUrl['host']) === strtolower($request->getHost())) { + return $url; + } + + return $default; + } } From e314dd56d27e250319bdb7a9d1d05e1fb79b599c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 11:58:56 +0000 Subject: [PATCH 3/4] Improve validateRedirectUrl to reject dangerous schemes and special characters Co-authored-by: dpfaffenbauer <5981845+dpfaffenbauer@users.noreply.github.com> --- .../Controller/FrontendController.php | 19 +++++++++++++++++-- .../Controller/StorageListController.php | 19 +++++++++++++++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php b/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php index 62528c7ae4..dcbeccf147 100644 --- a/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php +++ b/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php @@ -79,7 +79,7 @@ protected function getParameterFromRequest(Request $request, string $key, $defau * * Only allows: * - Relative URLs (starting with "/" but not "//") - * - URLs on the same host as the current request + * - URLs on the same host as the current request with http/https scheme * * @param Request $request The current request to validate against * @param string $url The URL to validate @@ -99,8 +99,13 @@ protected function validateRedirectUrl(Request $request, string $url, string $de return $default; } - // Relative URLs (starting with /) are safe + // Relative URLs (starting with /) are safe if they don't contain dangerous characters if (str_starts_with($url, '/')) { + // Reject URLs with backslashes, control characters, or whitespace that could be misinterpreted + if (preg_match('/[\\\\\\x00-\\x1f\\x7f]/', $url)) { + return $default; + } + return $url; } @@ -112,6 +117,16 @@ protected function validateRedirectUrl(Request $request, string $url, string $de return $default; } + // Only allow http and https schemes + if (isset($parsedUrl['scheme']) && !in_array(strtolower($parsedUrl['scheme']), ['http', 'https'], true)) { + return $default; + } + + // Reject URLs with @ in the authority component (user:pass@host manipulation) + if (isset($parsedUrl['user']) || str_contains($url, '@')) { + return $default; + } + // Check if the host matches the current request host if (strtolower($parsedUrl['host']) === strtolower($request->getHost())) { return $url; diff --git a/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php b/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php index 220f9455cb..a82b3d7c0d 100644 --- a/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php +++ b/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php @@ -300,7 +300,7 @@ protected function getParameterFromRequest(Request $request, string $key, $defau * * Only allows: * - Relative URLs (starting with "/" but not "//") - * - URLs on the same host as the current request + * - URLs on the same host as the current request with http/https scheme * * @param Request $request The current request to validate against * @param string $url The URL to validate @@ -320,8 +320,13 @@ protected function validateRedirectUrl(Request $request, string $url, string $de return $default; } - // Relative URLs (starting with /) are safe + // Relative URLs (starting with /) are safe if they don't contain dangerous characters if (str_starts_with($url, '/')) { + // Reject URLs with backslashes, control characters, or whitespace that could be misinterpreted + if (preg_match('/[\\\\\\x00-\\x1f\\x7f]/', $url)) { + return $default; + } + return $url; } @@ -333,6 +338,16 @@ protected function validateRedirectUrl(Request $request, string $url, string $de return $default; } + // Only allow http and https schemes + if (isset($parsedUrl['scheme']) && !in_array(strtolower($parsedUrl['scheme']), ['http', 'https'], true)) { + return $default; + } + + // Reject URLs with @ in the authority component (user:pass@host manipulation) + if (isset($parsedUrl['user']) || str_contains($url, '@')) { + return $default; + } + // Check if the host matches the current request host if (strtolower($parsedUrl['host']) === strtolower($request->getHost())) { return $url; From 30441bd7509ae86aefcfac7555b2119c2b3f82df Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 19:35:50 +0000 Subject: [PATCH 4/4] Extract validateRedirectUrl into RedirectUrlValidationTrait to avoid duplication Co-authored-by: dpfaffenbauer <5981845+dpfaffenbauer@users.noreply.github.com> --- .../Controller/FrontendController.php | 64 +------------- .../Controller/RedirectUrlValidationTrait.php | 85 +++++++++++++++++++ .../Controller/StorageListController.php | 64 +------------- 3 files changed, 91 insertions(+), 122 deletions(-) create mode 100644 src/CoreShop/Bundle/ResourceBundle/Controller/RedirectUrlValidationTrait.php diff --git a/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php b/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php index dcbeccf147..c75859e3e7 100644 --- a/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php +++ b/src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php @@ -19,6 +19,7 @@ namespace CoreShop\Bundle\FrontendBundle\Controller; use CoreShop\Bundle\FrontendBundle\TemplateConfigurator\TemplateConfiguratorInterface; +use CoreShop\Bundle\ResourceBundle\Controller\RedirectUrlValidationTrait; use CoreShop\Component\Core\Context\ShopperContextInterface; use CoreShop\Component\Order\Context\CartContextInterface; use CoreShop\Component\SEO\SEOPresentationInterface; @@ -29,6 +30,8 @@ abstract class FrontendController extends AbstractController { + use RedirectUrlValidationTrait; + public function __construct( \Psr\Container\ContainerInterface $container, ) { @@ -73,65 +76,4 @@ protected function getParameterFromRequest(Request $request, string $key, $defau return $default; } - - /** - * Validates a redirect URL to prevent open redirects. - * - * Only allows: - * - Relative URLs (starting with "/" but not "//") - * - URLs on the same host as the current request with http/https scheme - * - * @param Request $request The current request to validate against - * @param string $url The URL to validate - * @param string $default The default URL to return if validation fails - * - * @return string The validated URL or the default if invalid - */ - protected function validateRedirectUrl(Request $request, string $url, string $default): string - { - // Empty URL, use default - if ('' === $url) { - return $default; - } - - // Check for protocol-relative URLs (//example.com) which could be used for open redirects - if (str_starts_with($url, '//')) { - return $default; - } - - // Relative URLs (starting with /) are safe if they don't contain dangerous characters - if (str_starts_with($url, '/')) { - // Reject URLs with backslashes, control characters, or whitespace that could be misinterpreted - if (preg_match('/[\\\\\\x00-\\x1f\\x7f]/', $url)) { - return $default; - } - - return $url; - } - - // For absolute URLs, verify the host matches the current request - $parsedUrl = parse_url($url); - - // If parsing failed or no host is present, use default - if (false === $parsedUrl || !isset($parsedUrl['host'])) { - return $default; - } - - // Only allow http and https schemes - if (isset($parsedUrl['scheme']) && !in_array(strtolower($parsedUrl['scheme']), ['http', 'https'], true)) { - return $default; - } - - // Reject URLs with @ in the authority component (user:pass@host manipulation) - if (isset($parsedUrl['user']) || str_contains($url, '@')) { - return $default; - } - - // Check if the host matches the current request host - if (strtolower($parsedUrl['host']) === strtolower($request->getHost())) { - return $url; - } - - return $default; - } } diff --git a/src/CoreShop/Bundle/ResourceBundle/Controller/RedirectUrlValidationTrait.php b/src/CoreShop/Bundle/ResourceBundle/Controller/RedirectUrlValidationTrait.php new file mode 100644 index 0000000000..e866d84590 --- /dev/null +++ b/src/CoreShop/Bundle/ResourceBundle/Controller/RedirectUrlValidationTrait.php @@ -0,0 +1,85 @@ +getHost())) { + return $url; + } + + return $default; + } +} diff --git a/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php b/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php index a82b3d7c0d..c3cc4f34f1 100644 --- a/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php +++ b/src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php @@ -18,6 +18,7 @@ namespace CoreShop\Bundle\StorageListBundle\Controller; +use CoreShop\Bundle\ResourceBundle\Controller\RedirectUrlValidationTrait; use CoreShop\Component\Resource\Model\ResourceInterface; use CoreShop\Component\Resource\Repository\RepositoryInterface; use CoreShop\Component\StorageList\Context\StorageListContextInterface; @@ -45,6 +46,8 @@ class StorageListController extends AbstractController { + use RedirectUrlValidationTrait; + public function __construct( ContainerInterface $container, protected string $identifier, @@ -294,65 +297,4 @@ protected function getParameterFromRequest(Request $request, string $key, $defau return $default; } - - /** - * Validates a redirect URL to prevent open redirects. - * - * Only allows: - * - Relative URLs (starting with "/" but not "//") - * - URLs on the same host as the current request with http/https scheme - * - * @param Request $request The current request to validate against - * @param string $url The URL to validate - * @param string $default The default URL to return if validation fails - * - * @return string The validated URL or the default if invalid - */ - protected function validateRedirectUrl(Request $request, string $url, string $default): string - { - // Empty URL, use default - if ('' === $url) { - return $default; - } - - // Check for protocol-relative URLs (//example.com) which could be used for open redirects - if (str_starts_with($url, '//')) { - return $default; - } - - // Relative URLs (starting with /) are safe if they don't contain dangerous characters - if (str_starts_with($url, '/')) { - // Reject URLs with backslashes, control characters, or whitespace that could be misinterpreted - if (preg_match('/[\\\\\\x00-\\x1f\\x7f]/', $url)) { - return $default; - } - - return $url; - } - - // For absolute URLs, verify the host matches the current request - $parsedUrl = parse_url($url); - - // If parsing failed or no host is present, use default - if (false === $parsedUrl || !isset($parsedUrl['host'])) { - return $default; - } - - // Only allow http and https schemes - if (isset($parsedUrl['scheme']) && !in_array(strtolower($parsedUrl['scheme']), ['http', 'https'], true)) { - return $default; - } - - // Reject URLs with @ in the authority component (user:pass@host manipulation) - if (isset($parsedUrl['user']) || str_contains($url, '@')) { - return $default; - } - - // Check if the host matches the current request host - if (strtolower($parsedUrl['host']) === strtolower($request->getHost())) { - return $url; - } - - return $default; - } }