Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When using HTTP Basic Auth from a browser plugin such as Dev HTTP Client in Chrome, the session cookie can possibly be sent even though the route uses Basic Auth. This causes an issue because it triggers a check for the CSRF token.
Proposed resolution
Only check for CSRF if the active authentication provider is cookie.
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff.txt | 764 bytes | jessebeach |
#32 | rest-csrf-2099439-32.patch | 7.21 KB | jessebeach |
#29 | 2099439-29-rest-csrf_no-fix.patch | 7.09 KB | linclark |
#29 | interdiff.txt | 1.72 KB | linclark |
#29 | 2099439-29-rest-csrf.patch | 7.18 KB | linclark |
Comments
Comment #1
linclark CreditAttribution: linclark commentedThis patch checks whether the route has any auth enabled, and if it does that cookie auth is included in the enabled auth.
Comment #2
jessebeach CreditAttribution: jessebeach commentedVerified using Postman. A POST request using basic authorization successfully creates a node, returning a 201 response. Without this patch, POST requests were returning 403, access denied.
Comment #4
klausiI think this depends on #2065193: supported_formats and supported_auth should work in the same way where we make the _auth property in the REST routes mandatory.
Comment #5
linclark CreditAttribution: linclark commentedWe need to add a test for this, but unfortunately our current test seems to work unpredictably. This will be fixed in #2099679: Changing the order of asserts in AuthTest::testRead makes them fail.
Comment #6
linclark CreditAttribution: linclark commentedSo we need to change approach here. I don't think we can handle CSRF as an access check.
It would be possible for both basic auth and cookie auth to be enabled on the same route. There is no way to determine at the time of access check whether the authentication was handled by Basic Auth or Cookie, so we can't determine whether the CSRF should apply.
CSRF is really part of the authentication, though, not part of the authorization. For example, if someone is forging a user that user might be authorized to access a resource, but the forger should not be authenticated as the user they are mimicking. While many frameworks do respond with a 403 (which is an authorization error), others respond with 400 (e.g. Mojolicious and web.py)
I think we should move the logic to the Cookie Provider and then throw a 400 if the request method is non-read and the CSRF token doesn't match.
Comment #7
linclark CreditAttribution: linclark commentedIn order to move forward with my suggested approach, we would need to require CSRF tokens for all write requests. This currently breaks Form API and would potentially break module defined callbacks in contrib and custom modules.
This would however mean that contrib modules would have fewer potential security holes. Would it be acceptable for us to break those callbacks but provide good instructions for how to implement CSRF token support?
Comment #8
linclark CreditAttribution: linclark commentedRelated issue: #1891052: Protect write operations from CRSF by requiring an token (when using cookie/session-based authentication). There was also a related security issue which I discussed with @frega. It seems that requiring CSRF tokens on all write operations was also discussed there.
Comment #9
linclark CreditAttribution: linclark commentedThis patch works with Postman, but will break form submission on the site. It includes code from #2099679: Changing the order of asserts in AuthTest::testRead makes them fail.
Comment #10
linclark CreditAttribution: linclark commentedWith this patch, form based submissions work as well. I'm going to run it through the testbot to see what fails.
Comment #12
klausiI'm not convinced that we should do this. CSRFAccessCheck is specifically used in REST module, the form API and the other parts of Drupal use their own CSRF checking and I think it is too late to change that for D8.
Workaround for Postman users and other browser clients that always send the cookies along: logout your human session from your drupal site and use a second browser.
I'm also not convinced that CSRF checking is part of authentication. Drupal is able to successfully recognize you and continue your session (authentication), but it needs to check for CSRF before letting you write/manipulate resources on the site (sounds like authorization to me).
The AuthenticationManager holds a triggeredProviderId which contains the name of the authentication provider that was used in the current request. We could make that available on the request object (surely useful for contrib, too!) and check for that in the CSRFAccessCheck.
Anyway, the double authentication credentials sent by Postman and other plugins is still a problem and we are currently just lucky that the basic auth provider has a higher priority and runs first. Not sure we should even do anything about that - please make your clients only send one authentication possibility!
Comment #13
linclark CreditAttribution: linclark commentedI didn't realize that this was stored. Yes, I think you're right, at this point it is probably the best solution.
Comment #14
linclark CreditAttribution: linclark commentedIt turns out that the authentication_provider is already on the request. So all we need to do here is move from implicit logic (if there is a session cookie, we are using cookie auth) to explicit logic (if the _authentication_provider is cookie, then we are using cookie auth).
I have included a test for this.
Comment #14.0
linclark CreditAttribution: linclark commentedChanged proposed approach
Comment #15
linclark CreditAttribution: linclark commentedCan't we just build that in to the system? e.g. by adding a priority like we have on so many other systems, or even by hardcoding cookie to run last?
Not being able to test your REST services from the same browser instance you are using to configure it is a huge DX WTF, and it seems to me like a completely unnecessary one.
Comment #16
klausiMeh, I'm a bit reluctant to add yet another slow web test class and all the boiler plate code to set this up.
But on the other hand we need to keep our sanity when testing all of this, so a separate class probably makes sense.
Should be "Tests the CSRF protection."
Don't do this yourself, use the proper cURL options for Baisc Auth, see the REST AuthTest calls for example.
We should also assert here that the entity has been created in the database, not only the response code.
Same here: check that the entity now actually exists.
The rest looks good. The basic auth access provider already has a higher priority in the current core setup, so we don't have to fix anything there.
Comment #17
linclark CreditAttribution: linclark commentedI wasn't sure whether setting the headers using cURL's Basic Auth options would make the request behave differently (I honestly haven't worked enough with cURL to know how it computes the request from the options). Setting the header directly seemed to be more explicitly replicating the issue. But if you are confident that it doesn't alter the behavior of the request, I'm fine with that.
Added.
Comment #18
klausian entity_load_multiple() in the assertion should do the trick here, no need to parse the header. This is already tested somewhere else.
same here with an entity_load_multiple() instead.
And I think we should try to run this in one single test method, since webtests are slow and the overall code is not that long. We can use the unset($this->curlHandle) technique from #2099679: Changing the order of asserts in AuthTest::testRead makes them fail to make sure we reset the browser properly between the tests.
Comment #19
linclark CreditAttribution: linclark commentedIf we run it as a single test method, then we shouldn't simply use entity_load_multiple. This would automatically pass for the second test. We could test how many there are, but I think that is too brittle because moving the assertions around would cause it to break.
.... and it turns out this code no longer works. Who's been mucking about in access lately?Forgot about the http_basic -> basic_auth conversion.Comment #20
linclark CreditAttribution: linclark commentedSo I tried to combine these and even when using the hack from the other issue, I still got very strange, hard to debug fails. I think it is best to leave these as two separate tests.
Comment #21
linclark CreditAttribution: linclark commentedComment #22
linclark CreditAttribution: linclark commentedklausi and I discussed this in IRC and he was concerned with using assertTrue with entity_load_multiple. Here is a different approach that uses the location header.
Comment #23
klausiI would rather like
$this->assertTrue(entity_load_multiple(...) !== FALSE
to avoid writing so much test code and methods.Comment #24
linclark CreditAttribution: linclark commentedChecking that the actual entity is created is more accurate. This means that we can potentially combine the tests later on without worrying about corrupting the results. I don't see a downside, and it is only 3 lines of code.
Comment #25
Crell CreditAttribution: Crell commentedWe shouldn't be merging tests together in one method, as that's bad test practice. That said, $this->assertTrue(entity_load_multiple(...) !== FALSE strikes me as far too fragile. We should be testing the specific entity that was created, not that there is an entity. "that there is an entity somewhere" is not actually testing what we're doing; it is testing an incidental side-effect of what we're doing.
Comment #26
linclark CreditAttribution: linclark commentedAgreed, though Klaus does have a point about how long individual tests take to run. The current patch does not combine tests, so it should pass on this point.
Agreed. I believe that the loadEntityFromLocationHeader() method passes on this point.
So I think the current patch should be good. Can someone give this a final look through and RTBC? Hoping to get this in for WSCCI sprinting tomorrow.
Comment #27
klausi{@inheritdoc} missing, see https://drupal.org/node/325974 (yes, that changed recently).
{@inheritdoc} missing, see https://drupal.org/node/325974
This should be "Get the cURL options to create an entity with POST." We should at least mention what those options are good for.
"url" should be "URL". Also, it is a bit weird that this method is on the abstract base class although it is only used by the CSRF test class? Right, we could use it in a future test, so why not.
But apart from that nitpicks this looks good to me. Sanity before test running performance is probably the right thing to do here.
Comment #28
linclark CreditAttribution: linclark commentedThanks for the review. I made all the changes.
We can also use it in the CreateTest, I believe, but I figure that can be a follow up.
Comment #29
linclark CreditAttribution: linclark commentedIn the sprints, Alex pointed out some comment fixes and asked that I post the test without the fix.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedThanks.
Comment #30.0
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #31
alexpottComment #32
jessebeach CreditAttribution: jessebeach commentedRerolled. The patch was looking for
which changed to
recently (8 days ago) in 9d5aefb739809c20da3947a1609ddb364f12c72d.
Comment #33
klausiDid you upload the wrong interdiff?
Anyway, patch looks identical otherwise, assuming the testbot won't disagree.
Comment #34
jessebeach CreditAttribution: jessebeach commentedThe interdiff is just the rejected bit that I fixed.
Comment #35
Xano32: rest-csrf-2099439-32.patch queued for re-testing.
Comment #36
alexpottCommitted 423c075 and pushed to 8.x. Thanks!
Removed some unnecessary use statements due to everything being in the same namespace during commit