Skip to content
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

Using X-Forwarded-Prefix to Support Reverse Proxy #459

Merged
merged 7 commits into from
Aug 29, 2022

Conversation

rodja
Copy link
Collaborator

@rodja rodja commented Aug 26, 2022

There have been several attempts and multiple discussions (#402, #220) about hosting JustPy behind a reverse proxy like Nginx, Apache or Traefik:

This pull request simply adds a <base> tag in the html header which uses the http header field X-Forwarded-Prefix to set the correct url prefix. This header field is normally set by reverse proxies for exactly this purpose.

#fixes #220

Copy link
Collaborator

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the request and can confirm that this is exactly the solution that is running productively in most of our projects.

@frankie567
Copy link

@rodja I'll add my thoughts here so it's easier to follow 😃

From what I understand, we'll rely here on the <base> tag to make the browser aware of the root path. I'm not very fond of this solution, because it means we rely entirely on the browser behavior. Some people also have quite a lot of arguments against this tag.

We also need to manually build the WebSocket URL, and the complexity of the logic involved in this (having to handle the port, the path prefix, etc.) looks to me like a smell that something isn't quite right.

Personnally, I prefer the url_for solution — not very objective here, I admit 🙃 — because we trust Starlette to generate absolute URL and can therefore use the standard root_path parameter from the ASGI spec.

@rodja
Copy link
Collaborator Author

rodja commented Aug 26, 2022

Maybe there is still something wrong. In NiceGUI we experience this bug: zauberzeug/nicegui#64. Maybe some paths are not yet using relative addresses.

@frankie567
Copy link

frankie567 commented Aug 26, 2022

@rodja Dropping my two cents here about zauberzeug/nicegui#64. It's probably because those are fonts file that are defined in hard in the CSS files. Like here:

https://github.com/elimintz/justpy/blob/f878a28babfa67d5ceccc13afa03e21f97a63708/justpy/templates/local/materialdesignicons/iconfont/material-icons.css#L8-L10

https://github.com/elimintz/justpy/blob/f878a28babfa67d5ceccc13afa03e21f97a63708/justpy/templates/local/robotofont/robotofont.css#L173

I'm not really sure how to solve this properly, apart from loading those fonts from a CDN.

@WolfgangFahl WolfgangFahl added core enhancement New feature or request labels Aug 26, 2022
@rodja rodja mentioned this pull request Aug 27, 2022
@rodja
Copy link
Collaborator Author

rodja commented Aug 27, 2022

From what I understand, we'll rely here on the <base> tag to make the browser aware of the root path. I'm not very fond of this solution, because it means we rely entirely on the browser behavior. Some people also have quite a lot of arguments against this tag.

The <base> tag was only introduced to simplify the html code. The main idea of this pull request is the same as in #258: to use the X-Forwarded-Prefix header field to adapt the path.

We also need to manually build the WebSocket URL, and the complexity of the logic involved in this (having to handle the port, the path prefix, etc.) looks to me like a smell that something isn't quite right.

The WebSocket URL is already build manually (see https://github.com/elimintz/justpy/blob/72f94a9f1e7fcd2734b3c947ba2e830387083698/justpy/templates/main.html#L75-L79). But of course using the right path adds to the complexity.

Personnally, I prefer the url_for solution — not very objective here, I admit 🙃 — because we trust Starlette to generate absolute URL and can therefore use the standard root_path parameter from the ASGI spec.

As I wrote in #451 (comment), the url_for seems to not take the reverse-proxied path into account. Seems we cannot rely on Starlette for this...

@rodja rodja changed the title Reverse Proxy Support with <base> Tag Using X-Forwarded-Prefix to Support Reverse Proxy Aug 27, 2022
@rodja
Copy link
Collaborator Author

rodja commented Aug 27, 2022

In 0b30fd3 I also fixed the font issue. Thanks @frankie567 for pointing out the problem in #459 (comment)

@rodja
Copy link
Collaborator Author

rodja commented Aug 29, 2022

@frankie567 pointed out that using <base> tag to eases creating relative links may not be a good idea (see #459 (comment)). I think we have two options:

  1. try to stick with the <base> tag to reduce code clutter and test/work around the corner-cases pointed out on stackoverflow.
  2. create a helper function which provides the base path with X-Forwarded-Prefix and call it whenever the templates need to reference a ressource.

What do you think?

@WolfgangFahl WolfgangFahl added this to the 0.3.0 milestone Aug 29, 2022
@WolfgangFahl WolfgangFahl merged commit 9132842 into justpy-org:master Aug 29, 2022
@WolfgangFahl
Copy link
Collaborator

Tried to merge this but it breaks the CI

@WolfgangFahl
Copy link
Collaborator

I reverted back to v0.3.0 tag. Please restart your pull request making sure it doesn't break the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Equivalent of base_url?
4 participants