Fixes for cookie signing #315
Conversation
Codecov Report
@@ Coverage Diff @@
## master #315 +/- ##
============================================
+ Coverage 95.69% 95.74% +0.04%
- Complexity 426 434 +8
============================================
Files 55 55
Lines 1395 1409 +14
============================================
+ Hits 1335 1349 +14
Misses 60 60
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I'm running into this exact issue. Hope the PR is merged soon. |
GwendolenLynch
force-pushed
the
signed-cookies
branch
from
5de2c59
to
7cc3a47
Compare
| @@ -72,7 +72,7 @@ public function onKernelResponse(ResponseEvent $e): void | |||
| $response = $e->getResponse(); | |||
|
|
|||
| foreach ($response->headers->getCookies() as $cookie) { | |||
| if (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true)) { | |||
| if ($cookie->getValue() && (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true))) { | |||
There was a problem hiding this comment.
| if ($cookie->getValue() && (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true))) { | |
| if (null !== $cookie->getValue() && (true === $this->signedCookieNames || \in_array($cookie->getName(), $this->signedCookieNames, true))) { |
| @@ -75,7 +75,7 @@ private function generateSignature(string $value): string | |||
| */ | |||
| private function splitSignatureFromSignedValue(string $signedValue): array | |||
| { | |||
| $pos = strrpos($signedValue, '.'); | |||
| $pos = strrpos($signedValue, ','); | |||
There was a problem hiding this comment.
I'm not so sure about this.. On one hand it seems ok as a workaround, although I would like to still have BC here for . separated cookies so old cookies still work after deploying the new code (i.e. it should split using the last dot or comma found in the string if any).
On the other hand, it seems like the root cause is that you are signing session cookies, and
NelmioSecurityBundle/src/EventListener/SignedCookieListener.php
Lines 57 to 61 in 7cc3a47
So IMO we should probably write to $_COOKIE to unsign the cookies as well as writing to the request.. Any opinion here @nicolas-grekas from the Symfony/session perspective?
There was a problem hiding this comment.
Alternative fix would be to NOT sign the session cookie ever even if * is configured for signed cookies, because really that id should anyway be verified in the session storage to prevent session fixation, so I don't think signing this actually improves anything security wise.
|
@Seldaek What is the status of this PR? Our company is having the same bug. |
|
Nope it's not only the build, it's also my comments above which were not addressed. |
I hit a problem with CSRF tokens being rejected as invalid on both login and Symfony Form submissions.
After a lot of head scratching, I observed that multiple sessions were being created per request and that for some reason the session ID was being invalidated by NativeSessionStorage.
See https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L169-L172 and the comment above does the job explaining why.
The changes in Symfony were introduced here symfony/symfony#46249 and recently updated here symfony/symfony#46790
tl;dr Using a dot as the value delimiter when signing cookies, and signing session cookies, will cause sessions to fail.
Finally, if signing cookies with an empty value causes an exception. So the second fix just handles that case.
Fixes #154
Fixes #312
Fixes #313