Websocket connection should honor 'https_proxy' environment variable

[liuweichu]

• [Y] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [Y] I've read and agree to the Code of Conduct.
• [Y] I've searched for any related issues and avoided creating a duplicate issue.

Description

When we setup our hubot-slack behind a firewall. When starting the hubot, we set https_proxy environment variable. When starting up, hubot prints Logged in as BOT of TEAMNAME, then it silently did nothing. This is confusing.

After investigation, we found that we found that slack is able to get authenticated because it uses https_proxy. But after that it cannot connect to websocket because it ignored the https_proxy settings.

In the code, we found that although there is proxy support in ws.js, but client.js simply doesn't pass proxy opts. We passed the process.env.https_proxy to the wsTransport function, then hubot works correctly.

It will be good if either client.js or ws.js could check env.https_proxy and use it correctly during websocket creation.

Also, during investigation we found that there is a typo in Line 21 that wsTransportOpts.proxyUrl should be wsTransportOpts.proxyURL (upper case URL)

Reproducible in:

{project_name} version:
OS version(s): Ubuntu 14.04
Device(s):

Steps to reproduce:

1. generate a slack hubot yo hubot --adapter slack
2. start it on a machine behind firewall, that can only connect to a squid proxy. Set https_proxy variable

Expected result:

hubot logged in

Actual result:

hubot cannot login after printing: Logged in as BOT of TEAMNAME

Attachments:

e.g. Logs, screenshots, screencast, sample project, funny gif, etc.

[DEGoodmanWilson]

Howdy! Thanks for filing this report. Could I ask a favor? I don't have a suitable firewall available for testing—since you seem to have figured out where the problem lies, could I convince you to publish your fix in a PR? This would be a lot easier than me guessing at the right fix, and passing it to you for verification and testing.

[liuweichu]

@DEGoodmanWilson Hi, here is my patch. Feel free to use it 😺
If you insist however I can make a PR. (Just a little bit annoying to sign CLA)

node-slack-sdk (master) $ git diff
diff --git a/lib/clients/transports/ws.js b/lib/clients/transports/ws.js
index a62a622..f50f657 100644
--- a/lib/clients/transports/ws.js
+++ b/lib/clients/transports/ws.js
@@ -16,9 +16,11 @@ var WebSocket = require('ws');
 var wsTransport = function wsTransport(socketUrl, opts) {
   var wsTransportOpts = opts || {};
   var wsOpts = {};
+  var proxyURL = wsTransportOpts.proxyURL || process.env.https_proxy;

-  if (wsTransportOpts.proxyURL) {
-    wsOpts.agent = new HttpsProxyAgent(wsTransportOpts.proxyUrl);
+  if (proxyURL) {
+    console.log("Using https proxy: " + proxyURL)
+    wsOpts.agent = new HttpsProxyAgent(proxyURL);
   }

   return new WebSocket(socketUrl, wsOpts);

[DEGoodmanWilson]

I am very sorry that it is annoying, but it is for your protection as much as ours. Unfortunately, I cannot accept a patch without a signed CLA either 😭

[liuweichu]

PR created. Please kindly review :-D

[liuweichu]

Hi, I resubmitted the CLA again.
It's been a while so I forgot why CLA was not successfully submitted.