diff --git a/AGENTS.md b/AGENTS.md index 1e34c52cda..bf2b51f144 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -348,6 +348,7 @@ Note: This is separate from `persistence_dir` which is used for conversation sta - Clean caches: `make clean` - Run SDK examples: see [openhands-sdk/openhands/sdk/AGENTS.md](openhands-sdk/openhands/sdk/AGENTS.md). - The example workflow runs `uv run pytest tests/examples/test_examples.py --run-examples`; each successful example must print an `EXAMPLE_COST: ...` line to stdout (use `EXAMPLE_COST: 0` for non-LLM examples). +- Example scripts in `examples/` should use top-level code flow (e.g. `with` blocks, bare statements) rather than wrapping logic in a `def main()` function. The `def main` pattern creates unnecessary nesting that makes examples harder to read; keep the code flat and script-like. - Conversation plugins passed via `plugins=[...]` are lazy-loaded on the first `send_message()` or `run()`, so example code should inspect plugin-added skills or `resolved_plugins` only after that first interaction. - Programmatic settings live in `openhands-sdk/openhands/sdk/settings/`. Keep the exported schema focused on neutral config structure and semantics; downstream apps should own client-specific ordering, icons, widgets, and slash-command presentation. diff --git a/examples/02_remote_agent_server/12_settings_and_secrets_api.py b/examples/02_remote_agent_server/12_settings_and_secrets_api.py new file mode 100644 index 0000000000..5791a314e9 --- /dev/null +++ b/examples/02_remote_agent_server/12_settings_and_secrets_api.py @@ -0,0 +1,369 @@ +"""Example demonstrating the Settings and Secrets API. + +This example shows the recommended workflow for managing secrets: +1. Store secrets via PUT /api/settings/secrets (encrypted at rest) +2. Reference secrets in conversations via LookupSecret +3. Agent uses secrets via environment variables ($SECRET_NAME) +4. Clean up secrets via DELETE /api/settings/secrets/{name} + +This pattern enables: +- Secure secret storage (encrypted at rest with OH_SECRET_KEY) +- Lazy secret resolution at runtime (via LookupSecret URLs) +- Fine-grained secret lifecycle management (CRUD operations) +- Audit trail for secret access +""" + +import os +import subprocess +import sys +import tempfile +import threading +import time +from uuid import UUID + +import httpx + +from openhands.sdk import get_logger +from openhands.tools.file_editor import FileEditorTool +from openhands.tools.terminal import TerminalTool + + +logger = get_logger(__name__) + + +def _stream_output(stream, prefix, target_stream): + """Stream output from subprocess to target stream with prefix.""" + try: + for line in iter(stream.readline, ""): + if line: + target_stream.write(f"[{prefix}] {line}") + target_stream.flush() + except Exception as e: + print(f"Error streaming {prefix}: {e}", file=sys.stderr) + finally: + stream.close() + + +class ManagedAPIServer: + """Context manager for subprocess-managed OpenHands API server.""" + + def __init__(self, port: int = 8000, host: str = "127.0.0.1"): + self.port: int = port + self.host: str = host + self.process: subprocess.Popen[str] | None = None + self.base_url: str = f"http://{host}:{port}" + self.stdout_thread: threading.Thread | None = None + self.stderr_thread: threading.Thread | None = None + + def __enter__(self): + """Start the API server subprocess.""" + print(f"Starting OpenHands API server on {self.base_url}...") + + # Set OH_SECRET_KEY to enable encrypted secrets feature + # In production, this should be a secure randomly generated key + # Set TMUX_TMPDIR to a short path to avoid socket path length issues on macOS + env = { + "LOG_JSON": "true", + "OH_SECRET_KEY": "example-secret-key-for-demo-only-32b", + "TMUX_TMPDIR": "/tmp/oh-tmux", + **os.environ, + } + + self.process = subprocess.Popen( + [ + "python", + "-m", + "openhands.agent_server", + "--port", + str(self.port), + "--host", + self.host, + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + env=env, + ) + + assert self.process is not None + assert self.process.stdout is not None + assert self.process.stderr is not None + self.stdout_thread = threading.Thread( + target=_stream_output, + args=(self.process.stdout, "SERVER", sys.stdout), + daemon=True, + ) + self.stderr_thread = threading.Thread( + target=_stream_output, + args=(self.process.stderr, "SERVER", sys.stderr), + daemon=True, + ) + self.stdout_thread.start() + self.stderr_thread.start() + + # Wait for server to be ready + max_retries = 30 + for i in range(max_retries): + try: + response = httpx.get(f"{self.base_url}/health", timeout=2.0) + if response.status_code == 200: + print(f"✅ Server ready after {i + 1} attempts") + return self + except httpx.RequestError: + pass + time.sleep(1) + + raise RuntimeError(f"Server failed to start after {max_retries} seconds") + + def __exit__(self, exc_type, exc_val, exc_tb): + """Stop the API server subprocess.""" + if self.process: + print("Stopping API server...") + self.process.terminate() + try: + self.process.wait(timeout=5) + except subprocess.TimeoutExpired: + self.process.kill() + self.process.wait() + print("✅ Server stopped") + + +# Get LLM configuration from environment +api_key = os.getenv("LLM_API_KEY") +assert api_key is not None, "LLM_API_KEY environment variable is not set." +llm_model = os.getenv("LLM_MODEL", "anthropic/claude-sonnet-4-5-20250929") +llm_base_url = os.getenv("LLM_BASE_URL") # Optional custom base URL + +with ManagedAPIServer(port=8765) as server: + client = httpx.Client(base_url=server.base_url, timeout=120.0) + + try: + # ══════════════════════════════════════════════════════════════ + # Part 1: Store LLM Settings via Settings API + # ══════════════════════════════════════════════════════════════ + logger.info("\n" + "=" * 60) + logger.info("🔧 Storing LLM configuration via Settings API") + logger.info("=" * 60) + + # Store LLM configuration - the API key is encrypted at rest + llm_config: dict[str, str] = { + "model": llm_model, + "api_key": api_key, + } + if llm_base_url: + llm_config["base_url"] = llm_base_url + + response = client.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": llm_config}}, + ) + assert response.status_code == 200, f"PATCH settings failed: {response.text}" + settings = response.json() + + logger.info("✅ LLM settings stored successfully") + logger.info(f" - LLM model: {settings['agent_settings']['llm']['model']}") + if llm_base_url: + logger.info(f" - Base URL: {llm_base_url}") + logger.info(f" - API key set: {settings['llm_api_key_is_set']}") + + # ══════════════════════════════════════════════════════════════ + # Part 2: Store Custom Secret via Secrets API + # ══════════════════════════════════════════════════════════════ + logger.info("\n" + "=" * 60) + logger.info("🔐 Storing custom secret via Secrets API") + logger.info("=" * 60) + + # Store a custom secret - this could be an API token, database password, etc. + # The secret is encrypted at rest using OH_SECRET_KEY + secret_name = "MY_PROJECT_TOKEN" + secret_value = "super-secret-token-12345" + + response = client.put( + "/api/settings/secrets", + json={ + "name": secret_name, + "value": secret_value, + "description": "Example project token for demonstration", + }, + ) + assert response.status_code == 200, f"PUT secret failed: {response.text}" + logger.info(f"✅ Created secret: {secret_name}") + + # List secrets to verify (values are not exposed) + response = client.get("/api/settings/secrets") + assert response.status_code == 200 + secrets_list = response.json()["secrets"] + logger.info(f"✅ Server has {len(secrets_list)} secret(s) stored") + for secret in secrets_list: + logger.info(f" - {secret['name']}: {secret.get('description', '')}") + + # ══════════════════════════════════════════════════════════════ + # Part 3: Start Conversation with LookupSecret Reference + # ══════════════════════════════════════════════════════════════ + logger.info("\n" + "=" * 60) + logger.info("🤖 Starting conversation with secret reference") + logger.info("=" * 60) + + # Create a workspace directory + temp_workspace_dir = tempfile.mkdtemp(prefix="secrets_api_demo_") + + # Build the LookupSecret URL - agent server resolves this at runtime + # The URL points to the secrets endpoint on the same server + lookup_url = f"{server.base_url}/api/settings/secrets/{secret_name}" + + # Start conversation with LookupSecret reference + # The secret will be resolved lazily when the agent needs it + start_request = { + "agent": { + "kind": "Agent", + "llm": llm_config, # Use same LLM config (model, api_key, base_url) + "tools": [ + {"name": TerminalTool.name}, + {"name": FileEditorTool.name}, + ], + }, + "workspace": {"working_dir": temp_workspace_dir}, + # Reference the stored secret via LookupSecret + # This creates an environment variable $MY_PROJECT_TOKEN in the agent + "secrets": { + secret_name: { + "kind": "LookupSecret", + "url": lookup_url, + "description": "Project token resolved from secrets API", + } + }, + "initial_message": { + "role": "user", + "content": [ + { + "type": "text", + "text": f"Echo the value of the ${secret_name} environment " + "variable to see if you have access. " + "If so just respond `YES`, otherwise `NO`.", + } + ], + "run": True, # Auto-run after sending message + }, + } + + response = client.post("/api/conversations", json=start_request) + assert response.status_code == 201, ( + f"Start conversation failed: {response.text}" + ) + conversation_info = response.json() + conversation_id = UUID(conversation_info["id"]) + + logger.info("✅ Conversation started!") + logger.info(f" - Conversation ID: {conversation_id}") + logger.info(f" - Secret '{secret_name}' available as env var") + + # ══════════════════════════════════════════════════════════════ + # Part 4: Wait for Agent to Complete + # ══════════════════════════════════════════════════════════════ + logger.info("\n" + "=" * 60) + logger.info("⏳ Waiting for agent to complete task...") + logger.info("=" * 60) + + # Poll conversation until agent finishes + max_wait = 120 # seconds + poll_interval = 2 + elapsed = 0 + execution_status = "unknown" + + while elapsed < max_wait: + response = client.get(f"/api/conversations/{conversation_id}") + assert response.status_code == 200 + conversation_data = response.json() + execution_status = conversation_data.get("execution_status", "unknown") + + if execution_status in ("stopped", "paused", "error"): + break + + logger.info(f" Status: {execution_status} (waited {elapsed}s)") + time.sleep(poll_interval) + elapsed += poll_interval + + logger.info(f"✅ Agent finished with status: {execution_status}") + + # Get the agent's final response to verify the task was completed + response = client.get( + f"/api/conversations/{conversation_id}/agent_final_response" + ) + accumulated_cost = 0.0 + if response.status_code == 200: + result = response.json() + agent_response = result.get("response", "") + if agent_response: + # Truncate long responses for display + display_response = ( + agent_response[:200] + "..." + if len(agent_response) > 200 + else agent_response + ) + logger.info(f" Agent response: {display_response}") + logger.info(" ✅ Agent completed the task using the secret!") + + # Get conversation metrics from stats + response = client.get(f"/api/conversations/{conversation_id}") + if response.status_code == 200: + conversation_data = response.json() + # Metrics are tracked per-LLM usage in stats.usage_to_metrics + stats = conversation_data.get("stats") or {} + usage_to_metrics = stats.get("usage_to_metrics") or {} + # Sum accumulated_cost across all LLM usages + accumulated_cost = sum( + m.get("accumulated_cost", 0.0) for m in usage_to_metrics.values() + ) + + # Clean up - delete conversation + client.delete(f"/api/conversations/{conversation_id}") + logger.info(" Conversation deleted") + + # ══════════════════════════════════════════════════════════════ + # Part 5: Clean Up - Delete the Secret + # ══════════════════════════════════════════════════════════════ + logger.info("\n" + "=" * 60) + logger.info("🧹 Cleaning up - deleting secret") + logger.info("=" * 60) + + # Delete the secret after use + response = client.delete(f"/api/settings/secrets/{secret_name}") + assert response.status_code == 200, f"DELETE secret failed: {response.text}" + logger.info(f"✅ Deleted secret: {secret_name}") + + # Verify deletion + response = client.get(f"/api/settings/secrets/{secret_name}") + assert response.status_code == 404 + logger.info("✅ Verified deletion (secret no longer accessible)") + + # ══════════════════════════════════════════════════════════════ + # Part 6: Test Secret Name Validation + # ══════════════════════════════════════════════════════════════ + logger.info("\n" + "=" * 60) + logger.info("⚠️ Testing secret name validation") + logger.info("=" * 60) + + # Invalid: starts with number + response = client.put( + "/api/settings/secrets", + json={"name": "123_invalid", "value": "test"}, + ) + assert response.status_code == 422 + logger.info("✅ Rejected '123_invalid' (starts with number)") + + # Invalid: contains hyphen + response = client.put( + "/api/settings/secrets", + json={"name": "invalid-name", "value": "test"}, + ) + assert response.status_code == 422 + logger.info("✅ Rejected 'invalid-name' (contains hyphen)") + + logger.info("\n" + "=" * 60) + logger.info("🎉 All Settings and Secrets API tests passed!") + logger.info("=" * 60) + + print(f"EXAMPLE_COST: {accumulated_cost}") + + finally: + client.close() diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 5f4eab4681..e2bd321883 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -477,10 +477,23 @@ async def _start_conversation( # serialize to plain strings. Pass expose_secrets=True so StaticSecret values # are preserved through the round-trip; the dict is only used in-process to # construct StoredConversation, not sent over the network. - stored = StoredConversation( - id=conversation_id, - **request.model_dump(mode="json", context={"expose_secrets": True}), - ) + request_data = request.model_dump(mode="json", context={"expose_secrets": True}) + + # If secrets_encrypted=True, the agent's secrets (e.g., LLM api_key) are + # cipher-encrypted and need decryption during model validation. Pass the + # cipher in the validation context so validate_secret() can decrypt them. + if request.secrets_encrypted: + if self.cipher is None: + raise ValueError( + "Cannot decrypt secrets: cipher not configured. " + "Set OH_SECRET_KEY environment variable." + ) + stored = StoredConversation.model_validate( + {"id": conversation_id, **request_data}, + context={"cipher": self.cipher}, + ) + else: + stored = StoredConversation(id=conversation_id, **request_data) event_service = await self._start_event_service(stored) initial_message = request.initial_message if initial_message: diff --git a/openhands-agent-server/openhands/agent_server/persistence/__init__.py b/openhands-agent-server/openhands/agent_server/persistence/__init__.py new file mode 100644 index 0000000000..def3b52e10 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/persistence/__init__.py @@ -0,0 +1,43 @@ +"""Persistence module for settings and secrets storage.""" + +from openhands.agent_server.persistence.models import ( + SECRET_NAME_PATTERN, + CustomSecret, + CustomSecretCreate, + CustomSecretResponse, + PersistedSettings, + Secrets, + SecretsResponse, + SettingsUpdatePayload, +) +from openhands.agent_server.persistence.store import ( + FileSecretsStore, + FileSettingsStore, + SecretsStore, + SettingsStore, + get_secrets_store, + get_settings_store, + reset_stores, +) + + +__all__ = [ + # Constants + "SECRET_NAME_PATTERN", + # Models + "CustomSecret", + "CustomSecretCreate", + "CustomSecretResponse", + "PersistedSettings", + "Secrets", + "SecretsResponse", + "SettingsUpdatePayload", + # Stores + "FileSecretsStore", + "FileSettingsStore", + "SecretsStore", + "SettingsStore", + "get_secrets_store", + "get_settings_store", + "reset_stores", +] diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py new file mode 100644 index 0000000000..38e1f7fe9d --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -0,0 +1,397 @@ +"""Pydantic models for persisted settings and secrets. + +These models mirror the structure used in OpenHands app-server for consistency, +allowing the agent-server to be used standalone or as a drop-in replacement +for the Cloud API's settings/secrets endpoints. +""" + +from __future__ import annotations + +import re +from typing import Any, TypedDict + +from pydantic import ( + BaseModel, + ConfigDict, + Field, + SecretStr, + SerializationInfo, + ValidationInfo, + field_serializer, + field_validator, + model_validator, +) + +from openhands.sdk.settings import ( + AGENT_SETTINGS_SCHEMA_VERSION, + AgentSettings, + AgentSettingsConfig, + ConversationSettings, + default_agent_settings, +) +from openhands.sdk.settings.model import ( + _AGENT_SETTINGS_MIGRATIONS, + _apply_persisted_migrations, +) +from openhands.sdk.utils.pydantic_secrets import serialize_secret, validate_secret + + +class SettingsUpdatePayload(TypedDict, total=False): + """Typed payload for PersistedSettings.update() method.""" + + agent_settings_diff: dict[str, Any] + conversation_settings_diff: dict[str, Any] + + +def _deep_merge(base: dict[str, Any], overlay: dict[str, Any]) -> dict[str, Any]: + """Recursively merge overlay dict into base dict. + + For nested dicts, merges recursively. For other types, overlay wins. + """ + result = dict(base) + for key, value in overlay.items(): + if key in result and isinstance(result[key], dict) and isinstance(value, dict): + result[key] = _deep_merge(result[key], value) + else: + result[key] = value + return result + + +class PersistedSettings(BaseModel): + """Persisted settings for agent server. + + Agent settings (LLM config, MCP config, condenser) live in ``agent_settings``. + Conversation settings (max_iterations, confirmation_mode) live in + ``conversation_settings``. + """ + + agent_settings: AgentSettingsConfig = Field(default_factory=default_agent_settings) + conversation_settings: ConversationSettings = Field( + default_factory=ConversationSettings + ) + + model_config = ConfigDict(populate_by_name=True) + + @property + def llm_api_key_is_set(self) -> bool: + """Check if an LLM API key is configured.""" + raw = self.agent_settings.llm.api_key + if raw is None: + return False + secret_value = ( + raw.get_secret_value() if isinstance(raw, SecretStr) else str(raw) + ) + return bool(secret_value and secret_value.strip()) + + def update(self, payload: SettingsUpdatePayload) -> None: + """Apply a batch of changes from a nested dict. + + Accepts ``agent_settings_diff`` and ``conversation_settings_diff`` + for partial updates. Uses ``from_persisted()`` to apply any schema + migrations if the incoming diff contains an older schema version. + + Thread Safety: + This method is NOT thread-safe for concurrent in-memory updates. + The assignments to ``agent_settings`` and ``conversation_settings`` + are not atomic. However, the router wraps calls via ``store.update()`` + which uses file locking to prevent concurrent updates at the I/O layer. + Multiple ``PersistedSettings`` instances should NOT be shared across + threads without external synchronization. + + Atomicity: + Both updates are validated before any mutations occur. If either + validation fails, the object remains unchanged. + + Note: + Secret values are temporarily exposed in memory during the merge + operation. Merged dicts are cleared after use to minimize exposure. + + Raises: + ValueError: If validation fails (sanitized to avoid secret leakage). + """ + agent_update = payload.get("agent_settings_diff") + conv_update = payload.get("conversation_settings_diff") + + # Phase 1: Validate both updates before any mutations + new_agent: AgentSettingsConfig | None = None + new_conv: ConversationSettings | None = None + agent_merged: dict | None = None + conv_merged: dict | None = None + + try: + if isinstance(agent_update, dict): + agent_merged = _deep_merge( + self.agent_settings.model_dump( + mode="json", context={"expose_secrets": "plaintext"} + ), + agent_update, + ) + try: + new_agent = AgentSettings.from_persisted(agent_merged) + except Exception as e: + # Use 'from None' to break exception chain - the original + # exception may contain secret values in Pydantic errors + raise ValueError( + f"Failed to update agent settings: {type(e).__name__}" + ) from None + + if isinstance(conv_update, dict): + conv_merged = _deep_merge( + self.conversation_settings.model_dump(mode="json"), + conv_update, + ) + try: + new_conv = ConversationSettings.from_persisted(conv_merged) + except Exception as e: + # Use 'from None' to break exception chain - see above + raise ValueError( + f"Failed to update conversation settings: {type(e).__name__}" + ) from None + + # Phase 2: Apply validated changes atomically + if new_agent is not None: + self.agent_settings = new_agent + if new_conv is not None: + self.conversation_settings = new_conv + finally: + # Clear merged dicts to minimize plaintext exposure window + if agent_merged is not None: + agent_merged.clear() + if conv_merged is not None: + conv_merged.clear() + + @field_serializer("agent_settings") + def agent_settings_serializer( + self, + agent_settings: AgentSettingsConfig, + info: SerializationInfo, + ) -> dict[str, Any]: + # Pass through the full context (cipher, expose_secrets) to AgentSettings + # This ensures secrets are properly encrypted/exposed based on context + return agent_settings.model_dump(mode="json", context=info.context) + + @model_validator(mode="before") + @classmethod + def _normalize_inputs(cls, data: dict | object) -> dict | object: + """Normalize inputs during deserialization. + + Applies schema migrations for both agent and conversation settings, + ensuring forward compatibility when loading settings files saved with + older schema versions. + + Note: We keep agent_settings as a dict here so that Pydantic's normal + validation handles it with context. This allows cipher-based decryption + to work properly through nested field validators (e.g., LLM._validate_secrets). + """ + if not isinstance(data, dict): + return data + + # Apply migrations for agent_settings but keep as dict + # The dict will be validated by Pydantic with context for decryption + agent_settings = data.get("agent_settings") + if isinstance(agent_settings, dict): + coerced = _coerce_dict_secrets(agent_settings) + # Apply migrations only, return dict for Pydantic to validate with context + migrated = _apply_persisted_migrations( + coerced, + current_version=AGENT_SETTINGS_SCHEMA_VERSION, + migrations=_AGENT_SETTINGS_MIGRATIONS, + payload_name="AgentSettings", + ) + data["agent_settings"] = migrated + + # Apply migrations for conversation_settings + conv_settings = data.get("conversation_settings") + if isinstance(conv_settings, dict): + data["conversation_settings"] = ConversationSettings.from_persisted( + conv_settings + ) + + return data + + +# Validation pattern for secret names - exported for use by settings_router +# Names: start with letter, alphanumeric + underscores, 1-64 chars +SECRET_NAME_PATTERN = re.compile(r"^[a-zA-Z][a-zA-Z0-9_]{0,63}$") + + +class CustomSecret(BaseModel): + """A custom secret with name, value, and optional description.""" + + name: str + secret: SecretStr | None + description: str | None = None + + @field_validator("name") + @classmethod + def _validate_name(cls, v: str) -> str: + """Validate secret name format for safety. + + Secret names are used as environment variable names and may be logged, + so we enforce strict validation to prevent: + - Path traversal (../, null bytes) + - Log injection (control characters) + - Shell injection (special characters) + - Invalid env var names (starting with numbers, special chars) + + Note: The router also validates names, but this provides defense-in-depth + for secrets created directly via the store (bypassing the HTTP layer). + """ + if not SECRET_NAME_PATTERN.match(v): + raise ValueError( + "Secret name must start with a letter, contain only " + "letters/numbers/underscores, and be 1-64 characters" + ) + return v + + @field_validator("secret") + @classmethod + def _validate_secret( + cls, v: str | SecretStr | None, info: ValidationInfo + ) -> SecretStr | None: + return validate_secret(v, info) + + @field_serializer("secret", when_used="always") + def _serialize_secret(self, v: SecretStr | None, info: SerializationInfo): + return serialize_secret(v, info) + + +class Secrets(BaseModel): + """Model for storing custom secrets. + + Unlike OpenHands app-server which also stores provider tokens, + the agent-server only stores custom secrets since it doesn't + integrate with OAuth providers directly. + """ + + custom_secrets: dict[str, CustomSecret] = Field(default_factory=dict) + + model_config = ConfigDict(frozen=True) + + def get_env_vars(self) -> dict[str, str]: + """Get secrets as environment variables dict. + + Safely extracts secret values, logging warnings for malformed secrets. + """ + result: dict[str, str] = {} + for name, secret in self.custom_secrets.items(): + if secret.secret is None: + continue + try: + result[name] = secret.secret.get_secret_value() + except Exception: + # Log without exposing secret contents + from openhands.sdk.logger import get_logger + + get_logger(__name__).warning( + f"Failed to extract secret '{name}' - skipping" + ) + return result + + def get_descriptions(self) -> dict[str, str | None]: + """Get secret name to description mapping.""" + return { + name: secret.description for name, secret in self.custom_secrets.items() + } + + @field_serializer("custom_secrets") + def custom_secrets_serializer( + self, custom_secrets: dict[str, CustomSecret], info: SerializationInfo + ) -> dict[str, dict[str, Any]]: + # Delegate to CustomSecret.model_dump which uses serialize_secret + # This ensures cipher context flows through for encryption + result = {} + for name, secret in custom_secrets.items(): + result[name] = secret.model_dump(mode="json", context=info.context) + return result + + @model_validator(mode="before") + @classmethod + def _normalize_inputs(cls, data: dict | object) -> dict | object: + """Normalize dict inputs to the expected structure. + + Note: We deliberately keep values as raw strings/dicts here so that + Pydantic's field validators can handle cipher-based decryption via + the validation context. Wrapping in SecretStr here would bypass the + validate_secret() call that handles decryption. + """ + if not isinstance(data, dict): + return data + + custom_secrets = data.get("custom_secrets") + if isinstance(custom_secrets, dict): + converted = {} + for name, value in custom_secrets.items(): + if isinstance(value, CustomSecret): + converted[name] = value + elif isinstance(value, dict): + # Keep as dict - let Pydantic handle validation with context + # Note: Use None instead of "" for missing secret to preserve + # distinction between "empty secret" and "missing secret" + converted[name] = { + "name": name, + "secret": value.get("secret"), # None if missing + "description": value.get("description"), + } + elif isinstance(value, str): + converted[name] = { + "name": name, + "secret": value, + "description": None, + } + data["custom_secrets"] = converted + + return data + + +# ── Response Models for API ────────────────────────────────────────────── + + +class CustomSecretCreate(BaseModel): + """Request model for creating a custom secret.""" + + name: str + value: SecretStr + description: str | None = None + + +class CustomSecretResponse(BaseModel): + """Response model for a custom secret (without value).""" + + name: str + description: str | None = None + + +class SecretsResponse(BaseModel): + """Response model listing available secrets.""" + + secrets: list[CustomSecretResponse] + + +# ── Helper Functions ───────────────────────────────────────────────────── + + +def _coerce_dict_secrets(d: dict[str, Any]) -> dict[str, Any]: + """Recursively coerce SecretStr leaves to plain values. + + Note: SecretStr extraction is wrapped in error handling to prevent secret + values from leaking in exception tracebacks. + """ + from openhands.sdk.logger import get_logger + + _logger = get_logger(__name__) + out: dict[str, Any] = {} + for k, v in d.items(): + if isinstance(v, dict): + out[k] = _coerce_dict_secrets(v) + elif isinstance(v, SecretStr): + try: + out[k] = v.get_secret_value() + except Exception: + _logger.warning( + f"Failed to extract secret value for key '{k}' - skipping" + ) + out[k] = None + else: + out[k] = v + return out diff --git a/openhands-agent-server/openhands/agent_server/persistence/store.py b/openhands-agent-server/openhands/agent_server/persistence/store.py new file mode 100644 index 0000000000..3f9dfc1822 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/persistence/store.py @@ -0,0 +1,697 @@ +"""File-based storage implementations for settings and secrets. + +Following the same pattern as OpenHands app-server's FileSettingsStore +and FileSecretsStore for consistency. + +File locking uses fcntl on Unix and msvcrt on Windows. +""" + +from __future__ import annotations + +import json +import os +import stat +import sys +import threading +from abc import ABC, abstractmethod +from collections.abc import Callable, Iterator +from contextlib import contextmanager +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from pydantic import SecretStr + +from openhands.agent_server.persistence.models import ( + CustomSecret, + PersistedSettings, + Secrets, +) +from openhands.sdk.logger import get_logger +from openhands.sdk.utils.cipher import Cipher + + +# fcntl is Unix-only; on Windows, use msvcrt for file locking +if sys.platform != "win32": + import fcntl + + msvcrt = None +else: + fcntl = None # type: ignore[assignment] + import msvcrt + + +if TYPE_CHECKING: + from openhands.agent_server.config import Config + + +logger = get_logger(__name__) + +# File permission constants (owner read/write only) +_DIR_MODE = stat.S_IRWXU # 0o700 - rwx------ +_FILE_MODE = stat.S_IRUSR | stat.S_IWUSR # 0o600 - rw------- + +# Windows reserved filenames (case-insensitive) +_WINDOWS_RESERVED_NAMES = frozenset( + { + "CON", + "PRN", + "AUX", + "NUL", + "COM1", + "COM2", + "COM3", + "COM4", + "COM5", + "COM6", + "COM7", + "COM8", + "COM9", + "LPT1", + "LPT2", + "LPT3", + "LPT4", + "LPT5", + "LPT6", + "LPT7", + "LPT8", + "LPT9", + } +) + + +def _validate_filename(filename: str) -> None: + """Validate filename to prevent path traversal and injection attacks. + + Raises: + ValueError: If filename is invalid or potentially dangerous. + """ + # Check for empty filename (would resolve to parent directory) + if not filename: + raise ValueError("filename must not be empty") + + # Check for path separators + if "/" in filename or "\\" in filename: + raise ValueError("filename must not contain path separators") + + # Check for leading dots (hidden files, parent directory traversal) + if filename.startswith("."): + raise ValueError("filename must not start with '.'") + + # Check for null bytes (null byte injection) + if "\x00" in filename: + raise ValueError("filename must not contain null bytes") + + # Check for trailing dots/spaces (Windows path handling issues) + if filename.endswith(".") or filename.endswith(" "): + raise ValueError("filename must not end with '.' or space") + + # Check for Windows reserved names (split handles multi-extension files) + # e.g., "CON.txt.json" -> "CON" not "CON.txt" + basename = filename.split(".")[0].upper() + if basename in _WINDOWS_RESERVED_NAMES: + raise ValueError(f"filename '{filename}' uses a reserved name") + + +def _ensure_secure_directory(path: Path) -> None: + """Ensure directory exists with secure permissions. + + Creates all parent directories with secure permissions (0o700). + If it already exists, ensures permissions are correct. + """ + if not path.exists(): + # Create parents with secure permissions + current = path + to_create: list[Path] = [] + while not current.exists(): + to_create.append(current) + current = current.parent + + for dir_path in reversed(to_create): + dir_path.mkdir(mode=_DIR_MODE, exist_ok=True) + + # Ensure permissions are correct even if dir already existed + try: + path.chmod(_DIR_MODE) + except OSError as e: + logger.warning(f"Failed to set permissions on {path}: {e}") + + +@contextmanager +def _file_lock(lock_path: Path) -> Iterator[None]: + """Context manager for file-based locking. + + Uses Unix fcntl for exclusive locking to prevent race conditions during + read-modify-write operations. On Windows, uses msvcrt.locking. + """ + _ensure_secure_directory(lock_path.parent) + + # Create lock file - use O_RDWR for Windows compatibility with msvcrt + fd = os.open(lock_path, os.O_RDWR | os.O_CREAT, _FILE_MODE) + try: + if fcntl is not None: + # Unix: use fcntl for file locking + fcntl.flock(fd, fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + elif msvcrt is not None: + # Windows: use msvcrt for file locking + # Lock multiple bytes for more reliable locking behavior + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_LOCK, 100) + try: + yield + finally: + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_UNLCK, 100) + else: + # This should never happen on standard systems (Unix or Windows) + # Raise an error rather than silently proceeding without locking, + # which could cause data corruption from concurrent writes + raise RuntimeError( + "File locking not available on this platform. " + "Concurrent writes may cause data corruption." + ) + finally: + os.close(fd) + + +def _atomic_write_json(path: Path, data: dict) -> None: + """Write JSON atomically with secure permissions. + + Uses write-to-temp-then-rename pattern to prevent corruption + if interrupted. Creates temp file with owner-only permissions from + the start to prevent race conditions where sensitive data could + be read before chmod. + + Note: + The rename operation (Path.replace) is atomic on POSIX systems. + On Windows, it may not be fully atomic in all edge cases (e.g., + concurrent access, network drives), but provides reasonable + protection against corruption from interrupted writes. + """ + import uuid + + # Use PID, time, and uuid for unique temp filename to prevent collisions + # when multiple processes/threads write to the same file concurrently + unique_suffix = f".tmp.{os.getpid()}.{uuid.uuid4().hex[:8]}" + tmp_path = path.with_suffix(unique_suffix) + # Create file with secure permissions from the start using os.open + # O_EXCL ensures exclusive creation (fails if file exists) + fd = os.open(tmp_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, _FILE_MODE) + fdopen_succeeded = False + try: + f = os.fdopen(fd, "w", encoding="utf-8") + fdopen_succeeded = True + with f: + json.dump(data, f, indent=2) + except Exception: + # Only close fd manually if os.fdopen() didn't take ownership + if not fdopen_succeeded: + try: + os.close(fd) + except OSError: + pass + # Clean up temp file on error + try: + tmp_path.unlink(missing_ok=True) + except OSError: + pass + raise + + # Atomic rename - clean up temp file if replace() fails + try: + tmp_path.replace(path) # Atomic on POSIX + except Exception: + try: + tmp_path.unlink(missing_ok=True) + except OSError: + pass + raise + + +# Default storage directory (relative to working directory) +DEFAULT_PERSISTENCE_DIR = Path("workspace/.openhands") + + +class SettingsStore(ABC): + """Abstract base class for settings storage.""" + + @abstractmethod + def load(self) -> PersistedSettings | None: + """Load settings from storage.""" + + @abstractmethod + def save(self, settings: PersistedSettings) -> None: + """Save settings to storage.""" + + @abstractmethod + def update( + self, update_fn: Callable[[PersistedSettings], PersistedSettings] + ) -> PersistedSettings: + """Atomically update settings with file locking. + + Args: + update_fn: Function that receives current settings and returns + updated settings. + + Returns: + The updated settings after saving. + """ + + +class SecretsStore(ABC): + """Abstract base class for secrets storage.""" + + @abstractmethod + def load(self) -> Secrets | None: + """Load secrets from storage.""" + + @abstractmethod + def save(self, secrets: Secrets) -> None: + """Save secrets to storage.""" + + @abstractmethod + def get_secret(self, name: str) -> str | None: + """Get a single secret value by name.""" + + @abstractmethod + def set_secret(self, name: str, value: str, description: str | None = None) -> None: + """Set a single secret.""" + + @abstractmethod + def delete_secret(self, name: str) -> bool: + """Delete a secret. Returns True if it existed.""" + + +class FileSettingsStore(SettingsStore): + """File-based settings storage. + + Stores settings as JSON in a configurable directory. + Secrets within settings are encrypted using the provided cipher. + + Security features: + - Files created with owner-only permissions (0o600) + - Directory created with owner-only permissions (0o700) + - Atomic writes to prevent corruption + """ + + def __init__( + self, + persistence_dir: Path | str, + cipher: Cipher | None = None, + filename: str = "settings.json", + ): + # Validate filename to prevent path traversal and injection attacks + _validate_filename(filename) + self.persistence_dir = Path(persistence_dir) + self.cipher = cipher + self.filename = filename + self._path = self.persistence_dir / filename + self._lock_path = self.persistence_dir / ".settings.lock" + + def load(self) -> PersistedSettings | None: + """Load settings from file. + + If a cipher is provided, secrets are decrypted via Pydantic's + validation context. The cipher is passed to model_validate which + flows through to field validators using validate_secret(). + """ + if not self._path.exists(): + logger.debug(f"Settings file not found: {self._path}") + return None + + try: + with self._path.open("r", encoding="utf-8") as f: + data = json.load(f) + + # Pass cipher in context for automatic decryption of all secret fields + # This flows through to field validators using validate_secret() + context = {"cipher": self.cipher} if self.cipher else None + return PersistedSettings.model_validate(data, context=context) + except (PermissionError, OSError) as e: + # Critical filesystem errors should be re-raised + logger.error(f"Cannot access settings file: {e}") + raise + except json.JSONDecodeError as e: + # Corrupted file - log and return None to allow recovery + logger.error(f"Settings file is corrupted: {e}") + return None + except Exception: + # Validation or other errors - log and return None + logger.error("Failed to load settings", exc_info=True) + return None + + def save(self, settings: PersistedSettings) -> None: + """Save settings to file atomically with secure permissions. + + If a cipher is provided, secrets are encrypted via Pydantic's + serialization context. The cipher is passed to model_dump which + flows through to field serializers using serialize_secret(). + + Warning: + This method does NOT acquire a file lock. For concurrent-safe + updates, use :meth:`update` which wraps save() with file locking. + Direct calls to save() from multiple processes may cause lost updates. + + Warning: + If no cipher is provided, secrets are stored in plaintext. + This is logged as a security warning on first save. + """ + _ensure_secure_directory(self.persistence_dir) + + # Pass cipher in context for automatic encryption of all secret fields + # This flows through to field serializers using serialize_secret() + if self.cipher: + context: dict[str, Any] = {"cipher": self.cipher} + else: + context = {"expose_secrets": "plaintext"} + # Warn about plaintext secret storage (only if secrets exist) + if settings.llm_api_key_is_set: + logger.warning( + "Saving settings with secrets in PLAINTEXT (no cipher configured). " + "Configure OH_SECRET_KEY for production deployments." + ) + + data = settings.model_dump(mode="json", context=context) + + _atomic_write_json(self._path, data) + logger.debug(f"Settings saved to {self._path}") + + def update( + self, update_fn: Callable[[PersistedSettings], PersistedSettings] + ) -> PersistedSettings: + """Atomically update settings with file locking. + + Uses file locking to prevent concurrent updates from overwriting + each other. The update function is called within the lock. + + Args: + update_fn: Function that receives current settings and returns + updated settings. + + Returns: + The updated settings after saving. + + Raises: + RuntimeError: If the settings file exists but cannot be loaded + (e.g., corrupted JSON, decryption failure). This prevents + data loss from overwriting existing settings with defaults. + """ + with _file_lock(self._lock_path): + settings = self.load() + if settings is None: + # File doesn't exist or is empty - safe to use defaults + if self._path.exists(): + # File exists but load() returned None - corrupted or unreadable + raise RuntimeError( + f"Cannot load settings from {self._path}. " + "File may be corrupted or encrypted with a different key. " + "Refusing to overwrite with defaults to prevent data loss." + ) + settings = PersistedSettings() + updated = update_fn(settings) + self.save(updated) + return updated + + +class FileSecretsStore(SecretsStore): + """File-based secrets storage. + + Stores secrets as encrypted JSON in a configurable directory. + All secret values are encrypted using the provided cipher. + + Security features: + - Files created with owner-only permissions (0o600) + - Directory created with owner-only permissions (0o700) + - Atomic writes to prevent corruption + - File locking to prevent race conditions + + Note: + On Windows, the 0o600 file permissions are not enforced by the + filesystem. If storing secrets without encryption (cipher=None), + they may be readable by other local users. Configure OH_SECRET_KEY + to enable encryption for secure storage on all platforms. + """ + + def __init__( + self, + persistence_dir: Path | str, + cipher: Cipher | None = None, + filename: str = "secrets.json", + ): + # Use same validation as FileSettingsStore + _validate_filename(filename) + self.persistence_dir = Path(persistence_dir) + self.cipher = cipher + self.filename = filename + self._path = self.persistence_dir / filename + self._lock_path = self.persistence_dir / ".secrets.lock" + + # Warn about Windows security limitations when no encryption + if sys.platform == "win32" and not cipher: + logger.warning( + "Storing secrets without encryption on Windows. " + "File permissions are not enforced. Configure OH_SECRET_KEY " + "for secure storage." + ) + + def load(self) -> Secrets | None: + """Load secrets from file. + + If a cipher is provided, secrets are decrypted via Pydantic's + validation context. The cipher is passed to model_validate which + flows through to field validators using validate_secret(). + """ + if not self._path.exists(): + logger.debug(f"Secrets file not found: {self._path}") + return None + + try: + with self._path.open("r", encoding="utf-8") as f: + data = json.load(f) + + # Pass cipher in context for automatic decryption of all secret fields + context = {"cipher": self.cipher} if self.cipher else None + return Secrets.model_validate(data, context=context) + except (PermissionError, OSError) as e: + # Critical filesystem errors should be re-raised + logger.error(f"Cannot access secrets file: {e}") + raise + except json.JSONDecodeError as e: + # Corrupted file - log and return None to allow recovery + logger.error(f"Secrets file is corrupted: {e}") + return None + except Exception: + # Validation or other errors - log and return None + logger.error("Failed to load secrets", exc_info=True) + return None + + def save(self, secrets: Secrets) -> None: + """Save secrets to file atomically with secure permissions. + + If a cipher is provided, secrets are encrypted via Pydantic's + serialization context. The cipher is passed to model_dump which + flows through to field serializers using serialize_secret(). + + Warning: + This method does NOT acquire a file lock. For concurrent-safe + updates, use :meth:`set_secret` or :meth:`delete_secret` which + wrap save() with file locking. Direct calls to save() from + multiple processes may cause lost updates. + + Warning: + If no cipher is provided, secrets are stored in plaintext. + """ + _ensure_secure_directory(self.persistence_dir) + + # Pass cipher in context for automatic encryption of all secret fields + if self.cipher: + context: dict[str, Any] = {"cipher": self.cipher} + else: + context = {"expose_secrets": "plaintext"} + # Warn about plaintext secret storage (only if secrets exist) + if secrets.custom_secrets: + logger.warning( + "Saving secrets in PLAINTEXT (no cipher configured). " + "Configure OH_SECRET_KEY for production deployments." + ) + + data = secrets.model_dump(mode="json", context=context) + + _atomic_write_json(self._path, data) + logger.debug(f"Secrets saved to {self._path}") + + def get_secret(self, name: str) -> str | None: + """Get a single secret value by name. + + Uses file locking to prevent reading during concurrent writes. + """ + with _file_lock(self._lock_path): + secrets = self.load() + if secrets is None: + return None + secret = secrets.custom_secrets.get(name) + if secret is None or secret.secret is None: + return None + return secret.secret.get_secret_value() + + def set_secret(self, name: str, value: str, description: str | None = None) -> None: + """Set a single secret with file locking to prevent race conditions. + + Raises: + RuntimeError: If the secrets file exists but cannot be loaded + (e.g., corrupted JSON, decryption failure). This prevents + data loss from overwriting existing secrets with defaults. + """ + with _file_lock(self._lock_path): + secrets = self.load() + if secrets is None: + # File doesn't exist - safe to use defaults + if self._path.exists(): + # File exists but load() returned None - corrupted or unreadable + raise RuntimeError( + f"Cannot load secrets from {self._path}. " + "File may be corrupted or encrypted with a different key. " + "Refusing to overwrite with defaults to prevent data loss." + ) + secrets = Secrets() + + # Create new secrets dict with updated value + new_secrets = dict(secrets.custom_secrets) + new_secrets[name] = CustomSecret( + name=name, + secret=SecretStr(value), + description=description, + ) + + # Save with frozen model copy + self.save(Secrets(custom_secrets=new_secrets)) + + def delete_secret(self, name: str) -> bool: + """Delete a secret with file locking. Returns True if it existed. + + Raises: + RuntimeError: If the secrets file exists but cannot be loaded + (e.g., corrupted JSON, decryption failure). This prevents + data loss from overwriting existing secrets with defaults. + """ + with _file_lock(self._lock_path): + secrets = self.load() + if secrets is None: + # File doesn't exist - nothing to delete + if self._path.exists(): + # File exists but load() returned None - corrupted or unreadable + raise RuntimeError( + f"Cannot load secrets from {self._path}. " + "File may be corrupted or encrypted with a different key. " + "Refusing to modify to prevent data loss." + ) + return False + if name not in secrets.custom_secrets: + return False + + new_secrets = {k: v for k, v in secrets.custom_secrets.items() if k != name} + self.save(Secrets(custom_secrets=new_secrets)) + return True + + +# ── Global Store Access ────────────────────────────────────────────────── + +_settings_store: FileSettingsStore | None = None +_secrets_store: FileSecretsStore | None = None +_store_lock = threading.Lock() + + +def _get_persistence_dir(config: Config | None = None) -> Path: + """Get the persistence directory from config or default.""" + # Check environment variable first + env_dir = os.environ.get("OH_PERSISTENCE_DIR") + if env_dir: + return Path(env_dir) + + # Use config's conversations_path parent if available + if config is not None: + return config.conversations_path.parent / ".openhands" + + return DEFAULT_PERSISTENCE_DIR + + +def _get_cipher(config: Config | None = None) -> Cipher | None: + """Get cipher from config for encrypting secrets.""" + if config is not None: + return config.cipher + return None + + +def get_settings_store(config: Config | None = None) -> FileSettingsStore: + """Get the global settings store instance (thread-safe). + + Note: + The config parameter is only used on first initialization. + Subsequent calls return the existing instance regardless of config. + + Warning: + The cipher key (OH_SECRET_KEY) must NOT change during runtime. + The store singleton caches the cipher from first initialization. + If the cipher key changes: + - New data may be encrypted with a stale key + - Existing data may fail to decrypt + - This could trigger data loss protection in update operations + + To use a new cipher key, restart the server process. + For testing, use :func:`reset_stores` to clear the singletons. + """ + global _settings_store + if _settings_store is not None: + return _settings_store + + with _store_lock: + # Double-check after acquiring lock + if _settings_store is None: + _settings_store = FileSettingsStore( + persistence_dir=_get_persistence_dir(config), + cipher=_get_cipher(config), + ) + return _settings_store + + +def get_secrets_store(config: Config | None = None) -> FileSecretsStore: + """Get the global secrets store instance (thread-safe). + + Note: + The config parameter is only used on first initialization. + Subsequent calls return the existing instance regardless of config. + + Warning: + The cipher key (OH_SECRET_KEY) must NOT change during runtime. + The store singleton caches the cipher from first initialization. + If the cipher key changes: + - New data may be encrypted with a stale key + - Existing data may fail to decrypt + - This could trigger data loss protection in update operations + + To use a new cipher key, restart the server process. + For testing, use :func:`reset_stores` to clear the singletons. + """ + global _secrets_store + if _secrets_store is not None: + return _secrets_store + + with _store_lock: + # Double-check after acquiring lock + if _secrets_store is None: + _secrets_store = FileSecretsStore( + persistence_dir=_get_persistence_dir(config), + cipher=_get_cipher(config), + ) + return _secrets_store + + +def reset_stores() -> None: + """Reset global store instances (for testing).""" + global _settings_store, _secrets_store + with _store_lock: + _settings_store = None + _secrets_store = None diff --git a/openhands-agent-server/openhands/agent_server/settings_router.py b/openhands-agent-server/openhands/agent_server/settings_router.py index cbcd5beb8c..e2e5aee9ce 100644 --- a/openhands-agent-server/openhands/agent_server/settings_router.py +++ b/openhands-agent-server/openhands/agent_server/settings_router.py @@ -1,7 +1,20 @@ from functools import lru_cache +from typing import Any, Literal, cast -from fastapi import APIRouter +from fastapi import APIRouter, HTTPException, Request, Response, status +from pydantic import BaseModel, ValidationError +from openhands.agent_server.persistence import ( + SECRET_NAME_PATTERN, + CustomSecretCreate, + CustomSecretResponse, + PersistedSettings, + SecretsResponse, + get_secrets_store, + get_settings_store, +) +from openhands.agent_server.persistence.models import SettingsUpdatePayload +from openhands.sdk.logger import get_logger from openhands.sdk.settings import ( ConversationSettings, SettingsSchema, @@ -9,8 +22,26 @@ ) +logger = get_logger(__name__) + +# ── Route Path Constants ───────────────────────────────────────────────── +# These are relative to the router prefix (/settings). +# When mounted on /api, full paths become /api/settings, /api/settings/secrets, etc. +# Note: RemoteWorkspace (client) uses absolute paths (e.g., "/api/settings") +# while this router uses relative paths. The paths are intentionally separate +# to match their respective contexts (router prefix vs full URL path). +SETTINGS_PATH = "" # -> /api/settings +SECRETS_PATH = "/secrets" # -> /api/settings/secrets +SECRET_VALUE_PATH = "/secrets/{name}" # -> /api/settings/secrets/{name} + settings_router = APIRouter(prefix="/settings", tags=["Settings"]) +# Valid values for X-Expose-Secrets header +ExposeSecretsMode = Literal["encrypted", "plaintext"] + + +# ── Schema Endpoints ───────────────────────────────────────────────────── + @lru_cache(maxsize=1) def _get_agent_settings_schema() -> SettingsSchema: @@ -37,3 +68,388 @@ async def get_agent_settings_schema() -> SettingsSchema: async def get_conversation_settings_schema() -> SettingsSchema: """Return the schema used to render ConversationSettings-based forms.""" return _get_conversation_settings_schema() + + +# ── Settings CRUD Endpoints ────────────────────────────────────────────── + + +def _get_config(request: Request): + """Get config from app state. + + Raises: + HTTPException: 503 if config is not initialized. + """ + config = getattr(request.app.state, "config", None) + if config is None: + raise HTTPException(status_code=503, detail="Server not fully initialized") + return config + + +def _validate_secret_name(name: str) -> None: + """Validate secret name format. + + Secret names must: + - Start with a letter + - Contain only letters, numbers, and underscores + - Be 1-64 characters long + + Raises: + HTTPException: 422 if name format is invalid. + """ + if not SECRET_NAME_PATTERN.match(name): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + "Invalid secret name format. Must start with a letter, " + "contain only letters, numbers, and underscores, " + "and be 1-64 characters long." + ), + ) + + +def _parse_expose_secrets_header(request: Request) -> ExposeSecretsMode | None: + """Parse X-Expose-Secrets header value. + + Returns: + "encrypted", "plaintext", or None (if header not present or invalid). + + Raises: + HTTPException: 400 if header has invalid value. + """ + header_value = request.headers.get("X-Expose-Secrets", "").lower().strip() + + if not header_value: + return None + + # Legacy "true" value - treat as "encrypted" for safety + if header_value == "true": + return "encrypted" + + if header_value in ("encrypted", "plaintext"): + return cast(ExposeSecretsMode, header_value) + + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=( + f"Invalid X-Expose-Secrets header value: '{header_value}'. " + "Valid values are: 'encrypted', 'plaintext'." + ), + ) + + +class SettingsResponse(BaseModel): + """Response model for settings.""" + + agent_settings: dict[str, Any] + conversation_settings: dict[str, Any] + llm_api_key_is_set: bool + + +class SettingsUpdateRequest(BaseModel): + """Request model for updating settings.""" + + agent_settings_diff: dict[str, Any] | None = None + conversation_settings_diff: dict[str, Any] | None = None + + +@settings_router.get(SETTINGS_PATH, response_model=SettingsResponse) +async def get_settings(request: Request) -> SettingsResponse: + """Get current settings. + + Returns the persisted settings including agent configuration, + conversation settings, and whether an LLM API key is configured. + + Use the ``X-Expose-Secrets`` header to control secret exposure: + - ``encrypted``: Returns cipher-encrypted values (safe for frontend clients) + - ``plaintext``: Returns raw secret values (backend clients only!) + - (absent): Returns redacted values ("**********") + + Security: + When the server is configured with ``session_api_keys``, all endpoints + under ``/api`` (including this one) require the ``X-Session-API-Key`` + header. When no session API keys are configured, endpoints are open. + + **Trust model:** All authenticated clients are treated as equally + trusted. There is no role-based authorization for ``X-Expose-Secrets`` + modes—any authenticated client can request ``plaintext`` or + ``encrypted`` exposure. This design assumes: + + - All clients sharing session API keys operate in the same trust domain + - Network-level controls (firewalls, VPCs) restrict access to trusted + clients only + - Production deployments use session API keys to prevent anonymous access + + The ``plaintext`` mode exists for backend-to-backend communication + (e.g., RemoteWorkspace). Frontend clients should prefer ``encrypted`` + mode for round-tripping secrets, or omit the header to receive redacted + values. + """ + expose_mode = _parse_expose_secrets_header(request) + config = _get_config(request) + store = get_settings_store(config) + settings = store.load() or PersistedSettings() + + # Audit log all settings access for security visibility + # Use WARNING level for plaintext mode to highlight security-sensitive operations + client_host = request.client.host if request.client else "unknown" + log_extra = { + "client_host": client_host, + "expose_mode": expose_mode or "redacted", + "has_llm_api_key": settings.llm_api_key_is_set, + } + if expose_mode == "plaintext": + logger.warning("Settings accessed with PLAINTEXT secrets", extra=log_extra) + else: + logger.info("Settings accessed", extra=log_extra) + + # Build serialization context based on expose mode + if expose_mode: + context: dict[str, Any] = { + "expose_secrets": expose_mode, + "cipher": config.cipher, # Needed for "encrypted" mode + } + else: + context = {} + + try: + return SettingsResponse( + agent_settings=settings.agent_settings.model_dump( + mode="json", context=context + ), + conversation_settings=settings.conversation_settings.model_dump( + mode="json" + ), + llm_api_key_is_set=settings.llm_api_key_is_set, + ) + except Exception as e: + # Handle ValueError from serialize_secret when cipher is missing + if "no cipher configured" in str(e): + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Encryption not available: OH_SECRET_KEY is not configured", + ) + raise + + +@settings_router.patch(SETTINGS_PATH, response_model=SettingsResponse) +async def update_settings( + request: Request, payload: SettingsUpdateRequest +) -> SettingsResponse: + """Update settings with partial changes. + + Accepts ``agent_settings_diff`` and/or ``conversation_settings_diff`` + for incremental updates. Values are deep-merged with existing settings. + + Uses file locking to prevent concurrent updates from overwriting each other. + + Raises: + HTTPException: 400 if the update payload contains invalid values. + """ + config = _get_config(request) + store = get_settings_store(config) + + update_data = payload.model_dump(exclude_none=True) + if not update_data: + # No updates provided - this is a client error + raise HTTPException( + status_code=400, + detail=( + "At least one of agent_settings_diff or " + "conversation_settings_diff must be provided" + ), + ) + + # Apply updates atomically with file locking + def apply_update(settings: PersistedSettings) -> PersistedSettings: + settings.update(cast(SettingsUpdatePayload, update_data)) + return settings + + client_host = request.client.host if request.client else "unknown" + try: + settings = store.update(apply_update) + # Audit log: settings modified + logger.info( + "Settings updated", + extra={ + "client_host": client_host, + "agent_settings_modified": "agent_settings_diff" in update_data, + "conversation_settings_modified": ( + "conversation_settings_diff" in update_data + ), + }, + ) + except (ValueError, ValidationError): + # Audit log: validation failed + # Note: PersistedSettings.update() raises ValueError (sanitized message) + # while Pydantic validation raises ValidationError + logger.warning( + "Settings update validation failed", + extra={"client_host": client_host}, + ) + # 422 Unprocessable Entity - semantic validation failure + # Don't expose error details - could contain secrets in tracebacks + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="Settings validation failed", + ) + except RuntimeError as e: + # Data corruption protection triggered (file exists but unreadable) + logger.error(f"Settings update blocked: {e}") + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Settings file is corrupted or encrypted with a different key", + ) + except (OSError, PermissionError): + # Note: exc_info omitted to prevent secrets in scope from leaking in tracebacks + logger.error("Settings update failed - file I/O error") + raise HTTPException(status_code=500, detail="Failed to update settings") + + # Don't expose secrets in PATCH response (consistent with GET behavior) + return SettingsResponse( + agent_settings=settings.agent_settings.model_dump(mode="json"), + conversation_settings=settings.conversation_settings.model_dump(mode="json"), + llm_api_key_is_set=settings.llm_api_key_is_set, + ) + + +# ── Secrets CRUD Endpoints ─────────────────────────────────────────────── + + +@settings_router.get(SECRETS_PATH, response_model=SecretsResponse) +async def list_secrets(request: Request) -> SecretsResponse: + """List all available secrets (names and descriptions only, no values).""" + config = _get_config(request) + store = get_secrets_store(config) + secrets = store.load() + + client_host = request.client.host if request.client else "unknown" + secret_count = len(secrets.custom_secrets) if secrets else 0 + logger.info( + "Secrets list accessed", + extra={"client_host": client_host, "secret_count": secret_count}, + ) + + if secrets is None: + return SecretsResponse(secrets=[]) + + return SecretsResponse( + secrets=[ + CustomSecretResponse(name=name, description=secret.description) + for name, secret in secrets.custom_secrets.items() + ] + ) + + +@settings_router.get(SECRET_VALUE_PATH) +async def get_secret_value(request: Request, name: str) -> Response: + """Get a single secret value by name. + + Returns the raw secret value as plain text. This endpoint is designed + to be used with LookupSecret for lazy secret resolution. + + Raises: + HTTPException: 400 if name format is invalid, 404 if secret not found. + """ + _validate_secret_name(name) + + config = _get_config(request) + store = get_secrets_store(config) + value = store.get_secret(name) + + client_host = request.client.host if request.client else "unknown" + if value is None: + # Log failed access attempts to detect enumeration attacks + logger.warning( + "Secret access failed - not found", + extra={"secret_name": name, "client_host": client_host}, + ) + # Use generic message to prevent secret name enumeration attacks + raise HTTPException(status_code=404, detail="Secret not found") + + logger.info( + "Secret accessed", + extra={"secret_name": name, "client_host": client_host}, + ) + return Response(content=value, media_type="text/plain") + + +@settings_router.put(SECRETS_PATH, response_model=CustomSecretResponse) +async def create_secret( + request: Request, secret: CustomSecretCreate +) -> CustomSecretResponse: + """Create or update a custom secret (upsert). + + Raises: + HTTPException: 400 if secret name format is invalid, 500 if file is corrupted. + """ + _validate_secret_name(secret.name) + + config = _get_config(request) + store = get_secrets_store(config) + + try: + store.set_secret( + name=secret.name, + value=secret.value.get_secret_value(), + description=secret.description, + ) + except RuntimeError as e: + # Data corruption protection triggered (file exists but unreadable) + logger.error(f"Secret create blocked: {e}") + raise HTTPException( + status_code=500, + detail="Secrets file is corrupted or encrypted with a different key", + ) + except (OSError, PermissionError): + # Note: exc_info omitted to prevent secret values from leaking in tracebacks + logger.error("Failed to save secret - file I/O error") + raise HTTPException(status_code=500, detail="Failed to save secret") + + logger.info( + "Secret created/updated", + extra={ + "secret_name": secret.name, + "client_host": request.client.host if request.client else "unknown", + }, + ) + return CustomSecretResponse(name=secret.name, description=secret.description) + + +@settings_router.delete(SECRET_VALUE_PATH) +async def delete_secret(request: Request, name: str) -> dict[str, bool]: + """Delete a custom secret by name. + + Raises: + HTTPException: 400 if name format is invalid, 404 if secret not found, + 500 if file is corrupted. + """ + _validate_secret_name(name) + + config = _get_config(request) + store = get_secrets_store(config) + + client_host = request.client.host if request.client else "unknown" + try: + deleted = store.delete_secret(name) + except RuntimeError as e: + # Data corruption protection triggered (file exists but unreadable) + logger.error(f"Secret delete blocked: {e}") + raise HTTPException( + status_code=500, + detail="Secrets file is corrupted or encrypted with a different key", + ) + + if not deleted: + # Log failed deletion attempts to detect enumeration attacks + logger.warning( + "Secret deletion failed - not found", + extra={"secret_name": name, "client_host": client_host}, + ) + # Use generic message to prevent secret name enumeration attacks + raise HTTPException(status_code=404, detail="Secret not found") + + logger.info( + "Secret deleted", + extra={"secret_name": name, "client_host": client_host}, + ) + return {"deleted": True} diff --git a/openhands-sdk/openhands/sdk/conversation/request.py b/openhands-sdk/openhands/sdk/conversation/request.py index f8d7641834..77dabb4208 100644 --- a/openhands-sdk/openhands/sdk/conversation/request.py +++ b/openhands-sdk/openhands/sdk/conversation/request.py @@ -65,7 +65,7 @@ class _StartConversationRequestBase(BaseModel): workspace: LocalWorkspace = Field( ..., - description="Working directory for agent operations and tool execution", + description="Working directory for agent operations and tool execution.", ) conversation_id: UUID | None = Field( default=None, @@ -101,6 +101,19 @@ class _StartConversationRequestBase(BaseModel): default_factory=dict, description="Secrets available in the conversation", ) + secrets_encrypted: bool = Field( + default=False, + description=( + "If true, indicates that secret values in the agent configuration " + "are cipher-encrypted and should be decrypted by the server before " + "use. This enables secure round-tripping of settings through " + "untrusted clients (e.g., frontend) that received encrypted values " + "via the X-Expose-Secrets header. " + "Flow: client calls GET /api/settings with X-Expose-Secrets: encrypted " + "to receive cipher-encrypted secrets, then passes them in the agent " + "config with secrets_encrypted=True so the server can decrypt them." + ), + ) tool_module_qualnames: dict[str, str] = Field( default_factory=dict, description=( diff --git a/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py b/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py index 2d7e87e49c..8dde1bb609 100644 --- a/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py +++ b/openhands-sdk/openhands/sdk/utils/pydantic_secrets.py @@ -1,3 +1,6 @@ +import logging +from typing import Literal + from pydantic import SecretStr from openhands.sdk.utils.cipher import Cipher @@ -5,6 +8,11 @@ REDACTED_SECRET_VALUE = "**********" +# Type for expose_secrets context value +ExposeSecretsMode = Literal["encrypted", "plaintext"] | bool + +_logger = logging.getLogger(__name__) + def is_redacted_secret(v: str | SecretStr | None) -> bool: if v is None: @@ -16,26 +24,41 @@ def is_redacted_secret(v: str | SecretStr | None) -> bool: def serialize_secret(v: SecretStr | None, info): """ - Serialize secret fields with encryption or redaction. + Serialize secret fields with encryption, plaintext exposure, or redaction. - - If a cipher is provided in context, encrypts the secret value - - If expose_secrets flag is True in context, exposes the actual value - - Otherwise, lets Pydantic handle default masking (redaction) - - This prevents accidental secret disclosure - """ # noqa: E501 + Context options: + - ``cipher``: If provided, encrypts the secret value (takes precedence) + - ``expose_secrets``: Controls how secrets are exposed: + - ``"encrypted"``: Encrypt using cipher from context (requires cipher) + - ``"plaintext"`` or ``True``: Expose the actual value (backend use only) + - ``False`` or absent: Let Pydantic handle default masking (redaction) + + The ``"encrypted"`` mode is safe for frontend clients as they cannot decrypt. + The ``"plaintext"`` mode should only be used by trusted backend clients. + """ if v is None: return None - # check if a cipher is supplied - if info.context and info.context.get("cipher"): - cipher: Cipher = info.context.get("cipher") - return cipher.encrypt(v) + expose_mode = info.context.get("expose_secrets") if info.context else None + cipher: Cipher | None = info.context.get("cipher") if info.context else None - # Check if the 'expose_secrets' flag is in the serialization context - if info.context and info.context.get("expose_secrets"): + # Handle plaintext mode first - no encryption needed + if expose_mode == "plaintext" or expose_mode is True: return v.get_secret_value() - # Let Pydantic handle the default masking + # Handle encrypted mode (explicit or implicit via cipher presence) + # When cipher is present without explicit expose_mode, default to encryption + # This provides backward compatibility for storage operations + if expose_mode == "encrypted" or cipher: + if not cipher: + # Encrypted mode explicitly requested but no cipher available + raise ValueError( + "Cannot encrypt secret: no cipher configured. " + "Set OH_SECRET_KEY environment variable." + ) + return cipher.encrypt(v) + + # Default: let Pydantic handle masking (redaction) return v diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index 7a4078b53f..eba3e5dc5c 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -1,7 +1,60 @@ +import os +import tempfile +from base64 import urlsafe_b64encode +from pathlib import Path + +import pytest from fastapi.testclient import TestClient +from pydantic import SecretStr from openhands.agent_server.api import create_app from openhands.agent_server.config import Config +from openhands.agent_server.persistence import ( + FileSettingsStore, + PersistedSettings, + reset_stores, +) +from openhands.sdk.utils.cipher import Cipher + + +@pytest.fixture +def temp_persistence_dir(): + """Create a temporary directory for persistence files and reset stores.""" + with tempfile.TemporaryDirectory() as tmpdir: + # Reset global store singletons before test + reset_stores() + # Set environment variable for persistence directory + old_val = os.environ.get("OH_PERSISTENCE_DIR") + os.environ["OH_PERSISTENCE_DIR"] = tmpdir + yield Path(tmpdir) + # Cleanup: reset stores and restore environment + reset_stores() + if old_val is not None: + os.environ["OH_PERSISTENCE_DIR"] = old_val + else: + os.environ.pop("OH_PERSISTENCE_DIR", None) + + +@pytest.fixture +def secret_key(): + """Generate a valid Fernet key.""" + return urlsafe_b64encode(b"a" * 32).decode("ascii") + + +@pytest.fixture +def config_with_settings(temp_persistence_dir, secret_key): + """Create a config with secret key for encryption.""" + return Config( + static_files_path=None, + session_api_keys=[], + secret_key=SecretStr(secret_key), + ) + + +@pytest.fixture +def client_with_settings(config_with_settings): + """Create a test client with settings support.""" + return TestClient(create_app(config_with_settings)) def test_get_agent_settings_schema(): @@ -45,3 +98,521 @@ def test_get_conversation_settings_schema(): verification_field_keys = {field["key"] for field in verification_section["fields"]} assert "confirmation_mode" in verification_field_keys assert "security_analyzer" in verification_field_keys + + +# ── GET /api/settings tests ───────────────────────────────────────────── + + +def test_get_settings_returns_default_settings(client_with_settings): + """GET /api/settings returns default settings when none are persisted.""" + response = client_with_settings.get("/api/settings") + + assert response.status_code == 200 + body = response.json() + assert "agent_settings" in body + assert "conversation_settings" in body + assert "llm_api_key_is_set" in body + assert body["llm_api_key_is_set"] is False + + +def test_get_settings_without_header_redacts_secrets( + client_with_settings, temp_persistence_dir, secret_key +): + """GET /api/settings without X-Expose-Secrets header redacts secrets.""" + # First, save settings with a secret using the store + cipher = Cipher(secret_key) + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=cipher) + settings = PersistedSettings() + settings.agent_settings.llm.api_key = SecretStr("sk-test-secret-key") + store.save(settings) + + response = client_with_settings.get("/api/settings") + + assert response.status_code == 200 + body = response.json() + # Secret should be redacted (Pydantic default behavior) + api_key = body["agent_settings"]["llm"]["api_key"] + assert api_key == "**********" + assert body["llm_api_key_is_set"] is True + + +def test_get_settings_with_plaintext_header_exposes_secrets( + client_with_settings, temp_persistence_dir, secret_key +): + """GET /api/settings with X-Expose-Secrets: plaintext returns raw secrets.""" + # Save settings with a secret + cipher = Cipher(secret_key) + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=cipher) + settings = PersistedSettings() + settings.agent_settings.llm.api_key = SecretStr("sk-test-secret-key") + store.save(settings) + + response = client_with_settings.get( + "/api/settings", headers={"X-Expose-Secrets": "plaintext"} + ) + + assert response.status_code == 200 + body = response.json() + # Secret should be exposed + api_key = body["agent_settings"]["llm"]["api_key"] + assert api_key == "sk-test-secret-key" + + +def test_get_settings_with_encrypted_header_encrypts_secrets( + client_with_settings, temp_persistence_dir, secret_key +): + """GET /api/settings with X-Expose-Secrets: encrypted returns encrypted secrets.""" + # Save settings with a secret + cipher = Cipher(secret_key) + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=cipher) + settings = PersistedSettings() + settings.agent_settings.llm.api_key = SecretStr("sk-test-secret-key") + store.save(settings) + + response = client_with_settings.get( + "/api/settings", headers={"X-Expose-Secrets": "encrypted"} + ) + + assert response.status_code == 200 + body = response.json() + api_key = body["agent_settings"]["llm"]["api_key"] + # Should be encrypted (not plaintext, not redacted) + assert api_key != "sk-test-secret-key" + assert api_key != "**********" + # Should be decryptable + decrypted = cipher.decrypt(api_key) + assert decrypted is not None + assert decrypted.get_secret_value() == "sk-test-secret-key" + + +def test_get_settings_with_true_header_treats_as_encrypted( + client_with_settings, temp_persistence_dir, secret_key +): + """GET /api/settings with X-Expose-Secrets: true treats as encrypted (safety).""" + # Save settings with a secret + cipher = Cipher(secret_key) + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=cipher) + settings = PersistedSettings() + settings.agent_settings.llm.api_key = SecretStr("sk-test-secret-key") + store.save(settings) + + response = client_with_settings.get( + "/api/settings", headers={"X-Expose-Secrets": "true"} + ) + + assert response.status_code == 200 + body = response.json() + api_key = body["agent_settings"]["llm"]["api_key"] + # Should be encrypted (not plaintext) + assert api_key != "sk-test-secret-key" + # Should be decryptable + decrypted = cipher.decrypt(api_key) + assert decrypted is not None + assert decrypted.get_secret_value() == "sk-test-secret-key" + + +def test_get_settings_with_invalid_header_returns_400(client_with_settings): + """GET /api/settings with invalid X-Expose-Secrets value returns 400.""" + response = client_with_settings.get( + "/api/settings", headers={"X-Expose-Secrets": "invalid-value"} + ) + + assert response.status_code == 400 + assert "Invalid X-Expose-Secrets header" in response.json()["detail"] + + +# ── PATCH /api/settings tests ─────────────────────────────────────────── + + +def test_patch_settings_updates_llm_config(client_with_settings): + """PATCH /api/settings can update LLM configuration.""" + response = client_with_settings.patch( + "/api/settings", + json={ + "agent_settings_diff": {"llm": {"model": "gpt-4o", "api_key": "sk-new-key"}} + }, + ) + + assert response.status_code == 200 + body = response.json() + assert body["agent_settings"]["llm"]["model"] == "gpt-4o" + # Response should NOT expose secrets (no header) + assert body["agent_settings"]["llm"]["api_key"] == "**********" + assert body["llm_api_key_is_set"] is True + + +def test_patch_settings_empty_payload_returns_400(client_with_settings): + """PATCH /api/settings with empty payload returns 400.""" + response = client_with_settings.patch("/api/settings", json={}) + + assert response.status_code == 400 + assert "At least one of" in response.json()["detail"] + + +def test_patch_settings_deep_merges(client_with_settings): + """PATCH /api/settings deep-merges with existing settings.""" + # First update: set model + client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "gpt-4o"}}}, + ) + + # Second update: set api_key (should preserve model) + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"api_key": "sk-test-key"}}}, + ) + + assert response.status_code == 200 + body = response.json() + assert body["agent_settings"]["llm"]["model"] == "gpt-4o" + assert body["llm_api_key_is_set"] is True + + +# ── Secrets CRUD tests ────────────────────────────────────────────────── + + +def test_list_secrets_empty(client_with_settings): + """GET /api/settings/secrets returns empty list when no secrets exist.""" + response = client_with_settings.get("/api/settings/secrets") + + assert response.status_code == 200 + body = response.json() + assert body["secrets"] == [] + + +def test_create_and_list_secrets(client_with_settings): + """PUT /api/settings/secrets creates a secret, GET lists it.""" + # Create a secret + create_response = client_with_settings.put( + "/api/settings/secrets", + json={"name": "MY_SECRET", "value": "secret-value", "description": "Test"}, + ) + + assert create_response.status_code == 200 + assert create_response.json()["name"] == "MY_SECRET" + assert create_response.json()["description"] == "Test" + + # List secrets (should NOT include value) + list_response = client_with_settings.get("/api/settings/secrets") + + assert list_response.status_code == 200 + secrets = list_response.json()["secrets"] + assert len(secrets) == 1 + assert secrets[0]["name"] == "MY_SECRET" + assert secrets[0]["description"] == "Test" + assert "value" not in secrets[0] + + +def test_get_secret_value(client_with_settings): + """GET /api/settings/secrets/{name} returns the raw secret value.""" + # Create a secret + client_with_settings.put( + "/api/settings/secrets", + json={"name": "MY_SECRET", "value": "secret-value-123"}, + ) + + # Get the secret value + response = client_with_settings.get("/api/settings/secrets/MY_SECRET") + + assert response.status_code == 200 + assert response.text == "secret-value-123" + assert response.headers["content-type"] == "text/plain; charset=utf-8" + + +def test_get_secret_value_not_found(client_with_settings): + """GET /api/settings/secrets/{name} returns 404 for nonexistent secret.""" + response = client_with_settings.get("/api/settings/secrets/NONEXISTENT") + + assert response.status_code == 404 + + +def test_delete_secret(client_with_settings): + """DELETE /api/settings/secrets/{name} deletes the secret.""" + # Create a secret + client_with_settings.put( + "/api/settings/secrets", + json={"name": "MY_SECRET", "value": "secret-value"}, + ) + + # Delete it + delete_response = client_with_settings.delete("/api/settings/secrets/MY_SECRET") + assert delete_response.status_code == 200 + assert delete_response.json()["deleted"] is True + + # Verify it's gone + get_response = client_with_settings.get("/api/settings/secrets/MY_SECRET") + assert get_response.status_code == 404 + + +def test_secret_name_validation(client_with_settings): + """PUT /api/settings/secrets validates secret name format.""" + # Invalid: starts with number + response = client_with_settings.put( + "/api/settings/secrets", + json={"name": "123_invalid", "value": "test"}, + ) + assert response.status_code == 422 + + # Invalid: contains special characters + response = client_with_settings.put( + "/api/settings/secrets", + json={"name": "invalid-name", "value": "test"}, + ) + assert response.status_code == 422 + + # Valid: starts with letter, alphanumeric + underscore + response = client_with_settings.put( + "/api/settings/secrets", + json={"name": "VALID_NAME_123", "value": "test"}, + ) + assert response.status_code == 200 + + +# ── PATCH validation and error handling tests ─────────────────────────── + + +def test_patch_settings_validation_error_returns_422(client_with_settings): + """PATCH /api/settings with invalid data returns 422.""" + # Invalid: negative max_iterations + response = client_with_settings.patch( + "/api/settings", + json={"conversation_settings_diff": {"max_iterations": -5}}, + ) + assert response.status_code == 422 + # Error message should be sanitized (not expose secrets) + assert response.json()["detail"] == "Settings validation failed" + + +def test_patch_settings_validation_error_does_not_leak_secrets(client_with_settings): + """PATCH validation errors don't leak secret values in error messages.""" + # Try to update with invalid model value (causes validation to fail) + # This tests that even if the API key was in memory during validation, + # it doesn't appear in error messages + response = client_with_settings.patch( + "/api/settings", + json={ + "agent_settings_diff": { + "llm": { + "api_key": "sk-secret-value", + "model": "", + } # Empty model is invalid + } + }, + ) + # Should return 422 with sanitized message + assert response.status_code == 422 + # The error message should be sanitized - NOT contain the secret value + error_detail = response.json()["detail"] + assert "sk-secret-value" not in error_detail + # And it should be the generic sanitized message + assert error_detail == "Settings validation failed" + + +def test_secret_upsert_updates_existing(client_with_settings): + """PUT /api/settings/secrets updates existing secret (upsert behavior).""" + # Create initial secret + client_with_settings.put( + "/api/settings/secrets", + json={ + "name": "MY_SECRET", + "value": "original-value", + "description": "Original", + }, + ) + + # Update the secret (same name, new value) + update_response = client_with_settings.put( + "/api/settings/secrets", + json={"name": "MY_SECRET", "value": "updated-value", "description": "Updated"}, + ) + assert update_response.status_code == 200 + assert update_response.json()["description"] == "Updated" + + # Verify the value was updated + get_response = client_with_settings.get("/api/settings/secrets/MY_SECRET") + assert get_response.status_code == 200 + assert get_response.text == "updated-value" + + +def test_secret_name_validation_on_get(client_with_settings): + """GET /api/settings/secrets/{name} validates name format.""" + # Invalid name format + response = client_with_settings.get("/api/settings/secrets/123_invalid") + assert response.status_code == 422 + + +def test_secret_name_validation_on_delete(client_with_settings): + """DELETE /api/settings/secrets/{name} validates name format.""" + # Invalid name format + response = client_with_settings.delete("/api/settings/secrets/invalid-name") + assert response.status_code == 422 + + +# ── Concurrent update tests ──────────────────────────────────────────────── + + +def test_concurrent_patch_updates_preserve_data(client_with_settings): + """PATCH /api/settings handles concurrent updates without data loss. + + Tests that multiple sequential PATCH requests don't corrupt settings + or lose updates due to race conditions in the file locking mechanism. + """ + from concurrent.futures import ThreadPoolExecutor, as_completed + + # Initialize settings + client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "initial-model"}}}, + ) + + results = [] + errors = [] + + def update_settings(model_name: str): + """Make a PATCH request to update the model.""" + try: + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": model_name}}}, + ) + return (model_name, response.status_code) + except Exception as e: + return (model_name, str(e)) + + # Run concurrent updates + with ThreadPoolExecutor(max_workers=5) as executor: + futures = [executor.submit(update_settings, f"model-{i}") for i in range(10)] + for future in as_completed(futures): + result = future.result() + results.append(result) + if result[1] != 200: + errors.append(result) + + # All requests should succeed (file locking should serialize them) + assert len(errors) == 0, f"Some requests failed: {errors}" + + # Final state should be consistent (one of the model values) + final_response = client_with_settings.get("/api/settings") + assert final_response.status_code == 200 + final_model = final_response.json()["agent_settings"]["llm"]["model"] + # The final value should be one of the values we set (not corrupted) + assert final_model.startswith("model-"), f"Unexpected model value: {final_model}" + + +# ── Error handling tests ─────────────────────────────────────────────────── + + +def test_get_settings_encrypted_mode_without_cipher_returns_503(temp_persistence_dir): + """GET /api/settings with X-Expose-Secrets: encrypted without cipher returns 503. + + When OH_SECRET_KEY is not set, config.cipher is None and requesting + encrypted mode should fail fast with a clear error (503 Service Unavailable). + """ + # Create a config WITHOUT secret_key (cipher will be None) + config = Config( + static_files_path=None, + session_api_keys=[], + secret_key=None, # No cipher! + ) + client = TestClient(create_app(config)) + + # First, verify we can create settings (no cipher needed for plaintext) + # Note: Without cipher, we need to manually create a settings file + store = FileSettingsStore(persistence_dir=temp_persistence_dir, cipher=None) + settings = PersistedSettings() + settings.agent_settings.llm.api_key = SecretStr("sk-test-secret-key") + store.save(settings) + + # Now request encrypted mode - should fail because no cipher + response = client.get("/api/settings", headers={"X-Expose-Secrets": "encrypted"}) + + # Should return 503 (service unavailable - encryption not configured) + assert response.status_code == 503 + body = response.json() + # Error message may be in 'detail' or 'exception' depending on error handler config + error_text = body.get("detail", "") + body.get("exception", "") + assert "OH_SECRET_KEY" in error_text + + +def test_patch_settings_corrupted_file_returns_409( + client_with_settings, temp_persistence_dir +): + """PATCH /api/settings returns 409 when settings file is corrupted. + + Tests the RuntimeError handling path that catches corruption or + encryption key mismatches. + """ + # Initialize valid settings first + client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "gpt-4"}}}, + ) + + # Corrupt the settings file directly + settings_file = temp_persistence_dir / "settings.json" + settings_file.write_text("{ this is not valid JSON !!!}") + + # Attempt to update - should fail with 409 (corruption detected) + response = client_with_settings.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "gpt-4o"}}}, + ) + + # RuntimeError from store.update() should be caught and returned as 409 + assert response.status_code == 409 + assert "corrupted" in response.json()["detail"].lower() + + +# ── Corrupted secrets file tests ─────────────────────────────────────────── + + +def test_create_secret_corrupted_file_returns_500( + client_with_settings, temp_persistence_dir +): + """PUT /api/settings/secrets returns 500 when secrets file is corrupted. + + Tests that the data loss protection path is triggered when set_secret() + encounters a corrupted secrets file. + """ + # Create initial secret + client_with_settings.put( + "/api/settings/secrets", + json={"name": "MY_SECRET", "value": "test"}, + ) + + # Corrupt the secrets file + secrets_file = temp_persistence_dir / "secrets.json" + secrets_file.write_text("{ corrupted !!!}") + + # Attempt to create new secret - should fail to prevent data loss + response = client_with_settings.put( + "/api/settings/secrets", + json={"name": "OTHER_SECRET", "value": "value"}, + ) + + assert response.status_code == 500 + + +def test_delete_secret_corrupted_file_returns_500( + client_with_settings, temp_persistence_dir +): + """DELETE /api/settings/secrets returns 500 when secrets file is corrupted. + + Tests that the data loss protection path is triggered when delete_secret() + encounters a corrupted secrets file. + """ + # Create initial secret + client_with_settings.put( + "/api/settings/secrets", + json={"name": "MY_SECRET", "value": "test"}, + ) + + # Corrupt the secrets file + secrets_file = temp_persistence_dir / "secrets.json" + secrets_file.write_text("{ corrupted !!!}") + + # Attempt to delete secret - should fail to prevent data loss + response = client_with_settings.delete("/api/settings/secrets/MY_SECRET") + + assert response.status_code == 500 diff --git a/tests/cross/test_remote_conversation_live_server.py b/tests/cross/test_remote_conversation_live_server.py index 84c0f38445..7508260cf0 100644 --- a/tests/cross/test_remote_conversation_live_server.py +++ b/tests/cross/test_remote_conversation_live_server.py @@ -1767,3 +1767,129 @@ def fake_completion( assert obs_text.rstrip().endswith("relative to that directory.") conv.close() + + +def test_settings_and_secrets_api_with_live_server(server_env): + """End-to-end test for settings and secrets API endpoints. + + Validates the full REST API for settings and secrets management + through the live agent-server, including: + - GET/PATCH settings + - GET/PUT/DELETE secrets + - Secret name validation + - Encryption/decryption round-trip + """ + with httpx.Client(base_url=server_env["host"], timeout=10.0) as client: + # ── Test settings endpoints ──────────────────────────────────────── + # GET settings (initial state) + get_resp = client.get("/api/settings") + assert get_resp.status_code == 200 + initial = get_resp.json() + assert "agent_settings" in initial + assert "conversation_settings" in initial + assert "llm_api_key_is_set" in initial + + # PATCH settings (update LLM model) + patch_resp = client.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "gpt-4o"}}}, + ) + assert patch_resp.status_code == 200 + patched = patch_resp.json() + assert patched["agent_settings"]["llm"]["model"] == "gpt-4o" + + # ── Test secrets CRUD endpoints ──────────────────────────────────── + # List secrets (should be empty initially) + list_resp = client.get("/api/settings/secrets") + assert list_resp.status_code == 200 + assert list_resp.json()["secrets"] == [] + + # Create a secret + create_resp = client.put( + "/api/settings/secrets", + json={ + "name": "TEST_API_KEY", + "value": "sk-test-live-server-12345", + "description": "Test API key for live server test", + }, + ) + assert create_resp.status_code == 200 + created = create_resp.json() + assert created["name"] == "TEST_API_KEY" + assert created["description"] == "Test API key for live server test" + + # List secrets again (should have one) + list_resp = client.get("/api/settings/secrets") + assert list_resp.status_code == 200 + secrets = list_resp.json()["secrets"] + assert len(secrets) == 1 + assert secrets[0]["name"] == "TEST_API_KEY" + # Value should NOT be returned in list + assert "value" not in secrets[0] + + # Get secret value + value_resp = client.get("/api/settings/secrets/TEST_API_KEY") + assert value_resp.status_code == 200 + assert value_resp.text == "sk-test-live-server-12345" + + # Update the secret (upsert) + update_resp = client.put( + "/api/settings/secrets", + json={ + "name": "TEST_API_KEY", + "value": "sk-updated-value", + "description": "Updated description", + }, + ) + assert update_resp.status_code == 200 + + # Verify updated value + value_resp = client.get("/api/settings/secrets/TEST_API_KEY") + assert value_resp.status_code == 200 + assert value_resp.text == "sk-updated-value" + + # Create another secret + client.put( + "/api/settings/secrets", + json={"name": "ANOTHER_SECRET", "value": "another-value"}, + ) + list_resp = client.get("/api/settings/secrets") + assert len(list_resp.json()["secrets"]) == 2 + + # Delete one secret + delete_resp = client.delete("/api/settings/secrets/TEST_API_KEY") + assert delete_resp.status_code == 200 + assert delete_resp.json()["deleted"] is True + + # Verify deleted + get_deleted_resp = client.get("/api/settings/secrets/TEST_API_KEY") + assert get_deleted_resp.status_code == 404 + + # ── Test secret name validation ──────────────────────────────────── + # Invalid name: starts with number + invalid_resp = client.put( + "/api/settings/secrets", + json={"name": "123_invalid", "value": "test"}, + ) + assert invalid_resp.status_code == 422 + + # Invalid name: contains special characters + invalid_resp = client.put( + "/api/settings/secrets", + json={"name": "invalid-name", "value": "test"}, + ) + assert invalid_resp.status_code == 422 + + # ── Test settings with encrypted secrets ─────────────────────────── + # Update LLM API key + patch_resp = client.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"api_key": "sk-live-test-key"}}}, + ) + assert patch_resp.status_code == 200 + assert patch_resp.json()["llm_api_key_is_set"] is True + # Response should redact the key (no X-Expose-Secrets header) + assert patch_resp.json()["agent_settings"]["llm"]["api_key"] == "**********" + + # Cleanup + client.delete("/api/settings/secrets/ANOTHER_SECRET") diff --git a/tests/sdk/utils/test_pydantic_secrets.py b/tests/sdk/utils/test_pydantic_secrets.py new file mode 100644 index 0000000000..f0d502e1a4 --- /dev/null +++ b/tests/sdk/utils/test_pydantic_secrets.py @@ -0,0 +1,419 @@ +"""Tests for pydantic_secrets serialization and validation utilities.""" + +from base64 import urlsafe_b64encode +from unittest.mock import MagicMock + +import pytest +from pydantic import SecretStr + +from openhands.sdk.utils.cipher import Cipher +from openhands.sdk.utils.pydantic_secrets import ( + REDACTED_SECRET_VALUE, + is_redacted_secret, + serialize_secret, + validate_secret, +) + + +@pytest.fixture +def cipher(): + """Create a cipher for testing.""" + key = urlsafe_b64encode(b"a" * 32).decode("ascii") + return Cipher(key) + + +@pytest.fixture +def mock_info(): + """Create a mock SerializationInfo/ValidationInfo.""" + + def create_info(context=None): + info = MagicMock() + info.context = context + return info + + return create_info + + +# ── is_redacted_secret tests ──────────────────────────────────────────── + + +def test_is_redacted_secret_with_redacted_string(): + assert is_redacted_secret(REDACTED_SECRET_VALUE) is True + + +def test_is_redacted_secret_with_redacted_secretstr(): + assert is_redacted_secret(SecretStr(REDACTED_SECRET_VALUE)) is True + + +def test_is_redacted_secret_with_normal_string(): + assert is_redacted_secret("sk-test-123") is False + + +def test_is_redacted_secret_with_normal_secretstr(): + assert is_redacted_secret(SecretStr("sk-test-123")) is False + + +def test_is_redacted_secret_with_none(): + assert is_redacted_secret(None) is False + + +# ── serialize_secret tests ────────────────────────────────────────────── + + +def test_serialize_secret_none_returns_none(mock_info): + result = serialize_secret(None, mock_info({})) + assert result is None + + +def test_serialize_secret_no_context_returns_secretstr(mock_info): + """Without context, return SecretStr for Pydantic default masking.""" + secret = SecretStr("sk-test-123") + result = serialize_secret(secret, mock_info(None)) + assert isinstance(result, SecretStr) + assert result.get_secret_value() == "sk-test-123" + + +def test_serialize_secret_empty_context_returns_secretstr(mock_info): + """Empty context = no exposure, return SecretStr.""" + secret = SecretStr("sk-test-123") + result = serialize_secret(secret, mock_info({})) + assert isinstance(result, SecretStr) + + +def test_serialize_secret_plaintext_mode(mock_info): + """expose_secrets='plaintext' returns raw value.""" + secret = SecretStr("sk-test-123") + result = serialize_secret(secret, mock_info({"expose_secrets": "plaintext"})) + assert result == "sk-test-123" + + +def test_serialize_secret_plaintext_mode_bool_true(mock_info): + """expose_secrets=True (legacy) returns raw value.""" + secret = SecretStr("sk-test-123") + result = serialize_secret(secret, mock_info({"expose_secrets": True})) + assert result == "sk-test-123" + + +def test_serialize_secret_encrypted_mode_with_cipher(mock_info, cipher): + """expose_secrets='encrypted' with cipher encrypts the value.""" + secret = SecretStr("sk-test-123") + result = serialize_secret( + secret, mock_info({"expose_secrets": "encrypted", "cipher": cipher}) + ) + # Should be encrypted (not plaintext, not redacted) + assert result != "sk-test-123" + assert result != REDACTED_SECRET_VALUE + assert isinstance(result, str) + # Should be decryptable + decrypted = cipher.decrypt(result) + assert decrypted.get_secret_value() == "sk-test-123" + + +def test_serialize_secret_encrypted_mode_without_cipher_raises_error( + mock_info, +): + """expose_secrets='encrypted' without cipher raises ValueError.""" + secret = SecretStr("sk-test-123") + with pytest.raises(ValueError, match="no cipher configured"): + serialize_secret(secret, mock_info({"expose_secrets": "encrypted"})) + + +def test_serialize_secret_cipher_without_expose_mode_encrypts(mock_info, cipher): + """Cipher in context without expose_secrets still encrypts (backward compat).""" + secret = SecretStr("sk-test-123") + result = serialize_secret(secret, mock_info({"cipher": cipher})) + assert result != "sk-test-123" + # Should be decryptable + decrypted = cipher.decrypt(result) + assert decrypted.get_secret_value() == "sk-test-123" + + +def test_serialize_secret_cipher_with_plaintext_mode_returns_plaintext( + mock_info, cipher +): + """expose_secrets='plaintext' overrides cipher - returns raw value.""" + secret = SecretStr("sk-test-123") + result = serialize_secret( + secret, mock_info({"expose_secrets": "plaintext", "cipher": cipher}) + ) + assert result == "sk-test-123" + + +def test_serialize_secret_cipher_with_bool_true_returns_plaintext(mock_info, cipher): + """expose_secrets=True (legacy boolean) overrides cipher - returns raw value. + + This tests backward compatibility: when expose_secrets=True is passed with + a cipher, it should return plaintext instead of encrypting. + """ + secret = SecretStr("sk-test-123") + result = serialize_secret( + secret, mock_info({"expose_secrets": True, "cipher": cipher}) + ) + # Should be plaintext, not encrypted + assert result == "sk-test-123" + + +# ── validate_secret tests ─────────────────────────────────────────────── + + +def test_validate_secret_none_returns_none(mock_info): + result = validate_secret(None, mock_info({})) + assert result is None + + +def test_validate_secret_invalid_type_int_raises_error(mock_info): + """validate_secret raises TypeError for invalid int type. + + The function signature expects str | SecretStr | None. Passing an int + fails when trying to call .strip() on the value. + """ + with pytest.raises((TypeError, AttributeError)): + validate_secret(123, mock_info({})) # type: ignore[arg-type] + + +def test_validate_secret_invalid_type_dict_returns_none(mock_info): + """validate_secret handles empty dict gracefully (returns None). + + Empty dict is falsy, so it's treated as empty/missing secret. + Note: Non-empty dicts would fail when .strip() is called. + """ + result = validate_secret({}, mock_info({})) # type: ignore[arg-type] + assert result is None + + +def test_validate_secret_invalid_type_list_returns_none(mock_info): + """validate_secret handles empty list gracefully (returns None). + + Empty list is falsy, so it's treated as empty/missing secret. + Note: Non-empty lists would fail when .strip() is called. + """ + result = validate_secret([], mock_info({})) # type: ignore[arg-type] + assert result is None + + +def test_validate_secret_nonempty_dict_raises_error(mock_info): + """validate_secret raises error for non-empty dict (invalid type).""" + with pytest.raises((TypeError, AttributeError)): + validate_secret({"key": "value"}, mock_info({})) # type: ignore[arg-type] + + +def test_validate_secret_nonempty_list_raises_error(mock_info): + """validate_secret raises error for non-empty list (invalid type).""" + with pytest.raises((TypeError, AttributeError)): + validate_secret(["value"], mock_info({})) # type: ignore[arg-type] + + +def test_validate_secret_string_returns_secretstr(mock_info): + result = validate_secret("sk-test-123", mock_info({})) + assert isinstance(result, SecretStr) + assert result.get_secret_value() == "sk-test-123" + + +def test_validate_secret_secretstr_passthrough(mock_info): + secret = SecretStr("sk-test-123") + result = validate_secret(secret, mock_info({})) + assert isinstance(result, SecretStr) + assert result.get_secret_value() == "sk-test-123" + + +def test_validate_secret_empty_string_returns_none(mock_info): + result = validate_secret("", mock_info({})) + assert result is None + + +def test_validate_secret_whitespace_only_returns_none(mock_info): + result = validate_secret(" ", mock_info({})) + assert result is None + + +def test_validate_secret_redacted_value_returns_none(mock_info): + result = validate_secret(REDACTED_SECRET_VALUE, mock_info({})) + assert result is None + + +def test_validate_secret_with_cipher_decrypts(mock_info, cipher): + """Cipher in context triggers decryption.""" + secret = SecretStr("sk-test-123") + encrypted = cipher.encrypt(secret) + + result = validate_secret(encrypted, mock_info({"cipher": cipher})) + assert isinstance(result, SecretStr) + assert result.get_secret_value() == "sk-test-123" + + +def test_validate_secret_with_cipher_invalid_data_returns_none(mock_info, cipher): + """Invalid encrypted data with cipher returns None (graceful failure).""" + result = validate_secret("not-encrypted-data", mock_info({"cipher": cipher})) + assert result is None + + +def test_validate_secret_with_cipher_wrong_key_returns_none(mock_info, cipher): + """Wrong cipher key returns None (graceful failure).""" + # Encrypt with one key + secret = SecretStr("sk-test-123") + encrypted = cipher.encrypt(secret) + + # Try to decrypt with different key + other_key = urlsafe_b64encode(b"b" * 32).decode("ascii") + other_cipher = Cipher(other_key) + + result = validate_secret(encrypted, mock_info({"cipher": other_cipher})) + assert result is None + + +# ── Round-trip tests ──────────────────────────────────────────────────── + + +def test_roundtrip_encrypted_mode(mock_info, cipher): + """Full round-trip: serialize with encrypted mode, validate with cipher.""" + original = SecretStr("sk-test-api-key-12345") + + # Serialize with encrypted mode + encrypted = serialize_secret( + original, mock_info({"expose_secrets": "encrypted", "cipher": cipher}) + ) + assert encrypted != "sk-test-api-key-12345" + + # Validate (decrypt) with cipher + decrypted = validate_secret(encrypted, mock_info({"cipher": cipher})) + assert decrypted is not None + assert decrypted.get_secret_value() == "sk-test-api-key-12345" + + +def test_roundtrip_plaintext_mode(mock_info): + """Round-trip with plaintext mode (no encryption).""" + original = SecretStr("sk-test-api-key-12345") + + # Serialize with plaintext mode + plaintext = serialize_secret(original, mock_info({"expose_secrets": "plaintext"})) + assert plaintext == "sk-test-api-key-12345" + + # Validate (just wraps in SecretStr) + result = validate_secret(plaintext, mock_info({})) + assert result is not None + assert result.get_secret_value() == "sk-test-api-key-12345" + + +# ── Real Pydantic integration tests ───────────────────────────────────── + + +def test_real_pydantic_roundtrip_encrypted(cipher): + """Test encryption via actual Pydantic serialization (not mocks).""" + from openhands.agent_server.persistence.models import CustomSecret + + # Create with plaintext + secret = CustomSecret(name="TEST_KEY", secret=SecretStr("my-secret-value")) + + # Serialize with encrypted context (real model_dump call) + data = secret.model_dump( + mode="json", context={"expose_secrets": "encrypted", "cipher": cipher} + ) + + # Verify encrypted (not plaintext, not redacted) + assert data["secret"] != "my-secret-value" + assert data["secret"] != REDACTED_SECRET_VALUE + assert isinstance(data["secret"], str) + + # Validate (decrypt) with cipher context (real model_validate call) + restored = CustomSecret.model_validate(data, context={"cipher": cipher}) + assert restored.secret is not None + assert restored.secret.get_secret_value() == "my-secret-value" + + +def test_real_pydantic_roundtrip_plaintext(): + """Test plaintext via actual Pydantic serialization (not mocks).""" + from openhands.agent_server.persistence.models import CustomSecret + + # Create with plaintext + secret = CustomSecret(name="TEST_KEY", secret=SecretStr("my-secret-value")) + + # Serialize with plaintext context + data = secret.model_dump(mode="json", context={"expose_secrets": "plaintext"}) + + # Verify plaintext + assert data["secret"] == "my-secret-value" + + # Validate (no cipher - just wraps in SecretStr) + restored = CustomSecret.model_validate(data) + assert restored.secret is not None + assert restored.secret.get_secret_value() == "my-secret-value" + + +def test_real_pydantic_redacted_mode(): + """Test redaction via actual Pydantic serialization (default behavior).""" + from openhands.agent_server.persistence.models import CustomSecret + + # Create with plaintext + secret = CustomSecret(name="TEST_KEY", secret=SecretStr("my-secret-value")) + + # Serialize without context (default = redacted) + data = secret.model_dump(mode="json") + + # Verify redacted - Pydantic returns SecretStr repr for json mode + # which is "**********" (the default SecretStr repr) + assert data["secret"] == REDACTED_SECRET_VALUE + + +def test_real_pydantic_nested_secrets_roundtrip(cipher): + """Test encryption of nested secrets in Secrets model.""" + from openhands.agent_server.persistence.models import CustomSecret, Secrets + + # Create Secrets with multiple custom secrets + secrets = Secrets( + custom_secrets={ + "API_KEY": CustomSecret( + name="API_KEY", secret=SecretStr("sk-123"), description="API key" + ), + "DB_PASS": CustomSecret( + name="DB_PASS", + secret=SecretStr("password123"), + description="DB password", + ), + } + ) + + # Serialize with cipher (encrypts all secrets) + data = secrets.model_dump(mode="json", context={"cipher": cipher}) + + # Verify all secrets are encrypted + for name in ["API_KEY", "DB_PASS"]: + assert data["custom_secrets"][name]["secret"] not in [ + "sk-123", + "password123", + REDACTED_SECRET_VALUE, + ] + + # Validate (decrypt) all secrets + restored = Secrets.model_validate(data, context={"cipher": cipher}) + assert restored.custom_secrets["API_KEY"].secret is not None + assert restored.custom_secrets["API_KEY"].secret.get_secret_value() == "sk-123" + assert restored.custom_secrets["DB_PASS"].secret is not None + assert restored.custom_secrets["DB_PASS"].secret.get_secret_value() == "password123" + + +def test_real_pydantic_persisted_settings_roundtrip(cipher): + """Test PersistedSettings serialization with encrypted LLM api_key. + + This tests the primary use case: full PersistedSettings with + agent_settings.llm.api_key encrypted and round-tripped. + """ + from openhands.agent_server.persistence.models import PersistedSettings + + # Create settings with secret + settings = PersistedSettings() + settings.agent_settings.llm.api_key = SecretStr("sk-test-key-12345") + + # Serialize with cipher + data = settings.model_dump(mode="json", context={"cipher": cipher}) + encrypted_key = data["agent_settings"]["llm"]["api_key"] + + # Should be encrypted (not plaintext, not redacted) + assert encrypted_key != "sk-test-key-12345" + assert encrypted_key != REDACTED_SECRET_VALUE + + # Deserialize (decrypt) + restored = PersistedSettings.model_validate(data, context={"cipher": cipher}) + restored_key = restored.agent_settings.llm.api_key + assert restored_key is not None + assert isinstance(restored_key, SecretStr) + assert restored_key.get_secret_value() == "sk-test-key-12345"