diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eaa79fdb5..cd7a3a2c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,21 @@ ## 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 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. ## [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 aa6a28c550..7ed998d74a 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,6 +10,22 @@ const DEFAULT_PORTS: Record = { wss: 443, }; +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, or a no-op when + * `key` is already pinned, so the first pin's value and owner win. + */ +export function pinProxyEnv(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, @@ -25,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). @@ -38,30 +52,19 @@ 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?.hostname) { return ""; // Don't proxy URLs without a valid scheme or host. } - proto = proto.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 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. } - 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; @@ -88,7 +91,6 @@ function shouldProxy( const NO_PROXY = ( noProxy || noProxyFallback || - getEnv("npm_config_no_proxy") || getEnv("no_proxy") ).toLowerCase(); if (!NO_PROXY) { @@ -131,5 +133,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..696b455a4b 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, pinProxyEnv } from "../api/proxy"; import { LocalProxySettings } from "../settings/proxy"; import { sessionId } from "../telemetry/ids"; @@ -116,10 +116,14 @@ 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; } + if (env === process.env) { + unpins.push(pinProxyEnv(key)); + } previous[key] = env[key]; env[key] = value; } @@ -131,6 +135,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/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 1816c51614..3289100d0d 100644 --- a/test/unit/api/proxy.test.ts +++ b/test/unit/api/proxy.test.ts @@ -1,6 +1,16 @@ -import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + describe, + it, + expect, + beforeEach, + afterEach, + onTestFinished, + vi, +} from "vitest"; -import { getProxyForUrl } from "@/api/proxy"; +import { getProxyForUrl, pinProxyEnv } from "@/api/proxy"; + +import { clearProxyEnv } from "../../mocks/testHelpers"; describe("proxy", () => { const proxy = "http://proxy.example.com:8080"; @@ -27,21 +37,29 @@ 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(pinProxyEnv("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(""); }); @@ -80,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 }) => { @@ -151,15 +174,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 f9560cdeb9..56122c4ee1 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, @@ -8,6 +9,7 @@ import { import { sessionId } from "@/telemetry/ids"; import { + clearProxyEnv, config, PROXY_URL as proxy, withProxy, @@ -207,6 +209,21 @@ describe("applySshEnvironment", () => { applied.dispose(); } }); + + it("keeps proxy variables out of the API's env proxy fallback", () => { + clearProxyEnv(); + 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() {