fix(server): outbound fetch honours HTTPS_PROXY without NODE_USE_ENV_PROXY

apps/server/src/binCli.ts

@@ -15,7 +15,7 @@ import { browserCommand } from "./cli/browser.ts";
 import { connectCommand } from "./cli/connect.ts";
 import { pairCommand } from "./cli/pair.ts";
 import { hasCloudPublicConfig } from "./cloud/publicConfig.ts";
-import { extendConnectionAttemptTimeout, keepOutboundFetchOnHttp1 } from "./outboundHttp.ts";
+import { configureOutboundFetch, extendConnectionAttemptTimeout } from "./outboundHttp.ts";
 import { sharedServerCommandFlags } from "./cli/config.ts";
 import { projectCommand } from "./cli/project.ts";
 import { runDefaultServerCommand, serveCommand, startCommand } from "./cli/server.ts";
@@ -96,7 +96,7 @@ export const cli = makeCli();
 
 export function runCli() {
   extendConnectionAttemptTimeout();
-  keepOutboundFetchOnHttp1();
+  configureOutboundFetch();
   Command.run(cli, { version: packageJson.version }).pipe(
     Effect.scoped,
     Effect.provide(layerCliRuntime),

apps/server/src/outboundHttp.test.ts

@@ -1,4 +1,5 @@
-// @effect-diagnostics nodeBuiltinImport:off - the test runs a real TLS h2 server to see which protocol fetch negotiates.
+// @effect-diagnostics nodeBuiltinImport:off - the tests run real servers to see how fetch connects.
+import * as NodeHttp from "node:http";
 import * as NodeHttp2 from "node:http2";
 import * as NodeNet from "node:net";
 import * as NodeTls from "node:tls";
@@ -10,10 +11,11 @@ import * as Stream from "effect/Stream";
 import { ChildProcess, ChildProcessSpawner } from "effect/process";
 import { FetchHttpClient, HttpClient } from "effect/http";
 import * as Undici from "undici";
-import { afterAll, afterEach, beforeAll, describe, expect } from "vite-plus/test";
+import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, vi } from "vite-plus/test";
 
 import {
   builtinFetchUsesHttp2,
+  configureOutboundFetch,
   ignoreMacosTypeOfServiceEinval,
   installHttp1FetchDispatcher,
 } from "./outboundHttp.ts";
@@ -45,7 +47,29 @@ const server = NodeHttp2.createSecureServer(
   { allowHTTP1: true, key: TEST_KEY, cert: TEST_CERT },
   (request, response) => response.end(request.httpVersion),
 );
+const target = NodeHttp.createServer((_request, response) => response.end("direct"));
+const proxy = NodeHttp.createServer((_request, response) => response.end("proxied"));
+// Tunnels CONNECT requests to the TLS server, recording each authority it was asked for.
+const tunnelledAuthorities: Array<string> = [];
+const tunnelSockets = new Set<NodeNet.Socket>();
+proxy.on("connect", (request, clientSocket: NodeNet.Socket, head) => {
+  tunnelledAuthorities.push(request.url ?? "");
+  const [host, port] = (request.url ?? "").split(":");
+  const upstream = NodeNet.connect(Number(port), host, () => {
+    clientSocket.write("HTTP/1.1 200 Connection Established\r\n\r\n");
+    upstream.write(head);
+    upstream.pipe(clientSocket);
+    clientSocket.pipe(upstream);
+  });
+  for (const socket of [clientSocket, upstream]) {
+    tunnelSockets.add(socket);
+    socket.on("error", () => socket.destroy());
+    socket.on("close", () => tunnelSockets.delete(socket));
+  }
+});
 let origin = "";
+let targetOrigin = "";
+let proxyOrigin = "";
 const defaultCaCertificates = NodeTls.getCACertificates("default");
 const defaultDispatcher = Undici.getGlobalDispatcher();
 const defaultAttemptTimeout = NodeNet.getDefaultAutoSelectFamilyAttemptTimeout();
@@ -54,22 +78,52 @@ beforeAll(async () => {
   NodeTls.setDefaultCACertificates([...defaultCaCertificates, TEST_CERT]);
   await new Promise<void>((resolve) => server.listen(0, "127.0.0.1", resolve));
   origin = `https://127.0.0.1:${(server.address() as NodeNet.AddressInfo).port}/`;
+  targetOrigin = await listenOnLoopback(target);
+  proxyOrigin = await listenOnLoopback(proxy);
 });
 
 afterEach(() => {
+  vi.unstubAllEnvs();
   Undici.setGlobalDispatcher(defaultDispatcher);
   NodeNet.setDefaultAutoSelectFamilyAttemptTimeout(defaultAttemptTimeout);
 });
 
 afterAll(async () => {
   NodeTls.setDefaultCACertificates(defaultCaCertificates);
   await new Promise<void>((resolve) => server.close(() => resolve()));
+  for (const socket of tunnelSockets) socket.destroy();
+  for (const httpServer of [target, proxy]) {
+    httpServer.closeAllConnections();
+    await new Promise<void>((resolve) => httpServer.close(() => resolve()));
+  }
 });
 
+async function listenOnLoopback(httpServer: NodeHttp.Server) {
+  await new Promise<void>((resolve) => httpServer.listen(0, "127.0.0.1", resolve));
+  return `http://127.0.0.1:${(httpServer.address() as NodeNet.AddressInfo).port}/`;
+}
+
+const clearProxyEnvironment = () => {
+  for (const name of [
+    "HTTPS_PROXY",
+    "https_proxy",
+    "HTTP_PROXY",
+    "http_proxy",
+    "NO_PROXY",
+    "no_proxy",
+    "NODE_USE_ENV_PROXY",
+  ]) {
+    vi.stubEnv(name, undefined);
+  }
+};
+
+// Proxy variables in the developer's shell would otherwise reach every dispatcher test.
+beforeEach(() => clearProxyEnvironment());
+
 const versionViaGlobalDispatcher = async () => (await Undici.request(origin)).body.text();
 
-const versionViaBuiltinFetch = () =>
-  HttpClient.get(origin).pipe(
+const textViaBuiltinFetch = (url: string) =>
+  HttpClient.get(url).pipe(
     Effect.flatMap((response) => response.text),
     Effect.provide(FetchHttpClient.layer),
   );
@@ -93,13 +147,34 @@ describe("installHttp1FetchDispatcher", () => {
     "moves the built-in fetch off HTTP/2",
     () =>
       Effect.gen(function* () {
-        expect(yield* versionViaBuiltinFetch()).toBe("2.0");
+        expect(yield* textViaBuiltinFetch(origin)).toBe("2.0");
         installHttp1FetchDispatcher();
-        expect(yield* versionViaBuiltinFetch()).toBe("1.1");
+        expect(yield* textViaBuiltinFetch(origin)).toBe("1.1");
       }),
   );
 });
 
+describe("configureOutboundFetch", () => {
+  it.effect("tunnels an HTTPS request through HTTPS_PROXY", () =>
+    Effect.gen(function* () {
+      vi.stubEnv("HTTPS_PROXY", proxyOrigin);
+      tunnelledAuthorities.length = 0;
+      configureOutboundFetch();
+      expect(yield* textViaBuiltinFetch(origin)).toBe("1.1");
+      expect(tunnelledAuthorities).toEqual([new URL(origin).host]);
+    }),
+  );
+
+  it.effect("connects directly to hosts listed in NO_PROXY", () =>
+    Effect.gen(function* () {
+      vi.stubEnv("HTTP_PROXY", proxyOrigin);
+      vi.stubEnv("NO_PROXY", "127.0.0.1");
+      configureOutboundFetch();
+      expect(yield* textViaBuiltinFetch(targetOrigin)).toBe("direct");
+    }),
+  );
+});
+
 const runNode = Effect.fnUntraced(function* (args: Array<string>) {
   const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
   const child = yield* spawner.spawn(ChildProcess.make(process.execPath, args));

apps/server/src/outboundHttp.ts

@@ -14,14 +14,18 @@ const requireUndici = NodeModule.createRequire(import.meta.url);
 export const builtinFetchUsesHttp2 = (bundledUndiciVersion: string | undefined) =>
   Number.parseInt(bundledUndiciVersion ?? "", 10) >= 8;
 
+const hasProxyEnvironment = () =>
+  ["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"].some((name) => process.env[name]);
+
 /**
- * Node's `--use-env-proxy` / `NODE_USE_ENV_PROXY` installs an
- * `EnvHttpProxyAgent` at startup; that choice is kept so proxies still apply.
+ * Routes requests through `EnvHttpProxyAgent` when a proxy variable is set or
+ * Node's `--use-env-proxy` / `NODE_USE_ENV_PROXY` already installed one.
  */
 export function installHttp1FetchDispatcher(): void {
   const undici = requireUndici("undici") as typeof Undici;
   // Node's agent comes from its internal undici copy, so `instanceof` against the npm class is false.
-  const usesEnvProxy = undici.getGlobalDispatcher().constructor.name === "EnvHttpProxyAgent";
+  const usesEnvProxy =
+    hasProxyEnvironment() || undici.getGlobalDispatcher().constructor.name === "EnvHttpProxyAgent";
   undici.setGlobalDispatcher(
     usesEnvProxy
       ? new undici.EnvHttpProxyAgent({
@@ -38,9 +42,15 @@ export function installHttp1FetchDispatcher(): void {
  * An undici HTTP/2 session that hits its idle timeout can emit `'error'` on a
  * stream with no listener, which Node turns into an uncaught exception that
  * exits the server (https://github.com/nodejs/undici/issues/5936).
+ *
+ * Built-in fetch ignores `HTTPS_PROXY` and friends unless Node is started with
+ * `NODE_USE_ENV_PROXY=1`, yet users expect the variables curl honours to work (#21922).
  */
-export function keepOutboundFetchOnHttp1(): void {
-  if (builtinFetchUsesHttp2(process.versions.undici)) installHttp1FetchDispatcher();
+export function configureOutboundFetch(): void {
+  // npm undici is v8 on every runtime, so the replacement agent stays HTTP/1-only there too.
+  if (builtinFetchUsesHttp2(process.versions.undici) || hasProxyEnvironment()) {
+    installHttp1FetchDispatcher();
+  }
 }
 
 // @types/node does not declare setTypeOfService yet.