[mcollina]

• reject non-latin1 header values in HTTP/2 client requests
• reject unescaped characters in the HTTP/2 client :path pseudo-header

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/http2
• @nodejs/net

[pimterry]

There's merge conflicts here.

I think to go forward this should align with #63571 (just about to merge right now). We'd want a matching httpValidation option for HTTP/2 requests, so we can add strict behaviour like this (reject everything the RFC calls invalid) but keep relaxed validation for fetch/WHATWG compatibility.

I think the path validation is correct as-is, but for header values this PR rejects control codes like 0x01–0x08 while browser fetch does allow those: WHATWG excludes only >0xff, 0x0, and newlines (https://fetch.spec.whatwg.org/#header-value). Totally on board with the goal here, it would be good to follow the HTTP RFCs strictly by default for Node in general, as long as we keep the door open for that relaxed behaviour for fetch use cases.

For HTTP/2 we already have a related option: strictSingleValueFields - if we have a relaxed option, I'd suggest subsuming that into it as well (i.e. disable it by default when relaxed, make options mutually exclusive, eventually aim to deprecate the standalone options). We have strictFieldWhitespaceValidation too but I think that is explicitly malformed & invalid. I'm not sure if it's actually insecure, but imo we shouldn't enable for relaxed mode.

[mcollina]

I think to go forward this should align with #63571 (just about to merge right now). We'd want a matching httpValidation option for HTTP/2 requests, so we can add strict behaviour like this (reject everything the RFC calls invalid) but keep relaxed validation for fetch/WHATWG compatibility.

The linked PR is this PR. Which one you meant?

[pimterry]

😂 ah sorry! I meant #61597

[codecov]

Codecov Report

❌ Patch coverage is 76.47059% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.74%. Comparing base (128f247) to head (cfef7fb).
⚠️ Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	33.33%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63571      +/-   ##
==========================================
+ Coverage   90.69%   90.74%   +0.04%     
==========================================
  Files         797      798       +1     
  Lines      279611   280152     +541     
  Branches    53787    53912     +125     
==========================================
+ Hits       253603   254226     +623     
+ Misses      16295    16221      -74     
+ Partials     9713     9705       -8     

Files with missing lines	Coverage Δ
lib/internal/http2/util.js	93.13% <100.00%> (+0.20%)	⬆️
lib/internal/http2/core.js	94.65% <33.33%> (-0.28%)	⬇️

... and 80 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.