array_merge_recursive rather than delay setting the flash messages cookie #559
Conversation
dbu
force-pushed
the
fix-flash-cookie-handling
branch
2 times, most recently
from
5c76d46 to
082aa4a
Compare
we recently fixed that we lose previous cookies. but the fix was keeping the messages in the session and only setting the cookie on the last redirect, which means the messages are lost if the session is destroyed (e.g. redirect to page outside firewall) changed to merge the flash messages cookie from requests into the flashes. and using array_merge_recursive to keep multiple messages with the same key.
dbu
force-pushed
the
fix-flash-cookie-handling
branch
from
082aa4a to
afe9c56
Compare
dbu
force-pushed
the
fix-flash-cookie-handling
branch
from
afe9c56 to
4970863
Compare
|
@ismailcherri @spointecker may I ask you two to review this pull request and tell if you see any new pitfalls i might be introducing here? |
| // 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'])) { |
There was a problem hiding this comment.
before, we did not look at request cookies at all.
i was wondering if there is a security risk involved with this (depending on what weird things a dev might put into a flash message) but i guess if the attacker manages to inject a cookie for this domain, they can also do more direct attacks with javascript that do not involve looping through the backend.
|
LGTM. Thanks for the quick answer and fix, will test it with my app&tests as soon as it is released! |
|
LGTM. Thanks for the fix. |
|
thanks for the quick feedback! released https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/releases/tag/2.10.2 and https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/releases/tag/2.9.2 |
fix #558