You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by "Patrick Mueller (JIRA)" <ji...@apache.org> on 2012/09/17 23:31:07 UTC
[jira] [Created] (CB-1494) Supports running server behind a proxy,
such as Heroku Cedar
Patrick Mueller created CB-1494:
-----------------------------------
Summary: Supports running server behind a proxy, such as Heroku Cedar
Key: CB-1494
URL: https://issues.apache.org/jira/browse/CB-1494
Project: Apache Cordova
Issue Type: New Feature
Components: weinre
Reporter: Patrick Mueller
Assignee: Patrick Mueller
created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Matti Paksula (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13460485#comment-13460485 ]
Matti Paksula commented on CB-1494:
-----------------------------------
_> re: matcher; what if the XFF has multiple comma-separated addresses, with no spaces between them? _
You're right, fixed that now.
About XSS: it's not possible to inject through remoteAddress, because the link won't establish (https://github.com/apache/incubator-cordova-weinre/blob/master/weinre.server/lib/channelManager.coffee#L80) BUT even if that would be possible, it's eliminated already in the view layer: https://github.com/apache/incubator-cordova-weinre/blob/master/weinre.web/modules/weinre/client/RemotePanel.coffee#L161 where key.escapeHTML() is used.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13458757#comment-13458757 ]
Patrick Mueller commented on CB-1494:
-------------------------------------
re: matcher; what if the XFF has multiple comma-separated addresses, with no spaces between them?
re: XSS - I understand. Are there existing XSS holes? There may be, and we should look, and generalize a sanitizing story with a new bug, if we do in fact find something. But I'd also be happy if we could at least ensure that the resulting XFF header value that we calculate doesn't have any XSS possibilities, as we certainly haven't made the situation any worse. And in this case, I think we can.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13457374#comment-13457374 ]
Patrick Mueller commented on CB-1494:
-------------------------------------
Note that one of the pull comments says: "Signing Apache CCLA is in the works".
Note that technically, the CCLA is the CORPORATE CLA - http://www.apache.org/licenses/cla-corporate.txt
The corporate CCLA I think is only required if your company actually requires this, or if for some reason your contributions should be associated with your company. I think most people submit one of these for their company.
But there's another CLA that's needed, for each individual that contributes code. That's the ICLA (often just referred to as CLA). It's here:
http://www.apache.org/licenses/icla.txt
Whether or not you submit a CCLA, YOU WILL ALSO NEED TO SUBMIT AN ICLA.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13457394#comment-13457394 ]
Patrick Mueller commented on CB-1494:
-------------------------------------
This commit looks fine to me, in terms of the code.
I was wondering about "security aspects", but really the ip is only used to further namespacing of the connection, and to ensure subsequent connections are from the same ip. I don't think there are really any further security implications by blindly reading/writing the xff header value.
Although, I'm wondering about places we dump the ip address into HTML to being with - like in the remote panel. Can someone check that?
Also note that it wasn't immediately clear to me that the XFF header can actually be a set of IP addresses, not just one. See: http://rod.vagg.org/2011/07/wrangling-the-x-forwarded-for-header/
Again, I don't think this impacts anything, as it's a namespacing deal, primarily, though it does show up in the UI in places.
Should we add something to the doc? I'm thinking a quick mention that we support XFF should be good enough.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13457819#comment-13457819 ]
Patrick Mueller commented on CB-1494:
-------------------------------------
Thinking about it, I think it's the entire XFF header value which should be considered here, unless there are cases where that header value might change while you are in a debug session. Seems like that would be unusual.
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.
But I think considering the entire value will probably work, as it should uniquely identify a client.
I'm a little worried about is the display of that value to the user on the remote panel. 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.
In terms of security, I guess the case I'm wondering about is if you DON'T run behind a proxy that adds the XFFs, then an evil client can add one himself, and the server won't know any different. But it's also possible to spoof ip addresses at the IP level, so, not sure there any real loss of security here. And the only real security in weinre is obscurity anyway - we may need to look at this closer if we decide to add real security to weinre.
Adding a bullet to multiuser.html seems like a good place to doc this.
As for test cases, we don't have any real ones now - if you'd like to create one, even for just this patch, that would be awesome. I'd be happy to discuss tests in general at the mailing list, or in a new bug - take your pick. There are some tests in WebKit itself for Web Inspector, and it would be interesting to see if we could reuse these.
Documentation is an easier story. Fix whatever needs to be fixed. Wanna totally rewrite it, make it gorgeous? Do it. Open a new bug :-)
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (CB-1494) Supports running server behind a proxy,
such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Mueller resolved CB-1494.
---------------------------------
Resolution: Fixed
fixed in commit:
https://git-wip-us.apache.org/repos/asf?p=cordova-weinre.git;a=commit;h=19454cfe2cd3c4515f7fb615f35fa2c7b2565401
commit comment:
Easiest thing to do is just neuter the remote address check in
the getChannel() function in the weinre.server/lib/channelManager.coffee
source file.
All this provided before was a simplistic check to make sure the
requests always were coming from the same remote address; for
someone who wanted to cheat, there were ways of cheating, and so
still are. For everyone else, you can now run under proxy servers
transparently.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Matti Paksula (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13457763#comment-13457763 ]
Matti Paksula commented on CB-1494:
-----------------------------------
_> Whether or not you submit a CCLA, YOU WILL ALSO NEED TO SUBMIT AN ICLA._
Yes, both ICLA (5x from all of us at AppGyver) and CCLA are being submitted to ASF ~tomorrow.
_> Also note that it wasn't immediately clear to me that the XFF header can actually be a set of IP addresses, not just one._
You are right, I did not look into XFF in detail. I've updated the pull request to take the last ip or host from the string (the last is set by the proxy and I verified that it actually works in Heroku):
{quote}
$ curl --header "X-Forwarded-For: gooby.plz.dolan.io" "http://limitless-shelf-9248.herokuapp.com"
trololololo.dolan.io, 88.112.131.21
{quote}
... so actual host/ip gets set even if spoofing is attempted. So there should not be any security implications.
I've updated the pull-request with a new commit to support a set of addresses.
Testable pre-built NPM of that is here: https://github.com/downloads/AppGyver/incubator-cordova-weinre/apache-cordova-weinre-2.0.0-pre-H78XI5TK-incubating-bin.tar.gz
We could also add an option to turn support for XFF on, but I think it should be the default behaviour and used if set (like you would expect when you deploy Weinre to Heroku or other proxied environment)
_> Although, I'm wondering about places we dump the ip address into HTML to being with - like in the remote panel. Can someone check that?_
Yes it gets dumped, but should this be fixed?
_> Should we add something to the doc? I'm thinking a quick mention that we support XFF should be good enough._
Yes, adding a bullet in MultiUser.html notes section?
(are there any plans to write some tests for weinre?)
(what about the state of documentation?)
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13460506#comment-13460506 ]
Patrick Mueller commented on CB-1494:
-------------------------------------
The regex won't match headers with extra "," or spaces at the end. Prolly not a huge problem, but who knows. Simple fix to make it more robust below (add an extra (\s|,) before $).
{noformat}
node
> "a b c".match(/([^\s|,]+)$/)
[ 'c',
'c',
index: 4,
input: 'a b c' ]
> "a b c ".match(/([^\s|,]+)$/)
null
> "a b c".match(/([^\s|,]+)(\s|,)*$/)
[ 'c',
'c',
undefined,
index: 4,
input: 'a b c' ]
> "a b c ".match(/([^\s|,]+)(\s|,)*$/)
[ 'c ',
'c',
' ',
index: 4,
input: 'a b c ' ]
> " a , b, c , ".match(/([^\s|,]+)(\s|,)*$/)
[ 'c , ',
'c',
' ',
index: 9,
input: ' a , b, c , ' ]
{noformat}
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Matti Paksula (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13458744#comment-13458744 ]
Matti Paksula commented on CB-1494:
-----------------------------------
_> Unfortunately, your regex matcher in resolveRemoteAddressFrom() is not correct; it needs to parse against commas as well._
I think my matcher: /[^\s]+$/ is correct and actually better than something that takes commas into account it reads last "token" from the end, see below:
{quote}
$ node
> matcher = /[^\s]+$/
/[^\s]+$/
> "10.10.10.10, gooby.plz.dolan.io, 23.24.12.101".match(matcher)
[ '23.24.12.101',
index: 33,
input: '10.10.10.10, gooby.plz.dolan.io, 23.24.12.101' ]
> "23.24.12.101".match(matcher)
[ '23.24.12.101',
index: 0,
input: '23.24.12.101' ]
> "".match(matcher)
null
{quote}
_> If we can ensure the new remoteAddress will never contain any HTML, because we've done a great job parsing it, then I'm not concerned about XSS._
I think this should not be addressed on parsing remoteAddress, but on view layer instead, so that *any* input would be sanitized before outputting it to the browser (like all web frameworks do) -- that's why I see this as a new bug, that would not only contain remoteAddress in HTML, but all possible input, also for the future.
Ok, I'll also do doc change, also agree that.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13458599#comment-13458599 ]
Patrick Mueller commented on CB-1494:
-------------------------------------
re: using last element of the XFF header as remoteAddress: +1 - let's try that for now and tweak later if we need to. Unfortunately, your regex matcher in resolveRemoteAddressFrom() is not correct; it needs to parse against commas as well. The article http://rod.vagg.org/2011/07/wrangling-the-x-forwarded-for-header/ seems to have some regexes in there, I didn't look at them to see if they'd be appropriate (or correct).
re:xss on the value. If we can ensure the new remoteAddress will never contain any HTML, because we've done a great job parsing it, then I'm not concerned about XSS. Otherwise I am. I wouldn't necessarily consider this to be another bug. Highly related, I'm happy to keep a bunch of highly related changes together in one batch. But if that's the way you'd like to roll, fine.
re: multiuser.html fix. Again, all lumped into a single pull request, or separate; in this bug, or new bug. I prefer keeping everything like this together, minimize the # of pull requests and bugs.
re: test cases, yes; new bug sounds good.
re: doc, excellent! again, do whatever. new bug :-)
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (CB-1494) Supports running server behind a
proxy, such as Heroku Cedar
Posted by "Matti Paksula (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-1494?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13458570#comment-13458570 ]
Matti Paksula commented on CB-1494:
-----------------------------------
_> 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:
{quote}
$ 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, 88.112.131.21%
{quote}
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.
> Supports running server behind a proxy, such as Heroku Cedar
> ------------------------------------------------------------
>
> Key: CB-1494
> URL: https://issues.apache.org/jira/browse/CB-1494
> Project: Apache Cordova
> Issue Type: New Feature
> Components: weinre
> Reporter: Patrick Mueller
> Assignee: Patrick Mueller
>
> created for https://github.com/apache/incubator-cordova-weinre/pull/10
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira