fix: 修复 P1 复审漏洞 #20 #21(禁用用户会话失效 + 头像 XSS 链)
- #20 AuthRequired 改为 AuthRequired(db):受保护路由每次回库校验 Status == StatusNormal 且未软删,失败清 session(保留 lang/csrf_token, 与登录轮换口径一致)并 302 /login;session user_id 先断言为数值再入 GORM(呼应 #19),AdminRequired 同步加固;SetUserContext 仅在用户 存在且状态正常时置 is_logged_in——禁用用户发评论不再自动通过, 回落游客审核策略 - #21 头像两个分支(UploadAvatar / UpdateProfile)强制 Category == image,解码失败直接拒绝、删除"回退存原始字节"路径, 统一经 processAvatar 解码→256px 缩放→JPEG 重编码;addUploadFileType 增加危险扩展黑名单(.html/.htm/.xhtml/.xht/.svg/.xml/.js/.mjs), 拒绝添加并在上传设置页提示(模板 + 中英 i18n) - 附带修复:processAvatar 依赖的 png/gif 解码器此前未注册(旧代码靠 回退存原始字节掩盖,PNG 头像从未真正处理过),补 blank import - 新增回归测试 session_upload_security_test.go(6 用例:禁用/锁定/ 软删旧 cookie 302、锁定用户评论转 pending、6 组危险扩展拒绝、 伪装扩展名头像拒绝且磁盘零写入、正常图片转存 .jpg;已变异验证: 去掉任一修复对应测试即失败) - SECURITY_TODO.md 勾选 #20/#21 并更新执行顺序
This commit is contained in:
+15
-15
@@ -95,21 +95,23 @@
|
||||
- [x] goroutine 启动前同步提取 userID / ip / UA 为局部变量,`recordArticleView` 不再触碰 gin.Context 与 session
|
||||
- **验证**: ✅ `go test -race ./...` 全绿
|
||||
|
||||
### [ ] 20. 被禁用/锁定/删除用户的会话不失效(2026-08-27 复审新发现)
|
||||
### [x] 20. 被禁用/锁定/删除用户的会话不失效(2026-08-27 复审新发现)✅ 2026-08-27
|
||||
- **位置**: `middleware/auth.go:16-27`(AuthRequired 只看 session 是否有 user_id,不回库校验)
|
||||
- **问题**: 登录时的状态检查(`handlers/auth.go:50`)只在登录瞬间生效。管理员禁用/锁定/软删用户后,其已持有的 cookie 在最长 24h 内仍完全可用:发评论自动 Approved、写文章、传附件;`SetUserContext` 对已软删用户仍置 `is_logged_in=true`。
|
||||
- **修复**:
|
||||
- [ ] AuthRequired 回库校验 `Status == StatusNormal` 且未软删,失败则清 session 并跳转 /login
|
||||
- [ ] SetUserContext:用户查询失败时 `is_logged_in` 置 false
|
||||
- **验证**: 禁用用户后用旧 cookie 访问 `/my/articles` → 302 `/login`;其新评论不再自动通过
|
||||
- [x] `AuthRequired(db)` 回库校验 `Status == StatusNormal` 且未软删,失败则清 session(保留 lang 与 csrf_token)并跳转 /login;session user_id 一律先断言为数值再入 GORM(呼应 #19)
|
||||
- [x] SetUserContext:用户查询失败或非正常状态时 `is_logged_in` 置 false(评论自动通过随之失效,回落游客审核策略)
|
||||
- **验证**: ✅ `TestDisabledUserSessionInvalidated`(disabled/locked 旧 cookie → 302 /login)、`TestSoftDeletedUserSessionInvalidated`、`TestDisabledUserCommentsRequireApproval`(锁定后评论转 pending)
|
||||
|
||||
### [ ] 21. 头像上传缺类别校验 + 可添加任意扩展名 → 存储型 XSS 链(2026-08-27 复审新发现)
|
||||
### [x] 21. 头像上传缺类别校验 + 可添加任意扩展名 → 存储型 XSS 链(2026-08-27 复审新发现)✅ 2026-08-27
|
||||
- **位置**: `handlers/profile.go:196-208`(UploadAvatar 未限制 image 类别)、`handlers/profile.go:223-226`(processAvatar 失败回退存原始字节)、`handlers/profile.go:96-107`(UpdateProfile 头像分支同样无类别校验、原样落盘)、`handlers/settings.go:269-290`(addUploadFileType 无危险扩展黑名单)
|
||||
- **问题**: logo/favicon 上传要求 `Category == image`(settings.go:113/153),但头像上传只查扩展名白名单且解码失败仍存原始文件;管理员又可在上传设置里添加任意扩展名(含 `.html`/`.svg`)。组合链:添加 `.html` 类型 → 任意登录用户以头像名义上传 HTML → 落在 `/uploads/avatars/` 同源可执行(CSP `script-src 'self' 'unsafe-inline'` 放行)。
|
||||
- **修复**:
|
||||
- [ ] UploadAvatar / UpdateProfile 头像分支强制 `check.Type.Category == models.CategoryImage`,解码失败直接拒绝(不回退存原始字节)
|
||||
- [ ] addUploadFileType 增加危险扩展黑名单(.html/.htm/.svg/.xhtml/.xml 等),拒绝添加
|
||||
- **验证**: 上传 `.html` 头像 → 拒绝;后台添加 `.html` 类型 → 拒绝;正常图片仍成功
|
||||
- [x] UploadAvatar / UpdateProfile 头像分支强制 `check.Type.Category == models.CategoryImage`,解码失败直接拒绝(不再回退存原始字节)
|
||||
- [x] UpdateProfile 头像同样经 processAvatar 解码→缩放→JPEG 重编码,原始字节不再落盘
|
||||
- [x] addUploadFileType 增加危险扩展黑名单(.html/.htm/.xhtml/.xht/.svg/.xml/.js/.mjs),拒绝添加并提示(settings_upload 页新增错误提示 + i18n)
|
||||
- [x] 附带修复:processAvatar 依赖的 png/gif 解码器此前未注册(旧代码靠"失败回退"掩盖),补 blank import
|
||||
- **验证**: ✅ `TestAddUploadFileTypeRejectsDangerousExtensions`(6 组危险扩展拒绝 + .md 正常)、`TestUploadAvatarRejectsNonImage`(.html 拒绝 / 图片扩展名包 HTML 拒绝 / 正常 PNG 转存 .jpg)、`TestUpdateProfileAvatarRejectsNonImage`(表单头像同样拒绝 + 正常图片成功)
|
||||
|
||||
---
|
||||
|
||||
@@ -211,11 +213,9 @@
|
||||
|
||||
## 建议执行顺序
|
||||
|
||||
P0/P1 原有 8 项及 P0 新发现 #18/#19 均已完成。剩余:
|
||||
P0/P1 原有 8 项及 P0/P1 新发现 #18–#21 均已完成。剩余:
|
||||
|
||||
1. **#20**(会话失效校验,middleware 单点改动)
|
||||
2. **#21**(头像/XSS 链:类别校验 + 扩展名黑名单)
|
||||
3. **#22 → #23 → #24**(校验类小改动,可合并一个 PR)
|
||||
4. **#9 → #10 → #25**(CDN 本地化、登录限速 + 计时抹平,同一主题)
|
||||
5. 其余 P2/P3(#11 配置权限、#12 首启弱凭据、#13 socket 权限、#14 magic bytes、#17 bcrypt cost)按迭代排入
|
||||
6. (可选)#18 方案 A:数据库文件移出存储根
|
||||
1. **#22 → #23 → #24**(校验类小改动,可合并一个 PR)
|
||||
2. **#9 → #10 → #25**(CDN 本地化、登录限速 + 计时抹平,同一主题)
|
||||
3. 其余 P2/P3(#11 配置权限、#12 首启弱凭据、#13 socket 权限、#14 magic bytes、#17 bcrypt cost)按迭代排入
|
||||
4. (可选)#18 方案 A:数据库文件移出存储根
|
||||
+36
-6
@@ -19,6 +19,12 @@ import (
|
||||
xdraw "golang.org/x/image/draw"
|
||||
_ "golang.org/x/image/webp"
|
||||
|
||||
// Register the decoders processAvatar relies on. JPEG is registered by
|
||||
// the image/jpeg import above; png/gif must be blank-imported or
|
||||
// image.Decode would reject them.
|
||||
_ "image/gif"
|
||||
_ "image/png"
|
||||
|
||||
"go_blog/models"
|
||||
)
|
||||
|
||||
@@ -94,7 +100,7 @@ func UpdateProfile(db *gorm.DB, storagePath string) gin.HandlerFunc {
|
||||
|
||||
// Validate against the platform upload policy (switch + type + size).
|
||||
check := ValidateUpload(header)
|
||||
if !check.OK {
|
||||
if !check.OK || check.Type.Category != models.CategoryImage {
|
||||
session.Save()
|
||||
reason := "?error=upload"
|
||||
if !models.GetUploadConfig().Enabled {
|
||||
@@ -109,6 +115,21 @@ func UpdateProfile(db *gorm.DB, storagePath string) gin.HandlerFunc {
|
||||
// Determine file extension (validated to be in the whitelist).
|
||||
ext := strings.ToLower(filepath.Ext(header.Filename))
|
||||
|
||||
// SECURITY (#21): decode and re-encode the avatar as a normalized
|
||||
// JPEG instead of storing the original bytes — undecodable payloads
|
||||
// (e.g. HTML disguised behind an image extension) are rejected.
|
||||
imgBytes, err := io.ReadAll(file)
|
||||
if err != nil {
|
||||
c.Redirect(http.StatusFound, "/profile?error=upload")
|
||||
return
|
||||
}
|
||||
processedBytes, finalExt, err := processAvatar(imgBytes, ext)
|
||||
if err != nil {
|
||||
session.Save()
|
||||
c.Redirect(http.StatusFound, "/profile?error=upload")
|
||||
return
|
||||
}
|
||||
|
||||
// Save under storagePath/avatars/.
|
||||
avatarDir := filepath.Join(storagePath, "avatars")
|
||||
if err := os.MkdirAll(avatarDir, 0755); err != nil {
|
||||
@@ -123,7 +144,7 @@ func UpdateProfile(db *gorm.DB, storagePath string) gin.HandlerFunc {
|
||||
}
|
||||
|
||||
// Use user ID as filename base.
|
||||
savedName := fmt.Sprintf("%d%s", user.ID, ext)
|
||||
savedName := fmt.Sprintf("%d%s", user.ID, finalExt)
|
||||
savedPath := filepath.Join(avatarDir, savedName)
|
||||
|
||||
dst, err := os.Create(savedPath)
|
||||
@@ -133,7 +154,7 @@ func UpdateProfile(db *gorm.DB, storagePath string) gin.HandlerFunc {
|
||||
}
|
||||
defer dst.Close()
|
||||
|
||||
if _, err := io.Copy(dst, file); err != nil {
|
||||
if _, err := dst.Write(processedBytes); err != nil {
|
||||
c.Redirect(http.StatusFound, "/profile")
|
||||
return
|
||||
}
|
||||
@@ -207,6 +228,14 @@ func UploadAvatar(db *gorm.DB, storagePath string) gin.HandlerFunc {
|
||||
return
|
||||
}
|
||||
|
||||
// SECURITY (#21): avatars must be an image-category type from the
|
||||
// whitelist — the extension whitelist alone is admin-configurable and
|
||||
// could otherwise admit active content into /uploads/avatars/.
|
||||
if check.Type.Category != models.CategoryImage {
|
||||
c.JSON(http.StatusBadRequest, gin.H{"error": "file type not allowed"})
|
||||
return
|
||||
}
|
||||
|
||||
// Determine file extension (validated to be in the whitelist).
|
||||
ext := strings.ToLower(filepath.Ext(header.Filename))
|
||||
|
||||
@@ -220,9 +249,10 @@ func UploadAvatar(db *gorm.DB, storagePath string) gin.HandlerFunc {
|
||||
// Decode, resize, and re-encode the image.
|
||||
processedBytes, finalExt, err := processAvatar(imgBytes, ext)
|
||||
if err != nil {
|
||||
// Fall back to saving raw bytes if processing fails.
|
||||
processedBytes = imgBytes
|
||||
finalExt = ext
|
||||
// SECURITY (#21): reject undecodable payloads outright — storing
|
||||
// the raw bytes would let non-image content land in avatars/.
|
||||
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid image file"})
|
||||
return
|
||||
}
|
||||
|
||||
// Ensure avatar directory exists.
|
||||
|
||||
@@ -43,7 +43,7 @@ func newSecurityTestEnv(t *testing.T) *securityTestEnv {
|
||||
}
|
||||
if err := db.AutoMigrate(&models.User{}, &models.Article{}, &models.Attachment{}, &models.SiteSetting{},
|
||||
&models.UploadConfig{}, &models.UploadFileType{}, &models.CommentConfig{}, &models.NavLink{},
|
||||
&models.DownloadBaseURL{}); err != nil {
|
||||
&models.DownloadBaseURL{}, &models.Comment{}); err != nil {
|
||||
t.Fatalf("migrate: %v", err)
|
||||
}
|
||||
|
||||
@@ -80,8 +80,9 @@ func newSecurityTestEnv(t *testing.T) *securityTestEnv {
|
||||
r.GET("/login", LoginPage())
|
||||
r.POST("/login", Login(db))
|
||||
r.POST("/logout", Logout())
|
||||
r.POST("/article/:slug/comments", PostComment(db))
|
||||
|
||||
protected := r.Group("/my", middleware.AuthRequired())
|
||||
protected := r.Group("/my", middleware.AuthRequired(db))
|
||||
{
|
||||
protected.POST("/articles/attachments", UploadAttachment(db, storageDir))
|
||||
protected.POST("/articles/attachments/:id/delete", DeleteAttachment(db, storageDir))
|
||||
@@ -92,8 +93,21 @@ func newSecurityTestEnv(t *testing.T) *securityTestEnv {
|
||||
})
|
||||
}
|
||||
|
||||
// Profile routes (avatar upload XSS-chain regression coverage, #21).
|
||||
profile := r.Group("/profile", middleware.AuthRequired(db))
|
||||
{
|
||||
profile.POST("", UpdateProfile(db, storageDir))
|
||||
profile.POST("/avatar", UploadAvatar(db, storageDir))
|
||||
}
|
||||
|
||||
// Upload settings routes (dangerous-extension blacklist coverage, #21).
|
||||
adminSettings := r.Group("/admin/settings", middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
adminSettings.POST("/upload", UploadSettingsSave(db))
|
||||
}
|
||||
|
||||
// Admin user-management routes (SQL-injection regression coverage, #19).
|
||||
admin := r.Group("/admin", middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
admin := r.Group("/admin", middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
admin.GET("/users/:id/edit", UserEditPage(db))
|
||||
admin.POST("/users/:id/edit", UserUpdate(db))
|
||||
|
||||
@@ -0,0 +1,295 @@
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"fmt"
|
||||
"image"
|
||||
"image/color"
|
||||
"image/png"
|
||||
"mime/multipart"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"go_blog/models"
|
||||
)
|
||||
|
||||
// seedUploadType inserts an upload file-type row directly in the DB and
|
||||
// reloads the config cache, simulating policy rows created before a fix.
|
||||
func seedUploadType(t *testing.T, e *securityTestEnv, ext, category string) {
|
||||
t.Helper()
|
||||
if err := e.db.Create(&models.UploadFileType{
|
||||
Extension: ext, MimeType: "application/octet-stream", Category: category, Enabled: true,
|
||||
}).Error; err != nil {
|
||||
t.Fatalf("seed upload type %s: %v", ext, err)
|
||||
}
|
||||
models.LoadConfigCache(e.db)
|
||||
}
|
||||
|
||||
// pngBytes renders a small valid PNG.
|
||||
func pngBytes(t *testing.T) []byte {
|
||||
t.Helper()
|
||||
img := image.NewRGBA(image.Rect(0, 0, 8, 8))
|
||||
for x := 0; x < 8; x++ {
|
||||
for y := 0; y < 8; y++ {
|
||||
img.Set(x, y, color.RGBA{R: 0x33, G: 0x66, B: 0x99, A: 0xff})
|
||||
}
|
||||
}
|
||||
var buf bytes.Buffer
|
||||
if err := png.Encode(&buf, img); err != nil {
|
||||
t.Fatalf("encode png: %v", err)
|
||||
}
|
||||
return buf.Bytes()
|
||||
}
|
||||
|
||||
// multipartUpload posts a multipart form carrying one file field.
|
||||
func (e *securityTestEnv) multipartUpload(t *testing.T, path, cookie, csrfToken, fieldName, filename string, content []byte, fields map[string]string) *httptest.ResponseRecorder {
|
||||
t.Helper()
|
||||
var buf bytes.Buffer
|
||||
mw := multipart.NewWriter(&buf)
|
||||
mw.WriteField("_csrf", csrfToken)
|
||||
for k, v := range fields {
|
||||
mw.WriteField(k, v)
|
||||
}
|
||||
fw, _ := mw.CreateFormFile(fieldName, filename)
|
||||
fw.Write(content)
|
||||
mw.Close()
|
||||
return e.do(http.MethodPost, path, cookie, strings.NewReader(buf.String()), mw.FormDataContentType())
|
||||
}
|
||||
|
||||
func reloadAlice(t *testing.T, e *securityTestEnv) models.User {
|
||||
t.Helper()
|
||||
var alice models.User
|
||||
if err := e.db.Where("username = ?", "alice").First(&alice).Error; err != nil {
|
||||
t.Fatalf("load alice: %v", err)
|
||||
}
|
||||
return alice
|
||||
}
|
||||
|
||||
// --- #20: stale sessions of disabled / locked / deleted users ---
|
||||
|
||||
func TestDisabledUserSessionInvalidated(t *testing.T) {
|
||||
e := newSecurityTestEnv(t)
|
||||
aliceCookie := e.login(t, "alice")
|
||||
|
||||
// Sanity: the session works while the account is normal.
|
||||
if w := e.do(http.MethodGet, "/my/whoami", aliceCookie, nil, ""); w.Code != http.StatusOK {
|
||||
t.Fatalf("pre-disable /my/whoami: status=%d", w.Code)
|
||||
}
|
||||
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
status int
|
||||
}{
|
||||
{"disabled", models.StatusDisabled},
|
||||
{"locked", models.StatusLocked},
|
||||
} {
|
||||
e.db.Model(&models.User{}).Where("username = ?", "alice").Update("status", tc.status)
|
||||
w := e.do(http.MethodGet, "/my/whoami", aliceCookie, nil, "")
|
||||
if w.Code != http.StatusFound || w.Header().Get("Location") != "/login" {
|
||||
t.Fatalf("%s user with stale cookie: status=%d location=%q, want 302 /login",
|
||||
tc.name, w.Code, w.Header().Get("Location"))
|
||||
}
|
||||
// Restore so the next case starts from a normal account again.
|
||||
e.db.Model(&models.User{}).Where("username = ?", "alice").Update("status", models.StatusNormal)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSoftDeletedUserSessionInvalidated(t *testing.T) {
|
||||
e := newSecurityTestEnv(t)
|
||||
aliceCookie := e.login(t, "alice")
|
||||
|
||||
e.db.Where("username = ?", "alice").Delete(&models.User{})
|
||||
w := e.do(http.MethodGet, "/my/whoami", aliceCookie, nil, "")
|
||||
if w.Code != http.StatusFound || w.Header().Get("Location") != "/login" {
|
||||
t.Fatalf("soft-deleted user with stale cookie: status=%d location=%q, want 302 /login",
|
||||
w.Code, w.Header().Get("Location"))
|
||||
}
|
||||
}
|
||||
|
||||
func TestDisabledUserCommentsRequireApproval(t *testing.T) {
|
||||
e := newSecurityTestEnv(t)
|
||||
aliceCookie := e.login(t, "alice")
|
||||
token := e.csrfTokenFor(t, aliceCookie)
|
||||
|
||||
// Guests must pass moderation for this scenario.
|
||||
e.db.Model(&models.CommentConfig{}).Where("id = ?", 1).Update("guest_require_approval", true)
|
||||
models.LoadConfigCache(e.db)
|
||||
|
||||
postComment := func() models.Comment {
|
||||
t.Helper()
|
||||
form := url.Values{}
|
||||
form.Set("name", "alice")
|
||||
form.Set("email", "alice@example.com")
|
||||
form.Set("content", "comment body")
|
||||
form.Set("_csrf", token)
|
||||
w := e.do(http.MethodPost, "/article/alice-post/comments", aliceCookie,
|
||||
strings.NewReader(form.Encode()), "application/x-www-form-urlencoded")
|
||||
if w.Code != http.StatusFound {
|
||||
t.Fatalf("POST comment: status=%d body=%s", w.Code, w.Body.String())
|
||||
}
|
||||
var cm models.Comment
|
||||
if err := e.db.Last(&cm).Error; err != nil {
|
||||
t.Fatalf("load comment: %v", err)
|
||||
}
|
||||
return cm
|
||||
}
|
||||
|
||||
// Control: while alice is a normal user her comment is auto-approved.
|
||||
if cm := postComment(); cm.Status != models.CommentApproved {
|
||||
t.Fatalf("normal user comment status=%d, want approved", cm.Status)
|
||||
}
|
||||
|
||||
// After being locked, her stale session no longer grants auto-approval:
|
||||
// SetUserContext reports her as logged out, so the comment follows the
|
||||
// guest moderation policy.
|
||||
e.db.Model(&models.User{}).Where("username = ?", "alice").Update("status", models.StatusLocked)
|
||||
if cm := postComment(); cm.Status != models.CommentPending {
|
||||
t.Fatalf("locked user comment status=%d, want pending", cm.Status)
|
||||
}
|
||||
}
|
||||
|
||||
// --- #21: avatar upload XSS chain ---
|
||||
|
||||
func TestAddUploadFileTypeRejectsDangerousExtensions(t *testing.T) {
|
||||
e := newSecurityTestEnv(t)
|
||||
admin := e.login(t, "admin")
|
||||
token := e.csrfTokenFor(t, admin)
|
||||
|
||||
for _, ext := range []string{"html", ".htm", "SVG", "xhtml", ".xml", "js"} {
|
||||
form := url.Values{}
|
||||
form.Set("_csrf", token)
|
||||
form.Set("action", "add_type")
|
||||
form.Set("extension", ext)
|
||||
form.Set("category", models.CategoryImage)
|
||||
w := e.do(http.MethodPost, "/admin/settings/upload", admin,
|
||||
strings.NewReader(form.Encode()), "application/x-www-form-urlencoded")
|
||||
if w.Code != http.StatusFound || !strings.Contains(w.Header().Get("Location"), "error=dangerous_ext") {
|
||||
t.Fatalf("add type %q: status=%d location=%q, want 302 with error=dangerous_ext",
|
||||
ext, w.Code, w.Header().Get("Location"))
|
||||
}
|
||||
var count int64
|
||||
normalized := strings.ToLower(ext)
|
||||
if !strings.HasPrefix(normalized, ".") {
|
||||
normalized = "." + normalized
|
||||
}
|
||||
e.db.Model(&models.UploadFileType{}).Where("extension = ?", normalized).Count(&count)
|
||||
if count != 0 {
|
||||
t.Fatalf("dangerous extension %q was persisted", ext)
|
||||
}
|
||||
}
|
||||
|
||||
// Control: a benign extension is still accepted.
|
||||
form := url.Values{}
|
||||
form.Set("_csrf", token)
|
||||
form.Set("action", "add_type")
|
||||
form.Set("extension", "md")
|
||||
form.Set("category", models.CategoryDocument)
|
||||
w := e.do(http.MethodPost, "/admin/settings/upload", admin,
|
||||
strings.NewReader(form.Encode()), "application/x-www-form-urlencoded")
|
||||
if w.Code != http.StatusFound || !strings.Contains(w.Header().Get("Location"), "saved=1") {
|
||||
t.Fatalf("add benign type: status=%d location=%q", w.Code, w.Header().Get("Location"))
|
||||
}
|
||||
var count int64
|
||||
e.db.Model(&models.UploadFileType{}).Where("extension = ?", ".md").Count(&count)
|
||||
if count != 1 {
|
||||
t.Fatalf("benign extension .md not created (count=%d)", count)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUploadAvatarRejectsNonImage(t *testing.T) {
|
||||
e := newSecurityTestEnv(t)
|
||||
aliceCookie := e.login(t, "alice")
|
||||
token := e.csrfTokenFor(t, aliceCookie)
|
||||
|
||||
htmlPayload := []byte("<html><script>alert(document.cookie)</script></html>")
|
||||
|
||||
// Simulate a pre-existing dangerous type configured before the blacklist
|
||||
// (defense in depth): the category check must reject it.
|
||||
seedUploadType(t, e, ".html", models.CategoryOther)
|
||||
w := e.multipartUpload(t, "/profile/avatar", aliceCookie, token, "avatar", "evil.html", htmlPayload, nil)
|
||||
if w.Code != http.StatusBadRequest {
|
||||
t.Fatalf("upload .html (other category): status=%d body=%s, want 400", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
// Even a legacy .html row miscategorized as "image" is stopped by the
|
||||
// decode step — raw bytes are never stored anymore.
|
||||
seedUploadType(t, e, ".htm", models.CategoryImage)
|
||||
w = e.multipartUpload(t, "/profile/avatar", aliceCookie, token, "avatar", "evil.htm", htmlPayload, nil)
|
||||
if w.Code != http.StatusBadRequest {
|
||||
t.Fatalf("upload .htm (image category): status=%d body=%s, want 400", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
// HTML disguised behind a whitelisted image extension is likewise rejected.
|
||||
seedUploadType(t, e, ".jpg", models.CategoryImage)
|
||||
w = e.multipartUpload(t, "/profile/avatar", aliceCookie, token, "avatar", "x.jpg", htmlPayload, nil)
|
||||
if w.Code != http.StatusBadRequest {
|
||||
t.Fatalf("upload html as .jpg: status=%d body=%s, want 400", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
// Nothing was stored and the avatar is unchanged.
|
||||
if alice := reloadAlice(t, e); alice.Avatar != "" {
|
||||
t.Fatalf("avatar unexpectedly set to %q", alice.Avatar)
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(e.storageDir, "avatars")); !os.IsNotExist(err) {
|
||||
entries, _ := os.ReadDir(filepath.Join(e.storageDir, "avatars"))
|
||||
for _, en := range entries {
|
||||
t.Logf("avatars dir entry: %s", en.Name())
|
||||
}
|
||||
t.Fatal("avatar directory should not contain any file after rejected uploads")
|
||||
}
|
||||
|
||||
// A real image is accepted, processed to a normalized JPEG.
|
||||
seedUploadType(t, e, ".png", models.CategoryImage)
|
||||
w = e.multipartUpload(t, "/profile/avatar", aliceCookie, token, "avatar", "me.png", pngBytes(t), nil)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("upload valid png: status=%d body=%s", w.Code, w.Body.String())
|
||||
}
|
||||
alice := reloadAlice(t, e)
|
||||
if want := fmt.Sprintf("%d.jpg", alice.ID); alice.Avatar != want {
|
||||
t.Fatalf("avatar = %q, want %q", alice.Avatar, want)
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(e.storageDir, "avatars", alice.Avatar)); err != nil {
|
||||
t.Fatalf("processed avatar file missing: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateProfileAvatarRejectsNonImage(t *testing.T) {
|
||||
e := newSecurityTestEnv(t)
|
||||
aliceCookie := e.login(t, "alice")
|
||||
token := e.csrfTokenFor(t, aliceCookie)
|
||||
|
||||
seedUploadType(t, e, ".png", models.CategoryImage)
|
||||
seedUploadType(t, e, ".html", models.CategoryImage) // legacy miscategorized row
|
||||
|
||||
// HTML behind a whitelisted extension must be rejected with the upload
|
||||
// error redirect, and nothing may be written to avatars/.
|
||||
htmlPayload := []byte("<html><script>alert(1)</script></html>")
|
||||
w := e.multipartUpload(t, "/profile", aliceCookie, token, "avatar", "evil.html", htmlPayload,
|
||||
map[string]string{"display_name": "alice"})
|
||||
if w.Code != http.StatusFound || !strings.Contains(w.Header().Get("Location"), "error=upload") {
|
||||
t.Fatalf("update profile with html avatar: status=%d location=%q, want 302 error=upload",
|
||||
w.Code, w.Header().Get("Location"))
|
||||
}
|
||||
if alice := reloadAlice(t, e); alice.Avatar != "" {
|
||||
t.Fatalf("avatar unexpectedly set to %q", alice.Avatar)
|
||||
}
|
||||
|
||||
// A real image goes through processing and is saved as JPEG.
|
||||
w = e.multipartUpload(t, "/profile", aliceCookie, token, "avatar", "me.png", pngBytes(t),
|
||||
map[string]string{"display_name": "alice"})
|
||||
if w.Code != http.StatusFound || !strings.Contains(w.Header().Get("Location"), "saved=1") {
|
||||
t.Fatalf("update profile with valid avatar: status=%d location=%q", w.Code, w.Header().Get("Location"))
|
||||
}
|
||||
alice := reloadAlice(t, e)
|
||||
if want := fmt.Sprintf("%d.jpg", alice.ID); alice.Avatar != want {
|
||||
t.Fatalf("avatar = %q, want %q", alice.Avatar, want)
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(e.storageDir, "avatars", alice.Avatar)); err != nil {
|
||||
t.Fatalf("processed avatar file missing: %v", err)
|
||||
}
|
||||
}
|
||||
+25
-4
@@ -225,6 +225,9 @@ func UploadSettingsPage(db *gorm.DB) gin.HandlerFunc {
|
||||
if msg := c.Query("saved"); msg == "1" {
|
||||
data["Success"] = tr["settings_saved"]
|
||||
}
|
||||
if msg := c.Query("error"); msg == "dangerous_ext" {
|
||||
data["Error"] = tr["settings_upload_dangerous_ext"]
|
||||
}
|
||||
c.HTML(http.StatusOK, "settings_upload", data)
|
||||
}
|
||||
}
|
||||
@@ -232,11 +235,14 @@ func UploadSettingsPage(db *gorm.DB) gin.HandlerFunc {
|
||||
// UploadSettingsSave dispatches upload-config and file-type actions.
|
||||
func UploadSettingsSave(db *gorm.DB) gin.HandlerFunc {
|
||||
return func(c *gin.Context) {
|
||||
redirect := "/admin/settings/upload?saved=1"
|
||||
switch c.PostForm("action") {
|
||||
case "save_config":
|
||||
saveUploadConfig(db, c)
|
||||
case "add_type":
|
||||
addUploadFileType(db, c)
|
||||
if addUploadFileType(db, c) {
|
||||
redirect = "/admin/settings/upload?error=dangerous_ext"
|
||||
}
|
||||
case "toggle_type":
|
||||
toggleUploadFileType(db, c)
|
||||
case "size_type":
|
||||
@@ -245,7 +251,7 @@ func UploadSettingsSave(db *gorm.DB) gin.HandlerFunc {
|
||||
deleteUploadFileType(db, c)
|
||||
}
|
||||
models.RefreshConfigCache(db)
|
||||
c.Redirect(http.StatusFound, "/admin/settings/upload?saved=1")
|
||||
c.Redirect(http.StatusFound, redirect)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -266,14 +272,28 @@ func saveUploadConfig(db *gorm.DB, c *gin.Context) {
|
||||
db.Save(&u)
|
||||
}
|
||||
|
||||
func addUploadFileType(db *gorm.DB, c *gin.Context) {
|
||||
// dangerousUploadExtensions are never accepted as upload file types: files of
|
||||
// these extensions would be served same-origin from /uploads and can execute
|
||||
// active content (HTML/SVG/JS) in the site's origin, giving any logged-in
|
||||
// uploader a stored-XSS primitive (SECURITY_TODO #21).
|
||||
var dangerousUploadExtensions = map[string]bool{
|
||||
".html": true, ".htm": true, ".xhtml": true, ".xht": true,
|
||||
".svg": true, ".xml": true, ".js": true, ".mjs": true,
|
||||
}
|
||||
|
||||
// addUploadFileType creates a new permitted file type. It reports whether the
|
||||
// extension was rejected as dangerous.
|
||||
func addUploadFileType(db *gorm.DB, c *gin.Context) bool {
|
||||
ext := strings.ToLower(strings.TrimSpace(c.PostForm("extension")))
|
||||
if ext == "" {
|
||||
return
|
||||
return false
|
||||
}
|
||||
if !strings.HasPrefix(ext, ".") {
|
||||
ext = "." + ext
|
||||
}
|
||||
if dangerousUploadExtensions[ext] {
|
||||
return true
|
||||
}
|
||||
t := models.UploadFileType{
|
||||
Extension: ext,
|
||||
MimeType: strings.TrimSpace(c.PostForm("mime_type")),
|
||||
@@ -287,6 +307,7 @@ func addUploadFileType(db *gorm.DB, c *gin.Context) {
|
||||
}
|
||||
// Ignore duplicate-extension errors silently.
|
||||
db.Where("extension = ?", t.Extension).FirstOrCreate(&t)
|
||||
return false
|
||||
}
|
||||
|
||||
func toggleUploadFileType(db *gorm.DB, c *gin.Context) {
|
||||
|
||||
@@ -254,6 +254,7 @@ var translations = map[Lang]map[string]string{
|
||||
"settings_save": "Save",
|
||||
"settings_upload_title": "Upload Settings",
|
||||
"settings_upload_desc": "Attachment upload policy and permitted file types.",
|
||||
"settings_upload_dangerous_ext": "This extension is not allowed: files of this type can execute active content in the site's origin.",
|
||||
"settings_uploads_enabled":"Enable attachments",
|
||||
"settings_default_size": "Default max size (MB)",
|
||||
"settings_storage_dir": "Storage sub-directory",
|
||||
@@ -672,6 +673,7 @@ var translations = map[Lang]map[string]string{
|
||||
"settings_save": "保存",
|
||||
"settings_upload_title": "上传设置",
|
||||
"settings_upload_desc": "附件上传策略与允许的文件类型。",
|
||||
"settings_upload_dangerous_ext": "不允许该扩展名:此类文件可在站点同源执行活动内容。",
|
||||
"settings_uploads_enabled":"启用附件上传",
|
||||
"settings_default_size": "默认最大大小(MB)",
|
||||
"settings_storage_dir": "存储子目录",
|
||||
|
||||
@@ -126,7 +126,7 @@ func main() {
|
||||
|
||||
// Protected admin routes (admin role only).
|
||||
admin := router.Group("/admin")
|
||||
admin.Use(middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
admin.Use(middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
admin.GET("", handlers.AdminDashboard(db))
|
||||
admin.GET("/articles", handlers.ArticleListPage(db))
|
||||
@@ -140,7 +140,7 @@ func main() {
|
||||
|
||||
// Protected admin comment management routes (admin role only).
|
||||
comments := router.Group("/admin/comments")
|
||||
comments.Use(middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
comments.Use(middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
comments.GET("", handlers.CommentListPage(db))
|
||||
comments.POST("/:id/approve", handlers.CommentApprove(db))
|
||||
@@ -150,7 +150,7 @@ func main() {
|
||||
|
||||
// Protected admin user-management routes (admin role only).
|
||||
users := router.Group("/admin/users")
|
||||
users.Use(middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
users.Use(middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
users.GET("", handlers.UserListPage(db))
|
||||
users.GET("/new", handlers.UserCreatePage(db))
|
||||
@@ -162,7 +162,7 @@ func main() {
|
||||
|
||||
// Protected article attachment routes (admin role only).
|
||||
attachments := router.Group("/admin/articles")
|
||||
attachments.Use(middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
attachments.Use(middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
attachments.POST("/attachments", handlers.UploadAttachment(db, cfg.Path))
|
||||
attachments.POST("/attachments/:id/delete", handlers.DeleteAttachment(db, cfg.Path))
|
||||
@@ -171,7 +171,7 @@ func main() {
|
||||
|
||||
// Protected admin settings routes (platform configuration).
|
||||
settings := router.Group("/admin/settings")
|
||||
settings.Use(middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
settings.Use(middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
settings.GET("/site", handlers.SiteSettingsPage(db))
|
||||
settings.POST("/site", handlers.SiteSettingsSave(db, cfg.Path))
|
||||
@@ -187,14 +187,14 @@ func main() {
|
||||
|
||||
// Protected admin analytics routes (reading statistics).
|
||||
analytics := router.Group("/admin/analytics")
|
||||
analytics.Use(middleware.AuthRequired(), middleware.AdminRequired(db))
|
||||
analytics.Use(middleware.AuthRequired(db), middleware.AdminRequired(db))
|
||||
{
|
||||
analytics.GET("/views", handlers.ViewAnalyticsPage(db))
|
||||
}
|
||||
|
||||
// Protected profile routes.
|
||||
profile := router.Group("/profile")
|
||||
profile.Use(middleware.AuthRequired())
|
||||
profile.Use(middleware.AuthRequired(db))
|
||||
{
|
||||
profile.GET("", handlers.ProfilePage(db))
|
||||
profile.POST("", handlers.UpdateProfile(db, cfg.Path))
|
||||
@@ -203,7 +203,7 @@ func main() {
|
||||
|
||||
// Protected user article management routes (for non-admin users).
|
||||
myArticles := router.Group("/my")
|
||||
myArticles.Use(middleware.AuthRequired())
|
||||
myArticles.Use(middleware.AuthRequired(db))
|
||||
{
|
||||
myArticles.GET("/articles", handlers.MyArticlesPage(db))
|
||||
myArticles.GET("/articles/new", handlers.MyArticleCreatePage(db))
|
||||
@@ -215,7 +215,7 @@ func main() {
|
||||
|
||||
// Protected article attachment routes for user articles.
|
||||
myAttachments := router.Group("/my/articles")
|
||||
myAttachments.Use(middleware.AuthRequired())
|
||||
myAttachments.Use(middleware.AuthRequired(db))
|
||||
{
|
||||
myAttachments.POST("/attachments", handlers.UploadAttachment(db, cfg.Path))
|
||||
myAttachments.POST("/attachments/:id/delete", handlers.DeleteAttachment(db, cfg.Path))
|
||||
|
||||
+67
-14
@@ -11,13 +11,63 @@ import (
|
||||
"go_blog/models"
|
||||
)
|
||||
|
||||
// AuthRequired is middleware that protects routes. If the user is not logged in,
|
||||
// they are redirected to /login.
|
||||
func AuthRequired() gin.HandlerFunc {
|
||||
// sessionUserID extracts the logged-in user's numeric ID from the session,
|
||||
// defending against int/uint/int64/float64 storage. ok=false if absent or of
|
||||
// an unexpected type.
|
||||
func sessionUserID(session sessions.Session) (uint, bool) {
|
||||
userID := session.Get("user_id")
|
||||
if userID == nil {
|
||||
return 0, false
|
||||
}
|
||||
switch v := userID.(type) {
|
||||
case uint:
|
||||
return v, true
|
||||
case int:
|
||||
return uint(v), true
|
||||
case int64:
|
||||
return uint(v), true
|
||||
case float64:
|
||||
return uint(v), true
|
||||
default:
|
||||
return 0, false
|
||||
}
|
||||
}
|
||||
|
||||
// clearUserSession drops the authentication state from a session, keeping only
|
||||
// the harmless UI preferences (language and CSRF token, mirroring the login
|
||||
// handler's rotation) so forms already rendered in other tabs stay valid.
|
||||
func clearUserSession(session sessions.Session) {
|
||||
lang, _ := session.Get("lang").(string)
|
||||
csrfTok, _ := session.Get(CSRFSessionKey).(string)
|
||||
session.Clear()
|
||||
if lang != "" {
|
||||
session.Set("lang", lang)
|
||||
}
|
||||
if csrfTok != "" {
|
||||
session.Set(CSRFSessionKey, csrfTok)
|
||||
}
|
||||
session.Save()
|
||||
}
|
||||
|
||||
// AuthRequired is middleware that protects routes. If the user is not logged
|
||||
// in, they are redirected to /login. The session user is also re-validated
|
||||
// against the database on every request: an account that has since been
|
||||
// disabled, locked or soft-deleted loses access immediately instead of when
|
||||
// its cookie expires (SECURITY_TODO #20).
|
||||
func AuthRequired(db *gorm.DB) gin.HandlerFunc {
|
||||
return func(c *gin.Context) {
|
||||
session := sessions.Default(c)
|
||||
userID := session.Get("user_id")
|
||||
if userID == nil {
|
||||
uid, ok := sessionUserID(session)
|
||||
if !ok {
|
||||
c.Redirect(http.StatusFound, "/login")
|
||||
c.Abort()
|
||||
return
|
||||
}
|
||||
var user models.User
|
||||
if err := db.First(&user, uid).Error; err != nil || user.Status != models.StatusNormal {
|
||||
// Account no longer usable — kill the session so the stale cookie
|
||||
// cannot be replayed.
|
||||
clearUserSession(session)
|
||||
c.Redirect(http.StatusFound, "/login")
|
||||
c.Abort()
|
||||
return
|
||||
@@ -27,19 +77,19 @@ func AuthRequired() gin.HandlerFunc {
|
||||
}
|
||||
|
||||
// AdminRequired is middleware that restricts a route to admin-role users. It
|
||||
// must run after AuthRequired (which guarantees a session user exists). Non-admin
|
||||
// users are redirected back to the admin dashboard.
|
||||
// must run after AuthRequired (which guarantees a live, normal-status session
|
||||
// user). Non-admin users are redirected back to the admin dashboard.
|
||||
func AdminRequired(db *gorm.DB) gin.HandlerFunc {
|
||||
return func(c *gin.Context) {
|
||||
session := sessions.Default(c)
|
||||
userID := session.Get("user_id")
|
||||
if userID == nil {
|
||||
uid, ok := sessionUserID(session)
|
||||
if !ok {
|
||||
c.Redirect(http.StatusFound, "/login")
|
||||
c.Abort()
|
||||
return
|
||||
}
|
||||
var user models.User
|
||||
if err := db.First(&user, userID).Error; err != nil || user.Role != models.RoleAdmin {
|
||||
if err := db.First(&user, uid).Error; err != nil || user.Role != models.RoleAdmin {
|
||||
c.Redirect(http.StatusFound, "/admin")
|
||||
c.Abort()
|
||||
return
|
||||
@@ -94,17 +144,20 @@ func SetUserContext(db *gorm.DB) gin.HandlerFunc {
|
||||
c.Set("switch_lang", switchLang)
|
||||
|
||||
// --- Auth state ---
|
||||
userID := session.Get("user_id")
|
||||
// The user is only considered logged in if the account still exists
|
||||
// and is in normal status: a disabled/locked/soft-deleted account must
|
||||
// not keep template-level privileges (e.g. comment auto-approval)
|
||||
// after its session was invalidated (SECURITY_TODO #20).
|
||||
isLoggedIn := false
|
||||
var username string
|
||||
var avatar string
|
||||
var displayName string
|
||||
var role string
|
||||
|
||||
if userID != nil {
|
||||
isLoggedIn = true
|
||||
if uid, ok := sessionUserID(session); ok {
|
||||
var user models.User
|
||||
if err := db.First(&user, userID).Error; err == nil {
|
||||
if err := db.First(&user, uid).Error; err == nil && user.Status == models.StatusNormal {
|
||||
isLoggedIn = true
|
||||
username = user.Username
|
||||
avatar = user.Avatar
|
||||
displayName = user.DisplayName
|
||||
|
||||
@@ -14,6 +14,9 @@
|
||||
{{if .Success}}
|
||||
<div class="bg-green-50 border border-green-200 text-green-700 px-4 py-3 rounded-lg mb-6">{{.Success}}</div>
|
||||
{{end}}
|
||||
{{if .Error}}
|
||||
<div class="bg-red-50 border border-red-200 text-red-700 px-4 py-3 rounded-lg mb-6">{{.Error}}</div>
|
||||
{{end}}
|
||||
|
||||
<!-- Global policy -->
|
||||
<form action="/admin/settings/upload" method="post" class="bg-white rounded-xl shadow-sm border border-gray-200 p-6 mb-8">
|
||||
|
||||
Reference in New Issue
Block a user