diff --git a/openhands/app_server/app_conversation/app_conversation_service_base.py b/openhands/app_server/app_conversation/app_conversation_service_base.py index f524167524..d5d34bd109 100644 --- a/openhands/app_server/app_conversation/app_conversation_service_base.py +++ b/openhands/app_server/app_conversation/app_conversation_service_base.py @@ -4,7 +4,11 @@ import tempfile from abc import ABC from dataclasses import dataclass from pathlib import Path -from typing import AsyncGenerator +from typing import TYPE_CHECKING, AsyncGenerator +from uuid import UUID + +if TYPE_CHECKING: + import httpx import base62 @@ -29,6 +33,14 @@ from openhands.sdk.context.agent_context import AgentContext from openhands.sdk.context.condenser import LLMSummarizingCondenser from openhands.sdk.context.skills import load_user_skills from openhands.sdk.llm import LLM +from openhands.sdk.security.analyzer import SecurityAnalyzerBase +from openhands.sdk.security.confirmation_policy import ( + AlwaysConfirm, + ConfirmationPolicyBase, + ConfirmRisky, + NeverConfirm, +) +from openhands.sdk.security.llm_analyzer import LLMSecurityAnalyzer from openhands.sdk.workspace.remote.async_remote_workspace import AsyncRemoteWorkspace _logger = logging.getLogger(__name__) @@ -379,3 +391,95 @@ class AppConversationServiceBase(AppConversationService, ABC): condenser = LLMSummarizingCondenser(**condenser_kwargs) return condenser + + def _create_security_analyzer_from_string( + self, security_analyzer_str: str | None + ) -> SecurityAnalyzerBase | None: + """Convert security analyzer string from settings to SecurityAnalyzerBase instance. + + Args: + security_analyzer_str: String value from settings. Valid values: + - "llm" -> LLMSecurityAnalyzer + - "none" or None -> None + - Other values -> None (unsupported analyzers are ignored) + + Returns: + SecurityAnalyzerBase instance or None + """ + if not security_analyzer_str or security_analyzer_str.lower() == 'none': + return None + + if security_analyzer_str.lower() == 'llm': + return LLMSecurityAnalyzer() + + # For unknown values, log a warning and return None + _logger.warning( + f'Unknown security analyzer value: {security_analyzer_str}. ' + 'Supported values: "llm", "none". Defaulting to None.' + ) + return None + + def _select_confirmation_policy( + self, confirmation_mode: bool, security_analyzer: str | None + ) -> ConfirmationPolicyBase: + """Choose confirmation policy using only mode flag and analyzer string.""" + if not confirmation_mode: + return NeverConfirm() + + analyzer_kind = (security_analyzer or '').lower() + if analyzer_kind == 'llm': + return ConfirmRisky() + + return AlwaysConfirm() + + async def _set_security_analyzer_from_settings( + self, + agent_server_url: str, + session_api_key: str | None, + conversation_id: UUID, + security_analyzer_str: str | None, + httpx_client: 'httpx.AsyncClient', + ) -> None: + """Set security analyzer on conversation using only the analyzer string. + + Args: + agent_server_url: URL of the agent server + session_api_key: Session API key for authentication + conversation_id: ID of the conversation to update + security_analyzer_str: String value from settings + httpx_client: HTTP client for making API requests + """ + + if session_api_key is None: + return + + security_analyzer = self._create_security_analyzer_from_string( + security_analyzer_str + ) + + # Only make API call if we have a security analyzer to set + # (None is the default, so we can skip the call if it's None) + if security_analyzer is None: + return + + try: + # Prepare the request payload + payload = {'security_analyzer': security_analyzer.model_dump()} + + # Call agent server API to set security analyzer + response = await httpx_client.post( + f'{agent_server_url}/api/conversations/{conversation_id}/security_analyzer', + json=payload, + headers={'X-Session-API-Key': session_api_key}, + timeout=30.0, + ) + response.raise_for_status() + _logger.info( + f'Successfully set security analyzer for conversation {conversation_id}' + ) + except Exception as e: + # Log error but don't fail conversation creation + _logger.warning( + f'Failed to set security analyzer for conversation {conversation_id}: {e}', + exc_info=True, + ) diff --git a/openhands/app_server/app_conversation/live_status_app_conversation_service.py b/openhands/app_server/app_conversation/live_status_app_conversation_service.py index 4b710553ca..dd4b2dd499 100644 --- a/openhands/app_server/app_conversation/live_status_app_conversation_service.py +++ b/openhands/app_server/app_conversation/live_status_app_conversation_service.py @@ -13,7 +13,6 @@ from pydantic import Field, SecretStr, TypeAdapter from openhands.agent_server.models import ( ConversationInfo, - NeverConfirm, SendMessageRequest, StartConversationRequest, ) @@ -72,7 +71,6 @@ from openhands.integrations.provider import ProviderType from openhands.sdk import Agent, AgentContext, LocalWorkspace from openhands.sdk.llm import LLM from openhands.sdk.secret import LookupSecret, StaticSecret -from openhands.sdk.security.confirmation_policy import AlwaysConfirm from openhands.sdk.workspace.remote.async_remote_workspace import AsyncRemoteWorkspace from openhands.server.types import AppMode from openhands.tools.preset.default import ( @@ -308,6 +306,16 @@ class LiveStatusAppConversationService(AppConversationServiceBase): ) ) + # Set security analyzer from settings + user = await self.user_context.get_user_info() + await self._set_security_analyzer_from_settings( + agent_server_url, + sandbox.session_api_key, + info.id, + user.security_analyzer, + self.httpx_client, + ) + # Update the start task task.status = AppConversationStartTaskStatus.READY task.app_conversation_id = info.id @@ -749,8 +757,8 @@ class LiveStatusAppConversationService(AppConversationServiceBase): conversation_id=conversation_id, agent=agent, workspace=workspace, - confirmation_policy=( - AlwaysConfirm() if user.confirmation_mode else NeverConfirm() + confirmation_policy=self._select_confirmation_policy( + bool(user.confirmation_mode), user.security_analyzer ), initial_message=initial_message, secrets=secrets, diff --git a/tests/unit/app_server/test_app_conversation_service_base.py b/tests/unit/app_server/test_app_conversation_service_base.py index a179a11c24..356c454fcf 100644 --- a/tests/unit/app_server/test_app_conversation_service_base.py +++ b/tests/unit/app_server/test_app_conversation_service_base.py @@ -1,11 +1,13 @@ -"""Unit tests for git functionality in AppConversationServiceBase. +"""Unit tests for git and security functionality in AppConversationServiceBase. This module tests the git-related functionality, specifically the clone_or_init_git_repo method and the recent bug fixes for git checkout operations. """ import subprocess +from types import MethodType from unittest.mock import AsyncMock, MagicMock, Mock, patch +from uuid import uuid4 import pytest @@ -434,13 +436,298 @@ def test_create_condenser_plan_agent_with_custom_max_size(mock_condenser_class): mock_llm.model_copy.assert_called_once() +# ============================================================================= +# Tests for security analyzer helpers +# ============================================================================= + + +@pytest.mark.parametrize('value', [None, '', 'none', 'NoNe']) +def test_create_security_analyzer_returns_none_for_empty_values(value): + """_create_security_analyzer_from_string returns None for empty/none values.""" + # Arrange + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), bind_methods=('_create_security_analyzer_from_string',) + ) + + # Act + result = service._create_security_analyzer_from_string(value) + + # Assert + assert result is None + + +def test_create_security_analyzer_returns_llm_analyzer(): + """_create_security_analyzer_from_string returns LLMSecurityAnalyzer for llm string.""" + # Arrange + security_analyzer_str = 'llm' + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), bind_methods=('_create_security_analyzer_from_string',) + ) + + # Act + result = service._create_security_analyzer_from_string(security_analyzer_str) + + # Assert + from openhands.sdk.security.llm_analyzer import LLMSecurityAnalyzer + + assert isinstance(result, LLMSecurityAnalyzer) + + +def test_create_security_analyzer_logs_warning_for_unknown_value(): + """_create_security_analyzer_from_string logs warning and returns None for unknown.""" + # Arrange + unknown_value = 'custom' + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), bind_methods=('_create_security_analyzer_from_string',) + ) + + # Act + with patch( + 'openhands.app_server.app_conversation.app_conversation_service_base._logger' + ) as mock_logger: + result = service._create_security_analyzer_from_string(unknown_value) + + # Assert + assert result is None + mock_logger.warning.assert_called_once() + + +def test_select_confirmation_policy_when_disabled_returns_never_confirm(): + """_select_confirmation_policy returns NeverConfirm when confirmation_mode is False.""" + # Arrange + confirmation_mode = False + security_analyzer = 'llm' + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), bind_methods=('_select_confirmation_policy',) + ) + + # Act + policy = service._select_confirmation_policy(confirmation_mode, security_analyzer) + + # Assert + from openhands.sdk.security.confirmation_policy import NeverConfirm + + assert isinstance(policy, NeverConfirm) + + +def test_select_confirmation_policy_llm_returns_confirm_risky(): + """_select_confirmation_policy uses ConfirmRisky when analyzer is llm.""" + # Arrange + confirmation_mode = True + security_analyzer = 'llm' + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), bind_methods=('_select_confirmation_policy',) + ) + + # Act + policy = service._select_confirmation_policy(confirmation_mode, security_analyzer) + + # Assert + from openhands.sdk.security.confirmation_policy import ConfirmRisky + + assert isinstance(policy, ConfirmRisky) + + +@pytest.mark.parametrize('security_analyzer', [None, '', 'none', 'custom']) +def test_select_confirmation_policy_non_llm_returns_always_confirm( + security_analyzer, +): + """_select_confirmation_policy falls back to AlwaysConfirm for non-llm values.""" + # Arrange + confirmation_mode = True + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), bind_methods=('_select_confirmation_policy',) + ) + + # Act + policy = service._select_confirmation_policy(confirmation_mode, security_analyzer) + + # Assert + from openhands.sdk.security.confirmation_policy import AlwaysConfirm + + assert isinstance(policy, AlwaysConfirm) + + +@pytest.mark.asyncio +async def test_set_security_analyzer_skips_when_no_session_key(): + """_set_security_analyzer_from_settings exits early without session_api_key.""" + # Arrange + agent_server_url = 'https://agent.example.com' + conversation_id = uuid4() + httpx_client = AsyncMock() + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), + bind_methods=( + '_create_security_analyzer_from_string', + '_set_security_analyzer_from_settings', + ), + ) + + with patch.object(service, '_create_security_analyzer_from_string') as mock_create: + # Act + await service._set_security_analyzer_from_settings( + agent_server_url=agent_server_url, + session_api_key=None, + conversation_id=conversation_id, + security_analyzer_str='llm', + httpx_client=httpx_client, + ) + + # Assert + mock_create.assert_not_called() + httpx_client.post.assert_not_called() + + +@pytest.mark.asyncio +async def test_set_security_analyzer_skips_when_analyzer_none(): + """_set_security_analyzer_from_settings skips API call when analyzer resolves to None.""" + # Arrange + agent_server_url = 'https://agent.example.com' + session_api_key = 'session-key' + conversation_id = uuid4() + httpx_client = AsyncMock() + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), + bind_methods=( + '_create_security_analyzer_from_string', + '_set_security_analyzer_from_settings', + ), + ) + + with patch.object( + service, '_create_security_analyzer_from_string', return_value=None + ) as mock_create: + # Act + await service._set_security_analyzer_from_settings( + agent_server_url=agent_server_url, + session_api_key=session_api_key, + conversation_id=conversation_id, + security_analyzer_str='none', + httpx_client=httpx_client, + ) + + # Assert + mock_create.assert_called_once_with('none') + httpx_client.post.assert_not_called() + + +class DummyAnalyzer: + """Simple analyzer stub for testing model_dump contract.""" + + def __init__(self, payload: dict): + self._payload = payload + + def model_dump(self) -> dict: + return self._payload + + +@pytest.mark.asyncio +async def test_set_security_analyzer_successfully_calls_agent_server(): + """_set_security_analyzer_from_settings posts analyzer payload when available.""" + # Arrange + agent_server_url = 'https://agent.example.com' + session_api_key = 'session-key' + conversation_id = uuid4() + analyzer_payload = {'type': 'llm'} + httpx_client = AsyncMock() + http_response = MagicMock() + http_response.raise_for_status = MagicMock() + httpx_client.post.return_value = http_response + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), + bind_methods=( + '_create_security_analyzer_from_string', + '_set_security_analyzer_from_settings', + ), + ) + + analyzer = DummyAnalyzer(analyzer_payload) + + with ( + patch.object( + service, + '_create_security_analyzer_from_string', + return_value=analyzer, + ) as mock_create, + patch( + 'openhands.app_server.app_conversation.app_conversation_service_base._logger' + ) as mock_logger, + ): + # Act + await service._set_security_analyzer_from_settings( + agent_server_url=agent_server_url, + session_api_key=session_api_key, + conversation_id=conversation_id, + security_analyzer_str='llm', + httpx_client=httpx_client, + ) + + # Assert + mock_create.assert_called_once_with('llm') + httpx_client.post.assert_awaited_once_with( + f'{agent_server_url}/api/conversations/{conversation_id}/security_analyzer', + json={'security_analyzer': analyzer_payload}, + headers={'X-Session-API-Key': session_api_key}, + timeout=30.0, + ) + http_response.raise_for_status.assert_called_once() + mock_logger.info.assert_called() + + +@pytest.mark.asyncio +async def test_set_security_analyzer_logs_warning_on_failure(): + """_set_security_analyzer_from_settings warns but does not raise on errors.""" + # Arrange + agent_server_url = 'https://agent.example.com' + session_api_key = 'session-key' + conversation_id = uuid4() + analyzer_payload = {'type': 'llm'} + httpx_client = AsyncMock() + httpx_client.post.side_effect = RuntimeError('network down') + service, _ = _create_service_with_mock_user_context( + MockUserInfo(), + bind_methods=( + '_create_security_analyzer_from_string', + '_set_security_analyzer_from_settings', + ), + ) + + analyzer = DummyAnalyzer(analyzer_payload) + + with ( + patch.object( + service, + '_create_security_analyzer_from_string', + return_value=analyzer, + ) as mock_create, + patch( + 'openhands.app_server.app_conversation.app_conversation_service_base._logger' + ) as mock_logger, + ): + # Act + await service._set_security_analyzer_from_settings( + agent_server_url=agent_server_url, + session_api_key=session_api_key, + conversation_id=conversation_id, + security_analyzer_str='llm', + httpx_client=httpx_client, + ) + + # Assert + mock_create.assert_called_once_with('llm') + httpx_client.post.assert_awaited_once() + mock_logger.warning.assert_called() + + # ============================================================================= # Tests for _configure_git_user_settings # ============================================================================= -def _create_service_with_mock_user_context(user_info: MockUserInfo) -> tuple: - """Create a mock service with the actual _configure_git_user_settings method. +def _create_service_with_mock_user_context( + user_info: MockUserInfo, bind_methods: tuple[str, ...] | None = None +) -> tuple: + """Create a mock service with selected real methods bound for testing. Uses MagicMock for the service but binds the real method for testing. @@ -452,13 +739,16 @@ def _create_service_with_mock_user_context(user_info: MockUserInfo) -> tuple: # Create a simple mock service and set required attribute service = MagicMock() service.user_context = mock_user_context + methods_to_bind = ['_configure_git_user_settings'] + if bind_methods: + methods_to_bind.extend(bind_methods) + # Remove potential duplicates while keeping order + methods_to_bind = list(dict.fromkeys(methods_to_bind)) - # Bind the actual method from the real class to test real implementation - service._configure_git_user_settings = ( - lambda workspace: AppConversationServiceBase._configure_git_user_settings( - service, workspace - ) - ) + # Bind actual methods from the real class to test implementations directly + for method_name in methods_to_bind: + real_method = getattr(AppConversationServiceBase, method_name) + setattr(service, method_name, MethodType(real_method, service)) return service, mock_user_context diff --git a/tests/unit/experiments/test_experiment_manager.py b/tests/unit/experiments/test_experiment_manager.py index 85faa078f5..c389423cf5 100644 --- a/tests/unit/experiments/test_experiment_manager.py +++ b/tests/unit/experiments/test_experiment_manager.py @@ -153,6 +153,7 @@ class TestExperimentManagerIntegration: llm_api_key=None, confirmation_mode=False, condenser_max_size=None, + security_analyzer=None, ) async def get_secrets(self):