nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.22k stars 541 forks source link

Use primordials when built for Node.js (e.g. global.WebAssembly) #3812

Open tlhunter opened 7 hours ago

tlhunter commented 7 hours ago

I'm going to try solving this on my own but I figured I'd create an issue to track it. Suggestions for making the change are appreciated!

This would solve...

I'm working on a side project and one of the things it does is to delete a bunch of globals, such as global.WebAssembly. This is fine for all of the internal Node.js modules since they make use of primordials which is essentially caching of JavaScript globals before user code runs.

However the Undici bundled within Node.js throws an error.

Ideally it would be possible to delete most of the globals and Undici would be as stable as the other Node.js internal modules.

The implementation should look like...

Matteo suggested this change could be made as part of the build step when building Undici for Node.js. I think it can be solved with dependency injection. WebAssembly and any other globals that Undici depends on can be passed in. Perhaps the Undici code can be wrapped in a function like what Node.js does to inject __filename into CJS files. Maybe like this:

function primordialInjectionWrapper(WebAssembly, Foo, Bar) {
  // existing Undici code
}
primordialInjectionWrapper(primordials.WebAssembly, primordials.Foo, primordials.Bar);

I have also considered...

I suppose I could not delete all the globals in my silly side project. And notably this is not an issue with real applications. And I don't think there is any security benefits as one could still mutate WebAssembly later even if Undici is using a version that is pulled from the global early on.

Additional context

Here's a repro:

$ cat undici-primordial.js
delete global.WebAssembly
void Request;
$ node undici-primordial.js
node:internal/deps/undici/undici:5771
        mod = await WebAssembly.compile(llhttpWasmData || require_llhttp_wasm());
        ^

ReferenceError: WebAssembly is not defined
    at lazyllhttp (node:internal/deps/undici/undici:5771:9)
    at lib/dispatcher/client-h1.js (node:internal/deps/undici/undici:5817:25)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/dispatcher/client.js (node:internal/deps/undici/undici:7507:21)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/dispatcher/pool.js (node:internal/deps/undici/undici:7968:18)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/dispatcher/agent.js (node:internal/deps/undici/undici:8051:16)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/global.js (node:internal/deps/undici/undici:8151:17)

Node.js v23.1.0
KhafraDev commented 6 hours ago

It might fix deleting globals, but someone could still delete Array.prototype.push and break undici? I am not sure what this would solve.

metcoder95 commented 5 hours ago

I don't have a strong opinion on this one. While we do not disrupt the current lib feature state.

Just be mindful that this might be a shift in the overall way we manage the building step.

ronag commented 3 hours ago

I don't see what this solves