ryanbillingsley / express-ipfilter Goto Github PK

I'm trying to whitelist some CIDR blocks (Fastly CDN, to be specific) such that request from any other IPs are denied. However, requests from any IP appear to be allowed. Am I missing something obvious?

`
var listenCallback = function(/err/) {

var port = Number(process.env.PORT || 3005);
app.listen(port, function () {
winston.info('Elevation proxy is listening on port ' + port);
});
};

var whitelistFastly = function(cb) {

var options = {
method: 'GET',
uri: 'https://api.fastly.com/public-ip-list'
};

winston.debug('Requesting Fastly IP Addresses:', options);
request(options, function (error, response, body) {

if (error) {
  winston.error('Error received for Fastly IPs request:', error);
  return cb(err);
}

if (200 !== response.statusCode) {
  // No error, but not a 200 either
  winston.warn('Fastly IPs response NOT OK:', options.uri, response.statusCode);
  return cb();
}

var ips = JSON.parse(body).addresses;
winston.info('Whitelisting Fastly IP addresses:', ips);

app.use(ipfilter(ips, {mode: 'allow', allowedHeaders: ['X-Forwarded-For']}));

cb();

});
};

// Start the server once we have Fastly whitelisted
whitelistFastly(listenCallback);`

This is with express-ipfilter 0.2.0 running on node 6.9.1, express 4.14.0.

Thanks.

In my express app, ipv4 IPs are represented by ::ffff:255.255.255.255 which is their ipv6 alternative. This causes the ipfilter to break as it doesn't know how to handle the ::ffff: prefix. I've cheated by hard-coding this into the ipfilter for now, but it would be nice if you could add this as a core filter.

The hard-code I used is as folows:

if (~ipAddress.indexOf('::ffff:')) {
  ipAddress = ipAddress.split('f:')[1];
}
    
if (ipAddress == '::1') {
  ipAddress = '127.0.0.1'
}

I should add that I've not read the ipv6 spec, so I'm not sure if this is the proper way to filter the converted ipv4's.

If you like i can provide a PR with the definitions i came up with (by reverse engineering the JS code 🐟)

CircleCI should be set up so that when a build is successful on master, it will increment the version number and then publish the package to npm.

This library assumes that it may be behind a proxy, and blindly trusts the request headers: https://github.com/baminteractive/express-ipfilter/blob/5161db16503964b980fa143c8b8d61c436040571/lib/ipfilter.js#L62-L64

Since X-Forwarded-For and CF-Connecting-IP can be added by any client, the fact that the library relies on it makes it useless for access control. To fix the issue, do not use these request headers, and add an option to use these headers ONLY IF set by the developer.

express/express does offer application-wide settings to handle proxied request by setting 'trust proxy' option:

app.set('trust proxy', 'loopback') // specify a single subnet
app.set('trust proxy', 'loopback, 123.123.123.123') // specify a subnet and an address
app.set('trust proxy', 'loopback, linklocal, uniquelocal') // specify multiple subnets as CSV
app.set('trust proxy', ['loopback', 'linklocal', 'uniquelocal']) // specify multiple subnets as an array

The default value is false. Full documentation: http://expressjs.com/en/guide/behind-proxies.html

My suggestion is to integrate jshttp/proxy-addr into this project and drop the current solution of getClientIp, to work just like express works with proxied requests. People needing to control other headers could still append their getClientIp function into this module and enforce their desired headers. This way we could just append the same setting we use application-wide and it would work as expected out of the box, like it works in express.

I am implementing such a paradigm shift, in my fork of this project, if someone is interested in it as well take a look at it.

https://github.com/pdefreitas/express-ipfilter/

Thanks,
Pedro

Somebody may want to send error response in json format.

Of course, I am one of the somebody who want to this feature. ^^
My application spec requires to response error condition like below.
" {code:'failure', reason:'not allowed'} "

res.end() makes error ( it only accepts String ) but res.send() does not.

Hi,

I tried setting the logLevel to deny but I still get logs for excluded paths saying "Access granted for excluded path: /health" is there a way to turn that off?

hey, thanks for the library!

I wanted to ask if would be possible to add a feature in order to filter like this:

['80.150.69.1-80.150.69.55']

Thanks & Cheers
Qiong

Hi, I'm trying to use express-ipfilter to restrict access to a site via whitelisting, on azurewebsites.net which uses IP address forwarding via headers (x-forwarded-for works fine). When testing on Azure, I see these console.log entries:

Access denied to IP address: xx.xx.xx.xx:37442
req.ip: undefined
forw.ip: xx.xx.xx.xx:37442
whitelist: xx.xx.xx.xx

This is how we invoke the ipfilter middleware:

app.use(ipfilter(['::1', '127.0.0.1', process.env.IP_WHITELIST], {
  log: true,
  logLevel: 'all',
  mode: 'allow',
  allowedHeaders: ['x-forwarded-for']
}))

We're using the whitelist mode (mode: 'allow').

The first log entry is clearly the express-ipfilter output. The second line is where we check whether the req object provides an IP address, and then we look for an x-forwarded-for header. The last line is just the contents of the IP white list that's stored as an environment variable. By the way, we actually use an IP address range, but the problem occurs also when just using one IP address.

Could there be a problem with the fact that the port is present (and which changes for each request)? Because that's the only reason I can see.

Simulating this locally, in the dev environment works fine (with both IP v6 and v4 addresses). But here we don't have the port present.

Thanks for any help or guidance.

When the server receives a request from a blacklisted IP, the server crashes immediately.

Access denied to IP address: ::ffff:127.0.0.1
Error
    at /Users/mauriziocarboni/development/novus/novus-api/node_modules/express-ipfilter/src/ipfilter.js:186:15
    at Layer.handle [as handle_request] (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:312:13)
    at /Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:280:7
    at next (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:271:10)
    at expressInit (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/middleware/init.js:33:5)
    at Layer.handle [as handle_request] (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:312:13)
    at /Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:280:7
    at next (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:271:10)
    at query (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/middleware/query.js:44:5)
    at Layer.handle [as handle_request] (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:312:13)
    at /Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:280:7
    at next (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:271:10)
    at Function.handle (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/router/index.js:176:3)
    at Function.handle (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/application.js:173:10)
    at Server.app (/Users/mauriziocarboni/development/novus/novus-api/node_modules/express/lib/express.js:38:9)
    at emitTwo (events.js:106:13)
    at Server.emit (events.js:191:7)
    at HTTPParser.parserOnIncoming [as onIncoming] (_http_server.js:547:12)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:99:23)

This should make it possible to pull in IPs with every evaluation of the middle ware #47

Unfortunately, yesterday's pull request doesn't work as expected, even though the tests passed. After some research, I understood that the isV4Format method of the ip npm module used does not handle IP addresses with port numbers correctly. See the following code:

require('ip').isV4Format('127.0.0.1:1234') --> false

https://runkit.com/jfix/npm-ip-module-bug

This is, sadly, a known but not-yet-fixed issue in that module:

• indutny/node-ip#73

Therefore, I'd like to submit a new 'hotfix' to my pull request (of course also as a PR), as I've been able to test it in real life and know it will work this time.

Since u r using x-forwarded-for header and headers can of course be modified, creating random ip addresses and sending them through x-forwarded-for is possible...

Node seems to favour IPv6 where availiable. So locally my IP is detected as "::1".

Setting up the whilist as: var ips = ['127.0.0.1', '::1']; work exactly as advertised and i am allowed in.

However, when hosted the IP's are IPv4 and i need to CIDR aswell. I have tried the following:

var ips = ['127.0.0.1/32', '::1/32', '170.20.68.68/32', '170.20.70.30/32'];
var ips = ['127.0.0.1', '::1', '170.20.68.68/32', '170.20.70.30/32'];

Both result in this message sent to the browser: Invalid net address: ::1
It's not a node error, everything appears to work fine behind the screen.

It seems the CIDR filter doesn't work with IPv6, or the combination of IPv6 and 4 along with CIDR is malfunctioning.

Can you advice?

I'd like to have an option to shut off the logging. When I'm blacklisting, I'd only want to see attempts from the blacklisted IP show up in the logs, but it's logging every single request from every single IP telling me it granted them access. That's way more than I was wanting. How can I shut this off without uninstalling?

I just upgraded to 0.1.0 from 0.0.24 and noticed that the localhost address has changed, which in my case made my application block localhost. I solved it in OSX by using ::ffff:127.0.0.1 but on Windows under IISNode the localhost IP seems to be and empty string (Access denied to IP address:). Allowing an empty string in the array of allowed IPs seems to allow any IP.

Is there an issue with fetching the localhost IP properly?

In the recent version of Node and Express 4.0.0, req.connection.remoteAddress returns ::1 and this is currently not supported in this module.

see my comment on cc7de42#diff-0d88c891ffe77973a1401c66be34ce38R78 line 78

Whenever an IP is denied access - One gets an error message

 name: 'IpDeniedError',
  message: 'Access denied to IP address: 127.0.0.2',
  extra: undefined }

How does one get rid of the "undefined }" error message.

I'm trying to limit access to my express server and I have multiple values for the x-forwarded-for header. I want to allow access to the server if any of those values match the IPs i've designated. Is there a way to do this?

The primary example in "Usage with Express" is out of date. createServer has been deprecated in express 2.x. Updating with a working example using the current express 4.0 would be greatly appreciated - thanks!

Blocking IPs using cird ranges doesn't seem to work properly. It allows IPs even if they are in one of the ranges supplied.
The problem seems to be here. If you happen to set notBannedIp to true in a previous iteration of the loop, even if here allowedIp is false the function will return true here.

The module isn't supporting multiple ip's in combination with CIDR's anymore, latest version dropped this "feature".

I would like to be able to dynamically change the ips I'm filtering, since this is used as an express middleware if I want to change anything I have to define a whole new express app, since I have more middlewares defined, and I would like to just be able to update this list of ips.

#48

Vulnerability report: https://snyk.io/vuln/npm:lodash:20180130

Maybe lodash could be swapped out for some modern JS features? Not sure how this lib is handling potentially breaking changes however.

I'd like to update the IP block list on the fly, is there a good way to do this while still using it as an express middleware? Thanks

Hi.
I really like this module … but it messes up with IPv6, which is now standard on the net module of Node v4 …

https://github.com/baminteractive/express-ipfilter/blob/master/lib/ipfilter.js#L88

My log is quickly filling up with "Access granted" entries. I could turn off logging entirely, but for security purposes I would like to be able to see denied access attempts. How about adding an option be more specific in terms of logging? For example via a new option (string) logLevel with options of all|deny|allow defaulting to all.