From d3b60fa898166c03b8e62544d687d1518ad4dee2 Mon Sep 17 00:00:00 2001 From: Eric Hare Date: Wed, 22 Apr 2026 14:12:40 -0700 Subject: [PATCH] fix(security): use CodeQL-recognized sanitizer for path-injection alerts #205-213 (#12841) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): use CodeQL-recognised sanitiser for path-injection sinks Addresses CodeQL alerts #205-213 (py/path-injection, high). All three modules already contained path-containment logic, but relied on ``pathlib.Path.resolve()`` + ``Path.is_relative_to`` / ``startswith``, which CodeQL's dataflow model does not recognise as a sanitiser — so the alerts kept firing on ``.exists()``/``.resolve()`` sinks. Rewrites the sanitisers using ``os.path.realpath`` + ``startswith`` — the pattern documented by CodeQL as the recognised traversal sanitiser — without changing the security guarantees. ``realpath`` canonicalises and follows symlinks, so the containment check is robust against both ``..`` sequences and symlink escapes. Files touched (one sanitiser per file): * ``agentic/services/helpers/flow_loader.py`` — replace ``_validate_path_within_base`` with ``_safe_resolved_path`` that returns the canonicalised path, then have ``resolve_flow_path`` use that canonicalised path for all downstream ``.exists()`` calls. * ``api/v1/files.py`` — rewrite the profile-picture lookup to derive the candidate path from ``realpath`` of base + user components and verify containment before any filesystem access. * ``api/v1/flows_helpers.py`` — collapse the absolute/relative branches of ``_get_safe_flow_path`` into a single ``realpath`` + ``startswith`` containment check. Drops the now- unused ``pathlib.Path as StdlibPath`` import. No behavioural changes: all 92 existing path-validation/flow-loader unit tests and 54 ``files.py`` tests still pass. * fix(security): don't leak resolved base path in flow path error detail The containment-failure branch for absolute paths returned the fully resolved flows base directory (e.g. /var/lib/langflow/flows/) in the HTTP 400 detail, exposing server filesystem layout and the user's internal path component. Replace with a static message — the specific resolved path adds no diagnostic value for the client and leaks internal detail. --- .../agentic/services/helpers/flow_loader.py | 33 ++++---- src/backend/base/langflow/api/v1/files.py | 29 ++++--- .../base/langflow/api/v1/flows_helpers.py | 78 +++++++------------ 3 files changed, 59 insertions(+), 81 deletions(-) diff --git a/src/backend/base/langflow/agentic/services/helpers/flow_loader.py b/src/backend/base/langflow/agentic/services/helpers/flow_loader.py index 62604c9ebb..6adb9bd479 100644 --- a/src/backend/base/langflow/agentic/services/helpers/flow_loader.py +++ b/src/backend/base/langflow/agentic/services/helpers/flow_loader.py @@ -7,6 +7,7 @@ When both exist, .py takes priority for gradual migration. import importlib.util import inspect import json +import os import sys from contextlib import contextmanager from pathlib import Path @@ -37,16 +38,19 @@ def _temporary_sys_path(path: str): yield -def _validate_path_within_base(flow_path: Path) -> None: - """Validate that the resolved path stays within FLOWS_BASE_PATH. +def _safe_resolved_path(flow_path: Path) -> Path: + """Resolve *flow_path* and confirm it stays within FLOWS_BASE_PATH. - Defense-in-depth: even after rejecting '..' substrings, resolve the - final path and confirm it is still under the allowed base directory. + Uses ``os.path.realpath`` + ``startswith`` — the sanitiser pattern + recognised by CodeQL's ``py/path-injection`` analysis — so the + returned path is safe to pass to filesystem operations such as + ``Path.exists()``. Raises HTTPException 400 on escape attempts. """ - resolved = flow_path.resolve() - base_resolved = FLOWS_BASE_PATH.resolve() - if not resolved.is_relative_to(base_resolved): + base_resolved = os.path.realpath(str(FLOWS_BASE_PATH)) + resolved = os.path.realpath(str(flow_path)) + if resolved != base_resolved and not resolved.startswith(base_resolved + os.sep): raise HTTPException(status_code=400, detail="Invalid flow filename") + return Path(resolved) def resolve_flow_path(flow_filename: str) -> tuple[Path, str]: @@ -69,15 +73,13 @@ def resolve_flow_path(flow_filename: str) -> tuple[Path, str]: raise HTTPException(status_code=400, detail=f"Invalid flow filename: '{flow_filename}'") if flow_filename.endswith(".json"): - flow_path = FLOWS_BASE_PATH / flow_filename - _validate_path_within_base(flow_path) + flow_path = _safe_resolved_path(FLOWS_BASE_PATH / flow_filename) if flow_path.exists(): return flow_path, "json" raise HTTPException(status_code=404, detail=f"Flow file '{flow_filename}' not found") if flow_filename.endswith(".py"): - flow_path = FLOWS_BASE_PATH / flow_filename - _validate_path_within_base(flow_path) + flow_path = _safe_resolved_path(FLOWS_BASE_PATH / flow_filename) if flow_path.exists(): return flow_path, "python" raise HTTPException(status_code=404, detail=f"Flow file '{flow_filename}' not found") @@ -85,19 +87,16 @@ def resolve_flow_path(flow_filename: str) -> tuple[Path, str]: # Auto-detect: try Python first, then JSON (allows gradual migration) base_name = flow_filename.rsplit(".", 1)[0] if "." in flow_filename else flow_filename - py_path = FLOWS_BASE_PATH / f"{base_name}.py" - _validate_path_within_base(py_path) + py_path = _safe_resolved_path(FLOWS_BASE_PATH / f"{base_name}.py") if py_path.exists(): return py_path, "python" - json_path = FLOWS_BASE_PATH / f"{base_name}.json" - _validate_path_within_base(json_path) + json_path = _safe_resolved_path(FLOWS_BASE_PATH / f"{base_name}.json") if json_path.exists(): return json_path, "json" # Try without adding extension - direct_path = FLOWS_BASE_PATH / flow_filename - _validate_path_within_base(direct_path) + direct_path = _safe_resolved_path(FLOWS_BASE_PATH / flow_filename) if direct_path.exists(): if direct_path.suffix == ".py": return direct_path, "python" diff --git a/src/backend/base/langflow/api/v1/files.py b/src/backend/base/langflow/api/v1/files.py index 9d859316db..2447b3e2da 100644 --- a/src/backend/base/langflow/api/v1/files.py +++ b/src/backend/base/langflow/api/v1/files.py @@ -190,31 +190,28 @@ async def download_profile_picture( extension = safe_file.split(".")[-1] config_dir = settings_service.settings.config_dir - config_path = Path(config_dir).resolve() # type: ignore[arg-type] - # Construct the file path - file_path = (config_path / "profile_pictures" / safe_folder / safe_file).resolve() - - # SECURITY: Verify the resolved path is still within the allowed directory - # This prevents path traversal even if symbolic links are involved. - # Uses os.path.normpath + startswith (the pattern recognised by CodeQL as a sanitiser). - allowed_base = str((config_path / "profile_pictures").resolve()) - if not (str(file_path).startswith(allowed_base + os.sep) or str(file_path) == allowed_base): + # SECURITY: use os.path.realpath + startswith — the sanitiser pattern + # recognised by CodeQL's py/path-injection analysis. realpath canonicalises + # the path and resolves symlinks, so the subsequent startswith check is + # robust against both traversal sequences and symlink-based escapes. + # os.path.join is deliberate here (PTH118) to match CodeQL's sanitiser model. + allowed_base = os.path.realpath(os.path.join(str(config_dir), "profile_pictures")) # noqa: PTH118 + candidate = os.path.realpath(os.path.join(allowed_base, safe_folder, safe_file)) # noqa: PTH118 + if candidate != allowed_base and not candidate.startswith(allowed_base + os.sep): raise HTTPException(status_code=404, detail="Profile picture not found") + file_path = Path(candidate) # Fallback to package bundled profile pictures if not found in config_dir if not file_path.exists(): from langflow.initial_setup import setup - package_base = Path(setup.__file__).parent / "profile_pictures" - package_path = (package_base / safe_folder / safe_file).resolve() - - # SECURITY: Verify package path is also within allowed directory - allowed_package_base = str(package_base.resolve()) - pkg_path_str = str(package_path) - if not (pkg_path_str.startswith(allowed_package_base + os.sep) or pkg_path_str == allowed_package_base): + package_base = os.path.realpath(str(Path(setup.__file__).parent / "profile_pictures")) + package_candidate = os.path.realpath(os.path.join(package_base, safe_folder, safe_file)) # noqa: PTH118 + if package_candidate != package_base and not package_candidate.startswith(package_base + os.sep): raise HTTPException(status_code=404, detail="Profile picture not found") + package_path = Path(package_candidate) if package_path.exists(): file_path = package_path else: diff --git a/src/backend/base/langflow/api/v1/flows_helpers.py b/src/backend/base/langflow/api/v1/flows_helpers.py index 8092985d41..9df22b5302 100644 --- a/src/backend/base/langflow/api/v1/flows_helpers.py +++ b/src/backend/base/langflow/api/v1/flows_helpers.py @@ -6,10 +6,10 @@ Extracted from flows.py to keep the route-handler module concise. from __future__ import annotations import io +import os import re import zipfile from datetime import datetime, timezone -from pathlib import Path as StdlibPath from typing import TYPE_CHECKING, Any from uuid import UUID @@ -43,6 +43,11 @@ def _get_safe_flow_path(fs_path: str, user_id: UUID, storage_service: StorageSer """Get a safe filesystem path for flow storage, restricted to user's flows directory. Allows both absolute and relative paths, but ensures they're within the user's flows directory. + + Uses ``os.path.realpath`` + ``startswith`` for containment — the sanitiser pattern + recognised by CodeQL's ``py/path-injection`` analysis. ``realpath`` canonicalises + the path and follows symlinks, so the returned path is safe to pass to filesystem + operations. """ if not fs_path: raise HTTPException(status_code=400, detail="fs_path cannot be empty") @@ -62,15 +67,10 @@ def _get_safe_flow_path(fs_path: str, user_id: UUID, storage_service: StorageSer detail="Invalid fs_path: null bytes are not allowed", ) - # Build the safe base directory path + # Build and canonicalise the safe base directory path. base_dir = storage_service.data_dir / "flows" / str(user_id) - base_dir_str = str(base_dir) - - # Normalize base directory path (resolve to absolute, handle symlinks) - # resolve() doesn't require the path to exist, it just resolves symlinks try: - base_dir_stdlib = StdlibPath(base_dir_str).resolve() - base_dir_resolved = str(base_dir_stdlib) + base_dir_resolved = os.path.realpath(str(base_dir)) except (OSError, ValueError) as e: raise HTTPException(status_code=400, detail=f"Invalid base directory: {e}") from e @@ -78,49 +78,31 @@ def _get_safe_flow_path(fs_path: str, user_id: UUID, storage_service: StorageSer is_absolute = normalized_path.startswith("/") or (len(normalized_path) > 1 and normalized_path[1] == ":") if is_absolute: - # Absolute path - resolve and validate it's within base directory - try: - requested_path = StdlibPath(normalized_path).resolve() - requested_resolved = str(requested_path) - # Ensure resolved path stays within base (prevent symlink attacks) - if not requested_resolved.startswith(base_dir_resolved + "/") and requested_resolved != base_dir_resolved: - raise HTTPException( - status_code=400, - detail=f"Absolute path must be within your flows directory: {base_dir_resolved}", - ) - # Reconstruct the path from the base directory + relative portion - # so the returned value is derived from the safe base, not user input. - rel = StdlibPath(requested_resolved).relative_to(base_dir_stdlib) - return Path(str(base_dir_stdlib / rel)) - except HTTPException: - raise - except (OSError, ValueError) as e: + candidate = normalized_path + else: + relative_part = normalized_path.lstrip("/") + # os.path.join is deliberate here (PTH118) to match CodeQL's sanitiser model. + candidate = os.path.join(base_dir_resolved, relative_part) if relative_part else base_dir_resolved # noqa: PTH118 + + try: + resolved_str = os.path.realpath(candidate) + except (OSError, ValueError) as e: + raise HTTPException(status_code=400, detail=f"Invalid path: {e}") from e + + # SECURITY: containment check using os.path.realpath + startswith (CodeQL-recognised). + if resolved_str != base_dir_resolved and not resolved_str.startswith(base_dir_resolved + os.sep): + if is_absolute: raise HTTPException( status_code=400, - detail=( - f"Invalid file save path: {e}. " - f"Verify that the path is within your flows directory: {base_dir_resolved}" - ), - ) from e - else: - # Relative path - validate that it's within the base directory - relative_part = normalized_path.lstrip("/") - safe_path_stdlib = base_dir_stdlib / relative_part if relative_part else base_dir_stdlib - try: - resolved_path = safe_path_stdlib.resolve() - resolved_str = str(resolved_path) + detail="Absolute path must be within your flows directory", + ) + raise HTTPException( + status_code=400, + detail="Invalid path: resolves outside allowed directory", + ) - # Ensure resolved path stays within base (prevent symlink attacks) - if not resolved_str.startswith(base_dir_resolved + "/") and resolved_str != base_dir_resolved: - raise HTTPException( - status_code=400, - detail="Invalid path: resolves outside allowed directory", - ) - except (OSError, ValueError) as e: - raise HTTPException(status_code=400, detail=f"Invalid path: {e}") from e - - # Return the resolved path to prevent TOCTOU symlink attacks - return Path(resolved_str) + # Return the canonicalised path — safe for subsequent filesystem operations. + return Path(resolved_str) # Fields that may be updated via setattr on a Flow ORM instance.