_> I think it's the entire XFF header value which should be considered here _
Yes maybe, but it makes the current codebase more confusing as it's clearly being used as a single address, (named remoteAddress and being displayed as one everywhere). We shouldn't expose proxy servers on UI etc.
_> Just taking the first address (original client ip) won't work if those addresses are behind different NATs - you'll get multiple 192.168.1.2 values from two different machines. And taking the last doesn't work either, as that's just your last proxy - which in cases you're trying to handle, will be the same all the time. _
I don't think that this is the case here: Weinre server is running in an application server that is a sitting behind a proxy (service providers, such as Heroku). Clients network configuration (internal addresses) are not visible in XFF header, because the whole header is created at the point when the request enters Herokus network. This can be verified with:
$ ifconfig | grep 192
inet 192.168.1.118 netmask 0xffffff00 broadcast 192.168.1.255
$ curl --header "X-Forwarded-For: 10.0.0.1, something.else.com" "http://limitless-shelf-9248.herokuapp.com"
10.0.0.1, something.else.com, 220.127.116.11%
In above there is no 192.168.1.118. This is true also for cases when weinre server is running inside of companys private network. (You can also copy paste those commands and check it out)
> I'm happy to display it like we are today, and if it causes a problem later, we can fix it. Please make sure to HTML escape the string though, likely we were not doing that before.
This can not be used to XSS, because if XFF contains something else than the clients IP it will get caught here and no connection will be established: https://github.com/apache/incubator-cordova-weinre/blob/master/weinre.server/lib/channelManager.coffee#L80 that if check stop it.
Also: this would be completely separate issue and not related to this issue (support for proxies vs something in UI) – what is the process here, should one just open a new bug?
> Adding a bullet to multiuser.html seems like a good place to doc this.
Should I contribute this with the pull-request?
> I'd be happy to discuss tests in general at the mailing list, or in a new bug - take your pick.
I'll open a new bug and make the first test case to get this started. Currently it's really shaky to develop something.
Also thinking of fixing the documentation.