chore: update https-proxy-agent #37713
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| if (proxy.username) | ||
| proxyOpts.auth = `${proxy.username}:${proxy.password || ''}`; | ||
|
|
||
| if (forUrl && ['ws:', 'wss:'].includes(forUrl.protocol)) { |
There was a problem hiding this comment.
- Why removing all this code? It looks valuable to me, with all the comments and TODOs.
- It does not look like
url.format()understandsauthproperty, does it?
There was a problem hiding this comment.
- sorry, I short-circuited when I saw that both conditions do the same ^^ reverting
- it does! https://nodejs.org/docs/v22.20.0/api/url.html#urlobjectauth
| (parsedProxyURL as any).secureProxy = parsedProxyURL.protocol === 'https:'; | ||
|
|
||
| options.agent = new HttpsProxyAgent(parsedProxyURL); | ||
| options.agent = new HttpsProxyAgent(url.format(parsedProxyURL)); |
There was a problem hiding this comment.
- I'm not sure
url.format()understandssecureProxyoption. - Shall we switch to
new URL(proxyURL)instead ofparse()+format()?
There was a problem hiding this comment.
- I'm not sure why we had
secureProxyhere, in the first place. It got removed in a recent dependency update, but even before it doesn't make sense how we pass it here. See TooTallNate/proxy-agents@b3860aa. let me remove it. - I'll try that in a separate PR.
There was a problem hiding this comment.
I think the linked commit removes it from the agent, not from the input url. That said, I can't find any usage of secureProxy in the package...
There was a problem hiding this comment.
Which is why I don't understand why we had this in the first place.
There was a problem hiding this comment.
Ahh, this is where it was read! The old version of the library was in a different repository. https://github.com/TooTallNate/node-http-proxy-agent/blob/5368fa899134a9b0d7d49cc6917c1eecabe7aeb0/src/agent.ts#L54
|
|
||
| This project incorporates components from the projects listed below. The original copyright notices and the licenses under which Microsoft received such components are set forth below. Microsoft reserves all rights not expressly granted herein, whether by implication, estoppel or otherwise. | ||
|
|
||
| - [email protected] (https://github.com/TooTallNate/node-agent-base) |
There was a problem hiding this comment.
Let's make sure we update socks proxy agent as well.
There was a problem hiding this comment.
I'll do that in a separate PR.
There was a problem hiding this comment.
Thanks for the review.
Both the change from url.format to WHATWG URL (it throws on irregular inputs, whereas the current one returns null fields), and the socks update have a far higher potential for regression than the HTTPS Proxy update.
I'll work on those in separate PRs that we can revert individually, and keep this one solely about the ALPN issue at hand.
I've added a nifty little regression test to this PR.
| (parsedProxyURL as any).secureProxy = parsedProxyURL.protocol === 'https:'; | ||
|
|
||
| options.agent = new HttpsProxyAgent(parsedProxyURL); | ||
| options.agent = new HttpsProxyAgent(url.format(parsedProxyURL)); |
There was a problem hiding this comment.
- I'm not sure why we had
secureProxyhere, in the first place. It got removed in a recent dependency update, but even before it doesn't make sense how we pass it here. See TooTallNate/proxy-agents@b3860aa. let me remove it. - I'll try that in a separate PR.
| if (proxy.username) | ||
| proxyOpts.auth = `${proxy.username}:${proxy.password || ''}`; | ||
|
|
||
| if (forUrl && ['ws:', 'wss:'].includes(forUrl.protocol)) { |
There was a problem hiding this comment.
- sorry, I short-circuited when I saw that both conditions do the same ^^ reverting
- it does! https://nodejs.org/docs/v22.20.0/api/url.html#urlobjectauth
|
|
||
| This project incorporates components from the projects listed below. The original copyright notices and the licenses under which Microsoft received such components are set forth below. Microsoft reserves all rights not expressly granted herein, whether by implication, estoppel or otherwise. | ||
|
|
||
| - [email protected] (https://github.com/TooTallNate/node-agent-base) |
There was a problem hiding this comment.
I'll do that in a separate PR.
This comment has been minimized.
This comment has been minimized.
| this._serverEncrypted = await proxyAgent.callback(new EventEmitter() as any, { host: rewriteToLocalhostIfNeeded(this.host), port: this.port, secureEndpoint: false }); | ||
| else | ||
| if (proxyAgent) { | ||
| if ('callback' in proxyAgent) |
There was a problem hiding this comment.
Do we have callback vs connect due to different versions of socks vs https? If so, let's not forget to clean things up later.
There was a problem hiding this comment.
yup, it's because of different versions. I don't see us forgetting - any update to SOCKS will require it to also move to connect, at that point we'll be cleaning up.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"6 flaky46922 passed, 816 skipped Merge workflow run. |
2 participants
closes #37676