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

Make it possible to use relative URLs #5692

Closed
dhuebner opened this Issue Jan 10, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@dhuebner

dhuebner commented Jan 10, 2017

I'm trying to get bokeh running in a docker/docker-swarm behind a nginx with path to port mapping.
Unfortunately bokeh produces full URL's when creating links which doesn't work in my case.

Starting bokeh with:
bokeh serve untitled.py --host=* --disable-index-redirect

And accessing it over:
https://my.bokeh.server:8443/9335bf292e454cf8ba1181befd4d4a99/p5006/

Produce links like the following:
<link rel="stylesheet" href="http://127.0.0.1:5006/static/css/bokeh.min.css?v=7246afcfffc127faef7c138bce4742e9" type="text/css" />

I've tried setting the explicit --host and also tried the --prefix option, both doesn't fit my requirements. What would work is a possibility to switch using relative URL's. Means instead of http://127.0.0.1:5006/static/ use ./static/ or even better static/. This is the same approach that shiny Apps are using.
For the websocket it could be calculated on the client side

      var protocol = 'ws:';
      if (window.location.protocol === 'https:')
        protocol = 'wss:';
       // TODO check for '/' in pathname and pathname is not already decoded
      var defaultPath = window.location.pathname  + '/websocket/';
      var ws = new WebSocket(protocol + '//' + window.location.host + defaultPath);

So my enhancement is:
Please, make it possible to enable relative URL generation. For example with --use-relative-urls

See also https://groups.google.com/a/continuum.io/forum/#!topic/bokeh/lIrCvXZE18M

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jan 31, 2017

Member

@dhuebner I'd like to to work on this soon, but I don't have an easy set up to test this in "real" situations. Can you commit to testing a PR to confirm that it satisfies your needs?

Member

bryevdv commented Jan 31, 2017

@dhuebner I'd like to to work on this soon, but I don't have an easy set up to test this in "real" situations. Can you commit to testing a PR to confirm that it satisfies your needs?

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jan 31, 2017

Member

@dhuebner I have a branch that can be configured to use relative URLs for the resources. This is fairly straightforward and works great locally.

I am not sure about the ws part. The code that generates the ws URLs uses request.host

    def websocket_url_for_request(self, request, websocket_path):
        # websocket_path comes from the handler, and already has any
        # prefix included, no need to add here
        protocol = "ws"
        if request.protocol == "https":
            protocol = "wss"
        return protocol + "://" + request.host + websocket_path

It's not clear to me that this would need to change, even in the situation you describe? What is the value of the HOST header after passing through nginx?

Member

bryevdv commented Jan 31, 2017

@dhuebner I have a branch that can be configured to use relative URLs for the resources. This is fairly straightforward and works great locally.

I am not sure about the ws part. The code that generates the ws URLs uses request.host

    def websocket_url_for_request(self, request, websocket_path):
        # websocket_path comes from the handler, and already has any
        # prefix included, no need to add here
        protocol = "ws"
        if request.protocol == "https":
            protocol = "wss"
        return protocol + "://" + request.host + websocket_path

It's not clear to me that this would need to change, even in the situation you describe? What is the value of the HOST header after passing through nginx?

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Jan 31, 2017

Member

@dhuebner a WIP PR is here: #5830

Member

bryevdv commented Jan 31, 2017

@dhuebner a WIP PR is here: #5830

@dhuebner

This comment has been minimized.

Show comment
Hide comment
@dhuebner

dhuebner Feb 2, 2017

@bryevdv Regarding the host part. In a docker container it's some IP which is not public accessible because it handled bei yet another nginx proxy where I do not have access to. If I imagine that the app will run in a docker swarm it will become even worse. So the only part that is able to build the full URL for the Web socket is the client.
Could you explain me how to try out your PR? Or point me to a docu?

dhuebner commented Feb 2, 2017

@bryevdv Regarding the host part. In a docker container it's some IP which is not public accessible because it handled bei yet another nginx proxy where I do not have access to. If I imagine that the app will run in a docker swarm it will become even worse. So the only part that is able to build the full URL for the Web socket is the client.
Could you explain me how to try out your PR? Or point me to a docu?

@dhuebner

This comment has been minimized.

Show comment
Hide comment
@dhuebner

dhuebner Feb 2, 2017

@bryevdv I found this PR build https://travis-ci.org/bokeh/bokeh/jobs/197098047 but I have no idea hot to install from it.

dhuebner commented Feb 2, 2017

@bryevdv I found this PR build https://travis-ci.org/bokeh/bokeh/jobs/197098047 but I have no idea hot to install from it.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Feb 3, 2017

Member

@dhuebner does nginx not re-rwrite the host header? I guess I expected that it would. I'm not categorically opposed to having a path for the client to compute the ws URL but that change carries more risk and complexity so I want to be certain it is necessary.

To test the PR, here are the steps in broad outline, let me know if I can add more specifics

  • clone the GH repo or a fork, check out the PR branch
  • install node >= 6.3.1
  • cd bokehjs; npm install
  • cd back to top level
  • python setup.py install --build-js
Member

bryevdv commented Feb 3, 2017

@dhuebner does nginx not re-rwrite the host header? I guess I expected that it would. I'm not categorically opposed to having a path for the client to compute the ws URL but that change carries more risk and complexity so I want to be certain it is necessary.

To test the PR, here are the steps in broad outline, let me know if I can add more specifics

  • clone the GH repo or a fork, check out the PR branch
  • install node >= 6.3.1
  • cd bokehjs; npm install
  • cd back to top level
  • python setup.py install --build-js
@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Feb 4, 2017

Member

I suppose another option is to merge the current PR as-is, as a standalone improvement, and then to cut a dev build. That would probably be easier for you to test with. If you find it's not sufficient we can iterate on it, but ultimately since you seem to have some JS experience it would probably be much more direct / efficient if you can build Bokeh locally and try out some things in the actual environment.

Member

bryevdv commented Feb 4, 2017

I suppose another option is to merge the current PR as-is, as a standalone improvement, and then to cut a dev build. That would probably be easier for you to test with. If you find it's not sufficient we can iterate on it, but ultimately since you seem to have some JS experience it would probably be much more direct / efficient if you can build Bokeh locally and try out some things in the actual environment.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Feb 6, 2017

Member

@havocp @mattpap do either of you have any quick thoughts about the WS question above?

Member

bryevdv commented Feb 6, 2017

@havocp @mattpap do either of you have any quick thoughts about the WS question above?

@havocp

This comment has been minimized.

Show comment
Hide comment
@havocp

havocp Feb 6, 2017

Contributor

I don't know why we don't use relative urls... if they always work we don't need an option to enable them, we could get rid of the --host option and simply prohibit any use of request.host. If it's that simple I wonder if there's a reason we didn't do it that way already though?

wss url would have to be built on the client as @dhuebner suggested I guess.

Contributor

havocp commented Feb 6, 2017

I don't know why we don't use relative urls... if they always work we don't need an option to enable them, we could get rid of the --host option and simply prohibit any use of request.host. If it's that simple I wonder if there's a reason we didn't do it that way already though?

wss url would have to be built on the client as @dhuebner suggested I guess.

@havocp

This comment has been minimized.

Show comment
Hide comment
@havocp

havocp Feb 6, 2017

Contributor

If absolute urls do turn out to be needed, an alternative approach here would be for bokeh to have an insecure mode which trusts request.host and then the user would have to set up the nginx or other reverse proxy to block evil Host headers before Bokeh sees them.

I've seen many people who don't really understand what --host does and they may turn on that insecure mode to get things working and leave it on, though. It's a somewhat dangerous festure. Possibly the mode could complain if there's no X-forwarded header indicating a reverse proxy...

All above irrelevant if relative urls work and we can start ignoring the Host header.

Contributor

havocp commented Feb 6, 2017

If absolute urls do turn out to be needed, an alternative approach here would be for bokeh to have an insecure mode which trusts request.host and then the user would have to set up the nginx or other reverse proxy to block evil Host headers before Bokeh sees them.

I've seen many people who don't really understand what --host does and they may turn on that insecure mode to get things working and leave it on, though. It's a somewhat dangerous festure. Possibly the mode could complain if there's no X-forwarded header indicating a reverse proxy...

All above irrelevant if relative urls work and we can start ignoring the Host header.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Feb 6, 2017

Member

@havocp thank you for your comments, I am inclined now to agree, if we can get rid of the need for --host then we should, has definitely been confusing. I will re-work the PR to do this, and get it out in a dev build and ask for people to test it.

Member

bryevdv commented Feb 6, 2017

@havocp thank you for your comments, I am inclined now to agree, if we can get rid of the need for --host then we should, has definitely been confusing. I will re-work the PR to do this, and get it out in a dev build and ask for people to test it.

@bryevdv

This comment has been minimized.

Show comment
Hide comment
@bryevdv

bryevdv Feb 6, 2017

Member

ping @mrocklin @pitrou who may also have valuable input on this matter. If we can get rid of --hosts, I think we should, I am just not fully equipped to make that decision alone.

E.g., as far as I know, the initial connection (before the WS upgrade) is also subject to whitelist checking. Is that not really needed?

Member

bryevdv commented Feb 6, 2017

ping @mrocklin @pitrou who may also have valuable input on this matter. If we can get rid of --hosts, I think we should, I am just not fully equipped to make that decision alone.

E.g., as far as I know, the initial connection (before the WS upgrade) is also subject to whitelist checking. Is that not really needed?

@havocp

This comment has been minimized.

Show comment
Hide comment
@havocp

havocp Feb 6, 2017

Contributor

I think if we use the Host header we need a whitelist on it, but if we can cleanse the code of request.host (and ideally add automation to notice future mistaken use) then we wouldn't have to validate it. One approach could be to replace the whitelist code with code to delete the header!

Contributor

havocp commented Feb 6, 2017

I think if we use the Host header we need a whitelist on it, but if we can cleanse the code of request.host (and ideally add automation to notice future mistaken use) then we wouldn't have to validate it. One approach could be to replace the whitelist code with code to delete the header!

@havocp

This comment has been minimized.

Show comment
Hide comment
@havocp

havocp Feb 6, 2017

Contributor

if removing --host btw it might be friendly to accept it and do nothing (maybe with deprecation message) rather than throwing an error.

Contributor

havocp commented Feb 6, 2017

if removing --host btw it might be friendly to accept it and do nothing (maybe with deprecation message) rather than throwing an error.

@dhuebner

This comment has been minimized.

Show comment
Hide comment
@dhuebner

dhuebner Feb 7, 2017

@bryevdv @havocp Thanks for your comments and for the great work.
Using relative URLs plus remove the host header will make bokeh setup very easy. Looking forward to test it.

dhuebner commented Feb 7, 2017

@bryevdv @havocp Thanks for your comments and for the great work.
Using relative URLs plus remove the host header will make bokeh setup very easy. Looking forward to test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment