fix(security): harden MCP stdio configuration#14036
Open
erichare wants to merge 6 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughMCP stdio security validation is centralized in shared policy modules and applied across API schemas, runtime connections, tool updates, and legacy components. Tests now cover command boundaries, shell wrappers, environment variables, package sources, Docker arguments, structured configurations, uploads, and frontend persistence. ChangesMCP stdio security
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPServerConfig
participant update_tools
participant MCPStdioClient
participant Session
MCPServerConfig->>update_tools: command, args, env
update_tools->>MCPStdioClient: validated stdio configuration
MCPStdioClient->>Session: revalidate before spawn
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
erichare
changed the title
github-actions
Bot
added
bug
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/tests/unit/components/models_and_agents/test_mcp_component.py (1)
731-761: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest doesn't actually assert the API-provided config was used.
The trailing comment "Connect should be called with API-provided config as fallback" (Line 760) has no corresponding assertion —
mock_connectis never checked. Since this test was touched to represent the new{command, args}contract, it should verifymock_connectwas called with the structuredapi_provided_config(e.g., asserting the composed command/args), not just thatget_serverwas queried.🧪 Suggested assertion
# Database should be queried first mock_get_server.assert_called_once() # Connect should be called with API-provided config as fallback + mock_connect.assert_called_once() + full_command = mock_connect.call_args.args[0] + assert full_command == "uvx mcp-server-api-new --api-mode"Based on path instructions for
**/test_*.py: "verify the tests actually cover the new or changed behavior rather than acting as placeholders."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/components/models_and_agents/test_mcp_component.py` around lines 731 - 761, Complete test_rest_api_new_server_scenario by asserting mock_connect was called with the structured api_provided_config fallback, including its command, args, and env values. Keep the existing get_server assertion and verify the connection call reflects the new {command, args} contract rather than leaving the trailing comment untested.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lfx/src/lfx/base/mcp/security.py`:
- Around line 33-47: Update src/lfx/src/lfx/base/mcp/security.py:33-47 and
validate_mcp_stdio_config so standard npx confirmation flags are not rejected as
dangerous keywords, while preserving blocking of actual package-source or
execution-risk keywords. Update
src/lfx/tests/unit/mcp/test_stdio_source_policy.py:9-13 to route non-shell
allowed cases through validate_mcp_stdio_config, ensuring npx -y is validated by
the same runtime gate rather than only the source-policy helper.
---
Outside diff comments:
In `@src/backend/tests/unit/components/models_and_agents/test_mcp_component.py`:
- Around line 731-761: Complete test_rest_api_new_server_scenario by asserting
mock_connect was called with the structured api_provided_config fallback,
including its command, args, and env values. Keep the existing get_server
assertion and verify the connection call reflects the new {command, args}
contract rather than leaving the trailing comment untested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d31ba09-5bbf-4227-9cc6-f1639908c62e
📒 Files selected for processing (15)
src/backend/base/langflow/api/v2/schemas.pysrc/backend/tests/unit/api/v2/test_mcp_servers_file.pysrc/backend/tests/unit/base/mcp/test_mcp_util.pysrc/backend/tests/unit/components/models_and_agents/test_mcp_component.pysrc/backend/tests/unit/components/models_and_agents/test_mcp_component_cache.pysrc/backend/tests/unit/components/models_and_agents/test_mcp_component_dynamic.pysrc/backend/tests/unit/components/models_and_agents/test_mcp_component_flow_reload.pysrc/backend/tests/unit/test_mcp_command_injection_security.pysrc/frontend/tests/extended/features/mcp-server.spec.tssrc/lfx/src/lfx/base/mcp/security.pysrc/lfx/src/lfx/base/mcp/source_policy.pysrc/lfx/src/lfx/base/mcp/util.pysrc/lfx/src/lfx/components/deactivated/mcp_stdio.pysrc/lfx/tests/unit/mcp/test_mcp_runtime_config_validation.pysrc/lfx/tests/unit/mcp/test_stdio_source_policy.py
Comment on lines
+33
to
+47
| DANGEROUS_KEYWORDS = frozenset( | ||
| { | ||
| "-c", | ||
| "-e", | ||
| "-y", | ||
| "--yes", | ||
| "pip", | ||
| "install", | ||
| "npm", | ||
| "yarn", | ||
| "pnpm", | ||
| "eval", | ||
| "exec", | ||
| } | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
npx -y is blocked by policy, and the source-policy test masks it. Root cause: -y/--yes live in DANGEROUS_KEYWORDS, so validate_mcp_stdio_config rejects the standard non-interactive npx -y @scope/server`` launch at every runtime gate; the source-policy-only test helper never exercises that gate, so its allowed-list entry gives false confidence.
src/lfx/src/lfx/base/mcp/security.py#L33-L47: confirm blocking-y/--yesis intended; if not, exempt npx confirmation flags (they don't select a package source), otherwise legitimatenpx -yconfigs break.src/lfx/tests/unit/mcp/test_stdio_source_policy.py#L9-L13: route non-shell commands throughvalidate_mcp_stdio_configso the allowed cases mirror the real runtime gate (this currently fails fornpx -y, exposing the tension).
#!/bin/bash
# Confirm npx -y is rejected by the full config gate while source policy allows it.
python - <<'PY'
from lfx.base.mcp.security import validate_mcp_stdio_config
from lfx.base.mcp.source_policy import validate_mcp_stdio_source_policy
try:
validate_mcp_stdio_source_policy("npx", ["-y", "`@modelcontextprotocol/server-filesystem`", "/workspace"])
print("source_policy: ALLOWED")
except Exception as e:
print("source_policy: REJECTED ->", e)
try:
validate_mcp_stdio_config("npx", ["-y", "`@modelcontextprotocol/server-filesystem`", "/workspace"], None)
print("full_config: ALLOWED")
except Exception as e:
print("full_config: REJECTED ->", e)
PY📍 Affects 2 files
src/lfx/src/lfx/base/mcp/security.py#L33-L47(this comment)src/lfx/tests/unit/mcp/test_stdio_source_policy.py#L9-L13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lfx/src/lfx/base/mcp/security.py` around lines 33 - 47, Update
src/lfx/src/lfx/base/mcp/security.py:33-47 and validate_mcp_stdio_config so
standard npx confirmation flags are not rejected as dangerous keywords, while
preserving blocking of actual package-source or execution-risk keywords. Update
src/lfx/tests/unit/mcp/test_stdio_source_policy.py:9-13 to route non-shell
allowed cases through validate_mcp_stdio_config, ensuring npx -y is validated by
the same runtime gate rather than only the source-policy helper.
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
bug
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.10.3 #14036 +/- ##
=================================================
Coverage ? 58.58%
=================================================
Files ? 2309
Lines ? 220446
Branches ? 31229
=================================================
Hits ? 129140
Misses ? 89771
Partials ? 1535
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
commandand options inargs, while preserving executable paths with directory spaces and the legacy packed defaultTickets
Testing
Summary by CodeRabbit
New Features
Bug Fixes