From c7bcc89ada0fb651b4a701cd23e44eabae24b449 Mon Sep 17 00:00:00 2001 From: gitphill Date: Mon, 7 Oct 2019 18:00:28 +0100 Subject: [PATCH] fix: fetch patch ignoring command line arguments BST-889 Fetch patch was making http requests by using needle directly Changed to use lib request which uses command line arguments when invoking needle Added tests for lib request Changed fetch patch fail test to typescript Removed patch fetch proxy test (now covered by lib request test) Added @types/sinon devDependency to help write test with sinon assertions and matchers --- package.json | 1 + src/lib/protect/fetch-patch.ts | 50 ++-- test/patch-fetch-fail.test.js | 38 --- test/patch-fetch-fail.test.ts | 131 ++++++++++ test/patch-fetch-proxy.test.js | 89 ------- test/request.test.ts | 433 +++++++++++++++++++++++++++++++++ 6 files changed, 581 insertions(+), 161 deletions(-) delete mode 100644 test/patch-fetch-fail.test.js create mode 100644 test/patch-fetch-fail.test.ts delete mode 100644 test/patch-fetch-proxy.test.js create mode 100644 test/request.test.ts diff --git a/package.json b/package.json index d1a0015cf1..49c50f9de8 100644 --- a/package.json +++ b/package.json @@ -103,6 +103,7 @@ "@types/lodash": "^4.14.136", "@types/needle": "^2.0.4", "@types/node": "^6.14.4", + "@types/sinon": "^7.5.0", "@typescript-eslint/eslint-plugin": "^2.0.0", "@typescript-eslint/parser": "^2.0.0", "eslint": "^5.16.0", diff --git a/src/lib/protect/fetch-patch.ts b/src/lib/protect/fetch-patch.ts index 8c2e950a20..8273b2dd3e 100644 --- a/src/lib/protect/fetch-patch.ts +++ b/src/lib/protect/fetch-patch.ts @@ -1,57 +1,39 @@ -import * as needle from 'needle'; import * as fs from 'fs'; import * as analytics from '../analytics'; import * as debugModule from 'debug'; -import * as proxyFromEnv from 'proxy-from-env'; -import * as ProxyAgent from 'proxy-agent'; +import request = require('../request'); const debug = debugModule('snyk:fetch-patch'); -const getProxyForUrl = proxyFromEnv.getProxyForUrl; async function getPatchFile( patchUrl: string, patchFilename: string, ): Promise { - let options; - const proxyUri = getProxyForUrl(patchUrl); - if (proxyUri) { - debug('Using proxy: ', proxyUri); - options = { agent: new ProxyAgent(proxyUri) }; - } - - let res: needle.NeedleResponse; - let patchData: string; try { - res = await needle('get', patchUrl, options || {}); - if (!res || res.statusCode !== 200) { - throw res; + const response = await request({ url: patchUrl }); + if ( + !response || + !response.res || + !response.body || + response.res.statusCode !== 200 + ) { + throw response; } - patchData = res.body; + fs.writeFileSync(patchFilename, response.body); debug( - `Successfully downloaded patch from ${patchUrl}, patch size ${patchData.length} bytes`, + `Fetched patch from ${patchUrl} to ${patchFilename}, patch size ${response.body.length} bytes`, ); } catch (error) { - debug(`Failed to download patch from ${patchUrl}`, error); + const errorMessage = `Failed to fetch patch from ${patchUrl} to ${patchFilename}`; + debug(errorMessage, error); analytics.add('patch-fetch-fail', { - message: error && (error.message || error.body), - code: error && error.statusCode, - }); - throw error; - } - - try { - fs.writeFileSync(patchFilename, patchData); - debug(`Successfully wrote patch to ${patchFilename}`); - } catch (error) { - debug(`Failed to write patch to ${patchFilename}`, error); - analytics.add('patch-fetch-fail', { - message: error && error.message, + message: (error && error.message) || errorMessage, + code: error && error.res && error.res.statusCode, patchFilename, patchUrl, }); - throw error; + throw new Error(errorMessage); } - return patchFilename; } diff --git a/test/patch-fetch-fail.test.js b/test/patch-fetch-fail.test.js deleted file mode 100644 index 7ef71c2e9f..0000000000 --- a/test/patch-fetch-fail.test.js +++ /dev/null @@ -1,38 +0,0 @@ -var test = require('tap').test; -var proxyquire = require('proxyquire'); -var analyticsEvent; - -var getPatchFile = proxyquire('../src/lib/protect/fetch-patch', { - fs: { - writeFileSync: function() {}, - }, - '../analytics': { - add: function(type, data) { - analyticsEvent = { type, data }; - }, - }, -}); - -test('Fetch does what it should when request works properly', (t) => { - return getPatchFile('http://httpstat.us/200', 'name') - .then((name) => t.is(name, 'name')) - .catch(() => t.fail('Rejected')); -}); - -test('Fetch fails with 404', (t) => { - return getPatchFile('http://httpstat.us/404', 'name') - .then(() => t.fail('Should have failed')) - .catch(() => { - t.is(analyticsEvent.type, 'patch-fetch-fail', 'proper analytics type'); - t.is(analyticsEvent.data.code, 404, 'proper analytics data code'); - }); -}); - -test('Fetch fails with 502', (t) => { - return getPatchFile('http://httpstat.us/502', 'name') - .then(() => t.fail('Should have failed')) - .catch(() => { - t.is(analyticsEvent.type, 'patch-fetch-fail', 'proper analytics type'); - t.is(analyticsEvent.data.code, 502, 'proper analytics data code'); - }); -}); diff --git a/test/patch-fetch-fail.test.ts b/test/patch-fetch-fail.test.ts new file mode 100644 index 0000000000..ab215a68c0 --- /dev/null +++ b/test/patch-fetch-fail.test.ts @@ -0,0 +1,131 @@ +import { test } from 'tap'; +import * as proxyquire from 'proxyquire'; + +let analytics; + +const proxyFetchPatch = proxyquire('../src/lib/protect/fetch-patch', { + '../request': () => { + return Promise.resolve({ + res: { + statusCode: 200, + }, + body: 'patch content', + }); + }, + fs: { + writeFileSync() { + return; + }, + }, + '../analytics': { + add(type, data) { + analytics = { type, data }; + }, + }, +}); + +const proxyFetchPatchNotFound = proxyquire('../src/lib/protect/fetch-patch', { + '../request': () => { + return Promise.resolve({ + res: { + statusCode: 404, + }, + body: 'not found', + }); + }, + '../analytics': { + add(type, data) { + analytics = { type, data }; + }, + }, +}); + +const proxyFetchPatchErrorWriting = proxyquire( + '../src/lib/protect/fetch-patch', + { + '../request': () => { + return Promise.resolve({ + res: { + statusCode: 200, + }, + body: 'patch content', + }); + }, + fs: { + writeFileSync() { + throw new Error('Error writing file'); + }, + }, + '../analytics': { + add(type, data) { + analytics = { type, data }; + }, + }, + }, +); + +test('fetch patch returns filename when successful', (t) => { + return proxyFetchPatch('https://test.patch.url', 'file.patch') + .then((name) => t.is(name, 'file.patch', 'filename returned')) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('fetch patch handles error responses', (t) => { + return proxyFetchPatchNotFound('https://test.patch.url', 'file.patch') + .then(() => t.fail('should have failed')) + .catch(() => { + t.is( + analytics.type, + 'patch-fetch-fail', + 'analytics type is patch-fetch-fail', + ); + t.is(analytics.data.code, 404, 'analytics status code is 404'); + t.is( + analytics.data.message, + 'Failed to fetch patch from https://test.patch.url to file.patch', + 'analytics message is expected', + ); + t.is( + analytics.data.patchFilename, + 'file.patch', + 'analytics patch filename is expected', + ); + t.is( + analytics.data.patchUrl, + 'https://test.patch.url', + 'analytics patch url is expected', + ); + }); +}); + +test('fetch patch handles errors thrown while writing file', (t) => { + return proxyFetchPatchErrorWriting('https://test.patch.url', 'file.patch') + .then(() => t.fail('should have failed')) + .catch(() => { + t.is( + analytics.type, + 'patch-fetch-fail', + 'analytics type is patch-fetch-fail', + ); + t.is( + analytics.data.code, + undefined, + 'analytics status code is undefined', + ); + t.is( + analytics.data.message, + 'Error writing file', + 'analytics message indicates error writing file', + ); + t.is( + analytics.data.patchFilename, + 'file.patch', + 'analytics patch filename is expected', + ); + t.is( + analytics.data.patchUrl, + 'https://test.patch.url', + 'analytics patch url is expected', + ); + }); +}); diff --git a/test/patch-fetch-proxy.test.js b/test/patch-fetch-proxy.test.js deleted file mode 100644 index 51413f5dbd..0000000000 --- a/test/patch-fetch-proxy.test.js +++ /dev/null @@ -1,89 +0,0 @@ -var test = require('tap').test; -var proxyquire = require('proxyquire'); -var sinon = require('sinon'); -var spy = sinon.spy(); - -var PROXY_HOST = 'my.proxy.com'; -var PROXY_PORT = 4242; -var PATCH_URL = - 'https://s3.amazonaws.com/snyk-rules-pre-repository/' + - 'snapshots/master/patches/npm/qs/20170213/603_604.patch'; - -var getPatchFile = proxyquire('../src/lib/protect/fetch-patch', { - fs: { - writeFileSync: function() {}, - }, - needle: function() { - spy(...arguments); - return Promise.resolve({ - statusCode: 200, - headers: {}, - body: 'some patch content', - }); - }, - '../analytics': { - add: function() {}, - }, -}); - -test('Fetch gets patches with no proxy', function(t) { - t.plan(1); - return getPatchFile(PATCH_URL, 'unused') - .then(() => { - t.is(spy.getCall(0).args[2].agent, undefined, 'no proxy agent found'); - }) - .catch((err) => t.fail(err.message)); -}); - -/** - * Verify support for http(s) proxy from environments variables - * (https_proxy, HTTPS_PROXY, no_proxy) - * see https://www.gnu.org/software/wget/manual/html_node/Proxies.html - */ -test('proxy environment variables', function(t) { - t.plan(3); - - t.test('https_proxy', function(t) { - var proxyUri = 'http://' + PROXY_HOST + ':' + PROXY_PORT; - process.env.https_proxy = proxyUri; - return getPatchFile(PATCH_URL, 'unused') - .then(() => { - t.is( - spy.getCall(1).args[2].agent.proxyUri, - proxyUri, - 'proxy url found', - ); - }) - .catch((err) => t.fail(err.message)) - .then(() => delete process.env.https_proxy); - }); - - t.test('HTTPS_PROXY', function(t) { - var proxyUri = 'http://' + PROXY_HOST + ':' + PROXY_PORT; - process.env.HTTPS_PROXY = proxyUri; - return getPatchFile(PATCH_URL, 'unused') - .then(() => { - t.is( - spy.getCall(2).args[2].agent.proxyUri, - proxyUri, - 'proxy url found', - ); - }) - .catch((err) => t.fail(err.message)) - .then(() => delete process.env.HTTPS_PROXY); - }); - - t.test('no_proxy', function(t) { - process.env.https_proxy = 'http://' + PROXY_HOST + ':' + PROXY_PORT; - process.env.no_proxy = '*'; - return getPatchFile(PATCH_URL, 'unused') - .then(() => { - t.is(spy.getCall(3).args[2].agent, undefined, 'no proxy agent found'); - }) - .catch((err) => t.fail(err.message)) - .then(() => { - delete process.env.https_proxy; - delete process.env.no_proxy; - }); - }); -}); diff --git a/test/request.test.ts b/test/request.test.ts new file mode 100644 index 0000000000..50bc160cf7 --- /dev/null +++ b/test/request.test.ts @@ -0,0 +1,433 @@ +import { afterEach, test } from 'tap'; +import * as sinon from 'sinon'; +import * as proxyquire from 'proxyquire'; +import * as needle from 'needle'; +import { Global } from '../src/cli/args'; + +declare const global: Global; + +const needleStub = sinon.stub(needle, 'request'); + +const request = proxyquire('../src/lib/request', { + needle: { + request: needleStub, + }, +}); + +afterEach((done, t) => { + needleStub.resetHistory(); + delete process.env.https_proxy; + delete process.env.http_proxy; + delete process.env.no_proxy; + global.ignoreUnknownCA = false; + done(); +}); + +test('request calls needle as expected and returns status code and body', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://test.stub', + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: undefined, // default + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request to localhost calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://localhost', + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'http://localhost', // does not force https + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: undefined, // default + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with timeout calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://test.stub', + timeout: 100000, + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 100000, // provided + json: undefined, // default + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with query string calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://test.stub', + qs: { + key: 'value', + test: ['multi', 'value'], + }, + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/?key=value&test=multi&test=value', // turns http to https and appends querystring + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: undefined, // default + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with json calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://test.stub', + json: false, + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: false, // provided + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with custom header calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://test.stub', + headers: { + custom: 'header', + }, + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + custom: 'header', // provided + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: undefined, // default + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with https proxy calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + process.env.https_proxy = 'https://proxy:8443'; + const payload = { + url: 'http://test.stub', + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + 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 + }), + }), + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with http proxy calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + process.env.http_proxy = 'http://proxy:8080'; + const payload = { + url: 'http://localhost', + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'http://localhost', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + 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 + }), + }), + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with no proxy calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + process.env.http_proxy = 'http://proxy:8080'; + process.env.no_proxy = 'localhost'; + const payload = { + url: 'http://localhost', + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'http://localhost', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: undefined, // default + agent: undefined, // should not be set when no proxy + rejectUnauthorized: undefined, // should not be set when not use insecure mode + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request with insecure calls needle as expected', (t) => { + needleStub.yields(null, { statusCode: 200 }, 'text'); + global.ignoreUnknownCA = true; + const payload = { + url: 'http://test.stub', + }; + return request(payload) + .then((response) => { + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'https://test.stub/', // turns http to https and appends / + sinon.match.falsy, // no data + sinon.match({ + headers: sinon.match({ + 'x-snyk-cli-version': sinon.match.string, // dynamic version + 'content-encoding': undefined, // should not set when no data + 'content-length': undefined, // should not be set when no data + }), + follow_max: 5, // default + timeout: 300000, // default + json: undefined, // default + agent: undefined, // should not be set when not using proxy + rejectUnauthorized: false, // should be false when insecure mode enabled + }), + sinon.match.func, // callback function + ), + 'needle called as expected', + ); + }) + .catch((e) => t.fail('should not throw error', e)); +}); + +test('request rejects if needle fails', (t) => { + needleStub.yields( + 'Unexpected Error', + { statusCode: 500 }, + 'Internal Server Error', + ); + const payload = { + url: 'http://test.stub', + }; + return request(payload) + .then(() => t.fail('should have failed')) + .catch((e) => { + t.equals(e, 'Unexpected Error', 'rejects error'); + }); +});