Skip to content

Add noproxy configuration #2873

Closed
wants to merge 1 commit into from
@vvision
vvision commented Oct 12, 2012

When fetching, check if the url requested must use a proxy (if present) or not.
This allows package installation from lan without having to change/disable the proxy parameter each time.

"noproxy" is a string containing hostnames. When fetching a package from one of these hostnames, the specifed proxy won't be used.
So, this add a way to specify a list of hostnames that will not ever go through a proxy.

Note: Most relevant addition can be found in lib/utils/fetch.js.

@vvision vvision referenced this pull request in npm/npmconf Oct 16, 2012
Closed

Add noproxy configuration #6

@isaacs
npm member
isaacs commented Nov 7, 2012

I'm not interested in taking on mocha/chai as a dependency. There's already a test framework that npm uses (tap) and a bunch of packages in test/packages that will be installed and have their test scripts run. This should not be a 50,000 line change.

@vvision
vvision commented Nov 12, 2012

You're right. I think it's much better now.
I've removed all the useless stuff I had added: mocha, chai and express.
Furthermore, I've put my test npm-test-noproxy in test/packages.
Contrary to one of the commit, I'm not using tap anymore.

@VirgileD
VirgileD commented Dec 3, 2012

+1 (and more)

@isaacs isaacs commented on an outdated diff Dec 4, 2012
lib/utils/fetch.js
- , proxy: proxy
- , strictSSL: npm.config.get("strict-ssl")
- , ca: remote.host === regHost ? npm.config.get("ca") : undefined
- , headers: { "user-agent": npm.config.get("user-agent") }}
- var req = request(opts)
- req.on("error", function (er) {
- fstr.emit("error", er)
- })
- req.pipe(fstr)
- return req;
+ , proxy: proxy
+ , strictSSL: npm.config.get("strict-ssl")
+ , ca: remote.host === regHost ? npm.config.get("ca") : undefined
+ , headers: { "user-agent": npm.config.get("user-agent") }}
+
+ var req = request(opts)
@isaacs
npm member
isaacs added a note Dec 4, 2012

Why is this indentation changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs
npm member
isaacs commented Dec 4, 2012

This is good for the npm bit. However there are two other pieces:

  1. the config needs to be added to https://github.com/isaacs/npmconf with a default and type.
  2. in https://github.com/isaacs/npm-registry-client, it needs to respect the value in lib/request.js, so that registry urls will respect NO_PROXY if the registry is on that list.
@feugy
feugy commented Dec 18, 2012

👍

Please include this pull request.
We daily use dependencies that are accessible only within your society network, and have to change npm configuration very frequently (which is a real pain under windows because of its security management).

It will be very handy to have this configuration options !

Thank you

@vvision
vvision commented Dec 18, 2012

Concerning npmconf, I think this pull request now make sense: npm/npmconf#6.

Associated Pull Request;
npm/npmconf#13
npm/npm-registry-client#11

@vvision vvision referenced this pull request in npm/npm-registry-client Dec 18, 2012
Closed

Add noproxy configuration #11

@Filirom1

I am under a proxy at work and I can say that this pull request works like a charm.

Here is what I did to use it:

[~]> git clone https://github.com/vvision/npm
[~/npm]> npm install -g
[~/npm]> npm config -g set noproxy priv.repo
@vvision vvision referenced this pull request in npm/npmconf Mar 19, 2013
Closed

Add noproxy configuration #13

@vvision
vvision commented Mar 19, 2013

I've made a clean new pull request concerning the configuration needed in npmconf.
Is it good for npmconf and npm-registry-client?

@VirgileD

still using the fork, would be easier if integrated in the main npm package.
thx++

@simonlopez

👍

@Filirom1

+2

@pouicr
pouicr commented Jul 5, 2013

+4

It's so annoying to change configuration every time and not be able to easily install something that requires both priv and pub dependencies...

@domenic
npm member
domenic commented Sep 8, 2013

This seems pretty reasonable. Sorry @vvision for us taking so long to get to this.

Do you think it has addressed all the feedback? Can you rebase on master and squash into a single commit (no merge commits)?

@vvision
vvision commented Sep 8, 2013

I think it should now address all the feedback.

I wasn't familiar with rebasing but it seems I've managed to remove merge commits.
I must admit it's more readable with a single commit.

@domenic domenic and 1 other commented on an outdated diff Sep 8, 2013
lib/utils/fetch.js
@@ -73,9 +73,12 @@ function makeRequest (remote, fstr, headers) {
"Auth required and none provided. Please run 'npm adduser'"))
}
- var proxy
- if (remote.protocol !== "https:" || !(proxy = npm.config.get("https-proxy"))) {
- proxy = npm.config.get("proxy")
+ var proxy = null
+ var noproxy = npm.config.get("noproxy") ? npm.config.get("noproxy") : ""
+ if(noproxy.search(remote.hostname) === -1) {
@domenic
npm member
domenic added a note Sep 8, 2013

.search is not a good idea here, because it will convert its argument to a regular expression (so e.g. if remote.hostname is foo.bar.com, and noproxy contains fooxbarxcom.com, it will match). indexOf is better.

@vvision
vvision added a note Sep 9, 2013

You're right. This should work better:

var proxy = null
var noproxy = npm.config.get("noproxy") ? 
  npm.config.get("noproxy").replace(/\s/g, "").split(",") : []
if(noproxy.indexOf(remote.hostname) === -1) {

Like that, it's possible to have multiple noproxy url in the conf like "foo.bar.com, example.com".

@domenic
npm member
domenic added a note Sep 9, 2013

I think space-separated is probably better than comma-separated? But I am not sure what these sorts of things usually use.

@vvision
vvision added a note Sep 9, 2013

I think we can keep comma here.
I'm using GNU Wget manual about environment variables concerning proxies as a reference.

no_proxy
This variable should contain a comma-separated list of domain extensions proxy should not be used for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@domenic
npm member
domenic commented Sep 28, 2013

LGTM but I cannot merge due to #3948; tagging as ready-to-merge

@vvision
vvision commented Sep 30, 2013

Added the modification we discussed in the last outdated diff.

@PaulMougel

Any news?

@Filirom1

Please merge, this pull request will make my life easier :)

@luk-
luk- commented Dec 3, 2013

We should be able to merge this now if everything passes with it.

@rlidwka
rlidwka commented Dec 7, 2013

There is nothing wrong with noproxy config line, but no_proxy environment variable is kinda standard, and it is defined a bit differently. Not as a list of hosts, but a list of domain suffixes.

from wget manual:

no_proxy
This variable should contain a comma-separated list of domain extensions proxy should not be used for. For instance, if the value of no_proxy is `.mit.edu', proxy will not be used to retrieve documents from MIT.

I recently tried to implement this in another package, and it's a bit more tricky than this one.

So, creating different implementation of the same thing is a bit undesirable.

@goddabuzz

We could implement the Java notation and rename it to a more npm standard name non-proxy-hosts

See: http://docs.oracle.com/javase/8/docs/api/java/net/doc-files/net-properties.html

http.nonProxyHosts (default: localhost|127.*|[::1])

Indicates the hosts that should be accessed without going through the proxy. Typically this defines internal hosts. The value of this property is a list of hosts, separated by the '|' character. In addition the wildcard character '' can be used for pattern matching. For example -Dhttp.nonProxyHosts=”.foo.com|localhost” will indicate that every hosts in the foo.com domain and the localhost should be accessed directly even if a proxy server is specified.

The default value excludes all common variations of the loopback address.

@othiym23 othiym23 added review and removed ready-to-merge! labels Jul 23, 2014
@gaboom
gaboom commented Sep 22, 2014

++1

@vvision vvision referenced this pull request in npm/npm-registry-client Sep 22, 2014
Closed

Add noproxy configuration. #69

@gfinger
gfinger commented Sep 23, 2014

I would urgently need this feature. We live behind a company firewall and have some referenced modules external, needing the proxy, others internal, which must not use the proxy. Can anybody roughly estimate how long it will take to have a no_proxy feature in npm?

@mattiasrunge

@gfinger if it is urgent you could patch it in yourself I think. I have been running a patched version of NPM for a year now at my company which also has some internal servers and some external.

in node-0.10.31/lib/node_modules/npm/lib/utils/fetch.js replace the makeRequest function with this:

function makeRequest (remote, fstr, headers) {
  remote = url.parse(remote)
  log.http("GET", remote.href)
  regHost = regHost || url.parse(npm.config.get("registry")).host

  if (remote.host === regHost && npm.config.get("always-auth")) {
    remote.auth = new Buffer( npm.config.get("_auth")
                            , "base64" ).toString("utf8")
    if (!remote.auth) return fstr.emit("error", new Error(
      "Auth required and none provided. Please run 'npm adduser'"))
  }

  // no_proxy patch https://github.com/isaacs/npm/pull/2873
  var proxy = null
  var noproxy = npm.config.get("noproxy") ? 
    npm.config.get("noproxy").replace(/\s/g, "").split(",") : []
  if(noproxy.indexOf(remote.hostname) === -1) {
    if (remote.protocol !== "https:" || !(proxy = npm.config.get("https-proxy"))) {
      proxy = npm.config.get("proxy")
    }
  }

  var sessionToken = npm.registry.sessionToken
  if (!sessionToken) {
    sessionToken = crypto.randomBytes(8).toString("hex")
    npm.registry.sessionToken = sessionToken
  }

  var ca = remote.host === regHost ? npm.config.get("ca") : undefined
  var opts = { url: remote
             , proxy: proxy
             , strictSSL: npm.config.get("strict-ssl")
             , rejectUnauthorized: npm.config.get("strict-ssl")
             , ca: ca
             , headers:
               { "user-agent": npm.config.get("user-agent")
               , "npm-session": sessionToken
               , referer: npm.registry.refer
               }
             }
  var req = request(opts)
  req.on("error", function (er) {
    fstr.emit("error", er)
  })
  req.pipe(fstr)
  return req
}

And in node-0.10.31/etc/npmrc:

noproxy="my.internal.server.com,my.other.internal.server.com"
strict-ssl=false

And in our environment we have:

http_proxy := http://www-proxy.internal.com:8080
no_proxy := internal.com

I guess the no_proxy in the environment might be unused, but for completeness we have it for other apps.

@othiym23 othiym23 added the enterprise label Sep 23, 2014
@gfinger
gfinger commented Sep 24, 2014

Thanks for your answer. I'm on v0.10.31 but there is no fetch.js in the path you specified. I found it under this location

/usr/local/lib/node_modules/npm/node_modules/npm-registry-client//lib/fetch.js

When I substituted the makeRequest function with your code above I got a lot of errors when running npm update. It seems not to fit to the rest of the file. But based on your suggestion I will try to fix the existing file in a minimal invasive way.

Still I wonder, when on can expect to get this functionality in the released npm. The original request is two years old. Why can the fix not be transferred to the master branch of npm?

@vvision
vvision commented Sep 24, 2014

Indeed, the makeRequest function moved to npm-registry-client and proxy related settings can be found in lib/initialize.js.

So I now have a new ongoing pull request on npm/npm-registry-client#69.

It would be great if @othiym23 could provide us with some feedbacks.

@othiym23

@vvision I only have so much time. ;)

@isaacs and I walked through what we think is the best way to get this functionality into npm. While the functionality is implemented in npm-registry-client, the configuration is handled in npm and npmconf, so they'd need updates as well. @isaacs suggested – and I agree – that we should follow the precedent established by wget and curl, and do exactly what they're doing (which means using the same name they do as well, which in this case is no_proxy (wget) / NO_PROXY (curl)).

The most important suggestion we have is this: this work should actually be pushed onto request, because of the way request deals with redirects. If you have a request going through a proxy, and it encounters a redirect, request will handle that transparently. In the case where the original host is not on the no_proxy list, but the redirect target is, if request isn't no_proxy aware, it'll just naïvely redirect. This is an edge case, but there are large internet companies where this is done.

If you file an issue / submit a PR on request for this functionality (i.e. it takes a noProxy option that is a JavaScript array of hostnames to not match), it is likely to be speedily integrated. Then getting it into npm itself is a pretty straightforward matter of bumping the request version and making sure the option gets passed to request in npm-registry-client, and doing the necessary configuration handling, which would happen here and in npmconf.

Good luck and happy hunting! This would be a great feature to have in npm!

@gfinger
gfinger commented Sep 25, 2014

As far as I understand the node coding, the request module is indeed the best place to include the proxy handling. This way all usages of the request module would profit from it.

It would be cute, if the request could just evaluate the settings of the http_proxy, https_proxy and np_proxy variables from the environment. Having the proxy settings in the npm-configuration as it is currently is definitely not optimal, as it redundantly duplicates settings maintained elswhere.

@othiym23

@gfinger We discussed that, because that was my thought as well, but for other reasons npm itself needs to know about the no-proxy list, so the behavior would have to be overrideable from outside request.

There's also the matter of request suddenly sprouting an implicit environmental dependency, which has the potential to surprise people, but that's request's concern, not ours. ;)

@vvision
vvision commented Sep 25, 2014

@othiym23 Thanks!

@tauren
tauren commented Oct 20, 2014

request/request#1096

request@2.45.0 was recently updated to include no_proxy support, and npm@2.1.4 has it as a dependency. This issue can probably be closed.

@othiym23

@tauren request@2.45.0 was included in npm@2.1.5, not npm@2.1.4, so it's not in the latest stable version of npm, but you're right that this is now fixed in npm. I'll try to get to your PR shortly, but that won't land until npm@2.1.6, so it will be a week and a half or so until everything's copacetic in stable npm. Either way, thanks to everyone for their patience!

@othiym23 othiym23 closed this Oct 21, 2014
@othiym23 othiym23 removed the review label Oct 21, 2014
@tauren
tauren commented Oct 21, 2014

@othiym23 thanks for clarifying the version, that was a typo on my part. I look forward to npm@2.1.6.

@othiym23

See the PR – there's a fairly substantial amount of work to be done on it, and I could really use your help if we're to land this this week.

@gratex
gratex commented Oct 31, 2014

Has anyone tried this feature?

It does not work for me. I have tried to debug it, and I have found that the code which is responsible for handling NO_PROXY, is never called:

NO_PROXY is handled in getProxyFromURI method in request/request.js, which is called only if
proxy property is not defined in passed options:

if(!self.hasOwnProperty('proxy')) {
    self.proxy = getProxyFromURI(self.uri)
}

(If proxy property is not set, it will be be read from ENV variable as well as no_proxy)

But npm passes options to requset, which always contains proxy property; see npm-registry-client/lib/initialize.js

The proxy property is set to value defined in configuration or defaults to ENV variable or NULL; see lib/config/defaults.js.

@othiym23

@gratex npm-registry-client could work around this (by only adding a proxy property to the options when there is a proxy or https-proxy option configured in npm), but this looks like an overly restrictive check in request. if (!self.proxy) ... would be what I would expect it to do. Nevertheless, if you want npm-registry-client to work around this behavior, file an issue over there, and that work can be landed with @tauren's work, assuming we get that in landable shape soon. (I personally think this should be fixed in request as well as npm.)

@gratex gratex referenced this pull request in npm/npm-registry-client Nov 3, 2014
Closed

Send proxy property to request only if configured #82

@tauren
tauren commented Nov 6, 2014

I agree that the check in request seems overly restrictive, but I just discovered that when proxy === null, request forces no proxy to be used. This is why if (!self.proxy) ... is not used.

At this point, I'm not sure where it should be fixed. Maybe doing it in npm-registry-client would be best for now. I'm hoping @othiym23 and @nylen might have some ideas on a good solution.

@othiym23 othiym23 referenced this pull request in npm/npm-registry-client Jan 29, 2015
@othiym23 othiym23 request will handle proxy by itself (fixes 82) e2880c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.