upgrade proxy-agent to v6.3.0 to resolve vm2 vulnerability

[robbkidd]

This is the proxy-agent dependency update made by @mohitpubnub in #47 and follow-on work to update superagent-proxy's use of the new major version.

Resolves #48 (hopefully)

[robbkidd]

Whoop! With #49 merged, the CI lights up on this one! \o/

[robbkidd]

8 passing (844ms)
2 failing

Well, that's progress at least.

[robbkidd]

I suspect this is the wrong change to make here—to disregard the uri passed in to proxy()—but I am still unfamiliar with how proxy-agent works.

[robbkidd]

After looking over v6 of proxy-agent, it appears that ProxyAgent is now focused on returning an agent solely based on the conventional environment variables and does not provide anymore an API for returning an agent configured by passing in a URL string or connection object through code.

Is that correct?

If so, I reckon superagent-proxy's proxy(url) method needs its own implementation of an agent factory to instantiate one based on the proxy URL passed in.

[TooTallNate]

That is correct. The reason that an explicit URL was removed was because the intent of ProxyAgent is that it'll be used in CLI apps, and thus the end user should be in control over what proxy to connect to. Thus, specifying a URL within the code no longer made sense to me.

I suggest that superagent-proxy should also drop the parameter in .proxy(), and simply apply the ProxyAgent instance. Typing this out, I question whether this module even needs to exist anymore, since it's essentially would just boil down to:

const agent = new ProxyAgent();

superagent.agent(agent).…

[robbkidd]

I suggest that superagent-proxy should also drop the parameter in .proxy()

This removal would hurt a little for our use of superagent-proxy. We use .proxy(a_proxy_url) to configure an agent when our library's users have opted to set a proxy in code rather than with environment variables.† I am totally open to learning how to do that another way, in service to removing obsolete code.

---

† I know, right? I prefer env vars, too, but sometimes devs are gonna insist on deving.

[DmitriyKirakosyan]

Hi folks, the ProxyAgent ctor has an argument of type ProxyAgentOptions, which includes getProxyForUrl property. This is how we migrated to the new version of proxy-agent in appcenter-cli: https://github.com/microsoft/appcenter-cli/pull/2387/files#diff-dcbeaf7180359f73aa2d9027ca6bcbcac4c9f02ab01152a0f663ea5725aaeff1R17

[robbkidd]

What do you think about this?

Suggested change

	var agent = proxyAgent;
	var agent = uri ? new ProxyAgent({ getProxyForUrl: () => uri }) : proxyAgent;

So that superagent can get an agent configured with environment variables:

superagent.post(targelUrl).proxy()

… or with a proxy connection given as a parameter:

superagent.post(targetUrl).proxy(proxyUrl)

[DmitriyKirakosyan]

LGTM.
@robbkidd do you think this PR can be merged soon? Asking because we are using this lib in code-push repo.

[DmitriyKirakosyan]

Also, if you are applying the changes, I would correct it a bit:

var agent = uri ? new ProxyAgent({ getProxyForUrl: () => uri }) : new ProxyAgent();

And remove const proxyAgent = new ProxyAgent(); in the beginning, to avoid unnecessary instantiation.

[robbkidd]

@robbkidd do you think this PR can be merged soon?

I'm new here, not a maintainer, so my thoughts are not very influential on merging.

var agent = uri ? new ProxyAgent({ getProxyForUrl: () => uri }) : new ProxyAgent();

And remove const proxyAgent = new ProxyAgent(); in the beginning, to avoid unnecessary instantiation.

I believe that would replace a single, maybe-unnecessary instantiation of ProxyAgent with 𝒩 instantiations of a ProxyAgent all with the same configuration where 𝒩 is the number of times a proxied superagent request is made.

... which I realize now is also what would happen when calling .proxy(uri) with the same uri over and over. 🤔

[robbkidd]

... my thoughts are not very influential on merging.
— me

At the moment, as a user of this library, I'm inclined towards dropping the dependency on superagent-proxy in the code I maintain and to take a dependency on proxy-agent directly. We can instantiate it appropriately based on the configuration given to our code. Pretty much as @TooTallNate described as a possibility earlier.

[answerquest]

Hi, our Fossa tool is flagging this vm2 dependency. This doesn't seem to be a problem with the latest versions of superagent-proxy's dependencies though. Can we get this fix in soon?

[robbkidd]

@answerquest I recommend—not as a maintainer, but only as another (former) user of this package—that you replace your dependency on superagent-proxy in your software with a more direct dependency on proxy-agent. You will likely resolve your security audit finding faster that way.

The major version bump in proxy-agent changed its its design enough that superagent-proxy's use of it has been made mostly obsolete. Use proxy-agent directly to return a configured agent and pass that agent into a superagent client.