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

Use Tornado default WebSocket check_origin function #125

Merged
merged 3 commits into from
Jul 20, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Feb 26, 2020

These changes close #109

When I implemented the WebSocket handler, I used an existing example from Tornado docs, which had the check_origin function. As I didn't know how to use it well, I've disabled this check so that it would cause less issues for development. Time to revisit it and refine/review.

The function can be removed from the SubscriptionHandler. This will keep the parent WebSocketHandler's check_origin default behavior:

  • If the client connecting doesn't provide the Origin HTTP header, then this function is ignored
  • But if the client does provide it (e.g. most web browsers do) then this function is used
  • check_origin will return True meaning that the request passed the test, and False otherwise
  • The test is simple, it parses the URL from the Origin HTTP header, removes protocol, fragments, path, etc, leaving only the domain part. Then compares against the value of the HTTP header Host. If the same, then the function will return True. Otherwise, it means the connection is coming from another host (e.g. someone could be trying to connect to the WebSocket from mydashboard.somesite.com).

The risk without this, is that CORS/XORS attacks would be possible. Where someone could set up a site somewhere else and try to send malicious messages down the stream to the server. We have other authentication mechanisms in place to prevent that, but it's still recommended to enable this (both by OWASP and by the Tornado docs).

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • I have opened a documentation PR at Adding initial docs for websockets security cylc-doc#116

@kinow kinow added this to the 0.3 milestone Feb 26, 2020
@kinow kinow self-assigned this Feb 26, 2020
@kinow
Copy link
Member Author

kinow commented Feb 26, 2020

Tested locally, and it worked with no issues. Can't think of a deployment scenario right now where we would have issues, but if that happens, we should be able to find a workaround. Better than to leave it with no security protection to only add after something bad happens.

Set to 0.3, will mark as ready for review after the 0.2 release 👍

@codecov-io
Copy link

Codecov Report

Merging #125 into master will decrease coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #125      +/-   ##
=========================================
- Coverage   52.56%   52.3%   -0.26%     
=========================================
  Files           6       6              
  Lines         371     369       -2     
  Branches       58      58              
=========================================
- Hits          195     193       -2     
  Misses        173     173              
  Partials        3       3
Impacted Files Coverage Δ
cylc/uiserver/handlers.py 77.33% <ø> (-0.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d35ff4...f73e54d. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Feb 26, 2020

Added docs to the existing PR to cylc-doc for WebSocket security, replacing a TODO marker I had left there previously.

@kinow kinow marked this pull request as ready for review July 9, 2020 22:33
@kinow kinow force-pushed the prevent-cors-same-origin branch from f73e54d to a2ff234 Compare July 9, 2020 22:37
@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2020

Codecov Report

Merging #125 into master will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage   51.00%   50.75%   -0.25%     
==========================================
  Files           6        6              
  Lines         398      396       -2     
  Branches       64       64              
==========================================
- Hits          203      201       -2     
  Misses        192      192              
  Partials        3        3              
Impacted Files Coverage Δ
cylc/uiserver/handlers.py 77.10% <ø> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65e545...f15cc72. Read the comment docs.

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Tested and found working 🥊

@hjoliver hjoliver merged commit a4acfc5 into cylc:master Jul 20, 2020
@kinow kinow deleted the prevent-cors-same-origin branch July 20, 2020 00:15
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.

Implement same origin check for WebSockets
5 participants