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

Prevent subdomains of domains in NO_PROXY from getting proxied #1880

Closed
wants to merge 1 commit into from
Closed

Prevent subdomains of domains in NO_PROXY from getting proxied #1880

wants to merge 1 commit into from

Conversation

tshen8
Copy link

@tshen8 tshen8 commented Nov 15, 2018

Currently, the behaviour for parsing the no_proxy env variable is to check that that hostname ends with the no_proxy listed element, and that the element contains the same number of .s as the hostname.

This is kind of cumbersome because it forces you to list every possible subdomain of a domain you want not to go through a proxy. For example, if you want any requests to *.foo.com not to go through a proxy, you would have to have something like this:
set NO_PROXY=bar.foo.com, baz.qux.foo.com, quuz.foo.com

I think it would make more sense for
set NO_PROXY=.foo.com
to also cover all subdomains of foo.com as well.

Let me know what you guys think!

P.S I found this impossible to work this into the existing tests as they spawn servers on localhost, and I can't create subdomains on localhost without changing /etc/hosts. Open to suggestions of this

@tshen8
Copy link
Author

tshen8 commented Jan 25, 2019

Hi guys, can I get some eyes on this?

Copy link

@mapleeit mapleeit left a comment

Choose a reason for hiding this comment

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

Encountered this problem, too.

LGTM.

@nickuraltsev @emilyemorehouse anybody could take a look at this one?

Copy link

@rjoaopereira rjoaopereira left a comment

Choose a reason for hiding this comment

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

I would like to ask the original author (@chancedickson ) for the rationale behind this comparison.
This may be related to ndots.

@rjoaopereira
Copy link

As there is no answer, can this be merged @nickuraltsev @emilyemorehouse?

@rjoaopereira
Copy link

In order to use this library in an enterprise environment, we would need this particular issue to be fixed. It seems minor but it ends up requiring infrastructure architectural remediations. The code review and subsequent merge would be greatly appreciated!

Thank you for your awesome work :)

@OliverCole
Copy link

FWIW, this is the same logic used in request: https://github.com/request/request/blob/master/lib/getProxyFromURI.js#L29, and other apps like curl.
Can we please merge ASAP?

@lucasterra
Copy link

lucasterra commented Sep 27, 2019

Can we get this merged? We need this too.

@felipewmartins felipewmartins mentioned this pull request Sep 28, 2019
@felipewmartins
Copy link
Collaborator

Merged at #2442

@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants