X Tutup
The Wayback Machine - https://web.archive.org/web/20210120175411/https://github.com/nodejs/http-parser/pull/460
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

Allow duplicated Content-Length with the same value #460

Open
wants to merge 2 commits into
base: master
from

Conversation

@obatysh
Copy link

@obatysh obatysh commented Dec 20, 2018

No description provided.

@indutny
Copy link
Member

@indutny indutny commented Dec 21, 2018

Hello!

Thank you for submitting this patch. Wouldn't allowing duplicate content-length header be a violation of protocol specification?

@obatysh
Copy link
Author

@obatysh obatysh commented Dec 22, 2018

RFC https://tools.ietf.org/html/rfc7230#page-31 says that duplicate Content-Length fields with the same decimal value can be either rejected as invalid or accepted by replacing duplicate with a single valid Content-Length field. So, it is not a violation, it is one of the possible options.
Not sure about "replacement" implementation, maybe this pull-request should be improved not to report the Content-Length field second time.

@indutny
Copy link
Member

@indutny indutny commented Dec 22, 2018

Good point!

I'd love to hear opinion of @nodejs/http on that? Would we prefer to reject it or parse it?

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Dec 23, 2018

it's worth noting that this costs an additional 8 bytes per struct http_parser

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 23, 2018

Yes. That means it breaks ABI and can't land in a v2.x release. See discussion in #435.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.
X Tutup