Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Fail gracefully if the security token cannot be unserialized from the session#25669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
thewilkybarkid commentedJan 3, 2018
Travis failure looks unrelated. |
nicolas-grekas commentedJan 3, 2018
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. |
thewilkybarkidJan 4, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can usefinally 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't this be an error instead? It means that something goes wrong somewhere.
Can you renamekey tosession_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can do, though I was keeping it in-line with
| $this->logger->warning('Expected a security token from the session, got something else.',array('key' =>$this->sessionKey,'received' =>$token)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair enough, best to keep it consistent then (regarding the key).
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
thewilkybarkid commentedJan 5, 2018
@nicolas-grekas Implemented in23e6fee. |
23e6fee to219118aCompare…ed from the session
219118a to053fa43Comparenicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@thewilkybarkid I pushed some changes directly on your fork, PR ready on my side.
fabpot commentedJan 8, 2018
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.)