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

Fix possible content-type header duplication #189

Merged
merged 3 commits into from Aug 10, 2017

Conversation

@delaguilaluis
Copy link
Contributor

delaguilaluis commented Aug 9, 2017

Fixing issue #188.

@geek geek self-requested a review Aug 9, 2017
@geek geek self-assigned this Aug 9, 2017
@geek geek added the bug label Aug 9, 2017

return key.toLowerCase() === 'content-length';
});
const hasContentLength = Boolean(internals.findHeader('content-length', uri.headers));

This comment has been minimized.

Copy link
@geek

geek Aug 9, 2017

Member

This changes the meaning a little if content-length is 0. Instead we should

const hasContentLength = (typeof internals.findHeader('content-length', uri.headers) === 'number');

This comment has been minimized.

Copy link
@delaguilaluis

delaguilaluis Aug 9, 2017

Author Contributor

You are right. What do you think about doing

const hasContentLength = internals.findHeader('content-length', uri.headers) !== undefined;

?

This comment has been minimized.

Copy link
@geek

geek Aug 9, 2017

Member

That should be fine, I doubt we have to deal with a null... why not number?

This comment has been minimized.

Copy link
@delaguilaluis

delaguilaluis Aug 9, 2017

Author Contributor

Just to determine the cases where this should be false based on a value and not on its type.

At the end this variable answers the question "does it has content-length?" not "does it has a content-length which type is number?"

internals.findHeader = function (headerName, headers) {

const foundKey = Object.keys(headers)
.find((key) => key.toLowerCase() === headerName);

This comment has been minimized.

Copy link
@delaguilaluis

delaguilaluis Aug 9, 2017

Author Contributor

I will update this to compare the lower case version of both values.

@geek geek added this to the 12.2.3 milestone Aug 10, 2017
@geek geek merged commit b0dfc0e into hapijs:master Aug 10, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.