Fixes #1: ShareUrl: Dynamically created by client

Made the client contruct the shareUrl, to account for the use of proxies. Now the server sends the client only its ip, and the endpoint. The client then constructs the shareUrl link by getting the same url he is connected to. If the url is "localhost" though, he will use the "ip" sent by the server (since most people connect with "localhost", but want to have a function share link).

added 1 commit

• 130b5625 - Removed parasite println

Compare with previous version

Thanks! Don't you need the same logic for the websocket?

Beyond that I think the code looks reasonable. An alternative might have been to keep everything on the server side (using the host header), but I'm not sure whether it would be better.

Although since we're going to do Host validation in !35 (merged) it might be better to do the host calculation on the server.

Yes I agree with you, I think that if the server can construct the correct url it should. (So that we do not have to send our ip to the client, this would defeat the use of a proxy service..).

(Proof that it can be possible on the server : [17/Dec/2024:12:52:21 +0000] "GET /static/main.css HTTP/1.1" 200 3622 "https://gramegna.ch/" "Mozilla/5.0 (X11; Linux x86_64; rv:133.0) Gecko/20100101 Firefox/133.0" request_body=- headers=gramegna.ch (NGINX log of traffic proxied through cloudflare; we have access to the host, and the protocol (https))

If we do it statically (on the server) for the shareUrl, why also not statically for the websocket?

If we do it statically (on the server) for the shareUrl, why also not statically for the websocket?

Some routers don't less a computer connect to itself using its local IP, so you can't access yourself at localhost but not at your own LAN IP, so we made sure that the websocket uses the exact same location as the current page (hence the client-side construction). In contrast, the share link is for others, so should use the LAN IP.

I think I agree with you that we want to avoid leaking the server's IP in general, so making the decision server-side seems reasonable. On the other hand, we have to do some work client-side, because by the time we get to the server we've lost the port and the protocol. Worse: some proxies may want to rewrite the host as well.

I think the simplest here may be a configuration parameter. If it's true, we give the shareUrl (like the websocket url) a {{ hostName }} template argument. If it's false, we give the server's IP and port.

Okay I have changed the code to implement this; tell me what you think:

If the parameter [..] is set to true : the webapplib functions as it did before those changes. Here is the code :

       val hostName = 
          if Config.IS_SHARE_URL_FIXED_BY_SERVER then
            f"$hostAddress:${Config.HTTP_PORT}"
          else
            "{{hostName}}"
        val shareUrl = 
            f"{{protocol}}://${hostName}$shareEndPoint${Endpoints.App}/${inst.appInfo.id}/$instanceId"  

As you can see, I did not use the cask.Request.headers.get("host"), because I have had a little bit of trouble with it returning a scala Buffer, and since not finding/(reading sourcecode) documentation, I am not really comfortable using it.

It seems to me that this is a good balance since : -If you want to deploy the app, I see no reason to not want the shareUrl the same as the url the client is connected to. -If you do not want to deploy it and want to test with friends on LAN, the shareUrl will function as before, which was no problem.

Furthermore, I have put the parameter (don't mind the name, I am not very good at naming things) in the Commons.scala, however it is not really a practical place, since the user creating a webapp will have a hard time (possibly impossible) overriding this parameter. A good solution would be to put this in the ApplicationJVM (easily overridable by the user), however it involved changing a lot of code to pass the parameter to the routes, so I did not do it as I am sure there is a better solution I have not yet found..

Then in Pages.scala here is how I handle the url (same as how websocket is handled ):

      val port = dom.window.location.port
      val hostName = dom.window.location.hostname
      val tls = dom.window.location.protocol.startsWith("https") 
      val endpoint = appInfo.wsEndpoint
        .replace("{{protocol}}", if tls then "wss" else "ws")
        .replace("{{hostName}}", hostName + (if port.nonEmpty then f":$port" else ""))
        .replace("{{userId}}", URLEncoder.encode(userId, "UTF-8"))
      val shareUrl = appInfo.shareUrl
        .replace("{{protocol}}", if tls then "https" else "http")
        .replace("{{hostName}}", hostName + (if port.nonEmpty then f":$port" else ""))

As a last point, I have put that the protocol is always handled by the client. This is a little bit cosmetic, but I think that in all cases you want the shareUrl to be in the same protocol your are connected to.

Below are examples of results of such changes : (As you can see here, with the parameter put to TRUE, the shareLink functions as before. The problem with the port is still here, but easily fixable, however I did not fix it since it would involve using the cask.Request.headers, which I am not really comfortable using. (Also, I do not know if it worth fixing, because if you are not deploying, no reason to change HTTP_PORT)

Here is instead when deployed, with parameter set to FALSE (As you can see, everything seems to work as wanted)

NOTE: the code is a little bit messy, and its application is really specific; I coded it for my use, but maybe it adds unnecessary complexity for the normal user, and we should just leave "as is" for the webapp-lib.

added 1 commit

• f39177f6 - ShareUrl: 127.0.0.1 now treated as localhost

Compare with previous version

added 2 commits

• 8ef6527c - Revert "ShareUrl: Dynamically created by client for proxies"
• c3224638 - ShareUrl: can be handled by client or server.

Compare with previous version

added 1 commit

• 42772120 - Refactor: refacted shareUrl link creation.

Compare with previous version

mentioned in merge request !37 (merged)

Rebased and cleaned up slightly here (with authorship preserved, of course): !37 (merged). Could you have a look?

This is great! I like the addition of the support of the env. variables, this will make the life of next year students much easier if they want to change some parameters (also opens the road to more user configuration for future changes). I tested the code and it works as intended. Thank you very much for taking the time for fixing this issue, I hope !37 (merged) will get merged. I'll close this MR since it is not needed anymore.

Perfect, thanks a lot for your contribution and your review. Merged!

closed