http: avoid obscure http parser error on CONNECT.

[bjouhier]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

http

Description of change

Some proxies return a 403 when attempting SSL tunnelling through
a port other than 443. This is reported to the HTTP client as an
obscure "Parse Error" / HPE_INVALID_CONSTANT, which is difficult to
troubleshoot.

This commit improves this by catching status codes >= 400 in CONNECT
responses and by emitting an ECONNREFUSED error with the status code
included in the error message.

[bjouhier]

This is my first dive into low level HTTP parser code so there are probably better ways to fix it. I tried to interfere as little as possible with existing code. One problem is that this fix does not really stop the C++ parser which is still emitting a "Parse Error". I'm preventing this second error to surface to the http client by removing all error listeners after emitting the ECONNREFUSED error. A bit hacky but it allowed me to fix the problem without changing the C++ parser.

Also, I am not sure about the most appropriate error code (ECONNREFUSED?) and error message for this specific error condition.

[rvagg]

@bjouhier putting your description, or a variation of it, into the top of the test file would be good so future readers have a clue why the test was added.

/cc @nodejs/http

[bjouhier]

@rvagg I'll do it.

I also noticed that the description is a bit wrong because I developed the fix against branch v4.x and I cherry-picked to master. In master the obscure error is a hang up, not a parse error. I'll fix that too.

[bnoordhuis]

I don't think this is a proper fix. For starters, it would completely break handling of 407 responses, Proxy Authentication Required.

I think the logic should be:

1. 200 response - tunnel established, emit 'connect' event.
2. Any other response - no tunnel established, treat as a normal HTTP response.

An open question is how to treat 2xx responses != 200.

[bjouhier]

@bnoordhuis This was my other option but it was more complex to implement and it was a behavior change (connect returning a response rather than an error in the 403 case). I had missed the 407 case.

I can attempt to fix it, unless someone else volunteers.

[bnoordhuis]

It will probably end up being a semver-major change, yes, although one could argue the current logic is so broken that any improvement is a bug fix.

[bjouhier]

There is a refactoring on the way (#6533) and I don't have time for the deeper fix. So I'm closing this PR.