fix: SECURITY_TODO #30 最后管理员防线事务化,消除 TOCTOU 竞态
UserUpdate/UserDelete 的 adminCount<=1 计数与 Save/Delete 非原子,两个 并发的'降级/删除倒数第二位管理员'请求可同时通过检查,站点失去管理员 (MySQL 下真实窗口;SQLite 单写锁下窗口极小)。 - handlers/admin_user.go: 检查+写入包进 db.Transaction; ensureNotLastAdmin 在事务内锁定管理员集合后再计数: MySQL 用 SELECT ... FOR UPDATE(gorm clause.Locking)串行化 并发事务;SQLite 无 FOR UPDATE,叠加进程内互斥锁 lastAdminMu (应用按设计单实例部署),计数在写锁串行化后重读 - 顺带修正:原实现中 f.Role 覆盖 user.Role 后检查恒为 true 的语义 保留(基于原角色判定),避免降级检查被新值掩盖 - 测试: TestConcurrentLastAdminDowngrade(两位管理员并发降级: 恰好 1 成功 1 拒绝、最终至少保留一位管理员;-race 通过)
This commit is contained in:
+4
-3
@@ -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 复选框)
|
||||
|
||||
+70
-17
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user