make fetch compatible with Object.freeze(globalThis)

[davidje13]

This would solve...

The NodeJS "security best practices" document suggests using Object.freeze(globalThis) to ensure no globals can be replaced, but this is unexpectedly not compatible with the built-in fetch:

# node
Welcome to Node.js v23.5.0.
Type ".help" for more information.
> Object.freeze(globalThis);
[...]
> fetch('https://example.com');
Uncaught:
TypeError: Cannot define property Symbol(undici.globalDispatcher.1), object is not extensible
    at Function.defineProperty (<anonymous>)
    at setGlobalDispatcher2 (node:internal/deps/undici/undici:8241:14)
    at lib/global.js (node:internal/deps/undici/undici:8235:7)
    at __require (node:internal/deps/undici/undici:6:50)
    at node:internal/deps/undici/undici:13478:52
    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:402:7)
    at requireBuiltin (node:internal/bootstrap/realm:433:14)
    at fetch (node:internal/bootstrap/web/exposed-window-or-worker:78:28)

The implementation should look like...

The issue is that undici attempts to add a dispatcher to globalThis:

undici/lib/global.js

Line 17 in bd98a63

Object.defineProperty(globalThis, globalDispatcher, {

(there also seems to be another one for globalOrigin)

This makes sense when undici is used as a userspace library since it avoids extra resource useage due to multiple versions coexisting, but makes less sense when it is part of NodeJS itself. The NodeJS integration should probably side-step this, or alternatively register the necessary globals before handing over to user code.

I have also considered...

This burden could be shifted to the user if there were some way to pre-initialise fetch; then the user could call this before freezing globalThis:

fetch.initialise();
Object.freeze(globalThis);

But this is an unexpected requirement and likely to catch developers out. Better for it to be automatic.

Another alternative would be to update the recommended security practices to suggest a method of freezing the existing properties of globalThis without preventing defining new properties, but I am not aware of a standard way to do this.

[mcollina]

You'd need to call Object.freeze(globalThis) after your application initialization is finished.

In fact, the following should have worked:

import { getGlobalDispatcher, fetch } from './index.js';

getGlobalDispatcher() // just initialize this

Object.freeze(globalThis)

await fetch('https://example.com/')

However, it does not work because AbortController is lazy loaded as well: https://github.com/nodejs/node/blob/0e7ec5e7a1427eb418bf82833a5c308cb8e0ecda/lib/util.js#L463-L468. Therefore:

import { getGlobalDispatcher, fetch } from 'undici.';

AbortController // needed to intialize
getGlobalDispatcher() // needed to initialize the agent

Object.freeze(globalThis)

await fetch('https://example.com/')

This is not perfect.

[mcollina]

I've never run an application with globalThis frozen or with frozen intrinsics, as many things break.

If anyone would like to propose a new design to achieve the same behavior without patching globalThis, I'm happy to review.

cc @nodejs/security

[davidje13]

If anyone would like to propose a new design to achieve the same behavior

Unless I'm missing something, the simplest option would be to use a regular global value:

const { InvalidArgumentError } = require('./core/errors')
const Agent = require('./dispatcher/agent')

let globalDispatcher = new Agent()

function setGlobalDispatcher (agent) {
  if (!agent || typeof agent.dispatch !== 'function') {
    throw new InvalidArgumentError('Argument agent must implement Agent')
  }
  globalDispatcher = agent
}

function getGlobalDispatcher () {
  return globalDispatcher
}

module.exports = {
  setGlobalDispatcher,
  getGlobalDispatcher
}

Originally I was thinking this approach would mean the dispatcher would not be shared between versions of undici when used as a user-space module, but now I realise that the Symbol being used currently is already unique to the file, so that was already the case anyway. Maybe I'm missing something about why globalThis is being used in the first place. edit: thanks targos for pointing out the original code uses Symbol.for not Symbol, so the symbol isn't unique to the file.

[targos]

The value returned by Symbol.for is not unique to the file. It will be the same in all instances of the package.

[mcollina]

also cc @nodejs/security-wg and @joyeecheung. The way we lazy load things for startup performance is at odd with the recommendation at https://nodejs.org/en/learn/getting-started/security-best-practices#monkey-patching-cwe-349. Basically we'd need a process.diligentLoad() to load all lazy things from Node.js (I suspect there are other things in the Web compact that are lazily loaded).

However, I'm more inclined to remove that best practice, as frankly, it does not add much security IMHO, but it's debatable. Or anyhow update the documentation saying that it comes with a lot of caveats.

[joyeecheung]

Basically we'd need a process.diligentLoad() to load all lazy things from Node.js

Not sure if I am following what this suggestion is, are you talking about something similar to pre-execution?

However, I'm more inclined to remove that best practice, as frankly, it does not add much security IMHO, but it's debatable.

I second this. The motivation already is a lost cause as even if you freeze globalThis, the builtin prototypes are still mutable and other code can equally swap them out. Even if you use something like the primordials in Node.js, the Array methods are not hardenable because the spec requires array methods to call getters on the prototype for index access, so anyone can smuggle their code into your app as long as you e.g. arr.push(). Also if it's done at the library level, the library must be loaded before everything else but that needs to be done by consumers instead. A security practice with obvious holes is a misconception that invites vulnerability, as people would make wrong assumptions and trust things they should not trust/develop threat models with holes.

[joyeecheung]

I've never run an application with globalThis frozen or with frozen intrinsics, as many things break.

I have seen apps using it but it's not for defending untrusted code, but for making sure different bundles of code don't step on each other's toes IIRC. Doing it for security so that untrusted code cannot affect your code is a lost cause because of e.g. prototype mutation and array methods, as mentioned above.

[mcollina]

Basically we'd need a process.diligentLoad() to load all lazy things from Node.js

Not sure if I am following what this suggestion is, are you talking about something similar to pre-execution?

I mean a call that loads all the lazy-loading this in Node.js core into globalThis.

[joyeecheung]

I mean a call that loads all the lazy-loading this in Node.js core into globalThis.

I think you can practically get it done by iterating all the public builtins in module.builtinModules and load them all?

[mcollina]

I think you can practically get it done by iterating all the public builtins in module.builtinModules and load them all?

Yes! I mean that our public guide recommends something, and without specific knowledge of the internals a user would encounter some issues.