fix(fetch): handle URL credentials in dispatch path extraction#4892
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4892 +/- ##
=======================================
Coverage 92.89% 92.89%
=======================================
Files 112 112
Lines 35635 35638 +3
=======================================
+ Hits 33102 33106 +4
+ Misses 2533 2532 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
230ae5a to
79bd034
Compare
|
@KhafraDev @domenic can you urgently review this? I need to land asap, ship a minor to be included in the upcoming Node 24 security release |
domenic
left a comment
There was a problem hiding this comment.
Code LGTM, left some nits on the tests
| test('fetch path extraction does not match hostnames inside scheme', async (t) => { | ||
| const hosts = ['h', 't', 'p', 'ht', 'tp', 'tt'] | ||
|
|
||
| for (const scheme of ['http', 'https']) { |
There was a problem hiding this comment.
You could add for (const userinfo of ['', 'user:pass@']) and then update the URL to ${scheme}://${userinfo}${host}/test?a=b#frag for extra coverage.
There was a problem hiding this comment.
Not sure I understand the motivation behind changing this file.
Summary
?behavior introduced in fix fetch stripping trailing ? from url #4837Details
Using
url.href.slice(url.origin.length)breaks when userinfo is present (user:pass@...), becauseoriginexcludes credentials whilehrefincludes them. That produces an invalid path and prevents dispatch.This change builds the path from
url.pathname + url.searchand explicitly preserves trailing?when present.Test plan
node --test test/websocket/issue-4889.jsnpx borp -p "test/fetch/issue-4836.js"npx eslint lib/web/fetch/index.js test/websocket/issue-4889.jsCloses #4889.