From 4874480a87d97c020396fc1d1a36061352f68b11 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 22 Oct 2026 00:52:56 +0300 Subject: [PATCH 1/3] fix(api): ignore SSH proxy variables in the env proxy fallback To pass the proxy to `coder ssh`, `applySshEnvironment` writes `HTTP_PROXY`, `HTTPS_PROXY` and `NO_PROXY` into the extension host's `process.env`. When `http.proxy` is unset, the HTTP agent falls back to proxy environment variables, so after you removed `http.proxy` it read those values back and kept using the old proxy until reload. Pin each variable's inherited value before writing it, and have the env fallback read the pinned value until the SSH environment is disposed. Fixes #1162 --- CHANGELOG.md | 3 +++ src/api/proxy.ts | 21 ++++++++++++++++++++- src/remote/environment.ts | 8 +++++++- test/unit/api/proxy.test.ts | 24 +++++++++++++++++++++--- test/unit/remote/environment.test.ts | 17 +++++++++++++++++ 5 files changed, 68 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eaa79fdb5..96d6728b05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ that returned after every reload when `http.useLocalProxyConfiguration` is off. API requests now also keep using the local proxy settings after the remote connects. +- Fix API requests that kept using the old proxy after you removed `http.proxy` + during a session. They no longer fall back to the proxy variables set for the + SSH connection. ## [v1.17.1](https://github.com/coder/vscode-coder/releases/tag/v1.17.1) 2026-10-15 diff --git a/src/api/proxy.ts b/src/api/proxy.ts index aa6a28c550..7021793ed0 100644 --- a/src/api/proxy.ts +++ b/src/api/proxy.ts @@ -12,6 +12,22 @@ const DEFAULT_PORTS: Record = { wss: 443, }; +/** Inherited values of variables this process overwrote, keyed in upper case. */ +const pinnedEnv = new Map(); + +/** + * Keep the env fallback on `key`'s current value after this process overwrites + * it for child processes. Returns a function that unpins it. + */ +export function pinEnv(key: string): () => void { + const upper = key.toUpperCase(); + if (pinnedEnv.has(upper)) { + return () => undefined; + } + pinnedEnv.set(upper, getEnv(key)); + return () => pinnedEnv.delete(upper); +} + /** Join a no-proxy list into a comma string, dropping blanks. */ export function joinNoProxy( entries: string[] | null | undefined, @@ -131,5 +147,8 @@ function shouldProxy( * @private */ function getEnv(key: string): string { - return process.env[key.toLowerCase()] || process.env[key.toUpperCase()] || ""; + return ( + pinnedEnv.get(key.toUpperCase()) ?? + (process.env[key.toLowerCase()] || process.env[key.toUpperCase()] || "") + ); } diff --git a/src/remote/environment.ts b/src/remote/environment.ts index ad7cbef1f3..e556e36867 100644 --- a/src/remote/environment.ts +++ b/src/remote/environment.ts @@ -1,6 +1,6 @@ import * as vscode from "vscode"; -import { joinNoProxy } from "../api/proxy"; +import { joinNoProxy, pinEnv } from "../api/proxy"; import { LocalProxySettings } from "../settings/proxy"; import { sessionId } from "../telemetry/ids"; @@ -116,10 +116,15 @@ function applyEnvironment( ): { dispose(): void } { // Stored `undefined` means the key was absent and should be deleted on cleanup. const previous: Environment = {}; + const unpins: Array<() => void> = []; for (const [key, value] of Object.entries(values)) { if (value === undefined) { continue; } + // Keep the API's env proxy fallback on the inherited values. + if (env === process.env) { + unpins.push(pinEnv(key)); + } previous[key] = env[key]; env[key] = value; } @@ -131,6 +136,7 @@ function applyEnvironment( return; } disposed = true; + unpins.forEach((unpin) => unpin()); for (const [key, value] of Object.entries(previous)) { if (value === undefined) { delete env[key]; diff --git a/test/unit/api/proxy.test.ts b/test/unit/api/proxy.test.ts index 1816c51614..83f188f59e 100644 --- a/test/unit/api/proxy.test.ts +++ b/test/unit/api/proxy.test.ts @@ -1,6 +1,14 @@ -import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; - -import { getProxyForUrl } from "@/api/proxy"; +import { + describe, + it, + expect, + beforeEach, + afterEach, + onTestFinished, + vi, +} from "vitest"; + +import { getProxyForUrl, pinEnv } from "@/api/proxy"; describe("proxy", () => { const proxy = "http://proxy.example.com:8080"; @@ -42,6 +50,16 @@ describe("proxy", () => { expect(getProxyForUrl("https://example.com", null, null)).toBe(""); }); + it("keeps the pinned value after the variable changes", () => { + vi.stubEnv("HTTPS_PROXY", "http://inherited.example.com:8080"); + onTestFinished(pinEnv("HTTPS_PROXY")); + vi.stubEnv("HTTPS_PROXY", "http://ssh.example.com:8080"); + + expect(getProxyForUrl("https://example.com", null, null)).toBe( + "http://inherited.example.com:8080", + ); + }); + it("returns empty string for invalid URLs", () => { expect(getProxyForUrl("invalid", proxy, null)).toBe(""); }); diff --git a/test/unit/remote/environment.test.ts b/test/unit/remote/environment.test.ts index f9560cdeb9..ff0922068d 100644 --- a/test/unit/remote/environment.test.ts +++ b/test/unit/remote/environment.test.ts @@ -1,6 +1,7 @@ import { spawnSync } from "node:child_process"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { getProxyForUrl } from "@/api/proxy"; import { applySshEnvironment, getSshProxyEnvironment, @@ -207,6 +208,22 @@ describe("applySshEnvironment", () => { applied.dispose(); } }); + + it("keeps proxy variables out of the API's env proxy fallback", () => { + vi.stubEnv("HTTPS_PROXY", ""); + vi.stubEnv("https_proxy", ""); + const applied = applySshEnvironment( + config(withProxy()), + fakeEnvCollection(), + ); + + try { + expect(process.env.HTTPS_PROXY).toBe(proxy); + expect(getProxyForUrl("https://coder.example.com", null, null)).toBe(""); + } finally { + applied.dispose(); + } + }); }); function fakeEnvCollection() { From a4eb3583c732413a571e46f204b9a58173297c05 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 22 Oct 2026 13:24:25 +0300 Subject: [PATCH 2/3] refactor(api): sync proxy.ts with proxy-from-env 2.1.0 Parse URLs with the WHATWG `URL` API instead of the deprecated `url.parse`, and stop reading the `npm_config_*` proxy variables, as upstream did in 2.0. The header now says the file is based on upstream rather than copied. Rename `pinEnv` to `pinProxyEnv` and document that the first pin wins. Share `clearProxyEnv` between the proxy, HTTP agent and SSH environment tests so they no longer depend on proxy variables set on the host. Add an integration test that removes `http.proxy` mid-session in a real VS Code. --- CHANGELOG.md | 10 ++++-- docs/TESTING.md | 3 ++ src/api/proxy.ts | 36 +++++++------------ src/remote/environment.ts | 5 ++- test/integration/.vscode-test.mjs | 1 + test/integration/proxy.test.ts | 52 ++++++++++++++++++++++++++++ test/integration/setup.ts | 13 +++++++ test/mocks/testHelpers.ts | 18 ++++++++++ test/unit/api/httpAgent.test.ts | 26 +------------- test/unit/api/proxy.test.ts | 33 ++++++++---------- test/unit/remote/environment.test.ts | 4 +-- 11 files changed, 126 insertions(+), 75 deletions(-) create mode 100644 test/integration/proxy.test.ts create mode 100644 test/integration/setup.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 96d6728b05..cd7a3a2c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ ## Unreleased +### Changed + +- API requests no longer read the `npm_config_proxy`, `npm_config_http_proxy`, + `npm_config_https_proxy` and `npm_config_no_proxy` environment variables. Set + `http.proxy`, `http.noProxy` or the standard `HTTP_PROXY`, `HTTPS_PROXY` and + `NO_PROXY` variables instead. + ### Fixed - Fix a "settings changed. Reload window to apply." prompt for proxy settings @@ -14,8 +21,7 @@ off. API requests now also keep using the local proxy settings after the remote connects. - Fix API requests that kept using the old proxy after you removed `http.proxy` - during a session. They no longer fall back to the proxy variables set for the - SSH connection. + during a session. ## [v1.17.1](https://github.com/coder/vscode-coder/releases/tag/v1.17.1) 2026-10-15 diff --git a/docs/TESTING.md b/docs/TESTING.md index 2d11f1bf19..0f39853e25 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -68,6 +68,9 @@ command with `xvfb-run -a`. - Tests use Mocha (VS Code test runner requirement), not Vitest. - Test files in `test/integration/` are compiled to `out/` before running. +- Tests that import from `src/` get compiled copies that don't share state + with the running extension's bundle. `test/integration/setup.ts` initializes + what activation would for them. - The first run downloads each VS Code version into `.vscode-test/`. Tests run in that copy with its own user data directory, so an open VS Code doesn't interfere. diff --git a/src/api/proxy.ts b/src/api/proxy.ts index 7021793ed0..139d46c616 100644 --- a/src/api/proxy.ts +++ b/src/api/proxy.ts @@ -1,7 +1,5 @@ -// This file is copied from proxy-from-env with added support to use something -// other than environment variables. - -import { parse as parseUrl } from "node:url"; +// Based on proxy-from-env 2.1.0. Proxy and no-proxy lists can come from +// settings, and the env fallback reads pinned values first. const DEFAULT_PORTS: Record = { ftp: 21, @@ -12,14 +10,14 @@ const DEFAULT_PORTS: Record = { wss: 443, }; -/** Inherited values of variables this process overwrote, keyed in upper case. */ const pinnedEnv = new Map(); /** * Keep the env fallback on `key`'s current value after this process overwrites - * it for child processes. Returns a function that unpins it. + * it for child processes. Returns a function that unpins it, or a no-op when + * `key` is already pinned, so the first pin's value and owner win. */ -export function pinEnv(key: string): () => void { +export function pinProxyEnv(key: string): () => void { const upper = key.toUpperCase(); if (pinnedEnv.has(upper)) { return () => undefined; @@ -41,7 +39,7 @@ export function joinNoProxy( } /** - * @param {string|object} url - The URL, or the result from url.parse. + * @param {string} url - The URL to request. * @param {string} httpProxy - The proxy URL to use. * @param {string} noProxy - Primary no-proxy list (e.g., coder.proxyBypass). * @param {string} noProxyFallback - Fallback no-proxy list (e.g., http.noProxy). @@ -54,30 +52,21 @@ export function getProxyForUrl( noProxy: string | null | undefined, noProxyFallback?: string | null, ): string { - const parsedUrl = typeof url === "string" ? parseUrl(url) : url || {}; - let proto = parsedUrl.protocol; - let hostname = parsedUrl.host; - const portRaw = parsedUrl.port; - if (typeof hostname !== "string" || !hostname || typeof proto !== "string") { + const parsedUrl = URL.parse(url); + if (!parsedUrl?.host) { return ""; // Don't proxy URLs without a valid scheme or host. } - proto = proto.split(":", 1)[0]; + const proto = parsedUrl.protocol.split(":", 1)[0]; // Stripping ports in this way instead of using parsedUrl.hostname to make // sure that the brackets around IPv6 addresses are kept. - hostname = hostname.replace(/:\d*$/, ""); - const port = - (portRaw && Number.parseInt(portRaw)) || DEFAULT_PORTS[proto] || 0; + const hostname = parsedUrl.host.replace(/:\d*$/, ""); + const port = Number.parseInt(parsedUrl.port) || DEFAULT_PORTS[proto] || 0; if (!shouldProxy(hostname, port, noProxy, noProxyFallback)) { return ""; // Don't proxy URLs that match NO_PROXY. } - let proxy = - httpProxy || - getEnv("npm_config_" + proto + "_proxy") || - getEnv(proto + "_proxy") || - getEnv("npm_config_proxy") || - getEnv("all_proxy"); + let proxy = httpProxy || getEnv(proto + "_proxy") || getEnv("all_proxy"); if (proxy && !proxy.includes("://")) { // Missing scheme in proxy, default to the requested URL's scheme. proxy = proto + "://" + proxy; @@ -104,7 +93,6 @@ function shouldProxy( const NO_PROXY = ( noProxy || noProxyFallback || - getEnv("npm_config_no_proxy") || getEnv("no_proxy") ).toLowerCase(); if (!NO_PROXY) { diff --git a/src/remote/environment.ts b/src/remote/environment.ts index e556e36867..696b455a4b 100644 --- a/src/remote/environment.ts +++ b/src/remote/environment.ts @@ -1,6 +1,6 @@ import * as vscode from "vscode"; -import { joinNoProxy, pinEnv } from "../api/proxy"; +import { joinNoProxy, pinProxyEnv } from "../api/proxy"; import { LocalProxySettings } from "../settings/proxy"; import { sessionId } from "../telemetry/ids"; @@ -121,9 +121,8 @@ function applyEnvironment( if (value === undefined) { continue; } - // Keep the API's env proxy fallback on the inherited values. if (env === process.env) { - unpins.push(pinEnv(key)); + unpins.push(pinProxyEnv(key)); } previous[key] = env[key]; env[key] = value; diff --git a/test/integration/.vscode-test.mjs b/test/integration/.vscode-test.mjs index 87f6afa344..d72deb5fdc 100644 --- a/test/integration/.vscode-test.mjs +++ b/test/integration/.vscode-test.mjs @@ -13,6 +13,7 @@ const baseConfig = { launchArgs: ["--enable-proposed-api", "coder.coder-remote", "--disable-gpu"], mocha: { ui: "tdd", + require: ["../../out/test/integration/setup.js"], timeout: 20000, }, }; diff --git a/test/integration/proxy.test.ts b/test/integration/proxy.test.ts new file mode 100644 index 0000000000..7c5c4fb98d --- /dev/null +++ b/test/integration/proxy.test.ts @@ -0,0 +1,52 @@ +import * as assert from "assert"; +import * as vscode from "vscode"; + +import { createHttpAgent } from "../../src/api/httpAgent"; +import { applySshEnvironment } from "../../src/remote/environment"; + +import type { ClientRequest } from "node:http"; + +const API_URL = "https://coder.example.com"; +const PROXY_URL = "http://proxy.example.com:3128"; + +// The extension doesn't expose its context, and the terminal side of the SSH +// environment isn't under test. +const envCollection = { + persistent: false, + replace: () => undefined, + clear: () => undefined, +}; + +suite("Proxy", () => { + test("API requests stop using http.proxy once it is removed mid-session", async () => { + const inherited = await proxyForApi(); + await setHttpProxy(PROXY_URL); + const sshEnvironment = applySshEnvironment( + vscode.workspace.getConfiguration(), + envCollection, + ); + + try { + assert.strictEqual(await proxyForApi(), PROXY_URL); + + await setHttpProxy(undefined); + assert.strictEqual(process.env.HTTPS_PROXY, PROXY_URL); + assert.strictEqual(await proxyForApi(), inherited); + } finally { + sshEnvironment.dispose(); + await setHttpProxy(undefined); + } + }); +}); + +async function proxyForApi(): Promise { + const agent = await createHttpAgent(vscode.workspace.getConfiguration()); + // Our getProxyForUrl wrapper only uses the URL, not the request. + return agent.getProxyForUrl(API_URL, {} as ClientRequest); +} + +async function setHttpProxy(value: string | undefined): Promise { + await vscode.workspace + .getConfiguration() + .update("http.proxy", value, vscode.ConfigurationTarget.Global); +} diff --git a/test/integration/setup.ts b/test/integration/setup.ts new file mode 100644 index 0000000000..22f46bb28b --- /dev/null +++ b/test/integration/setup.ts @@ -0,0 +1,13 @@ +import { LocalProxySettings } from "../../src/settings/proxy"; + +import type * as vscode from "vscode"; + +let localProxySettings: vscode.Disposable | undefined; + +export function mochaGlobalSetup(): void { + localProxySettings = LocalProxySettings.initialize(); +} + +export function mochaGlobalTeardown(): void { + localProxySettings?.dispose(); +} diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 1ad36bbc48..6c2bc77798 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -220,6 +220,24 @@ export function config(settings: Settings = {}): MockConfigurationProvider { return cfg; } +const proxyEnvVars = [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "ALL_PROXY", + "all_proxy", + "NO_PROXY", + "no_proxy", +]; + +/** Stub every proxy variable `getProxyForUrl` reads to empty. */ +export function clearProxyEnv(): void { + for (const envVar of proxyEnvVars) { + vi.stubEnv(envVar, ""); + } +} + /** Settings with http.proxy set. */ export function withProxy(settings: Settings = {}): Settings { return { "http.proxy": PROXY_URL, ...settings }; diff --git a/test/unit/api/httpAgent.test.ts b/test/unit/api/httpAgent.test.ts index 500b58e8a5..14f227794d 100644 --- a/test/unit/api/httpAgent.test.ts +++ b/test/unit/api/httpAgent.test.ts @@ -4,6 +4,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { createHttpAgent, needToken } from "@/api/httpAgent"; import { + clearProxyEnv, config, PROXY_URL as proxy, withProxy, @@ -28,25 +29,6 @@ const proxyAuthorization = "Basic dXNlcjpwYXNz"; // The request parameter exists to match proxy-agent's callback signature. const mockRequest = {} as http.ClientRequest; -const proxyEnvVars = [ - "HTTP_PROXY", - "http_proxy", - "HTTPS_PROXY", - "https_proxy", - "ALL_PROXY", - "all_proxy", - "npm_config_http_proxy", - "NPM_CONFIG_HTTP_PROXY", - "npm_config_https_proxy", - "NPM_CONFIG_HTTPS_PROXY", - "npm_config_proxy", - "NPM_CONFIG_PROXY", - "NO_PROXY", - "no_proxy", - "npm_config_no_proxy", - "NPM_CONFIG_NO_PROXY", -]; - async function createAgentOptions(settings: Settings = {}) { const agent = await createHttpAgent(config(settings)); return agent.connectOpts as AgentOpts | undefined; @@ -66,12 +48,6 @@ async function proxyForUrl( return await getProxy(url); } -function clearProxyEnv(): void { - for (const envVar of proxyEnvVars) { - vi.stubEnv(envVar, ""); - } -} - describe("needToken", () => { interface NeedTokenCase { name: string; diff --git a/test/unit/api/proxy.test.ts b/test/unit/api/proxy.test.ts index 83f188f59e..147ac4781d 100644 --- a/test/unit/api/proxy.test.ts +++ b/test/unit/api/proxy.test.ts @@ -8,7 +8,9 @@ import { vi, } from "vitest"; -import { getProxyForUrl, pinEnv } from "@/api/proxy"; +import { getProxyForUrl, pinProxyEnv } from "@/api/proxy"; + +import { clearProxyEnv } from "../../mocks/testHelpers"; describe("proxy", () => { const proxy = "http://proxy.example.com:8080"; @@ -35,24 +37,22 @@ describe("proxy", () => { }); it("returns empty string when no proxy is configured", () => { - const proxyEnvVars = [ - "HTTPS_PROXY", - "https_proxy", - "HTTP_PROXY", - "http_proxy", - "ALL_PROXY", - "all_proxy", - "npm_config_https_proxy", - "npm_config_proxy", - ]; - proxyEnvVars.forEach((v) => vi.stubEnv(v, "")); + clearProxyEnv(); + expect(getProxyForUrl("https://example.com", null, null)).toBe(""); + }); + + it("ignores npm_config proxy variables", () => { + clearProxyEnv(); + vi.stubEnv("npm_config_https_proxy", proxy); + vi.stubEnv("npm_config_proxy", proxy); expect(getProxyForUrl("https://example.com", null, null)).toBe(""); }); it("keeps the pinned value after the variable changes", () => { + clearProxyEnv(); vi.stubEnv("HTTPS_PROXY", "http://inherited.example.com:8080"); - onTestFinished(pinEnv("HTTPS_PROXY")); + onTestFinished(pinProxyEnv("HTTPS_PROXY")); vi.stubEnv("HTTPS_PROXY", "http://ssh.example.com:8080"); expect(getProxyForUrl("https://example.com", null, null)).toBe( @@ -169,15 +169,10 @@ describe("proxy", () => { } it.each([ - { name: "npm_config_no_proxy", envVar: "npm_config_no_proxy" }, { name: "NO_PROXY", envVar: "NO_PROXY" }, { name: "no_proxy (lowercase)", envVar: "no_proxy" }, ])("falls back to $name env var", ({ envVar }) => { - // Clear all no_proxy env vars first - vi.stubEnv("npm_config_no_proxy", ""); - vi.stubEnv("NO_PROXY", ""); - vi.stubEnv("no_proxy", ""); - + clearProxyEnv(); vi.stubEnv(envVar, targetHost); expect(getProxyForUrl(targetUrl, proxy, null, null)).toBe(""); }); diff --git a/test/unit/remote/environment.test.ts b/test/unit/remote/environment.test.ts index ff0922068d..56122c4ee1 100644 --- a/test/unit/remote/environment.test.ts +++ b/test/unit/remote/environment.test.ts @@ -9,6 +9,7 @@ import { import { sessionId } from "@/telemetry/ids"; import { + clearProxyEnv, config, PROXY_URL as proxy, withProxy, @@ -210,8 +211,7 @@ describe("applySshEnvironment", () => { }); it("keeps proxy variables out of the API's env proxy fallback", () => { - vi.stubEnv("HTTPS_PROXY", ""); - vi.stubEnv("https_proxy", ""); + clearProxyEnv(); const applied = applySshEnvironment( config(withProxy()), fakeEnvCollection(), From f8ec59d42aa23b7592397a68ac56ab82078fb7ac Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Fri, 23 Oct 2026 19:52:41 +0300 Subject: [PATCH 3/3] refactor(api): read proxy host and scheme from WHATWG URL fields --- src/api/proxy.ts | 8 +++----- test/unit/api/proxy.test.ts | 5 +++++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/api/proxy.ts b/src/api/proxy.ts index 139d46c616..7ed998d74a 100644 --- a/src/api/proxy.ts +++ b/src/api/proxy.ts @@ -53,14 +53,12 @@ export function getProxyForUrl( noProxyFallback?: string | null, ): string { const parsedUrl = URL.parse(url); - if (!parsedUrl?.host) { + if (!parsedUrl?.hostname) { return ""; // Don't proxy URLs without a valid scheme or host. } - const proto = parsedUrl.protocol.split(":", 1)[0]; - // Stripping ports in this way instead of using parsedUrl.hostname to make - // sure that the brackets around IPv6 addresses are kept. - const hostname = parsedUrl.host.replace(/:\d*$/, ""); + const proto = parsedUrl.protocol.slice(0, -1); + const hostname = parsedUrl.hostname; const port = Number.parseInt(parsedUrl.port) || DEFAULT_PORTS[proto] || 0; if (!shouldProxy(hostname, port, noProxy, noProxyFallback)) { return ""; // Don't proxy URLs that match NO_PROXY. diff --git a/test/unit/api/proxy.test.ts b/test/unit/api/proxy.test.ts index 147ac4781d..3289100d0d 100644 --- a/test/unit/api/proxy.test.ts +++ b/test/unit/api/proxy.test.ts @@ -98,6 +98,11 @@ describe("proxy", () => { noProxy: "example.com:8443", url: "https://example.com:8443", }, + { + name: "bracketed IPv6", + noProxy: "[::1]", + url: "http://[::1]:8080", + }, ])( "bypasses proxy when hostname matches noProxy ($name)", ({ noProxy, url }) => {