-
-
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 all 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 |
---|---|---|
@@ -1,8 +1,9 @@ | ||
/*! | ||
* forwarded | ||
* Copyright(c) 2014 Douglas Christopher Wilson | ||
* Copyright(c) 2015 Ahmad Nassri | ||
* MIT Licensed | ||
*/ | ||
*/ | ||
|
||
'use strict' | ||
|
||
|
@@ -11,29 +12,83 @@ | |
* @public | ||
*/ | ||
|
||
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 | ||
* @return {array} | ||
* @param {http.IncomingMessage} req | ||
* @return {object} | ||
* @public | ||
*/ | ||
|
||
function forwarded(req) { | ||
module.exports = function forwarded (req, options) { | ||
options = options || {} | ||
|
||
var opts = { | ||
// default to only standard and x-forwarded for backward compatibility | ||
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 |
||
'rfc7239', | ||
'x-forwarded' | ||
] | ||
} | ||
|
||
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 ? req.connection.remotePort.toString() : undefined, | ||
ports: [], | ||
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 |
||
} | ||
|
||
// add default port to ports array if present | ||
if (forwarded.port) { | ||
forwarded.ports.push(forwarded.port) | ||
} | ||
|
||
// alias "for" to keep with RFC7239 naming | ||
forwarded.for = forwarded.addrs | ||
|
||
return opts.schemas | ||
// check if schemas exist | ||
.map(function (name) { | ||
// adjust case | ||
name = name.toLowerCase() | ||
|
||
if (!schemas[name]) { | ||
throw new Error('invalid schema') | ||
} | ||
|
||
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, schema) { | ||
var result = processor(req.headers, schema) | ||
|
||
// ensure reverse order of addresses | ||
if (typeof result.addrs === 'string') { | ||
result.addrs = result.addrs | ||
.split(/ *, */) | ||
.filter(Boolean) | ||
.reverse() | ||
} | ||
|
||
// 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,30 @@ | ||
'use strict' | ||
|
||
var debug = require('debug-log')('forwarded') | ||
|
||
module.exports = function processor (headers, schema) { | ||
if (typeof schema === 'function') { | ||
return schema(headers) | ||
} | ||
|
||
var result = {} | ||
|
||
var fields = ['addrs', 'host', 'port'] | ||
|
||
fields.forEach(function (field) { | ||
if (schema[field] && headers[schema[field]]) { | ||
var value = headers[schema[field]] | ||
|
||
debug('found header [%s = %s]', schema[field], value) | ||
|
||
result[field] = value | ||
} | ||
}) | ||
|
||
// get protocol | ||
if (typeof schema.proto === 'function') { | ||
result.proto = schema.proto(headers) | ||
} | ||
|
||
return result | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
'use strict' | ||
|
||
var debug = require('debug-log')('forwarded') | ||
|
||
module.exports = { | ||
addrs: 'cf-connecting-ip', | ||
proto: function protocol (headers) { | ||
try { | ||
var cf = JSON.parse(headers['cf-visitor']) | ||
if (cf.scheme) { | ||
return cf.scheme | ||
} | ||
} catch (e) { | ||
debug('could not parse "cf-visitor" header: %s', headers['cf-visitor']) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'fastly-client-ip', | ||
port: 'fastly-client-port', | ||
proto: function protocol (headers) { | ||
return headers['fastly-ssl'] ? 'https' : undefined | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
'cloudflare': require('./cloudflare'), | ||
'fastly': require('./fastly'), | ||
'microsoft': require('./microsoft'), | ||
'rfc7239': require('./rfc7239'), | ||
'weblogic': require('./weblogic'), | ||
'x-cluster': require('./x-cluster'), | ||
'x-forwarded': require('./x-forwarded'), | ||
'x-real': require('./x-real'), | ||
'z-forwarded': require('./z-forwarded') | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
proto: function protocol (headers) { | ||
if (headers['front-end-https']) { | ||
return headers['front-end-https'] === 'on' ? 'https' : 'http' | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
'use strict' | ||
|
||
var parse = require('forwarded-parse') | ||
var debug = require('debug-log')('forwarded') | ||
|
||
module.exports = function (headers) { | ||
if (!headers.forwarded) { | ||
return {} | ||
} | ||
|
||
try { | ||
var result = parse(headers.forwarded) | ||
|
||
var forwarded = { | ||
addrs: result.for ? result.for.reverse() : [], | ||
by: result.by ? result.by[0] : undefined, | ||
host: result.host ? result.host[0] : undefined, | ||
port: result.port ? result.port[0] : undefined, | ||
ports: result.port, | ||
proto: result.proto ? result.proto[0] : undefined | ||
} | ||
} catch (e) { | ||
debug(e) | ||
} | ||
|
||
return forwarded | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'wl-proxy-client-ip' | ||
} |
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,20 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'x-forwarded-for', | ||
host: 'x-forwarded-host', | ||
port: 'x-forwarded-port', | ||
proto: function protocol (headers) { | ||
if (headers['x-forwarded-proto']) { | ||
return headers['x-forwarded-proto'] | ||
} | ||
|
||
if (headers['x-forwarded-protocol']) { | ||
return headers['x-forwarded-protocol'] | ||
} | ||
|
||
if (headers['x-forwarded-ssl']) { | ||
return headers['x-forwarded-ssl'] === 'on' ? 'https' : 'http' | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
'use strict' | ||
|
||
module.exports = { | ||
addrs: 'x-real-ip', | ||
port: 'x-real-port', | ||
proto: function protocol (headers) { | ||
if (headers['x-real-proto']) { | ||
return headers['x-real-proto'] | ||
} | ||
|
||
if (headers['x-url-scheme']) { | ||
return headers['x-url-scheme'] | ||
} | ||
} | ||
} |
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!) :)