From 6d67579889f184cc6eefde30da41f8776167413a Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Tue, 30 Mar 2021 19:18:51 +0200 Subject: [PATCH 1/5] fix: replace vulnerable proxy dependency --- package.json | 2 +- src/lib/request/request.ts | 7 ++++--- test/request.test.ts | 12 ++---------- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/package.json b/package.json index 89d8d76e0a..285089e5d6 100644 --- a/package.json +++ b/package.json @@ -86,6 +86,7 @@ "configstore": "^5.0.1", "debug": "^4.1.1", "diff": "^4.0.1", + "global-agent": "^2.1.12", "hcl-to-json": "^0.1.1", "lodash.assign": "^4.2.0", "lodash.camelcase": "^4.3.0", @@ -110,7 +111,6 @@ "ora": "5.3.0", "os-name": "^3.0.0", "promise-queue": "^2.2.5", - "proxy-agent": "^3.1.1", "proxy-from-env": "^1.0.0", "rimraf": "^2.6.3", "semver": "^6.0.0", diff --git a/src/lib/request/request.ts b/src/lib/request/request.ts index 1a0772df10..42ca5162a7 100644 --- a/src/lib/request/request.ts +++ b/src/lib/request/request.ts @@ -5,7 +5,7 @@ import * as querystring from 'querystring'; import * as zlib from 'zlib'; import * as config from '../config'; import { getProxyForUrl } from 'proxy-from-env'; -import * as ProxyAgent from 'proxy-agent'; +import { bootstrap } from 'global-agent'; import * as analytics from '../analytics'; import { Global } from '../../cli/args'; import { Payload } from './types'; @@ -120,8 +120,9 @@ export = function makeRequest( const proxyUri = getProxyForUrl(url); if (proxyUri) { snykDebug('using proxy:', proxyUri); - // proxyAgent type is an EventEmitter and not an http Agent - options.agent = (new ProxyAgent(proxyUri) as unknown) as http.Agent; + bootstrap({ + environmentVariableNamespace: '', + }); } else { snykDebug('not using proxy'); } diff --git a/test/request.test.ts b/test/request.test.ts index dc455e5bd1..bfe99279f3 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -283,11 +283,7 @@ test('request with https proxy calls needle as expected', (t) => { follow_max: 5, // default timeout: 300000, // default json: undefined, // default - agent: sinon.match({ - proxy: sinon.match({ - href: 'https://proxy:8443/', // should be set when using proxy - }), - }), + agent: sinon.match.truthy, rejectUnauthorized: undefined, // should not be set when not use insecure mode }), sinon.match.func, // callback function @@ -335,11 +331,7 @@ test('request with http proxy calls needle as expected', (t) => { follow_max: 5, // default timeout: 300000, // default json: undefined, // default - agent: sinon.match({ - proxy: sinon.match({ - href: 'http://proxy:8080/', // should be set when using proxy - }), - }), + agent: sinon.match.truthy, rejectUnauthorized: undefined, // should not be set when not use insecure mode }), sinon.match.func, // callback function From e59784648ca5108263d96a0087cef7c2e7d8fc7d Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Tue, 30 Mar 2021 20:14:22 +0200 Subject: [PATCH 2/5] test(proxy): acceptance test for Proxy envvar settings --- test/jest/acceptance/proxy-behavior.spec.ts | 119 ++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 test/jest/acceptance/proxy-behavior.spec.ts diff --git a/test/jest/acceptance/proxy-behavior.spec.ts b/test/jest/acceptance/proxy-behavior.spec.ts new file mode 100644 index 0000000000..972c8a6dc8 --- /dev/null +++ b/test/jest/acceptance/proxy-behavior.spec.ts @@ -0,0 +1,119 @@ +import { exec } from 'child_process'; +import { sep } from 'path'; +const main = './dist/cli/index.js'.replace(/\//g, sep); + +const SNYK_API_HTTPS = 'https://snyk.io/api/v1'; +const SNYK_API_HTTP = 'http://snyk.io/api/v1'; +const FAKE_HTTP_PROXY = 'http://localhost:12345'; +const testTimeout = 50000; + +describe('Proxy configuration behavior', () => { + describe('*_PROXY against HTTPS host', () => { + it( + 'tries to connect to the HTTPS_PROXY when HTTPS_PROXY is set', + (done) => { + exec( + `node ${main} woof -d`, + { + env: { + HTTPS_PROXY: FAKE_HTTP_PROXY, + SNYK_API: SNYK_API_HTTPS, + }, + }, + (err, stdout, stderr) => { + if (err) { + throw err; + } + + expect(stderr).toContain( + `snyk analytics Error: connect ECONNREFUSED 127.0.0.1:${ + FAKE_HTTP_PROXY.split(':')[2] + }`, + ); + done(); + }, + ); + }, + testTimeout, + ); + + it( + 'does not try to connect to the HTTP_PROXY when it is set', + (done) => { + exec( + `node ${main} woof -d`, + { + env: { + HTTP_PROXY: FAKE_HTTP_PROXY, + SNYK_API: SNYK_API_HTTPS, + }, + }, + (err, stdout, stderr) => { + if (err) { + throw err; + } + + expect(stderr).not.toContain('ECONNREFUSED'); + done(); + }, + ); + }, + testTimeout, + ); + }); + + describe('*_PROXY against HTTP host', () => { + it( + 'tries to connect to the HTTP_PROXY when HTTP_PROXY is set', + (done) => { + exec( + `node ${main} woof -d`, + { + env: { + HTTP_PROXY: FAKE_HTTP_PROXY, + SNYK_API: SNYK_API_HTTP, + SNYK_HTTP_PROTOCOL_UPGRADE: '0', + }, + }, + (err, stdout, stderr) => { + if (err) { + throw err; + } + + expect(stderr).toContain( + `snyk analytics Error: connect ECONNREFUSED 127.0.0.1:${ + FAKE_HTTP_PROXY.split(':')[2] + }`, + ); + done(); + }, + ); + }, + testTimeout, + ); + + it( + 'does not try to connect to the HTTPS_PROXY when it is set', + (done) => { + exec( + `node ${main} woof -d`, + { + env: { + HTTPS_PROXY: FAKE_HTTP_PROXY, + SNYK_API: SNYK_API_HTTP, + SNYK_HTTP_PROTOCOL_UPGRADE: '0', + }, + }, + (err, stdout, stderr) => { + // TODO: incorrect behavior when Needle tries to upgrade connection after 301 http->https and the Agent option is set to a strict protocol + expect(stderr).toContain( + 'TypeError [ERR_INVALID_PROTOCOL]: Protocol "https:" not supported. Expected "http:"', + ); + done(); + }, + ); + }, + testTimeout, + ); + }); +}); From 0d0c76aa6f9b875fe463af84f16cb7812b899cde Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Tue, 30 Mar 2021 22:51:38 +0200 Subject: [PATCH 3/5] feat: support lowercase http_proxy envvars --- src/lib/request/request.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/lib/request/request.ts b/src/lib/request/request.ts index 42ca5162a7..2580347753 100644 --- a/src/lib/request/request.ts +++ b/src/lib/request/request.ts @@ -21,6 +21,16 @@ declare const global: Global; export = function makeRequest( payload: Payload, ): Promise<{ res: needle.NeedleResponse; body: any }> { + // This ensures we support lowercase http(s)_proxy values as well + // The weird IF around it ensures we don't create an envvar with a value of undefined, which throws error when trying to use it as a proxy + if (process.env.HTTP_PROXY || process.env.http_proxy) { + process.env.HTTP_PROXY = process.env.HTTP_PROXY || process.env.http_proxy; + } + if (process.env.HTTPS_PROXY || process.env.https_proxy) { + process.env.HTTPS_PROXY = + process.env.HTTPS_PROXY || process.env.https_proxy; + } + return getVersion().then( (versionNumber) => new Promise((resolve, reject) => { From 8045cebb56119626c8f7d5a04f6fd85025496560 Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Tue, 30 Mar 2021 22:52:09 +0200 Subject: [PATCH 4/5] test: update proxy tests for the new proxy global-agent --- test/jest/acceptance/proxy-behavior.spec.ts | 10 ++-- test/proxy.test.js | 57 +++++++++++++++------ 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/test/jest/acceptance/proxy-behavior.spec.ts b/test/jest/acceptance/proxy-behavior.spec.ts index 972c8a6dc8..0a9f587b6d 100644 --- a/test/jest/acceptance/proxy-behavior.spec.ts +++ b/test/jest/acceptance/proxy-behavior.spec.ts @@ -25,8 +25,9 @@ describe('Proxy configuration behavior', () => { throw err; } + // It will *attempt* to connect to a FAKE_HTTP_PROXY (and fails, because it's not a real proxy server) expect(stderr).toContain( - `snyk analytics Error: connect ECONNREFUSED 127.0.0.1:${ + `Error: connect ECONNREFUSED 127.0.0.1:${ FAKE_HTTP_PROXY.split(':')[2] }`, ); @@ -53,6 +54,7 @@ describe('Proxy configuration behavior', () => { throw err; } + // It will *not attempt* to connect to a proxy and /analytics call won't fail expect(stderr).not.toContain('ECONNREFUSED'); done(); }, @@ -80,8 +82,9 @@ describe('Proxy configuration behavior', () => { throw err; } + // It will *attempt* to connect to a FAKE_HTTP_PROXY (and fails, because it's not a real proxy server) expect(stderr).toContain( - `snyk analytics Error: connect ECONNREFUSED 127.0.0.1:${ + `Error: connect ECONNREFUSED 127.0.0.1:${ FAKE_HTTP_PROXY.split(':')[2] }`, ); @@ -105,7 +108,8 @@ describe('Proxy configuration behavior', () => { }, }, (err, stdout, stderr) => { - // TODO: incorrect behavior when Needle tries to upgrade connection after 301 http->https and the Agent option is set to a strict protocol + // TODO: incorrect behavior when Needle tries to upgrade connection after 301 http->https and the Agent option is set to a strict http/s protocol + // See lines with `keepAlive` in request.ts for more details expect(stderr).toContain( 'TypeError [ERR_INVALID_PROTOCOL]: Protocol "https:" not supported. Expected "http:"', ); diff --git a/test/proxy.test.js b/test/proxy.test.js index 7f744a525a..659f15e679 100644 --- a/test/proxy.test.js +++ b/test/proxy.test.js @@ -1,14 +1,14 @@ -var tap = require('tap'); -var test = tap.test; -var url = require('url'); -var http = require('http'); -var nock = require('nock'); -var request = require('../src/lib/request'); - -var proxyPort = 4242; -var httpRequestHost = 'http://localhost:8000'; -var httpsRequestHost = 'https://snyk.io:443'; -var requestPath = '/api/v1/verify/token'; +const tap = require('tap'); +const test = tap.test; +const url = require('url'); +const http = require('http'); +const nock = require('nock'); +const request = require('../src/lib/request'); + +const proxyPort = 4242; +const httpRequestHost = 'http://localhost:8000'; +const httpsRequestHost = 'https://snyk.io:443'; +const requestPath = '/api/v1/verify/token'; /** * Verify support for http(s) proxy from environments variables @@ -51,10 +51,12 @@ test('request respects proxy environment variables', function(t) { t.teardown(() => { process.env.NO_PROXY = tmpNoProxy; delete process.env.http_proxy; + delete process.env.HTTP_PROXY; + delete global.GLOBAL_AGENT; }); process.env.http_proxy = `http://localhost:${proxyPort}`; - var proxy = http.createServer(function(req, res) { + const proxy = http.createServer(function(req, res) { t.equal(req.url, httpRequestHost + requestPath, 'http_proxy url ok'); res.end(); }); @@ -63,6 +65,7 @@ test('request respects proxy environment variables', function(t) { return request({ method: 'post', url: httpRequestHost + requestPath }) .catch((err) => t.fail(err.message)) .then(() => { + t.equal(process.env.http_proxy, process.env.HTTP_PROXY); proxy.close(); }); }); @@ -76,6 +79,7 @@ test('request respects proxy environment variables', function(t) { t.teardown(() => { process.env.NO_PROXY = tmpNoProxy; delete process.env.HTTP_PROXY; + delete global.GLOBAL_AGENT; }); process.env.HTTP_PROXY = `http://localhost:${proxyPort}`; @@ -93,8 +97,20 @@ test('request respects proxy environment variables', function(t) { }); t.test('https_proxy', function(t) { + // NO_PROXY is set in CircleCI and brakes test purpose + const tmpNoProxy = process.env.NO_PROXY; + delete process.env.NO_PROXY; + + t.teardown(() => { + process.env.NO_PROXY = tmpNoProxy; + delete process.env.https_proxy; + delete process.env.HTTPS_PROXY; + delete global.GLOBAL_AGENT; + }); + + // eslint-disable-next-line @typescript-eslint/camelcase process.env.https_proxy = `http://localhost:${proxyPort}`; - var proxy = http.createServer(); + const proxy = http.createServer(); proxy.setTimeout(1000); proxy.on('connect', (req, cltSocket, head) => { const proxiedUrl = url.parse(`https://${req.url}`); @@ -123,14 +139,24 @@ test('request respects proxy environment variables', function(t) { return request({ method: 'post', url: httpsRequestHost + requestPath }) .catch(() => {}) // client socket being closed generates an error here .then(() => { + t.equal(process.env.https_proxy, process.env.HTTPS_PROXY); proxy.close(); - delete process.env.https_proxy; }); }); t.test('HTTPS_PROXY', function(t) { + // NO_PROXY is set in CircleCI and brakes test purpose + const tmpNoProxy = process.env.NO_PROXY; + delete process.env.NO_PROXY; + + t.teardown(() => { + process.env.NO_PROXY = tmpNoProxy; + delete process.env.HTTPS_PROXY; + delete global.GLOBAL_AGENT; + }); + process.env.HTTPS_PROXY = `http://localhost:${proxyPort}`; - var proxy = http.createServer(); + const proxy = http.createServer(); proxy.setTimeout(1000); proxy.on('connect', (req, cltSocket, head) => { const proxiedUrl = url.parse(`https://${req.url}`); @@ -160,7 +186,6 @@ test('request respects proxy environment variables', function(t) { .catch(() => {}) // client socket being closed generates an error here .then(() => { proxy.close(); - delete process.env.HTTPS_PROXY; }); }); }); From eec11b7f2c75064dbab2c2e5dfb6a0d5bd0af3d2 Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Tue, 30 Mar 2021 23:10:37 +0200 Subject: [PATCH 5/5] test: raise timeout for snyk protect tests hitting real Snyk API --- .../test/unit/protect-unit-tests.spec.ts | 74 +++++++++++-------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/packages/snyk-protect/test/unit/protect-unit-tests.spec.ts b/packages/snyk-protect/test/unit/protect-unit-tests.spec.ts index 6902998c65..e7bf6a84a8 100644 --- a/packages/snyk-protect/test/unit/protect-unit-tests.spec.ts +++ b/packages/snyk-protect/test/unit/protect-unit-tests.spec.ts @@ -5,7 +5,13 @@ import { PhysicalModuleToPatch, PackageAndVersion } from '../../src/lib/types'; import { extractPatchMetadata } from '../../src/lib/snyk-file'; import { checkProject } from '../../src/lib/explore-node-modules'; import { getPatches } from '../../src/lib/get-patches'; -import { extractTargetFilePathFromPatch, patchString } from '../../src/lib/patch'; +import { + extractTargetFilePathFromPatch, + patchString, +} from '../../src/lib/patch'; + +// TODO: lower it once Protect stops hitting real Snyk API endpoints +const testTimeout = 30000; describe('parsing .snyk file content', () => { it('works with a single patch', () => { @@ -44,7 +50,7 @@ patch: SNYK-JS-LODASH-567746: - tap > nyc > istanbul-lib-instrument > babel-types > lodash: patched: '2021-02-17T13:43:51.857Z' - + SNYK-FAKE-THEMODULE-000000: - top-level > some-other > the-module: patched: '2021-02-17T13:43:51.857Z' @@ -173,35 +179,43 @@ describe('checkProject', () => { // These tests makes a real API calls to Snyk // TODO: would be better to mock the response describe('getPatches', () => { - it('seems to work', async () => { - const packageAndVersions: PackageAndVersion[] = [ - { - name: 'lodash', - version: '4.17.15', - } as PackageAndVersion, - ]; - const vulnIds = ['SNYK-JS-LODASH-567746']; - const patches = await getPatches(packageAndVersions, vulnIds); - expect(Object.keys(patches)).toEqual(['lodash']); - const lodashPatches = patches['lodash']; - expect(lodashPatches).toHaveLength(1); - const theOnePatch = lodashPatches[0]; - expect(theOnePatch.id).toBe('patch:SNYK-JS-LODASH-567746:0'); - expect(theOnePatch.diffs).toHaveLength(1); - expect(theOnePatch.diffs[0]).toContain('index 9b95dfef..43e71ffb 100644'); // something from the actual patch - }); + it( + 'seems to work', + async () => { + const packageAndVersions: PackageAndVersion[] = [ + { + name: 'lodash', + version: '4.17.15', + } as PackageAndVersion, + ]; + const vulnIds = ['SNYK-JS-LODASH-567746']; + const patches = await getPatches(packageAndVersions, vulnIds); + expect(Object.keys(patches)).toEqual(['lodash']); + const lodashPatches = patches['lodash']; + expect(lodashPatches).toHaveLength(1); + const theOnePatch = lodashPatches[0]; + expect(theOnePatch.id).toBe('patch:SNYK-JS-LODASH-567746:0'); + expect(theOnePatch.diffs).toHaveLength(1); + expect(theOnePatch.diffs[0]).toContain('index 9b95dfef..43e71ffb 100644'); // something from the actual patch + }, + testTimeout, + ); - it('does not download patch for non-applicable version', async () => { - const packageAndVersions: PackageAndVersion[] = [ - { - name: 'lodash', - version: '4.17.20', // this version is not applicable to the patch - } as PackageAndVersion, - ]; - const vulnIds = ['SNYK-JS-LODASH-567746']; - const patches = await getPatches(packageAndVersions, vulnIds); - expect(patches).toEqual({}); // expect nothing to be returned because SNYK-JS-LODASH-567746 does not apply to 4.17.20 of lodash - }); + it( + 'does not download patch for non-applicable version', + async () => { + const packageAndVersions: PackageAndVersion[] = [ + { + name: 'lodash', + version: '4.17.20', // this version is not applicable to the patch + } as PackageAndVersion, + ]; + const vulnIds = ['SNYK-JS-LODASH-567746']; + const patches = await getPatches(packageAndVersions, vulnIds); + expect(patches).toEqual({}); // expect nothing to be returned because SNYK-JS-LODASH-567746 does not apply to 4.17.20 of lodash + }, + testTimeout, + ); }); describe('applying patches', () => {