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

Mechanism for skipping CSRF checks on API posts #835

Closed
simonw opened this issue Jun 11, 2020 · 13 comments
Closed

Mechanism for skipping CSRF checks on API posts #835

simonw opened this issue Jun 11, 2020 · 13 comments

Comments

@simonw
Copy link
Owner

simonw commented Jun 11, 2020

While experimenting with https://github.com/simonw/datasette-auth-tokens I realized it's not currently possible to build API client programs that POST to Datasette because there's no mechanism for them to skip the CSRF checks added in #798.

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

Tweeted about this here: https://twitter.com/simonw/status/1273655053170077701

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

I think there are a couple of steps to this one.

The nature of CSRF is that it's about hijacking existing authentication credentials. If your Datasette site runs without any authentication plugins at all CSRF protection isn't actually useful.

Some POST endpoints should be able to opt-out of CSRF protection entirely. A writable canned query that accepts anonymous poll submissions for example might determine that CSRF is not needed.

If a plugin adds Authorization: Bearer xxx token support that plugin should also be able to specify that CSRF protection can be skipped. https://github.com/simonw/datasette-auth-tokens could do this.

This means I need two new mechanisms:

  • A way for wrapped views to indicate "actually don't CSRF protect me". I'm not sure how feasible this is without a major redesign, since the decision to return a 403 forbidden status is made before the wrapped function has even been called.
  • A way for authentication plugins like datasette-auth-tokens to say "CSRF protection is not needed for this request". This is a bit tricky too, since right now the actor_from_request hook doesn't have a channel for information other than returning the actor dictionary.

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

Here's the Rails pattern for this: https://gist.github.com/maxivak/a25957942b6c21a41acd

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

The only way I can think of for a view to opt-out of CSRF protection is for them to be able to reconfigure the asgi-csrf middleware to skip specific URL patterns.

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

datasette-auth-tokens could switch to using asgi_wrapper instead of actor_from_request - then it could add a scope["skip_csrf"] = True scope property to indicate that CSRF should not be protected.

Since asgi_wrapper wraps the CSRF protection middleware changes made to the scope by an asgi_wrapper will be visible to the CSRF middleware:

datasette/datasette/app.py

Lines 877 to 888 in d2aef9f

asgi = AsgiLifespan(
AsgiTracer(
asgi_csrf.asgi_csrf(
DatasetteRouter(self, routes),
signing_secret=self._secret,
cookie_name="ds_csrftoken",
)
),
on_startup=setup_db,
)
for wrapper in pm.hook.asgi_wrapper(datasette=self):
asgi = wrapper(asgi)

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

I wonder if it's safe to generically say "Don't do CSRF protection on any request that includes a Authorization: Bearer... header - because it's not possible for a regular browser to send that header since the format is different from the header used in browser-based HTTP basic auth?

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

if you did Origin based CSRF checks, then could the absence of an Origin header be used?
https://twitter.com/cnorthwood/status/1273674392757829632

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

Idea: a mechanism where the asgi_csrf() can take an optional should_protect() callback function which gets called with the scope and decides if the current request should be protected or not. It can then look at headers and paths and suchlike and make its own decisions. Datasette could then provide a should_protect() callback which can interact with plugins.

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

So maybe one really easy fix here is to disable CSRF checks entirely for any request that doesn't have any cookies? Also suggested here: https://twitter.com/mrkurt/status/1273682965168603137

@simonw
Copy link
Owner Author

simonw commented Jun 18, 2020

Problem there is Login CSRF attacks: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#login-csrf - I still want to perform CSRF checks on login forms, even though the user may not yet have any cookies.

Maybe I can turn off CSRF checks for cookie-free requests but allow login forms to specifically opt back in to CSRF protection?

@simonw
Copy link
Owner Author

simonw commented Jun 27, 2020

Skipping CSRF on Authorization: Bearer xxx headers also makes sense for JWT applications, which tend to send JWTs using that form of header.

@simonw
Copy link
Owner Author

simonw commented Jul 1, 2020

I'm going to add some tests for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant