3. Improve handling of qr timeout & state errors when polling #210
Conversation
johanib
force-pushed
the
feature/implement-session-required-atttribute
branch
from
ffa280b to
31ed973
Compare
|
@MKodde I went through this. It appears to be in order. Afaiks, all tickets are covered: https://www.pivotaltracker.com/n/projects/1163646/stories/188205232 I did not test the timeout feature. Is that easy to test on dev? As for what next, I want to address the issues I found and then squash & write an adequate commit message. |
johanib
force-pushed
the
feature/implement-session-required-atttribute
branch
3 times, most recently
from
947862a to
bd9b710
Compare
| return null; | ||
| } | ||
|
|
||
| return hash('sha256', $this->appSecret . substr($sessionCookie, 0, 10)); |
There was a problem hiding this comment.
The session cookie value is not something that we want to expose. Even to the client, because it is an http-only cookie. Session cookies are shot lived. The appSecret is long lived en we do not want it exposed outside the server. The length of this secret depends on the configuration.
As discussed with @johanib truncating the hash to e.g. 64 bits (8 hex characters) is fine, it does not matter which part of a secure hash you take. The goal of the correlation ID is to be unique for all authentications in about a day, even then a clash is not a security issue.
I see a lot of variation in the configuration options for the session ID: https://www.php.net/manual/en/session.configuration.php. A typical minimum is 128 bits of entropy (32 hex characters – 4 bits per character).
The value of the session ID is controllable by an attacker, and the resulting correlation ID is visible to the attacker. This means that the current algorithm can help an attacker to find the appSecret. Truncating the hash won't help, because an attack can calculate the result for as many session ID's as they want.
I don't like the idea of introducing the app secret here. Introducing a nonce (from the .ENV) is fine.
substr(hash('sha256', $sessionCookie.$independent_nonce),0, 8) is better
Check that independent_nonce is at least 16 characters, and return null if it is not.
I can even live doing this without the nonce, because the session id is short lived en the ID is exposed to the client only.
johanib
force-pushed
the
feature/implement-session-required-atttribute
branch
3 times, most recently
from
ab28148 to
1519ea9
Compare
…entifier along with the polls
johanib
force-pushed
the
feature/implement-session-required-atttribute
branch
2 times, most recently
from
303e565 to
44e890a
Compare
johanib
changed the title
#[RequiresActiveSession] attribute#[RequiresActiveSession] attribute
Prior to this change, when the code expired, the user was shown the error page. This change shows the user a friendly message and a retry link that serves a fresh qr code. When a state error occurs, an invalid-request message is sent to the status page, that will then show a 'something went wrong, refresh to try again' message. See https://www.pivotaltracker.com/n/projects/1163646/stories/188205289
Prior to this change, the application would not check if the qr code was expired. If the user scanned an expired qr code, it would try to see if it was invalid. This resulted in errors in the logs, because it should not even try to see if the scanned qr code is valid if it is expired. This change checks if the qr code is expired before attempting to check its validity. See https://www.pivotaltracker.com/n/projects/1163646/stories/188205272
johanib
force-pushed
the
feature/implement-session-required-atttribute
branch
from
44e890a to
0e18752
Compare
johanib
changed the title
#[RequiresActiveSession] attribute
Prior to this change there we weren't able to keep track of sessions that got lost. This change allows us to see every time a session is created and distinguish them by their correlation id. Log an error on a route that requires an active session when there is none Prior to this change all routes were able to called, even though the user might not have had an active session This change will start logging errors when the session wasn't found, or is in an unexpected state Prior to this change session information got lost. We had no way of tracking down what happened to user sessions in the logs. This change logs whether a session existed and if it's in a valid state. Log information is enriched with a correlation id to be able to distinguish them. See https://www.pivotaltracker.com/n/projects/1163646/stories/188205214
Prior to this change, when a polling action from the browser hit the server, and the server returned a 500, or some other error, it was not possible to trace which session the browser belonged to. This change adds a correlation id to the polling mechanism, so that id can be used to search te logs for the origin of the session. This should help retrace errors. See: https://www.pivotaltracker.com/story/show/188205232
Hi Pieter, I split the pr's up in smaller parts, also splitting the refactors into separate pr's, to be reviewed in order:
I hope this makes it easier to review. |
|
Given I'm the author of this PR, I'm not able to approve this change. But for what its worth ✔️ |
…rors 4. Improve logging on session related errors
…olling 5. Add the session id derived id to polling requests
Improve handling of qr timeout
Prior to this change, the application would not check if the qr code was expired. If the user scanned an expired qr code, it would try to see if it was invalid.
This resulted in errors in the logs, because it should not even try to see if the scanned qr code is valid if it is expired.
This change checks if the qr code is expired before attempting to check its validity.
See https://www.pivotaltracker.com/n/projects/1163646/stories/188205272
Improve handling of state errors when polling
Prior to this change, when the code expired, the user was shown the error page.
This change shows the user a friendly message and a retry link that serves a fresh qr code.
When a state error occurs, an invalid-request message is sent to the status page, that will then show a 'something went wrong, refresh to try again' message.
See https://www.pivotaltracker.com/n/projects/1163646/stories/188205289