Implement anti-CSRF tokens or require custom headers for non-safe requests
Nobody has claimed this yet.
- Dominant language
- Clojure
- Stars
- 49.3k
- Forks
- 6.8k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 653
Description
- Our anti-CSRF story is pretty good with
SameSite=Lax cookies, but not perfect.- As OWASP says:
It is important to note that this attribute should be implemented as an additional layer defense in depth concept. [...] This attribute should not replace having a CSRF Token. Instead, it should co-exist with that token in order to protect the user in a more robust way.
- Login is an example of a place we’re not fully covered -- it would be possible to log in to Metabse from a 3rd-party site via CSRF, since the cookies only protect authenticated requests, rather than all POST requests. Someone theoretically might be able to use this to capture sensitive credentials -- see https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#login-csrf
- As OWASP says:
- Thus, we should implement another anti-CSRF strategy to have maximum protection. Here are the basics of a few ideas we could implement:
- Traditional Synchronizer Pattern Anti-CSRF Tokens
- https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#synchronizer-token-pattern
- The basic idea here is to create a token tied to a specific Session and then pass it along to the frontend when we initialize the client/when the user logs in. The FE then submits that token as a header or hidden form param on any non-safe request, and we check it to confirm that its the correct value
- We’re already doing this in some cases in the EE code, so it’s a simple matter of doing it everywhere
- This would make API usage a little annoying outside of the FE client -- consider implementing OAuth support or another alternative auth mechanism to make API usage convenient
- Require a custom request header on all non-safe requests
- https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#use-of-custom-request-headers
- The basic idea here is we require all non-safe requests to have some header such as X-Metabase-Request: true or we reject them.
- From OWASP:
This defense relies on the same-origin policy (SOP) restriction that only JavaScript can be used to add a custom header, and only within its origin. By default, browsers do not allow JavaScript to make cross origin requests with custom headers.
- We could of course combine this with the anti-CSRF token proposal and require the anti-CSRF token to be passed as a
X-Metabase-Anti-CSRF-Tokenheader or something like that. I think we’re already doing this for the places we’re using it? - We’d have to carve out an exception for the small handful of places where we are actually using submit actions, such as query results downloads. But we’d still be able to better protect the other 99% of our REST API endpoints
- This is sort of like the time when I tried to require
Content-Type: application/jsonheaders for all API requests, I still think we should do this, but carve out exceptions for the results download endpoints that are triggered by<form>submit actions- https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html#validate-request-content-types
- Original PR: https://github.com/metabase/metabase/pull/9547. Reverted in https://github.com/metabase/metabase/pull/9559 because it broke results downloads. We should re-enable but exempt results downloads
- Traditional Synchronizer Pattern Anti-CSRF Tokens
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
The issue names no files, tests, or entry points; begin by tracing the login and REST API request paths, then review the existing EE anti-CSRF handling. Compare the synchronizer-token and custom-header approaches, including form-based results downloads and the reverted PRs 9547 and 9559; done requires a decided strategy with appropriate exceptions.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- clojure
- Domain
- api, authentication, security
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100