Cannot send CSRF token in the request body if using HTTP DELETE
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Daimona
	May 27 2022, 11:03 AM

Description

The DELETE method is listed in Validator::NO_BODY_METHODS, which means that body validation is skipped for it. However, this makes it impossible to provide a CSRF token to any endpoint using DELETE, and I can't think of sensible alternatives.

As far as I can see, no specification forbids a request body for DELETE requests. Some do mention that the behaviour could be undefined, or that the server may reject the request (e.g. RFC 7231); hence, ignoring the body is fine. However, processing the body should also be fine and the CSRF token seem to be a valid use case (at least until we'll have better support for using OAuth tokens instead of cookies). So I'd propose that the request body would not be ignored for DELETE requests.

(As a case in point, here is an endpoint in the CampaignEvents extension that uses DELETE and also needs to check the CSRF token)

Details

	Subject	Repo	Branch	Lines +/-
	Allow REST API delete method request to contain a body	mediawiki/core	master	+7 -3
	Add unit testing for MediaWiki\Rest\Validator	mediawiki/core	master	+142 -0

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved		Daimona	T305697 Add registration header to public event page
		Resolved		BPirkle	T309388 Cannot send CSRF token in the request body if using HTTP DELETE

Event Timeline

Daimona created this task.May 27 2022, 11:03 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 27 2022, 11:03 AM

Daimona mentioned this in T305697: Add registration header to public event page.May 27 2022, 11:04 AM

ldelench_wmf added a project: Campaign-Tools.May 27 2022, 7:47 PM

ldelench_wmf moved this task from Backlog to Following on the Campaign-Tools board.May 27 2022, 7:49 PM

ldelench_wmf subscribed.

ldelench_wmf added a parent task: T305697: Add registration header to public event page.May 30 2022, 1:05 PM

Marking this one "high priority" for Campaigns as it blocks a requirement for our V0 release in July; please let us know if further discussion is needed. cc @LNguyen @BPirkle

For prior WMF usages of combining DELETE and tokens, I found this in RESTBase. I don't fully understand RESTBase, but at a glance this seems to at least not prohibit that combination.

I tried to read up on csrf protection and DELETE, and there are a lot of contradictory opinions online. My understanding thus far is that DELETE is not subject to csrf attacks, because modern browsers won't send a cross-origin DELETE. Unless the server sends headers (specifically Access-Control-Allow-Methods) allowing the brower to do so, in which case it can be. And regardless of what WMF's servers do or don't send, MediaWiki is available for use by third parties whose server configurations we have no control over. And (as is the case here) MediaWiki is generally extendable in ways that might add requirements beyond what the core endpoints need, so the core REST infrastructure should accommodate reasonable use cases.

The question for this task, though isn't specifically csrf (although I understand that's the immediate blocker) but whether DELETE can send a body at all. Mozilla seems to say that DELETE may have a body (without comment on why, or whether it is a good or bad idea). Some random article that Google found says that certain software will reject/ignore/not support bodies on DELETE, so it may be an anti-pattern.

I was briefly concerned about the bullet point in the above linked article that OpenAPI 3 disallowed it, but apparently 3.1 added it back, which also makes me a little more comfortable in not prohibiting it.

From what I've seen so far, my formative opinion is that we should, where possible, avoid DELETE bodies, and work toward a state where they are never necessary in our software. But also that the core REST infrastructure prohibiting it is too strong an opinion, and we should remove that prohibition.

Any strong opinions from others?
@tstarling
@daniel

In T309388#7968143, @BPirkle wrote:

My understanding thus far is that DELETE is not subject to csrf attacks, because modern browsers won't send a cross-origin DELETE.

PUT requests are probably also theoretically safe for similar reasons. However, I think we should still try and enforce anti-CSRF checks, just in case someone comes up with a way to circumvent the existing limitations.

The question for this task, though isn't specifically csrf (although I understand that's the immediate blocker) but whether DELETE can send a body at all. Mozilla seems to say that DELETE may have a body (without comment on why, or whether it is a good or bad idea). Some random article that Google found says that certain software will reject/ignore/not support bodies on DELETE, so it may be an anti-pattern.

Most documents I could find online (like RFC 7231 linked in the task description) either say that the body could be ignored, or that the body of a DELETE request has no defined semantics, but none of them seem to say that it MUST be ignored or disallowed.

I started to implement this, decided to do TDD by writing the test first, looked for the unit test for the Validator class, and didn't find one. Validator is largely a wrapper, so maybe we originally didn't think it necessary. But it seems like the behavior of confirming which methods must or must not have a body is worth testing, especially as we've found it necessary to make changes to that behavior. So I'll add that unit test as part of this task.

Change 802219 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/core@master] Add unit testing for MediaWiki\Rest\Validator

https://gerrit.wikimedia.org/r/802219

gerritbot added a project: Patch-For-Review.Jun 2 2022, 12:30 AM

To be clear, the patch I just uploaded ONLY adds tests for the existing functionality, to establish a baseline. After tests are merged, I'll make the requested change in a separate patch. That way in the unlikely event that the change has to be rolled back, we don't lose the tests. And it is also a bit clearer looking back at git history months from now exactly what was done and why.

Change 802219 merged by jenkins-bot:

[mediawiki/core@master] Add unit testing for MediaWiki\Rest\Validator

https://gerrit.wikimedia.org/r/802219

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.15; 2022-06-06).Jun 3 2022, 7:00 PM

Change 802866 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/core@master] Allow REST API delete method request to contain a body

https://gerrit.wikimedia.org/r/802866