Add GitLab Duo management OAuth and PAT endpoints#433
Add GitLab Duo management OAuth and PAT endpoints#433LuxVTZ wants to merge 1 commit intorouter-for-me:mainfrom
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 significantly enhances the management control panel by integrating GitLab Duo authentication capabilities. It provides users with the flexibility to authenticate using either OAuth or Personal Access Tokens, moving beyond CLI-only login methods. This change is particularly beneficial for Windows and release users, offering a more accessible and user-friendly experience for managing GitLab Duo access. 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. Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Related Web UI PR for the management panel GitLab cards: router-for-me/Cli-Proxy-API-Management-Center#143 |
![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 adds management API endpoints for GitLab Duo, including OAuth and PAT login flows, and callback handling. However, two significant security issues were identified in the RequestGitLabToken endpoint: sensitive OAuth credentials are passed via query parameters, leading to leakage in logs, and the lack of validation for the base_url parameter combined with the use of a GET route for state-changing operations creates a risk of SSRF and credential harvesting via CSRF or social engineering. Additionally, there's a critical panic risk in the OAuth flow due to unhandled errors, a medium-severity issue where raw errors are exposed to the client, and several medium-severity maintainability issues related to logging, error handling, and code redundancy. Addressing these security and maintainability concerns is crucial.
| var payload map[string]string | ||
| _ = json.Unmarshal(data, &payload) | ||
| _ = os.Remove(waitFile) |
There was a problem hiding this comment.
There's a potential panic here. If os.ReadFile succeeds but the file content is not valid JSON, json.Unmarshal will return an error, which is currently ignored. payload will be nil, and the subsequent access payload["error"] on line 1797 will cause a panic. You should handle the error from json.Unmarshal to prevent this.
var payload map[string]string
if err := json.Unmarshal(data, &payload); err != nil {
log.Errorf("Failed to unmarshal gitlab oauth callback file for state %s: %v", state, err)
SetOAuthSessionError(state, "Invalid callback data received")
_ = os.Remove(waitFile) // Attempt to clean up
return
}
_ = os.Remove(waitFile)| func (h *Handler) RequestGitLabToken(c *gin.Context) { | ||
| ctx := context.Background() | ||
| ctx = PopulateAuthContext(ctx, c) | ||
|
|
||
| fmt.Println("Initializing GitLab Duo authentication...") | ||
|
|
||
| baseURL := gitLabBaseURLFromRequest(c) | ||
| clientID := strings.TrimSpace(c.Query("client_id")) | ||
| clientSecret := strings.TrimSpace(c.Query("client_secret")) |
There was a problem hiding this comment.
The RequestGitLabToken handler retrieves sensitive information like client_id and client_secret from query parameters. Sensitive information like OAuth client secrets should never be passed in query strings as they are logged in plain text by web servers, proxies, and browser history, leading to credential leakage. Additionally, this endpoint performs state-changing operations (registering sessions, starting goroutines) but is defined as a GET route. According to RFC 7231, GET methods should be idempotent and not have side effects. Using GET for such operations also makes the endpoint vulnerable to Cross-Site Request Forgery (CSRF) attacks, especially given the permissive CORS configuration in this project. It is recommended to convert this endpoint to a POST route and pass all parameters in the request body. Furthermore, the fmt.Println call on line 1718, and similar calls within this block, should be replaced with calls to the structured logger (e.g., log.Info or log.Infof). Using the project's logger ensures consistent, level-based logging which is more maintainable and useful, especially in production.
|
|
||
| fmt.Println("Initializing GitLab Duo authentication...") | ||
|
|
||
| baseURL := gitLabBaseURLFromRequest(c) |
There was a problem hiding this comment.
The RequestGitLabToken handler accepts an arbitrary base_url from query parameters without validation. This URL is subsequently used to make server-side requests, including POST requests to the /oauth/token endpoint where the client_secret is transmitted.
An attacker could potentially trick an authenticated administrator into clicking a specially crafted link that initiates this flow with an attacker-controlled base_url. This would result in the server sending the OAuth client secret to the attacker's server (SSRF / Credential Leakage).
| if provider := strings.TrimSpace(direct.ModelDetails.ModelProvider); provider != "" { | ||
| modelDetails["model_provider"] = provider | ||
| metadata["model_provider"] = provider | ||
| } | ||
| if model := strings.TrimSpace(direct.ModelDetails.ModelName); model != "" { | ||
| modelDetails["model_name"] = model | ||
| metadata["model_name"] = model | ||
| } |
There was a problem hiding this comment.
The model_provider and model_name fields are being added to both the top-level metadata map and a nested model_details map. This seems redundant. To improve maintainability and reduce duplication, consider storing these values only in the model_details map. If both are required for backward compatibility, adding a comment to explain this would be beneficial.
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"status": "error", "error": err.Error()}) | ||
| return |
There was a problem hiding this comment.
Returning err.Error() directly in the API response can leak internal implementation details or sensitive information from upstream services, which could be a security risk. It's better to log the detailed error on the server and return a more generic error message to the client.
if err != nil {
log.Errorf("Failed to get GitLab current user: %v", err)
c.JSON(http.StatusBadRequest, gin.H{"status": "error", "error": "failed to validate personal access token"})
return
}| errStr = c.Query("error_description") | ||
| } | ||
| if state != "" { | ||
| _, _ = managementHandlers.WriteOAuthCallbackFileForPendingSession(s.cfg.AuthDir, "gitlab", state, code, errStr) |
There was a problem hiding this comment.
The error returned from WriteOAuthCallbackFileForPendingSession is being ignored. This function can fail for several reasons (e.g., invalid state, file system errors). Ignoring the error can make debugging difficult if the OAuth flow fails. It's recommended to log this error.
if _, err := managementHandlers.WriteOAuthCallbackFileForPendingSession(s.cfg.AuthDir, "gitlab", state, code, errStr); err != nil {
log.Warnf("Failed to write gitlab oauth callback file for state %s: %v", state, err)
}1 participant
Summary
Why
This makes GitLab Duo usable from the management control panel instead of requiring CLI-only login flows, which is important for Windows/release users.
Testing