Concurrent calls cause a permanent NO_PROXY env-var leak

Because withNoProxyForLocalhost is async and the env-var snapshot is captured at entry, two overlapping calls can corrupt the restored value through ordinary async interleaving:

1. Call A enters: origNoProxy = undefined, sets NO_PROXY = "localhost,127.0.0.1,[::1]", then awaits fn().
2. Call B enters (while A is awaited): origNoProxy = "localhost,127.0.0.1,[::1]" (A's injected value), alreadyCoversLocalhost = true → no mutation, then awaits its own fn().
3. Call A finally: delete process.env.NO_PROXY (correct).
4. Call B finally: process.env.NO_PROXY = "localhost,127.0.0.1,[::1]" — restores the stale value it captured from A, permanently leaking the env var.

After both callers complete, NO_PROXY is set to the loopback string even though it was originally undefined. Any subsequent code (or spawned processes) that reads NO_PROXY will see a value that should not be there.

This function is called in fetchChecked, fetchChromeVersion, and connectBrowser, all of which can run concurrently (e.g., the startup loop in launchOpenClawChrome polls isChromeReachable while other operations may be in flight).

A safe alternative is to use a reference counter and set the env var on first entry / restore on last exit, or better, avoid env-var mutation entirely for the fetch calls and rely only on the explicit agent approach (which is already used for ws).

This is a comment left during a code review.
Path: src/browser/cdp-proxy-bypass.ts
Line: 60-92

Comment:
**Concurrent calls cause a permanent `NO_PROXY` env-var leak**

Because `withNoProxyForLocalhost` is `async` and the env-var snapshot is captured at entry, two overlapping calls can corrupt the restored value through ordinary async interleaving:

1. **Call A** enters: `origNoProxy = undefined`, sets `NO_PROXY = "localhost,127.0.0.1,[::1]"`, then `await`s `fn()`.
2. **Call B** enters (while A is awaited): `origNoProxy = "localhost,127.0.0.1,[::1]"` (A's injected value), `alreadyCoversLocalhost = true` → no mutation, then `await`s its own `fn()`.
3. **Call A** `finally`: `delete process.env.NO_PROXY` (correct).
4. **Call B** `finally`: `process.env.NO_PROXY = "localhost,127.0.0.1,[::1]"` — restores the stale value it captured from A, permanently leaking the env var.

After both callers complete, `NO_PROXY` is set to the loopback string even though it was originally `undefined`. Any subsequent code (or spawned processes) that reads `NO_PROXY` will see a value that should not be there.

This function is called in `fetchChecked`, `fetchChromeVersion`, and `connectBrowser`, all of which can run concurrently (e.g., the startup loop in `launchOpenClawChrome` polls `isChromeReachable` while other operations may be in flight).

A safe alternative is to use a reference counter and set the env var on first entry / restore on last exit, or better, avoid env-var mutation entirely for the `fetch` calls and rely only on the explicit `agent` approach (which is already used for `ws`).

How can I resolve this? If you propose a fix, please make it concise.

[::1] not included in the "already covered" check

The guard only checks for "localhost" and "127.0.0.1", so if an existing NO_PROXY value is e.g. "localhost,127.0.0.1" (without [::1]), the condition returns true and no entries are appended — leaving IPv6 loopback connections unprotected and still routed through the proxy.

This is a comment left during a code review.
Path: src/browser/cdp-proxy-bypass.ts
Line: 70

Comment:
**`[::1]` not included in the "already covered" check**

The guard only checks for `"localhost"` and `"127.0.0.1"`, so if an existing `NO_PROXY` value is e.g. `"localhost,127.0.0.1"` (without `[::1]`), the condition returns `true` and no entries are appended — leaving IPv6 loopback connections unprotected and still routed through the proxy.

```suggestion
  const alreadyCoversLocalhost =
    current.includes("localhost") && current.includes("127.0.0.1") && current.includes("[::1]");
```

How can I resolve this? If you propose a fix, please make it concise.

Make NO_PROXY restore logic reentrant-safe

withNoProxyForLocalhost writes process-wide env vars and then always restores the snapshot taken at entry, which is unsafe when calls overlap. With HTTP_PROXY set, two concurrent invocations can interleave so the later one captures the first call’s temporary NO_PROXY value and restores it last, leaving NO_PROXY/no_proxy permanently altered after both calls complete. That makes proxy behavior nondeterministic across unrelated requests in concurrent browser operations; this should be guarded with reentrant state (e.g., lock/refcount/stack) rather than blind restore.

Useful? React with 👍 / 👎.

Check IPv6 loopback before skipping NO_PROXY extension

The alreadyCoversLocalhost condition only looks for localhost and 127.0.0.1, so an existing NO_PROXY=localhost,127.0.0.1 is treated as complete and ::1 is never appended. In that common setup, CDP endpoints addressed via IPv6 loopback ([::1]) can still be proxied when withNoProxyForLocalhost wraps connectOverCDP, so the proxy-bypass fix remains incomplete for IPv6 localhost connections.

Useful? React with 👍 / 👎.

Restore NO_PROXY when the last concurrent call exits

The finally guard restores env only when noProxyRefCount === 0 && isFirst, which fails if call A enters first, call B enters second, and A finishes before B: A drops the count to 1 (no restore), then B drops it to 0 but has isFirst === false (still no restore). In that valid interleaving the temporary NO_PROXY/no_proxy value leaks past the wrapper and changes proxy behavior for later requests.

Useful? React with 👍 / 👎.

Preserve NO_PROXY when bypass entries already exist

The snapshot variables are only populated inside if (isFirst && !noProxyAlreadyCoversLocalhost()), so when NO_PROXY already contains the loopback entries, no snapshot is taken; the finally block still runs restoration logic and deletes NO_PROXY/no_proxy because the saved values are undefined. This causes wrapped CDP calls to silently wipe an already-correct proxy exclusion configuration.

Useful? React with 👍 / 👎.

the && isFirst check breaks restoration when first caller exits before last caller

if call A enters first (sets isFirst=true, count=1) but exits early, and call B enters second (sets isFirst=false, count=2) but exits last (count=0), then B's exit has noProxyRefCount === 0 but isFirst=false, so restoration never happens

This is a comment left during a code review.
Path: src/browser/cdp-proxy-bypass.ts
Line: 95

Comment:
the `&& isFirst` check breaks restoration when first caller exits before last caller

if call A enters first (sets `isFirst=true`, count=1) but exits early, and call B enters second (sets `isFirst=false`, count=2) but exits last (count=0), then B's exit has `noProxyRefCount === 0` but `isFirst=false`, so restoration never happens

```suggestion
    if (noProxyRefCount === 0) {
```

How can I resolve this? If you propose a fix, please make it concise.

Parse NO_PROXY entries before deciding localhost is covered

noProxyAlreadyCoversLocalhost() uses raw substring checks, so values like NO_PROXY=localhost.internal,127.0.0.10,[::10] are treated as already covering loopback even though they do not match localhost, 127.0.0.1, or ::1 as discrete NO_PROXY tokens. In that case withNoProxyForLocalhost() skips appending the real loopback entries and CDP localhost connections can still be sent through the proxy, so the bypass fix is incomplete for common comma-delimited NO_PROXY lists with similar hostnames.

Useful? React with 👍 / 👎.

Pre-existing NO_PROXY is deleted when it already covers loopback

When isFirst is true but noProxyAlreadyCoversLocalhost() is also true, the code correctly skips the save-and-mutate block — but savedNoProxy and savedNoProxyLower are never assigned and remain undefined. When the last caller exits (noProxyRefCount === 0), the cleanup path sees savedNoProxy === undefined and unconditionally runs delete process.env.NO_PROXY, wiping the pre-existing value that was never touched.

Concrete scenario:

1. User sets NO_PROXY=localhost,127.0.0.1,[::1] and HTTP_PROXY=http://proxy:8080.
2. hasProxyEnv() → true; noProxyAlreadyCoversLocalhost() → true.
3. isFirst=true, refcount → 1, but the save block is skipped (savedNoProxy stays undefined).
4. finally: refcount → 0; savedNoProxy === undefined → delete process.env.NO_PROXY. The env var the user explicitly configured is now gone.

The fix is to track whether the env was actually mutated with a separate module-level flag and only attempt restoration when that flag is set:

let noProxyRefCount = 0;
let savedNoProxy: string | undefined;
let savedNoProxyLower: string | undefined;
let didMutateNoProxy = false;

// ...

export async function withNoProxyForLocalhost<T>(fn: () => Promise<T>): Promise<T> {
  if (!hasProxyEnv()) {
    return fn();
  }

  const isFirst = noProxyRefCount === 0;
  noProxyRefCount++;

  if (isFirst && !noProxyAlreadyCoversLocalhost()) {
    didMutateNoProxy = true;
    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;
  }

  try {
    return await fn();
  } finally {
    noProxyRefCount--;
    if (noProxyRefCount === 0 && didMutateNoProxy) {
      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;
      didMutateNoProxy = false;
    }
  }
}

This is a comment left during a code review.
Path: src/browser/cdp-proxy-bypass.ts
Line: 82-108

Comment:
**Pre-existing `NO_PROXY` is deleted when it already covers loopback**

When `isFirst` is `true` but `noProxyAlreadyCoversLocalhost()` is also `true`, the code correctly skips the save-and-mutate block — but `savedNoProxy` and `savedNoProxyLower` are never assigned and remain `undefined`. When the last caller exits (`noProxyRefCount === 0`), the cleanup path sees `savedNoProxy === undefined` and unconditionally runs `delete process.env.NO_PROXY`, wiping the pre-existing value that was never touched.

Concrete scenario:
1. User sets `NO_PROXY=localhost,127.0.0.1,[::1]` and `HTTP_PROXY=http://proxy:8080`.
2. `hasProxyEnv()` → `true`; `noProxyAlreadyCoversLocalhost()` → `true`.
3. `isFirst=true`, refcount → 1, **but the save block is skipped** (`savedNoProxy` stays `undefined`).
4. `finally`: refcount → 0; `savedNoProxy === undefined` → `delete process.env.NO_PROXY`. The env var the user explicitly configured is now gone.

The fix is to track whether the env was actually mutated with a separate module-level flag and only attempt restoration when that flag is set:

```
let noProxyRefCount = 0;
let savedNoProxy: string | undefined;
let savedNoProxyLower: string | undefined;
let didMutateNoProxy = false;

// ...

export async function withNoProxyForLocalhost<T>(fn: () => Promise<T>): Promise<T> {
  if (!hasProxyEnv()) {
    return fn();
  }

  const isFirst = noProxyRefCount === 0;
  noProxyRefCount++;

  if (isFirst && !noProxyAlreadyCoversLocalhost()) {
    didMutateNoProxy = true;
    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;
  }

  try {
    return await fn();
  } finally {
    noProxyRefCount--;
    if (noProxyRefCount === 0 && didMutateNoProxy) {
      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;
      didMutateNoProxy = false;
    }
  }
}
```

How can I resolve this? If you propose a fix, please make it concise.

Preserve lowercase NO_PROXY entries when extending bypasses

withNoProxyForLocalhost computes extended from savedNoProxy || savedNoProxyLower, so if both NO_PROXY and no_proxy are set but differ, any lowercase-only exclusions are dropped during the callback. This wrapper is used around CDP connect/fetch paths, including remote CDP profiles, so a host excluded only via no_proxy can be temporarily re-routed through HTTP_PROXY and fail (e.g., NO_PROXY=browser, no_proxy=remote.example, HTTP_PROXY=...). Merge both env values instead of preferring one.

Useful? React with 👍 / 👎.