From ea1ea366d26219a88b0016821698cca827f7f9d1 Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Tue, 10 Oct 2023 07:48:35 +0200 Subject: [PATCH] lib/api: Check basic auth (and set session cookie) before noauth exceptions (#9159) This is motivated by the Android app: https://github.com/syncthing/syncthing-android/pull/1982#issuecomment-1752042554 The planned fix in response to basic auth behaviour changing in #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 e6e4df4d7034302b729ada6d91cff6e2b29678da fail before the change in 0e47d37e738d4c15736c496e01cd949afb372e71 is applied. --- lib/api/api_auth.go | 16 ++++++++-------- lib/api/api_test.go | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/lib/api/api_auth.go b/lib/api/api_auth.go index ee3bb88e6..1dfaeee0f 100644 --- a/lib/api/api_auth.go +++ b/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 } diff --git a/lib/api/api_test.go b/lib/api/api_test.go index c85b86ddf..72c2ff381 100644 --- a/lib/api/api_test.go +++ b/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,13 +715,19 @@ 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") } }) @@ -711,15 +735,20 @@ func TestHTTPLogin(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") }