From 291bbda8b91266ab7e86d441d90420d7855e2ad7 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 5 Jul 2026 19:20:21 +0500 Subject: [PATCH 1/3] fix(bundle): resolve file:// download_url via the file-URL helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _download_manifest built the local path from raw parsed.path, which keeps the leading slash of file:///C:/x (yielding a \C:\x path that never exists on Windows) and skips percent-decoding (my%20bundles stays encoded on every OS) — so a catalog entry whose download_url is the canonical URI Python itself produces via Path.as_uri() always fails with 'Bundle manifest not found'. Route the file scheme through the existing bundler.services.adapters._file_url_to_path helper, which already handles drive letters, UNC hosts, and percent-decoding for catalog file:// URLs (make_catalog_fetcher). The bare-path branch is unchanged. Co-Authored-By: Claude Fable 5 --- src/specify_cli/commands/bundle/__init__.py | 12 +++++- .../integration/test_bundler_local_install.py | 37 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index b3c84c6ba7..56bc7567d9 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -766,7 +766,17 @@ def _download_manifest(resolved, *, offline: bool): # On Windows an absolute path like ``C:\bundle.yml`` parses with a # single-letter ``scheme``; treat it as a local file, not a URL scheme. if scheme in ("", "file") or re.match(r"^[A-Za-z]:[\\/]", url): - local = Path(parsed.path if scheme == "file" else url) + if scheme == "file": + # Raw ``parsed.path`` keeps the leading slash of ``file:///C:/x`` + # (yielding ``\C:\x`` on Windows) and skips percent-decoding + # (``my%20bundles``). Reuse the adapters helper that already + # handles drive letters, UNC hosts, and percent-encoding for + # catalog ``file://`` URLs. + from ...bundler.services.adapters import _file_url_to_path + + local = _file_url_to_path(parsed) + else: + local = Path(url) manifest = _local_manifest_source(str(local)) if manifest is None: raise BundlerError(f"Bundle manifest not found: {local}") diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index a150ffce04..4e1759e6c8 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -112,3 +112,40 @@ def test_install_bundled_extension_from_zip_offline(tmp_path: Path): assert not ExtensionManager(project).registry.is_installed("agent-context") finally: os.chdir(previous) + + +def test_download_manifest_resolves_file_url(tmp_path: Path): + """A catalog ``file://`` download_url must resolve to the manifest. + + ``Path(parsed.path)`` kept the leading slash of ``file:///C:/x`` + (yielding a ``/C:/x``-style path on Windows, which never exists) and skipped + percent-decoding, so ``my%20bundles`` stayed encoded on every OS. + The space in the directory name below makes the failure reproduce on + POSIX CI too, not just Windows. + """ + from types import SimpleNamespace + + from specify_cli.commands.bundle import _download_manifest + + manifest_path = write_manifest(tmp_path / "my bundles") + resolved = SimpleNamespace( + entry=SimpleNamespace(id="demo-bundle", download_url=manifest_path.as_uri()) + ) + + manifest = _download_manifest(resolved, offline=True) + assert manifest.bundle.id == "demo-bundle" + + +def test_download_manifest_bare_windows_path_still_resolves(tmp_path: Path): + """The non-URL branch (bare absolute path) keeps working unchanged.""" + from types import SimpleNamespace + + from specify_cli.commands.bundle import _download_manifest + + manifest_path = write_manifest(tmp_path / "plain") + resolved = SimpleNamespace( + entry=SimpleNamespace(id="demo-bundle", download_url=str(manifest_path)) + ) + + manifest = _download_manifest(resolved, offline=True) + assert manifest.bundle.id == "demo-bundle" From 9f94be5c0ffc5d4564672b7840f298781f488adb Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 22:28:39 +0500 Subject: [PATCH 2/3] fix(bundle): reject file:// / local download_url; catalog URLs are HTTPS-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer review (route B): file:// in a catalog download_url was never intended — catalog URLs are HTTPS-only (http for localhost) across the extensions/presets/workflows catalog systems, and disk installs go through the positional path (specify bundle install ), handled by _local_manifest_source before catalog resolution. Remove the file:///bare-path branch from _download_manifest and route everything through _download_remote_manifest (HTTPS-only via _require_https), with an actionable error pointing at the positional install. Invert the file:// tests to assert rejection (+ a positional-path resolution test), and migrate the three bundle-info contract tests off local download_urls onto an HTTPS-only entry with a mocked manifest fetch. Co-Authored-By: Claude Fable 5 --- src/specify_cli/commands/bundle/__init__.py | 63 +++++++++---------- tests/contract/test_bundle_cli.py | 36 ++++++++--- .../integration/test_bundler_local_install.py | 31 +++++---- 3 files changed, 77 insertions(+), 53 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index 56bc7567d9..19a50dc114 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -746,11 +746,16 @@ def _resolve_manifest_path(path: Path | None) -> Path: def _download_manifest(resolved, *, offline: bool): """Resolve a bundle's manifest from its catalog ``download_url``. - Local/``file://`` URLs always work offline and may point at a ``.zip`` - artifact, a bundle directory, or a ``bundle.yml`` (handled by - :func:`_local_manifest_source`). Remote ``https://`` URLs are fetched with - the shared authenticated, redirect-validated HTTP client, and only when not - ``--offline``. + Catalog ``download_url``s are HTTPS-only (``http`` allowed for localhost), + matching the extensions/presets/workflows catalog systems. Remote URLs are + fetched with the shared authenticated, redirect-validated HTTP client, and + only when not ``--offline``. + + Local and ``file://`` sources are intentionally not resolved here: to + install a bundle from disk, pass the path positionally + (``specify bundle install ./path/to/bundle.yml`` — a bundle directory or a + ``.zip`` artifact also works), which :func:`_local_manifest_source` handles + before catalog resolution and which never touches ``download_url``. """ from urllib.parse import urlparse @@ -763,36 +768,28 @@ def _download_manifest(resolved, *, offline: bool): parsed = urlparse(url) scheme = parsed.scheme.lower() - # On Windows an absolute path like ``C:\bundle.yml`` parses with a - # single-letter ``scheme``; treat it as a local file, not a URL scheme. + # ``file://`` URLs and bare filesystem paths (including Windows drive paths + # like ``C:\bundle.yml``, which urlparse reads as a single-letter scheme) + # are not valid catalog download URLs. Catalog URLs are HTTPS-only across + # every catalog system; installing from disk is done by passing the path + # positionally, which never reaches URL resolution. Give an actionable + # error rather than accepting a scheme the rest of the codebase rejects. if scheme in ("", "file") or re.match(r"^[A-Za-z]:[\\/]", url): - if scheme == "file": - # Raw ``parsed.path`` keeps the leading slash of ``file:///C:/x`` - # (yielding ``\C:\x`` on Windows) and skips percent-decoding - # (``my%20bundles``). Reuse the adapters helper that already - # handles drive letters, UNC hosts, and percent-encoding for - # catalog ``file://`` URLs. - from ...bundler.services.adapters import _file_url_to_path - - local = _file_url_to_path(parsed) - else: - local = Path(url) - manifest = _local_manifest_source(str(local)) - if manifest is None: - raise BundlerError(f"Bundle manifest not found: {local}") - return manifest - - if scheme in ("http", "https"): - if offline: - raise BundlerError( - f"Network access disabled; cannot download bundle '{resolved.entry.id}' " - f"from {url}." - ) - return _download_remote_manifest(resolved.entry.id, url) + raise BundlerError( + f"Catalog entry '{resolved.entry.id}' has a local/file:// download_url " + f"({url}); catalog download URLs must be HTTPS (http for localhost). " + "To install a bundle from disk, pass the path directly: " + "'specify bundle install '." + ) - raise BundlerError( - f"Unsupported download_url scheme for bundle '{resolved.entry.id}': {url}" - ) + if offline: + raise BundlerError( + f"Network access disabled; cannot download bundle '{resolved.entry.id}' " + f"from {url}." + ) + # HTTPS-only (http for localhost) is enforced by _require_https inside + # _download_remote_manifest, matching the other catalog systems. + return _download_remote_manifest(resolved.entry.id, url) def _require_https(label: str, url: str) -> None: diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 1705c5945d..df9c8dae10 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -175,7 +175,23 @@ def test_build_produces_artifact(project: Path): assert len(artifacts) == 1 -def test_info_expands_full_component_set(project: Path): +def _mock_manifest_download(monkeypatch, source_path: Path) -> None: + """Mock the HTTPS manifest fetch to return a locally-authored manifest. + + Catalog ``download_url``s are HTTPS-only, so ``info`` tests can no longer + point one at a local file. Patch ``_download_manifest`` to return the + manifest parsed from *source_path* (a bundle.yml or a .zip artifact), + exercising ``info``'s expansion without a network call. + """ + from specify_cli.commands.bundle import _local_manifest_source + + monkeypatch.setattr( + "specify_cli.commands.bundle._download_manifest", + lambda resolved, *, offline: _local_manifest_source(str(source_path)), + ) + + +def test_info_expands_full_component_set(project: Path, monkeypatch): bundle_dir = project / "src-bundle" bundle_dir.mkdir() (bundle_dir / "bundle.yml").write_text( @@ -183,13 +199,14 @@ def test_info_expands_full_component_set(project: Path): ) catalog = project / "local-catalog.json" entry = catalog_entry_dict( - "demo-bundle", download_url=str(bundle_dir / "bundle.yml") + "demo-bundle", download_url="https://example.com/demo-bundle.zip" ) write_catalog_file(catalog, {"demo-bundle": entry}) added = runner.invoke( app, ["bundle", "catalog", "add", str(catalog), "--id", "local"] ) assert added.exit_code == 0, added.output + _mock_manifest_download(monkeypatch, bundle_dir / "bundle.yml") result = runner.invoke(app, ["bundle", "info", "demo-bundle", "--json", "--offline"]) assert result.exit_code == 0, result.output @@ -207,7 +224,7 @@ def test_info_expands_full_component_set(project: Path): assert "Trust" in text.output -def test_info_expands_discovery_only_bundle(project: Path): +def test_info_expands_discovery_only_bundle(project: Path, monkeypatch): # Discovery-only bundles must still be fully inspectable via `info`; # only `install` is refused for them. bundle_dir = project / "disc-bundle" @@ -217,7 +234,7 @@ def test_info_expands_discovery_only_bundle(project: Path): ) catalog = project / "disc-catalog.json" entry = catalog_entry_dict( - "demo-bundle", download_url=str(bundle_dir / "bundle.yml") + "demo-bundle", download_url="https://example.com/demo-bundle.zip" ) write_catalog_file(catalog, {"demo-bundle": entry}) config = { @@ -230,6 +247,7 @@ def test_info_expands_discovery_only_bundle(project: Path): (project / ".specify" / "bundle-catalogs.yml").write_text( yaml.safe_dump(config), encoding="utf-8" ) + _mock_manifest_download(monkeypatch, bundle_dir / "bundle.yml") result = runner.invoke(app, ["bundle", "info", "demo-bundle", "--json", "--offline"]) assert result.exit_code == 0, result.output payload = json.loads(result.output) @@ -237,8 +255,9 @@ def test_info_expands_discovery_only_bundle(project: Path): assert ("extensions", "ext-a") in components -def test_info_resolves_local_zip_download_url(project: Path): - # A local .zip artifact as download_url is extracted to read bundle.yml. +def test_info_expands_zip_sourced_bundle(project: Path, monkeypatch): + # A .zip artifact is extracted to read bundle.yml; info expands it. (The + # download itself is HTTPS-only now and mocked here — see contract note.) bundle_dir = project / "zip-src" bundle_dir.mkdir() (bundle_dir / "bundle.yml").write_text( @@ -249,12 +268,15 @@ def test_info_resolves_local_zip_download_url(project: Path): catalog = project / "zip-catalog.json" write_catalog_file( catalog, - {"demo-bundle": catalog_entry_dict("demo-bundle", download_url=str(artifact))}, + {"demo-bundle": catalog_entry_dict( + "demo-bundle", download_url="https://example.com/demo-bundle.zip" + )}, ) added = runner.invoke( app, ["bundle", "catalog", "add", str(catalog), "--id", "local"] ) assert added.exit_code == 0, added.output + _mock_manifest_download(monkeypatch, artifact) result = runner.invoke(app, ["bundle", "info", "demo-bundle", "--json", "--offline"]) assert result.exit_code == 0, result.output payload = json.loads(result.output) diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index 4e1759e6c8..a5c83b20f8 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -114,14 +114,10 @@ def test_install_bundled_extension_from_zip_offline(tmp_path: Path): os.chdir(previous) -def test_download_manifest_resolves_file_url(tmp_path: Path): - """A catalog ``file://`` download_url must resolve to the manifest. - - ``Path(parsed.path)`` kept the leading slash of ``file:///C:/x`` - (yielding a ``/C:/x``-style path on Windows, which never exists) and skipped - percent-decoding, so ``my%20bundles`` stayed encoded on every OS. - The space in the directory name below makes the failure reproduce on - POSIX CI too, not just Windows. +def test_download_manifest_rejects_file_url(tmp_path: Path): + """A catalog ``file://`` download_url is rejected — catalog URLs are + HTTPS-only, matching extensions/presets/workflows. Disk installs go through + the positional path (see the local-source tests above), not download_url. """ from types import SimpleNamespace @@ -132,12 +128,12 @@ def test_download_manifest_resolves_file_url(tmp_path: Path): entry=SimpleNamespace(id="demo-bundle", download_url=manifest_path.as_uri()) ) - manifest = _download_manifest(resolved, offline=True) - assert manifest.bundle.id == "demo-bundle" + with pytest.raises(BundlerError, match="bundle install"): + _download_manifest(resolved, offline=True) -def test_download_manifest_bare_windows_path_still_resolves(tmp_path: Path): - """The non-URL branch (bare absolute path) keeps working unchanged.""" +def test_download_manifest_rejects_bare_path(tmp_path: Path): + """A bare filesystem path download_url is likewise rejected.""" from types import SimpleNamespace from specify_cli.commands.bundle import _download_manifest @@ -147,5 +143,14 @@ def test_download_manifest_bare_windows_path_still_resolves(tmp_path: Path): entry=SimpleNamespace(id="demo-bundle", download_url=str(manifest_path)) ) - manifest = _download_manifest(resolved, offline=True) + with pytest.raises(BundlerError, match="bundle install"): + _download_manifest(resolved, offline=True) + + +def test_local_install_still_resolves_via_positional_path(tmp_path: Path): + """The supported local route — a positional path, not a download_url — + still resolves the manifest via _local_manifest_source.""" + manifest_path = write_manifest(tmp_path / "my bundles") + manifest = _local_manifest_source(str(manifest_path)) + assert manifest is not None assert manifest.bundle.id == "demo-bundle" From 4a0cacb20f98e1f1be61105aa9376ecfa5a77d1b Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 9 Jul 2026 19:29:26 +0500 Subject: [PATCH 3/3] fix(bundle): validate HTTPS before the offline gate in _download_manifest Per review: for a non-local download_url the offline check ran before any URL validation, so an invalid/non-HTTPS scheme surfaced a misleading 'Network access disabled' error under --offline when the real problem is the URL would be rejected even online. Call _require_https before the offline gate so the correct error is reported in every mode. Co-Authored-By: Claude Fable 5 --- src/specify_cli/commands/bundle/__init__.py | 9 +++++++-- tests/integration/test_bundler_local_install.py | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index 19a50dc114..d73509562f 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -782,13 +782,18 @@ def _download_manifest(resolved, *, offline: bool): "'specify bundle install '." ) + # Validate the scheme/host *before* the offline gate so an invalid or + # non-HTTPS download_url reports the real problem in every mode, rather + # than a misleading "Network access disabled" under --offline. + # (_download_remote_manifest re-checks this, but only once network access + # is permitted.) HTTPS-only, http allowed for localhost. + _require_https(f"bundle '{resolved.entry.id}'", url) + if offline: raise BundlerError( f"Network access disabled; cannot download bundle '{resolved.entry.id}' " f"from {url}." ) - # HTTPS-only (http for localhost) is enforced by _require_https inside - # _download_remote_manifest, matching the other catalog systems. return _download_remote_manifest(resolved.entry.id, url) diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index a5c83b20f8..32972ac684 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -154,3 +154,20 @@ def test_local_install_still_resolves_via_positional_path(tmp_path: Path): manifest = _local_manifest_source(str(manifest_path)) assert manifest is not None assert manifest.bundle.id == "demo-bundle" + + +def test_download_manifest_rejects_non_https_url_even_offline(tmp_path: Path): + """A non-HTTPS download_url must report the HTTPS problem, not a misleading + 'Network access disabled', even under --offline (scheme is validated before + the offline gate).""" + from types import SimpleNamespace + + from specify_cli.commands.bundle import _download_manifest + + resolved = SimpleNamespace( + entry=SimpleNamespace( + id="demo-bundle", download_url="http://example.com/bundle.zip" + ) + ) + with pytest.raises(BundlerError, match="HTTPS"): + _download_manifest(resolved, offline=True)