Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is this still maintained? #2

Closed
emirotin opened this issue Apr 24, 2017 · 13 comments
Closed

Is this still maintained? #2

emirotin opened this issue Apr 24, 2017 · 13 comments

Comments

@emirotin
Copy link
Contributor

Hey, is this module still maintained?

All the tests in Travis are failing for node newer than 0.10: https://travis-ci.org/np-maintain/global-tunnel

@wzrdtales
Copy link
Member

Yes, maintained, but not actively worked on. This is a hardfork of the orignal salesforce package which itself is completely silent. You do not even get answers from them though.

The tests however do fail since I hard forked, see salesforce/global-tunnel#17 (comment)

I have had no time yet to work through the tests to fix them for newer node versions. However, if I mind right it does work on them though. If you want you can shoot a PR with fixes for the tests, only if you have some spare time though :)

@emirotin
Copy link
Contributor Author

emirotin commented Apr 24, 2017

This is a hardfork of the orignal salesforce package which itself is completely silent.

Yeah I've come here from that repo.

I'm trying to fix the tests indeed as I need to be confident about this module. I've found the implementation change in Node that does probably influence them, but didn't fix it yet. If I succeed will send a PR.

@emirotin
Copy link
Contributor Author

emirotin commented Apr 24, 2017

After digging more I believe the module is actually broken in Node v7 (at least). The node HTTPS agent request proxies to the HTTP request (see Snippet 1 below).

Then the HTTP request is being made with the OuterHttpAgent (no s), but with the default HTTPS agent. Which causes the Snippet 2 to throw


Snippet 1 (https.js from Node):

exports.request = function request(options, cb) {
  if (typeof options === 'string') {
    options = url.parse(options);
    if (!options.hostname) {
      throw new Error('Unable to determine the domain name');
    }
  } else if (options instanceof url.URL) {
    options = urlToOptions(options);
  } else {
    options = util._extend({}, options);
  }
  options._defaultAgent = globalAgent;
  return http.request(options, cb);
};

Snipper 2 (_http_client.js from Node):

  var protocol = options.protocol || defaultAgent.protocol;
  var expectedProtocol = defaultAgent.protocol;
  if (self.agent && self.agent.protocol)
    expectedProtocol = self.agent.protocol;

  var path;
  if (options.path) {
    path = '' + options.path;
    var invalidPath;
    if (path.length <= 39) { // Determined experimentally in V8 5.4
      invalidPath = isInvalidPath(path);
    } else {
      invalidPath = /[\u0000-\u0020]/.test(path);
    }
    if (invalidPath)
      throw new TypeError('Request path contains unescaped characters');
  }

  if (protocol !== expectedProtocol) {
    throw new Error('Protocol "' + protocol + '" not supported. ' +
                    'Expected "' + expectedProtocol + '"');
  }

@wzrdtales
Copy link
Member

Quite possible though, I think I last tested this with node 6 or so. Have been quite a while since I looked into the code of this (luckily I don't need such stuff anymore today (the last time it has been a company that was still stuck with using proxies like you did in the 90s).

I quickly forked the tunnel dependency and added node 6 and 7 to their build list I suspect that this would be need to be fixed over there.

https://travis-ci.org/wzrdtales/node-tunnel/builds/225258426

@wzrdtales
Copy link
Member

lets wait for the tests

@wzrdtales
Copy link
Member

ok this seems to still work though

@emirotin
Copy link
Contributor Author

We have some client who use proxies (we don't ourselves) so we need to add the proxy support to our CLI product.

Soo I've investigated the problem a bit more and have a good idea of what's going on (I think).
Hope it's OK to spam you a bit more?;)

Let's take this case: the proxy is HTTP, the target URL is HTTPS.

  • As the proxy is non-secure the outer protocol is HTTP and OuterHttpAgent is used as the global default agent for both http and https.
  • Then the request is made to the HTTPS URL
  • The patched request method is being called with the bound protocol being https: and the bound module being https and the agent being set to the OuterHttpAgent
  • this calls the original https.request which sets the options._defaultAgent to that very same OuterHttpAgent which has the protocol property set to http: of course
  • and then it calls http.request with options.protocol being https: (as we're making the HTTPS requestand_defaultAgent.protocolbeinghttp:` which in effect causes the exception thrown as shown by the two snippets above.

Have to think what should be done and what actually is the correct behavior here.

@emirotin
Copy link
Contributor Author

Interestingly this exception has been in place for more than 3 years (nodejs/node@6d15b16 is the latest change there and it has only changed the wording)

@wzrdtales
Copy link
Member

Soo I've investigated the problem a bit more and have a good idea of what's going on (I think).
Hope it's OK to spam you a bit more?;)

Sure :)

@wzrdtales
Copy link
Member

wzrdtales commented Apr 24, 2017

so we need to add the proxy support to our CLI product.

If you work with request this works out of the box though, but I guess here lies the problem :) I needed this package to get david-www (modified it to work with gitlab though) working behind a proxy.

@emirotin
Copy link
Contributor Author

We work with fetch / node-fetch which does directly support proxy settings, but we also have some 3rd-party deps that use superagent, and we'd like to have a single drop-in proxy config. So a module like this looks like an ideal solution.

@emirotin
Copy link
Contributor Author

OK I've solved this problem (and another one), so the tests passing are 36 vs 14 failing now. I'm going to open the PR to keep watching its progress on Travis as I (hopefully) fix more tests.

@wzrdtales
Copy link
Member

good job 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants