Skip to content

fix: resolve GitHub release asset API URL for private repo bundle downloads#3136

Open
lselvar wants to merge 8 commits into
github:mainfrom
lselvar:fix/private-github-release-bundle-downloads
Open

fix: resolve GitHub release asset API URL for private repo bundle downloads#3136
lselvar wants to merge 8 commits into
github:mainfrom
lselvar:fix/private-github-release-bundle-downloads

Conversation

@lselvar

@lselvar lselvar commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix — one download path is now covered

`_download_remote_manifest` (catalog-based bundle manifest download):

  • Resolves browser release URLs to REST API asset URLs before downloading the bundle manifest (YAML or ZIP)
  • Passes `github_hosts=github_provider_hosts()` so GHES browser release URLs resolve via `/api/v3` (parity with extensions/presets)
  • Direct REST API asset URLs (`api.github.com/repos/.../releases/assets/`) are passed through directly with `Accept: application/octet-stream`
  • ZIP payloads are detected via 4-byte magic signatures (`PK\x03\x04`, `PK\x05\x06`, `PK\x07\x08`) so direct REST API asset URLs pointing to ZIP bundles (which carry no file extension) are routed through the ZIP extraction path
  • URL extension detection uses `PurePosixPath(urlparse(url).path).suffix` to strip query strings/fragments before checking the `.zip` suffix
  • `require_https` is called on the resolved URL before issuing the download request (defence in depth)
  • `zipfile.BadZipFile` and `yaml.YAMLError` from post-download parsing are wrapped in `BundlerError` for clean user-facing error messages
  • Error messages include the original catalog URL and, when it differs, the resolved API URL so users know which entry to fix
  • Other URLs: existing behavior unchanged

Implementation

Uses the existing shared `resolve_github_release_asset_api_url(download_url, open_url_fn, timeout, github_hosts)` function from `_github_http.py` — the same utility used by the extension, preset, and workflow fixes. No new helpers needed.

Test plan

  • Run the bundler contract test suite:
    UV_NATIVE_TLS=true SSL_CERT_FILE=/opt/homebrew/etc/openssl@3/cert.pem UV_DEFAULT_INDEX=https://pypi.org/simple PYTHONPATH=src uv run --extra test pytest tests/contract/test_bundle_cli.py -v
    Expected: 30 passed (includes 6 new tests)
  • Run full bundler test suite:
    UV_NATIVE_TLS=true SSL_CERT_FILE=/opt/homebrew/etc/openssl@3/cert.pem UV_DEFAULT_INDEX=https://pypi.org/simple PYTHONPATH=src uv run --extra test pytest tests/contract/ tests/unit/ tests/integration/ tests/test_github_http.py
    Expected: all passing
  • Verify `specify bundle install ` works when the catalog's `download_url` points at a GitHub release asset on a private repo
  • Verify `specify bundle info ` works for the same case

AI disclosure: This PR was developed with Claude Code assistance.

🤖 Generated with Claude Code

…nloads

For private/SSO-protected GitHub repos, browser release download URLs
(https://github.com/<owner>/<repo>/releases/download/<tag>/<asset>)
redirect to an HTML/SSO page instead of delivering the asset, causing
bundle manifest downloads to fail.

Extends the pattern from github#2855 (presets/workflows) to cover the bundle
manifest download path in _download_remote_manifest:

- Resolves browser release URLs to GitHub REST API asset URLs via
  resolve_github_release_asset_api_url before downloading
- Direct REST API asset URLs (api.github.com/repos/.../releases/assets/<id>)
  are passed through directly
- Both cases use Accept: application/octet-stream so the API returns the
  binary payload rather than JSON metadata
- The original catalog URL is used to determine artifact format (.zip vs
  YAML) since the resolved API URL does not carry the file extension

Adds two CLI-level contract tests:
- bundle info resolves browser release URL via GitHub tags API
- bundle info passes direct API asset URL through with octet-stream

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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 extends the existing “private GitHub release asset download URL” fix to the bundle manifest download path, ensuring bundle info / bundle install can successfully download manifests from private/SSO-protected GitHub release assets by resolving browser release URLs to GitHub REST asset URLs and downloading them with Accept: application/octet-stream.

Changes:

  • Apply resolve_github_release_asset_api_url() in _download_remote_manifest() to translate GitHub browser release download URLs into REST asset URLs prior to downloading.
  • Send Accept: application/octet-stream when downloading via GitHub REST asset URLs to retrieve the binary payload instead of JSON metadata.
  • Add contract tests validating browser-URL resolution and direct REST asset URL passthrough for bundle info.
Show a summary per file
File Description
src/specify_cli/commands/bundle/__init__.py Resolve GitHub release asset URLs before manifest download; download assets via REST with octet-stream header.
tests/contract/test_bundle_cli.py Add contract coverage for browser release URL resolution and direct REST asset URL behavior for bundle manifest downloads.

Copilot's findings

Tip

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

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment thread tests/contract/test_bundle_cli.py
Comment thread src/specify_cli/commands/bundle/__init__.py Outdated

@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

Address Copilot review feedback on PR github#3136:

1. Detect ZIP payloads by magic bytes (PK\x03\x04) in addition to the
   '.zip' URL suffix so that direct GitHub REST asset URLs — which carry
   no file extension — are correctly routed through the ZIP extraction
   path when the asset is a ZIP bundle artifact.

2. Add two new contract tests:
   - test_bundle_info_resolves_github_browser_release_url_zip: exercises
     the '.zip' browser release URL path end-to-end, verifying the tags
     API lookup fires, octet-stream header is used, and bundle.yml is
     successfully extracted from the ZIP payload.
   - test_bundle_info_api_asset_url_zip_detected_by_magic_bytes: verifies
     that a direct REST asset URL returning ZIP bytes is detected by magic
     and parsed correctly without a tags API call.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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.

Review details

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

Comment thread src/specify_cli/commands/bundle/__init__.py Outdated
Comment thread src/specify_cli/commands/bundle/__init__.py Outdated
Comment thread tests/contract/test_bundle_cli.py Outdated
@mnriem

mnriem commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback ;)

Address second-round Copilot review feedback on PR github#3136:

- Error message: when the download fails, report the original catalog
  download_url so the user knows which entry to fix; include the resolved
  REST API URL when it differs for easier debugging.
- ZIP detection: broaden the magic-bytes check from PK\x03\x04 to raw[:2]
  == b"PK", covering all valid ZIP variants (local-file header PK\x03\x04,
  empty-archive PK\x05\x06, spanned/split PK\x07\x08).
- Tests: remove the unused tmp_path parameter from
  test_bundle_info_resolves_github_browser_release_url_zip.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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.

Review details

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

Comment thread src/specify_cli/commands/bundle/__init__.py Outdated
@mnriem

mnriem commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback. Almost there!

Address Copilot feedback: raw[:2] == b"PK" is too broad and could
misclassify any payload starting with ASCII "PK" as a ZIP, producing
a confusing "not a valid bundle" error.

Use the three specific 4-byte ZIP magic signatures instead:
  PK\x03\x04 — local file header (standard ZIP)
  PK\x05\x06 — end-of-central-directory (empty archive)
  PK\x07\x08 — data descriptor / spanning marker

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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.

Review details

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

Comment thread tests/contract/test_bundle_cli.py
@mnriem

mnriem commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

lselvar and others added 4 commits June 30, 2026 21:16
- Promote _ZIP_SIGNATURES to module-level constant (was redefined per call)
- Use PurePosixPath for URL path suffix extraction so query strings and
  fragments are ignored and URL paths are treated as POSIX on all OSes
- Move yaml/BundleManifest imports to function top to flatten the
  previously nested try/except into a single handler with explicit
  except _yaml.YAMLError and except Exception clauses
- Re-add None guard on _local_manifest_source return: the function is
  typed Optional[BundleManifest] and without the guard a None return
  propagates silently to callers that degrade gracefully rather than
  raising an actionable error; comment explains it is defensive not dead
- Assert exact resolved asset URL in browser-URL download tests, not
  just the Accept header, so a regression where download uses the
  original URL instead of the resolved one would be caught
- Add resolution-failure test: when tags API finds no matching asset the
  code falls back to the original URL and exits non-zero with Error:

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…wnloads

Extends the GHES support pattern from extensions and presets (github#2855, github#3157)
to the bundle manifest download path: resolve_github_release_asset_api_url
now receives github_hosts=github_provider_hosts() so browser release URLs
from GitHub Enterprise Server instances are resolved via /api/v3 rather
than falling back to the unauthenticated download path.

Also adds a contract test covering the GHES resolution path for
_download_remote_manifest (analogous to the existing github.com tests).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes private/SSO-protected GitHub release bundle downloads by resolving
browser release URLs to GitHub REST API asset URLs, with full GHES support.
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