Conversation
![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 replaces the experimental Node.js/Bun-based Gemini API CLI with a robust Go implementation generated by Speakeasy, introducing comprehensive commands for managing agents, environments, triggers, and webhooks. The review feedback identifies a security concern where errors from storing secrets in the OS keychain are silently ignored, potentially falling back to plaintext storage in the configuration file without warning the user. Additionally, the reviewer noted that the readSecret helper function is unused and should be removed to clean up the codebase.
| if f := cmd.Flags().Lookup("api-key"); f != nil && f.Changed { | ||
| v, _ := cmd.Flags().GetString("api-key") | ||
| if config.StoreSecret("api-key", v, &cfg.Security.ApiKey) == nil { | ||
| keychainStored = true | ||
| } | ||
| changed = true | ||
| } | ||
| if f := cmd.Flags().Lookup("access-token"); f != nil && f.Changed { | ||
| v, _ := cmd.Flags().GetString("access-token") | ||
| if config.StoreSecret("access-token", v, &cfg.Security.AccessToken) == nil { | ||
| keychainStored = true | ||
| } | ||
| changed = true | ||
| } |
There was a problem hiding this comment.
When storing credentials via config.StoreSecret, any error returned by the function (such as keyring locked or DBus connection issues) is silently ignored. If the OS keychain is available but the storage operation fails, the secret falls back to being written in plaintext to config.yaml without any warning to the user.
To improve security and transparency, consider checking the returned error and printing a warning to cmd.ErrOrStderr() if the error is not config.ErrKeyringUnavailable.
| if f := cmd.Flags().Lookup("api-key"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("api-key") | |
| if config.StoreSecret("api-key", v, &cfg.Security.ApiKey) == nil { | |
| keychainStored = true | |
| } | |
| changed = true | |
| } | |
| if f := cmd.Flags().Lookup("access-token"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("access-token") | |
| if config.StoreSecret("access-token", v, &cfg.Security.AccessToken) == nil { | |
| keychainStored = true | |
| } | |
| changed = true | |
| } | |
| changed := false | |
| if f := cmd.Flags().Lookup("api-key"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("api-key") | |
| if err := config.StoreSecret("api-key", v, &cfg.Security.ApiKey); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store api-key in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| changed = true | |
| } | |
| if f := cmd.Flags().Lookup("access-token"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("access-token") | |
| if err := config.StoreSecret("access-token", v, &cfg.Security.AccessToken); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store access-token in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| changed = true | |
| } |
| if authApiKey != "" { | ||
| if config.StoreSecret("api-key", authApiKey, &cfg.Security.ApiKey) == nil { | ||
| keychainStored = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the non-interactive path, any error returned by config.StoreSecret when storing the api-key is silently ignored. Consider checking the error and printing a warning if the OS keychain is available but the storage operation fails.
| if authApiKey != "" { | |
| if config.StoreSecret("api-key", authApiKey, &cfg.Security.ApiKey) == nil { | |
| keychainStored = true | |
| } | |
| } | |
| if authApiKey != "" { | |
| if err := config.StoreSecret("api-key", authApiKey, &cfg.Security.ApiKey); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store api-key in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| } |
| if authAccessToken != "" { | ||
| if config.StoreSecret("access-token", authAccessToken, &cfg.Security.AccessToken) == nil { | ||
| keychainStored = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the non-interactive path, any error returned by config.StoreSecret when storing the access-token is silently ignored. Consider checking the error and printing a warning if the OS keychain is available but the storage operation fails.
if authAccessToken != "" {
if err := config.StoreSecret("access-token", authAccessToken, &cfg.Security.AccessToken); err == nil {
keychainStored = true
} else if err != config.ErrKeyringUnavailable {
fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store access-token in OS keychain: %v. Falling back to plaintext config file.\n", err)
}
}| if f := cmd.Flags().Lookup("api-key"); f != nil && f.Changed { | ||
| v, _ := cmd.Flags().GetString("api-key") | ||
| if config.StoreSecret("api-key", v, &cfg.Security.ApiKey) == nil { | ||
| keychainStored = true | ||
| } | ||
| changed = true | ||
| } | ||
| if f := cmd.Flags().Lookup("access-token"); f != nil && f.Changed { | ||
| v, _ := cmd.Flags().GetString("access-token") | ||
| if config.StoreSecret("access-token", v, &cfg.Security.AccessToken) == nil { | ||
| keychainStored = true | ||
| } | ||
| changed = true | ||
| } |
There was a problem hiding this comment.
When configuring credentials, any error returned by config.StoreSecret is silently ignored. If the OS keychain is available but the storage operation fails, the secret falls back to being written in plaintext to config.yaml without any warning to the user.
To improve security and transparency, consider checking the returned error and printing a warning to cmd.ErrOrStderr() if the error is not config.ErrKeyringUnavailable.
| if f := cmd.Flags().Lookup("api-key"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("api-key") | |
| if config.StoreSecret("api-key", v, &cfg.Security.ApiKey) == nil { | |
| keychainStored = true | |
| } | |
| changed = true | |
| } | |
| if f := cmd.Flags().Lookup("access-token"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("access-token") | |
| if config.StoreSecret("access-token", v, &cfg.Security.AccessToken) == nil { | |
| keychainStored = true | |
| } | |
| changed = true | |
| } | |
| if f := cmd.Flags().Lookup("api-key"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("api-key") | |
| if err := config.StoreSecret("api-key", v, &cfg.Security.ApiKey); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store api-key in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| changed = true | |
| } | |
| if f := cmd.Flags().Lookup("access-token"); f != nil && f.Changed { | |
| v, _ := cmd.Flags().GetString("access-token") | |
| if err := config.StoreSecret("access-token", v, &cfg.Security.AccessToken); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store access-token in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| changed = true | |
| } |
| if authApiKey != "" { | ||
| if config.StoreSecret("api-key", authApiKey, &cfg.Security.ApiKey) == nil { | ||
| keychainStored = true | ||
| } | ||
| } | ||
|
|
||
| if authAccessToken != "" { | ||
| if config.StoreSecret("access-token", authAccessToken, &cfg.Security.AccessToken) == nil { | ||
| keychainStored = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the non-interactive path, any error returned by config.StoreSecret when storing the credentials is silently ignored. Consider checking the error and printing a warning if the OS keychain is available but the storage operation fails.
| if authApiKey != "" { | |
| if config.StoreSecret("api-key", authApiKey, &cfg.Security.ApiKey) == nil { | |
| keychainStored = true | |
| } | |
| } | |
| if authAccessToken != "" { | |
| if config.StoreSecret("access-token", authAccessToken, &cfg.Security.AccessToken) == nil { | |
| keychainStored = true | |
| } | |
| } | |
| if authApiKey != "" { | |
| if err := config.StoreSecret("api-key", authApiKey, &cfg.Security.ApiKey); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store api-key in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| } | |
| if authAccessToken != "" { | |
| if err := config.StoreSecret("access-token", authAccessToken, &cfg.Security.AccessToken); err == nil { | |
| keychainStored = true | |
| } else if err != config.ErrKeyringUnavailable { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to store access-token in OS keychain: %v. Falling back to plaintext config file.\n", err) | |
| } | |
| } |
| func readSecret(cmd *cobra.Command) ([]byte, error) { | ||
| if f, ok := cmd.InOrStdin().(*os.File); ok && term.IsTerminal(int(f.Fd())) { | ||
| return term.ReadPassword(int(f.Fd())) | ||
| } | ||
|
|
||
| // Non-terminal fallback: read a line from cmd input (supports test harness piped input) | ||
| reader := bufio.NewReader(cmd.InOrStdin()) | ||
| line, err := reader.ReadString('\n') | ||
| if err != nil && line == "" { | ||
| return nil, err | ||
| } | ||
| return []byte(strings.TrimRight(line, "\r\n")), nil | ||
| } |
There was a problem hiding this comment.
The readSecret function is defined but never used anywhere in the codebase. To keep the codebase clean and maintainable, consider removing this unused function.
b31d6a4 to
c4e0b80
Compare
PiperOrigin-RevId: 978052929
c4e0b80 to
688ffab
Compare
1 participant
No public description