From 48c2e19d237a2a458d2c12ebbdcbf19aeccd5a73 Mon Sep 17 00:00:00 2001 From: Jordan Frazier Date: Sun, 7 Jun 2026 20:49:25 -0400 Subject: [PATCH] fix(security): deny server secrets/DB within the local-file-access boundary restrict_local_file_access confined reads to config_dir, but config_dir IS the storage dir AND holds the server-managed secrets as siblings of the per-flow upload subdirs: secret_key (Fernet master key), private_key.pem / public_key.pem (JWT signing keys), and the SQLite DB (save_db_in_config_dir). A tenant File-component input of "/../secret_key" routed through build_full_path (no '..' check) resolved back to /secret_key, passed the is_relative_to(config_dir) boundary, and was read -- disclosing the key that decrypts every tenant's stored credentials. The control thus failed its own stated goal under the very multi-tenant mode it adds. enforce_local_file_access now denies the exact config_dir locations of secret_key / private_key.pem / public_key.pem and the sqlite DB derived from database_url (including the -wal / -shm / -journal sidecars that hold the same row data, and the async sqlite+aiosqlite:/// + ?query URL forms), even though they resolve inside the boundary. Matched at exact location only, not by basename, so a tenant upload that merely shares a reserved name inside a flow subdir stays readable. Fails safe on non-sqlite / empty / unavailable settings. Known limitation (tracked separately): this does not scope reads per tenant, so cross-tenant reads of // uploads remain possible and need per-user/per-flow scoping in a follow-up. Tests: reserved file blocked, traversal-to-reserved blocked, DB + WAL/SHM/ journal sidecars blocked, async+query URL blocked, same-named upload in a flow subdir still allowed. --- src/lfx/src/lfx/utils/file_path_security.py | 61 ++++++++++++++++ .../unit/utils/test_file_path_security.py | 73 ++++++++++++++++++- 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/src/lfx/src/lfx/utils/file_path_security.py b/src/lfx/src/lfx/utils/file_path_security.py index b39e81f1f8..96556129e3 100644 --- a/src/lfx/src/lfx/utils/file_path_security.py +++ b/src/lfx/src/lfx/utils/file_path_security.py @@ -8,10 +8,24 @@ When ``LANGFLOW_RESTRICT_LOCAL_FILE_ACCESS`` is enabled, resolved local file pat within the storage data directory (``settings.config_dir``), where uploads live. The check is a no-op when the setting is disabled (OSS default), so single-tenant deployments keep the existing "read any local file by absolute path" behavior. + +Reserved-secret denial: the storage data directory IS ``config_dir``, which also holds the +server-managed secret files as siblings of the per-flow upload subdirectories — the Fernet +master key (``secret_key``), the JWT signing keys (``private_key.pem`` / ``public_key.pem``), +and the SQLite DB when ``save_db_in_config_dir`` is set. The storage-boundary check alone would +permit reading those (e.g. ``/../secret_key`` resolves back inside ``config_dir``), which +would defeat the control's purpose — reading ``secret_key`` discloses every tenant's stored +credentials. So those exact files are denied explicitly even though they sit inside the boundary. + +KNOWN LIMITATION (tracked for follow-up, see ``.claude/security-audit-findings.md``): this does +NOT scope reads per tenant — any ``//`` upload is still in +bounds, so one tenant can read another tenant's uploaded files. Closing that requires +per-user/per-flow scoping at the call sites and is deferred. """ from __future__ import annotations +import contextlib from pathlib import Path from lfx.logging import logger @@ -22,6 +36,13 @@ class LocalFileAccessError(ValueError): """Raised when a resolved path escapes the allowed storage root under restriction.""" +# Server-managed secret/key file names that live directly under config_dir (see auth.py: +# ``secret_key``, ``private_key.pem``, ``public_key.pem``). Matched only at their exact +# config_dir location, never by basename — a tenant upload happens to be named "secret_key" +# inside a flow subdir is a different path and stays readable. +_RESERVED_SECRET_FILENAMES = frozenset({"secret_key", "private_key.pem", "public_key.pem"}) + + def is_local_file_access_restricted() -> bool: """Return True if local file access is restricted to the storage directory.""" try: @@ -34,6 +55,36 @@ def is_local_file_access_restricted() -> bool: return False +def _reserved_secret_paths(data_dir: Path) -> set[Path]: + """Resolved paths of server-managed secret/key/DB files under the storage dir. + + Reading any of these would compromise the deployment (the Fernet master key decrypts every + tenant's credentials; the ``*.pem`` keys allow auth-token forgery; the SQLite DB holds all + rows), so they are denied even though they resolve inside the containment boundary. + """ + reserved = {(data_dir / name).resolve() for name in _RESERVED_SECRET_FILENAMES} + + # Add the SQLite DB file when it lives under config_dir (``save_db_in_config_dir``). + # database_url is assembled as ``sqlite:///`` (see settings/base.py); the + # async ``sqlite+aiosqlite:///`` form is also covered by the ``sqlite`` prefix. + try: + db_url = get_settings_service().settings.database_url or "" + except Exception: # noqa: BLE001 - settings may be unavailable; nothing to add + db_url = "" + if db_url.startswith("sqlite") and ":///" in db_url: + # Drop any ``?query`` so a custom LANGFLOW_DATABASE_URL still resolves to the file. + db_path_str = db_url.split(":///", 1)[1].split("?", 1)[0] + if db_path_str: + with contextlib.suppress(OSError): + db_path = Path(db_path_str).resolve() + reserved.add(db_path) + # WAL/SHM/journal sidecars hold un-checkpointed DB pages (the same row data), + # so they must be denied alongside the main DB file. + for suffix in ("-wal", "-shm", "-journal"): + reserved.add(Path(str(db_path) + suffix)) + return reserved + + def enforce_local_file_access(resolved_path: str | Path) -> Path: """Ensure a resolved local path is inside the storage data dir when restriction is on. @@ -68,4 +119,14 @@ def enforce_local_file_access(resolved_path: str | Path) -> Path: "(LANGFLOW_RESTRICT_LOCAL_FILE_ACCESS=true). Use an uploaded file instead." ) raise LocalFileAccessError(msg) + + # The storage dir is config_dir, which also holds server-managed secret/key/DB files as + # siblings of the upload subdirs; deny those explicitly (a traversal like "/../secret_key" + # resolves back inside the boundary but must not be readable). + if candidate in _reserved_secret_paths(data_dir): + msg = ( + "Access to this server-managed file is not permitted " + "(LANGFLOW_RESTRICT_LOCAL_FILE_ACCESS=true)." + ) + raise LocalFileAccessError(msg) return path diff --git a/src/lfx/tests/unit/utils/test_file_path_security.py b/src/lfx/tests/unit/utils/test_file_path_security.py index 83c3719bde..09159d367f 100644 --- a/src/lfx/tests/unit/utils/test_file_path_security.py +++ b/src/lfx/tests/unit/utils/test_file_path_security.py @@ -13,11 +13,13 @@ from lfx.utils.file_path_security import ( @contextmanager -def mock_settings(*, restricted: bool, config_dir: str): +def mock_settings(*, restricted: bool, config_dir: str, database_url: str = ""): with patch("lfx.utils.file_path_security.get_settings_service") as mock_get: settings = MagicMock() settings.settings.restrict_local_file_access = restricted settings.settings.config_dir = config_dir + # Explicit string so the reserved-DB derivation in _reserved_secret_paths is deterministic. + settings.settings.database_url = database_url mock_get.return_value = settings yield @@ -91,3 +93,72 @@ def test_symlink_inside_storage_pointing_inside_allowed(tmp_path): link.symlink_to(real) with mock_settings(restricted=True, config_dir=str(storage)): assert enforce_local_file_access(str(link)) == Path(str(link)) + + +@pytest.mark.parametrize("name", ["secret_key", "private_key.pem", "public_key.pem"]) +def test_reserved_secret_file_blocked(tmp_path, name): + """The server-managed secret/key files in config_dir are denied even though they sit inside it.""" + (tmp_path / name).write_text("SENSITIVE") + with mock_settings(restricted=True, config_dir=str(tmp_path)), pytest.raises(LocalFileAccessError): + enforce_local_file_access(str(tmp_path / name)) + + +def test_reserved_secret_file_via_traversal_blocked(tmp_path): + """A traversal that resolves back to a reserved secret file is denied. + + This is the actual exploit shape: a storage-path input like "/../secret_key" routes + through build_full_path (no '..' check) to //../secret_key, which resolves + back inside the boundary. + """ + (tmp_path / "secret_key").write_text("MASTER KEY") + traversal = str(tmp_path / "some-flow" / ".." / "secret_key") + with mock_settings(restricted=True, config_dir=str(tmp_path)), pytest.raises(LocalFileAccessError): + enforce_local_file_access(traversal) + + +def test_reserved_db_file_blocked(tmp_path): + """The SQLite DB under config_dir (save_db_in_config_dir) is denied.""" + db = tmp_path / "langflow.db" + db.write_text("db") + with ( + mock_settings(restricted=True, config_dir=str(tmp_path), database_url=f"sqlite:///{db}"), + pytest.raises(LocalFileAccessError), + ): + enforce_local_file_access(str(db)) + + +@pytest.mark.parametrize("suffix", ["-wal", "-shm", "-journal"]) +def test_reserved_db_sidecar_blocked(tmp_path, suffix): + """SQLite WAL/SHM/journal sidecars hold un-checkpointed DB pages and are denied too.""" + db = tmp_path / "langflow.db" + sidecar = tmp_path / f"langflow.db{suffix}" + sidecar.write_text("pages") + with ( + mock_settings(restricted=True, config_dir=str(tmp_path), database_url=f"sqlite:///{db}"), + pytest.raises(LocalFileAccessError), + ): + enforce_local_file_access(str(sidecar)) + + +def test_reserved_db_with_async_driver_and_query_blocked(tmp_path): + """An async sqlite URL with a query string still resolves to the protected DB file.""" + db = tmp_path / "langflow.db" + db.write_text("db") + url = f"sqlite+aiosqlite:///{db}?check_same_thread=false" + with ( + mock_settings(restricted=True, config_dir=str(tmp_path), database_url=url), + pytest.raises(LocalFileAccessError), + ): + enforce_local_file_access(str(db)) + + +def test_upload_named_like_secret_in_flow_subdir_allowed(tmp_path): + """A tenant upload that merely shares a reserved name but lives in a flow subdir stays readable. + + Proves the denial matches the exact config_dir location, not the basename anywhere. + """ + upload = tmp_path / "flow-id" / "secret_key" + upload.parent.mkdir(parents=True) + upload.write_text("just a user file named secret_key") + with mock_settings(restricted=True, config_dir=str(tmp_path)): + assert enforce_local_file_access(str(upload)) == Path(str(upload))