[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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.69%. Comparing base (128f247) to head (57788f4).
⚠️ Report is 70 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63571      +/-   ##
==========================================
- Coverage   90.69%   90.69%   -0.01%     
==========================================
  Files         797      801       +4     
  Lines      279611   281905    +2294     
  Branches    53787    54329     +542     
==========================================
+ Hits       253603   255676    +2073     
- Misses      16295    16363      +68     
- Partials     9713     9866     +153     

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	94.95% <100.00%> (+0.02%)	⬆️
lib/internal/http2/util.js	93.13% <100.00%> (+0.20%)	⬆️

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