-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
v1.0.0-rc.1 #1
base: master
Are you sure you want to change the base?
v1.0.0-rc.1 #1
Changes from 6 commits
d500075
7416d5a
0b8bbc3
e9badf7
5f36bf9
3e90938
0962a47
560dfa6
34ad6cc
4e7e8f4
2d937df
1520eae
8694d1c
5e77858
cbab687
1761f9d
7a3cb41
f39a24f
3363328
553fb6f
e8f6794
6ce460a
3459974
e2c1292
9e54f7a
7aa4ffe
8cf1117
0dc53a6
482dcec
104f0a5
18e0181
9712f4d
462d7e5
4b97e71
cb066f8
8980fbf
cad75c3
bbe1a0d
43b9fae
7f03ddf
f7e83c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ node_js: | |
- "0.8" | ||
- "0.10" | ||
- "0.11" | ||
- "0.12" | ||
matrix: | ||
allow_failures: | ||
- node_js: "0.11" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,65 @@ | ||
/*! | ||
* forwarded | ||
* Copyright(c) 2014 Douglas Christopher Wilson | ||
* MIT Licensed | ||
*/ | ||
|
||
/** | ||
* Module exports. | ||
*/ | ||
'use strict' | ||
|
||
module.exports = forwarded | ||
var Processor = require('./lib/processor') | ||
var schemas = require('./lib/schemas') | ||
|
||
/** | ||
* Get all addresses in the request, using the `X-Forwarded-For` header. | ||
* | ||
* @param {Object} req | ||
* @param {http.IncomingMessage} req | ||
* @api public | ||
*/ | ||
|
||
function forwarded(req) { | ||
module.exports = function forwarded (req, options) { | ||
options = options || {} | ||
|
||
var opts = { | ||
// default to only common + standard | ||
// array order matters here | ||
schemas: options.schemas || [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo it should only default to a single schema, not multiple. The reason is people are mainly going to be set up for one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would argue that typically in libraries such as this, it should be the standard only (i.e. however due to the long history of in that light, both are the defacto default here, other scenarios would be:
this is precisely why the logic at index.js#L52 overwrites properties in the sorted order of this schemas array, existing implementations of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, but this is the biggest issue I have with this assumption: people literally just are not using that header, nor are they blocking it. Because of this, it is extremely easy to fall in a trap where people rely on this module because customers are paying for IP whitelisting. An attacker can then just include a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically I think the standard should be the default and people can always change it (and we can simply add messaging to the README). I just don't like the default being multiple types, because I can just see the confusion/issues coming (I have a lot of experience with the things people report ;) ). I like the idea of the module letting you use multiple at once, though, if the user chooses. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to clarify are you suggesting to keep also, another possible option is to not offer a default, leaving it to the implementer to decide what to support. e.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sure that works. Specifically, I'm suggesting that the default option should be only one schema; which one it is really doesn't matter as much, because people will change it.
Well, that is a possibility, yes, but I'm undecided on that (though if you did it, you'd need to move the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved in 8694d1c |
||
'xff', | ||
'rfc7239' | ||
] | ||
} | ||
|
||
// consistent case | ||
opts.schemas.map(Function.prototype.call, String.prototype.toLowerCase) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Result of map is not being used. Either a mistake or should be forEach. |
||
|
||
if (!req) { | ||
throw new TypeError('argument req is required') | ||
} | ||
|
||
// simple header parsing | ||
var proxyAddrs = (req.headers['x-forwarded-for'] || '') | ||
.split(/ *, */) | ||
.filter(Boolean) | ||
.reverse() | ||
var socketAddr = req.connection.remoteAddress | ||
var addrs = [socketAddr].concat(proxyAddrs) | ||
// start with default values from socket connection | ||
var forwarded = { | ||
addrs: [req.connection.remoteAddress], | ||
by: null, | ||
host: req.headers && req.headers.host ? req.headers.host : undefined, | ||
port: req.connection.remotePort.toString(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a disconnected socket, |
||
ports: [req.connection.remotePort.toString()], | ||
proto: req.connection.encrypted ? 'https' : 'http' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure this actually works in Node.js 0.6 and the others? I only ask because the test is just a mock, so we have to verify manually if we cannot add a real test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was relying on the travis test to be honest, I managed to manually test all the way down to 0.8, then gave up on getting 0.6 installed on my machine. I'll get that working today and confirm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are only as honest as your tests are--since you are using mocks, then you are not actually testing anything (you have to test a real |
||
} | ||
|
||
// alias "for" to keep with RFC7239 naming | ||
forwarded.for = forwarded.addrs | ||
|
||
return opts.schemas | ||
// check if schemas exist | ||
.filter(function (name) { | ||
return schemas[name] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unknown schemas need to throw an error, not be silently dropped. A user making a typo in a schema name may not realize for a while. |
||
}) | ||
|
||
// process schemas | ||
.reduce(function (forwarded, name) { | ||
var result = new Processor(req, schemas[name]) | ||
|
||
// return all addresses | ||
return addrs | ||
// update forwarded object | ||
return { | ||
addrs: forwarded.addrs.concat(result.addrs).filter(Boolean), | ||
by: result.by ? result.by : forwarded.by, | ||
host: result.host ? result.host : forwarded.host, | ||
port: result.port ? result.port : forwarded.port, | ||
ports: forwarded.ports.concat([result.port]).filter(Boolean), | ||
proto: result.proto ? result.proto : forwarded.proto | ||
} | ||
}, forwarded) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
'use strict' | ||
|
||
var debug = require('debug')('forwarded') | ||
|
||
function processor (req, schema) { | ||
if (typeof schema === 'function') { | ||
return schema(req) | ||
} | ||
|
||
this.req = req | ||
this.schema = schema | ||
|
||
return { | ||
addrs: this.addrs(), | ||
host: this.host(), | ||
port: this.port(), | ||
proto: this.protocol() | ||
} | ||
} | ||
|
||
processor.prototype.host = function () { | ||
if (this.schema.host && this.req.headers[this.schema.host]) { | ||
var value = this.req.headers[this.schema.host] | ||
|
||
debug('found header [%s = %s]', this.schema.host, value) | ||
|
||
return this.req.headers[this.schema.host] | ||
} | ||
} | ||
|
||
processor.prototype.port = function () { | ||
if (this.schema.port && this.req.headers[this.schema.port]) { | ||
var value = this.req.headers[this.schema.port] | ||
|
||
debug('found header [%s = %s]', this.schema.port, value) | ||
|
||
return value | ||
} | ||
} | ||
|
||
processor.prototype.addrs = function () { | ||
if (this.schema.addrs && this.req.headers[this.schema.addrs]) { | ||
var value = this.req.headers[this.schema.addrs] | ||
|
||
debug('found header [%s = %s]', this.schema.addrs, value) | ||
|
||
return value | ||
.split(/ *, */) | ||
.filter(Boolean) | ||
.reverse() | ||
} | ||
} | ||
|
||
processor.prototype.protocol = function () { | ||
// utility | ||
function runner (obj) { | ||
// multiple possible values | ||
if (Array.isArray(obj)) { | ||
// get the last succesful item | ||
return obj.map(runner.bind(this)).reduce(function (prev, curr) { | ||
return curr ? curr : prev | ||
}) | ||
} | ||
|
||
if (typeof obj === 'function') { | ||
return obj.call(this, this.req) | ||
} | ||
|
||
if (this.req.headers[obj]) { | ||
debug('found header [%s = %s]', obj, this.req.headers[obj]) | ||
|
||
return this.req.headers[obj] | ||
} | ||
} | ||
|
||
// actually run | ||
if (this.schema.proto) { | ||
return runner.call(this, this.schema.proto) | ||
} | ||
} | ||
|
||
module.exports = processor |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
'use strict' | ||
|
||
var debug = require('debug')('forwarded') | ||
|
||
function isSecure (req) { | ||
try { | ||
var cf = JSON.parse(req.headers['cf-visitor']) | ||
return cf.scheme !== undefined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is broken; according to https://support.cloudflare.com/hc/en-us/articles/200170536-How-do-I-redirect-HTTPS-traffic-with-Flexible-SSL-and-Apache- the value |
||
} catch (e) { | ||
debug('could not parse "cf-visitor" header: %s', req.headers['cf-visitor']) | ||
} | ||
|
||
return false | ||
} | ||
|
||
module.exports = { | ||
addrs: 'cf-connecting-ip', | ||
proto: isSecure | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
'use strict' | ||
|
||
function isSecure (req) { | ||
return ~['1', 'true'].indexOf(req.headers['fastly-ssl']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems according to the docs at https://docs.fastly.com/guides/backend-servers/maintaining-separate-http-and-https-requests-to-backend-servers just the existence of the header with any value is enough to signal SSL. Is there a reason to check the value here? |
||
} | ||
|
||
module.exports = { | ||
addrs: 'fastly-client-ip', | ||
port: 'fastly-client-port', | ||
proto: isSecure | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
cloudflare: require('./cloudflare'), | ||
fastly: require('./fastly'), | ||
microsoft: require('./microsoft'), | ||
nginx: require('./nginx'), | ||
rackspace: require('./rackspace'), | ||
rfc7239: require('./rfc7239'), | ||
xff: require('./xff'), | ||
zscaler: require('./zscaler') | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
proto: function isSecure (req) { | ||
return req.headers['front-end-https'] === 'on' | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'x-real-ip', | ||
port: 'x-real-port', | ||
proto: ['x-real-proto', 'x-url-scheme'] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'x-cluster-client-ip' | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
'use strict' | ||
|
||
function splitMap (string, separator, cb) { | ||
// split into elements | ||
return string.split(separator) | ||
.filter(Boolean) | ||
.forEach(cb) | ||
} | ||
|
||
function parsePart (part) { | ||
var pair = part.split(/ *= */) | ||
|
||
var name = pair[0].toLowerCase() | ||
var value = pair[1] | ||
|
||
if (value) { | ||
switch (typeof this[name]) { | ||
case 'undefined': | ||
this[name] = value | ||
break | ||
|
||
// convert to array | ||
case 'string': | ||
this[name] = [this[name], value] | ||
break | ||
|
||
// append to array | ||
case 'object': | ||
this[name].push(value) | ||
break | ||
} | ||
} | ||
} | ||
|
||
var ELEMENT_SEPARATOR = / *; */ | ||
var PART_SEPARATOR = / *, */ | ||
|
||
module.exports = function (req) { | ||
var forwarded = {} | ||
var header = req.headers.forwarded | ||
|
||
if (!header) { | ||
return forwarded | ||
} | ||
|
||
splitMap(header, ELEMENT_SEPARATOR, function parseElement (el) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like this will fail to parse the header There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from my reading of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is irrelevant; an incoming header can contain any values, valid or not. The current implementation will do the wrong thing and this is not good, as the only reason you are parsing these headers is for logging, auditing, or access controls. Just think of a standard proxy that does not modify anything existing in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, what I'm saying is that the current parse is actually not RFC 7239 compliant, as you are not correctly following the ABNF, which allows for
The given header, Because the RFC allows extensions, perhaps your proxy decides it's going to include a custom field of the authenticated user's name. Uh, oh!
Even though the header is 100% syntactically valid, with a single forwarded entry of And I'm not even a security expert; I can tell you that if we don't fix the parsing, it will most likely end up as a critical security bug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
can you clarify what you mean by that? the token syntax does not actually allow a comma to be present:
so, if anything, we should discard verification vs. validationfurther, imo, the purpose of this library is to parse the headers, _not to verify values_, the resulting values should be filtered through a secondary layer if the implementer chooses to verify values, such as network addresses, host names, etc ... which leaves us with the question: should we add validation here? specifically: to validate addresses, hostnames, ports. as a side, RFC 7239 node identifier syntax could also include unknown and obfuscated identifiers:
which we can easily validate as well. |
||
return splitMap(el, PART_SEPARATOR, parsePart.bind(forwarded)) | ||
}) | ||
|
||
// re-mapping | ||
forwarded.addrs = forwarded.for | ||
|
||
return forwarded | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
'use strict' | ||
|
||
function isSecure (req) { | ||
return req.headers['x-forwarded-ssl'] === 'on' | ||
} | ||
|
||
module.exports = { | ||
addrs: 'x-forwarded-for', | ||
host: 'x-forwarded-host', | ||
port: 'x-forwarded-port', | ||
proto: ['x-forwarded-proto', 'x-forwarded-protocol', isSecure], | ||
protoFn: isSecure | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'z-forwarded-for', | ||
host: 'z-forwarded-host', | ||
port: 'z-forwarded-port', | ||
proto: ['z-forwarded-proto', 'z-forwarded-protocol'] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to keep these headers so we know where in the source the copyrights lives (and so the note is retained when people copy the file but not the license).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, forgot to re-add those! (mind tends to zone those out!) :)