[v12.x] http: fix monkey-patching of http_parser

[Jimbly]

Fixes inability to monkey-patch the HTTP Parser on Node v12.x. This is not an issue on Node v13+ since the parser was renamed back to the old (already monkey-patchable) http_parser name, so I made this PR to the v12.x branch, but I'm unclear as to whether or not that's the right move. However the test in master could also use updating so this doesn't break again (I'll send a separate PR for that).

Fixes: creationix/http-parser-js#65

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[addaleax]

I guess this okay, but understand that this kind of monkey-patching is generally not supported or encouraged…

[Jimbly]

Understood! However, since Node's HTTP parser historically throws process-killing exceptions, or at least fails to parse, when encountering slightly malformed HTTP, it's pretty much a requirement if you want to do any website scraping or indexing using Node (and still take advantage of the robust NPM ecosystem that obviously calls directly into http and such). No one expects Node's internal modules to follow any kind of stable API, because that'd be unreasonable to support, but I think as long as the http parser is a stand-alone module, it should be replaceable.

[addaleax]

@Jimbly Right – but in that case it would be much nicer to know what the parser fails to parse, and figure out a way to make that usable through the supported API.

[Jimbly]

There's lots of stuff that the current parser fails to parse, but it fails to parse it because it is technically invalid HTTP. For security and stability reasons, I think it's probably important that Node's HTTP parser only accepts actually valid HTTP (at least when parsing requests - people almost entirely leverage http-parser-js when dealing with invalid responses) - alternative is to add a lenient mode to it, but having a fast valid parser as the core is way more important, especially if a non-spec-conforming, lenient parser can live in user-land.

[addaleax]

@Jimbly Yeah, that lenient mode would probably be a good idea.

Or to put it another way: Monkey-patching is simply not a viable long-term solution for this problem. This PR is a short-term workaround, and code will be broken when people hook into private, internal APIs.

[BridgeAR]

@Jimbly I agree with @addaleax that adding a lenient mode would probably be best. It would be nice to just switch to that parser in case it's required for individual http calls. That should make it pretty straight forward for everyone who has to deal with broken http code.

[Jimbly]

code will be broken when people hook into private, internal APIs

Of course! http-parser-js has broken with every major node release in the past almost a decade, but that's to be expected. Luckily the HTTP parser has a super-narrow API, so it' never been more than an hour or so of work (plus, occasionally, many hours dealing with making PRs to Node ; ).

If someone wants to add a lenient mode to the parser in core node, I can happily gather up which of the tests fail on Node (basically anything added to that tree without a commit of "update for node v4", etc). But I certainly don't have the bandwidth to tackle adding it myself, nor would I want to go anywhere near trying to add complexity to code presumably in Node's critical performance path.

[addaleax]

If someone wants to add a lenient mode to the parser in core node, I can happily gather up which of the tests fail on Node (basically anything added to that tree without a commit of "update for node v4", etc).

I think that would be a huge help, if you’re willing to do that.

But I certainly don't have the bandwidth to tackle adding it myself, nor would I want to go anywhere near trying to add complexity to code presumably in Node's critical performance path.

Yeah, that’s fine :)

[Jimbly]

@addaleax Here ya go!: https://github.com/creationix/http-parser-js/blob/master/tests/README.md

Seems the new Node HTTP parser gracefully handles a lot more things. At least, none of the tests are crashing the Node process anymore, and a few of the old tests now mostly work (bad headers turning into empty strings in the headers list, but I'm sure that's fine). Only two real issues (at least, that we have tests for - but usually we try to add a test if a user reports something) - dealing with ambiguous chunked encoding + content-length and users opting in to allowing headers larger than the old 80KB limit (now 8KB limit in Node v12+, it seems).

[addaleax]

Thank you! I’m not sure, but @indutny – would you be willing/able to help with some of the changes that would have to go into llhttp for a lenient mode? I’d probably be able to do that, but you already know your way around that codebase. (There’s at least one TODO commment in http.ts that indicates that a lenient mode might be a reasonable idea for the parser.)

[bdurrani]

I dont mean to hijack this thread, but another case @Jimbly 's parser handles is http responses where the content-length header is duplicated, with the same value. I didn't see this case mentioned explicitly in the test notes above, so I thought I would mention it.

Here is what the curl output looks like for such a case

curl http://localhost:3000 -v
* Rebuilt URL to: http://localhost:3000/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.58.0
> Accept: */*
>
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Content-Type: text/html
< Content-Length: 27
< Content-Length: 27
<
* Closing connection 0

Browsers seem to handle this fine. Node's http parser crashes with a code: HPE_UNEXPECTED_CONTENT_LENGTH and with the (correct) reason: 'Duplicate Content-Length'

I believe this case should be reasonable with the actual content length values are correct.

A lenient mode would be greatly appreciated. I would love to help with this effort, if that is ok. I can imagine this mode is something I would enable on a case-by-case basis, not enabled the whole time.

[Jimbly]

@bdurrani You're not hijacking at all, at least, no more than it already was, but that's mostly my fault =). I forgot about that case! I've added it to the http-parser-js test suite, and added it to the failing tests list. Someone should probably go back through the closed issues and see what else was fixed without ever being added to the tests =). Just note though, HPE_UNEXPECTED_CONTENT_LENGTH is from the legacy parser (Node v11 and lower, I think), however the new parser also errors on this, and just gives a generic "Parse Error" in this case.

[indutny]

@addaleax I'll be happy to help you. Feel free to send me questions over email, IRC, or on a github issue.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26469/

[MylesBorins]

landed in 54635f5