From 158709ff623de1558dbd72c597b82e5579af33fb Mon Sep 17 00:00:00 2001
From: Marcus Widing <widing.marcus@gmail.com>
Date: Mon, 2 Mar 2026 09:25:41 +0100
Subject: [PATCH] fix: make withNoProxyForLocalhost reentrant-safe, fix [::1]
 check

Address Greptile review feedback:
- Replace snapshot/restore pattern with reference counter to prevent
  permanent NO_PROXY env-var leak under concurrent async calls
- Include [::1] in the alreadyCoversLocalhost guard
- Add concurrency regression test
---
 src/browser/cdp-proxy-bypass.test.ts | 46 +++++++++++++++++++++
 src/browser/cdp-proxy-bypass.ts      | 60 ++++++++++++++++++----------
 2 files changed, 85 insertions(+), 21 deletions(-)

diff --git a/src/browser/cdp-proxy-bypass.test.ts b/src/browser/cdp-proxy-bypass.test.ts
index bcb60e662..b8267acf1 100644
--- a/src/browser/cdp-proxy-bypass.test.ts
+++ b/src/browser/cdp-proxy-bypass.test.ts
@@ -157,3 +157,49 @@ describe("cdp-proxy-bypass", () => {
     });
   });
 });
+
+describe("withNoProxyForLocalhost concurrency", () => {
+  it("does not leak NO_PROXY when called concurrently", async () => {
+    const origNoProxy = process.env.NO_PROXY;
+    const origNoProxyLower = process.env.no_proxy;
+    delete process.env.NO_PROXY;
+    delete process.env.no_proxy;
+    process.env.HTTP_PROXY = "http://proxy:8080";
+
+    try {
+      const { withNoProxyForLocalhost } = await import("./cdp-proxy-bypass.js");
+
+      // Simulate concurrent calls
+      const delay = (ms: number) => new Promise((r) => setTimeout(r, ms));
+      const callA = withNoProxyForLocalhost(async () => {
+        // While A is running, NO_PROXY should be set
+        expect(process.env.NO_PROXY).toContain("localhost");
+        expect(process.env.NO_PROXY).toContain("[::1]");
+        await delay(50);
+        return "a";
+      });
+      const callB = withNoProxyForLocalhost(async () => {
+        await delay(20);
+        return "b";
+      });
+
+      await Promise.all([callA, callB]);
+
+      // After both complete, NO_PROXY should be restored (deleted)
+      expect(process.env.NO_PROXY).toBeUndefined();
+      expect(process.env.no_proxy).toBeUndefined();
+    } finally {
+      delete process.env.HTTP_PROXY;
+      if (origNoProxy !== undefined) {
+        process.env.NO_PROXY = origNoProxy;
+      } else {
+        delete process.env.NO_PROXY;
+      }
+      if (origNoProxyLower !== undefined) {
+        process.env.no_proxy = origNoProxyLower;
+      } else {
+        delete process.env.no_proxy;
+      }
+    }
+  });
+});
diff --git a/src/browser/cdp-proxy-bypass.ts b/src/browser/cdp-proxy-bypass.ts
index 61e8eda2e..7331a0e07 100644
--- a/src/browser/cdp-proxy-bypass.ts
+++ b/src/browser/cdp-proxy-bypass.ts
@@ -51,26 +51,39 @@ export function hasProxyEnv(): boolean {
 }
 
 /**
- * Run an async function with NO_PROXY temporarily extended to include
- * localhost and 127.0.0.1. Restores the original value afterwards.
+ * Reentrant-safe NO_PROXY extension for CDP localhost connections.
  *
- * Used for third-party code (e.g. Playwright) that reads env vars
- * internally and doesn't accept an explicit agent.
+ * Uses a reference counter so concurrent async callers share the same
+ * env-var mutation window. The env vars are set on first entry and
+ * restored on last exit, avoiding the snapshot/restore race that would
+ * permanently leak NO_PROXY when calls overlap.
  */
+let noProxyRefCount = 0;
+let savedNoProxy: string | undefined;
+let savedNoProxyLower: string | undefined;
+
+const LOOPBACK_ENTRIES = "localhost,127.0.0.1,[::1]";
+
+function noProxyAlreadyCoversLocalhost(): boolean {
+  const current = process.env.NO_PROXY || process.env.no_proxy || "";
+  return (
+    current.includes("localhost") && current.includes("127.0.0.1") && current.includes("[::1]")
+  );
+}
+
 export async function withNoProxyForLocalhost<T>(fn: () => Promise<T>): Promise<T> {
   if (!hasProxyEnv()) {
     return fn();
   }
 
-  const origNoProxy = process.env.NO_PROXY;
-  const origNoProxyLower = process.env.no_proxy;
-  const loopbackEntries = "localhost,127.0.0.1,[::1]";
+  const isFirst = noProxyRefCount === 0;
+  noProxyRefCount++;
 
-  const current = origNoProxy || origNoProxyLower || "";
-  const alreadyCoversLocalhost = current.includes("localhost") && current.includes("127.0.0.1");
-
-  if (!alreadyCoversLocalhost) {
-    const extended = current ? `${current},${loopbackEntries}` : loopbackEntries;
+  if (isFirst && !noProxyAlreadyCoversLocalhost()) {
+    savedNoProxy = process.env.NO_PROXY;
+    savedNoProxyLower = process.env.no_proxy;
+    const current = savedNoProxy || savedNoProxyLower || "";
+    const extended = current ? `${current},${LOOPBACK_ENTRIES}` : LOOPBACK_ENTRIES;
     process.env.NO_PROXY = extended;
     process.env.no_proxy = extended;
   }
@@ -78,15 +91,20 @@ export async function withNoProxyForLocalhost<T>(fn: () => Promise<T>): Promise<
   try {
     return await fn();
   } finally {
-    if (origNoProxy !== undefined) {
-      process.env.NO_PROXY = origNoProxy;
-    } else {
-      delete process.env.NO_PROXY;
-    }
-    if (origNoProxyLower !== undefined) {
-      process.env.no_proxy = origNoProxyLower;
-    } else {
-      delete process.env.no_proxy;
+    noProxyRefCount--;
+    if (noProxyRefCount === 0 && isFirst) {
+      if (savedNoProxy !== undefined) {
+        process.env.NO_PROXY = savedNoProxy;
+      } else {
+        delete process.env.NO_PROXY;
+      }
+      if (savedNoProxyLower !== undefined) {
+        process.env.no_proxy = savedNoProxyLower;
+      } else {
+        delete process.env.no_proxy;
+      }
+      savedNoProxy = undefined;
+      savedNoProxyLower = undefined;
     }
   }
 }