Skip to content
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

Reduce frequency of CSRF Tokens being refreshed/reset #3168

Merged
merged 4 commits into from Mar 22, 2021

Conversation

tdonohue
Copy link
Member

@tdonohue tdonohue commented Mar 2, 2021

References

Loosely related to DSpace/dspace-angular#1024 , but both PRs are standalone and not dependent on one another

Description

While testing batch uploads (see DSpace/dspace-angular#1024), I realized that the CSRF token is being refreshed/changed every time it is used. This means it refreshes on every POST/PUT/PATCH/DELETE request. While this seemed like a good idea in initial implementation, it now seems potentially problematic / complex for batch operations.

The reason is that during a batch upload, the CSRF token can change between every uploaded file (when each file is sent as a separate request). This can be difficult on the client side to achieve/manage, as any batch operations need to carefully monitor the CSRF Token.

It also runs the risk of an (unproven) possible "race" condition: If the client sends the next request before or while reading the response of the last one & that response changes the CSRF token, then the "next request" may include an outdated CSRF token (causing a 403 error).

So, this PR changes the logic so that CSRF Tokens are only reset/refreshed in the following scenarios:

  • Anytime your AuthToken changes, e.g. during Login/Logout. This is more in line with when Spring Security defaults to changing the CSRF token...see for example their CsrfAuthenticationStrategy
  • If the CSRF Token was sent by the client to the server via a querystring or form parameter (as these parameters can be easily read by a third party & therefore are less secure)
  • If an InvalidCsrfTokenException occurs, as this means the client sent an invalid CSRF token to the server. So, the server should invalidate and send a new one (allowing the client to resync its CSRF token). This scenario is not covered in this PR, as it is already in the codebase in DSpaceAccessDeniedHandler

Instructions for Reviewers

  • Review code & new tests to prove the new scenarios are working as expected.
    • NOTE: I also updated/added JavaDocs to RestAuthenticationService to make these changes easier to understand.
  • Attempt login & logout via Angular UI or Hal Browse and verify the CSRF Token changes. Verify also that other operations (e.g. editing an Item or starting a new Submission) do not change the CSRF Token
    • This is easiest to verify from the Angular UI by opening Chrome DevTools and watching the Cookies in the "Application" tab, verifying the XSRF-TOKEN cookie updates after login & logout, but not during other operations.

@tdonohue tdonohue added this to the 7.0beta5 milestone Mar 2, 2021
@tdonohue tdonohue self-assigned this Mar 2, 2021
@tdonohue tdonohue added this to Needs Reviewers Assigned in DSpace 7 Beta 5 via automation Mar 2, 2021
@tdonohue tdonohue changed the title Reduce the number of times CSRF Tokens are refreshed/reset Reduce frequency of CSRF Tokens being refreshed/reset Mar 2, 2021
@tdonohue tdonohue requested a review from abollini March 4, 2021 16:03
@tdonohue tdonohue moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 5 Mar 4, 2021
Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @tdonohue I have put a question inline, can you help me to understand the code?

@tdonohue
Copy link
Member Author

@abollini : I've responded to your questions above. As noted in those responses, I don't think there's any code changes necessary for this PR, but let me know if you disagree.

@abollini
Copy link
Member

I'm ok with the improvement that you have suggested, +1 to merge

DSpace 7 Beta 5 automation moved this from Under Review to Reviewer Approved Mar 18, 2021
@tdonohue tdonohue force-pushed the reduce_csrf_token_refreshing branch from c9337a3 to 404b828 Compare March 18, 2021 21:06
@tdonohue tdonohue merged commit f4b5e9c into DSpace:main Mar 22, 2021
DSpace 7 Beta 5 automation moved this from Reviewer Approved to Done Mar 22, 2021
@tdonohue tdonohue deleted the reduce_csrf_token_refreshing branch March 22, 2021 15:07
DSpace 7 Beta 5 automation moved this from Done to Reviewer Approved Apr 15, 2021
DSpace 7 Beta 5 automation moved this from Reviewer Approved to Done Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants