From bee58629dc3d6a0eab9a278d68bb2bfdc8b92d8c Mon Sep 17 00:00:00 2001 From: Pascal Thesing Date: Thu, 5 May 2022 09:04:35 +0200 Subject: [PATCH 1/4] Revert "Merge branch 'sw-26689/set-cookie-secure' into '5.7'" This reverts commit 0b03afcc94b0f097c6a2bb7f284800e273d766c4, reversing changes made to 8d20e101375e5167ece90cef166495bf6ed1d935. --- .../Components/CSRFTokenValidator.php | 61 +++++------------- .../Components/CSRFTokenValidatorTest.php | 62 ++----------------- .../_public/src/js/jquery.csrf-protection.js | 11 +++- 3 files changed, 27 insertions(+), 107 deletions(-) diff --git a/engine/Shopware/Components/CSRFTokenValidator.php b/engine/Shopware/Components/CSRFTokenValidator.php index dd4ba8c2ad2..b5f5d07ef1c 100644 --- a/engine/Shopware/Components/CSRFTokenValidator.php +++ b/engine/Shopware/Components/CSRFTokenValidator.php @@ -148,8 +148,6 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) } if ($request->isGet() && !$this->isProtected($controller)) { - $this->updateCsrfTokenIfNotAvailable($request, $response); - return; } @@ -173,55 +171,19 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) public function regenerateToken(Request $request, Response $response): string { - $shop = $this->contextService->getShopContext()->getShop(); - $name = $this->getCsrfName(); - - $token = Random::getAlphanumericString(30); - $this->container->get('session')->set($name, $token); + $context = $this->contextService->getShopContext(); - $response->headers->clearCookie($name); - - /* - * Appending a '/' to the $basePath is not strictly necessary, but it is - * done to all cookie base paths in the - * `themes/Frontend/Bare/frontend/index/index.tpl` template. It's done - * here as well for compatibility reasons. - */ - $response->headers->setCookie(new Cookie( - $name, - $token, - 0, - sprintf('%s/', $shop->getPath() ?: ''), - null, - $shop->getSecure(), - false - )); + $name = self::CSRF_SESSION_KEY . $context->getShop()->getId(); - return $token; - } - - private function updateCsrfTokenIfNotAvailable(Request $request, Response $response): void - { - $name = $this->getCsrfName(); - - if ($this->container->get('session')->has($name)) { - return; + if ($context->getShop()->getParentId() && $this->componentsConfig->get('shareSessionBetweenLanguageShops')) { + $name = self::CSRF_SESSION_KEY . $context->getShop()->getParentId(); } - $this->regenerateToken($request, $response); - } - - private function getCsrfName(): string - { - $shop = $this->contextService->getShopContext()->getShop(); - - $name = self::CSRF_SESSION_KEY . $shop->getId(); - - if ($shop->getParentId() && $this->componentsConfig->get('shareSessionBetweenLanguageShops')) { - $name = self::CSRF_SESSION_KEY . $shop->getParentId(); - } + $token = Random::getAlphanumericString(30); + $this->container->get('session')->set($name, $token); + $response->headers->setCookie(new Cookie($name, $token, 0, '/', null, $request->isSecure(), false)); - return $name; + return $token; } /** @@ -229,7 +191,12 @@ private function getCsrfName(): string */ private function checkRequest(Request $request, Response $response): bool { - $name = $this->getCsrfName(); + $context = $this->contextService->getShopContext(); + $name = self::CSRF_SESSION_KEY . $context->getShop()->getId(); + + if ($context->getShop()->getParentId() && $this->componentsConfig->get('shareSessionBetweenLanguageShops')) { + $name = self::CSRF_SESSION_KEY . $context->getShop()->getParentId(); + } $token = $this->container->get('session')->get($name); diff --git a/tests/Functional/Components/CSRFTokenValidatorTest.php b/tests/Functional/Components/CSRFTokenValidatorTest.php index 98a3a421256..72762d9bd2d 100644 --- a/tests/Functional/Components/CSRFTokenValidatorTest.php +++ b/tests/Functional/Components/CSRFTokenValidatorTest.php @@ -47,14 +47,11 @@ class CSRFTokenValidatorTest extends TestCase public const EXISTING_ACTION_NAME = 'foo'; - private const CSRF_TOKEN_FOR_SHOP_ONE = '__csrf_token-1'; - /** * @before */ public function enableCsrfInFrontend(): void { - $this->getContainer()->get('session')->offsetUnset(self::CSRF_TOKEN_FOR_SHOP_ONE); Utils::hijackProperty($this->getContainer()->get(CSRFTokenValidator::class), 'isEnabledFrontend', true); } @@ -63,7 +60,6 @@ public function enableCsrfInFrontend(): void */ public function disableCsrfInFrontend(): void { - $this->getContainer()->get('session')->offsetUnset(self::CSRF_TOKEN_FOR_SHOP_ONE); Utils::hijackProperty($this->getContainer()->get(CSRFTokenValidator::class), 'isEnabledFrontend', false); } @@ -89,7 +85,7 @@ public function testFrontendTokenIsValid(): void $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - static::assertIsString($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); + static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); static::assertTrue($incomingRequest->getAttribute('isValidated')); } @@ -119,8 +115,8 @@ public function testFrontendTokenValidationThrowsError(): void static::assertInstanceOf(CSRFTokenValidationException::class, $e); } - static::assertIsString($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); - static::assertNotEquals($token, $this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); + static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); + static::assertNotEquals($token, $this->getContainer()->get('session')->get('__csrf_token-1')); } public function testCsrfExceptionIsThrownWhenNoSession(): void @@ -145,7 +141,7 @@ public function testCsrfExceptionIsThrownWhenNoSession(): void static::assertInstanceOf(CSRFTokenValidationException::class, $e); } - static::assertIsString($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); + static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); } public function testCsrfExceptionIsThrownWhenNoRequestCsrfIsSet(): void @@ -171,51 +167,7 @@ public function testCsrfExceptionIsThrownWhenNoRequestCsrfIsSet(): void static::assertInstanceOf(CSRFTokenValidationException::class, $e); } - static::assertIsString($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); - } - - public function testCsrfTokenIsUpdatedIfItIsNotAvailableInTheSessionAndIsGetRequest(): void - { - $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - - static::assertNull($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); - - $controller = new NotProtectionAwareController(); - $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); - $incomingRequest->setMethod('GET'); - $createResponse = new Enlight_Controller_Response_ResponseTestCase(); - $controller->setRequest($incomingRequest); - $controller->setResponse($createResponse); - $enlightEventArgs = new Enlight_Event_EventArgs([ - 'subject' => $controller, - ]); - - $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - - static::assertIsString($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); - } - - public function testCsrfTokenIsNotUpdatedIfItIsNotAvailableInTheSession(): void - { - $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - - static::assertNull($this->getContainer()->get('session')->get(self::CSRF_TOKEN_FOR_SHOP_ONE)); - - $controller = new MockController(); - $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); - $incomingRequest->setMethod('GET'); - $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); - $createResponse = new Enlight_Controller_Response_ResponseTestCase(); - $controller->setRequest($incomingRequest); - $controller->setResponse($createResponse); - $enlightEventArgs = new Enlight_Event_EventArgs([ - 'subject' => $controller, - ]); - - $this->expectException(CSRFTokenValidationException::class); - $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); + static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); } } @@ -228,7 +180,3 @@ public function getCSRFProtectedActions() ]; } } - -class NotProtectionAwareController extends Enlight_Controller_Action -{ -} diff --git a/themes/Frontend/Responsive/frontend/_public/src/js/jquery.csrf-protection.js b/themes/Frontend/Responsive/frontend/_public/src/js/jquery.csrf-protection.js index 2370fa09a3d..4a3029374d0 100644 --- a/themes/Frontend/Responsive/frontend/_public/src/js/jquery.csrf-protection.js +++ b/themes/Frontend/Responsive/frontend/_public/src/js/jquery.csrf-protection.js @@ -174,6 +174,7 @@ $.ajax({ url: window.csrfConfig.generateUrl, success: function(response, status, xhr) { + me.saveToken(xhr.getResponseHeader('x-csrf-token')); $.publish('plugin/swCsrfProtection/requestToken', [me, me.getToken()]); me.afterInit(); } @@ -182,11 +183,15 @@ }, /** - * @deprecated will be removed with v5.8.0 - * + * Save token into a cookie * @param token */ - saveToken: function(token) {}, + saveToken: function(token) { + var me = this, + basePath = window.csrfConfig.basePath || '/'; + + document.cookie = me.storageKey + '=' + token + '; path=' + basePath + ($.isSecure() ? '; secure;' : ''); + }, /** * Initialize the CSRF protection From 01a9d62eafe7c9ddb44a6db225b84f25ced84166 Mon Sep 17 00:00:00 2001 From: Pascal Thesing Date: Thu, 5 May 2022 09:05:04 +0200 Subject: [PATCH 2/4] Revert "Merge branch 'sw-26662/thorw-csrf-exception' into '5.7'" This reverts commit 334db6c52615ff21571a9391492ed781d5071680, reversing changes made to a25fe81b8cae57c03e8cce6dd71def4929d1246b. --- .../Components/CSRFTokenValidator.php | 13 ++--- .../Components/CSRFTokenValidatorTest.php | 53 ------------------- 2 files changed, 5 insertions(+), 61 deletions(-) diff --git a/engine/Shopware/Components/CSRFTokenValidator.php b/engine/Shopware/Components/CSRFTokenValidator.php index b5f5d07ef1c..0ace7f182d9 100644 --- a/engine/Shopware/Components/CSRFTokenValidator.php +++ b/engine/Shopware/Components/CSRFTokenValidator.php @@ -161,7 +161,6 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) } if (!$this->checkRequest($request, $response)) { - $this->regenerateToken($request, $response); throw new CSRFTokenValidationException(sprintf('The provided X-CSRF-Token for path "%s" is invalid. Please go back, reload the page and try again.', $request->getRequestUri())); } @@ -200,17 +199,15 @@ private function checkRequest(Request $request, Response $response): bool $token = $this->container->get('session')->get($name); - if (!\is_string($token)) { - return false; - } - $requestToken = $request->get(self::CSRF_TOKEN_ARGUMENT, $request->headers->get(self::CSRF_TOKEN_HEADER)); - if (!\is_string($requestToken)) { - return false; + $isValidToken = hash_equals($token, $requestToken); + + if (!$isValidToken) { + $this->regenerateToken($request, $response); } - return hash_equals($token, $requestToken); + return $isValidToken; } /** diff --git a/tests/Functional/Components/CSRFTokenValidatorTest.php b/tests/Functional/Components/CSRFTokenValidatorTest.php index 72762d9bd2d..5d0279e757d 100644 --- a/tests/Functional/Components/CSRFTokenValidatorTest.php +++ b/tests/Functional/Components/CSRFTokenValidatorTest.php @@ -85,7 +85,6 @@ public function testFrontendTokenIsValid(): void $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); static::assertTrue($incomingRequest->getAttribute('isValidated')); } @@ -115,60 +114,8 @@ public function testFrontendTokenValidationThrowsError(): void static::assertInstanceOf(CSRFTokenValidationException::class, $e); } - static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); static::assertNotEquals($token, $this->getContainer()->get('session')->get('__csrf_token-1')); } - - public function testCsrfExceptionIsThrownWhenNoSession(): void - { - $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - - $controller = new MockController(); - $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); - $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); - $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); - $incomingRequest->setParam(CSRFTokenValidator::CSRF_TOKEN_ARGUMENT, 'NOT_FITTING'); - $controller->setRequest($incomingRequest); - $controller->setResponse($incomingResponse); - $enlightEventArgs = new Enlight_Event_EventArgs([ - 'subject' => $controller, - ]); - - try { - $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - } catch (Throwable $e) { - static::assertInstanceOf(CSRFTokenValidationException::class, $e); - } - - static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); - } - - public function testCsrfExceptionIsThrownWhenNoRequestCsrfIsSet(): void - { - $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - $createRequest = new Enlight_Controller_Request_RequestTestCase(); - $createResponse = new Enlight_Controller_Response_ResponseTestCase(); - $tokenValidator->regenerateToken($createRequest, $createResponse); - - $controller = new MockController(); - $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); - $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); - $controller->setRequest($incomingRequest); - $controller->setResponse($incomingResponse); - $enlightEventArgs = new Enlight_Event_EventArgs([ - 'subject' => $controller, - ]); - - try { - $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - } catch (Throwable $e) { - static::assertInstanceOf(CSRFTokenValidationException::class, $e); - } - - static::assertNotNull($this->getContainer()->get('session')->get('__csrf_token-1')); - } } class MockController extends Enlight_Controller_Action implements CSRFGetProtectionAware From 6788e7b4488903ebad3af7c183cf2110bcefd580 Mon Sep 17 00:00:00 2001 From: Pascal Thesing Date: Thu, 5 May 2022 09:05:23 +0200 Subject: [PATCH 3/4] Revert "Merge branch 'sw-26662/improve-csrf-protection' into '5.7'" This reverts commit 5ab806d9aef7501cf0bc04b7d6fd214f5308bc6f, reversing changes made to 808aac066e0e14999c19bd1ad85e7d6513050fe5. --- .phpstan.neon | 4 - .../Components/CSRFTokenValidator.php | 82 +++-------- .../Controllers/frontend.xml | 1 - .../DependencyInjection/services.xml | 2 - .../Controllers/Frontend/Csrftoken.php | 16 +-- engine/Shopware/Core/sAdmin.php | 11 +- .../Components/CSRFTokenValidatorTest.php | 129 ------------------ .../Controllers/Frontend/CsrftokenTest.php | 72 ---------- 8 files changed, 26 insertions(+), 291 deletions(-) delete mode 100644 tests/Functional/Components/CSRFTokenValidatorTest.php delete mode 100644 tests/Functional/Controllers/Frontend/CsrftokenTest.php diff --git a/.phpstan.neon b/.phpstan.neon index 7352f106978..47af89e4c5c 100644 --- a/.phpstan.neon +++ b/.phpstan.neon @@ -177,7 +177,3 @@ parameters: - # For testing purposes the constructor of the price struct gets strings message: '#Parameter .* of class Shopware\\Components\\Cart\\Struct\\Price constructor expects float.*string given#' path: tests/Functional/Components/Cart/ProportionalTaxCalculatorTest.php - - - # The Enlight_Response and Enlight_Request interfaces do not extend from Symfony, and therefore we have to find a unified usage pattern - message: '#Parameter .* of method Shopware\\Components\\CSRFTokenValidator\:\:regenerateToken\(\) expects Symfony\\Component\\HttpFoundation\\Request.* given#' - path: engine/Shopware/Core/sAdmin.php diff --git a/engine/Shopware/Components/CSRFTokenValidator.php b/engine/Shopware/Components/CSRFTokenValidator.php index 0ace7f182d9..68de8c08147 100644 --- a/engine/Shopware/Components/CSRFTokenValidator.php +++ b/engine/Shopware/Components/CSRFTokenValidator.php @@ -28,21 +28,12 @@ use Enlight_Controller_Action; use Enlight_Controller_ActionEventArgs as ActionEventArgs; use Enlight_Event_EventArgs; -use Shopware\Bundle\StoreFrontBundle\Service\ContextServiceInterface; use Shopware_Components_Config; use Symfony\Component\DependencyInjection\ContainerInterface; -use Symfony\Component\HttpFoundation\Cookie; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\Response; class CSRFTokenValidator implements SubscriberInterface { - public const CSRF_SESSION_KEY = '__csrf_token-'; - - public const CSRF_TOKEN_ARGUMENT = '__csrf_token'; - - public const CSRF_TOKEN_HEADER = 'X-CSRF-Token'; - /** * @var ContainerInterface */ @@ -51,7 +42,7 @@ class CSRFTokenValidator implements SubscriberInterface /** * @var string */ - private $tokenName = self::CSRF_TOKEN_HEADER; + private $tokenName = 'X-CSRF-Token'; /** * @var bool @@ -63,26 +54,15 @@ class CSRFTokenValidator implements SubscriberInterface */ private $isEnabledBackend; - private ContextServiceInterface $contextService; - - private Shopware_Components_Config $componentsConfig; - /** * @param bool $isEnabledFrontend * @param bool $isEnabledBackend */ - public function __construct( - ContainerInterface $container, - ContextServiceInterface $contextService, - Shopware_Components_Config $componentsConfig, - $isEnabledFrontend = true, - $isEnabledBackend = true - ) { + public function __construct(ContainerInterface $container, $isEnabledFrontend = true, $isEnabledBackend = true) + { $this->container = $container; $this->isEnabledFrontend = (bool) $isEnabledFrontend; $this->isEnabledBackend = (bool) $isEnabledBackend; - $this->contextService = $contextService; - $this->componentsConfig = $componentsConfig; } /** @@ -118,7 +98,7 @@ public function checkBackendTokenValidation(ActionEventArgs $args) $token = $controller->Request()->getHeader($this->tokenName); if (empty($token)) { - $token = $controller->Request()->getParam(self::CSRF_TOKEN_ARGUMENT); + $token = $controller->Request()->getParam('__csrf_token'); } if (!hash_equals($expected, $token)) { @@ -140,7 +120,6 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) /** @var Enlight_Controller_Action $controller */ $controller = $args->getSubject(); $request = $controller->Request(); - $response = $controller->Response(); // do not check internal sub-requests or validated requests if ($request->getAttribute('_isSubrequest') || $request->getAttribute('isValidated')) { @@ -160,7 +139,7 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) return; } - if (!$this->checkRequest($request, $response)) { + if (!$this->checkRequest($request)) { throw new CSRFTokenValidationException(sprintf('The provided X-CSRF-Token for path "%s" is invalid. Please go back, reload the page and try again.', $request->getRequestUri())); } @@ -168,53 +147,34 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) $request->setAttribute('isValidated', true); } - public function regenerateToken(Request $request, Response $response): string - { - $context = $this->contextService->getShopContext(); - - $name = self::CSRF_SESSION_KEY . $context->getShop()->getId(); - - if ($context->getShop()->getParentId() && $this->componentsConfig->get('shareSessionBetweenLanguageShops')) { - $name = self::CSRF_SESSION_KEY . $context->getShop()->getParentId(); - } - - $token = Random::getAlphanumericString(30); - $this->container->get('session')->set($name, $token); - $response->headers->setCookie(new Cookie($name, $token, 0, '/', null, $request->isSecure(), false)); - - return $token; - } - /** - * Check if the submitted CSRF token in cookie or header matches with the token stored in the session + * Check if the submitted CSRF token matches with the token stored in the cookie or header + * + * @return bool */ - private function checkRequest(Request $request, Response $response): bool + private function checkRequest(Request $request) { - $context = $this->contextService->getShopContext(); - $name = self::CSRF_SESSION_KEY . $context->getShop()->getId(); + $context = $this->container->get(\Shopware\Bundle\StoreFrontBundle\Service\ContextServiceInterface::class)->getShopContext(); + $name = '__csrf_token-' . $context->getShop()->getId(); - if ($context->getShop()->getParentId() && $this->componentsConfig->get('shareSessionBetweenLanguageShops')) { - $name = self::CSRF_SESSION_KEY . $context->getShop()->getParentId(); + if ($context->getShop()->getParentId() && $this->container->get(Shopware_Components_Config::class)->get('shareSessionBetweenLanguageShops')) { + $name = '__csrf_token-' . $context->getShop()->getParentId(); } - $token = $this->container->get('session')->get($name); + $token = $request->cookies->get($name); - $requestToken = $request->get(self::CSRF_TOKEN_ARGUMENT, $request->headers->get(self::CSRF_TOKEN_HEADER)); + $requestToken = $request->get('__csrf_token', $request->headers->get('X-CSRF-Token')); - $isValidToken = hash_equals($token, $requestToken); - - if (!$isValidToken) { - $this->regenerateToken($request, $response); - } - - return $isValidToken; + return hash_equals($token, $requestToken); } /** * Check if the controller has opted in for CSRF whitelisting and if the * called action is on the whitelist + * + * @return bool */ - private function isWhitelisted(Enlight_Controller_Action $controller): bool + private function isWhitelisted(Enlight_Controller_Action $controller) { if (!($controller instanceof CSRFWhitelistAware)) { return false; @@ -231,8 +191,10 @@ private function isWhitelisted(Enlight_Controller_Action $controller): bool /** * Check if a controller has opted in for CSRF protection and if the called action * should be protected + * + * @return bool */ - private function isProtected(Enlight_Controller_Action $controller): bool + private function isProtected(Enlight_Controller_Action $controller) { if (!($controller instanceof CSRFGetProtectionAware)) { return false; diff --git a/engine/Shopware/Components/DependencyInjection/Controllers/frontend.xml b/engine/Shopware/Components/DependencyInjection/Controllers/frontend.xml index c62caf74605..a1f6b6af133 100644 --- a/engine/Shopware/Components/DependencyInjection/Controllers/frontend.xml +++ b/engine/Shopware/Components/DependencyInjection/Controllers/frontend.xml @@ -38,7 +38,6 @@ - diff --git a/engine/Shopware/Components/DependencyInjection/services.xml b/engine/Shopware/Components/DependencyInjection/services.xml index 5e70de8c484..442fce25aa9 100644 --- a/engine/Shopware/Components/DependencyInjection/services.xml +++ b/engine/Shopware/Components/DependencyInjection/services.xml @@ -612,8 +612,6 @@ - - %shopware.csrfprotection.frontend% %shopware.csrfprotection.backend% diff --git a/engine/Shopware/Controllers/Frontend/Csrftoken.php b/engine/Shopware/Controllers/Frontend/Csrftoken.php index 97d5334e90b..5c8329ab933 100644 --- a/engine/Shopware/Controllers/Frontend/Csrftoken.php +++ b/engine/Shopware/Controllers/Frontend/Csrftoken.php @@ -22,18 +22,8 @@ * our trademarks remain entirely with us. */ -use Shopware\Components\CSRFTokenValidator; -use Symfony\Component\HttpFoundation\Request; - class Shopware_Controllers_Frontend_Csrftoken extends Enlight_Controller_Action { - private CSRFTokenValidator $CSRFTokenValidator; - - public function __construct(CSRFTokenValidator $CSRFTokenValidator) - { - $this->CSRFTokenValidator = $CSRFTokenValidator; - } - /** * Loads auth and script renderer resource */ @@ -45,10 +35,10 @@ public function preDispatch() /** * Generates a token and fills the cookie and session */ - public function indexAction(Request $request) + public function indexAction() { - $token = $this->CSRFTokenValidator->regenerateToken($request, $this->Response()); + $token = \Shopware\Components\Random::getAlphanumericString(30); - $this->Response()->headers->set(CSRFTokenValidator::CSRF_TOKEN_HEADER, $token); + $this->Response()->headers->set('x-csrf-token', $token); } } diff --git a/engine/Shopware/Core/sAdmin.php b/engine/Shopware/Core/sAdmin.php index 1267ef0a803..6f2584a547f 100644 --- a/engine/Shopware/Core/sAdmin.php +++ b/engine/Shopware/Core/sAdmin.php @@ -39,7 +39,6 @@ use Shopware\Components\Cart\CartPersistServiceInterface; use Shopware\Components\Cart\ConditionalLineItemServiceInterface; use Shopware\Components\Compatibility\LegacyStructConverter; -use Shopware\Components\CSRFTokenValidator; use Shopware\Components\HolidayTableUpdater; use Shopware\Components\Model\ModelManager; use Shopware\Components\Password\Manager; @@ -206,8 +205,6 @@ class sAdmin implements \Enlight_Hook 'payment' => [], ]; - private CSRFTokenValidator $csrfTokenValidator; - public function __construct( Enlight_Components_Db_Adapter_Pdo_Mysql $db = null, Enlight_Event_EventManager $eventManager = null, @@ -222,8 +219,7 @@ public function __construct( EmailValidatorInterface $emailValidator = null, Shopware_Components_Translation $translationComponent = null, Connection $connection = null, - OptInLoginServiceInterface $optInLoginService = null, - CSRFTokenValidator $csrfTokenValidator = null + OptInLoginServiceInterface $optInLoginService = null ) { $this->db = $db ?: Shopware()->Db(); $this->eventManager = $eventManager ?: Shopware()->Events(); @@ -247,7 +243,6 @@ public function __construct( $this->optInLoginService = $optInLoginService ?: Shopware()->Container()->get(OptInLoginService::class); $this->conditionalLineItemService = Shopware()->Container()->get(ConditionalLineItemServiceInterface::class); $this->cartOrderNumberProvider = Shopware()->Container()->get(CartOrderNumberProviderInterface::class); - $this->csrfTokenValidator = $csrfTokenValidator ?: Shopware()->Container()->get(CSRFTokenValidator::class); } /** @@ -718,8 +713,6 @@ public function logout() $this->contextService->initializeContext(); - $this->csrfTokenValidator->regenerateToken($this->front->Request(), $this->front->Response()); - if (!$this->config->get('clearBasketAfterLogout')) { $this->moduleManager->Basket()->sRefreshBasket(); @@ -3277,8 +3270,6 @@ protected function loginUser($getUser, $email, $password, $isPreHashed, $encoder $this->session->offsetSet('sUserId', $userId); $this->session->offsetSet('sNotesQuantity', $this->moduleManager->Basket()->sCountNotes()); - $this->csrfTokenValidator->regenerateToken($this->front->Request(), $this->front->Response()); - if (!$this->sCheckUser()) { return; } diff --git a/tests/Functional/Components/CSRFTokenValidatorTest.php b/tests/Functional/Components/CSRFTokenValidatorTest.php deleted file mode 100644 index 5d0279e757d..00000000000 --- a/tests/Functional/Components/CSRFTokenValidatorTest.php +++ /dev/null @@ -1,129 +0,0 @@ -getContainer()->get(CSRFTokenValidator::class), 'isEnabledFrontend', true); - } - - /** - * @after - */ - public function disableCsrfInFrontend(): void - { - Utils::hijackProperty($this->getContainer()->get(CSRFTokenValidator::class), 'isEnabledFrontend', false); - } - - public function testFrontendTokenIsValid(): void - { - $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - $createRequest = new Enlight_Controller_Request_RequestTestCase(); - $createResponse = new Enlight_Controller_Response_ResponseTestCase(); - - $token = $tokenValidator->regenerateToken($createRequest, $createResponse); - - $controller = new MockController(); - $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); - $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); - $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); - $incomingRequest->setParam(CSRFTokenValidator::CSRF_TOKEN_ARGUMENT, $token); - $controller->setRequest($incomingRequest); - $controller->setResponse($incomingResponse); - $enlightEventArgs = new Enlight_Event_EventArgs([ - 'subject' => $controller, - ]); - - $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - - static::assertTrue($incomingRequest->getAttribute('isValidated')); - } - - public function testFrontendTokenValidationThrowsError(): void - { - $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - $createRequest = new Enlight_Controller_Request_RequestTestCase(); - $createResponse = new Enlight_Controller_Response_ResponseTestCase(); - - $token = $tokenValidator->regenerateToken($createRequest, $createResponse); - - $controller = new MockController(); - $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); - $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); - $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); - $incomingRequest->setParam(CSRFTokenValidator::CSRF_TOKEN_ARGUMENT, 'NOT_FITTING'); - $controller->setRequest($incomingRequest); - $controller->setResponse($incomingResponse); - $enlightEventArgs = new Enlight_Event_EventArgs([ - 'subject' => $controller, - ]); - - try { - $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); - } catch (Throwable $e) { - static::assertInstanceOf(CSRFTokenValidationException::class, $e); - } - - static::assertNotEquals($token, $this->getContainer()->get('session')->get('__csrf_token-1')); - } -} - -class MockController extends Enlight_Controller_Action implements CSRFGetProtectionAware -{ - public function getCSRFProtectedActions() - { - return [ - 'foo', - ]; - } -} diff --git a/tests/Functional/Controllers/Frontend/CsrftokenTest.php b/tests/Functional/Controllers/Frontend/CsrftokenTest.php deleted file mode 100644 index 769bf9d9f1e..00000000000 --- a/tests/Functional/Controllers/Frontend/CsrftokenTest.php +++ /dev/null @@ -1,72 +0,0 @@ -getContainer()->get('shopware_controllers_frontend_csrftoken'); - - $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); - $request = new Enlight_Controller_Request_RequestTestCase(); - $response = new Enlight_Controller_Response_ResponseTestCase(); - $controller->setRequest($request); - $controller->setResponse($response); - $controller->indexAction($request); - - $token = $response->headers->get('x-csrf-token'); - static::assertNotEmpty($token); - $cookies = $response->headers->getCookies(); - $sessionToken = $this->getContainer()->get('session')->get('__csrf_token-1'); - static::assertNotEmpty($sessionToken); - static::assertCount(1, $cookies); - static::assertNotEmpty($response->headers->getCookies()); - - $controller->indexAction($request); - $renewedToken = $response->headers->get('x-csrf-token'); - static::assertNotEmpty($renewedToken); - static::assertNotEquals($token, $renewedToken); - $renewedCookies = $response->headers->getCookies(); - $renewedSessionToken = $this->getContainer()->get('session')->get('__csrf_token-1'); - - static::assertNotEquals($sessionToken, $renewedSessionToken); - static::assertNotEmpty($renewedSessionToken); - static::assertCount(1, $renewedCookies); - static::assertNotEmpty($response->headers->getCookies()); - } -} From 09492de5e99c59d2522dbc9c69ad5bdb1c9f897b Mon Sep 17 00:00:00 2001 From: Pascal Thesing Date: Thu, 5 May 2022 10:03:12 +0200 Subject: [PATCH 4/4] SW-26704 - change csrf token validation --- .../Components/CSRFTokenValidator.php | 85 ++++++++--- .../DependencyInjection/services.xml | 2 + .../Controllers/Frontend/Csrftoken.php | 4 +- engine/Shopware/Core/sAdmin.php | 11 +- .../Components/CSRFTokenValidatorTest.php | 144 ++++++++++++++++++ 5 files changed, 226 insertions(+), 20 deletions(-) create mode 100644 tests/Functional/Components/CSRFTokenValidatorTest.php diff --git a/engine/Shopware/Components/CSRFTokenValidator.php b/engine/Shopware/Components/CSRFTokenValidator.php index 68de8c08147..07abc54d963 100644 --- a/engine/Shopware/Components/CSRFTokenValidator.php +++ b/engine/Shopware/Components/CSRFTokenValidator.php @@ -28,12 +28,22 @@ use Enlight_Controller_Action; use Enlight_Controller_ActionEventArgs as ActionEventArgs; use Enlight_Event_EventArgs; +use Shopware\Bundle\StoreFrontBundle\Service\ContextServiceInterface; use Shopware_Components_Config; use Symfony\Component\DependencyInjection\ContainerInterface; +use Symfony\Component\HttpFoundation\Cookie; use Symfony\Component\HttpFoundation\Request; class CSRFTokenValidator implements SubscriberInterface { + public const CSRF_KEY = '__csrf_token-'; + + public const CSRF_TOKEN_ARGUMENT = '__csrf_token'; + + public const CSRF_TOKEN_HEADER = 'X-CSRF-Token'; + + private const CSRF_WAS_VALIDATED = 'isValidated'; + /** * @var ContainerInterface */ @@ -42,7 +52,7 @@ class CSRFTokenValidator implements SubscriberInterface /** * @var string */ - private $tokenName = 'X-CSRF-Token'; + private $tokenName = self::CSRF_TOKEN_HEADER; /** * @var bool @@ -54,15 +64,26 @@ class CSRFTokenValidator implements SubscriberInterface */ private $isEnabledBackend; + private ContextServiceInterface $contextService; + + private Shopware_Components_Config $componentsConfig; + /** * @param bool $isEnabledFrontend * @param bool $isEnabledBackend */ - public function __construct(ContainerInterface $container, $isEnabledFrontend = true, $isEnabledBackend = true) - { + public function __construct( + ContainerInterface $container, + ContextServiceInterface $contextService, + Shopware_Components_Config $componentsConfig, + $isEnabledFrontend = true, + $isEnabledBackend = true + ) { $this->container = $container; $this->isEnabledFrontend = (bool) $isEnabledFrontend; $this->isEnabledBackend = (bool) $isEnabledBackend; + $this->contextService = $contextService; + $this->componentsConfig = $componentsConfig; } /** @@ -98,7 +119,7 @@ public function checkBackendTokenValidation(ActionEventArgs $args) $token = $controller->Request()->getHeader($this->tokenName); if (empty($token)) { - $token = $controller->Request()->getParam('__csrf_token'); + $token = $controller->Request()->getParam(self::CSRF_TOKEN_ARGUMENT); } if (!hash_equals($expected, $token)) { @@ -122,7 +143,7 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) $request = $controller->Request(); // do not check internal sub-requests or validated requests - if ($request->getAttribute('_isSubrequest') || $request->getAttribute('isValidated')) { + if ($request->getAttribute('_isSubrequest') || $request->getAttribute(self::CSRF_WAS_VALIDATED)) { return; } @@ -144,7 +165,23 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) } // mark request as validated to avoid double validation - $request->setAttribute('isValidated', true); + $request->setAttribute(self::CSRF_WAS_VALIDATED, true); + } + + public function clearExistingCookie(): void + { + $shop = $this->contextService->getShopContext()->getShop(); + $name = $this->getCsrfName(); + + $front = $this->container->get('front'); + $response = $front->Response(); + $response->headers->clearCookie( + $name, + sprintf('%s/', $shop->getPath() ?: ''), + '', + $shop->getSecure(), + false + ); } /** @@ -154,27 +191,41 @@ public function checkFrontendTokenValidation(Enlight_Event_EventArgs $args) */ private function checkRequest(Request $request) { - $context = $this->container->get(\Shopware\Bundle\StoreFrontBundle\Service\ContextServiceInterface::class)->getShopContext(); - $name = '__csrf_token-' . $context->getShop()->getId(); + $name = $this->getCsrfName(); - if ($context->getShop()->getParentId() && $this->container->get(Shopware_Components_Config::class)->get('shareSessionBetweenLanguageShops')) { - $name = '__csrf_token-' . $context->getShop()->getParentId(); + $token = $request->cookies->get($name); + + if (!\is_string($token)) { + return false; } - $token = $request->cookies->get($name); + $requestToken = $request->get(self::CSRF_TOKEN_ARGUMENT, $request->headers->get(self::CSRF_TOKEN_HEADER)); - $requestToken = $request->get('__csrf_token', $request->headers->get('X-CSRF-Token')); + if (!\is_string($requestToken)) { + return false; + } return hash_equals($token, $requestToken); } + private function getCsrfName(): string + { + $shop = $this->contextService->getShopContext()->getShop(); + + $name = self::CSRF_KEY . $shop->getId(); + + if ($shop->getParentId() && $this->componentsConfig->get('shareSessionBetweenLanguageShops')) { + $name = self::CSRF_KEY . $shop->getParentId(); + } + + return $name; + } + /** * Check if the controller has opted in for CSRF whitelisting and if the * called action is on the whitelist - * - * @return bool */ - private function isWhitelisted(Enlight_Controller_Action $controller) + private function isWhitelisted(Enlight_Controller_Action $controller): bool { if (!($controller instanceof CSRFWhitelistAware)) { return false; @@ -191,10 +242,8 @@ private function isWhitelisted(Enlight_Controller_Action $controller) /** * Check if a controller has opted in for CSRF protection and if the called action * should be protected - * - * @return bool */ - private function isProtected(Enlight_Controller_Action $controller) + private function isProtected(Enlight_Controller_Action $controller): bool { if (!($controller instanceof CSRFGetProtectionAware)) { return false; diff --git a/engine/Shopware/Components/DependencyInjection/services.xml b/engine/Shopware/Components/DependencyInjection/services.xml index 442fce25aa9..5e70de8c484 100644 --- a/engine/Shopware/Components/DependencyInjection/services.xml +++ b/engine/Shopware/Components/DependencyInjection/services.xml @@ -612,6 +612,8 @@ + + %shopware.csrfprotection.frontend% %shopware.csrfprotection.backend% diff --git a/engine/Shopware/Controllers/Frontend/Csrftoken.php b/engine/Shopware/Controllers/Frontend/Csrftoken.php index 5c8329ab933..1f5df3a9e75 100644 --- a/engine/Shopware/Controllers/Frontend/Csrftoken.php +++ b/engine/Shopware/Controllers/Frontend/Csrftoken.php @@ -24,6 +24,8 @@ class Shopware_Controllers_Frontend_Csrftoken extends Enlight_Controller_Action { + private const CSRF_TOKEN_HEADER = 'x-csrf-token'; + /** * Loads auth and script renderer resource */ @@ -39,6 +41,6 @@ public function indexAction() { $token = \Shopware\Components\Random::getAlphanumericString(30); - $this->Response()->headers->set('x-csrf-token', $token); + $this->Response()->headers->set(self::CSRF_TOKEN_HEADER, $token); } } diff --git a/engine/Shopware/Core/sAdmin.php b/engine/Shopware/Core/sAdmin.php index 6f2584a547f..f4040dc62b8 100644 --- a/engine/Shopware/Core/sAdmin.php +++ b/engine/Shopware/Core/sAdmin.php @@ -39,6 +39,7 @@ use Shopware\Components\Cart\CartPersistServiceInterface; use Shopware\Components\Cart\ConditionalLineItemServiceInterface; use Shopware\Components\Compatibility\LegacyStructConverter; +use Shopware\Components\CSRFTokenValidator; use Shopware\Components\HolidayTableUpdater; use Shopware\Components\Model\ModelManager; use Shopware\Components\Password\Manager; @@ -205,6 +206,8 @@ class sAdmin implements \Enlight_Hook 'payment' => [], ]; + private CSRFTokenValidator $csrfTokenValidator; + public function __construct( Enlight_Components_Db_Adapter_Pdo_Mysql $db = null, Enlight_Event_EventManager $eventManager = null, @@ -219,7 +222,8 @@ public function __construct( EmailValidatorInterface $emailValidator = null, Shopware_Components_Translation $translationComponent = null, Connection $connection = null, - OptInLoginServiceInterface $optInLoginService = null + OptInLoginServiceInterface $optInLoginService = null, + CSRFTokenValidator $csrfTokenValidator = null ) { $this->db = $db ?: Shopware()->Db(); $this->eventManager = $eventManager ?: Shopware()->Events(); @@ -243,6 +247,7 @@ public function __construct( $this->optInLoginService = $optInLoginService ?: Shopware()->Container()->get(OptInLoginService::class); $this->conditionalLineItemService = Shopware()->Container()->get(ConditionalLineItemServiceInterface::class); $this->cartOrderNumberProvider = Shopware()->Container()->get(CartOrderNumberProviderInterface::class); + $this->csrfTokenValidator = $csrfTokenValidator ?: Shopware()->Container()->get(CSRFTokenValidator::class); } /** @@ -713,6 +718,8 @@ public function logout() $this->contextService->initializeContext(); + $this->csrfTokenValidator->clearExistingCookie(); + if (!$this->config->get('clearBasketAfterLogout')) { $this->moduleManager->Basket()->sRefreshBasket(); @@ -3270,6 +3277,8 @@ protected function loginUser($getUser, $email, $password, $isPreHashed, $encoder $this->session->offsetSet('sUserId', $userId); $this->session->offsetSet('sNotesQuantity', $this->moduleManager->Basket()->sCountNotes()); + $this->csrfTokenValidator->clearExistingCookie(); + if (!$this->sCheckUser()) { return; } diff --git a/tests/Functional/Components/CSRFTokenValidatorTest.php b/tests/Functional/Components/CSRFTokenValidatorTest.php new file mode 100644 index 00000000000..27910ab5ddd --- /dev/null +++ b/tests/Functional/Components/CSRFTokenValidatorTest.php @@ -0,0 +1,144 @@ +getContainer()->get(CSRFTokenValidator::class), 'isEnabledFrontend', true); + } + + /** + * @after + */ + public function disableCsrfInFrontend(): void + { + Utils::hijackProperty($this->getContainer()->get(CSRFTokenValidator::class), 'isEnabledFrontend', false); + } + + public function testFrontendTokenIsValid(): void + { + $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); + $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); + + $token = Random::getAlphanumericString(30); + + $controller = new MockController(); + $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); + $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); + $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); + $incomingRequest->setParam(CSRFTokenValidator::CSRF_TOKEN_ARGUMENT, $token); + $incomingRequest->cookies->set(self::CSRF_TOKEN_FOR_SHOP_ONE, $token); + $controller->setRequest($incomingRequest); + $controller->setResponse($incomingResponse); + $enlightEventArgs = new Enlight_Event_EventArgs([ + 'subject' => $controller, + ]); + + $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); + + static::assertTrue($incomingRequest->getAttribute('isValidated')); + } + + public function testFrontendTokenValidationThrowsErrorWhenCookieIsNotSet(): void + { + $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); + $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); + $token = Random::getAlphanumericString(30); + + $controller = new MockController(); + $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); + $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); + $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); + $incomingRequest->setParam(CSRFTokenValidator::CSRF_TOKEN_ARGUMENT, $token); + $controller->setRequest($incomingRequest); + $controller->setResponse($incomingResponse); + $enlightEventArgs = new Enlight_Event_EventArgs([ + 'subject' => $controller, + ]); + + $this->expectException(CSRFTokenValidationException::class); + $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); + } + + public function testFrontendTokenValidationThrowsErrorWhenParamIsNotSet(): void + { + $tokenValidator = $this->getContainer()->get(CSRFTokenValidator::class); + $this->getContainer()->get(ContextServiceInterface::class)->createShopContext(1); + + $token = Random::getAlphanumericString(30); + + $controller = new MockController(); + $incomingRequest = new Enlight_Controller_Request_RequestTestCase(); + $incomingResponse = new Enlight_Controller_Response_ResponseTestCase(); + $incomingRequest->setActionName(self::EXISTING_ACTION_NAME); + $incomingRequest->setParam(CSRFTokenValidator::CSRF_TOKEN_ARGUMENT, $token); + $controller->setRequest($incomingRequest); + $controller->setResponse($incomingResponse); + $enlightEventArgs = new Enlight_Event_EventArgs([ + 'subject' => $controller, + ]); + + $this->expectException(CSRFTokenValidationException::class); + $tokenValidator->checkFrontendTokenValidation($enlightEventArgs); + } +} + +class MockController extends Enlight_Controller_Action implements CSRFGetProtectionAware +{ + public function getCSRFProtectedActions() + { + return [ + 'foo', + ]; + } +}