Skip to content

Commit

Permalink
Merge pull request #1781 from snyk/fix/replace-proxy
Browse files Browse the repository at this point in the history
fix: replace vulnerable proxy dependency
  • Loading branch information
JackuB authored Mar 30, 2021
2 parents 1449c57 + eec11b7 commit 8987918
Show file tree
Hide file tree
Showing 6 changed files with 225 additions and 60 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
74 changes: 44 additions & 30 deletions packages/snyk-protect/test/unit/protect-unit-tests.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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', () => {
Expand Down
17 changes: 14 additions & 3 deletions src/lib/request/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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) => {
Expand Down Expand Up @@ -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');
}
Expand Down
123 changes: 123 additions & 0 deletions test/jest/acceptance/proxy-behavior.spec.ts
Original file line number Diff line number Diff line change
@@ -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,
);
});
});
57 changes: 41 additions & 16 deletions test/proxy.test.js
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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();
});
Expand All @@ -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();
});
});
Expand All @@ -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}`;
Expand All @@ -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}`);
Expand Down Expand Up @@ -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}`);
Expand Down Expand Up @@ -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;
});
});
});
Loading

0 comments on commit 8987918

Please sign in to comment.