http: add CONNECT method handling for default Host header when using proxy

[Archkon]

Fixes: #63945

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[pimterry]

Hi @Archkon! Thanks for this PR. It looks like there's a failing test (which pins the previous incorrect output) and the commit message is failing the linting. You'll need to fix the test, and update the commit message (git commit -s --amend) to fix the issues, and then force push to update this.

Otherwise the code looks good to me. My main concern is whether this is a breaking change: I agree the current behaviour is wrong, but it I'm not sure if changing it is likely to cause issues elsewhere in code that now expects this wrong Host value (even if the RFCs says they shouldn't) e.g. our own test suite.

What do @nodejs/http think? AFAICT the current behaviour has been in place for more than a decade. Tempted to lean semver-major just in case, even though it's fundamentally really a bug fix.

[Archkon]

Hi @Archkon! Thanks for this PR. It looks like there's a failing test (which pins the previous incorrect output) and the commit message is failing the linting. You'll need to fix the test, and update the commit message (git commit -s --amend) to fix the issues, and then force push to update this.

Otherwise the code looks good to me. My main concern is whether this is a breaking change: I agree the current behaviour is wrong, but it I'm not sure if changing it is likely to cause issues elsewhere in code that now expects this wrong Host value (even if the RFCs says they shouldn't) e.g. our own test suite.

What do @nodejs/http think? AFAICT the current behaviour has been in place for more than a decade. Tempted to lean semver-major just in case, even though it's fundamentally really a bug fix.

I just feel that many framework developers aren’t aware of this underlying bug, because they assume CONNECT method will ensure RFC-compliant handling.

[mcollina]

@pimterry we use the "baking for lts" label and ship, that allows us to asses ecosystem impact.

[mcollina]

lgtm

[nodejs-github-bot]

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

[Archkon]

lgtm

Rebased this PR on the latest nodejs:main and resolved the conflict with #64108
@mcollina Could you please trigger a fresh CI run by applying request-ci? Thanks !