diff --git a/docker-compose.yaml b/docker-compose.yaml index 85a38bc62..03bd6450d 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -81,6 +81,7 @@ services: - "8080:8080" volumes: - ./lightspeed-stack.yaml:/app-root/lightspeed-stack.yaml:z + - ./tests/e2e/secrets/mcp-token:/tmp/mcp-secret-token:ro environment: - OPENAI_API_KEY=${OPENAI_API_KEY} # Azure Entra ID credentials (AZURE_API_KEY is obtained dynamically) diff --git a/src/utils/types.py b/src/utils/types.py index 220a85239..a78370a0b 100644 --- a/src/utils/types.py +++ b/src/utils/types.py @@ -222,6 +222,27 @@ class ResponsesApiParams(BaseModel): description="Extra HTTP headers to send with the request (e.g. x-llamastack-provider-data)", ) + def model_dump(self, *args: Any, **kwargs: Any) -> dict[str, Any]: + """Serialize params, re-injecting MCP authorization stripped by exclude=True. + + llama-stack-api marks ``InputToolMCP.authorization`` with + ``Field(exclude=True)`` to prevent token leakage in API responses. + The base ``model_dump()`` therefore strips the field, but we need it + in the request payload so llama-stack server can authenticate with + MCP servers. See LCORE-1414 / GitHub issue #1269. + """ + result = super().model_dump(*args, **kwargs) + dumped_tools = result.get("tools") + if not self.tools or not isinstance(dumped_tools, list): + return result + if len(dumped_tools) != len(self.tools): + return result + for tool, dumped_tool in zip(self.tools, dumped_tools): + authorization = getattr(tool, "authorization", None) + if authorization is not None and isinstance(dumped_tool, dict): + dumped_tool["authorization"] = authorization + return result + class ToolCallSummary(BaseModel): """Model representing a tool call made during response generation (for tool_calls list).""" diff --git a/tests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yaml b/tests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yaml new file mode 100644 index 000000000..1ff0d425e --- /dev/null +++ b/tests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yaml @@ -0,0 +1,25 @@ +name: Lightspeed Core Service (LCS) +service: + host: 0.0.0.0 + port: 8080 + auth_enabled: false + workers: 1 + color_log: true + access_log: true +llama_stack: + # Library mode - embeds llama-stack as library + use_as_library_client: true + library_client_config_path: run.yaml +user_data_collection: + feedback_enabled: true + feedback_storage: "/tmp/data/feedback" + transcripts_enabled: true + transcripts_storage: "/tmp/data/transcripts" +authentication: + module: "noop" +mcp_servers: + - name: "mcp-file-auth" + provider_id: "model-context-protocol" + url: "http://mock-mcp:3001" + authorization_headers: + Authorization: "/tmp/mcp-secret-token" diff --git a/tests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yaml b/tests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yaml new file mode 100644 index 000000000..d39f55399 --- /dev/null +++ b/tests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yaml @@ -0,0 +1,26 @@ +name: Lightspeed Core Service (LCS) +service: + host: 0.0.0.0 + port: 8080 + auth_enabled: false + workers: 1 + color_log: true + access_log: true +llama_stack: + # Server mode - connects to separate llama-stack service + use_as_library_client: false + url: http://llama-stack:8321 + api_key: xyzzy +user_data_collection: + feedback_enabled: true + feedback_storage: "/tmp/data/feedback" + transcripts_enabled: true + transcripts_storage: "/tmp/data/transcripts" +authentication: + module: "noop" +mcp_servers: + - name: "mcp-file-auth" + provider_id: "model-context-protocol" + url: "http://mock-mcp:3001" + authorization_headers: + Authorization: "/tmp/mcp-secret-token" diff --git a/tests/e2e/features/environment.py b/tests/e2e/features/environment.py index bf434f4b4..0ca8781d0 100644 --- a/tests/e2e/features/environment.py +++ b/tests/e2e/features/environment.py @@ -53,6 +53,10 @@ "tests/e2e/configuration/{mode_dir}/lightspeed-stack-auth-rh-identity.yaml", "tests/e2e-prow/rhoai/configs/lightspeed-stack-auth-rh-identity.yaml", ), + "mcp-file-auth": ( + "tests/e2e/configuration/{mode_dir}/lightspeed-stack-mcp-file-auth.yaml", + "tests/e2e-prow/rhoai/configs/lightspeed-stack-mcp-file-auth.yaml", + ), } @@ -383,6 +387,12 @@ def before_feature(context: Context, feature: Feature) -> None: switch_config(context.feature_config) restart_container("lightspeed-stack") + if "MCPFileAuth" in feature.tags: + context.feature_config = _get_config_path("mcp-file-auth", mode_dir) + context.default_config_backup = create_config_backup("lightspeed-stack.yaml") + switch_config(context.feature_config) + restart_container("lightspeed-stack") + def after_feature(context: Context, feature: Feature) -> None: """Run after each feature file is exercised. @@ -415,3 +425,8 @@ def after_feature(context: Context, feature: Feature) -> None: switch_config(context.default_config_backup) restart_container("lightspeed-stack") remove_config_backup(context.default_config_backup) + + if "MCPFileAuth" in feature.tags: + switch_config(context.default_config_backup) + restart_container("lightspeed-stack") + remove_config_backup(context.default_config_backup) diff --git a/tests/e2e/features/mcp_file_auth.feature b/tests/e2e/features/mcp_file_auth.feature new file mode 100644 index 000000000..455f0740c --- /dev/null +++ b/tests/e2e/features/mcp_file_auth.feature @@ -0,0 +1,20 @@ +@MCPFileAuth +Feature: MCP file-based authorization tests + + Regression tests for LCORE-1414: MCP authorization tokens configured via + file-based authorization_headers must survive model_dump() serialization + and reach the MCP server as a valid Bearer token. + + Background: + Given The service is started locally + And REST API service prefix is /v1 + + @skip-in-library-mode + Scenario: Query succeeds with file-based MCP authorization + Given The system is in default state + When I use "query" to ask question + """ + {"query": "Use the mock_tool_e2e tool to send the message 'hello'", "model": "{MODEL}", "provider": "{PROVIDER}"} + """ + Then The status code of the response is 200 + And The body of the response contains mock_tool_e2e diff --git a/tests/e2e/secrets/mcp-token b/tests/e2e/secrets/mcp-token new file mode 100644 index 000000000..c05b9cec9 --- /dev/null +++ b/tests/e2e/secrets/mcp-token @@ -0,0 +1 @@ +test-secret-token diff --git a/tests/e2e/test_list.txt b/tests/e2e/test_list.txt index 42dc6b22a..3f94d09e0 100644 --- a/tests/e2e/test_list.txt +++ b/tests/e2e/test_list.txt @@ -15,4 +15,5 @@ features/rlsapi_v1_errors.feature features/streaming_query.feature features/rest_api.feature features/mcp.feature +features/mcp_file_auth.feature features/models.feature diff --git a/tests/unit/utils/test_types.py b/tests/unit/utils/test_types.py index 882a83711..232f31834 100644 --- a/tests/unit/utils/test_types.py +++ b/tests/unit/utils/test_types.py @@ -2,6 +2,10 @@ import pytest from llama_stack_api import ImageContentItem, TextContentItem, URL, _URLOrData +from llama_stack_api.openai_responses import ( + OpenAIResponseInputToolFileSearch as InputToolFileSearch, + OpenAIResponseInputToolMCP as InputToolMCP, +) from pydantic import AnyUrl, ValidationError from pytest_mock import MockerFixture @@ -9,6 +13,7 @@ from utils.types import ( GraniteToolParser, ReferencedDocument, + ResponsesApiParams, ToolCallSummary, ToolResultSummary, content_to_str, @@ -194,3 +199,130 @@ def test_constructor_partial_fields(self) -> None: doc = ReferencedDocument(doc_title="Test Title") assert doc.doc_url is None assert doc.doc_title == "Test Title" + + +class TestResponsesApiParamsModelDump: + """Tests for ResponsesApiParams.model_dump() MCP authorization fix. + + Regression tests for LCORE-1414 / GitHub issue #1269: llama-stack-api's + InputToolMCP.authorization has Field(exclude=True), causing the base + model_dump() to silently strip authorization tokens. + """ + + def _make_params(self, tools: list) -> ResponsesApiParams: + """Build minimal ResponsesApiParams with given tools.""" + return ResponsesApiParams( + input="test question", + model="provider/model", + conversation="conv-id", + store=False, + stream=False, + tools=tools, + ) + + def test_mcp_authorization_survives_model_dump(self) -> None: + """Test that MCP authorization is re-injected after model_dump().""" + tool = InputToolMCP( + server_label="auth-server", + server_url="http://localhost:3000", + require_approval="never", + authorization="my-secret-token", + ) + assert tool.authorization == "my-secret-token" + assert "authorization" not in tool.model_dump() + + params = self._make_params([tool]) + dumped = params.model_dump(exclude_none=True) + assert dumped["tools"][0]["authorization"] == "my-secret-token" + + def test_mcp_authorization_none_not_injected(self) -> None: + """Test that None authorization is not added to the dump.""" + tool = InputToolMCP( + server_label="no-auth-server", + server_url="http://localhost:3000", + require_approval="never", + ) + params = self._make_params([tool]) + dumped = params.model_dump(exclude_none=True) + assert "authorization" not in dumped["tools"][0] + + def test_non_mcp_tools_unaffected(self) -> None: + """Test that non-MCP tools are not modified by the override.""" + tool = InputToolFileSearch( + type="file_search", + vector_store_ids=["vs-1"], + ) + params = self._make_params([tool]) + dumped = params.model_dump(exclude_none=True) + assert "authorization" not in dumped["tools"][0] + + def test_mixed_tools_only_mcp_gets_authorization(self) -> None: + """Test mixed tool list: only MCP tools get authorization re-injected.""" + mcp_tool = InputToolMCP( + server_label="auth-server", + server_url="http://localhost:3000", + require_approval="never", + authorization="secret", + ) + file_tool = InputToolFileSearch( + type="file_search", + vector_store_ids=["vs-1"], + ) + params = self._make_params([file_tool, mcp_tool]) + dumped = params.model_dump(exclude_none=True) + + assert "authorization" not in dumped["tools"][0] + assert dumped["tools"][1]["authorization"] == "secret" + + def test_multiple_mcp_tools_each_preserves_authorization(self) -> None: + """Test that each MCP tool gets its own authorization re-injected.""" + tool_a = InputToolMCP( + server_label="server-a", + server_url="http://a:3000", + require_approval="never", + authorization="token-a", + ) + tool_b = InputToolMCP( + server_label="server-b", + server_url="http://b:3000", + require_approval="never", + authorization="token-b", + ) + params = self._make_params([tool_a, tool_b]) + dumped = params.model_dump(exclude_none=True) + + assert dumped["tools"][0]["authorization"] == "token-a" + assert dumped["tools"][1]["authorization"] == "token-b" + + def test_exclude_changing_tool_list_shape_skips_reinjection(self) -> None: + """Test that exclude removing tool indices does not mis-assign authorization.""" + tool_a = InputToolMCP( + server_label="server-a", + server_url="http://a:3000", + require_approval="never", + authorization="token-a", + ) + tool_b = InputToolMCP( + server_label="server-b", + server_url="http://b:3000", + require_approval="never", + authorization="token-b", + ) + params = self._make_params([tool_a, tool_b]) + dumped = params.model_dump(exclude={"tools": {0}}) + assert len(dumped["tools"]) == 1 + assert dumped["tools"][0]["server_label"] == "server-b" + assert "authorization" not in dumped["tools"][0] + + def test_no_tools_does_not_error(self) -> None: + """Test that model_dump() works when tools is None.""" + params = ResponsesApiParams( + input="test", + model="provider/model", + conversation="conv-id", + store=False, + stream=False, + tools=None, + ) + dumped = params.model_dump(exclude_none=True) + assert "tools" not in dumped