fix: close tenant-isolation gaps in security hardening#14044
Merged
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:
WalkthroughThe PR adds MCP runtime hardening and package allowlists, tenant-scoped local-file enforcement, expanded public-flow secret scrubbing, authenticated MCP cache partitioning, refreshed embedded components, and regression tests across Docker, MCP, file-security, and flow snapshots. ChangesMCP execution hardening
Tenant-scoped file access
Public-flow secret scrubbing
Starter flow schema refresh
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (6 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 |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.11.0 #14044 +/- ##
==================================================
+ Coverage 60.16% 60.40% +0.24%
==================================================
Files 2333 2341 +8
Lines 228353 230126 +1773
Branches 32038 34353 +2315
==================================================
+ Hits 137386 139016 +1630
- Misses 89351 89438 +87
- Partials 1616 1672 +56
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
bug
erichare
force-pushed
the
security-hardening-p1-followups
branch
from
e6b96b3 to
359344b
Compare
github-actions
Bot
added
bug
This comment has been minimized.
This comment has been minimized.
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: 9
🧹 Nitpick comments (2)
src/lfx/tests/unit/services/settings/test_settings_composition.py (1)
252-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock down the interpreter-hardening default.
Add
assert settings.mcp_server_interpreter_hardening is Falsealongside the other new security defaults to preserve the documented single-tenant compatibility contract.As per coding guidelines, tests must verify newly changed behavior rather than act 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/lfx/tests/unit/services/settings/test_settings_composition.py` around lines 252 - 255, Add an assertion for settings.mcp_server_interpreter_hardening being False in the settings composition test, alongside the existing security-default assertions. Ensure the test verifies the documented single-tenant compatibility default without changing the other assertions.Source: Coding guidelines
src/lfx/src/lfx/utils/file_path_security.py (1)
133-177: 🩺 Stability & Availability | 🔵 TrivialScope narrowing is a breaking change for deployments already running with restriction enabled.
Per the line-range summary, this changes the containment check from "path is inside
config_dir" to "path is inside at least one allowed scope root." For any deployment that already hadLANGFLOW_RESTRICT_LOCAL_FILE_ACCESS=truebefore this change, files previously saved/read directly underconfig_dir(outside any user/flow subdirectory) will now be rejected — silently breakingSaveToFileComponentappend-mode continuity (a new, differently-scoped file gets created instead of appending) and any component reading a previously-valid absolute path from that era.This is the right security direction, but worth a migration/upgrade note (or a one-time compatibility fallback) so operators upgrading with the flag already on aren't surprised by files "disappearing" or appends silently restarting.
🤖 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/utils/file_path_security.py` around lines 133 - 177, The updated scope containment in enforce_local_file_access breaks existing restricted deployments using files directly under config_dir. Add an explicit migration or upgrade note describing this behavior and the required relocation of legacy files into user/flow storage scopes; do not broaden the security check or add a compatibility fallback unless the project’s established migration policy requires it.
🤖 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/backend/base/langflow/api/utils/core.py`:
- Around line 165-175: Update _contains_url_credentials to inspect network-path
and relative URL references by removing the absolute-scheme requirement, while
preserving safe handling of invalid values. Detect credentials in parsed
userinfo and secret-named parameters from both query and fragment components. In
src/backend/tests/unit/api/v1/test_public_flow_secret_scrub.py lines 60-65, add
endpoint regressions covering ?api_key=..., `#access_token`=..., and
//user:password@host.
In `@src/backend/tests/unit/api/v1/test_public_flow_secret_scrub.py`:
- Around line 60-65: Expand the credential-scrubbing tests around the
authenticated_url fixture to cover absolute URLs with secret query parameters,
relative and network-path URLs, and secret fragments. Assert that each
credential value is scrubbed while the URL structure remains intact, exercising
the parse_qsl branch and the leak fixed in core.py.
In `@src/backend/tests/unit/test_docker_security_defaults.py`:
- Around line 15-20: Add an assertion to
test_published_images_enable_mcp_hardening for
LANGFLOW_MCP_SERVER_ALLOWED_PACKAGES=mcp-proxy,lfx, ensuring every
PUBLISHED_DOCKERFILES image preserves the documented package-runner allowlist
alongside the existing hardening settings.
In `@src/lfx/src/lfx/_assets/component_index.json`:
- Around line 7049-7050: Remove the character-based file path blocklist in
_process_docling_subprocess_impl, including the check for semicolons, pipes,
ampersands, and backticks. Pass the validated string path through the existing
argument-list subprocess invocation without rejecting legitimate filenames,
while preserving the current subprocess isolation and JSON stdin handling.
- Around line 7049-7050: The subprocess handling in
_process_docling_subprocess_impl can deadlock because stdout and stderr are read
only after proc.poll() exits, allowing large child output to fill the pipes.
Update this method to continuously drain both streams while the child runs, or
redirect the child output to a temporary file, while preserving the timeout,
heartbeat logging, and existing JSON/error processing.
In `@src/lfx/src/lfx/base/mcp/security.py`:
- Around line 576-587: Update the wrapped-command handling around
_validate_package_runner and _validate_interpreter_invocation to apply the
selected Docker policy to nested_args whenever nested_base == "docker", so
wrapped commands such as sh -c docker invocations receive the same hardened or
lenient validation as top-level Docker commands. Add a regression test covering
a wrapped Docker command with a host-root volume mount.
- Around line 366-373: Update the shell-wrapper validation around
_is_shell_exec_flag and its corresponding logic near the other referenced
location so -c or /c is accepted only when it appears in the expected leading
position before any script operand. Do not allow later flags, such as those
following /tenant/evil.sh, to bypass the approved-command check; preserve
rejection of direct shell scripts.
In `@src/lfx/src/lfx/components/models_and_agents/mcp_component.py`:
- Around line 369-387: The update_build_config flow still accesses the shared
servers cache based only on use_cache, allowing unauthenticated cross-request
reuse. In update_build_config, derive and use the same use_shared_cache
condition as update_tool_list by requiring _mcp_cache_user_id() to be set before
reading or writing self._shared_component_cache["servers"], while preserving
per-instance caching. Add a regression test covering unauthenticated
build_config requests and asserting the shared cache is neither read nor
updated.
In `@src/lfx/tests/unit/utils/test_file_path_security.py`:
- Around line 133-138: Update test_reserved_secret_file_blocked and the related
reserved-file tests so each candidate remains within an allowed scope root while
also matching _reserved_secret_paths. Configure scope_ids and directory layout
accordingly, then assert LocalFileAccessError contains the “server-managed file”
message to verify the reserved-file check, not generic scope rejection.
---
Nitpick comments:
In `@src/lfx/src/lfx/utils/file_path_security.py`:
- Around line 133-177: The updated scope containment in
enforce_local_file_access breaks existing restricted deployments using files
directly under config_dir. Add an explicit migration or upgrade note describing
this behavior and the required relocation of legacy files into user/flow storage
scopes; do not broaden the security check or add a compatibility fallback unless
the project’s established migration policy requires it.
In `@src/lfx/tests/unit/services/settings/test_settings_composition.py`:
- Around line 252-255: Add an assertion for
settings.mcp_server_interpreter_hardening being False in the settings
composition test, alongside the existing security-default assertions. Ensure the
test verifies the documented single-tenant compatibility default without
changing the other assertions.
🪄 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: 167f2683-c1e3-4e25-aeea-9d6547af57c3
📒 Files selected for processing (39)
docker/build_and_push.Dockerfiledocker/build_and_push_backend.Dockerfiledocker/build_and_push_base.Dockerfiledocker/build_and_push_ep.Dockerfiledocker/build_and_push_with_extras.Dockerfiledocs/docs/Develop/api-keys-and-authentication.mdxsrc/backend/base/langflow/agentic/flows/SystemMessageGen.jsonsrc/backend/base/langflow/agentic/flows/TemplateAssistant.jsonsrc/backend/base/langflow/api/utils/core.pysrc/backend/base/langflow/initial_setup/starter_projects/Content Aggregator.jsonsrc/backend/base/langflow/initial_setup/starter_projects/Financial Report Parser.jsonsrc/backend/base/langflow/initial_setup/starter_projects/Hybrid Search RAG.jsonsrc/backend/base/langflow/initial_setup/starter_projects/Portfolio Website Code Generator.jsonsrc/backend/base/langflow/initial_setup/starter_projects/Structured Data Analysis Agent.jsonsrc/backend/base/langflow/initial_setup/starter_projects/Text Sentiment Analysis.jsonsrc/backend/tests/unit/agentic/flows/test_mcp_security_snapshots.pysrc/backend/tests/unit/api/v1/test_public_flow_secret_scrub.pysrc/backend/tests/unit/components/files_and_knowledge/test_directory_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/test_docker_security_defaults.pysrc/lfx/src/lfx/_assets/component_index.jsonsrc/lfx/src/lfx/base/data/base_file.pysrc/lfx/src/lfx/base/mcp/security.pysrc/lfx/src/lfx/components/data_source/csv_to_data.pysrc/lfx/src/lfx/components/data_source/json_to_data.pysrc/lfx/src/lfx/components/files_and_knowledge/directory.pysrc/lfx/src/lfx/components/files_and_knowledge/file.pysrc/lfx/src/lfx/components/files_and_knowledge/save_file.pysrc/lfx/src/lfx/components/langchain_utilities/csv_agent.pysrc/lfx/src/lfx/components/langchain_utilities/json_agent.pysrc/lfx/src/lfx/components/langchain_utilities/openapi.pysrc/lfx/src/lfx/components/models_and_agents/mcp_component.pysrc/lfx/src/lfx/services/settings/groups/mcp.pysrc/lfx/src/lfx/services/settings/groups/security.pysrc/lfx/src/lfx/utils/file_path_security.pysrc/lfx/tests/unit/mcp/test_mcp_stdio_security.pysrc/lfx/tests/unit/services/settings/test_settings_composition.pysrc/lfx/tests/unit/utils/test_file_path_security.py
Collaborator
Author
|
Also addressed the two CodeRabbit review-summary nitpicks in 3e3b70b: added the interpreter-hardening default assertion and documented the restricted-file upgrade/relocation behavior. |
github-actions
Bot
added
bug
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
bug
…rdening-p1-followups # Conflicts: # src/lfx/src/lfx/_assets/component_index.json
github-actions
Bot
added
bug
Contributor
|
Build successful! ✅ |
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
Stacked security follow-up to #13530. This PR targets
security-hardeningso the original PR can enter the merge queue unchanged.npx/uvxMCP transports to operator-approved packages, rejecting direct URLs, paths, and package-injection optionsSecurity invariants
Validation
security-hardeningSummary by CodeRabbit
New Features
Bug Fixes
Documentation