From e80c5414424bccedbcf53a4cd53bd6efa63cd05d Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Fri, 31 Jul 2026 17:51:23 +0500 Subject: [PATCH 1/3] fix: eliminate TOCTOU races in catalog_fetch() for file:// and bare path URLs Remove exists() pre-checks and catch FileNotFoundError from read_text() to provide clear BundlerError messages even under race conditions. --- src/specify_cli/bundler/services/adapters.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index ca39a2489b..52ce8e5433 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -143,15 +143,17 @@ def fetch(source: CatalogSource) -> dict: if scheme == "file": path = _file_url_to_path(parsed) - if not path.exists(): - raise BundlerError(f"Catalog file not found: {path}") - return load_json(path) + try: + return loads_json(path.read_text(encoding="utf-8"), origin=str(path)) + except FileNotFoundError: + raise BundlerError(f"Catalog file not found: {path}") from None if scheme == "" or _is_windows_drive_path(url): path = Path(url) - if not path.exists(): - raise BundlerError(f"Catalog file not found: {path}") - return load_json(path) + try: + return loads_json(path.read_text(encoding="utf-8"), origin=str(path)) + except FileNotFoundError: + raise BundlerError(f"Catalog file not found: {path}") from None if scheme in ("http", "https"): if not allow_network: From 09a8ed5a698db2bb5930fb83080405d60ada027e Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 11 Aug 2026 02:24:16 +0500 Subject: [PATCH 2/3] fix: eliminate TOCTOU races in catalog_fetch() for file:// and bare path URLs Remove exists() pre-checks and catch FileNotFoundError from read_text() for both file:// and bare path catalog sources. Also catches OSError/UnicodeError to preserve the decode-error wrapping contract. Add regression test for the TOCTOU fix covering both file:// URLs and bare paths: mocked Path is observable as present (exists() returns True) but read_text() raises FileNotFoundError, proving the exists() removal eliminates the race window. Co-authored-by: GitHub Copilot (model: mimo-v2.5-free, supervised) --- src/specify_cli/bundler/services/adapters.py | 4 ++++ tests/unit/test_bundler_adapters.py | 25 ++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index 52ce8e5433..4f8e84f186 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -147,6 +147,8 @@ def fetch(source: CatalogSource) -> dict: return loads_json(path.read_text(encoding="utf-8"), origin=str(path)) except FileNotFoundError: raise BundlerError(f"Catalog file not found: {path}") from None + except (OSError, UnicodeError) as exc: + raise BundlerError(f"Could not read {path}: {exc}") from exc if scheme == "" or _is_windows_drive_path(url): path = Path(url) @@ -154,6 +156,8 @@ def fetch(source: CatalogSource) -> dict: return loads_json(path.read_text(encoding="utf-8"), origin=str(path)) except FileNotFoundError: raise BundlerError(f"Catalog file not found: {path}") from None + except (OSError, UnicodeError) as exc: + raise BundlerError(f"Could not read {path}: {exc}") from exc if scheme in ("http", "https"): if not allow_network: diff --git a/tests/unit/test_bundler_adapters.py b/tests/unit/test_bundler_adapters.py index 854e60df3f..8727330068 100644 --- a/tests/unit/test_bundler_adapters.py +++ b/tests/unit/test_bundler_adapters.py @@ -1,6 +1,9 @@ """Unit tests for catalog-fetch adapters (auth + redirect safety).""" from __future__ import annotations +from pathlib import Path +from unittest.mock import MagicMock, patch + import pytest from specify_cli.bundler import BundlerError @@ -201,3 +204,25 @@ def test_validate_remote_url_rejects_malformed_url_cleanly(url): caller. Bundler sibling of #3369.""" with pytest.raises(BundlerError): adapters._validate_remote_url("team", url) + + +@pytest.mark.parametrize("use_file_url", [False, True], ids=["path", "file-url"]) +def test_local_catalog_toctou_race(tmp_path, use_file_url): + """Regression guard: a file that disappears between the old exists() pre-check + and read_text() must raise BundlerError, not a raw FileNotFoundError. + + The mocked Path is observable as present (exists() returns True) but + read_text() raises FileNotFoundError, simulating a deletion between the two + calls — the exact race window the exists() removal eliminates.""" + catalog_path = tmp_path / "catalog.json" + url = catalog_path.as_uri() if use_file_url else str(catalog_path) + + mock_path = MagicMock(spec=Path) + mock_path.exists.return_value = True + mock_path.read_text.side_effect = FileNotFoundError(str(catalog_path)) + + fetcher = adapters.make_catalog_fetcher(allow_network=False) + + with patch.object(adapters.Path, "__new__", return_value=mock_path): + with pytest.raises(BundlerError, match="Catalog file not found"): + fetcher(_source(url)) From 14e453a191114db0eab6c51e3e33dbd9d75b6172 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Tue, 18 Aug 2026 01:45:11 +0500 Subject: [PATCH 3/3] fix: remove unused load_json import and improve TOCTOU test docstring Remove load_json from imports (F401) since the TOCTOU fix switched to loads_json(path.read_text()). Improve test docstring to accurately describe the behavioral change. --- src/specify_cli/bundler/services/adapters.py | 2 +- tests/unit/test_bundler_adapters.py | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index 4f8e84f186..4bd741b4a9 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -18,7 +18,7 @@ from ..._assets import _locate_core_pack, _repo_root from ..._download_security import MAX_JSON_CATALOG_BYTES, read_response_limited from .. import BundlerError -from ..lib.yamlio import load_json, loads_json +from ..lib.yamlio import loads_json from ..models.catalog import CatalogSource from ..models.manifest import ComponentRef diff --git a/tests/unit/test_bundler_adapters.py b/tests/unit/test_bundler_adapters.py index 8727330068..32f411af90 100644 --- a/tests/unit/test_bundler_adapters.py +++ b/tests/unit/test_bundler_adapters.py @@ -208,12 +208,11 @@ def test_validate_remote_url_rejects_malformed_url_cleanly(url): @pytest.mark.parametrize("use_file_url", [False, True], ids=["path", "file-url"]) def test_local_catalog_toctou_race(tmp_path, use_file_url): - """Regression guard: a file that disappears between the old exists() pre-check - and read_text() must raise BundlerError, not a raw FileNotFoundError. + """A missing file at read time must retain the fetcher's BundlerError contract. - The mocked Path is observable as present (exists() returns True) but - read_text() raises FileNotFoundError, simulating a deletion between the two - calls — the exact race window the exists() removal eliminates.""" + The mocked Path raises FileNotFoundError from read_text(), simulating a + deletion immediately before the catalog is opened while verifying that no + existence pre-check is needed.""" catalog_path = tmp_path / "catalog.json" url = catalog_path.as_uri() if use_file_url else str(catalog_path)