Skip to content

feat: Add Secret support to MCPTool and MCPToolset #1900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 10, 2025

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Jun 5, 2025

Why:

Fixed a security vulnerability where sensitive data (API keys, tokens, environment variables) in MCP server configurations were being serialized as plain text instead of using Haystack's Secret class for proper protection.

What:

  • Core component: Enhanced MCPServerInfo classes (SSEServerInfo, StreamableHttpServerInfo, StdioServerInfo) to support Haystack Secret types
  • Key features:
    • Secure serialization that omits plain text sensitive data from output
    • Backward-compatible Secret resolution in MCP client classes
    • Comprehensive field metadata system for automatic secret detection
  • Integration: Follows Haystack's established Secret handling patterns used across other integrations (Anthropic, OpenAI, etc.)

How can it be used:

Recommended approach (with Secrets):

from haystack.utils import Secret
from haystack_integrations.tools.mcp import MCPTool

# SSE Server with Secret token
sse_info = SSEServerInfo(
    url="https://api.example.com/sse",
    token=Secret.from_env_var("API_TOKEN")  # Secure
)

# Stdio Server with Secret environment variables
stdio_info = StdioServerInfo(
    command="my-mcp-server",
    args=["--config", "prod"],
    env={
        "API_KEY": Secret.from_env_var("MY_API_KEY"),  # Secure
        "DATABASE_URL": Secret.from_env_var("DB_URL")  # Secure
    }
)

# Create MCP tool with secure configuration
mcp_tool = MCPTool(server_info=sse_info)

Backward-compatible approach (plain strings):

# Still works, but plain strings are omitted from serialization for security
stdio_info = StdioServerInfo(
    command="my-mcp-server", 
    env={"PUBLIC_CONFIG": "value"}  # Plain strings omitted from serialization
)

How did you test it:

  • Unit tests: Updated serialization tests to verify Secret objects are properly serialized while plain strings in sensitive fields are omitted for security
  • Security tests: Confirmed sensitive data protection through comprehensive serialization behavior validation

Notes for the reviewer:

  • Compatibility: Maintains full backward compatibility - existing code continues to work unchanged
  • Dependencies: No new external dependencies added, leverages existing Haystack Secret infrastructure
  • Pattern consistency: Follows the exact same Secret handling pattern in Haystack
  • Documentation: Added comprehensive docstrings with clear examples of both secure and backward-compatible usage patterns

@vblagoje vblagoje requested a review from a team as a code owner June 5, 2025 12:39
@vblagoje vblagoje requested review from davidsbatista and removed request for a team June 5, 2025 12:39
@github-actions github-actions bot added integration:mcp type:documentation Improvements or additions to documentation labels Jun 5, 2025
@vblagoje vblagoje requested a review from Amnah199 June 5, 2025 12:46
@vblagoje
Copy link
Member Author

vblagoje commented Jun 5, 2025

@davidsbatista I think it makes more sense to ask @Amnah199 for a review as she is much more familiar with this code and will maintain it with me. But please by all means have a look as well.

@vblagoje
Copy link
Member Author

vblagoje commented Jun 5, 2025

Tested with itinerary agent, which uses mcp heavily, e2e for ay potential issues - none found!

Copy link
Contributor

@davidsbatista davidsbatista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vblagoje
Copy link
Member Author

vblagoje commented Jun 6, 2025

Thank you @davidsbatista , @Amnah199 could you please prfioritize review of this one :-)

@vblagoje
Copy link
Member Author

@Amnah199 could you please also take a quick look?

Copy link
Contributor

@Amnah199 Amnah199 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Left a few comments

@vblagoje vblagoje requested review from Amnah199 June 10, 2025 11:50
@vblagoje
Copy link
Member Author

Spot on @Amnah199 - much simpler now - please have another look again

Copy link
Contributor

@Amnah199 Amnah199 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. LGTM!

@vblagoje vblagoje merged commit bef6a26 into main Jun 10, 2025
10 checks passed
@vblagoje vblagoje deleted the mcp_env_vars_serialization branch June 10, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration:mcp type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

env vars set on StdioServerInfo are logged as plain text to external tracing tools (langfuse)
3 participants