fix: validate flow file_save path is in a valid location#11039
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduced storage-aware, path-safe filesystem operations for flow management. Added helper functions to validate and sanitize file paths against directory traversal and null byte attacks, ensuring paths remain within user-specific directories. Updated flow creation, updating, and file upload endpoints to use the StorageService dependency for path validation before filesystem writes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (7 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/backend/tests/unit/api/v1/test_flows_path_validation.py (1)
38-55: Consider moving imports to module level.Lines 41-42 import
anyioandStdlibPathwithin the test function, butanyiois already imported at the module level (line 8). Consider importingStdlibPathat the module level as well for consistency.Apply this diff:
from fastapi import HTTPException +from pathlib import Path as StdlibPath from unittest.mock import MagicMock from uuid import uuid4 import anyio from langflow.api.v1.flows import _get_safe_flow_path from langflow.services.storage.service import StorageServiceThen remove the redundant imports from the test:
async def test_accepts_absolute_path_within_allowed_directory(self, mock_storage_service, user_id, tmp_path): """Test that absolute paths within the user's flows directory are accepted.""" - import anyio - from pathlib import Path as StdlibPath - mock_storage_service.data_dir = anyio.Path(tmp_path)src/backend/tests/unit/api/v1/test_flows.py (1)
317-489: Excellent security test coverage!These tests comprehensively validate the path security measures across create, update, and upload operations. The tests cover:
- Rejection of malicious paths (absolute outside directory, directory traversal, null bytes, Windows paths)
- Acceptance of valid paths (relative, nested)
- Edge cases (empty string vs None)
The error message assertions on lines 338 and 350 use OR conditions (e.g.,
"directory traversal" in ... or "absolute paths" in ...), which provides flexibility but could potentially mask incorrect error messages. This is acceptable for robustness, but consider tightening these assertions if you want to enforce specific error messages for each scenario.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/backend/base/langflow/api/v1/flows.py(8 hunks)src/backend/tests/unit/api/v1/test_flows.py(3 hunks)src/backend/tests/unit/api/v1/test_flows_path_validation.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
src/backend/**/*.py: Use FastAPI async patterns withawaitfor async operations in component execution methods
Useasyncio.create_task()for background tasks and implement proper cleanup with try/except forasyncio.CancelledError
Usequeue.put_nowait()for non-blocking queue operations andasyncio.wait_for()with timeouts for controlled get operations
Files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.pysrc/backend/base/langflow/api/v1/flows.py
src/backend/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
src/backend/tests/**/*.py: Place backend unit tests insrc/backend/tests/directory, component tests insrc/backend/tests/unit/components/organized by component subdirectory, and integration tests accessible viamake integration_tests
Use same filename as component with appropriate test prefix/suffix (e.g.,my_component.py→test_my_component.py)
Use theclientfixture (FastAPI Test Client) defined insrc/backend/tests/conftest.pyfor API tests; it provides an asynchttpx.AsyncClientwith automatic in-memory SQLite database and mocked environment variables. Skip client creation by marking test with@pytest.mark.noclient
Inherit from the correctComponentTestBasefamily class located insrc/backend/tests/base.pybased on API access needs:ComponentTestBase(no API),ComponentTestBaseWithClient(needs API), orComponentTestBaseWithoutClient(pure logic). Provide three required fixtures:component_class,default_kwargs, andfile_names_mapping
Create comprehensive unit tests for all new backend components. If unit tests are incomplete, create a corresponding Markdown file documenting manual testing steps and expected outcomes
Test both sync and async code paths, mock external dependencies appropriately, test error handling and edge cases, validate input/output behavior, and test component initialization and configuration
Use@pytest.mark.asynciodecorator for async component tests and ensure async methods are properly awaited
Test background tasks usingasyncio.create_task()and verify completion withasyncio.wait_for()with appropriate timeout constraints
Test queue operations using non-blockingqueue.put_nowait()andasyncio.wait_for(queue.get(), timeout=...)to verify queue processing without blocking
Use@pytest.mark.no_blockbustermarker to skip the blockbuster plugin in specific tests
For database tests that may fail in batch runs, run them sequentially usinguv run pytest src/backend/tests/unit/test_database.pyr...
Files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
**/test_*.py
📄 CodeRabbit inference engine (Custom checks)
**/test_*.py: Review test files for excessive use of mocks that may indicate poor test design - check if tests have too many mock objects that obscure what's actually being tested
Warn when mocks are used instead of testing real behavior and interactions, and suggest using real objects or test doubles when mocks become excessive
Ensure mocks are used appropriately for external dependencies only, not for core logic
Backend test files should follow the naming convention test_*.py with proper pytest structure
Test files should have descriptive test function names that explain what is being tested
Tests should be organized logically with proper setup and teardown
Consider including edge cases and error conditions for comprehensive test coverage
Verify tests cover both positive and negative scenarios where appropriate
For async functions in backend tests, ensure proper async testing patterns are used with pytest
For API endpoints, verify both success and error response testing
Files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
src/backend/base/langflow/api/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
Backend API endpoints should be organized by version (v1/, v2/) under
src/backend/base/langflow/api/with specific modules for features (chat.py, flows.py, users.py, etc.)
Files:
src/backend/base/langflow/api/v1/flows.py
🧠 Learnings (7)
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Use predefined JSON flows and utility functions from `tests.unit.build_utils` (create_flow, build_flow, get_build_events, consume_and_assert_stream) for flow execution testing
Applied to files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Use `aiofiles` and `anyio.Path` for async file operations in tests; create temporary test files using `tmp_path` fixture and verify file existence and content
Applied to files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Test Langflow REST API endpoints using the `client` fixture with appropriate HTTP methods (GET, POST, etc.), headers (logged_in_headers), and payload validation
Applied to files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Test component versioning and backward compatibility using `file_names_mapping` fixture with `VersionComponentMapping` objects mapping component files across Langflow versions
Applied to files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
📚 Learning: 2025-11-24T19:46:09.104Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/backend_development.mdc:0-0
Timestamp: 2025-11-24T19:46:09.104Z
Learning: Applies to tests/unit/**/*.py : Database and flow integration tests should use helper functions from `tests.unit.build_utils` such as `create_flow`, `build_flow`, and `get_build_events`
Applied to files:
src/backend/tests/unit/api/v1/test_flows.py
📚 Learning: 2025-11-24T19:47:28.997Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T19:47:28.997Z
Learning: Applies to src/backend/tests/**/*.py : Test both sync and async code paths, mock external dependencies appropriately, test error handling and edge cases, validate input/output behavior, and test component initialization and configuration
Applied to files:
src/backend/tests/unit/api/v1/test_flows.pysrc/backend/tests/unit/api/v1/test_flows_path_validation.py
📚 Learning: 2025-11-24T19:46:09.104Z
Learnt from: CR
Repo: langflow-ai/langflow PR: 0
File: .cursor/rules/backend_development.mdc:0-0
Timestamp: 2025-11-24T19:46:09.104Z
Learning: Applies to tests/**/*.py : Use `client` fixture from `conftest.py` and `logged_in_headers` for testing authenticated FastAPI endpoints
Applied to files:
src/backend/tests/unit/api/v1/test_flows.py
🧬 Code graph analysis (3)
src/backend/tests/unit/api/v1/test_flows.py (1)
src/backend/tests/conftest.py (1)
logged_in_headers(503-509)
src/backend/tests/unit/api/v1/test_flows_path_validation.py (1)
src/backend/base/langflow/api/v1/flows.py (1)
_get_safe_flow_path(47-108)
src/backend/base/langflow/api/v1/flows.py (2)
src/backend/base/langflow/services/deps.py (1)
get_storage_service(91-99)src/backend/base/langflow/services/storage/service.py (1)
StorageService(19-91)
🪛 GitHub Actions: Ruff Style Check
src/backend/base/langflow/api/v1/flows.py
[error] 49-49: W293 Blank line contains whitespace
🪛 GitHub Check: Ruff Style Check (3.13)
src/backend/base/langflow/api/v1/flows.py
[failure] 107-107: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:107:1: W293 Blank line contains whitespace
[failure] 105-105: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:105:1: W293 Blank line contains whitespace
[failure] 88-88: Ruff (E501)
src/backend/base/langflow/api/v1/flows.py:88:121: E501 Line too long (168 > 120)
[failure] 72-72: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:72:1: W293 Blank line contains whitespace
[failure] 64-64: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:64:1: W293 Blank line contains whitespace
[failure] 60-60: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:60:1: W293 Blank line contains whitespace
[failure] 59-59: Ruff (Q003)
src/backend/base/langflow/api/v1/flows.py:59:53: Q003 Change outer quotes to avoid escaping inner quotes
[failure] 57-57: Ruff (Q003)
src/backend/base/langflow/api/v1/flows.py:57:53: Q003 Change outer quotes to avoid escaping inner quotes
[failure] 54-54: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:54:1: W293 Blank line contains whitespace
[failure] 49-49: Ruff (W293)
src/backend/base/langflow/api/v1/flows.py:49:1: W293 Blank line contains whitespace
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Test Docker Images / Test docker images
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Run Backend Tests / LFX Tests - Python 3.10
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Run Frontend Tests / Determine Test Suites and Shard Distribution
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Test Starter Templates
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Update Component Index
- GitHub Check: Update Starter Projects
🔇 Additional comments (10)
src/backend/base/langflow/api/v1/flows.py (5)
111-121: LGTM! Path verification logic is sound.The function correctly distinguishes between
None(allowed) and empty string (rejected), and properly prepares the filesystem by creating parent directories and touching the file if needed.
123-138: LGTM! File saving logic is well-structured.The function properly validates paths, handles async file operations, and provides appropriate error handling with logging for troubleshooting.
236-269: LGTM! Storage service integration is correct.The endpoint properly injects the
StorageServicedependency and integrates it into the flow creation workflow. The flow is refreshed before filesystem save, ensuring consistency.
405-475: LGTM! Update flow logic properly validates path changes.The endpoint correctly validates
fs_pathonly when it's been updated, avoiding unnecessary validation of unchanged paths. The storage service integration follows the same pattern as flow creation.
519-567: LGTM! Upload endpoint properly handles multiple flows.The endpoint correctly processes multiple flows from uploads, validating and saving each one with appropriate error handling.
src/backend/tests/unit/api/v1/test_flows_path_validation.py (2)
1-26: Test fixtures are well-designed.The fixtures provide good isolation and proper mocking of the
StorageService. Thetmp_pathfixture ensures each test has a clean temporary directory.
155-171: LGTM! Path resolution test is thorough.This test effectively validates that the path resolution logic prevents symlink attacks by ensuring resolved paths stay within the base directory. The use of
StdlibPath.resolve()for verification mirrors the production code's security checks.src/backend/tests/unit/api/v1/test_flows.py (3)
10-45: LGTM! Test properly adapted to new path handling.The test now uses a relative path as expected by the new security validation. The focus on API response validation is appropriate, with filesystem validation covered by dedicated path validation tests.
101-143: LGTM! Update test correctly uses relative paths.The test properly validates the flow update API with the new relative path approach. Checking the updated name ensures the update operation worked correctly.
183-314: LGTM! Comprehensive user isolation test.This test thoroughly validates that flows are properly isolated per user, with appropriate cleanup to prevent test pollution. The use of a real second user in the database makes this a realistic integration test.
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (39.26%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11039 +/- ##
==========================================
+ Coverage 33.09% 33.14% +0.05%
==========================================
Files 1389 1389
Lines 65714 65769 +55
Branches 9730 9730
==========================================
+ Hits 21747 21799 +52
- Misses 42848 42851 +3
Partials 1119 1119
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Validates the
fs_pathused in the POST flow create endpoint is within the user flow directory. Additionally adds security guards to ensure traversal is no longer allowed.While the FE does not currently use
fs_path, this is a change in behavior where users attempting to save to a non-flow-directory location will begin to receive 4xx. Since this represents a security risk, this is acceptable and desired.