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

Protect RPC interface against CSRF #1668

Merged
merged 8 commits into from Apr 15, 2018
Merged

Conversation

kingosticks
Copy link
Member

A fix for some of #1659

@kingosticks
Copy link
Member Author

I will take a stab at the websocket too

@kingosticks kingosticks force-pushed the fix/cors branch 2 times, most recently from 77ea763 to 94d3069 Compare April 11, 2018 01:02
@jodal jodal added this to the v2.2 milestone Apr 11, 2018
@jodal jodal added the A-http Area: HTTP frontend label Apr 11, 2018
@@ -4,3 +4,4 @@ hostname = 127.0.0.1
port = 6680
static_dir =
zeroconf = Mopidy HTTP server on $hostname
allowed_origins =
Copy link
Member

Choose a reason for hiding this comment

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

Does localhost:6600 work without whitelisting it, or should we make that the default?

.. confval:: http/allowed_origins

A whitelist of domains allowed to perform Cross-Origin Resource Sharing
(CORS) requests. Entries must be in the format ``hostname``:``port``.
Copy link
Member

Choose a reason for hiding this comment

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

  • Consider s/whitelist/list/ and explain that multiple values can be separated by comma or put on separate lines. I don't remember how well we explain how to use config.List config values elsewhere.
  • Include the colon in the monospace part: hostname:port.

@@ -19,12 +20,16 @@

def make_mopidy_app_factory(apps, statics):
def mopidy_app_factory(config, core):
origin_whitelist = {
Copy link
Member

Choose a reason for hiding this comment

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

s/origin_whitelist/allowed_origins/ to keep the naming consistent?

if origin is None:
logger.debug('Origin was not set')
return False
origin_whitelist.add(request_headers.get('Host', None))
Copy link
Member

Choose a reason for hiding this comment

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

This allows access for a webpage at foo:80 if the RPC interface also is available at foo:80? Cute. That voids my comment about localhost:6600 as the default config value.

@@ -177,6 +197,18 @@ def set_extra_headers(self):
self.set_header('Accept', 'application/json')
self.set_header('Content-Type', 'application/json; utf-8')

def options(self):
origin = self.request.headers.get('Origin', None)
Copy link
Member

Choose a reason for hiding this comment

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

None is the default return value for .get(), so you can leave it out.

@kingosticks
Copy link
Member Author

So I think I have addressed those comments.

Now for the websocket, I thought we could just use the same function:

    def check_origin(self, origin):
        return check_origin(origin, self.request.headers, self.allowed_origins)

Copy link
Member

@jodal jodal left a comment

Choose a reason for hiding this comment

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

Looking good! Waiting for a final review from @adamcik before merging.

By now enforcing the Content-Type header is set to 'application/json', we force browsers attempting a cross-domain
request to first perform a CORS preflight OPTIONS request. This request always includes an Origin header which we
check against our whitelist. The whitelist contains the current Host as well as anything specified in the new optional
allowed_origins config value. Any non-browser tools must also now set the Context-type header.
Also Fixed up formatting following code review.
@kingosticks
Copy link
Member Author

Fixed conflict and added a bit about same-origin requests to the docs and a changelog entry.

@jodal jodal merged commit 53c8159 into mopidy:develop Apr 15, 2018
@jodal
Copy link
Member

jodal commented Apr 15, 2018

Thanks for taking the time to fix this properly :-)

kingosticks added a commit that referenced this pull request Oct 8, 2018
Must set 'Content-Type: application/json' header due to #1668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http Area: HTTP frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants