cmd/syncthing: Enforce stricter CSRF policy on /rest GET requests (fixes #3134)

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3137

cmd/syncthing/gui.go

@@ -461,10 +461,6 @@ func corsMiddleware(next http.Handler) http.Handler {
 	//
 	// See https://www.w3.org/TR/cors/ for details.
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// Add a generous access-control-allow-origin header since we may be
-		// redirecting REST requests over protocols
-		w.Header().Add("Access-Control-Allow-Origin", "*")
-
 		// Process OPTIONS requests
 		if r.Method == "OPTIONS" {
 			// Only GET/POST Methods are supported

cmd/syncthing/gui_csrf.go

@@ -41,7 +41,8 @@ func csrfMiddleware(unique string, prefix string, cfg config.GUIConfiguration, n
 			return
 		}
 
-		// Allow requests for the front page, and set a CSRF cookie if there isn't already a valid one.
+		// Allow requests for anything not under the protected path prefix,
+		// and set a CSRF cookie if there isn't already a valid one.
 		if !strings.HasPrefix(r.URL.Path, prefix) {
 			cookie, err := r.Cookie("CSRF-Token-" + unique)
 			if err != nil || !validCsrfToken(cookie.Value) {
@@ -56,18 +57,6 @@ func csrfMiddleware(unique string, prefix string, cfg config.GUIConfiguration, n
 			return
 		}
 
-		if r.Method == "GET" {
-			// Allow GET requests unconditionally, but if we got the CSRF
-			// token cookie do the verification anyway so we keep the
-			// csrfTokens list sorted by recent usage. We don't care about the
-			// outcome of the validity check.
-			if cookie, err := r.Cookie("CSRF-Token-" + unique); err == nil {
-				validCsrfToken(cookie.Value)
-			}
-			next.ServeHTTP(w, r)
-			return
-		}
-
 		// Verify the CSRF token
 		token := r.Header.Get("X-CSRF-Token-" + unique)
 		if !validCsrfToken(token) {

cmd/syncthing/gui_test.go

@@ -189,7 +189,9 @@ type httpTestCase struct {
 }
 
 func TestAPIServiceRequests(t *testing.T) {
+	const testAPIKey = "foobarbaz"
 	cfg := new(mockedConfig)
+	cfg.gui.APIKey = testAPIKey
 	baseURL, err := startHTTP(cfg)
 	if err != nil {
 		t.Fatal(err)
@@ -344,13 +346,13 @@ func TestAPIServiceRequests(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Log("Testing", tc.URL, "...")
-		testHTTPRequest(t, baseURL, tc)
+		testHTTPRequest(t, baseURL, tc, testAPIKey)
 	}
 }
 
 // testHTTPRequest tries the given test case, comparing the result code,
 // content type, and result prefix.
-func testHTTPRequest(t *testing.T, baseURL string, tc httpTestCase) {
+func testHTTPRequest(t *testing.T, baseURL string, tc httpTestCase, apikey string) {
 	timeout := time.Second
 	if tc.Timeout > 0 {
 		timeout = tc.Timeout
@@ -359,7 +361,14 @@ func testHTTPRequest(t *testing.T, baseURL string, tc httpTestCase) {
 		Timeout: timeout,
 	}
 
-	resp, err := cli.Get(baseURL + tc.URL)
+	req, err := http.NewRequest("GET", baseURL+tc.URL, nil)
+	if err != nil {
+		t.Errorf("Unexpected error requesting %s: %v", tc.URL, err)
+		return
+	}
+	req.Header.Set("X-API-Key", apikey)
+
+	resp, err := cli.Do(req)
 	if err != nil {
 		t.Errorf("Unexpected error requesting %s: %v", tc.URL, err)
 		return
@@ -400,7 +409,7 @@ func TestHTTPLogin(t *testing.T) {
 
 	// Verify rejection when not using authorization
 
-	req, _ := http.NewRequest("GET", baseURL+"/rest/system/status", nil)
+	req, _ := http.NewRequest("GET", baseURL, nil)
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		t.Fatal(err)
@@ -491,3 +500,73 @@ func startHTTP(cfg *mockedConfig) (string, error) {
 
 	return baseURL, nil
 }
+
+func TestCSRFRequired(t *testing.T) {
+	const testAPIKey = "foobarbaz"
+	cfg := new(mockedConfig)
+	cfg.gui.APIKey = testAPIKey
+	baseURL, err := startHTTP(cfg)
+
+	cli := &http.Client{
+		Timeout: time.Second,
+	}
+
+	// Getting the base URL (i.e. "/") should succeed.
+
+	resp, err := cli.Get(baseURL)
+	if err != nil {
+		t.Fatal("Unexpected error from getting base URL:", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatal("Getting base URL should succeed, not", resp.Status)
+	}
+
+	// Find the returned CSRF token for future use
+
+	var csrfTokenName, csrfTokenValue string
+	for _, cookie := range resp.Cookies() {
+		if strings.HasPrefix(cookie.Name, "CSRF-Token") {
+			csrfTokenName = cookie.Name
+			csrfTokenValue = cookie.Value
+			break
+		}
+	}
+
+	// Calling on /rest without a token should fail
+
+	resp, err = cli.Get(baseURL + "/rest/system/config")
+	if err != nil {
+		t.Fatal("Unexpected error from getting /rest/system/config:", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusForbidden {
+		t.Fatal("Getting /rest/system/config without CSRF token should fail, not", resp.Status)
+	}
+
+	// Calling on /rest with a token should succeed
+
+	req, _ := http.NewRequest("GET", baseURL+"/rest/system/config", nil)
+	req.Header.Set("X-"+csrfTokenName, csrfTokenValue)
+	resp, err = cli.Do(req)
+	if err != nil {
+		t.Fatal("Unexpected error from getting /rest/system/config:", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatal("Getting /rest/system/config with CSRF token should succeed, not", resp.Status)
+	}
+
+	// Calling on /rest with the API key should succeed
+
+	req, _ = http.NewRequest("GET", baseURL+"/rest/system/config", nil)
+	req.Header.Set("X-API-Key", testAPIKey)
+	resp, err = cli.Do(req)
+	if err != nil {
+		t.Fatal("Unexpected error from getting /rest/system/config:", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatal("Getting /rest/system/config with API key should succeed, not", resp.Status)
+	}
+}