[bugfix] insure proper handling of multiple Content-Length headers in http #4705
Conversation
There was a problem hiding this comment.
From RFC7230
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.
The behaviour in this PR looks to be exactly what the spec suggests.
src/http/common.cr
Outdated
| headers["Content-Length"]?.try &.to_u64? | ||
| length_header = headers.get? "Content-Length" | ||
| return nil unless length_header | ||
| if length_header.size > 1 && length_header.uniq.size != 1 |
There was a problem hiding this comment.
I think a faster implementation would be
length_headers = headers.get? "Content-Length"
return nil unless length_headers
first_header = length_headers[0]
if length_headers.size > 1 && length_headers.any? { |header| header != first_header }
raise ArgumentError.new("Multiple Content-Length headers received did not match: #{length_headers}")
end
first_header.to_u64
src/http/common.cr
Outdated
| length_header = headers.get? "Content-Length" | ||
| return nil unless length_header | ||
| if length_header.size > 1 && length_header.uniq.size != 1 | ||
| raise ArgumentError.new("Multiple different values found for content-length #{length_header}") |
There was a problem hiding this comment.
Perhaps "Multiple Content-Length headers received did not match: #{length_headers}" would be a better message?
| @@ -345,5 +345,16 @@ module HTTP | |||
| request.host_with_port.should eq("host.example.org:3000") | |||
| end | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
I'd suggest adding spec for a throwing case when values do not match.
|
Oh, and by the way it's "ensure" not "insure" :) |
bmmcginty
force-pushed
the
fix_content_length
branch
from
edcff61 to
10614ad
Compare
RX14
added
kind:bug
spec/std/http/request_spec.cr
Outdated
| @@ -345,5 +345,29 @@ module HTTP | |||
| request.host_with_port.should eq("host.example.org:3000") | |||
| end | |||
| end | |||
|
|
|||
| it "doesn't raise on request with multiple Content_length headers" do | |||
| io = IO::Memory.new %(GET / HTTP/1.1 | |||
There was a problem hiding this comment.
Please use heredoc with indentation (easier to read), like:
io = IO::Memory.new <<-REQ
Host: host
etc.
REQ
|
Up 👍 |
|
"asterite requested changes" :-) |
When multiple Content-Length headers are received, error out if all received values do not match. Return a single Content-Length header value.
RX14
force-pushed
the
fix_content_length
branch
from
10614ad to
cbd7d41
Compare
|
Pushed fixes for this, because it stalled. |
When multiple Content-Length headers are received, error out if all received values do not match.
Return a single Content-Length header value.
This seems kind of an odd case, but it was the first thing that happened when adding Crystal to my stack today.
Multiple Content-Length headers, each of the same value, were added.
I've added a spec which fails using the earlier behavior.