server: enforce Pro-only for actual LLM settings changes; add robust tests mirroring frontend full-payload behavior

- Precise LLM change detection to avoid blocking language-only saves
- SaaS: call subscription validation only when LLM fields truly change
- Tests updated to patch correct target and add regression for full-payload save

Co-authored-by: openhands <openhands@all-hands.dev>

openhands/server/settings_validation.py

@@ -1,5 +1,7 @@
 """Settings validation utilities for LLM settings access control."""
 
+from pydantic import SecretStr
+
 from openhands.core.logger import openhands_logger as logger
 from openhands.server.shared import server_config
 from openhands.server.types import AppMode
@@ -50,14 +52,41 @@ def check_llm_settings_changes(settings: Settings, existing_settings) -> bool:
     Returns:
         bool: True if any LLM settings are being changed, False otherwise
     """
-    # Core LLM settings - always validate if provided
-    core_llm_changes = any(
-        [
-            settings.llm_model is not None,
-            settings.llm_api_key is not None,
-            settings.llm_base_url is not None,
-        ]
+
+    # Core LLM settings - validate only if actually changing compared to existing
+    # Handle SecretStr safely by comparing raw values if both present
+    def _secret_equals(a: SecretStr | None, b: SecretStr | None) -> bool:
+        if a is None and b is None:
+            return True
+        if a is None or b is None:
+            return False
+        try:
+            return a.get_secret_value() == b.get_secret_value()
+        except Exception:
+            # If any issue, fall back to object equality (best effort)
+            return a == b
+
+    existing_model = (
+        getattr(existing_settings, 'llm_model', None) if existing_settings else None
     )
+    existing_base_url = (
+        getattr(existing_settings, 'llm_base_url', None) if existing_settings else None
+    )
+    existing_api_key = (
+        getattr(existing_settings, 'llm_api_key', None) if existing_settings else None
+    )
+
+    core_llm_changes = False
+    if settings.llm_model is not None:
+        core_llm_changes = core_llm_changes or (settings.llm_model != existing_model)
+    if settings.llm_base_url is not None:
+        core_llm_changes = core_llm_changes or (
+            settings.llm_base_url != existing_base_url
+        )
+    if settings.llm_api_key is not None:
+        core_llm_changes = core_llm_changes or (
+            not _secret_equals(settings.llm_api_key, existing_api_key)
+        )
 
     if core_llm_changes:
         return True

tests/conftest.py

@@ -0,0 +1,9 @@
+# Ensure the repository source tree is imported before any pre-installed site packages
+# This avoids picking up a preinstalled OpenHands copy under /openhands/code during tests
+import sys
+from pathlib import Path
+
+# tests/ -> repo root
+REPO_ROOT = str(Path(__file__).resolve().parent.parent)
+if REPO_ROOT not in sys.path:
+    sys.path.insert(0, REPO_ROOT)

tests/unit/server/routes/test_settings_security.py

@@ -106,7 +106,6 @@ def test_client_non_pro():
             'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
             AsyncMock(return_value=FileSettingsStore(InMemoryFileStore())),
         ),
-        # Mock the validation function at the routes level to return False (no access)
         patch(
             'openhands.server.routes.settings.validate_llm_settings_access',
             AsyncMock(return_value=False),
@@ -133,7 +132,6 @@ def test_client_pro():
             'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
             AsyncMock(return_value=FileSettingsStore(InMemoryFileStore())),
         ),
-        # Mock the validation function at the routes level to return True (has access)
         patch(
             'openhands.server.routes.settings.validate_llm_settings_access',
             AsyncMock(return_value=True),
@@ -273,7 +271,7 @@ async def test_expired_subscription_cannot_access_llm_settings():
         ),
         # Mock validation to return False (expired subscription, no access)
         patch(
-            'openhands.server.routes.settings.validate_llm_settings_access',
+            'openhands.server.settings_validation.validate_llm_settings_access',
             AsyncMock(return_value=False),
         ),
     ):
@@ -327,3 +325,79 @@ async def test_model_prefix_bypass_attempts_blocked(
     assert_forbidden_response(
         response, f"Bypass attempt with model '{malicious_model}'"
     )
+
+
+@pytest.mark.asyncio
+async def test_non_pro_user_can_save_language_only_with_full_payload():
+    """Regression test: Non-pro users changing only non-LLM settings should not be blocked
+    even when the frontend sends the full settings payload including unchanged LLM fields.
+    """
+    # Shared in-memory settings store used across pro and non-pro clients
+    shared_store = FileSettingsStore(InMemoryFileStore())
+
+    # Step 1: Initialize settings as a pro user (simulates existing saved settings)
+    with (
+        patch.dict(
+            os.environ, {'SESSION_API_KEY': '', 'APP_MODE': 'saas'}, clear=False
+        ),
+        patch('openhands.server.dependencies._SESSION_API_KEY', None),
+        patch('openhands.server.shared.server_config.app_mode', AppMode.OSS),
+        patch(
+            'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
+            AsyncMock(return_value=shared_store),
+        ),
+        patch(
+            'openhands.server.user_auth.user_auth.UserAuth.get_instance',
+            return_value=MockUserAuthPro(),
+        ),
+    ):
+        pro_client = TestClient(app)
+        initial_settings = {
+            'language': 'en',
+            'agent': 'CodeActAgent',
+            'max_iterations': 100,
+            # LLM fields as the frontend would send
+            'llm_model': 'openhands/claude-sonnet-4-20250514',
+            'llm_base_url': '',
+            'confirmation_mode': False,
+            'security_analyzer': 'llm',
+            'enable_default_condenser': True,
+            'condenser_max_size': 120,
+            'remote_runtime_resource_factor': 1,
+            # Frontend typically does NOT resend llm_api_key unless changed
+        }
+        resp = pro_client.post('/api/settings', json=initial_settings)
+        assert resp.status_code == 200
+
+    # Step 2: As a non-pro user, change only language but send full payload
+    with (
+        patch.dict(
+            os.environ, {'SESSION_API_KEY': '', 'APP_MODE': 'saas'}, clear=False
+        ),
+        patch('openhands.server.dependencies._SESSION_API_KEY', None),
+        patch('openhands.server.shared.server_config.app_mode', AppMode.SAAS),
+        patch(
+            'openhands.storage.settings.file_settings_store.FileSettingsStore.get_instance',
+            AsyncMock(return_value=shared_store),
+        ),
+        patch(
+            'openhands.server.user_auth.user_auth.UserAuth.get_instance',
+            return_value=MockUserAuthNonPro(),
+        ),
+    ):
+        nonpro_client = TestClient(app)
+        # Same payload as initial, except language changed
+        payload = {
+            'language': 'fr',
+            'agent': 'CodeActAgent',
+            'max_iterations': 100,
+            'llm_model': 'openhands/claude-sonnet-4-20250514',
+            'llm_base_url': '',
+            'confirmation_mode': False,
+            'security_analyzer': 'llm',
+            'enable_default_condenser': True,
+            'condenser_max_size': 120,
+            'remote_runtime_resource_factor': 1,
+        }
+        response = nonpro_client.post('/api/settings', json=payload)
+        assert response.status_code == 200, response.text