fix(auth): ensure absolute paths for auth file handling#2269
fix(auth): ensure absolute paths for auth file handling#2269LTbinglingfeng merged 1 commit intodevfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the robustness of authentication file handling by ensuring that all file paths are treated as absolute paths. This change addresses potential security concerns and ensures consistent behavior across different environments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 correctly ensures that paths are absolute before being used to calculate a relative path, which is a good fix. I've identified some opportunities for improvement, including code duplication and a redundant function call. My review comments provide specific suggestions to enhance the code's maintainability.
| path = filepath.Clean(path) | ||
| if !filepath.IsAbs(path) { | ||
| if abs, errAbs := filepath.Abs(path); errAbs == nil { | ||
| path = abs | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic to ensure a path is absolute is duplicated for authDir on lines 755-760. To improve maintainability and reduce code duplication, consider extracting this logic into a helper function.
For example:
func toAbs(p string) string {
p = filepath.Clean(p)
if !filepath.IsAbs(p) {
if abs, err := filepath.Abs(p); err == nil {
return abs
}
}
return p
}With such a helper, this block and the one for authDir could be simplified to a single function call.
| authDir = resolvedAuthDir | ||
| } | ||
| if authDir != "" { | ||
| authDir = filepath.Clean(authDir) |
There was a problem hiding this comment.
The util.ResolveAuthDir function called on line 751 already returns a cleaned path, making this call to filepath.Clean redundant. You can safely remove this line.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5331d51f27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| authDir := strings.TrimSpace(h.cfg.AuthDir) | ||
| if resolvedAuthDir, errResolve := util.ResolveAuthDir(authDir); errResolve == nil && resolvedAuthDir != "" { | ||
| authDir = resolvedAuthDir | ||
| } | ||
| if authDir != "" { |
There was a problem hiding this comment.
This starts canonicalizing cfg.AuthDir inside authIDForPath, but the store-backed IDs are still derived from the raw config value (sdk/auth/filestore.go:260-271, internal/store/gitstore.go:452-460) and the SDK/service startup path I checked (sdk/cliproxy/service.go:493-505) never calls ResolveAuthDir. In embedded/server setups that pass AuthDir as ~/... or another non-canonical form, the same auth file will now load from the store under one ID and be updated/deleted through management under a different ID, which leaves stale entries behind or creates duplicates instead of updating the existing auth.
Useful? React with 👍 / 👎.
fix(auth): ensure absolute paths for auth file handling
1 participant
No description provided.