Merge pull request #559 from FriendsOfSymfony/fix-flash-cookie-handling

array_merge_recursive rather than delay setting the flash messages cookie

.github/workflows/php.yml

@@ -0,0 +1,59 @@
+name: CI
+
+env:
+  SYMFONY_PHPUNIT_DIR: "$HOME/symfony-bridge/.phpunit"
+
+on:
+  push:
+      branches:
+          - master
+  pull_request:
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        include:
+          # Test the latest stable release
+          - php-version: '7.2'
+          - php-version: '7.3'
+          - php-version: '7.4'
+          # Test Symfony LTS versions. Read more at https://github.com/symfony/lts
+          - php-version: '7.4'
+            dependencies: 'symfony/lts:^3'
+          - php-version: '7.4'
+            symfony-version: '^4'
+          - php-version: '7.4'
+            symfony-version: '^5'
+          # Test latest unreleased versions
+          - php-version: '7.4'
+            symfony-version: '^5'
+            stability: 'dev'
+    name: PHP ${{ matrix.php-version }} Test on Symfony ${{ matrix.symfony-version }} ${{ matrix.dependencies}} ${{ matrix.stability }} ${{ matrix.composer-flag }}
+    steps:
+      - name: Pull the code
+        uses: actions/checkout@v2
+      - name: Install PHP and Composer
+        uses: shivammathur/setup-php@v2
+        with:
+          php-version: ${{ matrix.php-version }}
+          tools: composer:v2
+      - name: Check PHP Version
+        run: php -v
+      - name: Stability
+        run: composer config minimum-stability ${{ matrix.stability }}
+        if: ${{ matrix.stability }}
+      - name: Additional require
+        run: composer require --no-update ${{ matrix.dependencies }}
+        if: ${{ matrix.dependencies }}
+      - name: Symfony version
+        run: composer require --no-update symfony/flex && composer config extra.symfony.require ${{ matrix.symfony-version}}
+        if: ${{ matrix.symfony-version }}
+      - name: Composer update
+        run: composer update ${{ matrix.composer-flag }} --prefer-dist --no-interaction
+      - name: Composer validate
+        run: composer validate --strict --no-check-lock
+      - name: Run tests
+        run: php vendor/bin/simple-phpunit -v
+

CHANGELOG.md

@@ -1,6 +1,17 @@
 Changelog
 =========
 
+2.9.2
+-----
+
+### Fixed
+
+* 2.9.1 fixed overwriting flash messages on multiple redirects, but introduced
+  a risk to lose flash messages when redirecting to a path that is outside the
+  firewall or destroys the session.
+  This version hopefully fixes both cases. Existing flash messages in a request
+  cookie are merged with new flash messages from the session.
+
 2.9.1
 -----
 

src/EventListener/FlashMessageListener.php

@@ -87,26 +87,28 @@ public function onKernelResponse(FlashMessageResponseEvent $event)
             return;
         }
 
-        $response = $event->getResponse();
-
-        // If the response is a redirect, we should wait until the final response
-        // is reached
-        if ($response->isRedirect()) {
-            return;
-        }
-
         $flashBag = $this->session->getFlashBag();
         $flashes = $flashBag->all();
 
         if (empty($flashes)) {
             return;
         }
 
+        $response = $event->getResponse();
+
         $cookies = $response->headers->getCookies(ResponseHeaderBag::COOKIES_ARRAY);
         $host = (null === $this->options['host']) ? '' : $this->options['host'];
         if (isset($cookies[$host][$this->options['path']][$this->options['name']])) {
             $rawCookie = $cookies[$host][$this->options['path']][$this->options['name']]->getValue();
-            $flashes = array_merge($flashes, json_decode($rawCookie));
+            $flashes = array_merge_recursive($flashes, json_decode($rawCookie, true));
+        }
+
+        // Preserve existing flash message cookie from previous redirect if there was one.
+        // This covers multiple redirects where each redirect adds flash messages.
+        $request = $event->getRequest();
+        if ($request->cookies->has($this->options['name'])) {
+            $rawCookie = $request->cookies->get($this->options['name']);
+            $flashes = array_merge_recursive($flashes, json_decode($rawCookie, true));
         }
 
         $cookie = new Cookie(

tests/Functional/EventListener/FlashMessageListenerTest.php

@@ -13,7 +13,6 @@
 
 use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
 use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
-use Symfony\Component\HttpFoundation\Cookie;
 
 class FlashMessageListenerTest extends WebTestCase
 {
@@ -33,7 +32,6 @@ public function testFlashMessageCookieIsSet()
 
         $found = false;
         foreach ($cookies as $cookie) {
-            /** @var Cookie $cookie */
             if ('flash_cookie_name' !== $cookie->getName()) {
                 continue;
             }
@@ -45,9 +43,7 @@ public function testFlashMessageCookieIsSet()
             $found = true;
         }
 
-        if (!$found) {
-            $this->fail('Cookie flash_cookie_name not found in the cookie response header: '.implode(',', $cookies));
-        }
+        $this->assertTrue($found, 'Cookie "flash_cookie_name" not found in response cookies');
     }
 
     public function testFlashMessageCookieIsSetOnRedirect()
@@ -65,7 +61,6 @@ public function testFlashMessageCookieIsSetOnRedirect()
 
         $found = false;
         foreach ($cookies as $cookie) {
-            /** @var Cookie $cookie */
             if ('flash_cookie_name' !== $cookie->getName()) {
                 continue;
             }
@@ -77,8 +72,6 @@ public function testFlashMessageCookieIsSetOnRedirect()
             $found = true;
         }
 
-        if (!$found) {
-            $this->fail('Cookie flash_cookie_name not found in the cookie response header: '.implode(',', $cookies));
-        }
+        $this->assertTrue($found, 'Cookie "flash_cookie_name" not found in response cookies');
     }
 }