[Security] Fail gracefully if the security token cannot be unserialized from the session #25669
Conversation
|
Travis failure looks unrelated. |
|
Thanks for the fix, looks legit. I think for a full fledged implementation, we should borrow from |
| try { | ||
| $unserialized = unserialize($serialized); | ||
| } catch (\ErrorException $e) { | ||
| // To be rethrown after restoring the error handler. |
There was a problem hiding this comment.
Can use finally in Symfony 3+.
| $token = $this->unserialize($serializedToken); | ||
| } catch (\ErrorException $e) { | ||
| if (null !== $this->logger) { | ||
| $this->logger->warning('Failed to unserialize the security token from the session.', array('key' => $this->sessionKey, 'received' => $serializedToken, 'exception' => $e)); |
There was a problem hiding this comment.
Shouldn't this be an error instead? It means that something goes wrong somewhere.
Can you rename key to session_key? This should describe the function a bit more when viewing the logs. At first I thought this was the firewall (context) key, but that's included in the session key.
There was a problem hiding this comment.
Can do, though I was keeping it in-line with
There was a problem hiding this comment.
Fair enough, best to keep it consistent then (regarding the key).
There was a problem hiding this comment.
feels like nobody noticed my previous comment, here it is :)
Thanks for the fix, looks legit. I think for a full fledged implementation, we should borrow from ResourceCheckerConfigCache::safelyUnserialize().
The currently proposed logic is not robust enough AFAIK.
|
@nicolas-grekas Implemented in 23e6fee. |
nicolas-grekas
force-pushed
the
access-token-unserialization
branch
from
23e6fee
to
219118a
Compare
…ed from the session
nicolas-grekas
force-pushed
the
access-token-unserialization
branch
from
219118a
to
053fa43
Compare
There was a problem hiding this comment.
@thewilkybarkid I pushed some changes directly on your fork, PR ready on my side.
|
Thank you @thewilkybarkid. |
…t be unserialized from the session (thewilkybarkid) This PR was merged into the 2.7 branch. Discussion ---------- [Security] Fail gracefully if the security token cannot be unserialized from the session | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | If the security token in the session can't be unserialized, an `E_NOTICE` is issued. This prevents it (and provides a better log message if it's not even a `__PHP_Incomplete_Class`). This is similar to #24731, but I saw it triggered when changing OAuth library (elifesciences/journal#824), so the token class itself no longer exists. (I want to avoid having to manually invalidate all sessions, as not all sessions use that token class.) Commits ------- 053fa43 [Security] Fail gracefully if the security token cannot be unserialized from the session
If the security token in the session can't be unserialized, an
E_NOTICEis issued. This prevents it (and provides a better log message if it's not even a__PHP_Incomplete_Class).This is similar to #24731, but I saw it triggered when changing OAuth library (elifesciences/journal#824), so the token class itself no longer exists. (I want to avoid having to manually invalidate all sessions, as not all sessions use that token class.)