From 165b4b983afb238e75e3509ffd93a01eca0b7c93 Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Mon, 29 Mar 2021 14:43:40 +0200 Subject: [PATCH 1/2] feat: introduce envvar to control HTTP-HTTPS upgrade behavior --- help/commands-docs/_ENVIRONMENT.md | 27 ++++++++++++------- src/lib/request/request.ts | 8 ++++-- test/request.test.ts | 43 ++++++++++++++++++++++++++++-- 3 files changed, 65 insertions(+), 13 deletions(-) diff --git a/help/commands-docs/_ENVIRONMENT.md b/help/commands-docs/_ENVIRONMENT.md index e56fcfed29..b173949e0f 100644 --- a/help/commands-docs/_ENVIRONMENT.md +++ b/help/commands-docs/_ENVIRONMENT.md @@ -8,19 +8,28 @@ You can set these environment variables to change CLI run settings. [How to get your account token](https://snyk.co/ucT6J)
[How to use Service Accounts](https://snyk.co/ucT6L)
-- `SNYK_API`: - Sets API host to use for Snyk requests. Useful for on-premise instances and configuring proxies. - - `SNYK_CFG_`: Allows you to override any key that's also available as `snyk config` option. E.g. `SNYK_CFG_ORG`=myorg will override default org option in `config` with "myorg". - `SNYK_REGISTRY_USERNAME`: - Specify a username to use when connecting to a container registry. Note that using the `--username` flag will - override this value. This will be ignored in favour of local Docker binary credentials when Docker is present. - + Specify a username to use when connecting to a container registry. Note that using the `--username` flag will + override this value. This will be ignored in favour of local Docker binary credentials when Docker is present. + - `SNYK_REGISTRY_PASSWORD`: - Specify a password to use when connecting to a container registry. Note that using the `--password` flag will - override this value. This will be ignored in favour of local Docker binary credentials when Docker is present. - + Specify a password to use when connecting to a container registry. Note that using the `--password` flag will + override this value. This will be ignored in favour of local Docker binary credentials when Docker is present. + +## Connecting to Snyk API + +By default Snyk CLI will connect to `https://snyk.io/api/v1`. + +- `SNYK_API`: + Sets API host to use for Snyk requests. Useful for on-premise instances and configuring proxies. If set with `http` protocol CLI will upgrade the requests to `https`. Unless `SNYK_HTTP_PROTOCOL_UPGRADE` is set to `0`. + +- `SNYK_HTTP_PROTOCOL_UPGRADE`=0: + If set to the value of `0` the API requests aimed at `http` URLs will not be upgraded to `https`. Useful e.g., for reverse proxies. + +- `HTTPS_PROXY` and `HTTP_PROXY`: + Allows you to specify a proxy to use for `https` and `http` calls. The `https` in the `HTTPS_PROXY` means that _requests using `https` protocol_ will use this proxy. The proxy itself doesn't need to use `https`. diff --git a/src/lib/request/request.ts b/src/lib/request/request.ts index 0b51c14e1a..1a0772df10 100644 --- a/src/lib/request/request.ts +++ b/src/lib/request/request.ts @@ -71,7 +71,8 @@ export = function makeRequest( if ( parsedUrl.protocol === 'http:' && - parsedUrl.hostname !== 'localhost' + parsedUrl.hostname !== 'localhost' && + process.env.SNYK_HTTP_PROTOCOL_UPGRADE !== '0' ) { debug('forcing api request to https'); parsedUrl.protocol = 'https:'; @@ -95,7 +96,10 @@ export = function makeRequest( let url = payload.url; if (payload.qs) { - url = url + '?' + querystring.stringify(payload.qs); + // Parse the URL and append the search part - this will take care of adding the '/?' part if it's missing + const urlObject = new URL(url); + urlObject.search = querystring.stringify(payload.qs); + url = urlObject.toString(); delete payload.qs; } diff --git a/test/request.test.ts b/test/request.test.ts index d8b8fb92c8..dc455e5bd1 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -139,7 +139,7 @@ test('request with timeout calls needle as expected', (t) => { test('request with query string calls needle as expected', (t) => { needleStub.yields(null, { statusCode: 200 }, 'text'); const payload = { - url: 'http://test.stub', + url: 'https://test.stub', qs: { key: 'value', test: ['multi', 'value'], @@ -155,7 +155,7 @@ test('request with query string calls needle as expected', (t) => { t.ok( needleStub.calledWith( 'get', // default - 'https://test.stub/?key=value&test=multi&test=value', // turns http to https and appends querystring + 'https://test.stub/?key=value&test=multi&test=value', // appends querystring sinon.match.falsy, // no data sinon.match({ headers: sinon.match({ @@ -442,3 +442,42 @@ test('request rejects if needle fails', (t) => { t.equals(e, 'Unexpected Error', 'rejects error'); }); }); + +test('request calls needle as expected and will not update HTTP to HTTPS if envvar is set', (t) => { + process.env.SNYK_HTTP_PROTOCOL_UPGRADE = '0'; + needleStub.yields(null, { statusCode: 200 }, 'text'); + const payload = { + url: 'http://test.stub', + }; + return request(payload) + .then((response) => { + process.env.SNYK_HTTP_PROTOCOL_UPGRADE = '1'; + t.deepEquals( + response, + { res: { statusCode: 200 }, body: 'text' }, + 'response ok', + ); + t.ok( + needleStub.calledWith( + 'get', // default + 'http://test.stub', // won't upgrade http to 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: sinon.match.instanceOf(http.Agent), + 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)); +}); From fdd7f1ac6e8f74e3a2c4bc5a1e2727e167c5bf02 Mon Sep 17 00:00:00 2001 From: Jakub Mikulas Date: Tue, 30 Mar 2021 10:47:54 +0200 Subject: [PATCH 2/2] docs: update SNYK_HTTP_PROTOCOL_UPGRADE description Co-authored-by: Jeff McLean <44034094+maxjeffos@users.noreply.github.com> --- help/commands-docs/_ENVIRONMENT.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/help/commands-docs/_ENVIRONMENT.md b/help/commands-docs/_ENVIRONMENT.md index b173949e0f..1335be046f 100644 --- a/help/commands-docs/_ENVIRONMENT.md +++ b/help/commands-docs/_ENVIRONMENT.md @@ -29,7 +29,7 @@ By default Snyk CLI will connect to `https://snyk.io/api/v1`. Sets API host to use for Snyk requests. Useful for on-premise instances and configuring proxies. If set with `http` protocol CLI will upgrade the requests to `https`. Unless `SNYK_HTTP_PROTOCOL_UPGRADE` is set to `0`. - `SNYK_HTTP_PROTOCOL_UPGRADE`=0: - If set to the value of `0` the API requests aimed at `http` URLs will not be upgraded to `https`. Useful e.g., for reverse proxies. + If set to the value of `0`, API requests aimed at `http` URLs will not be upgraded to `https`. If not set, the default behavior will be to upgrade these requests from `http` to `https`. Useful e.g., for reverse proxies. - `HTTPS_PROXY` and `HTTP_PROXY`: Allows you to specify a proxy to use for `https` and `http` calls. The `https` in the `HTTPS_PROXY` means that _requests using `https` protocol_ will use this proxy. The proxy itself doesn't need to use `https`.