http: normalize CONNECT request paths

[efekrskl]

Fixes #34347

Partially a revival of #34412 which was apparently moving in the right direction but got stalled and closed

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.28%. Comparing base (a46087d) to head (7cb4008).
⚠️ Report is 1102 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/_http_client.js	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64876      +/-   ##
==========================================
- Coverage   90.29%   90.28%   -0.02%     
==========================================
  Files         760      790      +30     
  Lines      247061   271991   +24930     
  Branches    46584    51924    +5340     
==========================================
+ Hits       223096   245574   +22478     
- Misses      15437    16906    +1469     
- Partials     8528     9511     +983     

Files with missing lines	Coverage Δ
lib/_http_client.js	97.38% <94.44%> (-0.26%)	⬇️

... and 411 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pimterry]

Sorry it's taken me so long to get to this. I've put some comments here, but I think generally I'm -0.5 on this. As is, I think it can be a breaking change, and the upside is quite small.

More broadly, it changes the scope of HTTP validation we do in these APIs. Right now we don't do anything similar for other methods - we enforce syntactic correctness (no unescaped spaces, must be correctly framed & parseable) but we don't generally police anything else beyond that. If you want to send random strings as a cookie header, or send GET ??? or Host: ??? then you can (I just tested). We make sure your data gets delivered in an unambiguous form, we leave the interpretation errors to the remote server.

We could change that, it's an interesting idea, but if we're going to break this we might as well do something much larger. And to be honest I think it's not helpful: there's plenty of use cases for sending weird HTTP, notably including testing that your server correctly rejects it. This fits into a broader discussion about node:http vs Fetch APIs, where I think we're slowly aligning towards supporting parallel raw low-level with node:http vs high-level with guardrails fetch.

I think there's a central core we can do here safely and sensibly, roughly: if you specifically pass a URL as the request target and the method is CONNECT, then use the path without any leading slash as the target. No new errors or further validation. The risk of breakage there I think is much smaller (URL usage like this is quite unusual anyway, the correct behaviour was always ambiguous, and it'd only break for servers who exclusively accept the wrong format) and it solves the original issue. What do you think @efekrskl?

[efekrskl]

@pimterry thanks for the detailed review! Good points, and I agree.

I've narrowed down the change quite a bit to only strip the leading slash from URL derived CONNECT targets. This should keep the fix as surgical as possible while still solving the original issue.

[pimterry]

LGTM, thanks @efekrskl!

[nodejs-github-bot]

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

[github-actions]

Failed to resume CI

✖ CI job 77796 is not resumable

Full Auto Start CI output

�[36m⠋�[39m Validating Jenkins credentials
�[36m⠋�[39m Validating Jenkins credentials
✔  Jenkins credentials valid
�[36m⠙�[39m Looking for CI runs for pull request 64876
�[36m⠙�[39m Looking for CI runs for pull request 64876
�[36m⠙�[39m Getting PR from nodejs/node/pull/64876
�[36m⠙�[39m Getting reviews from nodejs/node/pull/64876
�[36m⠙�[39m Getting comments from nodejs/node/pull/64876
✔  Found PR CI job 77796
�[36m⠹�[39m Querying data for job/node-test-pull-request/77796/
�[36m⠹�[39m Querying data for job/node-test-pull-request/77796/
�[36m⠹�[39m Querying API for job/node-test-pull-request/77796/
✔  Build data downloaded
   ✖  CI job 77796 is not resumable

View workflow run

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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