fix: visibly warn about proxy settings

lib/Dredd.js

@@ -15,19 +15,12 @@ const Runner = require('./TransactionRunner');
 const { applyConfiguration } = require('./configuration');
 
 
-const PROXY_ENV_VARIABLES = ['HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY'];
 const FILE_DOWNLOAD_TIMEOUT = 5000;
 
 
 class Dredd {
   constructor(config) {
-    this.init(config);
-  }
-
-  // This is here only because there there is no way how to spy a constructor in CoffeScript
-  init(config) {
     this.configuration = applyConfiguration(config);
-    this.configuration.http = {};
     this.stats = {
       tests: 0,
       failures: 0,
@@ -44,28 +37,7 @@ class Dredd {
     this.logger = logger;
   }
 
-  logProxySettings() {
-    const proxySettings = [];
-    for (const envVariableName of Object.keys(process.env || {})) {
-      const envVariableValue = process.env[envVariableName];
-      if (!Array.from(PROXY_ENV_VARIABLES).includes(envVariableName.toUpperCase())) { continue; }
-      if (envVariableValue === '') { continue; }
-      proxySettings.push(`${envVariableName}=${envVariableValue}`);
-    }
-
-    if (proxySettings.length) {
-      const message = `\
-HTTP(S) proxy specified by environment variables: \
-${proxySettings.join(', ')}. Please read documentation on how \
-Dredd works with proxies: \
-https://dredd.org/en/latest/how-it-works/#using-https-proxy`;
-      this.logger.debug(message);
-    }
-  }
-
   run(callback) {
-    this.logProxySettings();
-
     this.logger.debug('Configuring reporters');
     configureReporters(this.configuration, this.stats, this.runner);
 

lib/configuration.js

@@ -2,6 +2,7 @@ const clone = require('clone');
 const { EventEmitter } = require('events');
 
 const logger = require('./logger');
+const getProxySettings = require('./getProxySettings');
 const reporterOutputLogger = require('./reporters/reporterOutputLogger');
 
 
@@ -239,6 +240,22 @@ function applyConfiguration(inConfig) {
     throw new Error('Could not configure Dredd');
   }
 
+  // HTTP settings for the 'request' library. Currently cannot be customized
+  // by users, but in the future it should be. Leaking of 'request' library's
+  // public interface must be prevented though - sensible user options should
+  // be coerced to this object.
+  outConfig.http = {};
+
+  // Log information about the HTTP proxy settings
+  const proxySettings = getProxySettings(process.env);
+  if (proxySettings.length) {
+    logger.warn(
+      `HTTP(S) proxy specified by environment variables: ${proxySettings.join(', ')}. `
+      + 'Please read documentation on how Dredd works with proxies: '
+      + 'https://dredd.org/en/latest/how-it-works/#using-https-proxy'
+    );
+  }
+
   return outConfig;
 }
 

lib/getProxySettings.js

@@ -0,0 +1,23 @@
+const PROXY_ENV_VARIABLES = ['HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY'];
+
+
+/**
+ * Expects an environment variables object (typically process.env)
+ * and returns an array of strings representing HTTP proxy settings,
+ * such as ['HTTPS_PROXY=https://proxy.example.com:8080', ...]
+ *
+ * Supports both upper and lower case names and skips env vars set as empty
+ * strings (other falsy values are not taken care of as env vars can only
+ * be strings).
+ *
+ * Note: The settings are later only printed to the user. Applying the settings
+ * is handled directly by the 'request' library, see
+ * https://github.com/request/request#user-content-proxies
+ */
+module.exports = function getProxySettings(env) {
+  // can't use Object.entries() yet (Node 6 support)
+  return Object.keys(env).map(name => [name, env[name]])
+    .filter(entry => PROXY_ENV_VARIABLES.includes(entry[0].toUpperCase()))
+    .filter(entry => entry[1] !== '')
+    .map(entry => `${entry[0]}=${entry[1]}`);
+};

test/unit/CLI-test.js

@@ -284,77 +284,6 @@ describe('CLI class', () => {
     });
   });
 
-
-  describe('when configuration was saved', () => {
-    before((done) => {
-      sinon.spy(DreddStub.prototype, 'init');
-      sinon.stub(DreddStub.prototype, 'run').callsFake((cb) => {
-        const stats = {
-          tests: 0,
-          failures: 0,
-          errors: 0,
-          passes: 0,
-          skipped: 0,
-          start: 0,
-          end: 0,
-          duration: 0,
-        };
-        cb(null, stats);
-      });
-
-      initStub.callsFake((config, cb) => cb());
-
-      sinon.stub(fsStub, 'existsSync').callsFake(() => true);
-
-      sinon.stub(configUtilsStub, 'load').callsFake(() => ({
-        _: ['blueprint', 'endpoint'],
-        'dry-run': true,
-        hookfiles: null,
-        save: null,
-        load: null,
-        server: null,
-        init: false,
-        custom: [],
-        names: false,
-        only: [],
-        reporter: [],
-        output: [],
-        header: [],
-        sorted: false,
-        user: null,
-        'inline-errors': false,
-        details: false,
-        method: [],
-        color: true,
-        loglevel: 'warning',
-        path: [],
-        $0: 'node ./bin/dredd',
-      }));
-
-      execCommand({ argv: ['--names'] }, done);
-    });
-
-    after(() => {
-      DreddStub.prototype.run.restore();
-      DreddStub.prototype.init.restore();
-      configUtilsStub.load.restore();
-      fsStub.existsSync.restore();
-    });
-
-    describe('and I pass another CLI argument', () => {
-      it('should want to exit with status 0', () => assert.equal(exitStatus, 0));
-
-      it('should call dredd run', () => assert.isTrue(DreddStub.prototype.run.called));
-
-      it('should override existing configuration', () => {
-        assert.isTrue(DreddStub.prototype.init.called);
-        const call = DreddStub.prototype.init.getCall(0);
-        const passedConf = call.args[0];
-        assert.propertyVal(passedConf.options, 'names', true);
-      });
-    });
-  });
-
   describe('when using --server', () => {
     before((done) => {
       sinon.stub(crossSpawnStub, 'spawn').callsFake();

test/unit/Dredd-test.js

@@ -696,94 +696,4 @@ GET /url
       }));
     });
   });
-
-  describe('#logProxySettings', () => {
-    const docsLinkSuffix = 'Please read documentation on how Dredd works with '
-      + 'proxies: https://dredd.org/en/latest/how-it-works/#using-https-proxy';
-    let debugLogger;
-
-    beforeEach(() => { debugLogger = sinon.spy(loggerStub, 'debug'); });
-    afterEach(() => loggerStub.debug.restore());
-
-    describe('when the proxy is set by lowercase environment variable', () => {
-      beforeEach((done) => {
-        process.env.http_proxy = 'http://proxy.example.com';
-        dredd = new Dredd({ options: {} });
-        sinon.stub(dredd.runner, 'run').callsArg(1);
-        dredd.run(done);
-      });
-      afterEach(() => delete process.env.http_proxy);
-
-      it('logs about the setting', () => {
-        assert.isTrue(debugLogger.calledWith(
-          'HTTP(S) proxy specified by environment variables: '
-          + 'http_proxy=http://proxy.example.com.'
-          + ` ${docsLinkSuffix}`
-        ));
-      });
-    });
-
-    describe('when the proxy is set by uppercase environment variable', () => {
-      beforeEach((done) => {
-        process.env.HTTPS_PROXY = 'http://proxy.example.com';
-        dredd = new Dredd({ options: {} });
-        sinon.stub(dredd.runner, 'run').callsArg(1);
-        dredd.run(done);
-      });
-      afterEach(() => delete process.env.HTTPS_PROXY);
-
-      it('logs about the setting', () => {
-        assert.isTrue(debugLogger.calledWith(
-          'HTTP(S) proxy specified by environment variables: '
-          + 'HTTPS_PROXY=http://proxy.example.com.'
-          + ` ${docsLinkSuffix}`
-        ));
-      });
-    });
-
-    describe('when NO_PROXY environment variable is set', () => {
-      beforeEach((done) => {
-        process.env.HTTPS_PROXY = 'http://proxy.example.com';
-        process.env.NO_PROXY = 'whitelisted.example.com';
-        dredd = new Dredd({ options: {} });
-        sinon.stub(dredd.runner, 'run').callsArg(1);
-        dredd.run(done);
-      });
-      afterEach(() => {
-        delete process.env.HTTPS_PROXY;
-        delete process.env.NO_PROXY;
-      });
-
-      it('logs about the setting', () => {
-        assert.isTrue(debugLogger.calledWith(
-          'HTTP(S) proxy specified by environment variables: '
-          + 'HTTPS_PROXY=http://proxy.example.com, '
-          + 'NO_PROXY=whitelisted.example.com.'
-          + ` ${docsLinkSuffix}`
-        ));
-      });
-    });
-
-    describe('when DUMMY_PROXY environment variable is set', () => {
-      beforeEach((done) => {
-        process.env.DUMMY_PROXY = 'http://proxy.example.com';
-        process.env.NO_PROXY = 'whitelisted.example.com';
-        dredd = new Dredd({ options: {} });
-        sinon.stub(dredd.runner, 'run').callsArg(1);
-        dredd.run(done);
-      });
-      afterEach(() => {
-        delete process.env.DUMMY_PROXY;
-        delete process.env.NO_PROXY;
-      });
-
-      it('is ignored', () => {
-        assert.isTrue(debugLogger.calledWith(
-          'HTTP(S) proxy specified by environment variables: '
-          + 'NO_PROXY=whitelisted.example.com.'
-          + ` ${docsLinkSuffix}`
-        ));
-      });
-    });
-  });
 });

test/unit/getProxySettings-test.js

@@ -0,0 +1,59 @@
+const { assert } = require('chai');
+
+const getProxySettings = require('../../lib/getProxySettings');
+
+
+describe('getProxySettings()', () => {
+  it('detects HTTP_PROXY', () => {
+    assert.deepEqual(getProxySettings({
+      SHELL: '/bin/bash',
+      USER: 'honza',
+      HTTP_PROXY: 'http://proxy.example.com:8080',
+    }), [
+      'HTTP_PROXY=http://proxy.example.com:8080',
+    ]);
+  });
+
+  it('detects HTTPS_PROXY', () => {
+    assert.deepEqual(getProxySettings({
+      SHELL: '/bin/bash',
+      USER: 'honza',
+      HTTPS_PROXY: 'https://proxy.example.com:8080',
+    }), [
+      'HTTPS_PROXY=https://proxy.example.com:8080',
+    ]);
+  });
+
+  it('detects NO_PROXY', () => {
+    assert.deepEqual(getProxySettings({
+      SHELL: '/bin/bash',
+      USER: 'honza',
+      NO_PROXY: '*',
+    }), [
+      'NO_PROXY=*',
+    ]);
+  });
+
+  it('detects both lower and upper case', () => {
+    assert.deepEqual(getProxySettings({
+      SHELL: '/bin/bash',
+      USER: 'honza',
+      http_proxy: 'http://proxy.example.com:8080',
+      NO_PROXY: '*',
+    }), [
+      'http_proxy=http://proxy.example.com:8080',
+      'NO_PROXY=*',
+    ]);
+  });
+
+  it('skips environment variables set to empty strings', () => {
+    assert.deepEqual(getProxySettings({
+      SHELL: '/bin/bash',
+      USER: 'honza',
+      http_proxy: 'http://proxy.example.com:8080',
+      NO_PROXY: '',
+    }), [
+      'http_proxy=http://proxy.example.com:8080',
+    ]);
+  });
+});