Content-length not set for HTTP methods expecting body when body is None

[demianbrecht]

BPO	23539
Nosy	@orsenthil, @bitdancer, @berkerpeksag, @vadmium, @demianbrecht
Files	issue23539-py27.patch issue23539-py27.patch issue23539-py27.patch issue23539-py3.patch issue23539-py27.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-03-22.19:21:10.807>
created_at = <Date 2015-02-27.17:30:43.379>
labels = ['type-bug', 'library']
title = 'Content-length not set for HTTP methods expecting body when body is None'
updated_at = <Date 2015-03-22.19:39:09.799>
user = 'https://github.com/demianbrecht'

bugs.python.org fields:

activity = <Date 2015-03-22.19:39:09.799>
actor = 'jimr'
assignee = 'none'
closed = True
closed_date = <Date 2015-03-22.19:21:10.807>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2015-02-27.17:30:43.379>
creator = 'demian.brecht'
dependencies = []
files = ['38267', '38270', '38276', '38569', '38570']
hgrepos = []
issue_num = 23539
keywords = ['patch']
message_count = 36.0
messages = ['236803', '236812', '236815', '236817', '236822', '236831', '236832', '236840', '236847', '236850', '236863', '236893', '236912', '236917', '236929', '236930', '236967', '236970', '236971', '237049', '237065', '237066', '237067', '237071', '237075', '237171', '237177', '238017', '238025', '238040', '238570', '238573', '238604', '238926', '238927', '238930']
nosy_count = 7.0
nosy_names = ['orsenthil', 'r.david.murray', 'python-dev', 'berker.peksag', 'martin.panter', 'demian.brecht', 'jimr']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23539'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[demianbrecht]

bpo-14721 solved setting the Content-Length header for 0-length bodies. However, it doesn't account for cases where body is None (reported by James Rutherford here: http://bugs.python.org/issue14721#msg236600).

One method of solving this might be something like this:

_METHODS_EXPECTING_BODIES = {'OPTIONS', 'POST', 'PUT', 'PATCH'}
if method.upper() in _METHODS_EXPECTING_BODIES and \
        'content-length' not in header_names:
    self._set_content_length(body)

(_set_content_length would have to be updated in order to allow for None)

This ensures that Content-Length will not be set for methods not expecting a body.

RFC 7230, Section 3.3.2:

A user agent SHOULD NOT send a Content-Length header field when the
request message does not contain a payload body and the method
semantics do not anticipate such a body.

[jimr]

Thanks for setting up the new issue, I'll cook up a patch. I'm assuming this affects all Python 3.X versions but I've specifically encountered it on Python 2.7.

[demianbrecht]

I'm assuming this affects all Python 3.X versions but I've specifically encountered it on Python 2.7.

Unless there are any core dev objections, I think it's applicable to 2.7, 3.4 and 3.5 as other minor 3.x versions are in security mode (https://docs.python.org/devguide/devcycle.html#security-branches).

[jimr]

OK, thanks.

[jimr]

OK, I've got a patch but it's failing on 'test_send_file'[1], which is sending a body on a GET request. According to the IETF memo[2]:

Bodies on GET requests have no defined semantics. Note that sending
a body on a GET request might cause some existing implementations to
reject the request.

So I don't know whether to remove the method constraint so this test passes, or to "fix" this test to use a different method. My feeling is that stripping out Content-Length for GET requests could cause problems for some code that for whatever reason uses this, so we should remove that constraint (it was only "SHOULD NOT" after all) and go back to the original proposal of just setting the content length for bodies that are None as well as empty strings.

[1] https://hg.python.org/cpython/file/325aec842e3e/Lib/test/test_httplib.py#l297
[2] http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-14#section-7.3

[jimr]

Patch attached for the 2.7 branch, including updated tests. All tests pass. Let me know if this looks like a sensible approach and I'll produce something comparable for 3.X.

The logic now is as it was before, except that we set a content length of zero if the body is None and the method is one of OPTIONS, PATCH, PUT, or POST.

[demianbrecht]

My concern about this is around the combination of the following two passages:

draft-ietf-httpbis-p2-semantics-14, Section 7.3:

Bodies on GET requests have no defined semantics. Note that sending
a body on a GET request might cause some existing implementations to
reject the request.

RFC 7230, Section 3.3.2:

The presence of a message body in a request is signaled by a Content-Length or Transfer-Encoding header field.

My concern around the above is the possibility that servers may have implementations that reject requests with methods that are not expecting bodies solely on the existence of the Content-Length header rather than the value (which may be 0). Without a good amount of research around whether or not this would be an actual problem in practice, I wouldn’t be comfortable adding Content-Length header to every request.

My feeling is that stripping out Content-Length for GET requests could cause problems for some code that for whatever reason uses this

That’s definitely a valid concern. What if the logic was tweaked a little:

expecting_len = method.upper() in _METHODS_EXPECTING_BODIES or \
    body is not None

if expecting_len and 'content-length' not in header_names:
    self._set_content_length(body)

That way, if someone is intentionally sending a body in a GET request, the value of Content-Length would still be automatically added, but would still be set to 0 for all other cases where bodies are expected.

[demianbrecht]

The logic now is as it was before, except that we set a content length of zero if the body is None and the method is one of OPTIONS, PATCH, PUT, or POST.

I see we definitely have similar thinking on the modifications required for this, but I don’t think I’m in favour of the approach in the patch. Consider the case where you have a GET request without a body. Now, instead of simply circumventing the _set_content_length method altogether as would have been done previously, the code path will now go into _set_content_length, have a TypeError exception raised on (str(len([None])) and then an AttributeError exception raised on os.fstat([None].fileno()). That seems to be quite a bit of additional overhead when it could be avoided altogether by an earlier check as I have in my example.

What do you think?

[vadmium]

I like Demian’s “expecting_len” technique slightly better because it would avoid calling None.fileno() and len(None), as Demian just said.

Also, I think OPTIONS should be removed from the list of methods that enforce a Content-Length. I wouldn’t normally expect any payload for OPTIONS, since RFC 7231 explicitly says it does not define a use for a payload, but requires a Content-Type if a payload is sent.

[jimr]

The first patch should actually be modified so the condition reads (update attached):

    if body is None and method_expects_body:
        thelen = 0
    elif body is not None:
        ...

Demian, I believe this is equivalent to your 'expecting_len' proposal, I've just put the logic inside _set_content_length. My approach was to leave as much existing logic as possible in place to avoid the introduction of new bugs (first patch wasn't entirely successful in this!).

As for OPTION,

If the OPTIONS request includes a message-body (as indicated by the
presence of Content-Length or Transfer-Encoding), then the media type
MUST be indicated by a Content-Type field. Although this
specification does not define any use for such a body, future
extensions to HTTP might use the OPTIONS body to make more detailed
queries on the server.

From http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-14#section-7.2

[demianbrecht]

Also, I think OPTIONS should be removed from the list of methods that enforce a Content-Length. I wouldn’t normally expect any payload for OPTIONS, since RFC 7231 explicitly says it does not define a use for a payload, but requires a Content-Type if a payload is sent.

I think Martin's right about this and it's consistent with my concern about servers treating the existence of a Content-Length header as an indicator of a request body.

In any event, with the new logic, a Content-Length header will be added if the body is not None.

I've just put the logic inside _set_content_length

Fair enough. It /does/ encapsulate all logic around setting the content length within the method.

I've left a couple minor comments in Rietveld. In addition to what's there, docs should also be updated to reflect the new behaviour.

Thanks for the work on this!

[jimr]

Happy to remove OPTIONS from the list of methods that gets a content-length where body is None, but do we also want to consider behaviour if it's the empty string? My feeling is that '' implies "present but empty" (so should have a content-length set to zero), whereas None implies "missing" (so should only have a content-length header set to zero if the method is expecting a body.

I've updated the patch with this logic, including ensuring proper explicit test coverage for the cases described. Docs updated too.

[vadmium]

Yes I agree with the behaviour that None means no body (for requests such as GET), and an empty string means an empty but present body. Correct me if I’m wrong, but I think the fix for bpo-14721 already does that.

Perhaps the new behaviour needs a “Changed in version X” tag? I’m not sure, but I tend to think of this as a new feature, rather than a bug fix.