Skip to content

Fix open redirect vulnerabilities in redirect parameter handling #2963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 4 commits into
base: 4.1
Choose a base branch
from
+122 −5
Draft

Fix open redirect vulnerabilities in redirect parameter handling #2963

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e9d141
Initial plan
Copilot Jan 29, 2026
94a7985
Fix open redirect vulnerabilities by validating _redirect parameter
Copilot Jan 29, 2026
e314dd5
Improve validateRedirectUrl to reject dangerous schemes and special c…
Copilot Jan 29, 2026
30441bd
Extract validateRedirectUrl into RedirectUrlValidationTrait to avoid …
Copilot Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/CoreShop/Bundle/FrontendBundle/Controller/CartController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));

Expand All @@ -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);

Expand Down
8 changes: 7 additions & 1 deletion src/CoreShop/Bundle/FrontendBundle/Controller/CustomerController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
);
}
}
Expand Down
61 changes: 61 additions & 0 deletions src/CoreShop/Bundle/FrontendBundle/Controller/FrontendController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,65 @@ 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
Copy link
Member

@dpfaffenbauer dpfaffenbauer Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot feels a bit weird that symfony doesn't provide anything like this?

Copy link
Member

@dpfaffenbauer dpfaffenbauer Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot and you also duplicated it

Copy link
Contributor Author

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symfony's HttpUtils class handles authentication-related redirects internally but doesn't expose a public method for general open redirect validation. The Security component's TargetPathTrait stores/retrieves redirect paths but doesn't validate them.

For arbitrary redirect parameters like _redirect, there's no built-in Symfony utility - custom validation is the standard approach. OWASP and Symfony security best practices recommend validating that redirects are either relative paths or same-origin URLs, which is what this implementation does.

Copy link
Contributor Author

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted the method into RedirectUrlValidationTrait in ResourceBundle/Controller. Both FrontendController and StorageListController now use this trait. (30441bd)

{
// 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;
}
}
7 changes: 6 additions & 1 deletion src/CoreShop/Bundle/FrontendBundle/Controller/RegisterController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
68 changes: 67 additions & 1 deletion src/CoreShop/Bundle/StorageListBundle/Controller/StorageListController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -289,4 +294,65 @@ 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;
}
}