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.

Sign up for GitHub

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
merged 1 commit into from
Jan 4, 2018
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/compiler/crystal/tools/playground/public/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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...

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.


this.ws.onopen = function() {
this._triggerReady();
Expand Down