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/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', () => { diff --git a/src/lib/request/request.ts b/src/lib/request/request.ts index 1a0772df10..2580347753 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'; @@ -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) => { @@ -120,8 +130,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/jest/acceptance/proxy-behavior.spec.ts b/test/jest/acceptance/proxy-behavior.spec.ts new file mode 100644 index 0000000000..0a9f587b6d --- /dev/null +++ b/test/jest/acceptance/proxy-behavior.spec.ts @@ -0,0 +1,123 @@ +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; + } + + // It will *attempt* to connect to a FAKE_HTTP_PROXY (and fails, because it's not a real proxy server) + expect(stderr).toContain( + `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; + } + + // It will *not attempt* to connect to a proxy and /analytics call won't fail + 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; + } + + // It will *attempt* to connect to a FAKE_HTTP_PROXY (and fails, because it's not a real proxy server) + expect(stderr).toContain( + `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 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:"', + ); + done(); + }, + ); + }, + testTimeout, + ); + }); +}); 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; }); }); }); 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