Skip to content

Commit 8987918

Browse files
JackuBJackuB
authoredMar 30, 2021
Merge pull request #1781 from snyk/fix/replace-proxy
fix: replace vulnerable proxy dependency
·
v1.1296.1v1.518.0
2 parents 1449c57 + eec11b7 commit 8987918

File tree

6 files changed

+225
-60
lines changed

6 files changed

+225
-60
lines changed
 

‎package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@
8686
"configstore": "^5.0.1",
8787
"debug": "^4.1.1",
8888
"diff": "^4.0.1",
89+
"global-agent": "^2.1.12",
8990
"hcl-to-json": "^0.1.1",
9091
"lodash.assign": "^4.2.0",
9192
"lodash.camelcase": "^4.3.0",
@@ -110,7 +111,6 @@
110111
"ora": "5.3.0",
111112
"os-name": "^3.0.0",
112113
"promise-queue": "^2.2.5",
113-
"proxy-agent": "^3.1.1",
114114
"proxy-from-env": "^1.0.0",
115115
"rimraf": "^2.6.3",
116116
"semver": "^6.0.0",

‎packages/snyk-protect/test/unit/protect-unit-tests.spec.ts

+44-30
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,13 @@ import { PhysicalModuleToPatch, PackageAndVersion } from '../../src/lib/types';
55
import { extractPatchMetadata } from '../../src/lib/snyk-file';
66
import { checkProject } from '../../src/lib/explore-node-modules';
77
import { getPatches } from '../../src/lib/get-patches';
8-
import { extractTargetFilePathFromPatch, patchString } from '../../src/lib/patch';
8+
import {
9+
extractTargetFilePathFromPatch,
10+
patchString,
11+
} from '../../src/lib/patch';
12+
13+
// TODO: lower it once Protect stops hitting real Snyk API endpoints
14+
const testTimeout = 30000;
915

1016
describe('parsing .snyk file content', () => {
1117
it('works with a single patch', () => {
@@ -44,7 +50,7 @@ patch:
4450
SNYK-JS-LODASH-567746:
4551
- tap > nyc > istanbul-lib-instrument > babel-types > lodash:
4652
patched: '2021-02-17T13:43:51.857Z'
47-
53+
4854
SNYK-FAKE-THEMODULE-000000:
4955
- top-level > some-other > the-module:
5056
patched: '2021-02-17T13:43:51.857Z'
@@ -173,35 +179,43 @@ describe('checkProject', () => {
173179
// These tests makes a real API calls to Snyk
174180
// TODO: would be better to mock the response
175181
describe('getPatches', () => {
176-
it('seems to work', async () => {
177-
const packageAndVersions: PackageAndVersion[] = [
178-
{
179-
name: 'lodash',
180-
version: '4.17.15',
181-
} as PackageAndVersion,
182-
];
183-
const vulnIds = ['SNYK-JS-LODASH-567746'];
184-
const patches = await getPatches(packageAndVersions, vulnIds);
185-
expect(Object.keys(patches)).toEqual(['lodash']);
186-
const lodashPatches = patches['lodash'];
187-
expect(lodashPatches).toHaveLength(1);
188-
const theOnePatch = lodashPatches[0];
189-
expect(theOnePatch.id).toBe('patch:SNYK-JS-LODASH-567746:0');
190-
expect(theOnePatch.diffs).toHaveLength(1);
191-
expect(theOnePatch.diffs[0]).toContain('index 9b95dfef..43e71ffb 100644'); // something from the actual patch
192-
});
182+
it(
183+
'seems to work',
184+
async () => {
185+
const packageAndVersions: PackageAndVersion[] = [
186+
{
187+
name: 'lodash',
188+
version: '4.17.15',
189+
} as PackageAndVersion,
190+
];
191+
const vulnIds = ['SNYK-JS-LODASH-567746'];
192+
const patches = await getPatches(packageAndVersions, vulnIds);
193+
expect(Object.keys(patches)).toEqual(['lodash']);
194+
const lodashPatches = patches['lodash'];
195+
expect(lodashPatches).toHaveLength(1);
196+
const theOnePatch = lodashPatches[0];
197+
expect(theOnePatch.id).toBe('patch:SNYK-JS-LODASH-567746:0');
198+
expect(theOnePatch.diffs).toHaveLength(1);
199+
expect(theOnePatch.diffs[0]).toContain('index 9b95dfef..43e71ffb 100644'); // something from the actual patch
200+
},
201+
testTimeout,
202+
);
193203

194-
it('does not download patch for non-applicable version', async () => {
195-
const packageAndVersions: PackageAndVersion[] = [
196-
{
197-
name: 'lodash',
198-
version: '4.17.20', // this version is not applicable to the patch
199-
} as PackageAndVersion,
200-
];
201-
const vulnIds = ['SNYK-JS-LODASH-567746'];
202-
const patches = await getPatches(packageAndVersions, vulnIds);
203-
expect(patches).toEqual({}); // expect nothing to be returned because SNYK-JS-LODASH-567746 does not apply to 4.17.20 of lodash
204-
});
204+
it(
205+
'does not download patch for non-applicable version',
206+
async () => {
207+
const packageAndVersions: PackageAndVersion[] = [
208+
{
209+
name: 'lodash',
210+
version: '4.17.20', // this version is not applicable to the patch
211+
} as PackageAndVersion,
212+
];
213+
const vulnIds = ['SNYK-JS-LODASH-567746'];
214+
const patches = await getPatches(packageAndVersions, vulnIds);
215+
expect(patches).toEqual({}); // expect nothing to be returned because SNYK-JS-LODASH-567746 does not apply to 4.17.20 of lodash
216+
},
217+
testTimeout,
218+
);
205219
});
206220

207221
describe('applying patches', () => {

‎src/lib/request/request.ts

+14-3
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import * as querystring from 'querystring';
55
import * as zlib from 'zlib';
66
import * as config from '../config';
77
import { getProxyForUrl } from 'proxy-from-env';
8-
import * as ProxyAgent from 'proxy-agent';
8+
import { bootstrap } from 'global-agent';
99
import * as analytics from '../analytics';
1010
import { Global } from '../../cli/args';
1111
import { Payload } from './types';
@@ -21,6 +21,16 @@ declare const global: Global;
2121
export = function makeRequest(
2222
payload: Payload,
2323
): Promise<{ res: needle.NeedleResponse; body: any }> {
24+
// This ensures we support lowercase http(s)_proxy values as well
25+
// 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
26+
if (process.env.HTTP_PROXY || process.env.http_proxy) {
27+
process.env.HTTP_PROXY = process.env.HTTP_PROXY || process.env.http_proxy;
28+
}
29+
if (process.env.HTTPS_PROXY || process.env.https_proxy) {
30+
process.env.HTTPS_PROXY =
31+
process.env.HTTPS_PROXY || process.env.https_proxy;
32+
}
33+
2434
return getVersion().then(
2535
(versionNumber) =>
2636
new Promise((resolve, reject) => {
@@ -120,8 +130,9 @@ export = function makeRequest(
120130
const proxyUri = getProxyForUrl(url);
121131
if (proxyUri) {
122132
snykDebug('using proxy:', proxyUri);
123-
// proxyAgent type is an EventEmitter and not an http Agent
124-
options.agent = (new ProxyAgent(proxyUri) as unknown) as http.Agent;
133+
bootstrap({
134+
environmentVariableNamespace: '',
135+
});
125136
} else {
126137
snykDebug('not using proxy');
127138
}

‎test/jest/acceptance/proxy-behavior.spec.ts

+123
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import { exec } from 'child_process';
2+
import { sep } from 'path';
3+
const main = './dist/cli/index.js'.replace(/\//g, sep);
4+
5+
const SNYK_API_HTTPS = 'https://snyk.io/api/v1';
6+
const SNYK_API_HTTP = 'http://snyk.io/api/v1';
7+
const FAKE_HTTP_PROXY = 'http://localhost:12345';
8+
const testTimeout = 50000;
9+
10+
describe('Proxy configuration behavior', () => {
11+
describe('*_PROXY against HTTPS host', () => {
12+
it(
13+
'tries to connect to the HTTPS_PROXY when HTTPS_PROXY is set',
14+
(done) => {
15+
exec(
16+
`node ${main} woof -d`,
17+
{
18+
env: {
19+
HTTPS_PROXY: FAKE_HTTP_PROXY,
20+
SNYK_API: SNYK_API_HTTPS,
21+
},
22+
},
23+
(err, stdout, stderr) => {
24+
if (err) {
25+
throw err;
26+
}
27+
28+
// It will *attempt* to connect to a FAKE_HTTP_PROXY (and fails, because it's not a real proxy server)
29+
expect(stderr).toContain(
30+
`Error: connect ECONNREFUSED 127.0.0.1:${
31+
FAKE_HTTP_PROXY.split(':')[2]
32+
}`,
33+
);
34+
done();
35+
},
36+
);
37+
},
38+
testTimeout,
39+
);
40+
41+
it(
42+
'does not try to connect to the HTTP_PROXY when it is set',
43+
(done) => {
44+
exec(
45+
`node ${main} woof -d`,
46+
{
47+
env: {
48+
HTTP_PROXY: FAKE_HTTP_PROXY,
49+
SNYK_API: SNYK_API_HTTPS,
50+
},
51+
},
52+
(err, stdout, stderr) => {
53+
if (err) {
54+
throw err;
55+
}
56+
57+
// It will *not attempt* to connect to a proxy and /analytics call won't fail
58+
expect(stderr).not.toContain('ECONNREFUSED');
59+
done();
60+
},
61+
);
62+
},
63+
testTimeout,
64+
);
65+
});
66+
67+
describe('*_PROXY against HTTP host', () => {
68+
it(
69+
'tries to connect to the HTTP_PROXY when HTTP_PROXY is set',
70+
(done) => {
71+
exec(
72+
`node ${main} woof -d`,
73+
{
74+
env: {
75+
HTTP_PROXY: FAKE_HTTP_PROXY,
76+
SNYK_API: SNYK_API_HTTP,
77+
SNYK_HTTP_PROTOCOL_UPGRADE: '0',
78+
},
79+
},
80+
(err, stdout, stderr) => {
81+
if (err) {
82+
throw err;
83+
}
84+
85+
// It will *attempt* to connect to a FAKE_HTTP_PROXY (and fails, because it's not a real proxy server)
86+
expect(stderr).toContain(
87+
`Error: connect ECONNREFUSED 127.0.0.1:${
88+
FAKE_HTTP_PROXY.split(':')[2]
89+
}`,
90+
);
91+
done();
92+
},
93+
);
94+
},
95+
testTimeout,
96+
);
97+
98+
it(
99+
'does not try to connect to the HTTPS_PROXY when it is set',
100+
(done) => {
101+
exec(
102+
`node ${main} woof -d`,
103+
{
104+
env: {
105+
HTTPS_PROXY: FAKE_HTTP_PROXY,
106+
SNYK_API: SNYK_API_HTTP,
107+
SNYK_HTTP_PROTOCOL_UPGRADE: '0',
108+
},
109+
},
110+
(err, stdout, stderr) => {
111+
// 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
112+
// See lines with `keepAlive` in request.ts for more details
113+
expect(stderr).toContain(
114+
'TypeError [ERR_INVALID_PROTOCOL]: Protocol "https:" not supported. Expected "http:"',
115+
);
116+
done();
117+
},
118+
);
119+
},
120+
testTimeout,
121+
);
122+
});
123+
});

‎test/proxy.test.js

+41-16
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
1-
var tap = require('tap');
2-
var test = tap.test;
3-
var url = require('url');
4-
var http = require('http');
5-
var nock = require('nock');
6-
var request = require('../src/lib/request');
7-
8-
var proxyPort = 4242;
9-
var httpRequestHost = 'http://localhost:8000';
10-
var httpsRequestHost = 'https://snyk.io:443';
11-
var requestPath = '/api/v1/verify/token';
1+
const tap = require('tap');
2+
const test = tap.test;
3+
const url = require('url');
4+
const http = require('http');
5+
const nock = require('nock');
6+
const request = require('../src/lib/request');
7+
8+
const proxyPort = 4242;
9+
const httpRequestHost = 'http://localhost:8000';
10+
const httpsRequestHost = 'https://snyk.io:443';
11+
const requestPath = '/api/v1/verify/token';
1212

1313
/**
1414
* Verify support for http(s) proxy from environments variables
@@ -51,10 +51,12 @@ test('request respects proxy environment variables', function(t) {
5151
t.teardown(() => {
5252
process.env.NO_PROXY = tmpNoProxy;
5353
delete process.env.http_proxy;
54+
delete process.env.HTTP_PROXY;
55+
delete global.GLOBAL_AGENT;
5456
});
5557

5658
process.env.http_proxy = `http://localhost:${proxyPort}`;
57-
var proxy = http.createServer(function(req, res) {
59+
const proxy = http.createServer(function(req, res) {
5860
t.equal(req.url, httpRequestHost + requestPath, 'http_proxy url ok');
5961
res.end();
6062
});
@@ -63,6 +65,7 @@ test('request respects proxy environment variables', function(t) {
6365
return request({ method: 'post', url: httpRequestHost + requestPath })
6466
.catch((err) => t.fail(err.message))
6567
.then(() => {
68+
t.equal(process.env.http_proxy, process.env.HTTP_PROXY);
6669
proxy.close();
6770
});
6871
});
@@ -76,6 +79,7 @@ test('request respects proxy environment variables', function(t) {
7679
t.teardown(() => {
7780
process.env.NO_PROXY = tmpNoProxy;
7881
delete process.env.HTTP_PROXY;
82+
delete global.GLOBAL_AGENT;
7983
});
8084

8185
process.env.HTTP_PROXY = `http://localhost:${proxyPort}`;
@@ -93,8 +97,20 @@ test('request respects proxy environment variables', function(t) {
9397
});
9498

9599
t.test('https_proxy', function(t) {
100+
// NO_PROXY is set in CircleCI and brakes test purpose
101+
const tmpNoProxy = process.env.NO_PROXY;
102+
delete process.env.NO_PROXY;
103+
104+
t.teardown(() => {
105+
process.env.NO_PROXY = tmpNoProxy;
106+
delete process.env.https_proxy;
107+
delete process.env.HTTPS_PROXY;
108+
delete global.GLOBAL_AGENT;
109+
});
110+
111+
// eslint-disable-next-line @typescript-eslint/camelcase
96112
process.env.https_proxy = `http://localhost:${proxyPort}`;
97-
var proxy = http.createServer();
113+
const proxy = http.createServer();
98114
proxy.setTimeout(1000);
99115
proxy.on('connect', (req, cltSocket, head) => {
100116
const proxiedUrl = url.parse(`https://${req.url}`);
@@ -123,14 +139,24 @@ test('request respects proxy environment variables', function(t) {
123139
return request({ method: 'post', url: httpsRequestHost + requestPath })
124140
.catch(() => {}) // client socket being closed generates an error here
125141
.then(() => {
142+
t.equal(process.env.https_proxy, process.env.HTTPS_PROXY);
126143
proxy.close();
127-
delete process.env.https_proxy;
128144
});
129145
});
130146

131147
t.test('HTTPS_PROXY', function(t) {
148+
// NO_PROXY is set in CircleCI and brakes test purpose
149+
const tmpNoProxy = process.env.NO_PROXY;
150+
delete process.env.NO_PROXY;
151+
152+
t.teardown(() => {
153+
process.env.NO_PROXY = tmpNoProxy;
154+
delete process.env.HTTPS_PROXY;
155+
delete global.GLOBAL_AGENT;
156+
});
157+
132158
process.env.HTTPS_PROXY = `http://localhost:${proxyPort}`;
133-
var proxy = http.createServer();
159+
const proxy = http.createServer();
134160
proxy.setTimeout(1000);
135161
proxy.on('connect', (req, cltSocket, head) => {
136162
const proxiedUrl = url.parse(`https://${req.url}`);
@@ -160,7 +186,6 @@ test('request respects proxy environment variables', function(t) {
160186
.catch(() => {}) // client socket being closed generates an error here
161187
.then(() => {
162188
proxy.close();
163-
delete process.env.HTTPS_PROXY;
164189
});
165190
});
166191
});

‎test/request.test.ts

+2-10
Original file line numberDiff line numberDiff line change
@@ -283,11 +283,7 @@ test('request with https proxy calls needle as expected', (t) => {
283283
follow_max: 5, // default
284284
timeout: 300000, // default
285285
json: undefined, // default
286-
agent: sinon.match({
287-
proxy: sinon.match({
288-
href: 'https://proxy:8443/', // should be set when using proxy
289-
}),
290-
}),
286+
agent: sinon.match.truthy,
291287
rejectUnauthorized: undefined, // should not be set when not use insecure mode
292288
}),
293289
sinon.match.func, // callback function
@@ -335,11 +331,7 @@ test('request with http proxy calls needle as expected', (t) => {
335331
follow_max: 5, // default
336332
timeout: 300000, // default
337333
json: undefined, // default
338-
agent: sinon.match({
339-
proxy: sinon.match({
340-
href: 'http://proxy:8080/', // should be set when using proxy
341-
}),
342-
}),
334+
agent: sinon.match.truthy,
343335
rejectUnauthorized: undefined, // should not be set when not use insecure mode
344336
}),
345337
sinon.match.func, // callback function

0 commit comments

Comments
 (0)
Please sign in to comment.