From 41333a7c265f8aa58f9ec341feaf1ff722e7fc94 Mon Sep 17 00:00:00 2001 From: openhands Date: Thu, 19 Mar 2026 11:28:34 +0000 Subject: [PATCH] fix: tighten marketplace path validation Co-authored-by: openhands --- .../__tests__/utils/settings-utils.test.ts | 8 ++-- frontend/src/utils/settings-utils.ts | 2 +- .../app_conversation_service_base.py | 7 +-- openhands/storage/data_models/settings.py | 2 +- .../test_app_conversation_service_base.py | 43 +++++++++++++++++++ .../unit/storage/data_models/test_settings.py | 2 + 6 files changed, 55 insertions(+), 9 deletions(-) diff --git a/frontend/__tests__/utils/settings-utils.test.ts b/frontend/__tests__/utils/settings-utils.test.ts index cc367dcdef..0e45fe1f3f 100644 --- a/frontend/__tests__/utils/settings-utils.test.ts +++ b/frontend/__tests__/utils/settings-utils.test.ts @@ -107,9 +107,6 @@ describe("isValidMarketplacePath", () => { it("should return true for valid simple paths", () => { expect(isValidMarketplacePath("marketplaces/default.json")).toBe(true); expect(isValidMarketplacePath("path/to/file.json")).toBe(true); - expect(isValidMarketplacePath("skills.json")).toBe(true); - expect(isValidMarketplacePath("my-marketplace.json")).toBe(true); - expect(isValidMarketplacePath("my_marketplace.json")).toBe(true); }); it("should return false for invalid paths", () => { @@ -126,7 +123,10 @@ describe("isValidMarketplacePath", () => { false, ); - // Missing .json extension + // Missing directory component or extension + expect(isValidMarketplacePath("skills.json")).toBe(false); + expect(isValidMarketplacePath("my-marketplace.json")).toBe(false); + expect(isValidMarketplacePath("my_marketplace.json")).toBe(false); expect(isValidMarketplacePath("marketplaces/default")).toBe(false); // Invalid characters diff --git a/frontend/src/utils/settings-utils.ts b/frontend/src/utils/settings-utils.ts index e19dfa3962..b4b488260c 100644 --- a/frontend/src/utils/settings-utils.ts +++ b/frontend/src/utils/settings-utils.ts @@ -72,7 +72,7 @@ export const parseMaxBudgetPerTask = (value: string): number | null => { * Regex pattern for validating marketplace_path. * Only relative JSON paths within the public skills repository are supported. */ -const MARKETPLACE_PATH_PATTERN = /^[a-zA-Z0-9_-]+(?:\/[a-zA-Z0-9_.-]+)*\.json$/; +const MARKETPLACE_PATH_PATTERN = /^[a-zA-Z0-9_-]+(?:\/[a-zA-Z0-9_.-]+)+\.json$/; /** * Validates a marketplace path value. diff --git a/openhands/app_server/app_conversation/app_conversation_service_base.py b/openhands/app_server/app_conversation/app_conversation_service_base.py index 97fbf37335..a0a12cce58 100644 --- a/openhands/app_server/app_conversation/app_conversation_service_base.py +++ b/openhands/app_server/app_conversation/app_conversation_service_base.py @@ -134,9 +134,10 @@ class AppConversationServiceBase(AppConversationService, ABC): try: user_info = await self.user_context.get_user_info() marketplace_path = user_info.marketplace_path - except Exception as e: - _logger.warning( - f'Failed to load marketplace_path from user settings: {e}' + except NotImplementedError: + _logger.debug( + 'User context does not provide user settings; ' + 'loading skills without marketplace_path override' ) # Single API call to agent-server for ALL skills diff --git a/openhands/storage/data_models/settings.py b/openhands/storage/data_models/settings.py index 18494965e3..421787010a 100644 --- a/openhands/storage/data_models/settings.py +++ b/openhands/storage/data_models/settings.py @@ -21,7 +21,7 @@ from openhands.core.config.mcp_config import MCPConfig from openhands.core.config.utils import load_openhands_config from openhands.storage.data_models.secrets import Secrets -MARKETPLACE_PATH_PATTERN = re.compile(r'^[A-Za-z0-9_-]+(?:/[A-Za-z0-9_.-]+)*\.json$') +MARKETPLACE_PATH_PATTERN = re.compile(r'^[A-Za-z0-9_-]+(?:/[A-Za-z0-9_.-]+)+\.json$') class SandboxGroupingStrategy(str, Enum): diff --git a/tests/unit/app_server/test_app_conversation_service_base.py b/tests/unit/app_server/test_app_conversation_service_base.py index 12ddfb9e3a..06680ac14b 100644 --- a/tests/unit/app_server/test_app_conversation_service_base.py +++ b/tests/unit/app_server/test_app_conversation_service_base.py @@ -1136,6 +1136,49 @@ class TestLoadAndMergeAllSkills: assert call_kwargs['project_dir'] == '/workspace' assert call_kwargs['marketplace_path'] is None + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_skills_from_agent_server' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.build_org_config' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.build_sandbox_config' + ) + async def test_skips_marketplace_override_when_user_info_is_unavailable( + self, + mock_build_sandbox_config, + mock_build_org_config, + mock_load_skills, + ): + mock_user_context = Mock(spec=UserContext) + mock_user_context.get_user_info = AsyncMock(side_effect=NotImplementedError) + with patch.object(AppConversationServiceBase, '__abstractmethods__', set()): + service = AppConversationServiceBase( + init_git_in_empty_workspace=True, user_context=mock_user_context + ) + + from openhands.app_server.sandbox.sandbox_models import ExposedUrl + + sandbox = Mock(spec=SandboxInfo) + exposed_url = ExposedUrl( + name='AGENT_SERVER', url='http://localhost:8000', port=8000 + ) + sandbox.exposed_urls = [exposed_url] + sandbox.session_api_key = 'test-key' + + mock_load_skills.return_value = [] + mock_build_org_config.return_value = None + mock_build_sandbox_config.return_value = None + + await service.load_and_merge_all_skills( + sandbox, None, '/workspace', 'http://localhost:8000' + ) + + call_kwargs = mock_load_skills.call_args[1] + assert call_kwargs['marketplace_path'] is None + @pytest.mark.asyncio @patch( 'openhands.app_server.app_conversation.app_conversation_service_base.load_skills_from_agent_server' diff --git a/tests/unit/storage/data_models/test_settings.py b/tests/unit/storage/data_models/test_settings.py index 5ace48cc32..c0683485df 100644 --- a/tests/unit/storage/data_models/test_settings.py +++ b/tests/unit/storage/data_models/test_settings.py @@ -167,6 +167,8 @@ def test_settings_marketplace_path_blank_string_maps_to_none(): 'marketplaces\\default.json', 'marketplaces/default', 'marketplaces/%2e%2e/secret.json', + 'skills.json', + 'my-marketplace.json', ], ) def test_settings_marketplace_path_rejects_invalid_values(value: str):