Loading the latest version of box-node-sdk changes the Node HTTPS module in surprising and breaking ways

[Avaq]

• I have checked that the SDK documentation doesn't solve my issue.
• I have checked that the API documentation doesn't solve my issue.
• I have searched the Box Developer Forums and my issue isn't already reported (or if it has been reported, I have attached a link to it, for reference).
• I have searched Issues in this repo and my issue isn't already reported.

Description of the Issue

I updated my box-node-sdk version from 1.33.0 to 1.35.0 and my codebase started exhibiting some strange behaviour when it comes to sending HTTP requests. After looking into it, I managed to isolate the issue to just require ("box-node-sdk"). The problem is that requiring this version of Box Node SDK causes the Node built-in http module to be monkey-patched in breaking ways. Based on the stack trace, I suspect #529 to be the culprit. See below.

Steps to Reproduce

1. Create an empty file called body.txt: touch body.txt ;)
2. Run the Node repl: node
3. Attempt to execute the following lines:

   var req = https.request ('https://example.com', {}, res => console.log (res.statusMessage));
   void stream.pipeline (fs.createReadStream('body.txt'), req, e => e && console.error (e.message));

4. Observe that this logs "OK" after the requests completes. Everything works as intended.
5. Now run the following lines:

   void require ('box-node-sdk');
   var req = https.request ('https://example.com', {}, res => console.log (res.statusMessage));
   void stream.pipeline (fs.createReadStream('body.txt'), req, e => e && console.error (e.message));

Expected Behavior

The code is not affected by require ('box-node-sdk'), and proceeds to log "OK" again.

Error Message, Including Stack Trace

Thrown:
TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type Function. Received type object
    at checkListener (events.js:77:11)
    at ClientRequest.once (events.js:326:3)
    at new ClientRequest (_http_client.js:186:10)
    at Object.request (https.js:309:10)
    at Object.<anonymous> (./node_modules/agent-base/patch-core.js:25:22)
    at Object.request (./node_modules/socks-proxy-agent/node_modules/agent-base/patch-core.js:23:20)
    at repl:1:17
    at Script.runInThisContext (vm.js:116:20)
    at REPLServer.defaultEval (repl.js:405:29)
    at bound (domain.js:419:14)
    at REPLServer.runBound [as eval] (domain.js:432:12)
    at REPLServer.onLine (repl.js:716:10)
    at REPLServer.emit (events.js:228:7)
    at REPLServer.EventEmitter.emit (domain.js:475:20)
    at REPLServer.Interface._onLine (readline.js:315:10)
    at REPLServer.Interface._line (readline.js:692:8) {
  code: 'ERR_INVALID_ARG_TYPE'
}

I've also observed another error where the https interface was used in a different way which did not produce this type error, but instead caused all my requests to go to 127.0.0.1, even if the URL contained a different domain:

Error: connect ECONNREFUSED 127.0.0.1:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1134:16) {
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 443
}

Versions Used

Node SDK: 1.35.0

[Avaq]

I tracked down the problem to the node_modules/agent-base/patch-core.js file, which contains the following (obviously buggy) code:

/**
 * This currently needs to be applied to all Node.js versions
 * in order to determine if the `req` is an HTTP or HTTPS request.
 *
 * There is currently no PR attempting to move this property upstream.
 */
const patchMarker = "__agent_base_https_request_patched__";
if (!https.request[patchMarker]) {
  https.request = (function(request) {
    return function(_options, cb) {
      let options;
      if (typeof _options === 'string') {
        options = url.parse(_options);
      } else {
        options = Object.assign({}, _options);
      }
      if (null == options.port) {
        options.port = 443;
      }
      options.secureEndpoint = true;
      return request.call(https, options, cb);
    };
  })(https.request);
  https.request[patchMarker] = true;
}

• As I suspected, the https.request function is being replaced with another one as soon as this module loads.
• The function it has been replaced with has several incompatibilities with the original https.request function.
• When a URL is passed as a first argument, and options as second (which is allowed on the built-in function) this new function replaces all options with the parsed URL, causing the connections to 127.0.0.1 I've been seeing.

[Avaq]

The problem in agent-base was fixed in TooTallNate/node-agent-base#36 and released in version 5.0.0, a year ago: https://github.com/TooTallNate/node-agent-base/releases/tag/5.0.0.

The actual problem thus becomes that the dependencies of box-node-sdk are outdated.

[Avaq]

Updating proxy-agent to version 4.0.0 should fix the issue: https://github.com/TooTallNate/node-proxy-agent/releases/tag/4.0.0

[Avaq]

#563 addresses this issue. Will a new version be released?

[Avaq]

The fix for this issue has been included in release v1.36.0.