plantuml / plantuml-server

PlantUML Online Server
https://plantuml.com/
GNU General Public License v3.0
1.6k stars 463 forks source link

Use relative paths as far as possible. #209

Closed HeinrichAD closed 1 year ago

HeinrichAD commented 2 years ago

Switch from absolute paths (hostpath) to relative paths (contextpath).

Unfortunately, for the URL input JavaScript is necessary to resolve the relative URL. Alternatively, we let this part untouched an use hostpath. But in this case it would be inconsistent. (And the same issues would still remain.) @ctn maybe you want to review this?

Also see Issue #205 and #207.

This also improves the nginx example docker-compose.yml files and fixes the usage of the diagram map. (usemap="#plantuml_map" inside the img tag was missing. Now hovering and clicking the link in the diagram is possible.)

@arnaudroques these changes needs further considerations, review and testing.

ctn commented 2 years ago

@HeinrichAD lgtm.

I've tested it and the input url works whether it is relative or absolute. However for copy-and-paste or aesthetic purposes, it is better to be absolute (inclusive of protocol), as you have done.

Other than that, everything else is relative, and works well.

I don’t think there is anything wrong in using JS to cast url to an absolute version. It is a “requirement” external to the browser processing itself.

In other words, it all looks good and consistent. Nothing kludgy about it.

Kverma517 commented 1 year ago

Can we please merge this pull request? I am trying to use this in a Kubernetes env. It redirects to HTTP instead of HTTPS and thus choking it.

HeinrichAD commented 1 year ago

I am trying to use this in a Kubernetes env. It redirects to HTTP instead of HTTPS [...].

Your problem may be related to issue #207 even if this PR could solve it.


You could also try to set the X-Forwarded-Proto header fix to https as a workaround.

https://github.com/plantuml/plantuml-server/blob/4a5e204e16b6ba811c2bed8b20a656b7947c71f7/src/main/java/net/sourceforge/plantuml/servlet/PlantUmlServlet.java#L299-L306

Kverma517 commented 1 year ago

Thanks for the response. Although the assets embedded on the page are properly using the scheme. The location redirects are still http. Notice the location header.

⬢ [Systemd] ➜ curl -v https://mesh.mydomain.com/plantuml/
:
< HTTP/2 302
< location: http://mesh.mydomain.com/plantuml/uml/SyfFKj2rKt3CoKnELR1Io4ZDoSa70000
< content-length: 0
< server: istio-envoy
< x-envoy-upstream-service-time: 5
< date: Fri, 24 Feb 2023 04:13:52 GMT
<
* Connection #0 to host mesh.mydomain.com left intact

I use the istio virtual service to add the header.

  http:
    - match:
        - port: 443
          uri:
            prefix: /plantuml
      route:
        - destination:
            host: plantuml-server.internal.svc.cluster.local
      headers:
        request:
          set:
            X-Forwarded-Proto: "https"
HeinrichAD commented 1 year ago

I just solved the merge conflicts. So keep in mind that I didn't further test it and I still think that these changes needs further considerations, review and testing. For example: The support of X-Forwarded-Proto is obviously removed, does this have any impact? Does the changes work if the base URL BASE_URL is changes? ...

@Kverma517 you could use the PR code and test your Kubernetes scenario. In the best case you would also go over the changes and consider if the PR could have any impact on other use cases of PlantUML server and test them.

arnaudroques commented 1 year ago

@HeinrichAD @Kverma517 Thanks for your contributions! I let you test: tell us if it is working for you :-)

Kverma517 commented 1 year ago

No It didn't work for me. Looks like following code is an issue.

https://github.com/plantuml/plantuml-server/blob/master/src/main/java/net/sourceforge/plantuml/servlet/PlantUmlServlet.java#L309-L323 Notice it gets the context path from request while redirecting. It should take in account the header X-Forward-Proto for scheme (HTTP or HTTPS). If someone can modify the code I will be happy to test again. Other parts of code do use above header. May be we should make a static until method and use it everywhere we need it.

Many thanks in advance.

HeinrichAD commented 1 year ago

As I understand it, your problem in this case does not necessarily have anything to do with this PR. Could you create a new issue that contains all the information needed to easily reproduce the error?

As it's now implemented, X-Forward-Proto isn't used anymore. That means that the client/the request maker is responsible for everything except the context path. (At least if everything is working as intended.)

HeinrichAD commented 1 year ago

@arnaudroques - Since nobody seems to want to test the PR so far, I decided to take some time myself. The result is that the PR with the current code does NOT work as originally intended, but didn't really break anything either.

Problem: request.getContextPath() doesn't know anything about a reverse proxy like NGINX and returns the wrong context path if the proxy uses a different context path than the proxy pass (plantuml server). @Kverma517 this is maybe also related to your problem.

I see two main ways to fix this at the moment:

  1. use a base tag, which can be easily customized in NGINX with sub_filter if needed.
  2. using a new environment variable e.g. REVERSE_PROXY_BASE_URL and/or a custom header variable e.g. X-Forwarded-Context-Path so that a custom getContextPath() method can be written.

It is also possible to combine the suggestions and do both for the most possible flexibility.

At least the moment, I don`t see any other reasonable solutions.