fix(bundle): reject file:// / local download_url — catalog URLs are HTTPS-only#3344
fix(bundle): reject file:// / local download_url — catalog URLs are HTTPS-only#3344jawwad-ali wants to merge 3 commits into
Conversation
_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>
|
Thanks for the careful writeup — the diagnosis is genuinely good. You correctly spotted that 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 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. 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 .zipThat's handled by 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:
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. |
|
Thanks @mnriem — that's a much better framing, and I'm happy to pivot. HTTPS-only catalog One wrinkle I hit while prototyping the removal, so we can decide scope deliberately: dropping the whole So there are two ways to land this, and I'd like your call:
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 (AI-assisted: analysis + prototyping with Claude Code under my direction; I verified the 3-test breakage locally before writing this.) |
|
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>
|
Done — pushed route B in Implementation:
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:
Bundle's (AI-assisted: implemented + audited with Claude Code under my direction; verified the full contract/local-install suites locally.) |
There was a problem hiding this comment.
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_manifestto rejectfile://and bare filesystem-pathdownload_urls and provide an actionable “use positional path” error message. - Updated bundle CLI contract tests to use HTTPS-shaped
download_urls and mock_download_manifestto keepbundle info --offlinetests network-free. - Added integration tests asserting
download_urlrejects 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
mnriem
left a comment
There was a problem hiding this comment.
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>
Description
A catalog entry's
download_urlacceptedfile://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): catalogdownload_urls are HTTPS-only (http for localhost), matching the extensions/presets/workflows catalog systems. Installing a bundle from disk goes through the positional path —specify bundle install ./path/to/bundle.yml(a bundle dir or.zipalso works) — handled by_local_manifest_sourcebefore catalog resolution, which never touchesdownload_url.Originally reported as a Windows-drive / percent-encoding bug in
_download_manifest(it built the local path from rawparsed.path); that diagnosis was correct, but rather than repair thefile://branch we drop it, sincefile://in a catalogdownload_urlwas never intended.Changes
_download_manifest: rejectfile://and bare-pathdownload_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").Testing
_download_manifestnow rejects afile://and a bare-pathdownload_url(was: resolved) — seetest_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_pathproves the supported disk route still works.bundle infocontract tests were migrated off localdownload_urls onto an HTTPS entry with a mocked manifest fetch. Fulltest_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_manifestwas the lone gap, now closed — so all four are consistent.AI Disclosure