Skip to content

fix(env-http-proxy-agent): assume http for schema-less proxy env vars#4860

Closed
nthbotast wants to merge 2 commits intonodejs:mainfrom
nthbotast:fix-4736-schemaless-proxy
Closed

fix(env-http-proxy-agent): assume http for schema-less proxy env vars#4860
nthbotast wants to merge 2 commits intonodejs:mainfrom
nthbotast:fix-4736-schemaless-proxy

Conversation

@nthbotast
Copy link
Copy Markdown

Summary

  • assume an http:// scheme when HTTP_PROXY / HTTPS_PROXY values are provided without a URL scheme
  • keep existing behavior unchanged for already well-formed proxy URLs
  • add coverage for schema-less proxy environment variables in EnvHttpProxyAgent tests

Testing

  • node --test test/env-http-proxy-agent.js

Closes #4736

Comment thread lib/dispatcher/env-http-proxy-agent.js Outdated
const HTTPS_PROXY = httpsProxy ?? process.env.https_proxy ?? process.env.HTTPS_PROXY
if (HTTPS_PROXY) {
this[kHttpsProxyAgent] = new ProxyAgent({ ...agentOpts, uri: HTTPS_PROXY })
this[kHttpsProxyAgent] = new ProxyAgent({ ...agentOpts, uri: normalizeProxyURI(HTTPS_PROXY) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the rationale of normalizing https to http?
unless tunneling, that can cause connect rejection from proxy.

@nthbotast

This comment was marked as off-topic.

@metcoder95
Copy link
Copy Markdown
Member

Let's do that.

I can see a bit of ambiguity if I try to set a schema less origin proxy.example that I know only accepts TLS traffic and starts attempting plain text connection.
On top, it seems is what also curl does.

@nthbotast

This comment was marked as off-topic.

@nthbotast

This comment was marked as off-topic.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.14%. Comparing base (f734c87) to head (d023525).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4860   +/-   ##
=======================================
  Coverage   93.13%   93.14%           
=======================================
  Files         109      109           
  Lines       34243    34258   +15     
=======================================
+ Hits        31893    31909   +16     
+ Misses       2350     2349    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Successfully merging this pull request may close these issues.

Schema-less HTTP_PROXY and HTTPS_PROXY env vars should maybe assume http

4 participants