Files
tamabox/outputs/TamaBox-v4-code-review-report-2026-08-26.md
T
tamakyi e3e59e7c69 chore: baseline snapshot before moderation-strictness feature
Repo .git was damaged (refs/ missing, index truncated, object store near-empty).
Repaired via git init (rebuilt refs), stale index preserved as .git/index.corrupt-20260910.bak,
then full worktree committed as a rollback baseline.
2026-09-10 10:56:27 +08:00

106 lines
8.9 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# TamaBox-v4 代码逻辑与安全审阅报告
- **审阅时间**:2026-08-26
- **审阅对象**:`C:\Users\Tamakyi Gekka\Downloads\TamaBox-v4-backup-20260813-025725-release\TamaBox-v4-new-toupiao`
- **审阅范围**:Go 后端、路由/中间件、投票与私聊业务、模板渲染与 Markdown 清洗、配置与测试
- **审阅方式**:静态代码审阅、调用链回溯、关键路径输入校验检查、现有测试执行
- **结论口径**:本报告只把能由当前代码直接确认的问题列为“确认问题”;无法在当前环境完整运行的部分单列为验证限制。
## 一、总体结论
当前版本已有较多安全加固(CSRF、会话密码版本、图片像素限制、代理 SSRF 防护、内容审核、投票提交唯一索引等),但仍发现 **4 项需要处理的确认问题**:
- **P1:2 项**:匿名投票指纹 Cookie 可伪造绕过防刷;Markdown 清洗器存在实体编码导致的存储型 XSS。
- **P2:2 项**:多选重复选项可被重复计数;“同一指纹可投次数”配置与数据库唯一索引/应用逻辑不一致,配置大于 1 时实际仍只能投 1 次。
因此,当前不建议直接以“P0/P1 已全部清零”作为上线判断。至少应先修复两项 P1,并对投票防刷策略做明确取舍。
## 二、确认问题清单
### P1-01 🔴 匿名投票指纹 Cookie 未签名,可无限伪造绕过防刷
| 项目 | 详情 |
|---|---|
| 位置 | `route/vote/vote.go:52-90`、`route/vote/vote.go:619-669` |
| 触发条件 | 匿名投票,攻击者可自行发送任意 `vote_fp` Cookie 值 |
| 证据 | `currentVoterToken` 直接返回 Cookie 值;`ensureVoterToken` 也只检查“是否存在”,未校验服务端签名或格式。匿名提交只有在 `currentVoterToken(ctx) == ""` 时才执行 IP 计数(`route/vote/vote.go:656-665`)。数据库唯一索引只约束同一 `vote_id + voter_token`,攻击者每次换一个伪造值即可创建一条新记录。 |
| 影响 | 可绕过匿名投票的 IP 兜底与 Cookie 防重复机制,批量提交大量票;结果统计可被污染,验证码未开启时尤其明显。 |
| 修复建议 | 不要把客户端 Cookie 当作可信身份。使用带 HMAC 的服务端签名 token(签名内容包含随机值、版本与过期时间),服务端拒绝无效/过期/格式异常 token;对匿名提交始终保留 IP/设备/行为限流,不应因 Cookie 存在而跳过 IP 保护。若需要强防刷,使用服务端持久化指纹或按投票维度的限速。 |
### P1-02 🔴 Markdown 清洗器对实体编码文本未再次转义,存在存储型 XSS 路径
| 项目 | 详情 |
|---|---|
| 位置 | `internal/template/markdown.go:95-117`,重点为 `internal/template/markdown.go:103-106` |
| 触发条件 | 用户输入经过 Markdown 渲染后,文本中包含实体编码形式的 HTML,例如 `<img src=x onerror=alert(1)>`;该结果通过 `MarkdownToHTML` 以 `template.HTML` 输出。 |
| 证据 | `html.Tokenizer.Text()` 返回“已解码”的文本(`golang.org/x/net/html` 文档/实现明确为 unescaped text);当前 `extractPlainText` 在 `html.TextToken` 分支直接 `out.WriteString(string(tokenizer.Text()))`,未调用 `html.EscapeString`。因此实体编码可能重新变成原始 `<...>` 字符并被当作 HTML 输出。`MarkdownToHTML` 最终在第 73 行以 `template.HTML(cleaned)` 返回,形成危险输出路径。现有 `internal/template/markdown_test.go:35-54` 没有覆盖实体编码绕过。 |
| 影响 | 提问、回答、投票描述等使用 Markdown 渲染的内容可能形成存储型 XSS;受影响页面上的访问者/登录用户可能执行攻击者脚本。 |
| 修复建议 | 在 `sanitizeHTML` 的 `html.TextToken` 分支对 tokenizer 文本再次使用 `html.EscapeString` 后写入;或改用成熟且经过长期验证的 HTML sanitizer(如 bluemonday),并补充 `&lt;script&gt;...`、双重编码、属性上下文等回归测试。修复后检查所有 `template.HTML` 输出点。 |
### P2-01 🟠 多选提交未拒绝重复选项 ID,可造成单次提交重复计票
| 项目 | 详情 |
|---|---|
| 位置 | `route/vote/vote.go:741-758` |
| 触发条件 | 对多选投票手工构造 POST,使 `option_ids` 重复,例如 `option_ids=12&option_ids=12`。 |
| 证据 | 后端只检查 `len(ids) > v.MaxChoices`、选项 ID 是否存在,然后直接将每个 ID追加到 `chosen`;未使用 `seen` 集合去重。结果聚合 `route/vote/vote.go:203-223` 遍历 `data.Options`,每次出现都增加该选项计数。 |
| 影响 | 一次提交可让同一选项被重复计数;若 `MaxChoices` 大于 1,可用重复值放大某选项票数,破坏统计可信度。 |
| 修复建议 | 解析时维护 `seenOptionIDs`,重复 ID 直接拒绝或去重;同时在统计层对历史异常数据做防御性去重,并补充重复参数测试。 |
### P2-02 🟠 “同一指纹可投次数”配置与实际实现矛盾
| 项目 | 详情 |
|---|---|
| 位置 | `route/user/vote.go:199-205`、`route/vote/vote.go:619-669`、`internal/db/vote_submissions.go:49-55` |
| 触发条件 | 箱主将 `max_votes_per_fingerprint` 设置为 2-100。 |
| 证据 | 管理端允许并保存 1-100(`route/user/vote.go:199-205`);但 `VoteSubmission` 定义了唯一索引 `UNIQUE(vote_id, voter_token)`,且提交前 `HasTokenVoted` 一旦已有记录就直接拒绝(`route/vote/vote.go:627-635`)。因此同一匿名指纹对同一投票始终只能提交一次。该配置只在“无 Cookie 时”的 IP 计数分支使用,无法实现其名称暗示的多次投票。 |
| 影响 | 管理员配置与实际行为不一致;若业务预期允许同一指纹投多次,功能失效;若业务只允许一票,配置项会误导并使防刷策略难以理解。 |
| 修复建议 | 二选一:①业务规则是一人一票,则把字段固定为 1、移除 2-100 UI 和相关逻辑;②业务确实需要多投,则去掉 `UNIQUE(vote_id, voter_token)`,改为服务端计数/窗口约束,并使预检、数据库约束、IP 兜底和结果策略统一。不要只改 UI。 |
## 三、已检查且未发现确认问题的重点区域
- **认证/会话**:登录成功后调用会话 ID 重生成;密码版本变更会使旧会话失效(`route/auth/login.go:64-74`、`internal/context/auth.go:26-30`)。
- **CSRF**:Web POST 统一进入 CSRF 校验;注销已改为 POST;当前未发现可直接确认的高风险 CSRF 缺口。
- **投票归属**:管理端通过 `getOwnedVote` 校验 `vote.UserID == ctx.User.ID`,结果查看、编辑、发布、关闭、删除、导出均复用该校验。
- **SQL 注入**:业务查询主要使用参数绑定;本次检索未发现由用户输入直接拼接 SQL 的确认路径。
- **远程图片代理**:域名白名单、私网 IP 拦截、重定向逐跳校验、固定解析 IP 拨号和响应体上限均已存在;仍建议后续补充集成测试。
- **图片上传**:已在解码前检查像素上限,降低解压炸弹风险。
- **Flash/模板普通文本**:`templates/base/alert.html` 使用普通模板转义;当前未发现历史报告所述的 FlashTip 原样 HTML 输出问题。
## 四、验证结果与环境限制
### 已通过
```text
go test ./internal/template ./route/vote ./templates ./internal/security/autoban \
./internal/security/censor ./internal/imagecheck ./internal/shared ./internal/i18n
PASS
```
### 完整测试未能作为全绿结论
`go test ./...` 当前失败,主要原因是:
1. 本机 CGO/WebP 依赖导致 `github.com/chai2010/webp` 编译错误(`webpGetInfo` 等符号缺失),进而使 `internal/storage`、`route/question`、`route/user`、主程序等包无法完整编译。
2. `internal/db` 测试未配置数据库类型,多个测试报 `Unknown database type: ""`。
上述失败不等同于本次发现的 P1/P2 已被测试覆盖;建议在带 C 工具链与测试数据库的 CI/预发布环境重新执行:
```text
go test ./...
go vet ./...
govulncheck ./...
```
## 五、修复优先级建议
1. **P1-02**:先修 Markdown 清洗器实体编码绕过,并新增 XSS 回归测试。
2. **P1-01**:改为服务端签名匿名 token;同时恢复匿名提交的稳定限流,不因客户端 Cookie 存在而完全跳过 IP/行为防刷。
3. **P2-01**:多选参数去重 + 统计层防御性去重。
4. **P2-02**:明确“一人一票”还是“同指纹多票”,统一数据库约束、应用预检、配置和 UI 文案。
5. 在修复后使用真实测试数据库、启用 CGO 的构建环境重新跑全量测试与漏洞扫描。
## 六、与历史报告的差异说明
项目内已有的 `code-review-report-final.md` 将上一轮结论描述为“P0/P1 全部清零”。本次复审发现的 2 项 P1 与投票相关 P2 属于当前代码路径中仍存在的独立问题,因此不能直接沿用上一轮“全部清零”的结论;应以本报告的当前代码证据为准。