Fix failing csp/nonce check due to timed out session #6360
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The CSP nonce is based on the CSRF token. This token does not change, unless you log in (or out). In case of the session data being lost, e.g. because php gets rid of old sessions, a new CSRF token is gen- erated. While this is fine in theory, it actually caused some annoying problems where the browser restored a tab and Nextcloud js was blocked due to an outdated nonce. The main problem here is that, while processing the request, we write out security headers relatively early. At that point the CSRF token is known/generated and transformed into a CSP nonce. During this request, however, we also log the user in because the session information was lost. At that point we also refresh the CSRF token, which eventually causes the browser to block any scripts as the nonce in the header does not match the one which is used to include scripts. This patch adds a flag to indicate whether the CSRF token should be refreshed or not. It is assumed that refreshing is only necessary if we want to re-generate the session id too. To my knowledge, this case only happens on fresh logins, not when we recover from a deleted session file. Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
added
3. to review
Member
|
Is this the same issue I see with the tasks app from time to time, when I see only placeholders and then have to do two hard refreshes before it works again? |
Member
Author
|
Probably, yes! |
Member
|
I have seen this with the spreed app as well. :/ |
rullzer
approved these changes
Sep 7, 2017
Member
rullzer
left a comment
rullzer
left a comment
There was a problem hiding this comment.
Makes sense to me. But I'd like @LukasReschke to have a look as well
Member
|
Nice! Fixes the Mail app issue for me 👍 |
Member
|
@ChristophWurst Mind to open the backport PR? |
Member
Author
Will take care of it ASAP! |
Member
Author
-> #6440 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The CSP nonce is based on the CSRF token. This token does not change,
unless you log in (or out). In case of the session data being lost,
e.g. because php gets rid of old sessions, a new CSRF token is gen-
erated. While this is fine in theory, it actually caused some annoying
problems where the browser restored a tab and Nextcloud js was blocked
due to an outdated nonce.
The main problem here is that, while processing the request, we write
out security headers relatively early. At that point the CSRF token
is known/generated and transformed into a CSP nonce. During this request,
however, we also log the user in because the session information was
lost. At that point we also refresh the CSRF token, which eventually
causes the browser to block any scripts as the nonce in the header
does not match the one which is used to include scripts.
This patch adds a flag to indicate whether the CSRF token should be
refreshed or not. It is assumed that refreshing is only necessary
if we want to re-generate the session id too. To my knowledge, this
case only happens on fresh logins, not when we recover from a deleted
session file.
@LukasReschke @rullzer this is the problem I've discussed with you a few times but never really found the problem. Now that I know how to reproduce it was relatively easy to attach a debugger and see what's happening inside our login magic.
Steps to reproduce:
Before, all scripts were blocked. You couldn't even see the apps menu on the top left. On this branch all scripts are executed as expected.
Fixes nextcloud/mail#322