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.

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

lowercase the content-length header #120

Merged
merged 2 commits into from Mar 4, 2016
Merged

lowercase the content-length header #120

merged 2 commits into from Mar 4, 2016

Conversation

@kturney
Copy link
Contributor

kturney commented Mar 4, 2016

I ran into duplicate content-lengths being set when h2o2 passThrough is set to true and the payload being proxied through is a buffer. content-length is being proxied and then wreck sets Content-Length. I guess this would be fine except we use nock for testing and it tries to lowercase all the header keys manually and blows up on duplicates.

I guess another solution could be to not set content-length if it is already set.

Kyle Turney
@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 4, 2016

@kturney I see the line you are talking about https://github.com/pgte/nock/blob/master/lib/common.js#L209-L211

https://tools.ietf.org/html/rfc7230#section-3.2

A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).

I think the best solution is to do what you suggest, if the header doesn't exist, then we set it. Can you update your PR to check for a content length first, then set it if it doesn't exist?

@geek geek added the bug label Mar 4, 2016
@kturney

This comment has been minimized.

Copy link
Contributor Author

kturney commented Mar 4, 2016

The test I added keeps coverage at 100%, but I'm not sure it does a great job of actually checking that options['content-length'] is being used instead of the buffer length. If I pass an inaccurate number in options to make sure that is where content-length is coming from, then I get a socket hangup.

@geek geek added this to the 7.0.2 milestone Mar 4, 2016
@geek geek self-assigned this Mar 4, 2016
geek added a commit that referenced this pull request Mar 4, 2016
lowercase the content-length header
@geek geek merged commit 42bde18 into hapijs:master Mar 4, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 5, 2016

@kturney see #121 for the case-insensitive check

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 5, 2016

This is now published @kturney

@kturney

This comment has been minimized.

Copy link
Contributor Author

kturney commented Mar 8, 2016

@geek sweet! Thanks for your help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.