139 lines
13 KiB
Markdown
139 lines
13 KiB
Markdown
# 代码审阅报告 —— 9-01 深夜加固批次（凌晨 02:11–02:23）
|
||
|
||
- 审阅范围：`internal/shared/ip.go`、`internal/route/ratelimit.go`、`internal/route/autoban.go`、`internal/template/markdown.go`、`internal/form/form.go`，及 3 个测试（`ip_test.go`、`markdown_test.go`、`csrf_binding_test.go`）
|
||
- 审阅方式：逐文件源码走查 + 交叉核对调用方契约（route.go 挂载点、login.go uid 键、ipblacklist 精确匹配、模板 FuncMap）+ **第三方库源码级语义核验**（flamego@v1.9.4 `run()`、flamego/csrf@v1.3.0 `Validate`、net/http 表单惰性解析）
|
||
- 审阅日期：2026-09-02
|
||
- 结论：**单个文件实现均正确、自洽，且有测试覆盖；但 form.go 的「请求体 12MiB 上限」因中间件执行顺序在真实链路上失效，属上一轮安全评审 P1-3「磁盘耗尽」未真正闭环；另有一处反序列化失败不中断业务链的通用缺陷。** 建议修复后再上线（本次仅审阅，未改码）。
|
||
|
||
---
|
||
|
||
## 一、🔴 P1-1 请求体 12MiB 上限实际失效：超大 multipart 仍可无节流落盘（磁盘耗尽 DoS 未闭环）
|
||
|
||
**位置**：`internal/form/form.go:59-79`（上限安装点）、`internal/context/context.go:180-182`（CSRF 校验点）、`internal/route/route.go:372-388`（中间件注册顺序）
|
||
|
||
### 问题
|
||
`maxRequestBodySize` 的 `http.MaxBytesReader` 被安装在 **form.Bind 内（路由级 handler）**，但真实链路上 CSRF 校验（`Contexter` → `x.Validate(ctx)`）是 **组级中间件、先于一切路由级 handler 执行**：
|
||
|
||
```
|
||
安全头 → IP黑名单 → autoban → cache/recaptcha/session → csrf.Csrfer → templater
|
||
→ Contexter（★ 此处对每个 POST 执行 csrf.Validate）
|
||
→ form.Bind（★ 12MiB 上限到这一步才装上）→ rateLimit → 业务动作
|
||
```
|
||
|
||
而 flamego/csrf 的 `Validate` 取值顺序是**先读 Header，无 Header 时 `FormValue("_csrf")`**——后者会触发 net/http 的**整包惰性解析**：
|
||
|
||
- multipart：`ParseMultipartForm(默认 32MiB 内存)`，超出部分**无上限落盘临时文件**；
|
||
- 解析**发生在 token 校验之前**：即使不带/带错 CSRF token（随后被 400 拒绝），超大 body 也已被完整读入并落盘；
|
||
- 该 400 由 `Contexter` 直接写出 → **响应已写、链路中断** → 路由级的 `rateLimitMiddleware` / autoban 计数**根本没机会执行**。
|
||
|
||
即：对任意 POST 路由（公开提问框 `/_/{domain}`、`/vote/{token}/submit`、`/login` 等），攻击者**无需有效 CSRF token、不受限流约束**，持续 POST >32MiB 的 multipart body 即可无限写临时目录，1C1G 机器可在数分钟内被打满磁盘。form.go 注释中想防御的场景（匿名可达接口的磁盘耗尽）依然存在。
|
||
|
||
> 补充：urlencoded 大 body 同理在 CSRF 阶段整体载入内存（无上限、无限流），为内存压力向量。
|
||
|
||
### 修复建议（单点修复，可同时覆盖下文 P2-2）
|
||
在**全局 `f.Use` 层**（`route.go:146` autobanMiddleware 之后即可）安装统一请求体上限，早于一切组级中间件与 body 解析：
|
||
|
||
```go
|
||
// internal/route/（建议独立文件 bodylimit.go）
|
||
const maxRequestBodySize = 12 << 20 // 与 form.go 语义一致；如需多图余量可统一上调
|
||
|
||
func requestBodyLimitMiddleware() flamego.Handler {
|
||
return func(ctx flamego.Context) {
|
||
req := ctx.Request().Request
|
||
req.Body = http.MaxBytesReader(ctx.ResponseWriter(), req.Body, maxRequestBodySize)
|
||
}
|
||
}
|
||
// route.go
|
||
f.Use(autobanMiddleware(ab))
|
||
f.Use(requestBodyLimitMiddleware()) // 必须在 Contexter 之前
|
||
```
|
||
|
||
- **取值论证**：合法单请求最大 body = 单张图片 ≤8MiB（`storage.MaxUploadBytes`，`internal/storage/s3.go:36`，且提问/回答只取 `f.Images[0]`）+ 表单字段/边界开销，远小于 12MiB；头像/背景 2MiB、私聊图 5MiB 均安全。12MiB 不会误伤合法提交。
|
||
- 该中间件同时封住：form.Bind 路由（含 HTML 表单路径）、chat `/send`、vote `/submit`，以及 CSRF 自身触发的解析。form.go 内层 wrap 可保留（幂等，误差同源）或删除。
|
||
- **回归测试建议**：仿 `route/csrf_binding_test.go` 增加路由级测试：向任一 POST 路由发送 Content-Length 超过上限的 multipart body，断言返回 4xx 且不落入业务（可借 `f.Post("/protected", ...)` 桩验证 body 未被读）。
|
||
|
||
---
|
||
|
||
## 二、🟠 P2-1 form.Bind 反序列化失败不阻断业务链：零值结构体进入业务动作
|
||
|
||
**位置**：`internal/form/form.go:70-78`（失败分支仅 `c.Map(Error{...})` 后 `return`）
|
||
|
||
### 问题
|
||
flamego@v1.9.4 `run()`（`context.go:190-228`）的链语义是：**handler 返回后只要 `ResponseWriter().Written() == false`，就继续执行下一 handler**（没有「不调用 Next 即停链」的语义）。因此 Bind 解析失败分支只 `Map(Error)`、**不 `SetError` 也不写响应** → 后续业务动作**以零值结构体照常执行**，`ctx.HasError()` 恒为 false，govalid 校验被整体跳过。
|
||
|
||
**触发面**：body 在 Bind 阶段解析失败（>12MiB、畸形编码）。HTML 表单提交的 body 在 CSRF 阶段已解析成功（Bind 幂等重解析不会失败），**实际只影响 Header 携带 CSRF token 的 JS/fetch 提交**（token 在头里时 CSRF 不解析 body，解析首次发生在 Bind）。匿名路由攻击者可自持「uid=0」的匿名 token 构造该请求。
|
||
|
||
**各动作影响（按危害排序）**：
|
||
|
||
| 动作 | 零值后果 | 二次防线 |
|
||
|---|---|---|
|
||
| `user.UpdateProfile`（`route/user/profile.go:66`） | `db.Users.Update` 用 **map 全字段无条件写**（`internal/db/users.go:336-357`）→ 昵称/简介/配色/社交链接被清空、`Notify` 置 `None`（**静默退订邮件通知**）；无任何错误提示 | 无验证码；仅限账户本人/持其 CSRF 的请求（自伤型数据丢失） |
|
||
| `user.UpdatePagePassword` / `UpdateAskPassword`（`route/user/password.go:17,32`） | 这两个 handler **连 `HasError` 都不查**，直接 `f.Password`（空=清除密码）→ 反序列化失败等于「关闭页面/提问密码」，**防护被静默解除** | 仅本人（自伤型） |
|
||
| 匿名 `question.New` / `PublishFollowUp`（`route/question/page.go:315`） | 零值内容绕过 govalid 与屏蔽词/长度校验进入业务；`New` 有验证码闸门兜底，`PublishFollowUp` **无验证码且该路由无限流** | 验证码开启时安全；站点关闭/未配验证码时可产生空内容垃圾数据 |
|
||
| `auth.Register/Login/Forgot/Recover` | 零值走 DB 校验失败路径 | 验证码 + DB 唯一性 |
|
||
|
||
### 修复建议（与既有 govalid 分支同构，改动最小）
|
||
`internal/form/form.go:71,76` 两处失败分支，在 `c.Map(...)` 前补 `c.SetError(err)`（或本地化包装，如「请求内容不合法，请重试」）。`SetError` 会置 `HasError=true` 并写入 `Data["Error"]`（`internal/context/context.go:71-86`），所有动作的 `if ctx.HasError()` 分支随即生效（重渲染当前页并提示），与校验失败体验一致。
|
||
|
||
```go
|
||
if err := r.ParseForm(); err != nil {
|
||
c.SetError(err)
|
||
c.Map(Error{Category: ErrorCategoryDeserialization, Error: err})
|
||
return
|
||
}
|
||
```
|
||
|
||
> 注意：`UpdatePagePassword`/`UpdateAskPassword` 本身不查 `HasError`（空密码=关防护是合法操作），若采用 SetError 方案需给这两个 handler 补一个 `if ctx.HasError() { ctx.Redirect(...); return }` 守卫，否则 SetError 后仍会往下写空密码。
|
||
|
||
### 回归测试建议
|
||
给 `internal/form`（或路由级）补用例：模拟 >12MiB 的 multipart body 经 Bind 后，断言 `HasError()==true`；若约束链上动作，断言动作未执行。
|
||
|
||
---
|
||
|
||
## 三、🟠 P2-2 chat `/send` 与 vote `/submit` 无任何请求体上限
|
||
|
||
**位置**：`internal/route/chat/chat_handler.go:127-141`、`route/vote/vote.go:678+`；两路由均不走 `form.Bind`（`route.go:245,257` 直接挂 handler）。
|
||
|
||
**问题**：`SendMessage` 用 `FormValue("content")`/`FormFile("image")`、`Submit` 用多个 `FormValue(...)` 惰性触发解析——在 Header token 路径下（CSRF 不预解析），首次解析发生在 **handler 内部、无 MaxBytesReader**：multipart 超 32MiB 落盘无上限。私聊图 5MiB 业务上限在**解析完成后**才检查（`chat_handler.go:233`）。
|
||
|
||
**缓解因素**：chat `/send` 前置 `reqUserSignIn`（`route.go:248`）+ 20/min 限流；vote `/submit` 前置三层限流。故列为 P2。**采用 P1-1 的全局 body 上限方案后本项自动闭环**，无需单独改动。
|
||
|
||
---
|
||
|
||
## 四、✅ 确认无问题项（本批次核心实现 + 测试）
|
||
|
||
| 文件 | 判定 | 依据 |
|
||
|---|---|---|
|
||
| `internal/shared/ip.go` | ✅ | 信任模型正确：`TrustProxy=off` 时仅取 `RemoteAddr`；on 时 CF-Connecting-IP → 可信代理 XFF 剥取。`LimitKey` IPv6 聚合 /64 防轮换绕过；注释明确「不得用于黑名单」——与 `ipblacklist.go` 的 exact/CIDR 匹配一致 |
|
||
| `internal/route/ratelimit.go` | ✅ | 滑动窗口 + 全局注册表 `sync.Once` 单清理协程（修复旧版每实例泄漏）；visitors>10000 快速清理守卫；匿名/登录 3 倍分级独立计数；vote 三层限流键无冲突。登录判定 `s.Get("uid").(uint)` 与 login.go 写入类型一致 |
|
||
| `internal/route/autoban.go` | ✅ | `LimitKey` 键与 `f.NotFound` 记录（`route.go:395`）一致，封禁可命中；403 直断（写响应即停链）；敏感路径 `defer Record` 于响应后计数；404 独立计数，避免业务 302→/ 误判 |
|
||
| `internal/template/markdown.go` | ✅ | 白名单消毒 + `safeURL`（拒 `//`、`\`、`javascript:` 等）+ `safeColorValue`；`MarkdownToHTML`/`PlainText` 与模板 FuncMap 注册一致；测试覆盖 XSS/协议相对 URL/字体色注入 |
|
||
| `csrf_binding_test.go` | ✅ | 守卫 `CSRFUserSessionKey="uid"` 与 login 绑定一致（误改回 "userID" 即失败）；会话 B 复用 A 的 token → 400 |
|
||
| `ip_test.go` / `markdown_test.go` | ✅ | 覆盖充分（IPv4 透传、v4-mapped 折叠、/64 聚合、端口、非法/空；markdown 六类用例） |
|
||
|
||
**次要观察（不改）**：`rateLimiter.allow()` 的 >10000 清理仅删「窗口内已无活动」键，活动大流量下 map 仍可增长——受 `LimitKey` 聚合 + 短窗口约束，实际可达量级（数万键 ≈ 数 MB）可接受，非回归。
|
||
|
||
---
|
||
|
||
## 五、✅ 修复实施记录（2026-09-02 夜，已落地并验证）
|
||
|
||
> 上节为原评审建议（针对 02:11–02:23 深夜加固批次）。用户批准后已全部实施，P2-2 随 P1-1 自动闭环。**复核中发现原 P2-1 影响面比初判更大**：`UpdateHarassment` / `UpdateBlockWords` / `UpdateTags` 与两个 password handler 一样**不检查 `HasError`**，零值结构体会静默关闭防骚扰 / 清空屏蔽词 / 清空标签——一并纳入守卫。
|
||
|
||
| 编号 | 改动文件 | 内容 | 验证 |
|
||
|---|---|---|---|
|
||
| P1-1 | `internal/route/bodylimit.go`（新增） | 全局 `requestBodyLimitMiddleware`：POST/PUT/PATCH 且 `ContentLength>12MiB` → 413 快速失败；其余用 `http.MaxBytesReader` 惰性封顶。挂载于 `f.Use` 链（autoban 之后、所有 Group 之前），先于 Contexter 内 CSRF 的整包惰性解析 | `go build -overlay` ✅ |
|
||
| P1-1 | `internal/route/route.go` | `f.Use(requestBodyLimitMiddleware())`（autoban 之后） | 全模块编译 ✅ |
|
||
| P1-1 | `internal/form/form.go` | `maxRequestBodySize` 导出为 `MaxRequestBodySize`（单一事实来源，供全局中间件引用） | `go vet` ✅ |
|
||
| P1-1 | `internal/route/bodylimit_test.go`（新增） | 回归测试：合法 8MiB → 200；Content-Length 超限 → 413；chunked 超限（绕过预检）→ MaxBytesReader 拒绝 | `go test ./internal/route/` ✅ |
|
||
| P2-1 | `internal/form/form.go` | 两个反序列化失败分支（ParseForm / ParseMultipartForm）补 `c.SetError(err)`——置位 HasError，使检查 HasError 的 action（含匿名提问/回答/追问）在零值结构体进入业务前早退 | 同上 |
|
||
| P2-1 | `route/user/harassment.go` | 新增 `abortOnFormError` helper（HasError → 错误经 flash 转交 → redirect 中止）；`UpdateHarassment`、`UpdateBlockWords` 开头加守卫 | 同上 |
|
||
| P2-1 | `route/user/tags.go` | `UpdateTags` 开头加守卫（防清空标签 + 关闭首页展示） | 同上 |
|
||
| P2-1 | `route/user/password.go` | `UpdatePagePassword` / `UpdateAskPassword` 开头加守卫（防空密码静默解除页面/提问防护；govalid 超长密码此前也会绕过前端限制直接入库，一并拦截） | 同上 |
|
||
| P2-2 | — | chat `/send`、vote `/submit` 已被全局中间件覆盖（超限时 CSRF/解析失败即拒），无需单独改动 | 见 P1-1 |
|
||
|
||
**测试结果**（`go test -overlay=dev/overlay.json`）：`templates`、`internal/template`、`internal/form`、`internal/shared`、`internal/security/autoban`、`internal/security/censor`、`internal/route`（含新增 bodylimit 回归 + csrf_binding）全部 PASS；`go build ./...` 与 `go vet ./internal/... ./route/...` exit 0。
|
||
|
||
---
|
||
|
||
*附：本报告评审结论基于源码走查与依赖库源码语义核验；修复部分已于 2026-09-02 夜落地并通过 overlay 全模块编译 / vet / 相关包测试。*
|