Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle multiple Content-Length headers with the same value #92

Closed
sethmlarson opened this issue Jan 8, 2020 · 4 comments · Fixed by #109
Closed

Handle multiple Content-Length headers with the same value #92

sethmlarson opened this issue Jan 8, 2020 · 4 comments · Fixed by #109

Comments

@sethmlarson
Copy link
Member

From RFC 7230 Section 3.3.2:

If a message is received that has multiple Content-Length header
   fields with field-values consisting of the same decimal value, or a
   single Content-Length header field with a field value containing a
   list of identical decimal values (e.g., "Content-Length: 42, 42"),
   indicating that duplicate Content-Length header fields have been
   generated or combined by an upstream message processor, then the
   recipient MUST either reject the message as invalid or replace the
   duplicated field-values with a single valid Content-Length field
   containing that decimal value prior to determining the message body
   length or forwarding the message.

Currently h11 will error out if multiple Content-Length headers are sent even when equal in value. Perhaps we should collapse the multiple headers into only one or allow the duplicate to pass through if the values are the same?
@njsmith
Copy link
Member

njsmith commented Jan 8, 2020

This is one of those places where we definitely should do that if it's necessary for real world interoperability, but I'm reluctant to jump through hoops for it unless it is necessary for real world interoperability.

Do you know of cases where folks are relying on duplicate identical content-lengths being accepted? Are you filing this because you ran into a case where the current behaviour broke something...?

@sethmlarson
Copy link
Member Author

User reported this issue on the HTTPX issue tracker: encode/httpx#740
Here's the IP address that reports two identical Content-Length headers: http://213.13.25.78

@njsmith
Copy link
Member

njsmith commented Jan 9, 2020

Yep, I see it:

~$ echo -e 'GET / HTTP/1.1\r\nHost: 213.13.25.78\r\n\r\n' | nc 213.13.25.78 80
HTTP/1.1 403 Forbidden
Server: nginx/1.0.4
Content-Type: text/html
Cache-Control: no-cache
Content-Length: 168
Accept-Ranges: bytes
Date: Wed, 08 Jan 2020 23:27:25 GMT
X-Varnish: 675632308
Age: 0
Via: 1.1 varnish
Connection: keep-alive
Content-Length: 168

<html>
<head><title>403 Forbidden</title></head>
<body bgcolor="white">
<center><h1>403 Forbidden</h1></center>
<hr><center>nginx/1.0.4</center>
</body>
</html>

Also, some websearching suggests that Chrome actually shipped some versions that errored out on pages like that (see e.g. https://varnish-cache.org/lists/pipermail/varnish-bugs/2010-October/003222.html), but Chrome no longer errors out on that site, so they must have explicitly decided to revert that change for better interoperability.

So, ugh, fine, I guess we should support it too.

I guess supporting it for our own content-length tracking is straightforward enough. Some questions:

  • Should we error out if someone tries to send duplicate content-length headers?
  • Should we normalize content-length headers to remove this junk when it happens, so that it can't confuse h11 users?

@sethmlarson
Copy link
Member Author

I'm thinking the following for those questions:

  • Probably, discourages behavior that's obviously wrong and potentially breaks receivers.
  • I'm thinking normalize. We can normalize the following:
Content-Length: x
Content-Length: x

and

Content-Length: x, x, x, ...

into Content-Length: x

and still error on Content-Length: x, y, or multiple headers with different values.

@StephenBrown2 StephenBrown2 mentioned this issue Jan 9, 2020
Closed
This was referenced Jan 15, 2020
Closed
Open
@tomchristie tomchristie mentioned this issue Jan 16, 2020
Closed
@pgjones pgjones mentioned this issue Oct 4, 2020
Merged
@simon-mo simon-mo mentioned this issue Nov 18, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@njsmith @sethmlarson