Skip to content

fix(bundle): reject file:// / local download_url — catalog URLs are HTTPS-only#3344

Open
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/bundle-download-manifest-file-url
Open

fix(bundle): reject file:// / local download_url — catalog URLs are HTTPS-only#3344
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/bundle-download-manifest-file-url

Conversation

@jawwad-ali

@jawwad-ali jawwad-ali commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Description

A catalog entry's download_url accepted file:// and bare filesystem paths, diverging from the rest of the codebase. Per maintainer review, the correct fix is to remove that path (not repair it): catalog download_urls are HTTPS-only (http for localhost), matching the extensions/presets/workflows catalog systems. Installing a bundle from disk goes through the positional pathspecify bundle install ./path/to/bundle.yml (a bundle dir or .zip also works) — handled by _local_manifest_source before catalog resolution, which never touches download_url.

Originally reported as a Windows-drive / percent-encoding bug in _download_manifest (it built the local path from raw parsed.path); that diagnosis was correct, but rather than repair the file:// branch we drop it, since file:// in a catalog download_url was never intended.

Changes

  • _download_manifest: reject file:// and bare-path download_urls with an actionable error pointing at the positional install; route everything else through _download_remote_manifest (HTTPS-only via _require_https). HTTPS/host is validated before the offline gate so an invalid scheme reports the real problem in every mode (not a misleading "Network access disabled").
  • Docstring updated to state the HTTPS-only contract.

Testing

  • _download_manifest now rejects a file:// and a bare-path download_url (was: resolved) — see test_download_manifest_rejects_file_url / test_download_manifest_rejects_bare_path; a non-HTTPS URL is rejected with the HTTPS error even offline (test_download_manifest_rejects_non_https_url_even_offline).
  • test_local_install_still_resolves_via_positional_path proves the supported disk route still works.
  • The three bundle info contract tests were migrated off local download_urls onto an HTTPS entry with a mocked manifest fetch. Full test_bundle_cli.py + test_bundler_local_install.py: 40 passed, ruff clean.

Catalog-system parity

Audited all four systems: extensions, presets, and workflows already enforce HTTPS on both catalog and per-entry download URLs; bundle's _download_manifest was the lone gap, now closed — so all four are consistent.

AI Disclosure

  • I used AI assistance (Claude Code, under my direction). AI implemented the pivot + migrated the affected tests; I verified the full contract/local-install suites locally and reviewed the diff.

_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 <noreply@anthropic.com>
@jawwad-ali jawwad-ali requested a review from mnriem as a code owner July 5, 2026 14:21
@mnriem

mnriem commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the careful writeup — the diagnosis is genuinely good. You correctly spotted that _download_manifest diverged from adapters._file_url_to_path, and the Windows-drive / percent-encoding failures you documented are real.

Digging into this against the broader catalog architecture, though, we've realized the fix should go the other direction — and that's on us, not you. The real problem is that file:// in a catalog entry's download_url was never meant to exist. It slipped through review when the bundler's offline-first catalog work landed.

Context: Spec Kit has four catalog systems — bundles, extensions, presets, workflows — and the other three deliberately enforce HTTPS-only for catalog and download URLs (HTTP only for localhost). Local/dev sources go through a separate, explicit route: a real filesystem path (e.g. specify extension add <dir> --dev), never a file:// URL through URL resolution.

The bundle command already follows that convention — local installs are the positional path argument:

specify bundle install ./path/to/bundle.yml     # or a bundle dir, or a .zip

That's handled by _local_manifest_source() before catalog resolution and never touches download_url. So the file:// download_url branch isn't filling a real gap — it's a divergence from the rest of the codebase.

Rather than close this, would you be up for pivoting the PR to remove that branch instead of repairing it? You've already built the ideal harness for it. Concretely:

  • In _download_manifest, drop the scheme in ("", "file") / Windows-drive branch and route everything through _download_remote_manifest, so catalog download_url is HTTPS-only (http for localhost) — matching extensions/presets/workflows. The _require_https helper right below already does exactly this.
  • Give file:///bare-path download URLs a clear, actionable error (point users at the positional local-path install).
  • Update the docstring/help that currently advertises file:// download URLs as first-class.
  • Invert the tests: your test_download_manifest_resolves_file_url becomes an assertion that a file:// download_url is rejected; keep a test proving local installs still work via the positional path.

If you'd rather not take it that direction, no problem at all — we can close this and track the removal separately, still crediting your report. But since you did the hard part (finding the divergence), we'd love to have you land the fix. 🙏

Happy to answer questions on any of the above.

@jawwad-ali

Copy link
Copy Markdown
Contributor Author

Thanks @mnriem — that's a much better framing, and I'm happy to pivot. HTTPS-only catalog download_urls (http for localhost) matching extensions/presets/workflows, with disk installs going through the positional path, is clearly the right convention.

One wrinkle I hit while prototyping the removal, so we can decide scope deliberately: dropping the whole scheme in ("", "file") / Windows-drive branch (not just file://) breaks three existing contract tests — test_info_expands_full_component_set, test_info_expands_discovery_only_bundle, and test_info_resolves_local_zip_download_url in tests/contract/test_bundle_cli.py. They drive bundle info --offline by pointing a local catalog's download_url at a bare local path (str(bundle_dir / "bundle.yml") / a local .zip). Unlike install, info has no positional-path variant, so those can't just move to the positional route — offline info on a local bundle currently depends on that branch.

So there are two ways to land this, and I'd like your call:

  • A — reject file:// only (the actual slip): keep the bare local-path branch that offline bundle info relies on, but reject file:// (and, if you want, Windows-drive) download_urls with an actionable error pointing at specify bundle install <path>. Minimal, all tests stay green, kills the divergent URL scheme.
  • B — full HTTPS-only (your literal suggestion): route everything through _download_remote_manifest, reject both file:// and bare paths, and I migrate those three info tests onto an HTTPS-mocked fixture. Cleaner alignment, but it does remove the ability to bundle info a purely-local bundle offline — worth a conscious sign-off since it's a small UX reduction for info.

I've got the harness ready for either and can turn it around quickly. My lean is A (fixes the scheme divergence + the Windows/percent-encoding bug you confirmed, with zero collateral), but if you'd rather have the strict HTTPS-only contract everywhere I'm glad to do B and handle the info test migration. Which do you prefer?

(AI-assisted: analysis + prototyping with Claude Code under my direction; I verified the 3-test breakage locally before writing this.)

@mnriem

mnriem commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

I would prefer route B and then we probably should validate all catalog systems work similarly

…TPS-only

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 <path>), 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 <noreply@anthropic.com>
@jawwad-ali

Copy link
Copy Markdown
Contributor Author

Done — pushed route B in 9f94be5.

Implementation:

  • _download_manifest now rejects file:// and bare-path download_urls with an actionable error pointing at the positional install, and routes everything else through _download_remote_manifest (HTTPS-only via _require_https). Docstring updated.
  • Inverted the tests: test_download_manifest_rejects_file_url / test_download_manifest_rejects_bare_path assert rejection, plus test_local_install_still_resolves_via_positional_path proves the supported disk route still works via _local_manifest_source.
  • Migrated the three bundle info contract tests (test_info_expands_full_component_set, test_info_expands_discovery_only_bundle, test_info_expands_zip_sourced_bundle) off local download_urls onto an HTTPS-only entry with a mocked manifest fetch — component-expansion coverage is preserved. Full test_bundle_cli.py + test_bundler_local_install.py: 39 passed, ruff clean.

On "validate all catalog systems work similarly" — I audited the other three, and the good news is they already enforce HTTPS on both the catalog URL and the per-entry download URL:

  • extensions — Extension download URL must use HTTPS (extensions/init.py ~2601)
  • presets — Preset download URL must use HTTPS (presets/init.py ~2497)
  • workflows — scheme checks on catalog + download URLs (workflows/catalog.py ~337/954)
  • shared catalogs.py / bundler adapters._validate_remote_url — catalog URLs HTTPS-only

Bundle's _download_manifest was the lone place that accepted a non-HTTPS download_url; this PR closes that gap, so all four are now consistent. Happy to add a small cross-system parity test (or a follow-up PR) if you'd like that invariant pinned explicitly — just say the word.

(AI-assisted: implemented + audited with Claude Code under my direction; verified the full contract/local-install suites locally.)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes how bundle catalog entries’ download_url values are handled during manifest resolution, shifting the bundle system toward HTTPS-only catalog downloads and updating tests to reflect that contract.

Changes:

  • Updated _download_manifest to reject file:// and bare filesystem-path download_urls and provide an actionable “use positional path” error message.
  • Updated bundle CLI contract tests to use HTTPS-shaped download_urls and mock _download_manifest to keep bundle info --offline tests network-free.
  • Added integration tests asserting download_url rejects local/file URLs while positional-path local install resolution still works.
Show a summary per file
File Description
src/specify_cli/commands/bundle/init.py Enforces HTTPS-only download_url handling by rejecting local/file URLs and routing valid remote URLs to the remote downloader.
tests/contract/test_bundle_cli.py Updates bundle info contract tests to avoid local download_urls and mocks manifest download for offline expansion coverage.
tests/integration/test_bundler_local_install.py Adds integration coverage for rejecting local/file download_urls and confirms positional-path local manifests still resolve.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Low

Comment thread src/specify_cli/commands/bundle/__init__.py
Comment thread src/specify_cli/commands/bundle/__init__.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

…fest

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 <noreply@anthropic.com>
@jawwad-ali jawwad-ali changed the title fix(bundle): resolve file:// download_url via the file-URL helper fix(bundle): reject file:// / local download_url — catalog URLs are HTTPS-only Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants