diff --git a/SECURITY_TODO.md b/SECURITY_TODO.md index 55e95e4..4d82219 100644 --- a/SECURITY_TODO.md +++ b/SECURITY_TODO.md @@ -201,12 +201,13 @@ - [x] saveSiteImage 读取字节后调用 `contentMatchesType(check.Type, content)`,不匹配返回 400(`settings_upload_bad_content`,i18n 中英新增;与头像上传口径一致) - **验证**: ✅ `TestSiteImageUploadRejectsMismatchedContent`(favicon/logo 各:PNG 扩展名 + HTML 字节 → 400;正常 PNG → 200) -### [ ] 30. 最后管理员防线存在 TOCTOU 竞态(2026-08-27 API 化复审新发现) +### [x] 30. 最后管理员防线存在 TOCTOU 竞态(2026-08-27 API 化复审新发现)✅ 2026-08-27 - **位置**: `handlers/admin_user.go`(UserUpdate 降级检查、UserDelete 删除检查) - **问题**: `adminCount <= 1` 检查与后续 Save/Delete 非原子:两个并发的"降级/删除最后一位管理员"请求可同时通过检查,导致站点失去管理员。SQLite 单写锁下窗口极小;MySQL 部署是真实窗口(需管理员 CSRF 或双开标签配合,可利用性低)。 - **修复**: - - [ ] 检查+写入包进 `db.Transaction`,事务内先计数再更新(MySQL 下依赖行锁或 `SELECT ... FOR UPDATE`) -- **验证**: [ ] 测试:并发降级最后管理员的请求,最终至少保留一个 admin(`-race`) + - [x] 检查+写入包进 `db.Transaction`;事务内 `ensureNotLastAdmin` 先锁定管理员集合(MySQL:`SELECT ... FOR UPDATE`,GORM `clause.Locking`)再计数,并发事务串行化后重读 + - [x] SQLite 无 FOR UPDATE(且纯 Go 驱动连接池可并发读):叠加进程内互斥锁 `lastAdminMu`(应用按设计单实例部署,见 #10 限流器注释)闭合同进程竞态;事务内计数在写锁串行化后重读 +- **验证**: ✅ `TestConcurrentLastAdminDowngrade`(两位管理员并发降级:恰好 1 成功 1 拒绝,最终管理员 ≥1,`-race` 通过) ### [ ] 31. 普通作者可置顶全站文章——需确认设计意图(2026-08-27 API 化复审新发现) - **位置**: `handlers/article.go`(ArticleCreate/ArticleUpdate 由 /api/my/articles 复用)、`templates/user/my_article_form.html`(is_top 复选框) diff --git a/handlers/admin_user.go b/handlers/admin_user.go index bfa5ab4..4ff3371 100644 --- a/handlers/admin_user.go +++ b/handlers/admin_user.go @@ -1,16 +1,19 @@ package handlers import ( + "errors" "fmt" "net/http" "strconv" "strings" + "sync" "time" "github.com/gin-gonic/gin" "go_blog/models" "gorm.io/gorm" + "gorm.io/gorm/clause" ) // userForm 是后台用户创建/更新接口的 JSON 请求体, @@ -69,6 +72,35 @@ func uintFormID(s string) uint { return uint(n) } +// errLastAdminRemoval 与 lastAdminMu 实现"最后管理员"防线 +// (SECURITY_TODO #30)。原实现先计数再写入,两者之间无原子性: +// 两个并发的"降级/删除倒数第二位管理员"请求可同时通过检查, +// 导致站点失去管理员。修复后检查与写入包进同一事务: +// - MySQL:事务内 SELECT ... FOR UPDATE 锁定管理员集合, +// 并发事务在锁上排队,先提交者生效,后到者重读计数后拒绝; +// - SQLite:不支持 FOR UPDATE,但其写锁串行化 + 本进程互斥锁 +// (应用按设计单实例部署,见 LoginRateLimiter 注释)双保险。 +var errLastAdminRemoval = errors.New("cannot remove the last admin") + +var lastAdminMu sync.Mutex + +// ensureNotLastAdmin 在事务内读取管理员数量(MySQL 下为锁定读取)。 +// 仅剩 1 位(或 0 位)管理员时返回 errLastAdminRemoval。 +func ensureNotLastAdmin(tx *gorm.DB) error { + q := tx.Model(&models.User{}).Where("role = ?", models.RoleAdmin) + if tx.Dialector.Name() != "sqlite" { + q = q.Clauses(clause.Locking{Strength: "UPDATE"}) + } + var admins []models.User + if err := q.Find(&admins).Error; err != nil { + return err + } + if len(admins) <= 1 { + return errLastAdminRemoval + } + return nil +} + // applyUserFormToData 将表单值写入模板数据映射, // 使渲染时表单被重新填充(初次加载或校验错误)。 func applyUserFormToData(data gin.H, f userForm) { @@ -320,21 +352,15 @@ func UserUpdate(db *gorm.DB) gin.HandlerFunc { currentID := userIDFromSession(c) isSelf := user.ID == currentID + // 最后管理员检查必须在 user.Role 被新值覆盖之前基于原值判定。 + wasAdmin := user.Role == models.RoleAdmin + // 自我保护:不能禁用/锁定自己的账户。 if isSelf && f.Status != models.StatusNormal { APIError(c, http.StatusForbidden, "user_cannot_disable_self") return } - // 自我保护:不能降级最后一位管理员。 - if user.Role == models.RoleAdmin && f.Role != models.RoleAdmin { - var adminCount int64 - db.Model(&models.User{}).Where("role = ?", models.RoleAdmin).Count(&adminCount) - if adminCount <= 1 { - APIError(c, http.StatusForbidden, "user_cannot_remove_last_admin") - return - } - } if f.Role == "" { f.Role = user.Role } @@ -358,7 +384,24 @@ func UserUpdate(db *gorm.DB) gin.HandlerFunc { } } - if err := db.Save(&user).Error; err != nil { + // 自我保护:不能降级最后一位管理员(SECURITY_TODO #30: + // 检查与写入在事务内原子执行,消除 TOCTOU 竞态)。 + lastAdminCheck := wasAdmin && f.Role != models.RoleAdmin + lastAdminMu.Lock() + err := db.Transaction(func(tx *gorm.DB) error { + if lastAdminCheck { + if err := ensureNotLastAdmin(tx); err != nil { + return err + } + } + return tx.Save(&user).Error + }) + lastAdminMu.Unlock() + if err != nil { + if errors.Is(err, errLastAdminRemoval) { + APIError(c, http.StatusForbidden, "user_cannot_remove_last_admin") + return + } APIError(c, http.StatusInternalServerError, "article_error") return } @@ -388,17 +431,27 @@ func UserDelete(db *gorm.DB) gin.HandlerFunc { APIError(c, http.StatusForbidden, "user_cannot_disable_self") return } - // 不能删除最后一位管理员。 - if user.Role == models.RoleAdmin { - var adminCount int64 - db.Model(&models.User{}).Where("role = ?", models.RoleAdmin).Count(&adminCount) - if adminCount <= 1 { + // 不能删除最后一位管理员(SECURITY_TODO #30: + // 检查与写入在事务内原子执行,消除 TOCTOU 竞态)。 + lastAdminCheck := user.Role == models.RoleAdmin + lastAdminMu.Lock() + err := db.Transaction(func(tx *gorm.DB) error { + if lastAdminCheck { + if err := ensureNotLastAdmin(tx); err != nil { + return err + } + } + return tx.Delete(&user).Error + }) + lastAdminMu.Unlock() + if err != nil { + if errors.Is(err, errLastAdminRemoval) { APIError(c, http.StatusForbidden, "user_cannot_remove_last_admin") return } + APIError(c, http.StatusInternalServerError, "article_error") + return } - - db.Delete(&user) APIOK(c, "/admin/users?saved=1&msg=deleted", nil) } } diff --git a/handlers/last_admin_test.go b/handlers/last_admin_test.go new file mode 100644 index 0000000..62f75d7 --- /dev/null +++ b/handlers/last_admin_test.go @@ -0,0 +1,82 @@ +package handlers + +import ( + "fmt" + "net/http" + "sync" + "testing" + + "github.com/gin-gonic/gin" + + "go_blog/models" +) + +// TestConcurrentLastAdminDowngrade 覆盖 SECURITY_TODO #30:两个并发的 +// "降级倒数第二位管理员"请求不再能同时通过检查——最终恰好一位管理员 +// 被降级、另一位被拒绝,且站点至少保留一位管理员。旧实现(计数与写入 +// 非原子)下两个请求都会成功,管理员清零(变异测试可验证)。 +func TestConcurrentLastAdminDowngrade(t *testing.T) { + e := newSecurityTestEnv(t) + + // 增加第二位管理员(admin2);alice/bob 保持 author。 + admin2 := mustUser(t, e.db, "admin2", models.RoleAdmin) + var admin1 models.User + if err := e.db.Where("username = ?", "admin").First(&admin1).Error; err != nil { + t.Fatalf("load admin1: %v", err) + } + + // 两位管理员分别用自己的会话并发发起"降级自己"的请求。 + session1 := e.login(t, "admin") + token1 := e.csrfTokenFor(t, session1) + session2 := e.login(t, "admin2") + token2 := e.csrfTokenFor(t, session2) + + start := make(chan struct{}) + var wg sync.WaitGroup + codes := make(chan int, 2) + requests := []struct { + id uint + cookie string + token string + }{ + {admin1.ID, session1, token1}, + {admin2.ID, session2, token2}, + } + for _, req := range requests { + wg.Add(1) + go func(id uint, cookie, token string) { + defer wg.Done() + <-start + w := postJSON(e, http.MethodPut, fmt.Sprintf("/api/admin/users/%d", id), cookie, token, + gin.H{"role": models.RoleAuthor, "status": models.StatusNormal}) + codes <- w.Code + }(req.id, req.cookie, req.token) + } + close(start) + wg.Wait() + close(codes) + + var success, rejected int + for code := range codes { + switch code { + case http.StatusOK: + success++ + case http.StatusForbidden: + rejected++ + default: + t.Fatalf("unexpected status %d (body-level check skipped; want 200 or 403)", code) + } + } + if success != 1 || rejected != 1 { + t.Fatalf("concurrent demotions: success=%d rejected=%d, want exactly 1/1", success, rejected) + } + + // 最终至少保留一位管理员。 + var count int64 + if err := e.db.Model(&models.User{}).Where("role = ?", models.RoleAdmin).Count(&count).Error; err != nil { + t.Fatalf("count admins: %v", err) + } + if count < 1 { + t.Fatal("no admin remains after concurrent demotions") + } +}