fix(github): download assets via API to respect GITHUB_TOKEN#6496
fix(github): download assets via API to respect GITHUB_TOKEN#6496
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes GitHub asset download authentication issues by switching from browser download URLs to API URLs that properly respect GITHUB_TOKEN authentication for private repositories.
- Added
urlfield toGithubAssetstruct to access API download URLs - Updated asset URL resolution to use
asset.urlinstead ofasset.browser_download_url
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/github.rs | Added url field to GithubAsset struct for API-based downloads |
| src/backend/github.rs | Updated asset URL resolution to use authenticated API URLs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cab898c to
c76988b
Compare
|
Guess this adds more calls towards the infamous GitHub rate limit 😢 which causes the tests to fail. |
|
There seem to be other issues too. We need to explicitly set an Accept header and also use an explicit name since the |
336daaa to
e6d48e9
Compare
| assert_contains "cat mise.lock" "[tools.\"github:jdx/mise-test-fixtures\".platforms.$PLATFORM_KEY]" | ||
| assert_contains "cat mise.lock" 'url = "https://github.com/jdx/mise-test-fixtures/releases/download/v1.0.0/hello-world-1.0.0.tar.gz"' | ||
| assert_contains "cat mise.lock" 'name = "hello-world-1.0.0.tar.gz"' | ||
| assert_contains "cat mise.lock" 'url = "https://api.github.com/repos/jdx/mise-test-fixtures/releases/assets/272317030"' |
There was a problem hiding this comment.
hmm I definitely don't like not seeing the tarball url in here
There was a problem hiding this comment.
Yeah, the only URL containing the tarball is the browser_download_url which does not work for private repositories.
There was a problem hiding this comment.
could we use browser_download_url if hostname == github.com?
There was a problem hiding this comment.
hostname is always github.com unless you self-host, so private repositories would still not work
There was a problem hiding this comment.
ohh I see, sorry I was thinking of ghe
There was a problem hiding this comment.
would it be a massive pain to make this fallback behavior? I really don't want to lose the tarball urls in the lockfile
There was a problem hiding this comment.
yes currently this is a breaking change since we need both url and name for it to work. we probably have to use a new api_url (beside the name) instead. question is when to use those? if we have a GITHUB_TOKEN set?
There was a problem hiding this comment.
if we have a GITHUB_TOKEN set?
I don't think this would work since we'd still want the tarball url for public repos even if GITHUB_TOKEN was set. I may wait for #6231 when I can work on that more since I'll already need to make some breaking changes with lockfiles
There was a problem hiding this comment.
Okey reworked it a bit.
Now we have a name and url_api derived from the release API call. url is the browser_download_url as before. We do a HEAD request to check if url (browser_download_url) works, if not we use the url_api. The name is used if it is set in the lockfile, if not we derive it from the url (like before).
|
bugbot run |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
bugbot run |
### 📦 Registry - add jules by @alefteris in [#6568](#6568) ### 🐛 Bug Fixes - **(docs)** improve favicon support for Safari by @jdx in [#6567](#6567) - **(github)** download assets via API to respect GITHUB_TOKEN by @roele in [#6496](#6496) - **(task)** load toml tasks in `task_config.includes` in system/global config and monorepo subdirs by @risu729 in [#6545](#6545) - **(task)** handle dots in monorepo directory names correctly by @jdx in [#6571](#6571) ### 📚 Documentation - **(readme)** add GitHub Issues & Discussions section by @rsyring in [#6573](#6573) - **(tasks)** create dedicated monorepo tasks documentation by @jdx in [#6561](#6561) - **(tasks)** enhance monorepo documentation with tool comparisons by @jdx in [#6563](#6563)
## Summary - Stop adding GitHub `Authorization` and `X-GitHub-Api-Version` headers to non-API GitHub URLs, including `github.com` release browser downloads and `*.githubusercontent.com` content/CDN hosts. - Keep GitHub auth/version headers for REST API URLs: `api.github.com`, GHES-style `HOSTNAME/api/v3`, and GHE.com data-residency `api.SUBDOMAIN.ghe.com`. - Remove the stale `is_gh_host` HTTP routing branch so GitHub header generation is reached only for URLs classified as GitHub REST API URLs. - Preserve API asset downloads by keeping `Accept: application/octet-stream` on `/releases/assets/` API URLs only. - Add regression coverage for browser/download hosts, GitHub API URLs, GHES API paths, GHE.com data-residency API hosts, and GHES/GHE.com browser URLs that must not receive auth headers. ## Root Cause The token/header policy added for GitHub-related hosts was too broad. Public release browser URLs on `github.com` can reject requests when a GitHub token is attached, which forces mise to fall back to the API asset URL. That fallback still works, but it loses the clearer browser download URL/filename path and increases API usage, which earlier work tried to avoid. ## GHE Notes - GitHub Enterprise Cloud without data residency uses the normal GitHub.com REST API host. - GitHub Enterprise Server uses `https://HOSTNAME/api/v3/...`, so this keeps auth on non-`github.com` `/api/v3` URLs. - GitHub Enterprise Cloud with data residency uses `SUBDOMAIN.ghe.com` for the web host and `api.SUBDOMAIN.ghe.com` for REST API. Token lookup tries `SUBDOMAIN.ghe.com` before `api.SUBDOMAIN.ghe.com`, matching the platform hostname users authenticate with, while still not sending headers to the web/download host. - GHE.com web hosts are explicitly excluded from the GHES `/api/v3` path matcher; only `api.SUBDOMAIN.ghe.com` is treated as a GHE.com REST API host. ## Review Follow-up - Addressed review feedback to remove the redundant `is_gh_host` branch from `src/http.rs` now that `github::get_headers` and `is_github_api_url` enforce API-only header behavior. - Addressed review feedback to exclude `.ghe.com` web hosts from the GHES `/api/v3` path arm. - Added the missing GHES browser download no-auth regression case. - Left `MISE_GITHUB_ENTERPRISE_TOKEN` precedence unchanged for non-`github.com` API hosts, including `api.SUBDOMAIN.ghe.com`, because this PR is scoped to header destination safety and the existing token precedence treats non-`github.com` GitHub Enterprise hosts as enterprise hosts. ## References - Fixes #8865 - Follow-up to #9060 - Rebalances host matching added in #8692 - Preserves the API fallback behavior from #6496 - Private release browser URLs still require API asset downloads: https://github.com/orgs/community/discussions/47453 - GitHub release asset API/browser behavior: https://docs.github.com/en/rest/releases/assets?apiVersion=2022-11-28#get-a-release-asset - GitHub API version header scope/defaults: https://docs.github.com/en/rest/about-the-rest-api/api-versions - GHES REST API base URL: https://docs.github.com/en/enterprise-server@3.17/rest/using-the-rest-api/getting-started-with-the-rest-api - GHE.com data-residency API and network details: https://docs.github.com/en/enterprise-cloud@latest/admin/data-residency/about-github-enterprise-cloud-with-data-residency and https://docs.github.com/en/enterprise-cloud@latest/admin/data-residency/network-details-for-ghecom ## Tests - `cargo fmt --all` - `cargo test --all-features github::tests` - `git diff --check` - `curl -sSIL` on the reported `github.com/cuotos/ecs-exec-pf` asset URL returns `200` and redirects to `release-assets.githubusercontent.com` without auth headers. Notes: `mise run format` did not discover tasks in this checkout, so I ran `cargo fmt --all` directly. Broad `cargo clippy --all-features -- -Dwarnings` and `cargo clippy -p mise --all-features --no-deps -- -Dwarnings` currently fail on unrelated pre-existing clippy warnings outside this change. *This PR description was AI-generated.*
Currently GitHub backend is not able to download assets from private repositories since the
asset.browser_download_urldoes not respect theGITHUB_TOKEN(Bearer token header respectively). We should useasset.urlinstead.Note
Switch downloads to API-backed asset URLs (token-aware), add asset name/url/url_api to lockfiles, and set correct headers for GitHub release assets.
ReleaseAsset { name, url, url_api }and propagate through resolution and install flows.name,url, andurl_apiintv.lock_platforms.url; on failure, fall back tourl_api; pass appropriate headers; set GitHubaccept: application/octet-streamfor/releases/assets/.GithubAsset.url.PlatformInfowithnameandurl_api(serde read/write, conversions, tests updated).nameandurl_apialongsideurlandchecksumfor GitHub assets.Written by Cursor Bugbot for commit 5d6e347. This will update automatically on new commits. Configure here.