Skip to content

security: make session cookie authentication stricter

Warren Gifford requested to merge sg/stricter-cookie-allowance into main

Created by: slimsag

Prior to this change we would allow session cookie authentication for so-called non-simple requests, even if the request was not from a trusted origin and did not pass the CORS preflight request. i.e. if the following headers were present:

  • Content-Type: application/json
  • Content-Type: application/json; charset=utf-8

This was safe to do because it is only possible to specify these content types in a browser if the request was sent via JavaScript (and not e.g. a <form> POST request.) and so the presence of these headers implied that the CORS preflight request must have succeeded earlier.

Why did we have this logic in the first place? I believe it's because we didn't always know if the request came from the Sourcegraph instance itself. If externalURL is not configured in the site configuration, then it would not be possible to navigate to the site configuration and update it (the GraphQL request would not be authenticated!)

But in recent years, we introduced X-Requested-With: Sourcegraph as an option to flag a request as "definitely having passed the CORS preflight" and we send this with all requests from our JavaScript frontend to our backend. This made this code useless, it has no effect today, we just never removed it.

  • If the request is from a third-party client (src-cli, curl, etc.) it will be using an access token, not session auth, and so this is irrelevant.
  • If the request came from the Sourcegraph frontend itself OR the browser extension, X-Requested-With is present and so this code does not run.
  • If the request came from another website, it must be in the corsOrigin allow list and therefor this code would not run.

In fact, having this code around is risky: if we ever screwed up our CORS handling logic, this code would effectively allow an untrusted origin to utilize cookie/session-based auth which is a classic CSRF vulnerability. Luckily, our CORS logic has been solid and so this was never in practice a vulnerability.

Additionally, this logic must NOT be present if we want to allow public access to our GraphQL API in the future (and we do), as doing so means allowing authenticated cross-origin requests from any domain but NOT allowing cookie auth unless the domain is trusted (which this logic violates today.)

Signed-off-by: Stephen Gutekanst [email protected]

Merge request reports