feat(token): use github proxy instead of retrieving raw tokens#56
feat(token): use github proxy instead of retrieving raw tokens#56risu729 wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the GitHub token management system by replacing direct token retrieval with a centralized GitHub proxy approach. Instead of clients fetching tokens from a token manager API and handling rate limits themselves, they now route all GitHub API requests through a proxy endpoint that manages tokens and rate limiting server-side.
Key Changes:
- Introduces a new GitHub proxy endpoint (
web/src/pages/gh/[...path].ts) that handles token rotation and rate limiting transparently - Updates all scripts and workflows to use
GITHUB_PROXY_URLandAPI_SECRETinstead ofTOKEN_MANAGER_URLandTOKEN_MANAGER_SECRET - Removes client-side token management code including
scripts/github-token.js
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/gh/[...path].ts | New GitHub proxy endpoint that manages token rotation, rate limiting, and retries server-side |
| scripts/update.sh | Refactored to use proxy via MISE_URL_REPLACEMENTS instead of managing tokens directly; removed token manager functions |
| scripts/migrate.js | Updated environment variable names and removed trailing whitespace |
| scripts/github-token.js | Deleted - token management logic moved to server-side proxy |
| scripts/fetch-metadata.js | Updated to use GitHub proxy for API calls; removed client-side token rotation logic |
| scripts/backfill-created-at.js | Updated to use GitHub proxy via MISE_URL_REPLACEMENTS; removed token manager integration |
| CLAUDE.md | Updated documentation to reflect proxy-based token management |
| .github/workflows/update.yml | Updated to use GITHUB_PROXY_URL and API_SECRET; fixed boolean default value |
| .github/workflows/tool-analysis.yml | Consolidated GITHUB_TOKEN environment variable; removed trailing whitespace |
| .github/workflows/python.yml | Consolidated GITHUB_TOKEN environment variable |
| .github/workflows/metadata.yml | Removed TOKEN_MANAGER_* variables and added GITHUB_PROXY_URL |
| .github/workflows/backfill.yml | Removed TOKEN_MANAGER_* variables and added GITHUB_PROXY_URL with API_SECRET |
| .github/workflows/aqua.yml | Consolidated GITHUB_TOKEN environment variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Forward request | ||
| const headers = new Headers(request.headers); | ||
| headers.set("Authorization", `Bearer ${token.token}`); |
There was a problem hiding this comment.
The proxy forwards all request headers from the client to GitHub API, which could potentially leak sensitive information or cause unexpected behavior. Consider explicitly allowlisting headers to forward (e.g., Accept, User-Agent, If-None-Match) instead of forwarding all headers, particularly removing the original Authorization header before setting the new one with the token.
There was a problem hiding this comment.
We basically use this proxy from GitHub Actions, so this should be fine. We need to remove some headers if GitHub refuses access with them (e.g. headers by Cloudflare).
|
Great idea! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
risu729
force-pushed
the
github-proxy
branch
from
bd02369 to
72e7e1f
Compare
risu729
force-pushed
the
github-proxy
branch
from
4e6ce0f to
7d7c377
Compare
Deactivate tokens immediately when GitHub returns 401 so invalid or revoked tokens are not retried across requests. Made-with: Cursor
Bring back registry-based cleanup so stale docs files are removed when tools no longer exist in the current mise registry. Made-with: Cursor
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the GitHub token handling by introducing a server-side proxy. This removes the client-side token manager, simplifying the scripts in scripts/ and improving security by not exposing raw tokens to the clients. The changes are well-executed, removing the old token management logic and adapting the scripts to use the new proxy via environment variables. I've found a minor issue in scripts/backfill-created-at.js where a function can be simplified.
Greptile SummaryThis PR replaces the client-side token manager with a server-side GitHub API proxy at Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant mise as mise / scripts
participant proxy as GitHub Proxy<br/>(mise-versions.jdx.dev/gh/...)
participant db as D1 Token DB
participant gh as api.github.com
mise->>proxy: GET /gh/{path}<br/>Authorization: Bearer <API_SECRET>
proxy->>proxy: requireApiAuth(API_SECRET)
proxy->>db: deactivateExpiredTokens() [async]
proxy->>db: getNextToken() [round-robin, LRU]
db-->>proxy: token {id, token}
proxy->>gh: GET /{path}<br/>Authorization: Bearer <github_token>
gh-->>proxy: response + x-ratelimit-* headers
alt 403/429 rate limited
proxy->>db: markTokenRateLimited(token.id, resetAt)
proxy->>db: getNextToken() [retry, up to 3x]
db-->>proxy: next token
proxy->>gh: retry with new token
gh-->>proxy: 200 OK
else 401 unauthorized
proxy->>db: deactivateToken(token.id)
proxy->>db: getNextToken() [retry]
db-->>proxy: next token
proxy->>gh: retry with new token
gh-->>proxy: 200 OK
else success
proxy-->>mise: forward response body + headers
end
Last reviewed commit: "Merge branch 'main' ..." |
Drop the unused retries parameter from timestamp fetch helper and update the call site to match current proxy-driven retry behavior. Made-with: Cursor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| (rateLimitRemaining === "0" && rateLimitReset) || | ||
| response.status === 429 | ||
| ) { | ||
| isRateLimited = true; |
There was a problem hiding this comment.
x-ratelimit-reset leaves bad token in pool
When GitHub returns a 403 with x-ratelimit-remaining: 0 but does not include an x-ratelimit-reset header, neither branch of the inner if/else if triggers:
(rateLimitRemaining === "0" && rateLimitReset)→false(becauserateLimitResetisnull)response.status === 429→falseresponse.status === 401→false
isRateLimited stays false, shouldRetry evaluates to false, and the 403 is returned directly to the caller — without the token being marked or the loop retrying. On the very next request, getNextToken() could select the same exhausted token again, producing another immediate 403.
GitHub almost always includes x-ratelimit-reset, but to be safe the condition could default to a 1-hour mark when the header is absent:
| if ( | |
| (rateLimitRemaining === "0" && rateLimitReset) || | |
| response.status === 429 | |
| ) { | |
| isRateLimited = true; | |
| if ( | |
| (rateLimitRemaining === "0") || | |
| response.status === 429 | |
| ) { |
With this change the existing fallback (let resetDate = new Date(Date.now() + 60 * 60 * 1000)) is used when rateLimitReset is absent, which is already handled a few lines below.
| // Forward request | ||
| const headers = new Headers(request.headers); | ||
| headers.set("Authorization", `Bearer ${token.token}`); |
There was a problem hiding this comment.
new Headers(request.headers) copies every incoming header verbatim — including Authorization: Bearer <api_secret> (which is correctly overridden one line later), Cookie, and Cloudflare internal headers like CF-Connecting-IP and CF-Ray. While the Authorization override prevents the API secret from leaking, the other headers are forwarded unnecessarily.
Consider building a fresh, minimal headers object instead of copying the full client request:
| // Forward request | |
| const headers = new Headers(request.headers); | |
| headers.set("Authorization", `Bearer ${token.token}`); | |
| const headers = new Headers({ | |
| Authorization: `Bearer ${token.token}`, | |
| Accept: request.headers.get("Accept") ?? "application/vnd.github.v3+json", | |
| "User-Agent": request.headers.get("User-Agent") ?? "mise-versions", | |
| }); |
This ensures only intentional, non-sensitive headers reach the GitHub API upstream.
| if (!shouldRetry) { | ||
| return new Response(response.body, { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers: response.headers, | ||
| }); |
There was a problem hiding this comment.
X-OAuth-Scopes) forwarded to callers
Passing headers: response.headers directly to the client includes GitHub API internal response headers such as X-OAuth-Scopes, X-Authenticated-OAuth-Client-Id, and X-GitHub-Request-Id. These reveal the scopes and identity of the token pool being used.
Although access is already gated by API_SECRET, stripping (or explicitly allowlisting) these headers before returning is a defensive-in-depth measure:
| if (!shouldRetry) { | |
| return new Response(response.body, { | |
| status: response.status, | |
| statusText: response.statusText, | |
| headers: response.headers, | |
| }); | |
| const filteredHeaders = new Headers(response.headers); | |
| for (const h of ["x-oauth-scopes", "x-authenticated-oauth-client-id"]) { | |
| filteredHeaders.delete(h); | |
| } | |
| return new Response(response.body, { | |
| status: response.status, | |
| statusText: response.statusText, | |
| headers: filteredHeaders, | |
| }); |
|
@jdx Are you planning to merge this? If so, I'm happy to resolve conflicts/reviews. |
|
hold off for now, I'm not sure if I want to |
This PR removes the token manager (currently kept for compatibility) and implements a GitHub proxy at
https://mise-versions.jdx.dev/gh/that automatically forwards requests with a rotated GitHub API token.This will slightly improve the security by keeping the tokens in the server, and also make the tokens easier to use, as we no longer need to implement retry logic on the consumer side.
Requires jdx/mise#7593. Will replace jdx/mise#7397.