v2 room deletion API relies on middleboxes not being confused by DELETE-with-body - suggesting an alternative that doesn't have that risk

[shadowcat-mst]

Note my comments here are based on https://matrix-org.github.io/synapse/develop/admin_api/rooms.html#version-2-new-version as of 2022-06-01 - if things have changed since then and/or I'm looking at the wrong place, my apologies.

Initial worry that led to me filing this: DELETE-with-a-body, while -allowed- by the HTTP spec, worries me reliability wise because given it's basically also -undefined- by the spec I am willing to bet that at least some proxies and other types of middlebox will do something counterproductive with it.

Idea that I think might solve that -and- look more elegant:

There's already a 'delete_status' endpoint that provides all currently-in-flight deletions for a room and additionally a 'delete_status/$deletion_id' endpoint that allows checking a specific one.

It seems to me that this would be a perfect case for a RESTy collection style API - i.e.

Step 1: rename 'delete_status' to 'deletions', otherwise maintaining current semantics

Step 2: switch the current DELETE call to being a POST to 'deletions' (with, otherwise, the same basic structure) to indicate that its goal is to create a new deletion.

Step 2a: Make the POST response be a 'Created' status and provide a Location: header to the 'deletions/$deletion_id' resource that is currently being served as 'delete_status/$deletion_id' (without changing the { deletion_id: $deletion_id } JSON return, mind, that's useful too)

I'm far from a REST zealot, but this seems like both a slightly more elegant API and also one that's more robust against unfortunate middleboxes.

(no reason you couldn't also keep the current DELETE for a while (and delete_status as a URL) to allow clients already using that to switch across, though simply for the reliability reason I'd suggest that if this suggestion if accepted it should be removed before v2 goes gold so everybody these the less-likely-to-break POST .../deletions approach instead)

[dklimpel]

Have a look there:

• The Delete Room API should use the http DELETE method, not POST #9032

[shadowcat-mst]

https://httpwg.org/specs/rfc7231.html#DELETE

"A payload within a DELETE request message has no defined semantics; sending a payload body on a DELETE request might cause some existing implementations to reject the request"

Ergo, unless you can guarantee that there are no proxies between the client and the server, this is simply not safe, and the previous implementation was more robust to the real world and HTTP as specified - and the asynchronous nature where there can be more than one deletion in-flight means that the DELETE verb isn't appropriate anyway, sadly.

Note: I have seen this be a problem in practice for GET requests, which are specified with the exact same language about bodies - hence there now being a draft for a QUERY method that actually specifies semantics for a body: https://www.ietf.org/archive/id/draft-ietf-httpbis-safe-method-w-body-02.html

[reivilibre]

I see where you're coming from and ambiguous specifications are never a pleasure for anyone, but on the other hand I'm not aware of any implementations that reject such a request (there don't appear to be complaints in practice about this from users of the admin API) and ideally we'd have a bit more of a concrete problem than a bet that something will cause trouble. As such this is likely going to be something that needs to come in as a contribution or in a future version of this admin API (in my opinion only).

I also note this from the page you link:

If a DELETE method is successfully applied, the origin server SHOULD send a 202 (Accepted) status code if the action will likely succeed but has not yet been enacted

You could argue that we should be sending a 202 response considering we only schedule a delete rather than do it all there and then :).

I take it by middle box you are mainly talking about reverse proxies/load balancers and the like?

[richvdh]

I think we're agreed not to make any changes here.