fix(github): clearer error when a GitHub token is rejected with 401#11236
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared GitHub token environment-variable selection and detailed diagnostics for GitHub API ChangesGitHub authentication diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GitHubAPI
participant send_once_inner
participant github_unauthorized_report
Client->>GitHubAPI: Send authenticated request
GitHubAPI-->>send_once_inner: Return 401 Unauthorized
send_once_inner->>github_unauthorized_report: Pass auth presence, token source, and bounded response body
github_unauthorized_report-->>Client: Return structured diagnostic report
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds clearer diagnostics for GitHub 401 responses.
Confidence Score: 5/5The PR appears safe to merge. No blocking failures remain within the scope of the follow-up review. Important Files Changed
Reviews (7): Last reviewed commit: "test(github): isolate token source tests" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http.rs`:
- Around line 880-884: Update the auth status match in the request handling flow
to only report the env-var token label when used_github_token is true;
unauthenticated requests must remain “no” even if env_var is present. Add a
regression case covering false with Some("GITHUB_TOKEN").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: acfaa51b-d00b-48bf-8c66-d9f6edc68066
📒 Files selected for processing (2)
src/github.rssrc/http.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http.rs`:
- Line 764: Update the 401 response handling around format_response_body so the
response body is streamed with a fixed byte budget before formatting, rather
than fully buffered by resp.text(). Reuse the same bounded-body helper for the
equivalent 403 handling, preserving the existing formatted error behavior while
ensuring oversized or indefinitely trickling responses cannot consume unbounded
memory or block indefinitely.
- Around line 760-764: Update the response-error handling around
used_github_token, env_var, and resp.text() so env_var is retained only when the
sent Authorization Bearer credential matches the selected environment token;
caller-provided or netrc headers must use the generic authenticated wording. Add
a regression case covering a netrc or custom Authorization header alongside an
unrelated environment token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e26a875-bcd2-4177-be18-74d8a77925c0
📒 Files selected for processing (2)
src/github.rssrc/http.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/github.rs
a1671db to
4bfb7f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/github.rs`:
- Around line 624-633: Update env_var_for_token so standard-token matching
ignores environment variables whose trimmed value is empty, preserving the
filtering behavior used by resolve_token. Ensure token == "" cannot match any
GITHUB_TOKEN_ENV_VARS entry, while retaining existing matching for non-empty
trimmed tokens and enterprise-token handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 86c2e4e4-874b-44ab-a261-72a9033391d2
📒 Files selected for processing (2)
src/github.rssrc/http.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/http.rs
mise already surfaces a helpful message for GitHub 403 responses (auth state, rate limit, response body), but a 401 from a static invalid/expired token — the common case being an invalid or enterprise `GITHUB_TOKEN` that isn't valid for github.com — fell straight through to a bare `HTTP status client error (401 Unauthorized)` with no hint that mise was authenticating or which env var supplied the token. Add a 401 branch mirroring the existing 403 handling: when a GitHub API request returns 401, return an error that reports the auth state, names the token env var when one is set, includes the response body, and links to the token guide. The token env var is looked up via a new side-effect-free `github::auth_env_var_for_host` (no credential_command / oauth / network), so the error path can't re-spawn the credential helper. The existing OAuth device-flow 401-refresh path is unchanged and still runs first for mise-managed tokens. This intentionally does not add an automatic unauthenticated retry on 401, which was rejected in jdx#9965. Fixes discussion jdx#7218. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Direction here is right — this is the "at minimum, tell me which env var supplied the token" ask from #7218, without re-litigating the unauthenticated retry from #9965. Scope is fine by me, including the bounded error-body read. Two things I'd like:
Neither is a correctness problem — as it stands this is a clear improvement on a bare This comment was generated by an AI coding assistant. |
|
Addressed both follow-ups in b3ac992:
Validated with the focused header-source, 401-report, and OAuth refresh tests, plus This comment was generated by an AI coding assistant. |
Problem
When GitHub rejects an invalid, expired, wrong-host, or insufficiently scoped
token, mise previously returned only a bare
401 Unauthorized. The initialchange added an actionable report, but only environment-variable tokens could
be attributed precisely; tokens from the gh CLI,
github_tokens.toml, OAuth,credential commands, and Git credentials were reported generically.
Reported in discussion #7218.
Change
When a GitHub API request returns 401, mise now:
TokenSource;hint;
Bearer token on the error path instead of resolving credentials again;
tokens as
GitHub OAuth;docs/errors.md.Example:
The host-and-token match prevents netrc or caller-provided Authorization headers
from being attributed to an unrelated mise credential. It also avoids calling
resolve_tokenfrom the async error path, where OAuth resolution could performa blocking refresh.
Out of scope
Automatically retrying unauthenticated on 401 is intentionally not done; that
behavior was rejected in #9965. The existing bounded error-body handling and
OAuth retry remain unchanged.
Testing
cargo fmt --all -- --checkmarkdownlint docs/errors.mdcargo check --all-featuresgithub_tokens.toml,host mismatches, and token mismatches
TokenSourcevariants, unknown sources, andunauthenticated requests
refreshed token is also rejected
Fixes discussion #7218.
This pull request was generated by an AI coding assistant.