Skip to content

fix: zai-web connection-test cookie probe now honors the configured proxy (#7058) - #7192

Merged
diegosouzapw merged 1 commit into
release/v3.8.49from
fix/7058-zai-web-proxy-connection-test
Jul 15, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.49from
fix/7058-zai-web-proxy-connection-test

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Closes #7058

Root cause

validateWebCookieProvider() (src/lib/providers/validation.ts) probes ${baseUrl}/models to validate a web-cookie provider's session. For zai-web (and every other web-cookie provider that has a providerRegistry entry but no specialty validator), that probe went through directHttpsRequest(), which hardcodes bypassProxyPatch: true — forcing safeOutboundFetch to use the pre-patch native fetch (getOriginalFetch()) and skip proxy-context/env-var resolution entirely.

That bypass was introduced in #3226 as a narrow, documented exception for a single NVIDIA NIM 504 workaround, but validateWebCookieProvider adopted it as its default transport from its own inception (#4023), silently extending the exception to every web-cookie provider lacking a specialty validator. Providers that DO have a specialty validator (chatgpt-web, grok-web, deepseek-web, qwen-web, …) already use validationRead/validationWrite, which correctly respect the resolved connection proxy — exactly the asymmetry the reporter observed ("other providers do support proxy").

Fix

Swap directHttpsRequest(...) for validationRead(testUrl, init, isLocalProvider(provider)) inside validateWebCookieProvider() — the same proxy-aware transport the specialty web-cookie validators already use. directHttpsRequest itself is untouched and still used by the NVIDIA NIM validator (its one documented exception).

Regression test

tests/unit/web-cookie-validation-proxy-7058.test.ts — spins up a local HTTP target plus a local forward proxy that only speaks CONNECT (matching how undici's ProxyAgent tunnels both HTTP and HTTPS traffic), points zai-web's registry baseUrl at a non-local-looking placeholder host, sets HTTP_PROXY, and asserts the cookie probe actually reaches the proxy.

  • RED (pre-fix): AssertionError: BUG #7058: zai-web cookie validation never reached the configured HTTP_PROXY ... false !== true
  • GREEN (post-fix): passes, sawConnect === true.

tests/unit/provider-validation-web-cookie-auth007.test.ts needed a small, line-neutral-in-intent update: it mocked globalThis.fetch before importing validation.ts, relying on the (accidental, previously-undocumented) fact that directHttpsRequest's native-fetch bypass hit the live network anyway. Now that the probe correctly uses the proxy-aware patched fetch, the mock must be installed after the import (matching the pattern in web-cookie-validation-fallback.test.ts) — otherwise the proxy-patch module's own globalThis.fetch assignment clobbers the test mock. Reordered + updated the comment; all 5 assertions in that file are otherwise unchanged and still pass.

Gates run

  • node --import tsx/esm --test tests/unit/web-cookie-validation-proxy-7058.test.ts — new regression test, RED→GREEN as above
  • node --import tsx/esm --test tests/unit/provider-validation-web-cookie-auth007.test.ts tests/unit/web-cookie-validation-fallback.test.ts tests/unit/proxy-bypass-scope-guard-3226.test.ts tests/unit/web-cookie-auth.test.ts tests/unit/web-cookie-providers-new.test.ts tests/unit/qwen-web-cookie-validation-3958.test.ts tests/unit/executor-web-cookie-sweep.test.ts — 102/102 pass
  • node scripts/check/check-file-size.mjs — OK
  • node scripts/check/check-complexity.mjs — OK (2056 violations, baseline unchanged)
  • node scripts/check/check-cognitive-complexity.mjs — OK (890 violations, baseline unchanged)
  • node scripts/check/check-changelog-integrity.mjs — OK
  • npm run typecheck:core — clean
  • npx eslint --suppressions-location config/quality/eslint-suppressions.json src/lib/providers/validation.ts tests/unit/web-cookie-validation-proxy-7058.test.ts tests/unit/provider-validation-web-cookie-auth007.test.ts — clean

Plan-file: _tasks/pipeline/bugs/2-implementing/7058-zai-web-proxy-not-used-in-connection-test.plan.md

…nnection-test cookie probe through the configured proxy (#7058)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request ensures that the web-cookie connection-test and cookie-validation probes honor the configured HTTP/SOCKS proxy by routing requests through validationRead instead of directHttpsRequest. It also updates existing tests and adds a regression test. The reviewer recommended wrapping the test server setup and listening calls in a try...finally block to prevent potential resource leaks if an error occurs during initialization.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +47 to +97
// Stand-in for chat.z.ai's /models probe target.
const target = http.createServer((_req, res) => {
res.writeHead(200, { "content-type": "application/json" });
res.end("{}");
});
await new Promise<void>((resolve) => target.listen(0, () => resolve()));
const targetPort = (target.address() as net.AddressInfo).port;

// Minimal forward proxy that only speaks CONNECT (like a real corporate proxy) and
// always tunnels to the local target above, regardless of the requested host — this
// lets the "upstream" host be a non-resolvable placeholder without any real DNS
// dependency, while still proving the request actually reached the proxy.
let sawConnect = false;
const proxy = http.createServer((_req, res) => {
res.writeHead(501);
res.end("CONNECT only");
});
proxy.on("connect", (_req, socket) => {
sawConnect = true;
const upstream = net.connect(targetPort, "127.0.0.1", () => {
socket.write("HTTP/1.1 200 Connection Established\r\n\r\n");
upstream.pipe(socket);
socket.pipe(upstream);
});
upstream.on("error", () => socket.destroy());
socket.on("error", () => upstream.destroy());
});
await new Promise<void>((resolve) => proxy.listen(0, () => resolve()));
const proxyPort = (proxy.address() as net.AddressInfo).port;

// A non-local-looking hostname: isLocalAddress()/resolveProxyForRequest() force a
// direct connection for any 127.*/localhost/LAN target, which would defeat this test.
zaiWebEntry!.baseUrl = "http://zai-web-validation-probe-7058.invalid";
process.env.HTTP_PROXY = `http://127.0.0.1:${proxyPort}`;
clearDispatcherCache();

try {
const result = await validateWebCookieProvider({ provider: "zai-web", apiKey: "token=fake" });

assert.equal(
sawConnect,
true,
"BUG #7058: zai-web cookie validation never reached the configured HTTP_PROXY " +
"(bypassProxyPatch:true unconditionally uses the native, unpatched fetch)"
);
assert.equal(result.valid, true, `expected a valid session, got ${JSON.stringify(result)}`);
} finally {
target.close();
proxy.close();
clearDispatcherCache();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If proxy.listen(0) fails or any error occurs during the setup before the try block, the target server will remain open and leak its socket/port. Wrapping the server creation, listening, and testing inside a single try...finally block ensures that both target and proxy servers are always closed properly, preventing resource leaks in the test suite.

  // Stand-in for chat.z.ai's /models probe target.
  const target = http.createServer((_req, res) => {
    res.writeHead(200, { "content-type": "application/json" });
    res.end("{}");
  });

  // Minimal forward proxy that only speaks CONNECT (like a real corporate proxy) and
  // always tunnels to the local target above, regardless of the requested host — this
  // lets the "upstream" host be a non-resolvable placeholder without any real DNS
  // dependency, while still proving the request actually reached the proxy.
  const proxy = http.createServer((_req, res) => {
    res.writeHead(501);
    res.end("CONNECT only");
  });

  try {
    await new Promise<void>((resolve, reject) => {
      target.listen(0, () => resolve()).on("error", reject);
    });
    const targetPort = (target.address() as net.AddressInfo).port;

    let sawConnect = false;
    proxy.on("connect", (_req, socket) => {
      sawConnect = true;
      const upstream = net.connect(targetPort, "127.0.0.1", () => {
        socket.write("HTTP/1.1 200 Connection Established\r\n\r\n");
        upstream.pipe(socket);
        socket.pipe(upstream);
      });
      upstream.on("error", () => socket.destroy());
      socket.on("error", () => upstream.destroy());
    });

    await new Promise<void>((resolve, reject) => {
      proxy.listen(0, () => resolve()).on("error", reject);
    });
    const proxyPort = (proxy.address() as net.AddressInfo).port;

    // A non-local-looking hostname: isLocalAddress()/resolveProxyForRequest() force a
    // direct connection for any 127.*/localhost/LAN target, which would defeat this test.
    zaiWebEntry!.baseUrl = "http://zai-web-validation-probe-7058.invalid";
    process.env.HTTP_PROXY = "http://127.0.0.1:" + proxyPort;
    clearDispatcherCache();

    const result = await validateWebCookieProvider({ provider: "zai-web", apiKey: "token=fake" });

    assert.equal(
      sawConnect,
      true,
      "BUG #7058: zai-web cookie validation never reached the configured HTTP_PROXY " +
        "(bypassProxyPatch:true unconditionally uses the native, unpatched fetch)"
    );
    assert.equal(result.valid, true, "expected a valid session, got " + JSON.stringify(result));
  } finally {
    target.close();
    proxy.close();
    clearDispatcherCache();
  }

@diegosouzapw
diegosouzapw merged commit 3729967 into release/v3.8.49 Jul 15, 2026
18 of 24 checks passed
@diegosouzapw
diegosouzapw deleted the fix/7058-zai-web-proxy-connection-test branch July 19, 2026 21:01
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant