Merge pull request #720 from aoberoi/proxy-testing

Disables automatic proxy detection

Author: aoberoi

package.json

@@ -35,7 +35,7 @@
     "lint": "tslint --project .",
     "test": "npm run build && npm run test:spec && npm run test:integration",
     "test:spec": "nyc mocha --opts src/mocha.opts src/*.spec.js src/**/*.spec.js",
-    "test:integration": "mocha --opts test/mocha.opts test/typescript/test.ts",
+    "test:integration": "mocha --opts test/mocha.opts test/typescript/test.ts test/proxy/test.js",
     "coverage": "codecov",
     "build": "npm run build:clean && tsc",
     "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
@@ -72,13 +72,16 @@
     "busboy": "^0.2.14",
     "chai": "^4.1.2",
     "codecov": "^3.0.0",
+    "http-shutdown": "^1.2.0",
+    "https-proxy-agent": "^2.2.1",
     "husky": "^0.14.3",
     "jsdoc-to-markdown": "^4.0.1",
     "lint-staged": "^6.1.0",
     "mocha": "^5.0.0",
     "nock": "^9.1.6",
     "nyc": "^13.3.0",
     "p-is-promise": "^1.1.0",
+    "proxy": "^0.2.4",
     "shx": "^0.2.2",
     "sinon": "^4.2.2",
     "source-map-support": "^0.5.3",

src/WebClient.ts

@@ -177,6 +177,11 @@ export class WebClient extends EventEmitter {
       transformRequest: [this.serializeApiCallOptions.bind(this)],
       validateStatus: () => true, // all HTTP status codes should result in a resolved promise (as opposed to only 2xx)
       maxRedirects: 0,
+      // disabling axios' automatic proxy support:
+      // axios would read from envvars to configure a proxy automatically, but it doesn't support TLS destinations.
+      // for compatibility with https://api.slack.com, and for a larger set of possible proxies (SOCKS or other
+      // protocols), users of this package should use the `agent` option to configure a proxy.
+      proxy: false,
     });
     // serializeApiCallOptions will always determine the appropriate content-type
     delete this.axios.defaults.headers.post['Content-Type'];
@@ -709,7 +714,7 @@ export class WebClient extends EventEmitter {
 
         return response;
       } catch (error) {
-        this.logger.debug('http request failed');
+        this.logger.debug('http request failed', error.message);
         if (error.request) {
           throw requestErrorWithOriginal(error);
         }

test/proxy/test.js

@@ -0,0 +1,66 @@
+require('mocha');
+const { createServer } = require('http');
+const shutdown = require('http-shutdown');
+const setup = require('proxy');
+const HttpsProxyAgent = require('https-proxy-agent');
+const { assert } = require('chai');
+const sinon = require('sinon');
+const { WebClient, retryPolicies } = require('../../dist');
+
+const proxyPort = 31280;
+
+describe('with a proxy server listening', function () {
+  beforeEach(function (done) {
+    this.proxyUrl = `http://localhost:${proxyPort}`;
+    this.server = shutdown(setup(createServer()));
+    this.server.listen(proxyPort, done);
+  });
+
+  it('should connect to a host through the proxy when a proxy agent is configured', function () {
+    const agent = new HttpsProxyAgent(this.proxyUrl);
+    const client = new WebClient(undefined, { agent, retryConfig: retryPolicies.rapidRetryPolicy });
+
+    // Hooking into authenticate just to observe when an inbound request is seen at the proxy
+    const spy = sinon.spy();
+    this.server.authenticate = (req, fn) => {
+      spy();
+      fn(null, true);
+    };
+
+    return client.apiCall('method')
+      .catch(() => {
+        assert(spy.called);
+      });
+  });
+
+  it('should NOT connect to a proxy when an environment variable is used', function () {
+    // manipulate the environment
+    process.env.http_proxy = this.proxyUrl;
+    after(function () {
+      delete process.env.http_proxy;
+    });
+
+    // NOTE: we are intentionally using a non-TLS URL here to make this test effective. otherwise we'd still see the
+    // effect that the proxy was not called, and that would be a false positive.
+    const client = new WebClient(undefined, {
+      slackApiUrl: 'http://example.com/',
+      retryConfig: retryPolicies.rapidRetryPolicy
+    });
+
+    // Hooking into authenticate just to observe when an inbound request is seen at the proxy
+    const spy = sinon.spy();
+    this.server.authenticate = (req, fn) => {
+      spy();
+      fn(null, true);
+    };
+
+    return client.apiCall('method')
+      .catch(() => {
+        assert(spy.notCalled);
+      });
+  });
+
+  afterEach(function (done) {
+    this.server.shutdown(done);
+  });
+});

test/typescript/test.ts

@@ -1,5 +1,5 @@
 import 'mocha';
-import { check, checkDirectory } from 'typings-tester';
+import { check } from 'typings-tester';
 
 describe('typescript typings tests', () => {
   it('should allow WebClient to work without a token', () => {