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

Support NO_PROXY and no_proxy #383

Closed
sgoff0 opened this issue Dec 1, 2021 · 2 comments
Closed

Support NO_PROXY and no_proxy #383

sgoff0 opened this issue Dec 1, 2021 · 2 comments

Comments

@sgoff0
Copy link

sgoff0 commented Dec 1, 2021

While mentioned in a needle Pull Request comment I believe lack of NO_PROXY support should be called out as its own issue.

I noticed this behavior while using Pact, a contract testing library, as it previously used request but replaced it with needle. With the move to needle http requests started failing behind our corporate proxy as needle does not respect the no_proxy environment variable we have set for localhost. Specifically in our case http requests via needle to localhost (something Pact does when running a contract test) were going through our system proxy which ended up returning a 302 to a proxy blocked website.

This issue is resolved if config.proxy were to be set to null prior to the needle.js proxy check when a URI is in the NO_PROXY environment variable

needle/lib/needle.js

Lines 302 to 315 in 4cb2ef6

// if proxy is present, set auth header from either url or proxy_user option.
if (config.proxy) {
if (config.proxy.indexOf('http') === -1)
config.proxy = 'http://' + config.proxy;
if (config.proxy.indexOf('@') !== -1) {
var proxy = (url.parse(config.proxy).auth || '').split(':');
options.proxy_user = proxy[0];
options.proxy_pass = proxy[1];
}
if (options.proxy_user)
config.headers['proxy-authorization'] = auth.basic(options.proxy_user, options.proxy_pass);
}

As an example in my use case manually adding config.proxy = null right before this check makes the network call behave as expected. This obviously is not the right fix just demonstrates that manually setting null here when desired seems to solve the problem. As an FYI the now deprecated request package already has logic to properly handle NO_PROXY and no_proxy, some of this could likely be leveraged here to add support for NO_PROXY

@tomas
Copy link
Owner

tomas commented Dec 1, 2021

Thanks for the heads up. I'll add NO_PROXY to the PR and merge it soon.

@nejch
Copy link

nejch commented Apr 11, 2022

Thanks for the work on that PR @tomas. I think this issue can remain open for now to make sure needle can address the syntax usually used in no_proxy described above. For example:

https://github.com/w3c/libwww/blob/8678b3dcb4191065ca39caea54bb1beba809a617/Library/src/HTAccess.c#L234-L239

Usually no_proxy will be used with domain wildcards, e.g. no_proxy=.company.com,.company.io, which should cover all ports and all subdomains for those 2 top-level domains. There is also some discussion in curl/curl#1208 that might be relevant.

@tomas tomas closed this as completed in c566dc2 Nov 17, 2022
tomas added a commit that referenced this issue Nov 17, 2022
Add utils module and refactor should_proxy_to() based on PR #411. Fixes #383
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

No branches or pull requests

3 participants