From 2c0c18c030842d9bf07e8314ab56c894cb88231a Mon Sep 17 00:00:00 2001 From: openhands Date: Sat, 22 Nov 2025 16:53:33 +0000 Subject: [PATCH] Add server-side automatic conversion of string secrets to StaticSecret - Add field validator to UpdateSecretsRequest.secrets that automatically converts plain strings to StaticSecret objects - Validator handles multiple input formats: plain strings, dict with value field, proper SecretSource objects - Enables backward compatibility for clients sending plain string secrets - Add comprehensive unit tests for model validation (7 test cases) - Add integration tests for API endpoint with string conversion (2 test cases) - All tests passing with proper type safety Co-authored-by: openhands --- .../openhands/agent_server/models.py | 38 +++++- .../agent_server/test_conversation_router.py | 114 +++++++++++++++++ tests/agent_server/test_models.py | 115 ++++++++++++++++++ 3 files changed, 265 insertions(+), 2 deletions(-) create mode 100644 tests/agent_server/test_models.py diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index 7c85e13a7d..8af39e2c89 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -1,10 +1,10 @@ from abc import ABC from datetime import datetime from enum import Enum -from typing import Literal +from typing import Any, Literal from uuid import uuid4 -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator from openhands.agent_server.utils import OpenHandsUUID, utc_now from openhands.sdk import LLM, AgentBase, Event, ImageContent, Message, TextContent @@ -159,6 +159,40 @@ class UpdateSecretsRequest(BaseModel): description="Dictionary mapping secret keys to values" ) + @field_validator("secrets", mode="before") + @classmethod + def convert_string_secrets(cls, v: Any) -> dict[str, Any]: + """Convert plain string secrets to StaticSecret objects. + + This allows clients to send either: + - Plain strings: {"API_KEY": "secret-value"} + - SecretSource objects: {"API_KEY": {"kind": "StaticSecret", "value": "..."}} + """ + if not isinstance(v, dict): + return v + + result = {} + for key, value in v.items(): + if isinstance(value, str): + # Convert plain string to StaticSecret dict format + result[key] = {"kind": "StaticSecret", "value": value} + elif isinstance(value, dict) and "kind" not in value: + # Handle case where client sends {"value": "secret"} without kind + if "value" in value: + result[key] = { + "kind": "StaticSecret", + "value": value["value"], + "description": value.get("description"), + } + else: + # Invalid dict format - add a default kind to prevent KeyError + # This will still fail validation later with a better error message + result[key] = {"kind": "StaticSecret", **value} + else: + # Already a SecretSource object or proper dict format + result[key] = value + return result + class SetConfirmationPolicyRequest(BaseModel): """Payload to set confirmation policy for a conversation.""" diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index 55308dbb0a..26634035bb 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -1265,3 +1265,117 @@ def test_security_analyzer_endpoint_with_malformed_analyzer_data( assert response.status_code == 422 response_data = response.json() assert "detail" in response_data + + +def test_update_conversation_secrets_string_conversion( + client, mock_conversation_service, mock_event_service, sample_conversation_id +): + """Test update_conversation_secrets endpoint with plain string secrets. + + This test verifies that plain string secrets are automatically converted + to StaticSecret objects on the server side. + """ + + # Mock the service responses + mock_conversation_service.get_event_service.return_value = mock_event_service + mock_event_service.update_secrets.return_value = None + + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + # Send plain string secrets (this should work with the new validator) + request_data = { + "secrets": { + "API_KEY": "plain-secret-value", + "TOKEN": "plain-token-value", + } + } + + response = client.post( + f"/api/conversations/{sample_conversation_id}/secrets", json=request_data + ) + + assert response.status_code == 200 + data = response.json() + assert data["success"] is True + + # Verify services were called + mock_conversation_service.get_event_service.assert_called_once_with( + sample_conversation_id + ) + mock_event_service.update_secrets.assert_called_once() + + # Verify that the secrets were converted to proper SecretSource objects + call_args = mock_event_service.update_secrets.call_args[0][0] + assert "API_KEY" in call_args + assert "TOKEN" in call_args + + # The values should be SecretSource objects, not plain strings + from openhands.sdk.conversation.secret_source import StaticSecret + + assert isinstance(call_args["API_KEY"], StaticSecret) + assert isinstance(call_args["TOKEN"], StaticSecret) + + # Verify the actual secret values + assert call_args["API_KEY"].get_value() == "plain-secret-value" + assert call_args["TOKEN"].get_value() == "plain-token-value" + + finally: + client.app.dependency_overrides.clear() + + +def test_update_conversation_secrets_mixed_formats( + client, mock_conversation_service, mock_event_service, sample_conversation_id +): + """Test update_conversation_secrets endpoint with mixed secret formats. + + This test verifies that the server can handle a mix of plain strings + and properly formatted SecretSource objects. + """ + + # Mock the service responses + mock_conversation_service.get_event_service.return_value = mock_event_service + mock_event_service.update_secrets.return_value = None + + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + # Mix of plain strings and proper SecretSource objects + request_data = { + "secrets": { + "PLAIN_SECRET": "plain-value", + "STATIC_SECRET": {"kind": "StaticSecret", "value": "static-value"}, + "LOOKUP_SECRET": { + "kind": "LookupSecret", + "url": "https://example.com/secret", + }, + } + } + + response = client.post( + f"/api/conversations/{sample_conversation_id}/secrets", json=request_data + ) + + assert response.status_code == 200 + data = response.json() + assert data["success"] is True + + # Verify services were called + mock_conversation_service.get_event_service.assert_called_once_with( + sample_conversation_id + ) + mock_event_service.update_secrets.assert_called_once() + + # Verify that all secrets were processed correctly + call_args = mock_event_service.update_secrets.call_args[0][0] + assert len(call_args) == 3 + assert "PLAIN_SECRET" in call_args + assert "STATIC_SECRET" in call_args + assert "LOOKUP_SECRET" in call_args + + finally: + client.app.dependency_overrides.clear() diff --git a/tests/agent_server/test_models.py b/tests/agent_server/test_models.py new file mode 100644 index 0000000000..40b11798f7 --- /dev/null +++ b/tests/agent_server/test_models.py @@ -0,0 +1,115 @@ +"""Tests for agent_server models.""" + +from typing import Any + +import pytest +from pydantic import SecretStr, ValidationError + +from openhands.agent_server.models import UpdateSecretsRequest +from openhands.sdk.conversation.secret_source import LookupSecret, StaticSecret + + +def test_update_secrets_request_string_conversion(): + """Test that plain string secrets are converted to StaticSecret objects.""" + + # Test with plain string secrets + request = UpdateSecretsRequest( + secrets={ # type: ignore[arg-type] + "API_KEY": "plain-secret-value", + "TOKEN": "another-secret", + } + ) + + # Verify conversion happened + assert isinstance(request.secrets["API_KEY"], StaticSecret) + assert isinstance(request.secrets["TOKEN"], StaticSecret) + + # Verify values are correct + assert request.secrets["API_KEY"].get_value() == "plain-secret-value" + assert request.secrets["TOKEN"].get_value() == "another-secret" + + +def test_update_secrets_request_proper_secret_source(): + """Test that properly formatted SecretSource objects are preserved.""" + + # Test with properly formatted SecretSource objects + request = UpdateSecretsRequest( + secrets={ + "STATIC_SECRET": StaticSecret(value=SecretStr("static-value")), + "LOOKUP_SECRET": LookupSecret(url="https://example.com/secret"), + } + ) + + # Verify objects are preserved as-is + assert isinstance(request.secrets["STATIC_SECRET"], StaticSecret) + assert isinstance(request.secrets["LOOKUP_SECRET"], LookupSecret) + + # Verify values + assert request.secrets["STATIC_SECRET"].get_value() == "static-value" + assert request.secrets["LOOKUP_SECRET"].url == "https://example.com/secret" + + +def test_update_secrets_request_mixed_formats(): + """Test that mixed formats (strings and SecretSource objects) work together.""" + + secrets_dict: dict[str, Any] = { + "PLAIN_SECRET": "plain-value", + "STATIC_SECRET": StaticSecret(value=SecretStr("static-value")), + "LOOKUP_SECRET": LookupSecret(url="https://example.com/secret"), + } + request = UpdateSecretsRequest(secrets=secrets_dict) # type: ignore[arg-type] + + # Verify all types are correct + assert isinstance(request.secrets["PLAIN_SECRET"], StaticSecret) + assert isinstance(request.secrets["STATIC_SECRET"], StaticSecret) + assert isinstance(request.secrets["LOOKUP_SECRET"], LookupSecret) + + # Verify values + assert request.secrets["PLAIN_SECRET"].get_value() == "plain-value" + assert request.secrets["STATIC_SECRET"].get_value() == "static-value" + assert request.secrets["LOOKUP_SECRET"].url == "https://example.com/secret" + + +def test_update_secrets_request_dict_without_kind(): + """Test handling of dict values without 'kind' field.""" + + request = UpdateSecretsRequest( + secrets={ # type: ignore[arg-type] + "SECRET_WITH_VALUE": { + "value": "secret-value", + "description": "A test secret", + }, + } + ) + + # Secret with value should be converted to StaticSecret + assert isinstance(request.secrets["SECRET_WITH_VALUE"], StaticSecret) + assert request.secrets["SECRET_WITH_VALUE"].get_value() == "secret-value" + assert request.secrets["SECRET_WITH_VALUE"].description == "A test secret" + + +def test_update_secrets_request_invalid_dict(): + """Test handling of invalid dict values without 'kind' or 'value' field.""" + + # This should raise a validation error since the dict is invalid + with pytest.raises(ValidationError): + UpdateSecretsRequest( + secrets={ # type: ignore[arg-type] + "SECRET_WITHOUT_VALUE": {"description": "No value"}, + } + ) + + +def test_update_secrets_request_empty_secrets(): + """Test that empty secrets dict is handled correctly.""" + + request = UpdateSecretsRequest(secrets={}) + assert request.secrets == {} + + +def test_update_secrets_request_invalid_input(): + """Test that invalid input types are handled appropriately.""" + + # Non-dict input should be preserved (will fail validation later) + with pytest.raises(ValidationError): + UpdateSecretsRequest(secrets="not-a-dict") # type: ignore[arg-type]