From 003480881be780879d27eaf6afb1b4bb65c3d564 Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 3 Jan 2022 18:19:21 -0600 Subject: [PATCH] fix: infinite proxy loop (#4676) I think the problem is that when a proxy is not in use proxy-agent returns the global agent...which is itself since we set it globally, causing the loop. VS Code already covers proxies meaning we only need to do it in our own requests so to fix this pass in the agent in the version fetch request instead of overidding globally. Also avoid proxy-from-env and pass in the proxy URI instead as both http_proxy and https_proxy can be used for either http or https requests but it does not allow that. --- package.json | 1 - src/node/constants.ts | 2 + src/node/entry.ts | 3 -- src/node/proxy_agent.ts | 71 ------------------------------ src/node/update.ts | 9 ++-- test/unit/node/proxy_agent.test.ts | 43 ------------------ yarn.lock | 40 ++++++++++++++--- 7 files changed, 41 insertions(+), 128 deletions(-) delete mode 100644 src/node/proxy_agent.ts delete mode 100644 test/unit/node/proxy_agent.test.ts diff --git a/package.json b/package.json index 542253c0..8bca2674 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,6 @@ "limiter": "^1.1.5", "pem": "^1.14.2", "proxy-agent": "^5.0.0", - "proxy-from-env": "^1.1.0", "qs": "6.10.2", "rotating-file-stream": "^3.0.0", "safe-buffer": "^5.1.1", diff --git a/src/node/constants.ts b/src/node/constants.ts index b2e53dc9..4e46849f 100644 --- a/src/node/constants.ts +++ b/src/node/constants.ts @@ -25,3 +25,5 @@ export const rootPath = path.resolve(__dirname, "../..") export const vsRootPath = path.join(rootPath, "vendor/modules/code-oss-dev") export const tmpdir = path.join(os.tmpdir(), "code-server") export const isDevMode = commit === "development" +export const httpProxyUri = + process.env.HTTPS_PROXY || process.env.https_proxy || process.env.HTTP_PROXY || process.env.http_proxy diff --git a/src/node/entry.ts b/src/node/entry.ts index 27b76f4f..92fd8a8e 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -2,12 +2,9 @@ import { logger } from "@coder/logger" import { optionDescriptions, parse, readConfigFile, setDefaults, shouldOpenInExistingInstance } from "./cli" import { commit, version } from "./constants" import { openInExistingInstance, runCodeServer, runVsCodeCli, shouldSpawnCliProcess } from "./main" -import { monkeyPatchProxyProtocols } from "./proxy_agent" import { isChild, wrapper } from "./wrapper" async function entry(): Promise { - monkeyPatchProxyProtocols() - // There's no need to check flags like --help or to spawn in an existing // instance for the child process because these would have already happened in // the parent and the child wouldn't have been spawned. We also get the diff --git a/src/node/proxy_agent.ts b/src/node/proxy_agent.ts deleted file mode 100644 index 35b38ba7..00000000 --- a/src/node/proxy_agent.ts +++ /dev/null @@ -1,71 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Coder Technologies. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import ProxyAgent from "proxy-agent" -import { getProxyForUrl } from "proxy-from-env" - -/** - * This file has nothing to do with the code-server proxy. - * It is to support $HTTP_PROXY, $HTTPS_PROXY and $NO_PROXY. - * - * - https://github.com/cdr/code-server/issues/124 - * - https://www.npmjs.com/package/proxy-agent - * - https://www.npmjs.com/package/proxy-from-env - * - */ - -/** - * monkeyPatch patches the node http,https modules to route all requests through the - * agent we get from the proxy-agent package. - * - * This approach only works if there is no code specifying an explicit agent when making - * a request. - * - * None of our code ever passes in a explicit agent to the http,https modules. - * VS Code's does sometimes but only when a user sets the http.proxy configuration. - * See https://code.visualstudio.com/docs/setup/network#_legacy-proxy-server-support - * - * Even if they do, it's probably the same proxy so we should be fine! And those knobs - * are deprecated anyway. - */ -export function monkeyPatchProxyProtocols(): void { - if (!shouldEnableProxy()) { - return - } - - const http = require("http") - const https = require("https") - - // If we do not pass in a proxy URL, proxy-agent will get the URL from the environment. - // See https://www.npmjs.com/package/proxy-from-env. - // Also see shouldEnableProxy. - const pa = new ProxyAgent() - http.globalAgent = pa - https.globalAgent = pa -} - -const sampleUrls = [new URL("http://example.com"), new URL("https://example.com")] - -// If they have $NO_PROXY set to example.com then this check won't work! -// But that's drastically unlikely. -export function shouldEnableProxy(): boolean { - const testedProxyEndpoints = sampleUrls.map((url) => { - return { - url, - proxyUrl: getProxyForUrl(url.toString()), - } - }) - - let shouldEnable = false - - for (const { url, proxyUrl } of testedProxyEndpoints) { - if (proxyUrl) { - console.debug(`${url.protocol} -- Using "${proxyUrl}"`) - shouldEnable = true - } - } - - return shouldEnable -} diff --git a/src/node/update.ts b/src/node/update.ts index 03d61ed9..f5d7f370 100644 --- a/src/node/update.ts +++ b/src/node/update.ts @@ -1,9 +1,10 @@ import { field, logger } from "@coder/logger" import * as http from "http" import * as https from "https" +import ProxyAgent from "proxy-agent" import * as semver from "semver" import * as url from "url" -import { version } from "./constants" +import { httpProxyUri, version } from "./constants" import { SettingsProvider, UpdateSettings } from "./settings" export interface Update { @@ -102,8 +103,10 @@ export class UpdateProvider { return new Promise((resolve, reject) => { const request = (uri: string): void => { logger.debug("Making request", field("uri", uri)) - const httpx = uri.startsWith("https") ? https : http - const client = httpx.get(uri, { headers: { "User-Agent": "code-server" } }, (response) => { + const isHttps = uri.startsWith("https") + const agent = httpProxyUri ? new ProxyAgent(httpProxyUri) : undefined + const httpx = isHttps ? https : http + const client = httpx.get(uri, { headers: { "User-Agent": "code-server" }, agent }, (response) => { if (!response.statusCode || response.statusCode < 200 || response.statusCode >= 400) { response.destroy() return reject(new Error(`${uri}: ${response.statusCode || "500"}`)) diff --git a/test/unit/node/proxy_agent.test.ts b/test/unit/node/proxy_agent.test.ts deleted file mode 100644 index 72fdfa9c..00000000 --- a/test/unit/node/proxy_agent.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { shouldEnableProxy } from "../../../src/node/proxy_agent" -import { mockLogger, useEnv } from "../../utils/helpers" - -describe("shouldEnableProxy", () => { - const [setHTTPProxy, resetHTTPProxy] = useEnv("HTTP_PROXY") - const [setHTTPSProxy, resetHTTPSProxy] = useEnv("HTTPS_PROXY") - const [setNoProxy, resetNoProxy] = useEnv("NO_PROXY") - - beforeAll(() => { - mockLogger() - }) - - beforeEach(() => { - jest.resetModules() // Most important - it clears the cache - resetHTTPProxy() - resetNoProxy() - resetHTTPSProxy() - }) - - it("returns true when HTTP_PROXY is set", () => { - setHTTPProxy("http://proxy.example.com") - expect(shouldEnableProxy()).toBe(true) - }) - it("returns true when HTTPS_PROXY is set", () => { - setHTTPSProxy("https://proxy.example.com") - expect(shouldEnableProxy()).toBe(true) - }) - it("returns false when NO_PROXY is set", () => { - setNoProxy("*") - expect(shouldEnableProxy()).toBe(false) - }) - it("should return false when neither HTTP_PROXY nor HTTPS_PROXY is set", () => { - expect(shouldEnableProxy()).toBe(false) - }) - it("should return false when NO_PROXY is set to https://example.com", () => { - setNoProxy("https://example.com") - expect(shouldEnableProxy()).toBe(false) - }) - it("should return false when NO_PROXY is set to http://example.com", () => { - setNoProxy("http://example.com") - expect(shouldEnableProxy()).toBe(false) - }) -}) diff --git a/yarn.lock b/yarn.lock index b739bcb1..566990ec 100644 --- a/yarn.lock +++ b/yarn.lock @@ -890,6 +890,11 @@ bytes@3.1.0: resolved "https://registry.yarnpkg.com/bytes/-/bytes-3.1.0.tgz#f6cf7933a360e0588fa9fde85651cdc7f805d1f6" integrity sha512-zauLjrfCG+xvoyaqLoV8bLVXXNGC4JqlxFCutSDWA6fJrTo2ZuvLYTqZ7aHBLZSMOopbzwv8f+wZcVzfVTI2Dg== +bytes@3.1.1: + version "3.1.1" + resolved "https://registry.yarnpkg.com/bytes/-/bytes-3.1.1.tgz#3f018291cb4cbad9accb6e6970bca9c8889e879a" + integrity sha512-dWe4nWO/ruEOY7HkUJ5gFt1DCFV9zPRoJr8pV0/ASQermOZjtq8jMjOprC0Kd10GLN+l7xaUPvxzJFWtxGu8Fg== + call-bind@^1.0.0, call-bind@^1.0.2: version "1.0.2" resolved "https://registry.yarnpkg.com/call-bind/-/call-bind-1.0.2.tgz#b1d4e89e688119c3c9a903ad30abb2f6a919be3c" @@ -2190,7 +2195,18 @@ http-errors@1.7.2: statuses ">= 1.5.0 < 2" toidentifier "1.0.0" -http-errors@1.7.3, http-errors@~1.7.2: +http-errors@1.8.1: + version "1.8.1" + resolved "https://registry.yarnpkg.com/http-errors/-/http-errors-1.8.1.tgz#7c3f28577cbc8a207388455dbd62295ed07bd68c" + integrity sha512-Kpk9Sm7NmI+RHhnj6OIWDI1d6fIoFAtFt9RLaTMRlg/8w49juAStsrBgp0Dp4OdxdVbRIeKhtCUvoi/RuAhO4g== + dependencies: + depd "~1.1.2" + inherits "2.0.4" + setprototypeof "1.2.0" + statuses ">= 1.5.0 < 2" + toidentifier "1.0.1" + +http-errors@~1.7.2: version "1.7.3" resolved "https://registry.yarnpkg.com/http-errors/-/http-errors-1.7.3.tgz#6c619e4f9c60308c38519498c14fbb10aacebb06" integrity sha512-ZTTX0MWrsQ2ZAhA1cejAwDLycFsd7I7nVtnkT3Ol0aqodaKW+0CTZDQ1uBv5whptCnc8e8HeRRJxRs0kmm/Qfw== @@ -3459,7 +3475,7 @@ proxy-agent@^5.0.0: proxy-from-env "^1.0.0" socks-proxy-agent "^5.0.0" -proxy-from-env@^1.0.0, proxy-from-env@^1.1.0: +proxy-from-env@^1.0.0: version "1.1.0" resolved "https://registry.yarnpkg.com/proxy-from-env/-/proxy-from-env-1.1.0.tgz#e102f16ca355424865755d2c9e8ea4f24d58c3e2" integrity sha512-D+zkORCbA9f1tdWRK0RaCR3GPv50cMxcrz4X8k5LTSUD1Dkw47mKJEZQNunItRTkWwgtaUSo1RVFRIG9ZXiFYg== @@ -3512,12 +3528,12 @@ raw-body@2.4.0: unpipe "1.0.0" raw-body@^2.2.0: - version "2.4.1" - resolved "https://registry.yarnpkg.com/raw-body/-/raw-body-2.4.1.tgz#30ac82f98bb5ae8c152e67149dac8d55153b168c" - integrity sha512-9WmIKF6mkvA0SLmA2Knm9+qj89e+j1zqgyn8aXGd7+nAduPoqgI9lO57SAZNn/Byzo5P7JhXTyg9PzaJbH73bA== + version "2.4.2" + resolved "https://registry.yarnpkg.com/raw-body/-/raw-body-2.4.2.tgz#baf3e9c21eebced59dd6533ac872b71f7b61cb32" + integrity sha512-RPMAFUJP19WIet/99ngh6Iv8fzAbqum4Li7AD6DtGaW2RpMB/11xDoalPiJMTbu6I3hkbMVkATvZrqb9EEqeeQ== dependencies: - bytes "3.1.0" - http-errors "1.7.3" + bytes "3.1.1" + http-errors "1.8.1" iconv-lite "0.4.24" unpipe "1.0.0" @@ -3817,6 +3833,11 @@ setprototypeof@1.1.1: resolved "https://registry.yarnpkg.com/setprototypeof/-/setprototypeof-1.1.1.tgz#7e95acb24aa92f5885e0abef5ba131330d4ae683" integrity sha512-JvdAWfbXeIGaZ9cILp38HntZSFSo3mWg6xGcJJsd+d4aRMOqauag1C63dJfDw7OaMYwEbHMOxEZ1lqVRYP2OAw== +setprototypeof@1.2.0: + version "1.2.0" + resolved "https://registry.yarnpkg.com/setprototypeof/-/setprototypeof-1.2.0.tgz#66c9a24a73f9fc28cbe66b09fed3d33dcaf1b424" + integrity sha512-E5LDX7Wrp85Kil5bhZv46j8jOeboKq5JMmYM3gVGdGH8xFpPWXUMsNrlODCrkoxMEeNi/XZIwuRvY4XNwYMJpw== + shebang-command@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/shebang-command/-/shebang-command-2.0.0.tgz#ccd0af4f8835fbdc265b82461aaf0c36663f34ea" @@ -4238,6 +4259,11 @@ toidentifier@1.0.0: resolved "https://registry.yarnpkg.com/toidentifier/-/toidentifier-1.0.0.tgz#7e1be3470f1e77948bc43d94a3c8f4d7752ba553" integrity sha512-yaOH/Pk/VEhBWWTlhI+qXxDFXlejDGcQipMlyxda9nthulaxLZUNcUqFxokp0vcYnvteJln5FNQDRrxj3YcbVw== +toidentifier@1.0.1: + version "1.0.1" + resolved "https://registry.yarnpkg.com/toidentifier/-/toidentifier-1.0.1.tgz#3be34321a88a820ed1bd80dfaa33e479fbb8dd35" + integrity sha512-o5sSPKEkg/DIQNmH43V0/uerLrpzVedkUh8tGNvaeXpfpuwjKenlSox/2O/BTlZUtEe+JG7s5YhEz608PlAHRA== + traverse@^0.6.6: version "0.6.6" resolved "https://registry.yarnpkg.com/traverse/-/traverse-0.6.6.tgz#cbdf560fd7b9af632502fed40f918c157ea97137"