Skip to content
This repository was archived by the owner on Jun 30, 2021. It is now read-only.

Conversation

@bluthen
Copy link

@bluthen bluthen commented Aug 24, 2017

We shouldn't add Content-Length header if we are doing a chunked transfer. It is fine on most browser but some, like Safari, have issues with it.

See:
https://stackoverflow.com/questions/3304126/chunked-encoding-and-content-length-header
http://greenbytes.de/tech/webdav/rfc2616.html#rfc.section.4.4 (point 3)

@bluthen bluthen changed the title Does not add content-length if there is already a chunk transfer-enco… Do not add content-length if chunk reply. Aug 24, 2017
@NathanFrench
Copy link
Collaborator

NathanFrench commented Aug 27, 2017

After looking over things, I decided to rework a bunch of stuff related to this.

From rfc7230 section 3.3.2 (https://tools.ietf.org/html/rfc7230#section-3.3.2), there are a lot of things we seem to be missing:

   A server MUST NOT send a Transfer-Encoding header field in any
   response with a status code of 1xx (Informational) or 204 (No
   Content).  A server MUST NOT send a Transfer-Encoding header field in
   any 2xx (Successful) response to a CONNECT request
   A server MUST NOT send a
   response containing Transfer-Encoding unless the corresponding
   request indicates HTTP/1.1 (or later).
   A server MAY send a Content-Length header field in a response to a
   HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send
   Content-Length in such a response unless its field-value equals the
   decimal number of octets that would have been sent in the payload
   body of a response if the same request had used the GET method.

And one that seems to relate the most to this patch

   A sender MUST NOT send a Content-Length header field in any message
   that contains a Transfer-Encoding header field.

If we look at https://github.com/criticalstack/libevhtp/blob/develop/parser.c#L1535 I'd say we could use this information, expose a header type enum, and we don't have to strcmp twice.

Thank you for pointing this out, I have a bit of work ahead of me :)

@NathanFrench NathanFrench added this to the v1.2.13 milestone Nov 2, 2017
@NathanFrench NathanFrench removed this from the v1.2.13 milestone Nov 14, 2017
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants