deps,test: fix undici globals on frozen globalThis#64622
Closed
mhayk wants to merge 1 commit into
Closed
Conversation
When Object.freeze(globalThis) is called before any undici-backed global
(fetch, WebSocket, Response, etc.) is accessed, the lazy initialisation
in setGlobalDispatcher throws:
TypeError: Cannot define property Symbol(undici.globalDispatcher.2),
object is not extensible
This is a regression introduced when globals were made lazy-loaded
(nodejs#45659). The Node.js security best practices guide explicitly
recommends Object.freeze(globalThis) as a monkey-patching defence
(CWE-349), so this failure is particularly unfortunate.
Fix: wrap the Object.defineProperty calls in setGlobalDispatcher with a
try/catch. When globalThis is not extensible the dispatcher is stored in
a module-level fallback variable. getGlobalDispatcher is updated to
return globalThis[symbol] ?? fallbackDispatcher so that the normal path
(extensible globalThis) is unchanged.
The bundled deps/undici/undici.js is updated to match.
A test is added in test/parallel/test-undici-frozen-globalthis.js that
freezes globalThis before accessing any of the affected globals.
Fixes: nodejs#46788
Collaborator
|
Review requested:
|
Contributor
|
This change should go to - https://github.com/nodejs/undici, and once it has a new version with your changes merged, it will be included here. |
Author
|
Thanks for the feedback! You're absolutely right, the fix should go upstream first. I'll close this PR and create a PR in the undici repository with the same fix. Once it's merged and a new version is released, it can be integrated into Node.js via the standard dependency update process. Closing in favour of upstream fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
Object.freeze(globalThis)is called before any undici-backed global (fetch,WebSocket,Response, etc.) is accessed for the first time, the lazy initialisation insidesetGlobalDispatcherthrows:This is particularly problematic because the Node.js security best practices guide explicitly recommends
Object.freeze(globalThis)as a defence against monkey-patching (CWE-349).Root cause
setGlobalDispatcherunconditionally callsObject.defineProperty(globalThis, ...). WhenglobalThisis not extensible the call throws instead of degrading gracefully.This became observable after globals were made lazy-loaded in #45659.
Fix
Wrap the
Object.definePropertycalls insetGlobalDispatcherwith atry/catch. WhenglobalThisis not extensible the dispatcher is stored in a module-level fallback variable.getGlobalDispatcheris updated to returnglobalThis[symbol] ?? fallbackDispatcherso the normal (extensible) path is unchanged.deps/undici/undici.js(the bundled file) is updated to match the source change indeps/undici/src/lib/global.js.Test
A new test
test/parallel/test-undici-frozen-globalthis.jsis added that:Object.freeze(globalThis)before any undici globals are touchedfetch,WebSocket,Response,Request,Headers, andFormDataare all accessible without throwingChecklist
make -j<N>build succeeds./node test/parallel/test-undici-frozen-globalthis.jssetGlobalDispatcher/getGlobalDispatcherbehaviour is unchangedFixes #46788