- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 21
v1.0.0-rc.1 #1
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
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) { | ||
|          | ||
| 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!) :)