From bd43f20ee5a3714719777d66869ad7aa145facd3 Mon Sep 17 00:00:00 2001 From: JunghwanNA <70629228+shaun0927@users.noreply.github.com> Date: Fri, 17 Apr 2026 09:03:25 +0900 Subject: [PATCH] Require authenticated access for MCP management API The management API currently accepts Authorization headers without validating them, and the tool-management call path drops the caller token before refresh and delete operations. This patch hardens the management endpoints and forwards the current request token so the existing MCP management flow keeps working under authentication. Constraint: Keep scope limited to MCP management auth hardening for mergeability Rejected: Broader API-to-MCP trust-boundary changes | needs product and architecture discussion Confidence: medium Scope-risk: narrow Directive: Keep management endpoint auth and caller-token forwarding aligned when adding new MCP management routes Tested: python3 -m py_compile on changed files; stubbed local auth-validation script; stubbed local header-forwarding script Not-tested: Full pytest suite in the current local environment (FastAPI/SDK dependency mismatch during collection) --- backend/apps/tool_config_app.py | 10 +++- backend/mcp_service.py | 16 ++++++ .../services/tool_configuration_service.py | 33 ++++++++--- test/backend/app/test_tool_config_app.py | 3 +- test/backend/services/test_mcp_service.py | 57 ++++++++++++++++--- .../test_tool_configuration_service.py | 17 ++++-- 6 files changed, 112 insertions(+), 24 deletions(-) diff --git a/backend/apps/tool_config_app.py b/backend/apps/tool_config_app.py index 11e76975f..0285ca4c6 100644 --- a/backend/apps/tool_config_app.py +++ b/backend/apps/tool_config_app.py @@ -73,7 +73,11 @@ async def scan_and_update_tool( """ Used to update the tool list and status """ try: user_id, tenant_id = get_current_user_id(authorization) - await update_tool_list(tenant_id=tenant_id, user_id=user_id) + await update_tool_list( + tenant_id=tenant_id, + user_id=user_id, + authorization_token=authorization + ) return JSONResponse( status_code=HTTPStatus.OK, content={"message": "Successfully update tool", "status": "success"} @@ -159,7 +163,7 @@ async def import_openapi_api( user_id, tenant_id = get_current_user_id(authorization) result = import_openapi_json(openapi_json, tenant_id, user_id) - mcp_result = _refresh_outer_api_tools_in_mcp(tenant_id) + mcp_result = _refresh_outer_api_tools_in_mcp(tenant_id, authorization) result["mcp_refresh"] = mcp_result return JSONResponse( @@ -246,7 +250,7 @@ async def delete_outer_api_tool_api( """ try: user_id, tenant_id = get_current_user_id(authorization) - deleted = delete_outer_api_tool(tool_id, tenant_id, user_id) + deleted = delete_outer_api_tool(tool_id, tenant_id, user_id, authorization) if not deleted: raise HTTPException( status_code=HTTPStatus.NOT_FOUND, diff --git a/backend/mcp_service.py b/backend/mcp_service.py index 445a47622..6f45e53e2 100644 --- a/backend/mcp_service.py +++ b/backend/mcp_service.py @@ -79,6 +79,19 @@ async def run(self, arguments: Dict[str, Any]) -> Any: _mcp_management_app = None +def _require_management_authorization(authorization: Optional[str]) -> tuple[str, str]: + """Validate the caller for MCP management endpoints.""" + try: + from utils.auth_utils import get_current_user_id + return get_current_user_id(authorization) + except Exception as e: + logger.warning(f"Unauthorized MCP management API access: {e}") + raise HTTPException( + status_code=401, + detail="Invalid or expired authentication token" + ) from e + + def get_mcp_management_app(): """Get or create FastAPI app for MCP management endpoints.""" global _mcp_management_app @@ -97,6 +110,7 @@ async def refresh_outer_api_tools_endpoint( to notify the MCP server to reload outer API tools. """ try: + _require_management_authorization(authorization) result = refresh_outer_api_tools(tenant_id) return { "status": "success", @@ -111,6 +125,7 @@ async def list_outer_api_tools_endpoint( authorization: Optional[str] = Header(None) ): """List all registered outer API tool names.""" + _require_management_authorization(authorization) return { "status": "success", "data": get_registered_outer_api_tools() @@ -131,6 +146,7 @@ async def remove_outer_api_tool_endpoint( Success status """ try: + _require_management_authorization(authorization) sanitized_name = _sanitize_function_name(tool_name) result = remove_outer_api_tool(sanitized_name) if result: diff --git a/backend/services/tool_configuration_service.py b/backend/services/tool_configuration_service.py index d7240db26..e8ed45b65 100644 --- a/backend/services/tool_configuration_service.py +++ b/backend/services/tool_configuration_service.py @@ -425,7 +425,11 @@ async def init_tool_list_for_tenant(tenant_id: str, user_id: str): return {"status": "success", "message": "Tool list initialized successfully"} -async def update_tool_list(tenant_id: str, user_id: str): +async def update_tool_list( + tenant_id: str, + user_id: str, + authorization_token: Optional[str] = None +): """ Scan and gather all available tools from local, MCP, and outer API sources. Also refreshes dynamic outer API tools in MCP server. @@ -440,7 +444,7 @@ async def update_tool_list(tenant_id: str, user_id: str): local_tools = get_local_tools() langchain_tools = get_langchain_tools() - _refresh_outer_api_tools_in_mcp(tenant_id) + _refresh_outer_api_tools_in_mcp(tenant_id, authorization_token) try: mcp_tools = await get_all_mcp_tools(tenant_id) @@ -1157,7 +1161,12 @@ def get_outer_api_tool(tool_id: int, tenant_id: str) -> Optional[Dict[str, Any]] return query_outer_api_tool_by_id(tool_id, tenant_id) -def delete_outer_api_tool(tool_id: int, tenant_id: str, user_id: str) -> bool: +def delete_outer_api_tool( + tool_id: int, + tenant_id: str, + user_id: str, + authorization_token: Optional[str] = None +) -> bool: """ Delete an outer API tool. @@ -1178,12 +1187,16 @@ def delete_outer_api_tool(tool_id: int, tenant_id: str, user_id: str) -> bool: if deleted and tool_name: # Also remove from MCP server - _remove_outer_api_tool_from_mcp(tool_name, tenant_id) + _remove_outer_api_tool_from_mcp(tool_name, tenant_id, authorization_token) return deleted -def _remove_outer_api_tool_from_mcp(tool_name: str, tenant_id: str) -> bool: +def _remove_outer_api_tool_from_mcp( + tool_name: str, + tenant_id: str, + authorization_token: Optional[str] = None +) -> bool: """ Remove a specific outer API tool from the MCP server via HTTP API. @@ -1196,7 +1209,8 @@ def _remove_outer_api_tool_from_mcp(tool_name: str, tenant_id: str) -> bool: """ remove_url = f"{MCP_MANAGEMENT_API}/tools/outer_api/{tool_name}" try: - response = requests.delete(remove_url, timeout=10) + headers = {"Authorization": authorization_token} if authorization_token else {} + response = requests.delete(remove_url, headers=headers, timeout=10) if response.ok: logger.info(f"Removed outer API tool '{tool_name}' from MCP server") return True @@ -1208,7 +1222,10 @@ def _remove_outer_api_tool_from_mcp(tool_name: str, tenant_id: str) -> bool: return False -def _refresh_outer_api_tools_in_mcp(tenant_id: str) -> Dict[str, Any]: +def _refresh_outer_api_tools_in_mcp( + tenant_id: str, + authorization_token: Optional[str] = None +) -> Dict[str, Any]: """ Refresh outer API tools in MCP server via HTTP API. @@ -1227,9 +1244,11 @@ def _refresh_outer_api_tools_in_mcp(tenant_id: str) -> Dict[str, Any]: for attempt in range(max_retries): try: + headers = {"Authorization": authorization_token} if authorization_token else {} response = requests.post( refresh_url, params={"tenant_id": tenant_id}, + headers=headers, timeout=30 ) response.raise_for_status() diff --git a/test/backend/app/test_tool_config_app.py b/test/backend/app/test_tool_config_app.py index 91d64dc60..ea07f8a56 100644 --- a/test/backend/app/test_tool_config_app.py +++ b/test/backend/app/test_tool_config_app.py @@ -4,6 +4,7 @@ # Add path for correct imports sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../../backend")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../../sdk")) # Mock boto3 to avoid dependency issues sys.modules['boto3'] = MagicMock() @@ -212,7 +213,7 @@ def test_scan_and_update_tool_success(self, mock_update_tool_list, mock_get_user mock_get_user_id.assert_called_once_with(None) mock_update_tool_list.assert_called_once_with( - tenant_id="tenant456", user_id="user123") + tenant_id="tenant456", user_id="user123", authorization_token=None) @patch('apps.tool_config_app.get_current_user_id') @patch('apps.tool_config_app.update_tool_list') diff --git a/test/backend/services/test_mcp_service.py b/test/backend/services/test_mcp_service.py index 23e4e6a62..cf6777bd7 100644 --- a/test/backend/services/test_mcp_service.py +++ b/test/backend/services/test_mcp_service.py @@ -80,8 +80,11 @@ stub_utils = types.ModuleType("utils") stub_utils.logging_utils = types.ModuleType("utils.logging_utils") stub_utils.logging_utils.configure_logging = MagicMock() +stub_utils.auth_utils = types.ModuleType("utils.auth_utils") +stub_utils.auth_utils.get_current_user_id = MagicMock(return_value=("user123", "tenant456")) sys.modules['utils'] = stub_utils sys.modules['utils.logging_utils'] = stub_utils.logging_utils +sys.modules['utils.auth_utils'] = stub_utils.auth_utils # Stub database stub_database = types.ModuleType("database") @@ -99,10 +102,7 @@ sys.modules['database.outer_api_tool_db'] = stub_outer_api_tool_db sys.modules['backend.database.outer_api_tool_db'] = stub_outer_api_tool_db -# Stub http -stub_http = types.ModuleType("http") -stub_http.HTTPStatus = types.SimpleNamespace(OK=200) -sys.modules['http'] = stub_http +# Keep the stdlib http module intact so later httpx imports still work. # Import the module under test import mcp_service @@ -1117,7 +1117,8 @@ def test_get_mcp_management_app_creates_once(self, mock_nexent_mcp): assert app1 is app2 @pytest.mark.asyncio - async def test_refresh_outer_api_tools_endpoint(self, mock_nexent_mcp): + @patch.object(mcp_service, '_require_management_authorization', return_value=("user123", "tenant456")) + async def test_refresh_outer_api_tools_endpoint(self, mock_require_auth, mock_nexent_mcp): """Test refresh outer API tools endpoint""" tools = [{"name": "api1", "url": "https://api.example.com/1"}] mcp_service.query_available_outer_api_tools.return_value = tools @@ -1133,9 +1134,11 @@ async def test_refresh_outer_api_tools_endpoint(self, mock_nexent_mcp): assert response.status_code == 200 data = response.json() assert data["status"] == "success" + mock_require_auth.assert_called_once() @pytest.mark.asyncio - async def test_list_outer_api_tools_endpoint(self, mock_nexent_mcp): + @patch.object(mcp_service, '_require_management_authorization', return_value=("user123", "tenant456")) + async def test_list_outer_api_tools_endpoint(self, mock_require_auth, mock_nexent_mcp): """Test list outer API tools endpoint""" app = mcp_service.get_mcp_management_app() client = TestClient(app) @@ -1146,9 +1149,11 @@ async def test_list_outer_api_tools_endpoint(self, mock_nexent_mcp): data = response.json() assert data["status"] == "success" assert "data" in data + mock_require_auth.assert_called_once() @pytest.mark.asyncio - async def test_remove_outer_api_tool_endpoint_success(self, mock_nexent_mcp): + @patch.object(mcp_service, '_require_management_authorization', return_value=("user123", "tenant456")) + async def test_remove_outer_api_tool_endpoint_success(self, mock_require_auth, mock_nexent_mcp): """Test remove outer API tool endpoint success""" tools = [{"name": "api1", "url": "https://api.example.com/1"}] mcp_service.query_available_outer_api_tools.return_value = tools @@ -1162,9 +1167,11 @@ async def test_remove_outer_api_tool_endpoint_success(self, mock_nexent_mcp): assert response.status_code == 200 data = response.json() assert data["status"] == "success" + mock_require_auth.assert_called_once() @pytest.mark.asyncio - async def test_remove_outer_api_tool_endpoint_not_found(self, mock_nexent_mcp): + @patch.object(mcp_service, '_require_management_authorization', return_value=("user123", "tenant456")) + async def test_remove_outer_api_tool_endpoint_not_found(self, mock_require_auth, mock_nexent_mcp): """Test remove outer API tool endpoint when tool not found""" app = mcp_service.get_mcp_management_app() client = TestClient(app) @@ -1175,9 +1182,11 @@ async def test_remove_outer_api_tool_endpoint_not_found(self, mock_nexent_mcp): # verifies that remove_outer_api_tool returns False for not found # In real test with FastAPI, this would return 404 assert response is not None + mock_require_auth.assert_called_once() @pytest.mark.asyncio - async def test_refresh_endpoint_exception(self, mock_nexent_mcp): + @patch.object(mcp_service, '_require_management_authorization', return_value=("user123", "tenant456")) + async def test_refresh_endpoint_exception(self, mock_require_auth, mock_nexent_mcp): """Test refresh endpoint handles exceptions""" mcp_service.query_available_outer_api_tools.side_effect = Exception("DB error") @@ -1193,6 +1202,36 @@ async def test_refresh_endpoint_exception(self, mock_nexent_mcp): # catches the exception and returns 500 # In real test with FastAPI, this would return 500 assert response is not None + mock_require_auth.assert_called_once() + + @pytest.mark.asyncio + @patch.object(mcp_service, '_require_management_authorization') + async def test_refresh_outer_api_tools_endpoint_unauthorized(self, mock_require_auth, mock_nexent_mcp): + """Test refresh outer API tools endpoint rejects unauthorized access.""" + mock_require_auth.side_effect = RealHTTPException(status_code=401, detail="Invalid or expired authentication token") + + app = mcp_service.get_mcp_management_app() + client = TestClient(app) + + response = await client.post( + "/tools/outer_api/refresh", + params={"tenant_id": "tenant1"} + ) + + assert response.status_code == 401 + + @pytest.mark.asyncio + @patch.object(mcp_service, '_require_management_authorization') + async def test_list_outer_api_tools_endpoint_unauthorized(self, mock_require_auth, mock_nexent_mcp): + """Test list outer API tools endpoint rejects unauthorized access.""" + mock_require_auth.side_effect = RealHTTPException(status_code=401, detail="Invalid or expired authentication token") + + app = mcp_service.get_mcp_management_app() + client = TestClient(app) + + response = await client.get("/tools/outer_api") + + assert response.status_code == 401 # --------------------------------------------------------------------------- diff --git a/test/backend/services/test_tool_configuration_service.py b/test/backend/services/test_tool_configuration_service.py index f970c284d..c36ed46af 100644 --- a/test/backend/services/test_tool_configuration_service.py +++ b/test/backend/services/test_tool_configuration_service.py @@ -4387,7 +4387,7 @@ def test_delete_outer_api_tool_success(self, mock_delete, mock_query, mock_remov assert result is True mock_delete.assert_called_once_with(1, "tenant1", "user1") - mock_remove.assert_called_once_with("test_tool", "tenant1") + mock_remove.assert_called_once_with("test_tool", "tenant1", None) @patch('backend.services.tool_configuration_service._remove_outer_api_tool_from_mcp') @patch('backend.services.tool_configuration_service.query_outer_api_tool_by_id') @@ -4433,7 +4433,11 @@ def test_remove_outer_api_tool_from_mcp_success(self, mock_delete): result = _remove_outer_api_tool_from_mcp("test_tool", "tenant1") assert result is True - mock_delete.assert_called_once() + mock_delete.assert_called_once_with( + "http://localhost:5015/tools/outer_api/test_tool", + headers={}, + timeout=10 + ) @patch('requests.delete') def test_remove_outer_api_tool_from_mcp_failure(self, mock_delete): @@ -4476,7 +4480,12 @@ def test_refresh_outer_api_tools_success(self, mock_post, mock_sleep): result = _refresh_outer_api_tools_in_mcp("tenant1") assert result == {"refreshed": 5} - mock_post.assert_called_once() + mock_post.assert_called_once_with( + "http://localhost:5015/tools/outer_api/refresh", + params={"tenant_id": "tenant1"}, + headers={}, + timeout=30 + ) @patch('time.sleep') @patch('requests.post') @@ -4551,7 +4560,7 @@ async def test_update_tool_list_calls_refresh(self, mock_update_table, mock_get_ from backend.services.tool_configuration_service import update_tool_list await update_tool_list("tenant123", "user456") - mock_refresh.assert_called_once_with("tenant123") + mock_refresh.assert_called_once_with("tenant123", None) @pytest.mark.asyncio @patch('backend.services.tool_configuration_service._refresh_outer_api_tools_in_mcp')