-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added support for secure web sockets when protocol is HTTPS #5527
Added support for secure web sockets when protocol is HTTPS #5527
Conversation
Though the compiler does not ship with any openssl support at this time, the playground may be served behind a proxy like Nginx which handles the TLS handshake. If the playground is served over HTTPS in this way, modern browsers will raise a SecurityError when the playground front-end attempts to open an insecure web socket (i.e.: ws://<domain>/client). This disables a key feature of the playground which is dynamically compiling and executing code provided by the front-end. With this change, we can select the secure/insecure web socket protocol based on the secure/insecure HTTP protocol being used. This is determined using `location.protocol`[0]. This change allows the playground to effectively be served over HTTPS (at least up to the proxy). A use case for this includes developing Crystal workbooks to be accessible to multiple users securely over a given network. Given an interactive class or presentation about Crystal, the presenter may host the playground server supplied with their own unique workbook examples while users interact securely with the examples provided by the presenter. This is in reference to issue 5519[1]. [0]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLHyperlinkElementUtils/protocol [1]: crystal-lang#5519
@@ -297,7 +297,8 @@ Playground.Session = function(options) { | |||
this.editor._playgroundSession = this; | |||
|
|||
this.connect = function() { | |||
this.ws = new WebSocket("ws://" + location.host + "/client"); | |||
var socketProtocol = location.protocol === "https:" ? "wss:" : "ws:"; | |||
this.ws = new WebSocket(socketProtocol + "//" + location.host + "/client"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also take location.port
into account when constructing the web socket URI, which would then support serving a playground with a non-default port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain it is necessary to consult location.port
. location.host
appears to include the port which is why the socket connection is successful when running crystal play
defaulting to port 8080. Have I missed a different scenario/use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I confused location.host
with location.hostname
...please ignore me...
@@ -297,7 +297,8 @@ Playground.Session = function(options) { | |||
this.editor._playgroundSession = this; | |||
|
|||
this.connect = function() { | |||
this.ws = new WebSocket("ws://" + location.host + "/client"); | |||
var socketProtocol = location.protocol === "https:" ? "wss:" : "ws:"; | |||
this.ws = new WebSocket(socketProtocol + "//" + location.host + "/client"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we tried just using "//host/client" with no scheme? At least on HTTP that ends up using the current protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should use protocol-relative URLs. It's considered an antipattern and shouldn't be advocated. This is easy to solve with JavaScript, let's just do it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @straight-shoota in the anti-pattern case. Nonetheless for the record, neither MDN's WebSocket nor the WebSocket spec specify a scheme state override passed to the URL parser which seems to result in a no scheme state -> relative state where the base URL's scheme will be used. If I interpret that correctly, that suggests the fallback will be HTTP or HTTPS in this case which is not exactly what we want even though the default ports for those are the same as WS and WSS respectively.
Though the compiler does not ship with any openssl support at this time, the playground may be served behind a proxy like Nginx which handles the TLS handshake. If the playground is served over HTTPS in this way, modern browsers will raise a SecurityError when the playground front-end attempts to open an insecure web socket (i.e.: ws:///client). This disables a key feature of the playground which is dynamically compiling and executing code provided by the front-end.
With this change, we can select the secure/insecure web socket protocol based on the secure/insecure HTTP protocol being used. This is determined using
location.protocol
. This change allows the playground to effectively be served over HTTPS (at least up to the proxy).A use case for this includes developing Crystal workbooks to be accessible to multiple users securely over a given network. Given an interactive class or presentation about Crystal, the presenter may host the playground server supplied with their own unique workbook examples while users interact securely with the examples provided by the presenter.
This is in reference to issue #5519.