Merge pull request #1778 from snyk/feat/dont-force-https

feat: introduce envvar to control HTTP-HTTPS upgrade behavior

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)<br />
   [How to use Service Accounts](https://snyk.co/ucT6L)<br />
 
-- `SNYK_API`:
-  Sets API host to use for Snyk requests. Useful for on-premise instances and configuring proxies.
-
 - `SNYK_CFG_`<KEY>:
   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`, 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`.

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;
         }
 

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));
+});