jech / galene

The Galène videoconference server
https://galene.org
MIT License
975 stars 132 forks source link

Support for paths in proxyURL #212

Closed vanrein closed 3 months ago

vanrein commented 3 months ago
vanrein commented 3 months ago

This is in response to #209 and I think you will like how little had to be done.

I have seen this work behind a proxy at / as well as at /internetwide/chat/galene/. I have tested video exchanges using IPv4 and TURN at :1194.

I am confident about the <base href="/"> approach, the idempotent sed -i command added to INSTALL but tests cannot cover everything, so it would be useful if you could double check if I left any links behind or overdid something. I aimed to be thorough, of course, but the eyes of the original author are unbeatable.

Thanks for Galène, it is a wondefully modest yet capable tool!

jech commented 3 months ago

This patch makes me extremely nervous. I'm really not sure if it is correct, and it would be a lot of work to test it. Without automated tests, I don't feel comfortable including it.

Remember that the files protocol.js and management.js are meant to be general-purpose libraries, they can be invoked at any level of the hierarchy. I'm really not sure if the changes you make there are correct.

How did you test your changes?

vanrein commented 3 months ago

Sorry to hear of your anxiety, in spite of having worked so hard to keep it simple. The thing is, it is a necessary feature for some installations.

All that I've done in the end is changing the reference point from which the three changed HTML files point to the other statics. The combined <base href="/"> with paths made relative to the current basedir logically imply that it must end up downloading the rest from the same places as before. More importantly, there are no relations to the complexities of WebRTC, TURN and so on, AFAIK.

I am accustomed to automated testing, with temporarily launched daemons and all, using a Python tool but you are probably thinking of browser simulations, and that is not a practical approach to automate, and not in style with Galène, I think. It would however be quite possible to extract all sorts of paths using commandline tools. And we are talking about reachability of the web files, right?

The change in galene.js is that the former implicit reference to the current document location has shifted to an explicit current document location. I;d say that the implied assumption that the current document holds no <base href...> element is more fragile than bypassing it.

The changes in management.js are more profound, though consistently made as far as I could oversee, so any error should be obvious (and not at all likely to be noticed in the / case, but rather show up as a bug after a prefix path).

I tested by comparing the behaviour both with and without a prefix> The setup used Nginx attached to a public IPv4 address, with two locations that I tested both,

      location / {
              proxy_pass http://123.45.67.89:8443/;
              location /ws {
                      proxy_pass http://123.45.67.89:8443/ws;
                      proxy_set_header Upgrade $http_upgrade;
                      proxy_set_header Connection "Upgrade";
              }
      }

        location /internetwide/chat/galene/ {
                proxy_pass http://123.45.67.89:8443/;
                location /internetwide/chat/galene/ws {
                        proxy_pass http://123.45.67.89:8443/ws;
                        proxy_set_header Upgrade $http_upgrade;
                        proxy_set_header Connection "Upgrade";
                }
        }

I ran Galène in the back with

galene -insecure -static modified.static/ -data data/ -groups groups/ -turn auto

with data/config.json set to

{
        "users":{"root": {"password":"xxx", "permissions": "admin"}},
        "publicServer": true,
        "proxyURL": "https://123.45.67.89/internetwide/chat/galene/"
}

(with appropriate changes for the / mounted case) and a few groups to play with.

I initially found a problem unknown up stream and/or unknown down stream in the JavaScript console but that went away after switching IPv4 and setting up TURN properly. (My original testing was on an IPv6-only host, rather a stretch I suppose.)

Is there anything I can do to calm your anxiety?

jech commented 3 months ago

I'm genuinely sorry, but you're asking me to maintain code that I don't feel comfortable with.

I think that your patch is smart, too smart for my liking. By abusing the base element in the way you do, you're requiring that every future contributor be aware of the smartness; for example, I do not know whether it now becomes impossible to use a #foo link within a page that uses base.

I trust your word that your patch is correct, however I am unfortunately not able to maintain it. Please feel free to maintain a fork of Galene.

jech commented 2 months ago

@vanrein Thanks for the friendly words in https://github.com/arpa2/galene/commit/6ab32c45e4645aa58818b69a88b393995cdfdf2b