From da7e8df2063042cf55aa84c2821cada451964844 Mon Sep 17 00:00:00 2001 From: Felix Luthman Date: Tue, 25 Oct 2022 21:44:44 +0200 Subject: [PATCH 1/8] match no_proxy to subdomains --- packages/http-client/src/proxy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http-client/src/proxy.ts b/packages/http-client/src/proxy.ts index f13409b5b6..bbea03bb94 100644 --- a/packages/http-client/src/proxy.ts +++ b/packages/http-client/src/proxy.ts @@ -51,7 +51,7 @@ export function checkBypass(reqUrl: URL): boolean { .split(',') .map(x => x.trim().toUpperCase()) .filter(x => x)) { - if (upperReqHosts.some(x => x === upperNoProxyItem)) { + if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) { return true } } From 586ad49adf4a681aebcd25676edba424fee39991 Mon Sep 17 00:00:00 2001 From: Felix Luthman Date: Sun, 30 Oct 2022 16:39:56 +0100 Subject: [PATCH 2/8] strip leading dot + '*' match all + testcases --- packages/http-client/__tests__/proxy.test.ts | 24 ++++++++++++++++++++ packages/http-client/src/proxy.ts | 12 +++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index 62e8e96268..48516226a7 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -145,6 +145,30 @@ describe('proxy', () => { expect(bypass).toBeFalsy() }) + it('checkBypass returns true if host with subdomain in no_proxy', () => { + process.env['no_proxy'] = 'myserver.com' + const bypass = pm.checkBypass(new URL('https://sub.myserver.com')) + expect(bypass).toBeTruthy() + }) + + it('checkBypass returns true if host with leading dot in no_proxy', () => { + process.env['no_proxy'] = '.myserver.com' + const bypass = pm.checkBypass(new URL('https://myserver.com')) + expect(bypass).toBeTruthy() + }) + + it('checkBypass returns false if no_proxy is subdomain', () => { + process.env['no_proxy'] = 'myserver.com' + const bypass = pm.checkBypass(new URL('https://myserver.com.evil.org')) + expect(bypass).toBeFalsy() + }) + + it('checkBypass returns true if no_proxy is "*"', () => { + process.env['no_proxy'] = '*' + const bypass = pm.checkBypass(new URL('https://anything.whatsoever.com')) + expect(bypass).toBeTruthy() + }) + it('HttpClient does basic http get request through proxy', async () => { process.env['http_proxy'] = _proxyUrl const httpClient = new httpm.HttpClient() diff --git a/packages/http-client/src/proxy.ts b/packages/http-client/src/proxy.ts index bbea03bb94..f42e7c6214 100644 --- a/packages/http-client/src/proxy.ts +++ b/packages/http-client/src/proxy.ts @@ -30,6 +30,11 @@ export function checkBypass(reqUrl: URL): boolean { return false } + // '*' match all hosts (https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/#standardizing-no_proxy) + if (noProxy === '*') { + return true + } + // Determine the request port let reqPort: number | undefined if (reqUrl.port) { @@ -50,8 +55,13 @@ export function checkBypass(reqUrl: URL): boolean { for (const upperNoProxyItem of noProxy .split(',') .map(x => x.trim().toUpperCase()) + .map(x => (x.startsWith('.') ? x.substring(1) : x)) // Strip leading dot (https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/#standardizing-no_proxy) .filter(x => x)) { - if (upperReqHosts.some(x => x.includes(upperNoProxyItem))) { + if ( + upperReqHosts.some( + x => x === upperNoProxyItem || x.endsWith(`.${upperNoProxyItem}`) + ) + ) { return true } } From 0e925a6dc5a88470659d28f0809772e4f450766f Mon Sep 17 00:00:00 2001 From: Felix Luthman <34520175+felixlut@users.noreply.github.com> Date: Fri, 27 Jan 2023 18:26:23 +0100 Subject: [PATCH 3/8] Update proxy.test.ts --- packages/http-client/__tests__/proxy.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index 48516226a7..3b4fad8018 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -152,7 +152,7 @@ describe('proxy', () => { }) it('checkBypass returns true if host with leading dot in no_proxy', () => { - process.env['no_proxy'] = '.myserver.com' + process.env['no_proxy'] = '.myserver.com' const bypass = pm.checkBypass(new URL('https://myserver.com')) expect(bypass).toBeTruthy() }) From 2ceb28b20f5ff26a7d22eb0cc0a64716fbea49fc Mon Sep 17 00:00:00 2001 From: Felix Luthman Date: Wed, 8 Feb 2023 15:26:27 +0100 Subject: [PATCH 4/8] Revert "Update proxy.test.ts" This reverts commit 0e925a6dc5a88470659d28f0809772e4f450766f. --- packages/http-client/__tests__/proxy.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index 3b4fad8018..48516226a7 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -152,7 +152,7 @@ describe('proxy', () => { }) it('checkBypass returns true if host with leading dot in no_proxy', () => { - process.env['no_proxy'] = '.myserver.com' + process.env['no_proxy'] = '.myserver.com' const bypass = pm.checkBypass(new URL('https://myserver.com')) expect(bypass).toBeTruthy() }) From b91602c89d71c1057bd54ed4c3e53e2034fc0eb8 Mon Sep 17 00:00:00 2001 From: Felix Luthman Date: Fri, 10 Feb 2023 10:52:52 +0100 Subject: [PATCH 5/8] remove support for leading dots and wildcard no_proxy --- packages/http-client/__tests__/proxy.test.ts | 8 +++++--- packages/http-client/src/proxy.ts | 6 ------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index 48516226a7..e95b76af0f 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -151,10 +151,11 @@ describe('proxy', () => { expect(bypass).toBeTruthy() }) - it('checkBypass returns true if host with leading dot in no_proxy', () => { + // Do not strip leading dots as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md + it('checkBypass returns false if host with leading dot in no_proxy', () => { process.env['no_proxy'] = '.myserver.com' const bypass = pm.checkBypass(new URL('https://myserver.com')) - expect(bypass).toBeTruthy() + expect(bypass).toBeFalsy() }) it('checkBypass returns false if no_proxy is subdomain', () => { @@ -163,10 +164,11 @@ describe('proxy', () => { expect(bypass).toBeFalsy() }) + // Do not match wildcard ("*") as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md it('checkBypass returns true if no_proxy is "*"', () => { process.env['no_proxy'] = '*' const bypass = pm.checkBypass(new URL('https://anything.whatsoever.com')) - expect(bypass).toBeTruthy() + expect(bypass).toBeFalsy() }) it('HttpClient does basic http get request through proxy', async () => { diff --git a/packages/http-client/src/proxy.ts b/packages/http-client/src/proxy.ts index f42e7c6214..16817adeae 100644 --- a/packages/http-client/src/proxy.ts +++ b/packages/http-client/src/proxy.ts @@ -30,11 +30,6 @@ export function checkBypass(reqUrl: URL): boolean { return false } - // '*' match all hosts (https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/#standardizing-no_proxy) - if (noProxy === '*') { - return true - } - // Determine the request port let reqPort: number | undefined if (reqUrl.port) { @@ -55,7 +50,6 @@ export function checkBypass(reqUrl: URL): boolean { for (const upperNoProxyItem of noProxy .split(',') .map(x => x.trim().toUpperCase()) - .map(x => (x.startsWith('.') ? x.substring(1) : x)) // Strip leading dot (https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/#standardizing-no_proxy) .filter(x => x)) { if ( upperReqHosts.some( From 2a1060254ceb83e51098e251b97a5c7549cc242f Mon Sep 17 00:00:00 2001 From: Felix Luthman Date: Fri, 10 Feb 2023 11:20:47 +0100 Subject: [PATCH 6/8] change order of tests for logic consistency --- packages/http-client/__tests__/proxy.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index e95b76af0f..2a41f80408 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -151,6 +151,12 @@ describe('proxy', () => { expect(bypass).toBeTruthy() }) + it('checkBypass returns false if no_proxy is subdomain', () => { + process.env['no_proxy'] = 'myserver.com' + const bypass = pm.checkBypass(new URL('https://myserver.com.evil.org')) + expect(bypass).toBeFalsy() + }) + // Do not strip leading dots as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md it('checkBypass returns false if host with leading dot in no_proxy', () => { process.env['no_proxy'] = '.myserver.com' @@ -158,12 +164,6 @@ describe('proxy', () => { expect(bypass).toBeFalsy() }) - it('checkBypass returns false if no_proxy is subdomain', () => { - process.env['no_proxy'] = 'myserver.com' - const bypass = pm.checkBypass(new URL('https://myserver.com.evil.org')) - expect(bypass).toBeFalsy() - }) - // Do not match wildcard ("*") as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md it('checkBypass returns true if no_proxy is "*"', () => { process.env['no_proxy'] = '*' From f9f138eca87ff8b557596da99afcb54639633ea1 Mon Sep 17 00:00:00 2001 From: Felix Date: Sun, 12 Feb 2023 00:48:35 +0100 Subject: [PATCH 7/8] add test for working leading dot --- packages/http-client/__tests__/proxy.test.ts | 6 ++++++ packages/http-client/src/proxy.ts | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index 2a41f80408..b16b314e57 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -164,6 +164,12 @@ describe('proxy', () => { expect(bypass).toBeFalsy() }) + it('checkBypass returns true if host with subdomain in no_proxy defined with leading "."', () => { + process.env['no_proxy'] = '.myserver.com' + const bypass = pm.checkBypass(new URL('https://sub.myserver.com')) + expect(bypass).toBeTruthy() + }) + // Do not match wildcard ("*") as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md it('checkBypass returns true if no_proxy is "*"', () => { process.env['no_proxy'] = '*' diff --git a/packages/http-client/src/proxy.ts b/packages/http-client/src/proxy.ts index 16817adeae..43df7fc54d 100644 --- a/packages/http-client/src/proxy.ts +++ b/packages/http-client/src/proxy.ts @@ -53,7 +53,7 @@ export function checkBypass(reqUrl: URL): boolean { .filter(x => x)) { if ( upperReqHosts.some( - x => x === upperNoProxyItem || x.endsWith(`.${upperNoProxyItem}`) + x => x === upperNoProxyItem || x.endsWith(`${upperNoProxyItem}`) ) ) { return true From bad8f88917b763d89c9c22d3b2861587e055d63c Mon Sep 17 00:00:00 2001 From: Felix Date: Sun, 12 Feb 2023 01:06:47 +0100 Subject: [PATCH 8/8] add check for partial domain, as opposed to subdomain --- packages/http-client/__tests__/proxy.test.ts | 6 ++++++ packages/http-client/src/proxy.ts | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/http-client/__tests__/proxy.test.ts b/packages/http-client/__tests__/proxy.test.ts index b16b314e57..be1a506909 100644 --- a/packages/http-client/__tests__/proxy.test.ts +++ b/packages/http-client/__tests__/proxy.test.ts @@ -157,6 +157,12 @@ describe('proxy', () => { expect(bypass).toBeFalsy() }) + it('checkBypass returns false if no_proxy is part of domain', () => { + process.env['no_proxy'] = 'myserver.com' + const bypass = pm.checkBypass(new URL('https://evilmyserver.com')) + expect(bypass).toBeFalsy() + }) + // Do not strip leading dots as per https://github.com/actions/runner/blob/97195bad5870e2ad0915ebfef1616083aacf5818/docs/adrs/0263-proxy-support.md it('checkBypass returns false if host with leading dot in no_proxy', () => { process.env['no_proxy'] = '.myserver.com' diff --git a/packages/http-client/src/proxy.ts b/packages/http-client/src/proxy.ts index 43df7fc54d..e4e43a5475 100644 --- a/packages/http-client/src/proxy.ts +++ b/packages/http-client/src/proxy.ts @@ -53,7 +53,11 @@ export function checkBypass(reqUrl: URL): boolean { .filter(x => x)) { if ( upperReqHosts.some( - x => x === upperNoProxyItem || x.endsWith(`${upperNoProxyItem}`) + x => + x === upperNoProxyItem || + x.endsWith(`.${upperNoProxyItem}`) || + (upperNoProxyItem.startsWith('.') && + x.endsWith(`${upperNoProxyItem}`)) ) ) { return true