From 9e0fee189036e197d721ad359fd12bd02a1b1d09 Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Thu, 27 Feb 2025 14:38:51 +0100 Subject: [PATCH] Refactor sandbox and security configurations (#6973) --- openhands/core/config/sandbox_config.py | 23 +++++++++- openhands/core/config/security_config.py | 26 +++++++++++- openhands/core/config/utils.py | 24 ++++++----- tests/unit/test_config.py | 53 ++++++++++++++---------- 4 files changed, 91 insertions(+), 35 deletions(-) diff --git a/openhands/core/config/sandbox_config.py b/openhands/core/config/sandbox_config.py index af4fefc801..47fbc9295c 100644 --- a/openhands/core/config/sandbox_config.py +++ b/openhands/core/config/sandbox_config.py @@ -1,6 +1,6 @@ import os -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, ValidationError class SandboxConfig(BaseModel): @@ -74,3 +74,24 @@ class SandboxConfig(BaseModel): selected_repo: str | None = Field(default=None) model_config = {'extra': 'forbid'} + + @classmethod + def from_toml_section(cls, data: dict) -> dict[str, 'SandboxConfig']: + """ + Create a mapping of SandboxConfig instances from a toml dictionary representing the [sandbox] section. + + The configuration is built from all keys in data. + + Returns: + dict[str, SandboxConfig]: A mapping where the key "sandbox" corresponds to the [sandbox] configuration + """ + # Initialize the result mapping + sandbox_mapping: dict[str, SandboxConfig] = {} + + # Try to create the configuration instance + try: + sandbox_mapping['sandbox'] = cls.model_validate(data) + except ValidationError as e: + raise ValueError(f'Invalid sandbox configuration: {e}') + + return sandbox_mapping diff --git a/openhands/core/config/security_config.py b/openhands/core/config/security_config.py index a4805e3ab8..20c975f786 100644 --- a/openhands/core/config/security_config.py +++ b/openhands/core/config/security_config.py @@ -1,4 +1,4 @@ -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, ValidationError class SecurityConfig(BaseModel): @@ -11,3 +11,27 @@ class SecurityConfig(BaseModel): confirmation_mode: bool = Field(default=False) security_analyzer: str | None = Field(default=None) + + model_config = {'extra': 'forbid'} + + @classmethod + def from_toml_section(cls, data: dict) -> dict[str, 'SecurityConfig']: + """ + Create a mapping of SecurityConfig instances from a toml dictionary representing the [security] section. + + The configuration is built from all keys in data. + + Returns: + dict[str, SecurityConfig]: A mapping where the key "security" corresponds to the [security] configuration + """ + + # Initialize the result mapping + security_mapping: dict[str, SecurityConfig] = {} + + # Try to create the configuration instance + try: + security_mapping['security'] = cls.model_validate(data) + except ValidationError as e: + raise ValueError(f'Invalid security configuration: {e}') + + return security_mapping diff --git a/openhands/core/config/utils.py b/openhands/core/config/utils.py index 84c3f7ab02..69edc3695c 100644 --- a/openhands/core/config/utils.py +++ b/openhands/core/config/utils.py @@ -166,28 +166,32 @@ def load_from_toml(cfg: AppConfig, toml_file: str = 'config.toml') -> None: # Process security section if present if 'security' in toml_config: try: - logger.openhands_logger.debug( - 'Attempt to load security config from config toml' - ) - security_config = SecurityConfig(**toml_config['security']) - cfg.security = security_config + security_mapping = SecurityConfig.from_toml_section(toml_config['security']) + # We only use the base security config for now + if 'security' in security_mapping: + cfg.security = security_mapping['security'] except (TypeError, KeyError, ValidationError) as e: logger.openhands_logger.warning( f'Cannot parse [security] config from toml, values have not been applied.\nError: {e}' ) + except ValueError: + # Re-raise ValueError from SecurityConfig.from_toml_section + raise ValueError('Error in [security] section in config.toml') # Process sandbox section if present if 'sandbox' in toml_config: try: - logger.openhands_logger.debug( - 'Attempt to load sandbox config from config toml' - ) - sandbox_config = SandboxConfig(**toml_config['sandbox']) - cfg.sandbox = sandbox_config + sandbox_mapping = SandboxConfig.from_toml_section(toml_config['sandbox']) + # We only use the base sandbox config for now + if 'sandbox' in sandbox_mapping: + cfg.sandbox = sandbox_mapping['sandbox'] except (TypeError, KeyError, ValidationError) as e: logger.openhands_logger.warning( f'Cannot parse [sandbox] config from toml, values have not been applied.\nError: {e}' ) + except ValueError: + # Re-raise ValueError from SandboxConfig.from_toml_section + raise ValueError('Error in [sandbox] section in config.toml') # Process extended section if present if 'extended' in toml_config: diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index a5c5604f68..5d8f122168 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -460,11 +460,8 @@ def test_load_from_toml_partial_invalid(default_config, temp_toml_file, caplog): This ensures that: 1. Valid configuration sections are properly loaded - 2. Invalid fields are ignored gracefully - 3. The config object maintains correct values for valid fields - 4. Appropriate warnings are logged for invalid fields - - See `openhands/core/schema/config.py` for the list of valid fields. + 2. Invalid fields in security and sandbox sections raise ValueError + 4. The config object maintains correct values for valid fields """ with open(temp_toml_file, 'w', encoding='utf-8') as f: f.write(""" @@ -472,7 +469,7 @@ def test_load_from_toml_partial_invalid(default_config, temp_toml_file, caplog): debug = true [llm] -# No set in `openhands/core/schema/config.py` +# Not set in `openhands/core/schema/config.py` invalid_field = "test" model = "gpt-4" @@ -484,7 +481,6 @@ invalid_field_in_sandbox = "test" """) # Create a string buffer to capture log output - # Referenced from test_logging.py and `mock_logger` log_output = StringIO() handler = logging.StreamHandler(log_output) handler.setLevel(logging.WARNING) @@ -493,31 +489,42 @@ invalid_field_in_sandbox = "test" openhands_logger.addHandler(handler) try: - load_from_toml(default_config, temp_toml_file) + # Since sandbox_config.from_toml_section now raises ValueError for invalid fields, + # we need to catch that exception + with pytest.raises(ValueError) as excinfo: + load_from_toml(default_config, temp_toml_file) + + # Verify the error message mentions the invalid sandbox field + assert 'Error in [sandbox] section in config.toml' in str(excinfo.value) + log_content = log_output.getvalue() - # invalid [llm] config - # Verify that the appropriate warning was logged + # The LLM config should still log a warning but not raise an exception assert 'Cannot parse [llm] config from toml' in log_content - assert 'values have not been applied' in log_content - # Error: LLMConfig.__init__() got an unexpected keyword argume - assert 'Cannot parse [llm] config from toml' in log_content - assert 'invalid_field' in log_content - # invalid [sandbox] config - assert 'Cannot parse [sandbox] config from toml' in log_content - assert 'values have not been applied' in log_content - assert 'invalid_field_in_sandbox' in log_content - - # Verify valid configurations are loaded. Load from default instead of `config.toml` - # assert default_config.debug is True + # Verify valid configurations are loaded before the error was raised assert default_config.debug is True - assert default_config.get_llm_config().model == 'claude-3-5-sonnet-20241022' - assert default_config.get_agent_config().memory_enabled is True finally: openhands_logger.removeHandler(handler) +def test_load_from_toml_security_invalid(default_config, temp_toml_file): + """Test that invalid security configuration raises ValueError.""" + with open(temp_toml_file, 'w', encoding='utf-8') as f: + f.write(""" +[core] +debug = true + +[security] +invalid_security_field = "test" +""") + + with pytest.raises(ValueError) as excinfo: + load_from_toml(default_config, temp_toml_file) + + assert 'Error in [security] section in config.toml' in str(excinfo.value) + + def test_finalize_config(default_config): # Test finalize config assert default_config.workspace_mount_path is None