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

Added support for secure web sockets when protocol is HTTPS #5527

Merged

Conversation

opiation
Copy link
Contributor

@opiation opiation commented Jan 4, 2018

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.

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");
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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");
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Contributor Author

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.

@RX14 RX14 added this to the Next milestone Jan 4, 2018
@RX14 RX14 merged commit 09f9650 into crystal-lang:master Jan 4, 2018
@opiation opiation deleted the playground-wss-front-end-support branch January 4, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants