From 80c59e2c2ce67faee031f5ee2ec30b5a2d514c7c Mon Sep 17 00:00:00 2001 From: Your Name Date: Fri, 29 May 2026 07:33:19 +0800 Subject: [PATCH] fix: harden avatar upload path and sync review truth --- docs/review-fix-closure-2026-05-28.md | 64 +++++++++++++++++++ docs/status/REAL_PROJECT_STATUS.md | 21 +++++- internal/api/handler/avatar_handler.go | 24 ++++++- .../api/handler/avatar_handler_path_test.go | 33 ++++++++++ 4 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 internal/api/handler/avatar_handler_path_test.go diff --git a/docs/review-fix-closure-2026-05-28.md b/docs/review-fix-closure-2026-05-28.md index 5159e53..753b48c 100644 --- a/docs/review-fix-closure-2026-05-28.md +++ b/docs/review-fix-closure-2026-05-28.md @@ -60,6 +60,70 @@ - `internal/api/middleware/ratelimit.go` - `frontend/admin/scripts/run-playwright-auth-e2e.sh` +### 7. 内存限流器全局误伤与条目泄漏风险 +- 问题：`internal/api/middleware/ratelimit.go` 之前按 endpoint 只创建单一 limiter，导致同一接口上的所有用户共享一个桶；同时缺少空闲条目清理策略，无法对历史 client key 做收敛。 +- 修复：改为按 `endpoint + user_id/IP` 分桶，并在访问路径上按 TTL 清理长期空闲的 limiter 条目。 +- 回归测试： + - 不同 IP 的登录限流相互独立 + - 共享 IP 下不同 `user_id` 的 API 限流相互独立 + - 空闲 limiter 会被清理，不再无限累积 +- 涉及文件： + - `internal/api/middleware/ratelimit.go` + - `internal/api/middleware/ratelimit_test.go` + +### 8. handler context 类型断言补强 +- 问题：`SSOHandler` 与 `WebhookHandler` 仍存在 `user_id.(int64)` / `username.(string)` 直接断言，若 middleware 注入异常类型会触发 panic。 +- 修复：统一复用 `getUserIDFromContext` / `getUsernameFromContext`，类型不匹配时返回 `401 unauthorized`，避免 handler panic。 +- 回归测试： + - `SSOHandler.Authorize` 非法 context 类型返回 `401` + - `SSOHandler.UserInfo` 非法 context 类型返回 `401` + - `WebhookHandler.CreateWebhook/ListWebhooks` 非法 context 类型返回 `401` +- 涉及文件： + - `internal/api/handler/auth_handler.go` + - `internal/api/handler/sso_handler.go` + - `internal/api/handler/webhook_handler.go` + - `internal/api/handler/context_guard_test.go` + +### 9. 密码强度 + 静默错误补强 +- 问题：review 报告中指出两类尾部问题： + - 默认密码校验对刚好达到最小长度的短密码过于宽松 + - TOTP / 操作日志链路存在 `_ = err`、`_ = json.Unmarshal(...)`、`_ = repo.Create(...)` 这类静默吞错 +- 修复： + - `validatePasswordStrength` 改为对“刚好达到最小长度”的密码要求至少 3 种字符类型；较长密码仍保留 2 种类型可过的兼容行为 + - `TOTPService` 对恢复码摘要、JSON 编解码、`UpdateTOTP` 持久化失败全部显式返回错误，不再静默忽略 + - `OperationLogMiddleware` 对 nil repo fail-safe 返回；异步落库失败改为写日志，不再无声吞错 +- 回归测试： + - 8 位两类字符密码被拒绝，8 位三类字符密码通过，较长两类字符密码仍通过 + - 损坏的恢复码 JSON 会返回解析错误 + - 恢复码消费后持久化失败会显式返回更新错误 + - operation log 在 nil repo 情况下不会 panic，参数脱敏/非 JSON fallback 继续受测 +- 涉及文件： + - `internal/service/auth.go` + - `internal/service/auth_service_test.go` + - `internal/service/auth_password_internal_test.go` + - `internal/service/totp.go` + - `internal/service/totp_internal_test.go` + - `internal/api/middleware/operation_log.go` + - `internal/api/middleware/operation_log_test.go` + +### 10. review 报告真相校准 + avatar 路径硬化 +- 真相校准：`PROJECT_REVIEW_REPORT.md` 中一批条目已不再代表当前仓库真相，至少包括： + - `uploadAvatar` 字段名错误：前后端当前都使用 `avatar`，该条为陈旧误报 + - `StateManager` 无法停止、`L1Cache` 无容量限制、密码强度过宽松、操作日志未转义、Webhooks 客户端全量分页、`ContactBindingsSection` 未复用：均已在后续提交中关闭 +- 仍值得继续跟踪、但已不构成功能 blocker 的尾项： + - `social_account_repo.go` 仍是原生 SQL 实现 + - `AuthProvider` 仍保留 React state + session store 双轨状态管理 + - `ApiResponse.data` 空值建模仍偏乐观（`T` 而非 `T | null`） +- 本轮额外修复： + - 将头像上传目录从运行时相对路径解析改为绝对路径归一化，避免 cwd 漂移导致文件落盘位置不稳定 + - 扩展名校验统一转小写，避免 `.JPG/.PNG` 这类常见文件名被误拒 +- 回归测试： + - `resolveAvatarUploadDir("")` 返回绝对路径且收敛到 `/uploads/avatars` + - 自定义根目录会被保留并归一化到 `/avatars` +- 涉及文件： + - `internal/api/handler/avatar_handler.go` + - `internal/api/handler/avatar_handler_path_test.go` + ## 验证结果 ### 后端 diff --git a/docs/status/REAL_PROJECT_STATUS.md b/docs/status/REAL_PROJECT_STATUS.md index 099033e..964512d 100644 --- a/docs/status/REAL_PROJECT_STATUS.md +++ b/docs/status/REAL_PROJECT_STATUS.md @@ -28,7 +28,7 @@ - 浏览器级真实 E2E 已闭环 **当前活跃阻塞：** -- 无新的功能性阻塞；剩余工作主要是提交边界整理与文档/工作树卫生收口 +- 无新的功能性阻塞；review 报告已完成真相校准，剩余工作以维护性尾项（如 raw SQL / 前端状态收敛 / 类型真相）和提交边界整理为主 ### 当前可诚实复用的一句话状态 @@ -1206,6 +1206,25 @@ - 前端 `window.alert/confirm/prompt/open` 保护链路已确认存在且有测试覆盖： - [`frontend/admin/src/app/bootstrap/installWindowGuards.ts`](/D:/project/frontend/admin/src/app/bootstrap/installWindowGuards.ts) +## 2026-05-28 review 后续修复补充 + +- 修复 `internal/api/middleware/ratelimit.go` 的真实运行时缺陷： + - 旧实现按 endpoint 共享单一内存桶，导致同一路由上的所有用户共用限流额度，存在全局误伤。 + - 旧实现也缺少历史 client limiter 的空闲清理策略，长期运行下存在条目累积风险。 +- 新实现改为按 `endpoint + user_id/IP` 分桶，并在访问路径上按 TTL 清理空闲 limiter 条目。 +- 补齐 handler context 类型守卫：`SSOHandler`、`WebhookHandler` 不再直接做 `user_id.(int64)` / `username.(string)` 断言，异常 context 会稳定返回 `401` 而不是 panic。 +- 新增回归测试覆盖： + - 不同 IP 的登录限流互不影响 + - 共享 IP 下不同 `user_id` 的 API 限流互不影响 + - 空闲 limiter 条目会被回收 + - `SSOHandler` / `WebhookHandler` 非法 context 类型返回 `401` +- 本轮后端验证已执行通过： + - `go test ./internal/api/middleware -count=1` + - `go test ./internal/api/handler -count=1` + - `go test ./... -count=1` + - `go vet ./...` + - `go build ./cmd/server` + ## 当前运行时真实能力 - 密码登录：启用 diff --git a/internal/api/handler/avatar_handler.go b/internal/api/handler/avatar_handler.go index 0166eac..eed6ad4 100644 --- a/internal/api/handler/avatar_handler.go +++ b/internal/api/handler/avatar_handler.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "github.com/gin-gonic/gin" @@ -42,6 +43,21 @@ func generateSecureToken(length int) (string, error) { return hex.EncodeToString(bytes)[:length], nil } +func resolveAvatarUploadDir(baseDir string) (string, error) { + if baseDir == "" { + baseDir = "./uploads" + } + cleanRoot := filepath.Clean(baseDir) + if !filepath.IsAbs(cleanRoot) { + absRoot, err := filepath.Abs(cleanRoot) + if err != nil { + return "", fmt.Errorf("resolve upload root: %w", err) + } + cleanRoot = absRoot + } + return filepath.Join(cleanRoot, "avatars"), nil +} + // UploadAvatar 上传用户头像 // @Summary 上传用户头像 // @Description 上传并更新用户头像（仅本人或管理员） @@ -92,7 +108,7 @@ func (h *AvatarHandler) UploadAvatar(c *gin.Context) { } // Validate file type - ext := filepath.Ext(file.Filename) + ext := strings.ToLower(filepath.Ext(file.Filename)) allowedExts := map[string]bool{".jpg": true, ".jpeg": true, ".png": true, ".gif": true, ".webp": true} if !allowedExts[ext] { c.JSON(http.StatusBadRequest, gin.H{"code": 400, "message": "invalid file type, allowed: jpg, jpeg, png, gif, webp"}) @@ -139,7 +155,11 @@ func (h *AvatarHandler) UploadAvatar(c *gin.Context) { return } avatarFilename := fmt.Sprintf("avatar_%d_%s%s", userID, token, ext) - uploadDir := "./uploads/avatars" + uploadDir, err := resolveAvatarUploadDir("") + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"code": 500, "message": "failed to resolve upload directory"}) + return + } // Create upload directory if not exists if err := os.MkdirAll(uploadDir, 0o755); err != nil { diff --git a/internal/api/handler/avatar_handler_path_test.go b/internal/api/handler/avatar_handler_path_test.go new file mode 100644 index 0000000..e6cc5e2 --- /dev/null +++ b/internal/api/handler/avatar_handler_path_test.go @@ -0,0 +1,33 @@ +package handler + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestResolveAvatarUploadDir_DefaultRootBecomesAbsolute(t *testing.T) { + dir, err := resolveAvatarUploadDir("") + if err != nil { + t.Fatalf("resolveAvatarUploadDir() error = %v", err) + } + if !filepath.IsAbs(dir) { + t.Fatalf("resolveAvatarUploadDir() = %q, want absolute path", dir) + } + if !strings.HasSuffix(filepath.ToSlash(dir), "/uploads/avatars") { + t.Fatalf("resolveAvatarUploadDir() = %q, want suffix /uploads/avatars", dir) + } +} + +func TestResolveAvatarUploadDir_CustomRootPreserved(t *testing.T) { + dir, err := resolveAvatarUploadDir("testdata/uploads-root") + if err != nil { + t.Fatalf("resolveAvatarUploadDir() error = %v", err) + } + if !filepath.IsAbs(dir) { + t.Fatalf("resolveAvatarUploadDir() = %q, want absolute path", dir) + } + if !strings.HasSuffix(filepath.ToSlash(dir), "/testdata/uploads-root/avatars") { + t.Fatalf("resolveAvatarUploadDir() = %q, want custom root suffix", dir) + } +}