request: read body from file if the method is DELETE

[jd]

Currently Request.from_file ignore the body of a request if the method is
DELETE. This fixes that by using the dictionary we have to know if a request
might have a body or not.

[mmerickel]

As far as I can tell an HTTP DELETE with a body is unspecified and is not supported by many clients [1]. Also you changed some code in webob that explicitly forbade having a body for a DELETE in the http_method_probably_has_body dict. Here is the commit[2] where this logic was added to webob and it states that HTTP DELETE bodies have no defined meaning and thus are ignored in webob.

This PR changes some fundamental parts of the request processing by modifying http_method_probably_has_body, and the only justification here is to allow Request.from_file to work, which isn't part of webob's hot route. I simply don't see a very compelling reason to change this.

[1] http://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request
[2] ee7f027

[jd]

Having a body in DELETE is nowadays often used in REST API, which is my case here. So having WebOb ignoring entirely it just because most client out there don't handle it either sounds like a weak argument.

Though I recognize it does change a previous assumption, but as you said, since it's not in the hot route, I don't think it's a big issue.

[mmerickel]

To be clear, I said you're changing webob's hot route as a side effect of changing Request.from_file. You made this PR claiming to fix Request.from_file when in fact the more important change is that you're affecting how webob handles DELETE requests over the wire. This means when I review the PR and see a fundamental change that you didn't mention as part of the PR I get worried.

Having a body in DELETE is nowadays often used in REST API, which is my case here.

This is not my experience with using REST APIs that support DELETE requests. As you can see in the StackOverflow link I provided, the body is ignored on several platforms and is not merely an issue of client libraries. I realize this SO post is from 2008 and things may have changed since then. However I don't see any movement or updates on RFC2616 which leaves this as undefined behavior. There were apparently some drafts where they made it allowed, but nothing that has been approved.

I personally don't mind if we choose to accept bodies on DELETE requests, but keeping things the way they are is justifiable as well considering it's undefined and webob definitely isn't the only library out there ignoring the body.

Anyway, I'll leave this for other reviewers to decide, but I just wanted to be sure to point out the full extent of what this PR affects in webob if it's accepted to avoid anything slipping through the cracks as it does actually affect how webob handles DELETE requests over the wire.

As a final comment, I think the PR looks good from a code quality perspective if we choose to merge it.

[digitalresistor]

RFC7231 (updates RFC2616):

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.

[digitalresistor]

@jd am I correct in assuming that there is a use case for this in OpenStack?

I personally don't think DELETE should have a body, but I can understand that there may be use cases that I am missing.

I want to do a review of Request and make sure that there are no other assumptions about what has a body and what doesn't.

[jd]

@bertjwregeer You are correct. FWIW, we use Pecan (http://www.pecanpy.org/) which supports DELETE with a body (and alternate POST ?_method=delete form as a fallback) and requests supports it too.

(And while I do now what the RFC says, "undefined" and "might reject" is not "forbidden". :-)

[digitalresistor]

Yeah, the same could be said for a HEAD/GET request for instance, bodies there have no defined semantics either, and some servers may reject it, but I know that ElasticSearch for instance has a body on GET.

I am planning to refactor Request so that every method can have a body. That will solve the problem across the board.

[digitalresistor]

So here's the problem, and why the limits were added in the first place:

1. the WSGI PEP's specify that env['wsgi.input'] should be file like (i.e. have read/readline)
2. The wsgi.input should provide an empty string as the end of input marker

However, wsgiref will pass the underlying file descriptor for the socket the client is connected to through to wsgi.input in the environ.

This means when you call .read() without a CONTENT_LENGTH, which may happen if the client is using Chunked Encoding (and WebOb tries its best to support that), then you and up hanging forever. Good servers like waitress of course don't do this, but we can't rely on the fact that we are going to be running under servers that took a "should" and properly implemented as if it were a "must", and IMHO that is the only right way to implement it, but alas we are stuck with implementations that do the wrong thing.

This patch as it currently stands to add DELETE with body support would require that chunked encoding support would have to be disabled for DELETE so that we don't break any existing consumers of WebOb. It would be all to trivial to use a simple:

curl -X POST http://someurl/

with a WSGI server that has wsgi.input with a read() that hangs to take up resources and potentially cause a Denial of Service.

[digitalresistor]

@jd I have created a PR that supersedes this one: #283