feat(copilot): implement GitHub Copilot authentication flow [2/5] #381
Conversation
- Add auth directory management helper (GetAuthDir) - Add random hex string generator for request IDs - Add helper for generating unique machine identifiers
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello @jeffnash, 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 integrates the full authentication mechanism for GitHub Copilot into the system. It provides a robust way for users to securely log in, obtain, and maintain their Copilot access tokens, supporting various account types and ensuring seamless operation. This foundational work is crucial for enabling future Copilot-powered features. Highlights
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
|
![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 introduces the authentication flow for GitHub Copilot, which is a substantial and important feature. The overall implementation is solid, but I've identified several critical issues that must be addressed. These include compilation errors due to a duplicated struct definition and a syntax error, a potential server crash from an unhandled error in the request ID generation, and a security vulnerability related to insecure directory permissions for storing tokens. I've also noted some medium-severity issues, such as ignoring potential errors from json.Marshal. Please review the detailed comments for suggestions on how to resolve these issues.
| if _, err := rand.Read(b); err != nil { | ||
| panic(fmt.Sprintf("failed to generate random bytes for request ID: %v", err)) | ||
| } |
There was a problem hiding this comment.
Calling panic in a server application can cause it to crash, making the service unavailable. The crypto/rand.Read function can fail if the operating system's entropy pool is exhausted. It's crucial to handle this error gracefully instead of panicking. A good approach is to fall back to a different UUID generation method.
Note: This suggestion requires importing github.com/google/uuid and github.com/sirupsen/logrus.
if _, err := rand.Read(b); err != nil {
log.Errorf("crypto/rand.Read failed for request ID, falling back to UUID: %v", err)
return uuid.New().String()
}
internal/config/config.go
Outdated
| // CopilotKey represents the configuration for GitHub Copilot API access. | ||
| // Authentication is handled via device code OAuth flow, not API keys. | ||
| type CopilotKey struct { | ||
| // AccountType is the Copilot subscription type (individual, business, enterprise). | ||
| // Defaults to "individual" if not specified. | ||
| AccountType string `yaml:"account-type" json:"account-type"` | ||
|
|
||
| // ProxyURL overrides the global proxy setting for Copilot requests if provided. | ||
| ProxyURL string `yaml:"proxy-url,omitempty" json:"proxy-url,omitempty"` | ||
|
|
||
| // AgentInitiatorPersist, when true, forces subsequent Copilot requests sharing the | ||
| // same prompt_cache_key to send X-Initiator=agent after the first call. Default false. | ||
| AgentInitiatorPersist bool `yaml:"agent-initiator-persist" json:"agent-initiator-persist"` | ||
| } |
There was a problem hiding this comment.
The CopilotKey struct is defined twice. This duplicate definition is a copy-paste error that will cause a compilation failure and should be removed.
| } | ||
| } |
There was a problem hiding this comment.
There is a syntax error here. The closing brace } on line 85 is incorrectly placed and indented, and the one on line 86 is extraneous. This will cause a compilation failure. The braces need to be adjusted to correctly close the nested if blocks.
| } | |
| } | |
| } | |
| } |
| info, err := os.Stat(dir) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| if mkErr := os.MkdirAll(dir, 0o755); mkErr != nil { |
There was a problem hiding this comment.
The auth directory is being created with 0o755 permissions, which is world-readable and -executable. For a directory intended to store sensitive authentication tokens, more restrictive permissions like 0700 (read/write/execute for owner only) should be used to enhance security. Another part of the code (internal/auth/copilot/storage.go) already uses 0700, so this change would also make permissions consistent across the application.
| if mkErr := os.MkdirAll(dir, 0o755); mkErr != nil { | |
| if mkErr := os.MkdirAll(dir, 0700); mkErr != nil { |
config.example.yaml
Outdated
| # GitHub Copilot account configuration | ||
| # Note: Copilot uses OAuth device code authentication, NOT API keys or tokens. | ||
| # Do NOT paste your GitHub access token or Copilot bearer token here. | ||
| # Tokens are stored only in auth-dir JSON files, never in config.yaml. | ||
| # | ||
| # To authenticate: | ||
| # - CLI: run with -copilot-login flag | ||
| # - Web: use the /copilot-auth-url management endpoint | ||
| # | ||
| # After OAuth login, tokens are managed automatically and stored in auth-dir. | ||
| # The entries below only configure account type and optional proxy settings. | ||
| #copilot-api-key: | ||
| # - account-type: "individual" # Options: individual, business, enterprise | ||
| # proxy-url: "socks5://proxy.example.com:1080" # optional: proxy for Copilot requests | ||
|
|
||
| # # When set to true, this flag forces subsequent requests in a session (sharing the same prompt_cache_key) | ||
| # # to send the header "X-Initiator: agent" instead of "vscode". This mirrors VS Code's behavior for | ||
| # # long-running agent interactions and helps prevent hitting standard rate limits. | ||
| # agent-initiator-persist: true |
There was a problem hiding this comment.
This configuration block for GitHub Copilot is a duplicate of the one above (lines 90-108). Please remove this redundant block to avoid confusion for users configuring the service.
| "client_id": GitHubClientID, | ||
| "scope": GitHubAppScopes, | ||
| } | ||
| bodyBytes, _ := json.Marshal(reqBody) |
There was a problem hiding this comment.
The error returned by json.Marshal is being ignored. While it's unlikely to fail for a map[string]string, it is a best practice to handle all errors to prevent potential panics or unexpected behavior in production.
bodyBytes, err := json.Marshal(reqBody)
if err != nil {
return nil, fmt.Errorf("%w: failed to marshal request body: %v", ErrDeviceCodeFailed, err)
}| "device_code": deviceCode, | ||
| "grant_type": "urn:ietf:params:oauth:grant-type:device_code", | ||
| } | ||
| bodyBytes, _ := json.Marshal(reqBody) |
There was a problem hiding this comment.
The error from json.Marshal is ignored. It's safer to handle this error to prevent unexpected behavior, even if a failure is unlikely with the current input.
bodyBytes, err := json.Marshal(reqBody)
if err != nil {
return "", fmt.Errorf("failed to marshal request body: %w", err)
}- Add Copilot section with agent-initiator-persist flag - Add scanner buffer size configuration - Add account type configuration option - Add copilot types with account type validation - Document configuration options in example config - Add tests for util and copilot types
- Add device-code OAuth flow with GitHub token exchange - Implement Copilot token acquisition and refresh logic - Add account type handling (individual/business/enterprise) - Add token persistence and storage management - Add CLI login command (cliproxy-api copilot login) - Register Copilot refresh handler in SDK - Validate account_type with warning for invalid values
jeffnash
force-pushed
the
feat/copilot-auth-flow
branch
from
9e7a1a4 to
0b7e617
Compare
2 participants
Summary
Implements the complete GitHub Copilot authentication system.
Changes
cliproxy-api copilot login)Stack
This is PR 2/5 in the Copilot feature stack:
Depends on: #380 - merge that first