mirror of
https://github.com/langflow-ai/langflow.git
synced 2026-07-26 21:14:42 +08:00
fix(security): use CodeQL-recognized sanitizer for path-injection alerts #205-213 (#12841)
* 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/<user-uuid>) 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.
This commit is contained in:
@ -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"
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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.
|
||||
|
||||
Reference in New Issue
Block a user