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: [Emscripten] Force secure WebSockets over HTTPS #9248

Merged
merged 1 commit into from May 13, 2021

Conversation

embeddedt
Copy link
Contributor

Motivation / Problem

Emscripten uses insecure WebSockets for all connections by default. This doesn't work on HTTPS as browsers force secure WebSockets to be used (and block any insecure connection from even being opened).

Description

This PR forces the use of the wss:// protocol when OpenTTD is loaded over an HTTPS connection.

Limitations

None.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@James103
Copy link
Contributor

This PR forces the use of the wss:// protocol when OpenTTD is loaded over an HTTPS connection.

Why not just force the use of the wss:// protocol regardless of whether the underlying website is served over HTTP or HTTPS?

@embeddedt
Copy link
Contributor Author

embeddedt commented May 11, 2021

There is a valid case where someone might want ws://, for instance, if they're testing locally and don't want to go through the hassle of setting up a self-signed SSL certificate.

Also, I was trying to preserve backwards-compatibility as much as possible. Since insecure WebSockets never have worked over HTTPS, there is nothing to break by changing them there.

(Though I don't know of any OpenTTD servers using Emscripten for anything serious yet...)

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Currently this part is up to who-ever runs emscripten, so having this or not doesn't really make a difference. But I like it suggests that you should be using https, so sure, why not.

os/emscripten/pre.js Show resolved Hide resolved
os/emscripten/pre.js Outdated Show resolved Hide resolved
os/emscripten/pre.js Show resolved Hide resolved
@TrueBrain TrueBrain merged commit 86741ad into OpenTTD:master May 13, 2021
@TrueBrain TrueBrain added the backport requested This PR should be backport to current release (RC / stable) label May 13, 2021
@TrueBrain TrueBrain removed the backport requested This PR should be backport to current release (RC / stable) label Oct 3, 2021
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.

None yet

3 participants