AUT-4272: migrate from csurf to csurf sync (v2)#2804
Closed
sw-mattw wants to merge 7 commits into
Closed
Conversation
Note that this helper is no longer used as `csrf-sync` does not accept cookie options as the session is used for token storage.
Refreshing per request was too fragile - we had issues when an optional script was not found causing the session token to refresh meaning the token in the form submission did not match (causing a 403). This means that with per request refresh if we make a mistake with an optional script and it 404s then the entire app is broken. This may also be problematic with slow page loads and double submits. Note that having a token per session mimics the previous implementation with `csurf` and is simpler than per request. This creates a session even when not authenticated, so works with the anonymous pages (e.g., cookies, contact-us). Other reference: https://security.stackexchange.com/a/22936
sw-mattw
force-pushed
the
AUT-4272/migrate-from-csurf-to-csurf-sync-v2
branch
from
98c398d to
f01288f
Compare
Note that if the session is invalid (e.g., if it has expired) then we wish to render the standard "session expired" page. Previously in this case we would render the CSRF error text which is not user friendly. This commit renders the "session expired" page if a CSRF error has occurred, but this is due to an invalid session. If the session is still active, then we render the "internal server error" page. The user can use the browsers back button to return to the form on the previous page, the token in the "hidden" input will be reset to a valid value automatically, allowing the user to resubmit the form successfully.
sw-mattw
force-pushed
the
AUT-4272/migrate-from-csurf-to-csurf-sync-v2
branch
from
f01288f to
9747a66
Compare
…endered If a session is valid but the csrf token is invalid, we render the "internal server error" page. If a session is invalid, we render the "session expired" page.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
This PR includes the same changes as #2793 with some modifications to the redirect behaviour (see below).
In #2793, if a session expired and a user submitted a form the CSRF plaintext error would be rendered which was not user friendly. The additional work under this PR handle this case, rendering the standard "session expired" page in this scenario. Additionally, if a session is valid but the token is missing or invalid, the standard "internal server error" page will now be rendered. For further details on these additions, see the description of 98c398d.
Release
We will perform a controlled release of this change as it affects all frontend pages (in this repo) with forms. We will pause the pipeline following deployment to staging, and then following deployment to integration, testing on these environments before proceeding.
Testing
Deployed to authdev1, and:
/enter-emailpage, changed the CSRF token hidden input value slightly, submitted form, received the "Sorry, there is a problem" 500 page (expected as the token in the hidden input did not match that in the session)./enter-emailpage, waited for the session to expire (5m on dev), submitted form, received the "session expired" page (as expected - token comparison fails as there is no session for the token to be stored in)./enter-emailpage, changed the CSRF token hidden input value slightly, waited for the session to expire (5m on dev), submitted form, received the "session expired" page (as expected - token comparison fails as although the hidden input value is wrong, there is no session for the token to be stored in)./enter-emailpage, waited for the session to expire (5m on dev), clicked the "Back" link, received the "session expired" page (as expected).For each of these, observed aps cookie being set (session) and the token in the hidden input.
How to review
Checklist
Related PRs