httptoolkit / httptoolkit-server

The backend of HTTP Toolkit
https://httptoolkit.com
GNU Affero General Public License v3.0
433 stars 96 forks source link

Terminal Intercepting crashes when using NodeJS worker threads #91

Closed leumasme closed 11 months ago

leumasme commented 11 months ago

HTTP Toolkit Terminal Interceptor currently crashes when executing a script that uses Worker threads

Exception ``` Worker error: Error [InvalidArgumentError]: Proxy opts.uri is mandatory at buildProxyOptions (S:\Apps\Scoop\apps\httptoolkit\1.13.0\resources\httptoolkit-server\overrides\js\node_modules\undici\lib\proxy-agent.js:28:11) at new ProxyAgent (S:\Apps\Scoop\apps\httptoolkit\1.13.0\resources\httptoolkit-server\overrides\js\node_modules\undici\lib\proxy-agent.js:40:20) at Object.wrapUndici [as wrap] (S:\Apps\Scoop\apps\httptoolkit\1.13.0\resources\httptoolkit-server\overrides\js\prepend-node.js:72:9) at fixModule (S:\Apps\Scoop\apps\httptoolkit\1.13.0\resources\httptoolkit-server\overrides\js\wrap-require.js:32:37) at mod._load (S:\Apps\Scoop\apps\httptoolkit\1.13.0\resources\httptoolkit-server\overrides\js\wrap-require.js:67:24) at Module.require (node:internal/modules/cjs/loader:1143:19) at require (node:internal/modules/cjs/helpers:110:18) at Object. (S:\Apps\Scoop\apps\httptoolkit\1.13.0\resources\httptoolkit-server\overrides\js\prepend-node.js:126:5) at Module._compile (node:internal/modules/cjs/loader:1256:14) at Module._extensions..js (node:internal/modules/cjs/loader:1310:10) { code: 'UND_ERR_INVALID_ARG' } ```

The error originates here: https://github.com/httptoolkit/httptoolkit-server/blob/89ba2ecae5f2fa4fa2a5a31a4a6fc8d3ef6aa04b/overrides/js/prepend-node.js#L61-L71

After some investigation, it turns out that you are setting the envoirement variable http_proxy, but in code attempt to access it as HTTP_PROXY. In a regular NodeJS context, this works fine since the process.env object does some getter magic to make property accessing case-insensitive. This is correct and expected, as envoirement variables on windows are intended to be case-insensitive.
Documented here: On Windows operating systems, environment variables are case-insensitive.

In worker threads, however, this behavior does not apply, and process.env.HTTP_PROXY returns undefined when the variable is called http_proxy, which causes the ProxyAgent to get called without an uri, causing the crash as seen above.

On new node versions this happens even without importing undici within the worker, since HTTP Toolkit automatically imports undici to proxy the global fetch object. https://github.com/httptoolkit/httptoolkit-server/blob/89ba2ecae5f2fa4fa2a5a31a4a6fc8d3ef6aa04b/overrides/js/prepend-node.js#L117-L124

The worker threads documentation doesn't explicitly state wether process.env in workers should be case-sensitive or -insensitive, but it seems reasonable to assume that this detail was simply forgotten.
I'm thus going to create an issue in the NodeJS repo as well, however HTTP Toolkit should still implement a workaround for this (perhaps trying to get both the fully-uppercase and the fully-lowercase variant, or getting the keys of process.env and finding the one that case-insensitively matches the env var name to even find variables that are matched inconsistently, e.g. Http-Proxy)

Here's a repro of the issue:

Reproduction code ```js // main.js const { Worker } = require("worker_threads"); const path = require("path"); const worker = new Worker(path.join(__dirname, "worker.js")); console.log("in parent:") console.log("http_proxy:",process.env.http_proxy) console.log("HTTP_PROXY:",process.env.HTTP_PROXY) worker.on("message", (data) => { console.log("in worker:") console.log("http_proxy:", data.http_proxy); console.log("HTTP_PROXY:", data.HTTP_PROXY); }); // --------- // worker.js const { parentPort } = require('worker_threads'); parentPort.postMessage({ http_proxy: process.env.http_proxy, HTTP_PROXY: process.env.HTTP_PROXY, }); ``` logs: ``` in parent: http_proxy: test HTTP_PROXY: test in worker: http_proxy: test HTTP_PROXY: undefined ```

TLDR: process.env isn't case insensitive in NodeJS Worker Threads, which breaks the Terminal Interceptor. Make the terminal interceptor manually search for the "http_toolkit" variable in different casings as a workaround.

pimterry commented 11 months ago

After some investigation, it turns out that you are setting the envoirement variable http_proxy, but in code attempt to access it as HTTP_PROXY.

Technically, we're setting both, here:

https://github.com/httptoolkit/httptoolkit-server/blob/89ba2ecae5f2fa4fa2a5a31a4a6fc8d3ef6aa04b/src/interceptors/terminal/terminal-env-overrides.ts#L81-L82

That said, it totally makes sense to me that Windows collapses these into a single variable when it ignores case, and then that Node bug (thanks for filing that!) makes it case sensitive to the selected result.

I don't want to overcomplicate this (e.g. to handle all possible casings) since in theory the current approach is 'correct' (set both vars, read one of them, expect that to be set) and it seems there's a proper fix en route for the specific node bug anyway. For now, I've just added the simplest fix, which should resolve this for your specific Undici issue and also various other cases (e.g. global-agent reads HTTP_PROXY internally I think, which is presumably broken as well, though not actually crashing).

I've pushed that here: https://github.com/httptoolkit/httptoolkit-server/commit/0a3d0af94a08237b443e62e79170820f049ee725. Can you give that a quick test and confirm it works for you?

leumasme commented 11 months ago

Yup, that patch fixes the issue for me!

leumasme commented 11 months ago

and it seems there's a proper fix en route for the specific node bug anyway.

Sadly it seems that the NodeJS team has decided to add this behavior to the docs instead of fixing it.
https://github.com/nodejs/node/pull/49008 https://github.com/nodejs/node/pull/48976#pullrequestreview-1558448330