From ca9d22298e0e9424266446032fa9261dbc34b2ba Mon Sep 17 00:00:00 2001 From: Daniel Siepmann Date: Tue, 20 Feb 2024 14:20:29 +0100 Subject: [PATCH] Fix broken cookie handling on PHP side Fix broken cookie path within middleware. For some reason we used the `/typo3/` path while storing cookies server side. But we used `/` in JavaScript. That didn't play together and was fixed to always be `/` for now, but it should be configurable in general. The fix revealed that the detection of whether to store a cookie was broken, which was fixed within the corresponding service. Furthermore the dates how long the cookie should be stored was different. We now always use 7 days. --- .../Middleware/CookieSessionMiddleware.php | 6 +- Classes/Session/CookieSessionService.php | 16 ++--- README.rst | 14 +++++ Resources/Public/JavaScript/Watchlist.js | 4 +- Tests/Functional/BasicsTest.php | 2 +- .../Unit/Session/CookieSessionServiceTest.php | 58 +++++++++++++++++++ 6 files changed, 88 insertions(+), 12 deletions(-) create mode 100644 Tests/Unit/Session/CookieSessionServiceTest.php diff --git a/Classes/Middleware/CookieSessionMiddleware.php b/Classes/Middleware/CookieSessionMiddleware.php index f593f02..8e69d1c 100644 --- a/Classes/Middleware/CookieSessionMiddleware.php +++ b/Classes/Middleware/CookieSessionMiddleware.php @@ -97,11 +97,13 @@ class CookieSessionMiddleware implements MiddlewareInterface throw new \Exception('Could not retrieve normalized params from request.', 1664357339); } + $days = 7; + return new Cookie( $this->cookieSession->getCookieName(), $this->cookieSession->getCookieValue(), - $GLOBALS['EXEC_TIME'] + 7776000, // 90 days - $normalizedParams->getSitePath() . TYPO3_mainDir, + $GLOBALS['EXEC_TIME'] + 24 * 60 * 60 * $days, + $normalizedParams->getSitePath(), '', false, false, diff --git a/Classes/Session/CookieSessionService.php b/Classes/Session/CookieSessionService.php index d687ad1..06ddcf0 100644 --- a/Classes/Session/CookieSessionService.php +++ b/Classes/Session/CookieSessionService.php @@ -34,6 +34,14 @@ class CookieSessionService implements SessionServiceInterface private array $watchlists = []; + public function __construct( + ) { + $this->watchlists['default'] = array_filter(explode( + ',', + $this->getRequest()->getCookieParams()[$this->getCookieName()] ?? '' + )); + } + // Seems to be a bug leading to different instances if we use constructor. public function injectPropertyMapper(PropertyMapper $propertyMapper): void { @@ -42,13 +50,7 @@ class CookieSessionService implements SessionServiceInterface public function getWatchlist(string $identifier): ?Watchlist { - $cookieName = $this->getCookieName(); - $cookie = $this->getRequest()->getCookieParams()[$cookieName] ?? ''; - $items = array_filter(explode( - ',', - $this->getRequest()->getCookieParams()['watchlist'] ?? '' - )); - + $items = $this->watchlists['default']; if ($items === []) { return null; } diff --git a/README.rst b/README.rst index d5ecb16..2d55967 100644 --- a/README.rst +++ b/README.rst @@ -169,3 +169,17 @@ v1.0.1 This is now fixed by properly calling the registerPlugin() method. That way extbase can find the CType definition and will add it as CType instead of list_type. + +* Fix broken cookie handling. + + * For some reason we used the `/typo3/` path while storing cookies server side. + But we used `/` in JavaScript. + That didn't play together and was fixed to always be `/` for now, but it should be configurable in general. + The fix revealed that the detection of whether to store a cookie was broken, which was fixed within the corresponding service. + + * Furthermore the dates how long the cookie should be stored was different. + We now always use 7 days. + + * And we now properly encode the value of the cookie, + in order to prevent issues with special meanings like `;`. + This was already done on PHP side but not within JS. diff --git a/Resources/Public/JavaScript/Watchlist.js b/Resources/Public/JavaScript/Watchlist.js index 2b41b06..4be17a1 100644 --- a/Resources/Public/JavaScript/Watchlist.js +++ b/Resources/Public/JavaScript/Watchlist.js @@ -27,7 +27,7 @@ return []; } - return cookieValue.split(','); + return decodeURIComponent(cookieValue).split(','); }, save: function(items) { var cookieValue = items.join(','); @@ -35,7 +35,7 @@ if (cookieValue == '') { cookie.delete(); } else { - cookie.set(items.join(',')); + cookie.set(encodeURIComponent(items.join(','))); } }, toggleItem: function(identifier) { diff --git a/Tests/Functional/BasicsTest.php b/Tests/Functional/BasicsTest.php index e176d6b..fbda7fb 100644 --- a/Tests/Functional/BasicsTest.php +++ b/Tests/Functional/BasicsTest.php @@ -145,7 +145,7 @@ class BasicsTest extends FunctionalTestCase self::assertSame('watchlist', $cookie->getName()); self::assertSame('page-1', $cookie->getValue()); self::assertNull($cookie->getDomain()); - self::assertSame('/typo3/', $cookie->getPath()); + self::assertSame('/', $cookie->getPath()); self::assertSame('strict', $cookie->getSameSite()); self::assertFalse($cookie->isSecure()); } diff --git a/Tests/Unit/Session/CookieSessionServiceTest.php b/Tests/Unit/Session/CookieSessionServiceTest.php new file mode 100644 index 0000000..02f42c0 --- /dev/null +++ b/Tests/Unit/Session/CookieSessionServiceTest.php @@ -0,0 +1,58 @@ + + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ + +namespace WerkraumMedia\Watchlist\Tests\Unit\Session; + +use PHPUnit\Framework\TestCase; +use TYPO3\CMS\Core\Http\ServerRequest; +use WerkraumMedia\Watchlist\Session\CookieSessionService; + +/** + * @covers \WerkraumMedia\Watchlist\Session\CookieSessionService + */ +final class CookieSessionServiceTest extends TestCase +{ + protected function tearDown(): void + { + unset($GLOBALS['TYPO3_REQUEST']); + + parent::tearDown(); + } + + /** + * @test + */ + public function returnsCookieValueFromCurrentRequest(): void + { + $request = $this->createStub(ServerRequest::class); + $request->method('getCookieParams')->willReturn([ + 'watchlist' => 'page-1', + ]); + $GLOBALS['TYPO3_REQUEST'] = $request; + + $subject = new CookieSessionService(); + $result = $subject->getCookieValue(); + + self::assertSame('page-1', $result); + } +}