Apache Cordova
  1. Apache Cordova
  2. CB-1494

Supports running server behind a proxy, such as Heroku Cedar

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: weinre
    • Labels:
      None

      Activity

      Hide
      Patrick Mueller added a comment -

      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.

      Show
      Patrick Mueller added a comment - 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.
      Hide
      Patrick Mueller added a comment -

      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.

      Show
      Patrick Mueller added a comment - 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.
      Hide
      Matti Paksula added a comment -

      > 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):

      $ curl --header "X-Forwarded-For: gooby.plz.dolan.io" "http://limitless-shelf-9248.herokuapp.com"
      trololololo.dolan.io, 88.112.131.21

      ... 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?)

      Show
      Matti Paksula added a comment - > 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): $ curl --header "X-Forwarded-For: gooby.plz.dolan.io" "http://limitless-shelf-9248.herokuapp.com" trololololo.dolan.io, 88.112.131.21 ... 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?)
      Hide
      Patrick Mueller added a comment -

      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

      Show
      Patrick Mueller added a comment - 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
      Hide
      Matti Paksula added a comment -

      _> 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, 88.112.131.21%

      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.

      Show
      Matti Paksula added a comment - _> 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, 88.112.131.21% 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.
      Hide
      Patrick Mueller added a comment -

      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

      Show
      Patrick Mueller added a comment - 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
      Hide
      Matti Paksula added a comment -

      > 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:

      $ 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

      > 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.

      Show
      Matti Paksula added a comment - > 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: $ 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 > 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.
      Hide
      Patrick Mueller added a comment -

      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.

      Show
      Patrick Mueller added a comment - 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.
      Hide
      Matti Paksula added a comment -

      _> 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.

      Show
      Matti Paksula added a comment - _> 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.
      Hide
      Patrick Mueller added a comment -

      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 $).

      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 , ' ]
      
      
      Show
      Patrick Mueller added a comment - 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 $). 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 , ' ]
      Hide
      Patrick Mueller added a comment -

      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.

      Show
      Patrick Mueller added a comment - 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.

        People

        • Assignee:
          Patrick Mueller
          Reporter:
          Patrick Mueller
        • Votes:
          0 Vote for this issue
          Watchers:
          2 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development