Repository navigation
Conversation
GitHub Actions
Inactive
cliAutomation
added
the
external
|
Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the gh auth git-credential helper to properly respect username parameters passed by git, enabling multi-account authentication workflows. Previously, the helper ignored usernames and always returned the active user's token, creating issues for users managing multiple GitHub accounts.
Key Changes:
- Restructured token lookup to check environment variables first, then look up user-specific tokens via
TokenForUser(), and finally fall back to the active user's token - Removed validation that rejected non-active usernames
- Added
TokenForUser()method to the config interface
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cmd/auth/gitcredential/helper.go | Implements new token lookup logic with environment variable priority, user-specific token lookups, and fallback handling |
| pkg/cmd/auth/gitcredential/helper_test.go | Adds test implementation for TokenForUser() and 3 new test cases for user-specific token scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // If user-specific token lookup failed, fall back to active token/user | ||
| if gotToken == "" { | ||
| gotToken, source = cfg.ActiveToken(lookupHost) | ||
| if gotToken == "" && strings.HasPrefix(lookupHost, "gist.") { | ||
| lookupHost = strings.TrimPrefix(lookupHost, "gist.") | ||
| gotToken, source = cfg.ActiveToken(lookupHost) | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback logic for user-specific token lookup uses the original lookupHost which may have already been modified in line 139. If the gist prefix was stripped during user-specific lookup, the fallback will use the modified lookupHost instead of the original wants[\"host\"], potentially causing incorrect token lookups.
| } else if strings.HasPrefix(wants["host"], "gist.") { | ||
| // Try without gist. prefix for user lookup | ||
| lookupHost = strings.TrimPrefix(wants["host"], "gist.") |
There was a problem hiding this comment.
The condition checks wants[\"host\"] but the prefix was already stripped from lookupHost on line 122. This should check strings.HasPrefix(lookupHost, \"gist.\") instead to correctly handle the gist fallback case for user-specific token lookups.
| } else if strings.HasPrefix(wants["host"], "gist.") { | |
| // Try without gist. prefix for user lookup | |
| lookupHost = strings.TrimPrefix(wants["host"], "gist.") | |
| } else if strings.HasPrefix(lookupHost, "gist.") { | |
| // Try without gist. prefix for user lookup | |
| lookupHost = strings.TrimPrefix(lookupHost, "gist.") |
Replace buggy implementation that mutates lookupHost with cleaner helper function approach. This fixes two issues identified in code review: 1. Lines 137-139 checked wants["host"] but should check lookupHost after gist prefix was stripped on line 122 2. Lines 146-153 used mutated lookupHost in fallback logic, potentially causing incorrect lookups The new tryBothHosts helper function encapsulates gist hostname fallback without state mutation, making the code clearer and less error-prone.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // If the host starts with "gist.", try without the prefix | ||
| if strings.HasPrefix(host, "gist.") { | ||
| strippedHost := strings.TrimPrefix(host, "gist.") | ||
| return lookup(strippedHost) |
There was a problem hiding this comment.
When the first lookup succeeds with err == nil but returns an empty token, the function returns (token, src, nil) at line 123. However, when the gist host fallback at line 128 fails, the function returns the original empty token from line 121, not the error from the fallback attempt. This could mask legitimate errors from the fallback lookup. Consider returning the fallback error when both lookups fail.
| return lookup(strippedHost) | |
| fallbackToken, fallbackSrc, fallbackErr := lookup(strippedHost) | |
| if fallbackErr == nil && fallbackToken != "" { | |
| return fallbackToken, fallbackSrc, nil | |
| } | |
| return fallbackToken, fallbackSrc, fallbackErr |
| if gotToken == "" { | ||
| gotToken, source, _ = tryBothHosts(lookupHost, func(host string) (string, string, error) { | ||
| token, src := cfg.ActiveToken(host) | ||
| if token == "" { | ||
| return "", "", fmt.Errorf("no token") | ||
| } | ||
| return token, src, nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
The error from tryBothHosts is silently discarded at line 159. This fallback logic should either handle the error or document why it's safe to ignore. If the active token lookup fails, the subsequent check at line 189 will catch the empty token, but explicit error handling would make the code's intent clearer.
| envToken, envSource, _ := tryBothHosts(lookupHost, func(host string) (string, string, error) { | ||
| token, src := cfg.ActiveToken(host) | ||
| if token == "" { | ||
| return "", "", fmt.Errorf("no token") | ||
| } | ||
| return token, src, nil | ||
| }) |
There was a problem hiding this comment.
The lambda function wrapping cfg.ActiveToken() is duplicated at lines 134-140, 159-165. Consider extracting this into a reusable helper function to reduce code duplication and improve maintainability.
- Return fallback error when both host lookups fail (instead of original error) - Extract activeTokenLookup helper to eliminate duplicate lambda functions - Add comment documenting intentional error discard in fallback logic
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if fallbackErr == nil && fallbackToken != "" { | ||
| return fallbackToken, fallbackSrc, nil | ||
| } | ||
| return fallbackToken, fallbackSrc, fallbackErr |
There was a problem hiding this comment.
When the gist host fallback fails, this returns the fallback error instead of the original error. This could mask the real issue. Consider returning the original error when both attempts fail, as it provides more accurate debugging information.
| return fallbackToken, fallbackSrc, fallbackErr | |
| return token, src, err |
There was a problem hiding this comment.
I dunno, I think this is debateable...
|
Thank you for your pull request! 🎉 This PR appears to fix the following issues that are not labeled with
help wanted
As outlined in our Contributing Guidelines, we expect that PRs are only created for issues that have been labeled While we appreciate your initiative, please note that:
What happens next:
Thank you for your understanding and contribution to the project! 🙏 This comment was automatically generated by cliAutomation. |
|
Closing as explained in #11938 (comment). |
Summary
gh auth git-credentialto properly look up tokens for specific users when git provides a username parameterauth git-credentialfails ifusernameprovided by git does not match active user #11938Changes
Previously, the git-credential helper ignored usernames passed by git (e.g.,
username=dilbertwork_acme) and always returned the active user's token. This behavior was inconsistent withgh auth token --user USERNAME, which correctly looks up tokens for specific users.This PR restructures the token lookup logic to:
TokenForUser()Testing
Fixes authentication for multi-account setups where git passes user-specific credentials to the helper.
Fixes #11938