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

http: Add config option to disable CSRF protection (Fixes: #1713) #1714

Merged
merged 1 commit into from Oct 15, 2018

Conversation

3 participants
@kingosticks
Member

kingosticks commented Oct 11, 2018

This will remove the requirement to set a Content-Type: application/json
header for JSON-RPC POST requests. It will also disable all same-origin
checks, effectively ignoring the allowed_origins config since requests
from any origin will be allowed. Lastly, all 'Access-Control-Allow-*
response headers will be suppressed.

I am not sold on the config name but I wanted to make it really clear that setting it is less secure. Alternate name suggestions welcome.

Also, I am not 100% sure if we should be disabling all the protections (as is currently implemented here) or if we should just remove the requirement to set a Content-Type header...

@jodal

jodal approved these changes Oct 11, 2018

I think the config name is OK. Maybe flip it around and remove the disable prefix?

.. confval:: http/disable_csrf_protection
Disable the HTTP server's protection against Cross-Site Request Forgery
(CSRF) from both JSON-RPC and Websocket requests.

This comment has been minimized.

@jodal

jodal Oct 11, 2018

Member

s/Websocket/WebSocket/

This will remove the requirement to set a ``Content-Type: application/json``
header for JSON-RPC POST requests. It will also disable all same-origin
checks, effectively ignoring the ``allowed_origins`` config since requests
from any origin will be allowed. Lastly, all ``'Access-Control-Allow-*``

This comment has been minimized.

@jodal

jodal Oct 11, 2018

Member

Extra quote mark before the header name.

@jodal jodal added this to the v2.2.1 milestone Oct 11, 2018

@kingosticks kingosticks force-pushed the kingosticks:feature/disable-http-csrf-config branch from fd9cba9 to 02f9244 Oct 11, 2018

@kingosticks

This comment has been minimized.

Member

kingosticks commented Oct 11, 2018

Also added the issue/pr numbers to the changelog.

@kingosticks kingosticks force-pushed the kingosticks:feature/disable-http-csrf-config branch from 02f9244 to 2828a43 Oct 11, 2018

@tkem

This comment has been minimized.

Member

tkem commented Oct 12, 2018

Looks good to me. Only suggestion I'd have is, if it's going to be called csrf_protection in the config (i.e. True means enforce CSRF protection), it might be preferable to also call it the same in code. Currently, there's things like

if not self.disable_csrf_protection:

and double negation always gives me headaches. But that's nit-picking, really.

@tkem

tkem approved these changes Oct 12, 2018

@@ -20,17 +20,23 @@
def make_mopidy_app_factory(apps, statics):
def mopidy_app_factory(config, core):
disable_csrf_protection = not config['http']['csrf_protection']

This comment has been minimized.

@jodal

jodal Oct 12, 2018

Member

You need to rename this variable and flip the logic here.

This comment has been minimized.

@jodal

jodal Oct 12, 2018

Member

Ok, I actually misread this and didn't notice the "not". I guess that proves my point. :-)

Disabling this will remove the requirement to set a ``Content-Type: application/json``
header for JSON-RPC POST requests. It will also disable all same-origin
checks, effectively ignoring the ``allowed_origins`` config since requests

This comment has been minimized.

@jodal

jodal Oct 12, 2018

Member
s/``allowed_origins``/:confval:`http/allowed_origins`/
@kingosticks

This comment has been minimized.

Member

kingosticks commented Oct 12, 2018

Ha well your belief in my tests says more! I'm away over the weekend but I can change it next week.

@kingosticks kingosticks force-pushed the kingosticks:feature/disable-http-csrf-config branch from 2828a43 to b7f4fd0 Oct 15, 2018

http: Add config option to control CSRF protection (Fixes: #1713)
Allows users to disable CSRF protection and revert to the HTTP server's
previous (less secure) behaviour. Users are advised to leave this config
value enabled if possible. However, if disabled this will:
  * Remove the requirement to set a ``Content-Type: application/json``
    header for JSON-RPC POST requests.
  * Disable all same-origin checks, effectively ignoring the ``allowed_origins``
    config since requests from any origin will be allowed.
  * Suppress all ``Access-Control-Allow-*`` response headers.

@kingosticks kingosticks force-pushed the kingosticks:feature/disable-http-csrf-config branch from b7f4fd0 to 10fafc0 Oct 15, 2018

@kingosticks

This comment has been minimized.

Member

kingosticks commented Oct 15, 2018

That should be all consistent now.

@jodal

jodal approved these changes Oct 15, 2018

@jodal jodal merged commit 2390f3a into mopidy:release-2.2 Oct 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment