Skip to content

http: validate ClientRequest path on set#62030

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
mcollina:fix/http-client-path-toctou
Mar 2, 2026
Merged

http: validate ClientRequest path on set#62030
nodejs-github-bot merged 1 commit intonodejs:mainfrom
mcollina:fix/http-client-path-toctou

Conversation

@mcollina
Copy link
Member

Summary

  • Add a getter/setter for ClientRequest.path that runs INVALID_PATH_REGEX validation on every assignment, not just at construction time
  • Store the path in an internal symbol (kPath) so the setter is always invoked
  • Prevents invalid characters (CRLF, null bytes, etc.) from being set on req.path after construction

Test plan

  • Added test/parallel/test-http-client-path-toctou.js verifying:
    • Setting req.path with \r\n, \r, \n, or \0 throws ERR_UNESCAPED_CHARACTERS
    • Path remains unchanged after a failed mutation
    • Valid path reassignment still works
  • All existing test-http-client-* tests pass (66/66)

The `path` property on `ClientRequest` was only validated at
construction time. Add a getter/setter so that the same
`INVALID_PATH_REGEX` check runs whenever `req.path` is reassigned,
preventing invalid characters from reaching `_implicitHeader()`.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

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

codecov bot commented Feb 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.65%. Comparing base (38a6da5) to head (951a901).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62030      +/-   ##
==========================================
- Coverage   89.74%   89.65%   -0.09%     
==========================================
  Files         676      676              
  Lines      206070   206249     +179     
  Branches    39517    39509       -8     
==========================================
- Hits       184928   184908      -20     
- Misses      13300    13466     +166     
- Partials     7842     7875      +33     
Files with missing lines Coverage Δ
lib/_http_client.js 97.43% <100.00%> (+0.04%) ⬆️

... and 46 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 pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2026
@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 2, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 2, 2026
@nodejs-github-bot nodejs-github-bot merged commit acb79bc into nodejs:main Mar 2, 2026
75 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in acb79bc

richardlau pushed a commit that referenced this pull request Mar 2, 2026
The `path` property on `ClientRequest` was only validated at
construction time. Add a getter/setter so that the same
`INVALID_PATH_REGEX` check runs whenever `req.path` is reassigned,
preventing invalid characters from reaching `_implicitHeader()`.

PR-URL: #62030
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
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. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants