security: make session cookie authentication stricter (#27313)

* security: make session cookie authentication stricter

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.)

* security: update CSRF threat model to reflect session auth changes

Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>

cmd/frontend/internal/session/session.go

@@ -300,24 +300,15 @@ func CookieMiddleware(db dbutil.DB, next http.Handler) http.Handler {
 }
 
 // CookieMiddlewareWithCSRFSafety is a middleware that authenticates HTTP requests using the
-// provided cookie (if any), *only if* the request is a non-simple CORS request (see
-// https://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0). This relies on the client's
-// CORS checks to guarantee that one of the following is true, thereby protecting against CSRF
-// attacks:
+// provided cookie (if any), *only if* one of the following is true.
 //
-// - The request originates from the same origin. -OR-
+// - The request originates from a trusted origin (the same origin, browser extension origin, or one
+//   in the site configuration corsOrigin allow list.)
+// - The request has the special X-Requested-With header present, which is only possible to send in
+//   browsers if the request passed the CORS preflight request (see the handleCORSRequest function.)
 //
-// - The request is cross-origin but passed the CORS preflight check (because otherwise the
-//   preflight OPTIONS response from secureHeadersMiddleware would have caused the browser to refuse
-//   to send this HTTP request).
-//
-// To determine if it's a non-simple CORS request, it checks for the presence of either
-// "Content-Type: application/json; charset=utf-8" or a non-empty HTTP request header whose name is
-// given in corsAllowHeader.
-//
-// If the request is a simple CORS request, or if neither of these is true, then the cookie is not
-// used to authenticate the request. The request is still allowed to proceed (but will be
-// unauthenticated unless some other authentication is provided, such as an access token).
+// If one of the above are not true, the request is still allowed to proceed but will be
+// unauthenticated unless some other authentication is provided, such as an access token.
 func CookieMiddlewareWithCSRFSafety(db dbutil.DB, next http.Handler, corsAllowHeader string, isTrustedOrigin func(*http.Request) bool) http.Handler {
 	corsAllowHeader = textproto.CanonicalMIMEHeaderKey(corsAllowHeader)
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -330,20 +321,6 @@ func CookieMiddlewareWithCSRFSafety(db dbutil.DB, next http.Handler, corsAllowHe
 			// Did the request come from a trusted origin? If so, it's trusted.
 			isTrusted = isTrustedOrigin(r)
 		}
-		if !isTrusted {
-			// The request doesn't have the X-Requested-With header.
-			// The request didn't come from a trusted origin.
-			// Did the request pass the CORS preflight? If so, it's trusted.
-			//
-			// Any origin with the ability to specify "Content-Type: application/json; charset=utf-8"
-			// would have passed CORS preflight.
-			//
-			// We allow this because it means you do not need to specify `X-Requested-With` in
-			// requests from an origin in the site config `corsOrigin` allow list, which is slightly
-			// nicer for API consumers.
-			contentType := r.Header.Get("Content-Type")
-			isTrusted = contentType == "application/json" || contentType == "application/json; charset=utf-8"
-		}
 		if isTrusted {
 			r = r.WithContext(authenticateByCookie(db, r, w))
 		}

doc/dev/security/csrf_security_model.md

@@ -195,9 +195,9 @@ The only mutable, privileged actions that do not go through Sourcegraph's `/.api
 
 Sourcegraph's API endpoints offer multiple forms of authentication for different use-cases:
 
-1. Session cookies, via the [`session.CookieMiddlewareWithCSRFSafety`](https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/sourcegraph/sourcegraph%24%40aefef0d+CookieMiddlewareWithCSRFSafety%28&patternType=literal) middleware. This allows session cookie authentication iff one of the following is true AND the request is not [a "simple" CORS request](https://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0) (`Content-Type` header is `application/json`, or the `X-Requested-With` header is present, implying that a CORS preflight check must have preceded the request successfully):
-   1. The request originates from the same origin.
-   2. OR the request is cross-origin, but passed the CORS preflight check (it is an allowed origin according to the site config `corsOrigins` setting)
+1. Session cookies, via the [`session.CookieMiddlewareWithCSRFSafety`](https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/sourcegraph/sourcegraph%24%40aefef0d+CookieMiddlewareWithCSRFSafety%28&patternType=literal) middleware. This allows session cookie authentication iff one of the following is true:
+   1. The request originates from a trusted origin (same origin, browser extension, or an origin in the site config `corsOrigin` allow list.)
+   2. The `X-Requested-With` header is present, which is only possible to send in a browser if the CORS preflight check preceded the request successfully. ([see the cors standard for details](https://fetch.spec.whatwg.org/#http-access-control-allow-headers).)
 2. Authentication tokens, created in the Sourcegraph UI (also via the API) - checked through the [`AccessTokenAuthMiddleware`](https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/sourcegraph/sourcegraph%24%40aefef0d+AccessTokenAuthMiddleware%28&patternType=literal) and specified by either:
    1. The basic auth `username` field.
    2. The `Authorization` header, in either `Authorization: token <token>` or `Authorization: token-sudo ...` form with a user to impersonate in the header value somewhere.