Skip to content

http: validate non-link headers in writeEarlyHints#62017

Open
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:http-early-hints-validate-headers
Open

http: validate non-link headers in writeEarlyHints#62017
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:http-early-hints-validate-headers

Conversation

@mcollina
Copy link
Member

Summary

  • Validate header names and values for non-link hints in writeEarlyHints() using validateHeaderName/validateHeaderValue, consistent with all other header-writing paths in the HTTP stack
  • Add assertValidHeader() to the HTTP/2 compat layer for defense in depth
  • Add tests for both HTTP/1.1 and HTTP/2

Previously, only the link hint was validated via validateLinkHeaderValue(), while all other hints were concatenated directly into the response without any character validation.

Test plan

  • test/parallel/test-http-early-hints-invalid-argument.js — validates ERR_INVALID_HTTP_TOKEN for bad header names and ERR_INVALID_CHAR for CRLF in values
  • test/parallel/test-http2-compat-write-early-hints-invalid-header.js — validates ERR_INVALID_HTTP_TOKEN for bad header names and ERR_HTTP2_INVALID_HEADER_VALUE for bad values
  • Existing early hints tests still pass

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Feb 27, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens writeEarlyHints() by validating non-link headers (names + values) before emitting 103 Early Hints, aligning behavior with other header-writing code paths across the HTTP stack.

Changes:

  • Add validateHeaderName() / validateHeaderValue() checks for non-link early hints in the HTTP/1.1 server response path.
  • Add assertValidHeader() checks for non-link early hints in the HTTP/2 compat server response path.
  • Add/extend parallel tests covering invalid early-hints header name/value handling for HTTP/1.1 and HTTP/2.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
lib/_http_server.js Validates non-link early-hints header names/values before writing the 103 response.
lib/internal/http2/compat.js Validates non-link early-hints headers in the HTTP/2 compat layer before sending informational headers.
test/parallel/test-http-early-hints-invalid-argument.js Adds assertions for invalid early-hints header names and CRLF in values (HTTP/1.1).
test/parallel/test-http2-compat-write-early-hints-invalid-header.js Adds assertions for invalid early-hints header names and invalid values (HTTP/2 compat).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mcollina mcollina force-pushed the http-early-hints-validate-headers branch 2 times, most recently from 7fd2181 to bf5cf85 Compare February 27, 2026 11:31
Validate header names and values for non-link hints passed to
writeEarlyHints() using validateHeaderName/validateHeaderValue,
consistent with all other header-writing paths in the HTTP stack.

Previously, only the `link` hint was validated via
validateLinkHeaderValue(), while all other hints were concatenated
directly into the response without any character validation.

Also add assertValidHeader() to the HTTP/2 compat layer for
defense in depth.
@mcollina mcollina force-pushed the http-early-hints-validate-headers branch from bf5cf85 to 69e6ce4 Compare February 27, 2026 12:36
@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (38a6da5) to head (69e6ce4).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/http2/compat.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62017      +/-   ##
==========================================
- Coverage   89.74%   89.73%   -0.01%     
==========================================
  Files         676      676              
  Lines      206070   206077       +7     
  Branches    39517    39523       +6     
==========================================
- Hits       184928   184921       -7     
- Misses      13300    13310      +10     
- Partials     7842     7846       +4     
Files with missing lines Coverage Δ
lib/_http_server.js 97.47% <100.00%> (+0.22%) ⬆️
lib/internal/http2/compat.js 97.05% <80.00%> (+0.11%) ⬆️

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

@mcollina mcollina requested a review from RafaelGSS February 27, 2026 17:49
@lpinca
Copy link
Member

lpinca commented Feb 27, 2026

semver-major?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants