lib/api: Check basic auth (and set session cookie) before noauth exce…

…ptions (syncthing#9159)

This is motivated by the Android app:
syncthing/syncthing-android#1982 (comment)

The planned fix in response to basic auth behaviour changing in syncthing#8757
was to add the `Authorization` header when opening the WebView, but it
turns out the function used only applies the header to the initial page
load, not any subsequent script loads or AJAX calls. The
`basicAuthAndSessionMiddleware` checks for no-auth exceptions before
checking the `Authorization` header, so the header has no effect on the
initial page load since the `/` path is a no-auth exception. Thus the
Android app fails to log in when opening the WebView.

This changes the order of checks in `basicAuthAndSessionMiddleware` so
that the `Authorization` header is always checked if present, and a
session cookie is set if it is valid. Only after that does the
middleware fall back to checking for no-auth exceptions.

`api_test.go` has been expanded with additional checks:
- Check that a session cookie is set whenever correct basic auth is
provided.
- Check that a session cookie is not set when basic auth is incorrect.
- Check that a session cookie is not set when authenticating with an API
token (either via `X-Api-Key` or `Authorization: Bearer`).

And an additional test case:
- Check that requests to `/` always succeed, but receive a session
cookie when correct basic auth is provided.

I have manually verified that
- The new assertions fail if the `createSession` call is removed in
`basicAuthAndSessionMiddleware`.
- The new test cases in e6e4df4 fail
before the change in 0e47d37 is
applied.

lib/api/api_auth.go

@@ -43,13 +43,11 @@ func antiBruteForceSleep() {
 }
 
 func unauthorized(w http.ResponseWriter) {
-	antiBruteForceSleep()
 	w.Header().Set("WWW-Authenticate", "Basic realm=\"Authorization Required\"")
 	http.Error(w, "Not Authorized", http.StatusUnauthorized)
 }
 
 func forbidden(w http.ResponseWriter) {
-	antiBruteForceSleep()
 	http.Error(w, "Forbidden", http.StatusForbidden)
 }
 
@@ -87,12 +85,6 @@ func basicAuthAndSessionMiddleware(cookieName string, guiCfg config.GUIConfigura
 			return
 		}
 
-		// Exception for static assets and REST calls that don't require authentication.
-		if isNoAuthPath(r.URL.Path) {
-			next.ServeHTTP(w, r)
-			return
-		}
-
 		cookie, err := r.Cookie(cookieName)
 		if err == nil && cookie != nil {
 			sessionsMut.Lock()
@@ -111,6 +103,12 @@ func basicAuthAndSessionMiddleware(cookieName string, guiCfg config.GUIConfigura
 			return
 		}
 
+		// Exception for static assets and REST calls that don't require authentication.
+		if isNoAuthPath(r.URL.Path) {
+			next.ServeHTTP(w, r)
+			return
+		}
+
 		// Some browsers don't send the Authorization request header unless prompted by a 401 response.
 		// This enables https://user:pass@localhost style URLs to keep working.
 		if guiCfg.SendBasicAuthPrompt {
@@ -141,6 +139,7 @@ func passwordAuthHandler(cookieName string, guiCfg config.GUIConfiguration, ldap
 		}
 
 		emitLoginAttempt(false, req.Username, r.RemoteAddr, evLogger)
+		antiBruteForceSleep()
 		forbidden(w)
 	})
 }
@@ -164,6 +163,7 @@ func attemptBasicAuth(r *http.Request, guiCfg config.GUIConfiguration, ldapCfg c
 	}
 
 	emitLoginAttempt(false, username, r.RemoteAddr, evLogger)
+	antiBruteForceSleep()
 	return "", false
 }
 

lib/api/api_test.go

@@ -649,6 +649,9 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedFailStatus {
 					t.Errorf("Unexpected non-%d return code %d for unauthed request", expectedFailStatus, resp.StatusCode)
 				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for unauthed request")
+				}
 			})
 
 			t.Run("incorrect password is rejected", func(t *testing.T) {
@@ -657,6 +660,9 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedFailStatus {
 					t.Errorf("Unexpected non-%d return code %d for incorrect password", expectedFailStatus, resp.StatusCode)
 				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for incorrect password")
+				}
 			})
 
 			t.Run("incorrect username is rejected", func(t *testing.T) {
@@ -665,6 +671,9 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedFailStatus {
 					t.Errorf("Unexpected non-%d return code %d for incorrect username", expectedFailStatus, resp.StatusCode)
 				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for incorrect username")
+				}
 			})
 
 			t.Run("UTF-8 auth works", func(t *testing.T) {
@@ -673,6 +682,9 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedOkStatus {
 					t.Errorf("Unexpected non-%d return code %d for authed request (UTF-8)", expectedOkStatus, resp.StatusCode)
 				}
+				if !hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Expected session cookie for authed request (UTF-8)")
+				}
 			})
 
 			t.Run("ISO-8859-1 auth works", func(t *testing.T) {
@@ -681,6 +693,9 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedOkStatus {
 					t.Errorf("Unexpected non-%d return code %d for authed request (ISO-8859-1)", expectedOkStatus, resp.StatusCode)
 				}
+				if !hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Expected session cookie for authed request (ISO-8859-1)")
+				}
 			})
 
 			t.Run("bad X-API-Key is rejected", func(t *testing.T) {
@@ -689,6 +704,9 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedFailStatus {
 					t.Errorf("Unexpected non-%d return code %d for bad API key", expectedFailStatus, resp.StatusCode)
 				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for bad API key")
+				}
 			})
 
 			t.Run("good X-API-Key is accepted", func(t *testing.T) {
@@ -697,29 +715,40 @@ func TestHTTPLogin(t *testing.T) {
 				if resp.StatusCode != expectedOkStatus {
 					t.Errorf("Unexpected non-%d return code %d for API key", expectedOkStatus, resp.StatusCode)
 				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for API key")
+				}
 			})
 
 			t.Run("bad Bearer is rejected", func(t *testing.T) {
 				t.Parallel()
 				resp := httpGetAuthorizationBearer(url, testAPIKey+"X")
 				if resp.StatusCode != expectedFailStatus {
-					t.Errorf("Unexpected non-%d return code %d for bad API key", expectedFailStatus, resp.StatusCode)
+					t.Errorf("Unexpected non-%d return code %d for bad Authorization: Bearer", expectedFailStatus, resp.StatusCode)
+				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for bad Authorization: Bearer")
 				}
 			})
 
 			t.Run("good Bearer is accepted", func(t *testing.T) {
 				t.Parallel()
 				resp := httpGetAuthorizationBearer(url, testAPIKey)
 				if resp.StatusCode != expectedOkStatus {
-					t.Errorf("Unexpected non-%d return code %d for API key", expectedOkStatus, resp.StatusCode)
+					t.Errorf("Unexpected non-%d return code %d for Authorization: Bearer", expectedOkStatus, resp.StatusCode)
+				}
+				if hasSessionCookie(resp.Cookies()) {
+					t.Errorf("Unexpected session cookie for bad Authorization: Bearer")
 				}
 			})
 		})
 	}
 
+	testWith(true, http.StatusOK, http.StatusOK, "/")
 	testWith(true, http.StatusOK, http.StatusUnauthorized, "/meta.js")
 	testWith(true, http.StatusNotFound, http.StatusUnauthorized, "/any-path/that/does/nooooooot/match-any/noauth-pattern")
 
+	testWith(false, http.StatusOK, http.StatusOK, "/")
 	testWith(false, http.StatusOK, http.StatusForbidden, "/meta.js")
 	testWith(false, http.StatusNotFound, http.StatusForbidden, "/any-path/that/does/nooooooot/match-any/noauth-pattern")
 }