Merge pull request #2434 from dgageot/fix7

fix: reject OAuth callback when expected state has not been set (CSRF)

Author: dgageot

pkg/tools/mcp/oauth_server.go

@@ -116,12 +116,14 @@ func (cs *CallbackServer) handleCallback(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	// Verify state parameter for CSRF protection
+	// Verify state parameter for CSRF protection.
+	// Reject the callback if the expected state has not been set yet
+	// (i.e. the callback arrived before SetExpectedState was called).
 	cs.mu.Lock()
 	expectedState := cs.expectedState
 	cs.mu.Unlock()
 
-	if expectedState != "" && state != expectedState {
+	if expectedState == "" || state != expectedState {
 		cs.errCh <- fmt.Errorf("state mismatch: expected %s, got %s", expectedState, state)
 		w.WriteHeader(http.StatusBadRequest)
 		fmt.Fprint(w, "Invalid state parameter")

pkg/tools/mcp/oauth_test.go

@@ -263,3 +263,27 @@ func TestRefreshAccessToken_SendsEmptyClientIDWhenNotStored(t *testing.T) {
 		t.Error("client_secret should not be sent when empty")
 	}
 }
+
+// TestCallbackServer_RejectsCallbackBeforeStateSet verifies that a callback
+// arriving before SetExpectedState is called is rejected (CSRF protection).
+func TestCallbackServer_RejectsCallbackBeforeStateSet(t *testing.T) {
+	cs, err := NewCallbackServer()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := cs.Start(); err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = cs.Shutdown(t.Context()) }()
+
+	// Send a callback before SetExpectedState has been called.
+	resp, err := http.Get(cs.GetRedirectURI() + "?code=authcode&state=anything")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusBadRequest {
+		t.Errorf("expected 400, got %d — callback accepted without expected state set", resp.StatusCode)
+	}
+}