-
Notifications
You must be signed in to change notification settings - Fork 38
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delay signature verification properly #64
Conversation
17c16d7
to
51fc53c
Compare
@@ -371,8 +372,8 @@ public function testSessionTokenParsingIsDelayedWhenSessionIsNotBeingUsed() | |||
{ | |||
/* @var $signer Signer|\PHPUnit_Framework_MockObject_MockObject */ | |||
$signer = $this->createMock(Signer::class); | |||
|
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.
Could you clarify why this test was incorrect without the signature?
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.
Restore this spacing
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.
Could you clarify why this test was incorrect without the signature?
Description of #63
SessionMiddleware#appendToken()
always callsSessionInterface#isEmpty()
which will initialise the session data.The test only passes because the token is not signed and then
Token#verify()
throws an exception that is silently ignored.
Commit message:
Signature verification was never being delayed because an unsecured
token was being used andToken#verify()
was throwing an exception
instead.
Do you want me to reword the commit message or is this enough?
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.
Restore this spacing
Sure, will do
@@ -248,25 +248,21 @@ public function extractSessionContainer(Token $token = null) : SessionInterface | |||
private function appendToken(SessionInterface $sessionContainer, Response $response, Token $token = null) : Response | |||
{ | |||
$sessionContainerChanged = $sessionContainer->hasChanged(); | |||
$sessionContainerEmpty = $sessionContainer->isEmpty(); |
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.
This was cached on purpose to avoid calling isEmpty()
twice, since the sessiondata is potentially mutable: can it be restored, or is this removal part of tge fix?
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.
It's part of the fix as described on the PR's decription
Moving
sessionContainer#isEmpty()
calls to the end of the expressions delays the initialisation properly.
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.
Basically that leads to the question: do we want to keep this lazy initialisation thing or not?
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.
Seen now - this is described in #63
do we want to keep this lazy initialisation thing or not?
Yes, reading/writing to session in each request, as well as running crypto, is otherwise a very good way to keep data-centers warm. Let's not do that :-)
Signature verification was never being delayed because an unsecured token was being used and `Token#verify()` was throwing an exception instead. Closes #63
51fc53c
to
510172c
Compare
@Ocramius space reverted... do you want me to change anything else? |
🚢 |
As explained on #63 the middleware was always actually trying to verify the signature.
Moving
sessionContainer#isEmpty()
calls to the end of the expressions delays the initialisation properly.