Something went wrong on our end. Please try again.
Created by: slimsag
Prior to this change, we protected against CSRF attacks two ways:
After this change, we rely solely on browser's CORS policies(1) to prevent against CSRF attacks.
The problem with using CSRF tokens is that it increases the code complexity and mental model complexity of our CSRF threat model, making it harder to reason about. For a relatively clear picture of this complexity, see the changes to doc/dev/security/csrf_security_model.md
.
Additionally, our CSRF token implementation was not good. In fact, it was very bad:
window.context
, that if we had relied on them anyway we would have been subject to caching vulnerabilities in which one user would get another user's CSRF token due to GET requests being cached.In short, our CSRF token implementation has always been very bad, we do not rely on it today, we have not relied on it in the past 4+ years, it has verifiably caused real security incidents for customers and Sourcegraph.com in the past, and it increases the hurdles to engineers attaining a solid understanding of our CSRF threat model.
DID is a reasonable strategy. I would argue that reducing security complexity so that engineers can reason about our threat model is more important than DID, and so removal is the right move.
One could argue having both CSRF tokens and strong CORS policies is the best, most ideal state to be in. I agree with that, but I will also explicitly call out that our implementation today is absolutely not it: If we do want DID, we need to re-visit the entire implementation of CSRF tokens from scratch and not rely on the garbage, ill-thought-out code we have here.
This also finally closes the issue I filed back in 2020, https://github.com/sourcegraph/sourcegraph/issues/7658, in which I recommended the same thing. This was also recommended by myself and Quinn as early as 2018 (see issue for more historical context.)
Without further ado: let's finally get rid of these!