Allow x-forwarded-for header to support lists with spaces around the commas

[mrlannigan]

Our network structure that alters X-Forwarded-For:
External Interface/Firewall -> HAProxy -> Nginx/Node

HAProxy appends to the list of ips with a , (comma + space).

This PR will allow the spaces to be parsed out of the list before it's split.

https://tools.ietf.org/html/draft-ietf-appsawg-http-forwarded-10#section-7.1

[coveralls]

Coverage increased (+0.4%) to 94.776% when pulling 632d5d6 on mrlannigan:master into 87fe0ba on primus:master.

[jcrugzz]

This LGTM 👍. Any perf considerations here with the regex? @lpinca @3rd-Eden if this is good with either of you i think we should merge

[coveralls]

Coverage increased (+0.5%) to 94.815% when pulling 1345af8 on mrlannigan:master into 87fe0ba on primus:master.

[lpinca]

I wonder if it's better to use trim() here:

ips = (headers[proxies[i].ip] || '').split(',').map(function map(ip) {
  return ip.trim();
});

so we can also remove the trim() from the whitelist check below.

[lpinca]

Probably the regex approach is slightly faster.

[lpinca]

Ok this is indeed faster:

'use strict';

const Benchmark = require('benchmark');

const suite = new Benchmark.Suite;
const re = /\s*,\s*/g;
let list = [];
let i = 0;

for (; i < 1000; i++) list.push(i);
list = list.join(' , ');

suite.add('map', function () {
  list.split(',').map(function map(elem) {
    return elem.trim();
  });
});
suite.add('regex', function () {
  list.replace(re, ',').split(',');
});
suite.on('cycle', function (event) {
  console.log(event.target.toString());
});
suite.on('complete', function () {
  console.log('Fastest is '+ this.filter('fastest').map('name'));
});

suite.run();

map x 3,514 ops/sec ±1.11% (82 runs sampled)
regex x 6,736 ops/sec ±1.09% (85 runs sampled)
Fastest is regex

so ignore the above idea.

[lpinca]

LGTM, thank you for the tests.

[lpinca]

I guess we need the g flag here.

[jcrugzz]

this is still necessary if there is more than 1 correct? Does this case practically happen? cc @3rd-Eden

[lpinca]

@jcrugzz correct. It should have been addressed here.

[lpinca]

With this change this trim() is no longer required, so we can remove it.

[3rd-Eden]

LGTM, going to merge this in. Thanks the PR and the accompanying tests.

On Mar 25, 2016, at 5:59 PM, Julian Lannigan notifications@github.com wrote:

Our network structure that alters X-Forwarded-For:
External Interface/Firewall -> HAProxy -> Nginx/Node

HAProxy appends to the list of ips with a , (comma + space).

This PR will allow the spaces to be parsed out of the list before it's split.

You can view, comment on, or merge this pull request online at:

#8

Commit Summary

Allow x-forwarded-for header to support lists with spaces around the
File Changes

M index.js (2)
M test.js (54)
Patch Links:

https://github.com/primus/forwarded-for/pull/8.patch
https://github.com/primus/forwarded-for/pull/8.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub