From 0a2b9f8864ae2adbe1ba0d0e6bfbd1eaf81f793e Mon Sep 17 00:00:00 2001 From: Glary-Bot Date: Thu, 11 Jun 2026 20:14:47 +0000 Subject: [PATCH] trim verbose comments and docstrings --- app/frontend_management.py | 71 +++++--------------- tests-unit/app_test/frontend_manager_test.py | 39 +---------- 2 files changed, 17 insertions(+), 93 deletions(-) diff --git a/app/frontend_management.py b/app/frontend_management.py index cdae228dc..8da20ca3e 100644 --- a/app/frontend_management.py +++ b/app/frontend_management.py @@ -207,19 +207,16 @@ def download_release_asset_zip(release: Release, destination_path: str) -> None: class FrontendManager: CUSTOM_FRONTENDS_ROOT = str(Path(__file__).parents[1] / "web_custom_versions") - # Version specifiers that resolve to a moving target on each invocation. - # Versions downloaded via these specifiers are tracked in the per-provider - # metadata file so that stale copies can be pruned when a new release - # becomes the current one. Explicitly pinned versions (e.g. ``@1.46.0`` or - # ``@v1.46.0``) are left alone so users can keep them around indefinitely - # for things like bisecting frontend regressions. + # Specifiers that resolve to a moving target. Downloads made via these are + # tracked so old copies can be pruned; explicit pins (@1.46.0, @v1.46.0) + # are left alone. AUTO_MANAGED_VERSION_SPECIFIERS = ("latest", "prerelease") - - # File written next to per-provider version folders that records which - # versions were downloaded via an auto-managed specifier. Hidden so it does - # not show up as a sibling release in casual ``ls`` output. AUTO_MANAGED_METADATA_FILENAME = ".auto_managed.json" + # Allowlist for version directory names. Prevents a tampered metadata + # file from steering cleanup at paths like "../etc". + _VERSION_DIRNAME_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") + @classmethod def _provider_dir(cls, repo_owner: str, repo_name: str) -> Path: return Path(cls.CUSTOM_FRONTENDS_ROOT) / f"{repo_owner}_{repo_name}" @@ -228,12 +225,6 @@ class FrontendManager: def _auto_managed_metadata_path(cls, repo_owner: str, repo_name: str) -> Path: return cls._provider_dir(repo_owner, repo_name) / cls.AUTO_MANAGED_METADATA_FILENAME - # A version directory name must look like a simple semver-ish token. We - # use this as a defensive allowlist when interpreting metadata so a - # malformed or tampered ``.auto_managed.json`` cannot point cleanup at - # paths outside the provider directory (e.g. ``../somewhere``). - _VERSION_DIRNAME_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") - @classmethod def _is_safe_version_dirname(cls, name: str) -> bool: if not isinstance(name, str): @@ -244,10 +235,8 @@ class FrontendManager: @classmethod def _read_auto_managed_versions(cls, repo_owner: str, repo_name: str) -> list[str]: - """Return the list of versions previously downloaded under an - auto-managed specifier for this provider. Missing, unreadable, or - otherwise malformed metadata is treated as an empty list so a bad - file never blocks startup or directs cleanup at unrelated paths.""" + """Versions tracked as auto-managed for this provider. Any error or + malformed content yields an empty list.""" metadata_path = cls._auto_managed_metadata_path(repo_owner, repo_name) if not metadata_path.exists(): return [] @@ -270,24 +259,17 @@ class FrontendManager: versions = data.get("auto_managed", []) if not isinstance(versions, list): return [] - # Filter out anything that doesn't look like a safe version dirname - # so a tampered file can't point us at, say, ``../../etc``. return [v for v in versions if cls._is_safe_version_dirname(v)] @classmethod def _write_auto_managed_versions( cls, repo_owner: str, repo_name: str, versions: list[str] ) -> None: - """Persist the auto-managed version list atomically. Deduped and - sorted for stability so the file is friendly to diffs. Any entry that - doesn't look like a safe version dirname is dropped before write so - the on-disk metadata always contains valid values.""" metadata_path = cls._auto_managed_metadata_path(repo_owner, repo_name) metadata_path.parent.mkdir(parents=True, exist_ok=True) safe_versions = [v for v in versions if cls._is_safe_version_dirname(v)] payload = {"auto_managed": sorted(set(safe_versions))} - # Atomic write via temp file + rename so a crashed process can't leave - # a half-written metadata file behind. + # Atomic write: tmp + rename so a crash can't leave a half-written file. tmp_path = metadata_path.with_suffix(metadata_path.suffix + ".tmp") try: with open(tmp_path, "w", encoding="utf-8") as fh: @@ -309,12 +291,8 @@ class FrontendManager: def _prune_auto_managed_versions( cls, repo_owner: str, repo_name: str, keep_version: str ) -> None: - """Remove all auto-managed version folders for this provider other - than ``keep_version`` and update the metadata to only list it. - - Folders that aren't currently tracked as auto-managed (i.e. explicitly - pinned downloads) are never touched. - """ + """Remove tracked auto-managed folders other than ``keep_version`` + and rewrite the metadata to list only it.""" tracked = cls._read_auto_managed_versions(repo_owner, repo_name) if not tracked and keep_version is None: return @@ -333,11 +311,8 @@ class FrontendManager: for stale_version in tracked: if stale_version == keep_version: continue - # ``_read_auto_managed_versions`` already filters tracked entries - # through ``_is_safe_version_dirname``, but re-check here so that - # this helper is also safe when called with externally-supplied - # version lists (and so a defense-in-depth audit can confirm the - # rmtree target lives under the provider directory). + # Defense in depth: read already filters, but re-check here so + # the rmtree target is provably under the provider dir. if not cls._is_safe_version_dirname(stale_version): logging.warning( "Refusing to clean up suspicious frontend version name: %r", @@ -356,8 +331,6 @@ class FrontendManager: exc, ) continue - # Ensure the resolved target lives strictly under the resolved - # provider directory (so symlinks / path tricks can't escape). if ( stale_resolved == provider_dir_resolved or provider_dir_resolved not in stale_resolved.parents @@ -388,10 +361,8 @@ class FrontendManager: def _untrack_auto_managed_version( cls, repo_owner: str, repo_name: str, version: str ) -> None: - """Drop ``version`` from the auto-managed list without deleting its - folder. Used when a user explicitly pins a version that previously - had been downloaded under ``@latest`` / ``@prerelease`` so the next - auto cleanup pass doesn't wipe it out.""" + """Drop ``version`` from auto-managed tracking without touching its + folder, so a subsequent explicit pin survives later @latest cleanups.""" tracked = cls._read_auto_managed_versions(repo_owner, repo_name) if version not in tracked: return @@ -577,9 +548,6 @@ comfyui-workflow-templates is not installed. logging.info( f"Using existing copy of specific frontend version tag: {repo_owner}/{repo_name}@{version}" ) - # User explicitly pinned this exact version: promote it out of - # the auto-managed set so future @latest cleanups won't wipe - # it out. cls._untrack_auto_managed_version( repo_owner, repo_name, version.lstrip("v") ) @@ -617,17 +585,10 @@ comfyui-workflow-templates is not installed. if download_succeeded: if is_auto_managed: - # Wipe out previously-tracked auto-managed versions and record - # the current one. This is what keeps disk usage bounded when - # users run with ``--front-end-version @latest`` over a - # long period of time (CORE-285). cls._prune_auto_managed_versions( repo_owner, repo_name, semantic_version ) else: - # An explicit version request matched a folder that had been - # downloaded under @latest previously. Promote it so it is no - # longer subject to auto-cleanup. cls._untrack_auto_managed_version( repo_owner, repo_name, semantic_version ) diff --git a/tests-unit/app_test/frontend_manager_test.py b/tests-unit/app_test/frontend_manager_test.py index 2e9a31f19..aa2e35c3f 100644 --- a/tests-unit/app_test/frontend_manager_test.py +++ b/tests-unit/app_test/frontend_manager_test.py @@ -291,14 +291,11 @@ def test_get_installed_templates_version_not_installed(): assert version is None -# --------------------------------------------------------------------------- # Auto-managed @latest / @prerelease cleanup (CORE-285) -# --------------------------------------------------------------------------- @pytest.fixture def custom_frontends_root(tmp_path, monkeypatch): - """Point ``FrontendManager.CUSTOM_FRONTENDS_ROOT`` at a fresh tmp dir.""" root = tmp_path / "web_custom_versions" root.mkdir() monkeypatch.setattr(FrontendManager, "CUSTOM_FRONTENDS_ROOT", str(root)) @@ -306,7 +303,6 @@ def custom_frontends_root(tmp_path, monkeypatch): def _make_version_dir(root, owner, repo, version): - """Create ``/_//index.html`` and return path.""" folder = root / f"{owner}_{repo}" / version folder.mkdir(parents=True, exist_ok=True) (folder / "index.html").write_text("") @@ -333,8 +329,6 @@ def test_read_auto_managed_versions_corrupt(custom_frontends_root): @pytest.mark.parametrize("payload", ["[]", "null", '"oops"', "42"]) def test_read_auto_managed_versions_non_dict_root(custom_frontends_root, payload): - """Valid JSON whose root isn't a dict must not raise — bug pointed out in - Oracle review: ``data.get(...)`` would throw on non-dict roots.""" provider_dir = custom_frontends_root / "o_r" provider_dir.mkdir() (provider_dir / FrontendManager.AUTO_MANAGED_METADATA_FILENAME).write_text(payload) @@ -342,8 +336,6 @@ def test_read_auto_managed_versions_non_dict_root(custom_frontends_root, payload def test_read_auto_managed_versions_drops_unsafe_names(custom_frontends_root): - """Tampered metadata containing path-traversal-y names must be ignored - at read time so cleanup never even sees them.""" provider_dir = custom_frontends_root / "o_r" provider_dir.mkdir() (provider_dir / FrontendManager.AUTO_MANAGED_METADATA_FILENAME).write_text( @@ -353,8 +345,6 @@ def test_read_auto_managed_versions_drops_unsafe_names(custom_frontends_root): def test_write_auto_managed_versions_drops_unsafe_names(custom_frontends_root): - """Even if a caller passes a tainted list, the file we persist must - only contain safe names.""" FrontendManager._write_auto_managed_versions( "o", "r", ["1.0.0", "../escape", "/etc/passwd", "..", "."] ) @@ -364,10 +354,6 @@ def test_write_auto_managed_versions_drops_unsafe_names(custom_frontends_root): def test_prune_refuses_to_delete_outside_provider_dir( custom_frontends_root, monkeypatch ): - """Defense in depth: even if a hostile name slips past the read/write - filters somehow, ``_prune_auto_managed_versions`` must refuse to rmtree - anything outside the provider directory.""" - # Set up a sibling directory that must NOT be touched. sibling = custom_frontends_root / "do-not-touch" sibling.mkdir() (sibling / "marker").write_text("keep me") @@ -376,8 +362,6 @@ def test_prune_refuses_to_delete_outside_provider_dir( provider_dir.mkdir() _make_version_dir(custom_frontends_root, "o", "r", "1.0.0") - # Bypass the read filter to simulate a corrupt-but-parseable list slipping - # through (e.g. older code wrote it before this hardening landed). monkeypatch.setattr( FrontendManager, "_read_auto_managed_versions", @@ -386,24 +370,19 @@ def test_prune_refuses_to_delete_outside_provider_dir( FrontendManager._prune_auto_managed_versions("o", "r", keep_version="2.0.0") - # Sibling untouched. assert sibling.exists() assert (sibling / "marker").read_text() == "keep me" - # In-bounds folder still gets cleaned. assert not (provider_dir / "1.0.0").exists() def test_prune_auto_managed_versions_removes_stale_and_keeps_pinned( custom_frontends_root, ): - # Two versions previously fetched via @latest, plus an explicitly pinned one. _make_version_dir(custom_frontends_root, "o", "r", "1.0.0") _make_version_dir(custom_frontends_root, "o", "r", "1.1.0") pinned = _make_version_dir(custom_frontends_root, "o", "r", "1.2.0") FrontendManager._write_auto_managed_versions("o", "r", ["1.0.0", "1.1.0"]) - # User runs @latest again and it resolves to 1.1.0 — older auto-managed - # 1.0.0 should be deleted, pinned 1.2.0 should remain untouched. FrontendManager._prune_auto_managed_versions("o", "r", keep_version="1.1.0") provider_dir = custom_frontends_root / "o_r" @@ -426,8 +405,6 @@ def test_untrack_auto_managed_version_does_not_delete_folder(custom_frontends_ro def test_init_frontend_latest_prunes_previous_auto_managed_versions( custom_frontends_root, mock_provider, mock_releases ): - # Pre-existing folders: 1.0.0 was previously downloaded via @latest, 1.1.5 - # was explicitly pinned by the user. Now @latest resolves to 2.0.0. _make_version_dir(custom_frontends_root, "test-owner", "test-repo", "1.0.0") pinned = _make_version_dir( custom_frontends_root, "test-owner", "test-repo", "1.1.5" @@ -436,7 +413,6 @@ def test_init_frontend_latest_prunes_previous_auto_managed_versions( "test-owner", "test-repo", ["1.0.0"] ) - # Stub out the actual download so we just create the destination dir. def fake_download(release, destination_path): Path(destination_path).mkdir(parents=True, exist_ok=True) (Path(destination_path) / "index.html").write_text("") @@ -450,55 +426,43 @@ def test_init_frontend_latest_prunes_previous_auto_managed_versions( ) provider_dir = custom_frontends_root / "test-owner_test-repo" - # 2.0.0 was downloaded and tracked. assert Path(result) == provider_dir / "2.0.0" assert (provider_dir / "2.0.0").exists() assert FrontendManager._read_auto_managed_versions( "test-owner", "test-repo" ) == ["2.0.0"] - # Old auto-managed 1.0.0 was pruned. assert not (provider_dir / "1.0.0").exists() - # Pinned 1.1.5 was left alone. assert pinned.exists() def test_init_frontend_explicit_version_promotes_out_of_auto_managed( custom_frontends_root, mock_provider ): - # 1.0.0 was previously downloaded via @latest. _make_version_dir(custom_frontends_root, "test-owner", "test-repo", "1.0.0") FrontendManager._write_auto_managed_versions( "test-owner", "test-repo", ["1.0.0"] ) - # User now explicitly pins it. The `v`-prefixed early-return path runs. result = FrontendManager.init_frontend_unsafe( "test-owner/test-repo@v1.0.0", mock_provider ) provider_dir = custom_frontends_root / "test-owner_test-repo" assert Path(result) == provider_dir / "1.0.0" - # It should no longer be tracked as auto-managed, so a future @latest run - # won't sweep it away. assert FrontendManager._read_auto_managed_versions( "test-owner", "test-repo" ) == [] - # The folder is still on disk. assert (provider_dir / "1.0.0").exists() -def test_init_frontend_explicit_version_no_v_prefix_promotes_out_of_auto_managed( +def test_init_frontend_explicit_no_v_prefix_promotes_out_of_auto_managed( custom_frontends_root, mock_provider ): - # 1.0.0 was previously downloaded via @latest, and is also already on - # disk so the GitHub-resolution path is a no-op for download. _make_version_dir(custom_frontends_root, "test-owner", "test-repo", "1.0.0") FrontendManager._write_auto_managed_versions( "test-owner", "test-repo", ["1.0.0"] ) - # No `v` prefix → goes through the GitHub release lookup path. The folder - # already exists, so download is skipped. with patch( "app.frontend_management.download_release_asset_zip" ) as mock_download_zip: @@ -507,7 +471,6 @@ def test_init_frontend_explicit_version_no_v_prefix_promotes_out_of_auto_managed ) mock_download_zip.assert_not_called() - # It should be promoted out of auto-managed even when the folder is reused. assert FrontendManager._read_auto_managed_versions( "test-owner", "test-repo" ) == []