diff --git a/src/llama_stack/providers/utils/tools/mcp.py b/src/llama_stack/providers/utils/tools/mcp.py index 309e3bf5d..2bc6cbf96 100644 --- a/src/llama_stack/providers/utils/tools/mcp.py +++ b/src/llama_stack/providers/utils/tools/mcp.py @@ -27,30 +27,33 @@ from llama_stack.providers.utils.tools.ttl_dict import TTLDict logger = get_logger(__name__, category="tools") - def prepare_mcp_headers(base_headers: dict[str, str] | None, authorization: str | None) -> dict[str, str]: - """Prepare headers for MCP requests with authorization handling. + """ + Prepare headers for MCP requests with authorization support. Args: - base_headers: Base headers to use (e.g., from mcp_tool.headers) - authorization: OAuth access token (just the token, not "Bearer ") + base_headers: Base headers dictionary (can be None) + authorization: OAuth access token (without "Bearer " prefix) Returns: - Final headers dict with Authorization header if authorization is provided + Headers dictionary with Authorization header if token provided Raises: - ValueError: If both base_headers contains Authorization and authorization parameter is provided + ValueError: If Authorization header is specified in the headers dict (security risk) """ headers = dict(base_headers or {}) + # Security check: reject any Authorization header in the headers dict + # Users must use the authorization parameter instead to avoid security risks + existing_keys_lower = {k.lower() for k in headers.keys()} + if "authorization" in existing_keys_lower: + raise ValueError( + "For security reasons, Authorization header cannot be passed via 'headers'. " + "Please use the 'authorization' parameter instead." + ) + + # Add Authorization header if token provided if authorization: - # Check if Authorization header already exists (case-insensitive check) - existing_keys_lower = {k.lower() for k in headers.keys()} - if "authorization" in existing_keys_lower: - raise ValueError( - "Cannot specify Authorization in both 'headers' and 'authorization' fields. " - "Please use only the 'authorization' field." - ) # OAuth access token - add "Bearer " prefix headers["Authorization"] = f"Bearer {authorization}" diff --git a/tests/integration/responses/test_mcp_authentication.py b/tests/integration/responses/test_mcp_authentication.py index bfcf578ac..a61de512c 100644 --- a/tests/integration/responses/test_mcp_authentication.py +++ b/tests/integration/responses/test_mcp_authentication.py @@ -91,8 +91,8 @@ def test_mcp_authorization_different_token(compat_client, text_model_id): assert response.output[1].error is None -def test_mcp_authorization_error_when_both_provided(compat_client, text_model_id): - """Test that providing both headers['Authorization'] and authorization field raises an error.""" +def test_mcp_authorization_error_when_header_provided(compat_client, text_model_id): + """Test that providing Authorization in headers raises a security error.""" if not isinstance(compat_client, LlamaStackAsLibraryClient): pytest.skip("in-process MCP server is only supported in library client") @@ -102,18 +102,17 @@ def test_mcp_authorization_error_when_both_provided(compat_client, text_model_id [ { "type": "mcp", - "server_label": "both-auth-mcp", + "server_label": "header-auth-mcp", "server_url": "", - "headers": {"Authorization": f"Bearer {test_token}"}, - "authorization": "should-cause-error", # This should trigger an error + "headers": {"Authorization": f"Bearer {test_token}"}, # Security risk - should be rejected } ], mcp_server_info, ) - # Create response - should raise ValueError + # Create response - should raise ValueError for security reasons with pytest.raises( - ValueError, match="Cannot specify Authorization in both 'headers' and 'authorization' fields" + ValueError, match="For security reasons, Authorization header cannot be passed via 'headers'" ): compat_client.responses.create( model=text_model_id,