feat: implement CLI Proxy API server with backup and restore function… #894
Conversation
Summary of ChangesHello @AoaoMH, 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! 此拉取请求为CLI Proxy API服务器引入了全面的备份和恢复功能。它允许用户通过命令行或API接口对重要的系统文件（如环境变量、配置文件和认证文件）进行备份、列出、下载、删除和恢复操作。这一功能显著增强了系统的可靠性和可管理性，确保关键配置和用户认证信息可以轻松地进行保存和恢复。 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. 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
这个拉取请求通过 CLI 和一组新的管理 API 端点引入了备份和恢复功能。该实现涵盖了将环境变量、配置和身份验证数据备份到 zip 存档中。我的审查发现了一些严重的安全漏洞，特别是恢复逻辑中的 Zip Slip 问题，这可能允许在预期目录之外写入文件。我还注意到 CLI 命令和 API 处理程序之间存在大量重复代码，为了更好的可维护性，应将其重构到一个共享包中。此外，对于允许使用绝对路径作为备份位置以及几处应处理的被忽略的错误，我也存在担忧。我已经提供了修复这些问题的具体建议。
| relativePath := strings.TrimPrefix(f.Name, "auths/") | ||
| if relativePath == "" { | ||
| continue // Skip directory entry | ||
| } | ||
| destPath = filepath.Join(authDir, relativePath) |
There was a problem hiding this comment.
restoreFromZip 函数中存在 Zip Slip 漏洞。精心制作的 zip 文件可能包含像 ../../etc/passwd 这样的路径，这可能导致在预期的目标目录之外写入文件。这是一个严重的安全风险。您应该验证解压后的目标路径是否仍在预期的 authDir 目录内。
relativePath := strings.TrimPrefix(f.Name, "auths/")
if relativePath == "" {
continue // Skip directory entry
}
destPath = filepath.Join(authDir, relativePath)
if !strings.HasPrefix(destPath, filepath.Clean(authDir)) {
return fmt.Errorf("zip slip vulnerability detected: illegal file path in zip: %s", f.Name)
}
internal/cmd/backup.go
Outdated
| relativePath := strings.TrimPrefix(f.Name, "auths/") | ||
| if relativePath == "" { | ||
| continue | ||
| } | ||
| destPath = filepath.Join(authDir, relativePath) | ||
| if opts.AuthsMode == "incremental" { | ||
| log.Infof("Restoring auths/%s (incremental)", relativePath) | ||
| } |
There was a problem hiding this comment.
DoRestore 函数中存在与 API 处理程序中相同的 Zip Slip 漏洞。恶意制作的 zip 文件可能导致在目标目录之外写入文件。您应该在解压文件之前验证目标路径。
relativePath := strings.TrimPrefix(f.Name, "auths/")
if relativePath == "" {
continue
}
destPath = filepath.Join(authDir, relativePath)
if !strings.HasPrefix(destPath, filepath.Clean(authDir)) {
log.Errorf("zip slip vulnerability detected: illegal file path in zip: %s", f.Name)
return
}
if opts.AuthsMode == "incremental" {
log.Infof("Restoring auths/%s (incremental)", relativePath)
}| func (h *Handler) getBackupDirFromConfig(customPath string) string { | ||
| if customPath != "" { | ||
| if filepath.IsAbs(customPath) { | ||
| return customPath | ||
| } | ||
| wd, _ := os.Getwd() | ||
| return filepath.Join(wd, customPath) | ||
| } | ||
| return h.getBackupDir() | ||
| } |
There was a problem hiding this comment.
允许通过 API 使用绝对路径作为备份目录会带来安全风险，因为它可能允许将文件写入服务器上的任意位置。建议禁止使用绝对路径。此更改将需要更新函数签名以返回错误，并且调用方（CreateBackup）需要处理该错误。
func (h *Handler) getBackupDirFromConfig(customPath string) (string, error) {
if customPath != "" {
if filepath.IsAbs(customPath) {
return "", fmt.Errorf("absolute paths are not allowed for backup directory for security reasons")
}
wd, err := os.Getwd()
if err != nil {
return "", fmt.Errorf("failed to get working directory: %w", err)
}
return filepath.Join(wd, customPath), nil
}
return h.getBackupDir(), nil
}
internal/cmd/backup.go
Outdated
| func addFileToZip(zipWriter *zip.Writer, filePath, zipPath string) error { | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer file.Close() | ||
|
|
||
| info, err := file.Stat() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| header, err := zip.FileInfoHeader(info) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| header.Name = zipPath | ||
| header.Method = zip.Deflate | ||
|
|
||
| writer, err := zipWriter.CreateHeader(header) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| _, err = io.Copy(writer, file) | ||
| return err | ||
| } | ||
|
|
||
| func addDirToZip(zipWriter *zip.Writer, dirPath, zipBasePath string) error { | ||
| return filepath.Walk(dirPath, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| relPath, err := filepath.Rel(dirPath, path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| zipPath := filepath.Join(zipBasePath, relPath) | ||
| zipPath = strings.ReplaceAll(zipPath, "\\", "/") | ||
|
|
||
| if info.IsDir() { | ||
| if relPath != "." { | ||
| _, err := zipWriter.Create(zipPath + "/") | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| return addFileToZip(zipWriter, path, zipPath) | ||
| }) | ||
| } |
There was a problem hiding this comment.
在 internal/cmd/backup.go 和 internal/api/handlers/management/backup.go 之间存在大量重复代码，包括核心的备份/恢复逻辑以及 addFileToZip 和 addDirToZip 等辅助函数。为了提高可维护性并避免错误，建议将此共享逻辑提取到一个新的内部包中（例如 internal/backup），然后由 CLI 命令和 API 处理程序共同调用。
| metadataBytes, _ := json.Marshal(metadata) | ||
| metaWriter, err := zipWriter.Create("backup_metadata.json") | ||
| if err == nil { | ||
| metaWriter.Write(metadataBytes) | ||
| } |
There was a problem hiding this comment.
json.Marshal、zipWriter.Create 和 metaWriter.Write 返回的错误被忽略了。这些错误应该被妥善处理，以防止在创建备份元数据时发生静默失败。
metadataBytes, err := json.Marshal(metadata)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to marshal backup metadata: %v", err)})
return
}
metaWriter, err := zipWriter.Create("backup_metadata.json")
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to create metadata file in zip: %v", err)})
return
}
if _, err := metaWriter.Write(metadataBytes); err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to write metadata to zip: %v", err)})
return
}
internal/cmd/backup.go
Outdated
| backupDir := opts.BackupPath | ||
| if backupDir == "" { | ||
| backupDir = filepath.Join(wd, "backup") | ||
| } | ||
| if !filepath.IsAbs(backupDir) { | ||
| backupDir = filepath.Join(wd, backupDir) | ||
| } |
There was a problem hiding this comment.
允许在 CLI 中使用绝对路径作为备份目录存在安全风险。为了与 API 保持一致并遵循安全最佳实践，建议禁止使用绝对路径。
| backupDir := opts.BackupPath | |
| if backupDir == "" { | |
| backupDir = filepath.Join(wd, "backup") | |
| } | |
| if !filepath.IsAbs(backupDir) { | |
| backupDir = filepath.Join(wd, backupDir) | |
| } | |
| backupDir := opts.BackupPath | |
| if backupDir == "" { | |
| backupDir = filepath.Join(wd, "backup") | |
| } else if filepath.IsAbs(backupDir) { | |
| log.Errorf("absolute paths are not allowed for backup directory for security reasons: %s", backupDir) | |
| return | |
| } else { | |
| backupDir = filepath.Join(wd, backupDir) | |
| } |
internal/cmd/backup.go
Outdated
| metadataBytes, _ := json.Marshal(metadata) | ||
| metaWriter, err := zipWriter.Create("backup_metadata.json") | ||
| if err == nil { | ||
| metaWriter.Write(metadataBytes) | ||
| } |
There was a problem hiding this comment.
json.Marshal、zipWriter.Create 和 metaWriter.Write 返回的错误被忽略了。这些错误应该被妥善处理，以确保备份元数据被正确创建和写入。
metadataBytes, err := json.Marshal(metadata)
if err != nil {
log.Errorf("failed to marshal backup metadata: %v", err)
return
}
metaWriter, err := zipWriter.Create("backup_metadata.json")
if err != nil {
log.Errorf("failed to create metadata file in zip: %v", err)
return
}
if _, err := metaWriter.Write(metadataBytes); err != nil {
log.Errorf("failed to write metadata to zip: %v", err)
return
}- Move backup/restore/list/delete logic to shared package - API and CLI now reuse the same core implementation - Add 100MB upload size limit for API - Add path validation (API disallows absolute paths) - Improve list output to show backup content types
internal/backup/backup.go
Outdated
| info, _ := entry.Info() | ||
| metadata = BackupMetadata{ | ||
| Name: entry.Name(), | ||
| Date: info.ModTime().Format(time.RFC3339), | ||
| Size: info.Size(), | ||
| } |
There was a problem hiding this comment.
entry.Info() 返回的错误被忽略了。如果这个调用失败，info 将为 nil，后续访问 info.ModTime() 和 info.Size() 将导致 panic。您应该处理这个错误以防止应用程序崩溃。
info, errInfo := entry.Info()
if errInfo != nil {
// 无法获取文件信息，记录错误并跳过此备份条目。
continue
}
metadata = BackupMetadata{
Name: entry.Name(),
Date: info.ModTime().Format(time.RFC3339),
Size: info.Size(),
}| if authDir == "" { | ||
| wd, _ := os.Getwd() | ||
| authDir = filepath.Join(wd, "auths") | ||
| } |
There was a problem hiding this comment.
os.Getwd() 的错误在这里被忽略了。这与 getBackupDir 的处理方式不一致，后者通过回退到配置文件的目录来处理错误。为了健壮性和一致性，您应该处理 os.Getwd() 可能返回的错误。
| if authDir == "" { | |
| wd, _ := os.Getwd() | |
| authDir = filepath.Join(wd, "auths") | |
| } | |
| wd, err := os.Getwd() | |
| if err != nil { | |
| wd = filepath.Dir(h.configFilePath) | |
| } | |
| authDir = filepath.Join(wd, "auths") |
| // Convert to API response format | ||
| var response []BackupMetadataResponse | ||
| for _, b := range backups { | ||
| dateTime, _ := time.Parse(time.RFC3339, b.Date) |
There was a problem hiding this comment.
time.Parse 返回的错误被忽略了。如果备份元数据中的日期字符串损坏或格式不正确，将导致得到一个零值的 time.Time，并且没有任何问题提示。这可能导致备份排序不正确。应该处理这个错误，例如记录日志。
dateTime, err := time.Parse(time.RFC3339, b.Date)
if err != nil {
fmt.Fprintf(gin.DefaultErrorWriter, "警告：无法解析备份 '%s' 的日期：%v\n", b.Name, err)
}| req.Content.Auths = true | ||
| } | ||
|
|
||
| wd, _ := os.Getwd() |
There was a problem hiding this comment.
os.Getwd() 的错误被忽略了。虽然 os.Getwd() 很少失败，但为了代码的健壮性，处理这个错误是一个好习惯。如果失败，服务器可能会使用不正确的工作目录继续执行。建议处理此错误，例如返回一个内部服务器错误。
wd, err := os.Getwd()
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get working directory"})
return
}|
|
||
| // DeleteBackup deletes a backup | ||
| func (h *Handler) DeleteBackup(c *gin.Context) { | ||
| name := c.Query("name") |
There was a problem hiding this comment.
为了与建议的 RESTful 路由设计 (DELETE /backups/:name) 保持一致，这里应该修改为从路径参数而不是查询参数中读取备份名称。
name := c.Param("name")| } | ||
|
|
||
| backupDir := h.getBackupDir() | ||
| wd, _ := os.Getwd() |
There was a problem hiding this comment.
os.Getwd() 的错误被忽略了。虽然 os.Getwd() 很少失败，但为了代码的健壮性，处理这个错误是一个好习惯。如果失败，服务器可能会使用不正确的工作目录继续执行。建议处理此错误，例如返回一个内部服务器错误。
wd, err := os.Getwd()
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get working directory"})
return
}| return | ||
| } | ||
|
|
||
| wd, _ := os.Getwd() |
There was a problem hiding this comment.
os.Getwd() 的错误被忽略了。虽然 os.Getwd() 很少失败，但为了代码的健壮性，处理这个错误是一个好习惯。如果失败，服务器可能会使用不正确的工作目录继续执行。建议处理此错误，例如返回一个内部服务器错误。
wd, err := os.Getwd()
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get working directory"})
return
}
internal/api/server.go
Outdated
| mgmt.DELETE("/backups", s.mgmt.DeleteBackup) | ||
| mgmt.GET("/backups/download", s.mgmt.DownloadBackup) |
There was a problem hiding this comment.
在 RESTful API 中，标识特定资源通常使用路径参数而不是查询参数。例如，DELETE /backups/{name} 比 DELETE /backups?name={name} 更符合习惯。我建议将 DeleteBackup 和 DownloadBackup 路由更改为使用路径参数。您还需要更新 internal/api/handlers/management/backup.go 中相应的处理程序，使用 c.Param("name") 代替 c.Query("name")。
| mgmt.DELETE("/backups", s.mgmt.DeleteBackup) | |
| mgmt.GET("/backups/download", s.mgmt.DownloadBackup) | |
| mgmt.DELETE("/backups/:name", s.mgmt.DeleteBackup) | |
| mgmt.GET("/backups/download/:name", s.mgmt.DownloadBackup) |
internal/cmd/backup.go
Outdated
| authDir := cfg.AuthDir | ||
| if authDir == "" { | ||
| authDir = filepath.Join(wd, "auths") | ||
| } |
There was a problem hiding this comment.
用于确定 authDir 的逻辑在这里、DoRestore 函数以及 internal/api/handlers/management/backup.go 文件中都重复了。为了提高可维护性并避免潜在的不一致，请考虑将此逻辑集中到一个共享的辅助函数中。例如，您可以在 config 包或一个工具包中添加一个解析认证目录的函数。
internal/cmd/backup.go
Outdated
|
|
||
| // ListBackups lists all available backups | ||
| func ListBackups(backupPath string) { | ||
| wd, _ := os.Getwd() |
There was a problem hiding this comment.
os.Getwd() 的错误被忽略了。这与同一文件中的 DoBackup 和 DoRestore 函数处理此错误的方式不一致。为了一致性和健壮性，您应该处理这个潜在的错误。
wd, err := os.Getwd()
if err != nil {
log.Errorf("failed to get working directory: %v", err)
return
}- Handle entry.Info() errors in ListBackups to prevent nil pointer panic - Handle os.Getwd() errors consistently across all backup handlers - Handle time.Parse() errors in ListBackups API handler - Change DeleteBackup and DownloadBackup routes to use path params (DELETE /backups/:name, GET /backups/download/:name) for RESTful design - Add ResolveAuthDir() helper to centralize auth directory resolution - Remove duplicated authDir logic from CLI and API handlers
1 participant
支持备份环境变量、配置和认证文件。