slimsag
commented
3 years ago Following up on this here: https://github.com/sourcegraph/sourcegraph/pull/23949
Open slimsag opened 4 years ago
slimsag
commented
3 years ago Following up on this here: https://github.com/sourcegraph/sourcegraph/pull/23949
Important: This issue does not describe a security vulnerability, it just describes how our CSRF cookies are redundant and not actually needed in order to protect ourselves against CSRF attacks today, and that we should remove them. The problem here is one of technical debt, not a potential vulnerability in Sourcegraph. The implementor of this proposal will need to have a comprehensive understanding of our CSRF protection and overall security model, and as with any security change it should be well-reviewed and tested to eliminate any doubts.
There exists a hard-coded CSRF protection value in our code which has come up and worried multiple people in the past:
The truth, though, is that this is not a security vulnerability. It looks worrying - definitely - but has been proven on multiple occasions to not be a problem:
In addition to the worry caused, our CSRF cookies have been a great source of complexity, difficulty for customers, and a time sink for us (example: https://sourcegraph.slack.com/archives/CJX299FGE/p1572635962235900 ).
The truth is that our CSRF cookies are a secondary means of protecting against CSRF attacks and nothing prevents us from removing them and remaining secure against CSRF. If you want to understand what the primary means is (CORS), see my writeup on this issue in 2018: https://github.com/sourcegraph/sourcegraph/issues/227#issuecomment-426482380
What we should do here: Do what Quinn proposed back in 2018 so we can be done with this mess once and for all https://github.com/sourcegraph/sourcegraph/pull/472#issuecomment-431715542