Add ability to specify CSRF token in header #925
Conversation
This file contains 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
matthew-white
commented
Jul 13, 2023
test/integration/api/sessions.js
Outdated
There was a problem hiding this comment.
I wrapped some tests in a describe block, so the diff will look nicer if you hide whitespace.
| @@ -298,18 +298,6 @@ describe('preprocessors', () => { | |||
| ) | |||
| )).should.be.rejectedWith(Problem, { problemCode: 401.2 })); | |||
|
|
|||
| it('should reject cookie auth with incorrect CSRF token for non-GET requests', () => | |||
There was a problem hiding this comment.
This test was moved below.
| ); | ||
| context.body.should.eql({ __csrf: 'secretcsrf', other: 'data' }); | ||
| }); | ||
| }); | ||
|
|
||
| it('should url-decode the CSRF token', () => |
There was a problem hiding this comment.
This test ("should url-decode the CSRF token") was moved below.
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.
If a non-GET request is sent, and a session cookie is used to authenticate, then the CSRF token associated with the session must also be provided. Right now, the CSRF token is expected to be in the request body. For example, Frontend provides the CSRF token from the iframe form in the
SubmissionDownloadcomponent. Enketo also provides the CSRF token when using cookie auth.However, I think the expectation that the CSRF token is in the request body doesn't work for all requests. The main issue that comes to mind is file uploads. For example, I think requiring the CSRF token to be in the request body would be an obstacle to using cookie auth to upload a form definition or form attachment.
This PR makes it possible to use cookie auth in such cases by allowing the CSRF token to be specified in a header instead of the request body.
What has been done to verify that this works as intended?
New tests.
Why is this the best possible solution? Were any other approaches considered?
I don't love that there are now two ways to specify the CSRF token. I thought about removing support for CSRF tokens in the request body so that there would be only one way to specify the token (via the header). However, that wouldn't work for HTML form submission like from the iframe form. I also didn't want to change anything about our Enketo integration.
I also don't think this PR adds that much complexity. Only two lines of code were changed.
If a user of cookie auth has a choice between specifying the token in the header vs. in the request body, specifying it in the header is probably slightly better. That's because it doesn't require the token to be removed from the request body.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
During regression testing, we will verify that the parts of Central that rely on cookie auth continue to work.
Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.
I don't think changes are needed. We discourage the use of cookie auth in the API docs, and we don't document any requirements around the CSRF token.
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes