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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fcf7f1
upgrade proxy-agent to v6.3.0 to resolve vm2 vulnerability
mohitpubnub Aug 2, 2023
bd604ab
update the proxy-agent import
robbkidd Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* Module dependencies.
*/

var proxyAgent = require('proxy-agent');
const { ProxyAgent } = require('proxy-agent');
const proxyAgent = new ProxyAgent();
var debug = require('debug')('superagent-proxy');

/**
Expand Down Expand Up @@ -68,7 +69,7 @@ function proxy (uri) {
setupUrl(this);

// attempt to get a proxying `http.Agent` instance
var agent = proxyAgent(uri);
var agent = proxyAgent;
Copy link
Contributor Author

@robbkidd robbkidd Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@DmitriyKirakosyan DmitriyKirakosyan Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 🤔

Copy link
Contributor Author

@robbkidd robbkidd Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.


// if we have an `http.Agent` instance then call the .agent() function
if (agent) this.agent(agent);
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
},
"dependencies": {
"debug": "^4.3.2",
"proxy-agent": "^5.0.0"
"proxy-agent": "^6.3.0"
},
"peerDependencies": {
"superagent": ">= 0.15.4 || 1 || 2 || 3"
Expand Down
Loading