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

Enforce that all HTTP requests always use the global agent #13

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@pimterry
Copy link

commented Jun 26, 2019

Some libraries (e.g. Stripe) provide their own default agent, which has to be explicitly overridden.

To do this nicely, I'd like to override that agent with the exact same agent that global-agent is using, but that's not always available. If you're using Node >= v11.7.0 it's exposed as {http,https}.globalAgent, but in older Node versions it only exists as part of the binding of the http/https methods.

This PR exposes the agents as part of the global configuration, so that they can be retrieved as global.GLOBAL_AGENT.HTTP_PROXY_AGENT and global.GLOBAL_AGENT.HTTPS_PROXY_AGENT.

@pimterry

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

There's actually one alternative to this I've just spotted: global-agent could always set {http,https}.globalAgent to the corresponding agent, rather than only doing it for node 11.7+.

It won't have an effect in older versions, so you'd need the method binding workaround as well, but it would make the behaviour a little more consistent across versions, and it is a clearer way to get at the agent. What do you think? Happy to go either way.

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

I read this multiple times and I still don't understand how would this be used in practice.

Can you provide a code example of how would you use this if agent would be available through {http,https}.globalAgent?

Regarding overriding it in older versions, I could be wrong, but I think the property is not configurable (although this can be hacked around if needed).

@pimterry

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

I read this multiple times and I still don't understand how would this be used in practice.

When using the Stripe SDK, global-agent doesn't work at all, because Stripe provides its own default agent. With this PR you easily configure it to use the same agent, like so:

const stripe = require('stripe')('api_key');

// With this PR:
stripe.setHttpAgent(global.GLOBAL_AGENT.HTTPS_PROXY_AGENT);
// Alternative approach suggested above:
stripe.setHttpAgent(require('https').globalAgent);

// All stripe calls from here on are proxied correctly

Regarding overriding it in older versions, I could be wrong, but I think the property is not configurable

I'm pretty sure it is. I've just tested with require('http').globalAgent = false, and it's successful & persisted in at least in node 4, 6, 8, 10 & 12.

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

When using the Stripe SDK, global-agent doesn't work at all, because Stripe provides its own default agent. With this PR you easily configure it to use the same agent, like so:

I see. In case of Stripe, you are lucky that you are able to override it. This approach wouldn't add much value if such API did not exist, which is often the case (e.g. all the packages that depend on request NPM module).

I think we could do something like:

const originalRequest = http.request.bind(http);

http.request = (...args) => {
  return originalRequest({
    ...args,
    agent: httpAgent
  });
};

to force all HTTP requests constructed using http(s) module to use global-agent.

@pimterry

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

True, that should forcibly ensure that 100% of requests use our agent.

I worry a little bit that this might cause other unexpected side effects, if packages are depending on the specific agents they're configuring. It doesn't look like that's very common though - I had a quick skim of the dependencies of agent-base (base class for http agents, with 6mil+ downloads a week) for example, and every single major usage of it on npm is for some kind of proxy, which should always be safe to override I think.

Most of the logic for this is already in place, we'd just need to remove the if (!options.agent) test in bindHttpMethod, and you would get exactly this behaviour. Worth doing do you think?

In that case, that would make this PR defunct, since there's never a need to get the agent if it's impossible to ever manually use an agent.

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

If you can give this method a try, I would be happy to merge a working solution. Otherwise, I will give this myself a try in the next couple of weeks. This primarily requires some sanity testing, e.g. making sure that it works with request/ Axios packages.

@pimterry pimterry force-pushed the httptoolkit:expose-agent branch from 546f652 to 74a7f71 Jun 27, 2019

@pimterry pimterry changed the title Expose the http and https proxy agents globally for explicit use Enforce that all HTTP requests always use the global agent Jun 27, 2019

@pimterry

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

@gajus I've made the change and updated this PR to include it.

It's slightly more complicated than expected, to handle the case where you call createGlobalProxyAgent repeatedly. Before, createGlobalProxyAgent was wrapping an extra layer around the http.* methods every time it was called, which only worked because when outermost layer injected the agent every other layer didn't change the agent it, since an agent was already provided. Now that we always change the agent this causes problems, and the tests were failing. I've fixed it by capturing the initial value of the http.* methods up front, and always binding those raw methods, so there's only ever one wrapper around them.

Not sure if that's clear, hopefully you can see what I mean from the code.

I've tested this with both Axios & Request, in Node 12.2.0 and 10.15.3, it all seems to work nicely for me.

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

Tested in Node 12 and it does not work.

import request from 'request';
import {
  createGlobalProxyAgent
} from 'global-agent';

const main = async () => {
  createGlobalProxyAgent({
    environmentVariableNamespace: ''
  });

  try {
    await request('http://gajus.com');
  } catch (error) {
    console.log('OK');
  }
};

main();
$ HTTP_PROXY=http://127.0.0.1:8068 babel-node ./test.js

The proxy server is throwing error:

Bad HTTP request line: b'GET http://127.0.0.1:8068http://gajus.com/\nHTTP/1.1

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

Looks like it is missing a check for if path is already an absolute URL.

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

9a8e0d8

This change makes global-agent work with request and axios (tested in v10 and v12).

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

Regarding Stripe, your proposed patch will work in Node.js v10, but not in v12.

@gajus gajus closed this in 64e5efe Jun 29, 2019

@gajus

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Jun 29, 2019

@pimterry

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

Tested in Node 12 and it does not work.

Looks like it is missing a check for if path is already an absolute URL.

Yes, I think there's been some confusion here. This PR was just intended to enforce the agent on all requests to fix libraries like Stripe, and just not break request & axios along the way, not to fix their HTTP_PROXY behaviour. You're talking about a separate issue we were discussing in #12.

It looks like you've taken this change though and also fixed the HTTP_PROXY issue on master too. Thanks for that! I'll close #12 now. Your changes look good to me, and seems to all work well for my case, nice work 👍

One thing that's worth noting is that now that HTTP_PROXY works with request & axios etc, the reasoning in the README for using GLOBAL_AGENT_HTTP_PROXY instead of HTTP_PROXY no longer makes sense. There's now no downside to simplifying this and using HTTP_PROXY directly. Your call of course, and it's not strictly necessary but it'd be a nice UX improvement.

@pimterry

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

Regarding Stripe, your proposed patch will work in Node.js v10, but not in v12.

Ah, just spotted this - that is still an issue, although one that's easy to work around for now (by passing https.globalAgent directly to Stripe/whoever, which works fine in 11.7+).

Fixing this as well would be good though to avoid that, and make this library behave the same way for all node versions. It's not too bad, it just requires rebinding the http methods for all node versions, instead of only < 11.7.

What do you think? Just setting globalAgent is simpler of course, but imo consistent behaviour is pretty valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.