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

fixed issue #21 #22

Closed
wants to merge 4 commits into from
Closed

fixed issue #21 #22

wants to merge 4 commits into from

Conversation

t0k4rt
Copy link

@t0k4rt t0k4rt commented Nov 15, 2020

I've setup snapcast at home and I'm using it in a docker environment using traefik https reverse proxy.
With default setup the interface cannot lod because of mixed content (https with unsecure web socket)
This PR should solve the problem.

Ii tested snapcontrol in my setup but I haven't tested snapstream yet.

  • use wss when scheme is https
  • test snapcontrol
  • test snapstream

This should fix #21

@badaix
Copy link
Owner

badaix commented Nov 16, 2020

Thanks, how does this PR compare to #20?
Can you please apply the PR to the develop branch?

@patagonaa
Copy link
Contributor

patagonaa commented Nov 16, 2020

Switching to WSS when calling the page via HTTPS is already possible by changing baseUrl in config.ts to (window.location.protocol === 'https:' ? 'wss://' : 'ws://') + window.location.host, I just didn't do this change in my PR #20 because it would break current behaviour (if running snapweb on the same server but under a different port than snapcast).

I think the change in config.ts would be the cleanest solution to this problem (as #22 introduces the same breaking change and as of now, breaks snapstream), I guess it's @badaix' choice if introducing that breaking change would be acceptable.

@t0k4rt
Copy link
Author

t0k4rt commented Nov 17, 2020

Sorry I haven't checked the existing PR before doing this one :-/

I'll rebase using develop branch.

#20 fix this problem but I just feel like it's not convenient to setup the UI like this.

First, IMHO you don't want to setup a websocket url, you'd prefer to setup the service url (eg: http://mysnapcast.local). Secondly, I don't see a lot of case where you would want to setup a seperate websocket url from the web url. And in this case you would probably run into cors issues which would be annoying.

So I think your service should provide have a standalone web ui that could work without any external configuration and this should be feasable.

For example we can infer websocket parameters like this:

class SnapWebConfig {
    constructor(snapServerUrl?:string, debug?:boolean) {
        if (snapServerUrl) {
            this.snapServerUrl = new URL(snapServerUrl);
            let snapServerWSUrl = new URL(snapServerUrl);
            
            // we only change protocol
            // this should allow compatibility with other case such as
            // snapcast web ui hosted like this http://mynas/snapweb
            if (snapServerWSUrl.protocol == "https:") {
                snapServerWSUrl.protocol = "wss:"
            } else {
                snapServerWSUrl.protocol = "ws:"
            }
            this.snapServerWSUrl = snapServerWSUrl
            
            // maybe later we would want to enable debug mode ?
            if (debug) {
                this.debug = debug
            }
        }
    }

    snapServerUrl: URL = new URL("http://localhost:1780")
    snapServerWSUrl: URL = new URL("ws://localhost:1780")
    debug: boolean = false
}

// snapcontrol
class SnapControl {
    constructor(snapconfig: SnapWebConfig) {
        this.connection = new WebSocket(snapconfig.snapServerWSUrl.toString())
    }
}

// snapstream
class SnapStream {
    constructor(snapconfig: SnapWebConfig) {
        this.connection = new WebSocket(snapconfig.snapServerWSUrl.toString())
    }
}

// we init snapcontrol at startup using current webpage location
window.onload = function (event: any) {
    let snapconfig = new SnapWebConfig(window.location.href)
    snapcontrol = new SnapControl(snapconfig);
}

With somehting like this we wouldn't need any extra config.js

@patagonaa
Copy link
Contributor

My changes that introduce config.ts/js are already in develop and master. The only thing thing required to get the behaviour you're proposing is to change baseUrl to (window.location.protocol === 'https:' ? 'wss://' : 'ws://') + window.location.host in config.ts so the host and port are inferred from the URL (which I didn't do initially because I didn't want to change existing behaviour). This works in 99% of cases (snapweb hosted via port 1780 of snapserver + an optional reverse proxy).

The 1% of edge cases (namely, hosting snapweb under a different path, port or server than snapserver, which might be useful for snapweb development) can be handled by editing config.js manually.

Also keep in mind that CORS policies do not apply to websockets, so it is possible to run snapweb on server1 and point the websocket to server2 without any issues (I doubt anyone does that outside of development though, to be honest).

I think changing config.ts to infer the URL by default is a cleaner solution than a SnapWebConfig that isn't actually configurable. In 99% of cases my proposed fix Just Works™ and even the remaining 1% can be handled by manually editing a single seperate file, which has the advantage that snapweb can still be upgraded without breaking anything.

@t0k4rt
Copy link
Author

t0k4rt commented Nov 17, 2020

As you wish

@t0k4rt t0k4rt closed this Nov 17, 2020
@badaix
Copy link
Owner

badaix commented Nov 22, 2020

boths solutions sound good to me, thanks anyway :)

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.

Doesn't work when behind a HTTPS reverse proxy
3 participants