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

Fix https and add wss support #201

Merged
merged 21 commits into from
Nov 22, 2023
Merged

Fix https and add wss support #201

merged 21 commits into from
Nov 22, 2023

Conversation

OmniTroid
Copy link
Contributor

This changes the way the serverlist works quite fundamentally. Notable points:

  • All logic related to connecting to servers from index.html has been removed
  • Renamed ip query param to connect, and make it a full connection string with protocol, host and port
  • Hack a subdomain change when joining a ws-only server
  • Asset links hopefully still work with https

The benefits is that the server list is more respectful of the user's privacy (no connection/giving away your IP on hovering) and it works with https (no more annoying warnings).

The drawbacks is that it requires configuring an "insecure" domain and that the client can't know if the server is reachable from the serverlist.

A proof of concept is live at https://web.troid.tech/ , it might require additional testing.

@stonedDiscord
Copy link
Member

did you test it?
i will before i merge it

@OmniTroid
Copy link
Contributor Author

Making this work properly depends on some very specific behavior from the server, that is:

If the request is http, but contains a wss: in the connect argument, redirect to https (this might not even necessary because browsers love bumping the request to https regardless, but should be added for completeness)
If the request is https, but contains a ws: in the connect argument, redirect to http

Especially the second case is hard/impossible from the browser point of view (js shouldn't be able to downgrade to http for security reasons). I don't see how this can be solved without additional configuration on the server side. The current implementation with two different subdomains is very stupid. I have a better idea for this implementation, so just hang on.

Here's what the nginx config for the required behavior looks like:

server {
    listen 80;
    server_name web.aceattorneyonline.com;

    location / {
        if ($arg_connect ~* (wss:)) {
            return 302 https://$host$request_uri;
        }
        root /var/www;
        index index.html;
    }
}

server {
    listen 443 ssl;
    server_name web.aceattorneyonline.com;

    location / {
        if ($arg_connect ~* (ws:)) {
            return 302 http://$host$request_uri;
        }
        root /var/www;
        index index.html;
    }
}

I am using 302 because I'm not 100% certain how different browsers may cache 301 redirects based on the combination of protocol, domain, port and query arguments. That makes it a bit safer.

So note that this change will require more testing and infrastructure changes before merging.

@OmniTroid
Copy link
Contributor Author

OmniTroid commented Nov 20, 2023

I have successfully tested this nginx config on https://webao.troid.tech

Again, this is required on the server side in order for this to be consistent and functional on all browsers

@OmniTroid OmniTroid marked this pull request as ready for review November 22, 2023 17:54
@stonedDiscord
Copy link
Member

can you please add the target="_blank" from the troid.tech version

@OmniTroid
Copy link
Contributor Author

The target=_blank in join and watch buttons should be present in the latest version.

@OmniTroid OmniTroid mentioned this pull request Nov 22, 2023
@stonedDiscord stonedDiscord merged commit 26e3cd4 into AttorneyOnline:master Nov 22, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants