From 3cf35a4d4847583e6d0c2149f6845a647364139c Mon Sep 17 00:00:00 2001 From: everbird Date: Fri, 24 Jul 2026 12:13:31 -0700 Subject: [PATCH 1/4] docs: archive completed plans --- docs/{ => archive}/ha-plan.md | 0 docs/{ => archive}/improvement-plan-2026-07.md | 2 +- docs/cdn-improvement-plan.md | 4 ++-- docs/maintenance-support-plan.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename docs/{ => archive}/ha-plan.md (100%) rename docs/{ => archive}/improvement-plan-2026-07.md (99%) diff --git a/docs/ha-plan.md b/docs/archive/ha-plan.md similarity index 100% rename from docs/ha-plan.md rename to docs/archive/ha-plan.md diff --git a/docs/improvement-plan-2026-07.md b/docs/archive/improvement-plan-2026-07.md similarity index 99% rename from docs/improvement-plan-2026-07.md rename to docs/archive/improvement-plan-2026-07.md index 6caeee6..395c21e 100644 --- a/docs/improvement-plan-2026-07.md +++ b/docs/archive/improvement-plan-2026-07.md @@ -13,7 +13,7 @@ next_item_id: "IMP-010" # 站点改进计划（2026-07） -> 本文是 ohmywod 应用安全、正确性、可发现性、性能和工程质量的工作计划。维护成本支持与 AdSense 去留以 [站点维护成本支持计划](maintenance-support-plan.md) 为准；备份、恢复和节点替换以 [单机 DR 与节点替换计划](ha-plan.md) 为准。 +> 本文是 ohmywod 应用安全、正确性、可发现性、性能和工程质量的工作计划。维护成本支持与 AdSense 去留以 [站点维护成本支持计划](../maintenance-support-plan.md) 为准；备份、恢复和节点替换以 [单机 DR 与节点替换计划](ha-plan.md) 为准。 > > **当前结论：IMP-001 至 IMP-009 已全部完成。应用安全、正确性、SEO、健康检查、SQLite 锁等待、防滥用和最小 CI 都已有实现与验证；生产已启用共享 Redis sitemap cache，并为战报 HTML 增加安全的条件请求。搜索在 11,515 条公开战报下实测 p95 约 11 ms，因此继续使用简单 LIKE，不引入 FTS；后续只在达到已记录阈值时重新评估性能设计。跨仓主线继续由 HA/DR 计划维护。** diff --git a/docs/cdn-improvement-plan.md b/docs/cdn-improvement-plan.md index 36d6a72..3e18814 100644 --- a/docs/cdn-improvement-plan.md +++ b/docs/cdn-improvement-plan.md @@ -7,13 +7,13 @@ language: zh-CN created_at: "2026-07-24" last_updated: "2026-07-24" review_commit: "907babd" -review_worktree: "dirty: existing docs/ha-plan.md changes preserved" +review_worktree: "dirty: existing docs/archive/ha-plan.md changes preserved" next_item_id: "CDN-003" --- # 国内访问与 CDN 改进计划（未来） -> 本文是战报网未来 CDN 与国内访问体验优化的工作计划，不是多地域部署方案。节点替换与恢复以 [单机 DR 与节点替换计划](ha-plan.md) 为准，已经完成的应用缓存与安全基线以 [站点改进计划](improvement-plan-2026-07.md) 为准。 +> 本文是战报网未来 CDN 与国内访问体验优化的工作计划，不是多地域部署方案。节点替换与恢复以 [单机 DR 与节点替换计划](archive/ha-plan.md) 为准，已经完成的应用缓存与安全基线以 [站点改进计划](archive/improvement-plan-2026-07.md) 为准。 > > **当前结论：继续使用 Cloudflare 橙云和东京源站，不增加国内节点或更换 CDN。下一步最有价值的两项小改进是：让公开且基本不可变的 `/r/raw/*` 战报 HTML 在 Cloudflare 边缘缓存 1 天；缩小每页都会加载的 1024×1024、约 956 KB logo。两项可以并行。metadata 页面、登录态页面和互动接口继续不缓存。** diff --git a/docs/maintenance-support-plan.md b/docs/maintenance-support-plan.md index 714e212..b70b5ce 100644 --- a/docs/maintenance-support-plan.md +++ b/docs/maintenance-support-plan.md @@ -354,7 +354,7 @@ Review 关注：确认复盘没有演变成新的分析系统，回馈没有形 - Review AI：`unassigned` - 关联事项：创建 SUP-001 至 SUP-006 - 状态变化：新增 4 个 `todo`、2 个 `assessing` -- 改动：新增 `docs/maintenance-support-plan.md` 草案；旧广告优化方案移入 `docs/archive/`；同步清理 `docs/improvement-plan-2026-07.md` 中把站点改进与广告增长绑定的旧表述；尚未修改应用代码或站外账号 +- 改动：新增 `docs/maintenance-support-plan.md` 草案；旧广告优化方案移入 `docs/archive/`；同步清理 `docs/archive/improvement-plan-2026-07.md` 中把站点改进与广告增长绑定的旧表述；尚未修改应用代码或站外账号 - 关键取舍：收入方向从 AdSense 优化改为自愿维护成本支持；推荐完全移除 AdSense；不再要求用数据证明撤广告合理 - 验证：核对现有两个广告单元、首页微信打赏入口和旧计划引用；相关文档链接可解析；`git diff --check` 通过 - 发生的问题：模板文件位于相邻 `ohmywod-ops` 仓库，本计划沿用其 front matter、工作项和 append-only changelog 结构 From 4cee54498edc5b781a647aece04598ba85be5d75 Mon Sep 17 00:00:00 2001 From: everbird Date: Sat, 25 Jul 2026 16:11:27 -0700 Subject: [PATCH 2/4] docs(codereview): add 2026-07-25 tri-AI review + stamped final MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude 初评 + Codex/Antigravity 交叉评审，reconcile 为 final（双方 stamp）。 Action Items 待实现。 Co-Authored-By: Claude Opus 4.8 --- .../2026-07-25-cross-review-antigravity.md | 114 +++++++ .../2026-07-25-cross-review-codex.md | 130 ++++++++ docs/codereview/2026-07-25-final.md | 292 ++++++++++++++++++ docs/codereview/2026-07-25-review.md | 56 ++++ 4 files changed, 592 insertions(+) create mode 100644 docs/codereview/2026-07-25-cross-review-antigravity.md create mode 100644 docs/codereview/2026-07-25-cross-review-codex.md create mode 100644 docs/codereview/2026-07-25-final.md create mode 100644 docs/codereview/2026-07-25-review.md diff --git a/docs/codereview/2026-07-25-cross-review-antigravity.md b/docs/codereview/2026-07-25-cross-review-antigravity.md new file mode 100644 index 0000000..efed061 --- /dev/null +++ b/docs/codereview/2026-07-25-cross-review-antigravity.md @@ -0,0 +1,114 @@ +# `ohmywod` 代码评审与三方交叉评估 + +- **日期**：2026-07-25 +- **评审者**：Antigravity +- **评审基线**：`ohmywod` 仓库代码 +- **交叉评审对象**： + - `2026-07-25-review.md`（Claude / Opus 4.8） + - `2026-07-25-cross-review-codex.md`（Codex） +- **定位与标准**：个人兴趣项目，以**实用、清晰、高性价比（投入产出比）**为唯一衡量标准，不搞工业级形式主义。 + +--- + +## 总体评估与结论 + +针对 `ohmywod` 应用仓，综合分析 Claude 与 Codex 的评审报告并核对源码后，**综合结论如下**： + +1. **Claude 的初评**抓住了基础安全与运维隐患（Open Redirect、Zip 暂存堆积、用户名大小写），但**漏掉了两个极其关键的路径逃逸与数据逻辑缺陷**。 +2. **Codex 的二次交叉评审非常精准**，通过深入源码发现了用户名/分类名路径逃逸、以及软删除对 raw/ID 视图失效的严重问题，并指出了 Claude 推荐修复函数在特定依赖版本不存在的问题。 +3. **Antigravity 在源码层面的验证结果**：完全证实 Codex 提出的两大核心风险。针对个人兴趣项目，无需引入复杂校验库，仅需增加几处极简的字符限制与条件过滤即可解决 95% 以上的隐患。 + +目前测试集中为 **`67 passed, 180 warnings`**（180 项均为 Python/SQLAlchemy 废弃告警）。项目整体架构简洁清晰，只需针对以下几点做极小成本的收尾。 + +--- + +## 三方交叉对比与逐项裁决 + +### 1. [高风险] 用户名与分类名可能导致文件路径逃逸出 `DATA_DIR` / `UPLOAD_DIR` + +- **代码位置**： + - `ohmywod/views/frontend.py:103-109`（注册用户名校验） + - `ohmywod/views/report.py:511-518`（新建分类名校验） + - `ohmywod/views/upload.py:75-103`（解压与上传落盘路径 `Path(DATA_DIR) / category.owner / category.name / filename`） +- **问题分析**： + - **Claude 判定**：漏报。仅关注了 Zip 内部成员的 Zip-Slip 逃逸。 + - **Codex 判定**：**正确捕获**。在 Python 中，`Path('/var/data') / owner / '/tmp/abc'` 遇上以 `/` 开头的绝对路径时，前面的根目录会被直接丢弃；包含 `..` 时会向上级目录逃逸。加上生产环境 Gunicorn 以 root 运行，风险极其显著。 + - **Antigravity 评估**：**完全同意 Codex**。即使 Zip 成员文件名安全，根目录逃逸也能让文件落到系统任意位置。 +- **实用修法（最简且有效）**： + - 在注册 `validate_username` 和创建分类 `validate_name` 表单校验中，加入正则限制：仅允许字母、数字、中文、下划线、中划线和空格，拒绝包含 `/`、`\`、`..` 或绝对路径。 + - 在 `upload.py` 写入前加一行兜底断言：`tpath.resolve().is_relative_to(Path(data_dir).resolve())`。 + +--- + +### 2. [中高风险] 战报/分类“删除”后，已有链接与 `/raw/` 路径仍可被公开访问 + +- **代码位置**： + - `ohmywod/controllers/report.py:50-75`（删除仅置 `status = 1`） + - `ohmywod/views/report.py:145-189`（`/raw/...` 完全不查数据库，只看磁盘文件） + - `ohmywod/views/report.py:192-210`, `:325-342`（按 id 获取分类/战报时不过滤 `status`） +- **问题分析**： + - **Claude 判定**：漏报。 + - **Codex 判定**：**正确捕获**。用户界面显示“删除”，但拿着旧 ID 链接或 raw 路径依然能看内容，属于符合直觉的逻辑 BUG。 + - **Antigravity 评估**：**完全同意 Codex**。个人分享站的删除诉求通常就是“让分享链接失效”。 +- **实用修法**： + - `ReportController.get_report` / `get_category` 默认增加 `status == None` 过滤。 + - `/raw////` 路由增加一次极轻量的 DB 状态查询，若分类或战报已置为已删除（`status == 1`），直接 `abort(404)`。 + +--- + +### 3. [中风险] 上传 Zip 暂存文件成功后未删除，坏 Zip 抛 500 且遗留垃圾 + +- **代码位置**： + - `ohmywod/views/upload.py:85-128` + - `tests/test_views.py:403-408`, `:438-442` +- **问题分析**： + - **Claude 判定**：正确发现成功路径没有删除 `UPLOAD_DIR` 中的 zip，磁盘满会导致全站无法上传。 + - **Codex 判定**：同意 Claude，并补充指出：现有 `test_views.py` 测试断言中错误地将“Zip 必须保留”写成了预期；且未捕获 `BadZipFile` 异常。 + - **Antigravity 评估**：**三方达成一致**。 +- **实用修法**： + - 使用 `try...finally` 结构，确保解压流程结束后无条件执行 `fpath.unlink(missing_ok=True)`。 + - 显式捕获 `zipfile.BadZipFile`，向前端返回 400 错误提示，而不是触发 500 服务器崩溃。 + - 同步修改 `test_views.py` 中对应的断言，使其符合删除 Zip 的正确预期。 + +--- + +### 4. [低风险] 登录 `next` 参数存在开放重定向（Open Redirect） + +- **代码位置**： + - `ohmywod/views/frontend.py:67-69` +- **问题分析**： + - **Claude 判定**：正确指出 `/login?next=https://evil.com` 风险，但建议使用 `flask_login.utils.url_has_allowed_host_and_scheme`。 + - **Codex 判定**：指出 **Flask-Login 0.6.3 并没有该函数**，建议直接判断 `next.startswith('/') and not next.startswith('//')`。 + - **Antigravity 评估**：**同意 Codex 的纠错**。单行字符串判断最干净可靠，无需依赖第三方 API。 +- **实用修法**： + - 跳转前检查 `if next_url and next_url.startswith('/') and not next_url.startswith('//'):` 允许跳转，否则回首页。 + +--- + +### 5. [低风险] 用户名大小写重名导致登录歧义 + +- **代码位置**： + - `ohmywod/controllers/user.py`（注册精确匹配，登录 `lower().first()`） +- **三方共识**： + - 属于极边角逻辑矛盾。按 Claude/Codex 共同建议：注册查重时改为不区分大小写的 `get_by_login` 逻辑即可，堵住新数据，不需去动 SQLite 存量数据库。 + +--- + +### 6. 其余低优先级项目总结 + +| 事项 | 三方评估结论 | 建议处理方式 | +|---|---|---| +| `per_page` 无上限 | 三方一致认定为小瑕疵 | 在 `utils.py` 中加上 `min(per_page, 100)` | +| Zip 解压展开体积限制 | Codex 提出解压配额防护 | 属于防御性加固，有空可加，优先级不高 | +| Admin HTTP Basic & CSRF 豁免 | 三方一致认定：个人单人运维，**不必管** | 维持现状 | +| `datetime.utcnow()` 警告 | 仅属于 Python 升级告警，**不必管** | 维持现状 | + +--- + +## 推荐的最小动作清单（按优先级排序） + +1. **[必做] 堵住路径逃逸**：对用户名和分类名格式做表单字符限制（禁止 `/`, `\`, `..`）。 +2. **[必做] Zip 清理与坏包处理**：`upload.py` 用 `finally` 清理 zip，捕获 `BadZipFile` 返回 400，更新单元测试。 +3. **[必做] 修复删除生效逻辑**：`get_report`/`get_category` 和 `/raw/` 增加 `status` 过滤。 +4. **[顺手做] 修复 `next` 重定向**：使用 `startswith('/') and not startswith('//')`。 +5. **[顺手做] 修复注册用户名大小写查重**。 diff --git a/docs/codereview/2026-07-25-cross-review-codex.md b/docs/codereview/2026-07-25-cross-review-codex.md new file mode 100644 index 0000000..2581420 --- /dev/null +++ b/docs/codereview/2026-07-25-cross-review-codex.md @@ -0,0 +1,130 @@ +# `ohmywod` 代码评审与交叉评估 + +- 日期：2026-07-25 +- 评审者：Codex +- 基线：`3cf35a4` +- 交叉评审对象：`2026-07-25-review.md`（Claude / Opus 4.8） +- 标准：个人兴趣项目，优先实际正确性、简单性和清晰度；不按工业级规范堆加固项。 + +## 结论 + +项目整体不差：认证已迁到 SQLite，密码方案稳妥，CSRF、限流、上传 HTML 的 CSP sandbox、缓存降级和测试覆盖都比较实用。现有测试在本次评审中实测为 **67 passed**。 + +不过，已有评审漏掉了两个比代码风格更实际的问题： + +1. 用户名和目录名直接进入文件系统路径，可让上传落到配置目录之外； +2. UI 所说的“删除”只从列表隐藏，拿着旧链接仍能访问战报和文件。 + +建议先修下面前四项；其余可以按空闲时间处理。 + +## 建议优先修 + +### 1. [高] 用户名/目录名可把上传路径带出 `UPLOAD_DIR` 和 `DATA_DIR` + +相关代码： + +- `ohmywod/views/frontend.py:103-109`：用户名只做重复校验； +- `ohmywod/views/report.py:511-518`：目录名只做重复校验； +- `ohmywod/views/upload.py:75-103`：二者未经路径约束便参与 `mkdir`、zip 保存和解压路径。 + +例如目录名可以是绝对路径或包含 `../`。`Path(DATA_DIR) / owner / category.name` 遇到绝对的 `category.name` 时会直接丢掉前面的根目录；包含 `..` 时也会向上逃逸。ops 仓还明确说明 Gunicorn 以 root 运行，因此影响不只是“某个用户目录写乱”，而是已注册用户能在配置根目录之外创建目录、保存 zip 和解压文件。 + +这与 zip 成员的 Zip Slip 检查是两件事：成员名可能安全，但解压目标目录本身已经不安全。 + +实用修法： + +- 对用户名和目录名做统一的“单一路径段”校验：允许中文和空格，但拒绝 `/`、`\`、绝对路径、`.`、`..` 和控制字符； +- 写文件前再做一次兜底：对目标 `resolve()` 后确认仍位于配置根目录下； +- 为 `/tmp/x`、`../x`、`a/b`、`a\b` 各补一个回归测试。 + +不要悄悄替换这些字符，因为数据库中的名字、URL 和磁盘路径会因此不一致；表单直接提示“不允许包含路径分隔符”更清楚。 + +### 2. [中] “删除”后旧链接仍能访问 + +相关代码： + +- `ohmywod/controllers/report.py:50-75`：删除仅设置 `status = 1`； +- `ohmywod/views/report.py:192-210`、`:325-342`、`:444-491`：按 id 读取时只检查对象是否存在，不检查 `status`； +- `ohmywod/views/report.py:145-189`：`/raw/...` 完全不查询数据库，只要文件还在就能访问。 + +因此删除战报或目录后： + +- 列表和 sitemap 中看不到； +- `/r/report/`、reader 和已知 raw URL 仍然可用； +- 删除目录时，目录下所有战报也有同样情况。 + +UI 明确写的是“删除”，404 页面也写“或者已经被删除”，所以这更像行为错误，而不是有意做“仅隐藏”。对分享站尤其实际：用户通常会认为删除后旧分享链接失效。 + +简单方案是让所有公开读取路径只接受 active 的 category/report，并让 raw 路径也校验对应的 active 数据库记录。文件可以先保留，方便误删恢复；若本意确实只是隐藏，则应把按钮文案改为“从列表隐藏”，避免误解。 + +### 3. [中] 上传 zip 被当作“临时文件”，但成功后永久保留；坏 zip 还会绕过清理 + +相关代码： + +- `ohmywod/config.py:27-31` 明确说 `UPLOAD_DIR` 只是本地 staging； +- `ohmywod/views/upload.py:85-128` 成功路径不删除 `fpath`； +- 异常只捕获 `OSError`，`BadZipFile` 等解析错误会返回 500，并留下 zip 或部分目标目录； +- `tests/test_views.py:403-408`、`:438-442` 反而把“zip 必须保留”写进了测试。 + +所以 Claude 提出的“zip 会越积越多”判断成立，但修复时不能只加一行 `unlink()`：现有测试也要改，而且清理应覆盖成功、坏 zip 和数据库写入失败等路径。 + +够用的修法： + +- 用 `finally` 清理 staging zip； +- 显式捕获 `BadZipFile`，给用户 400，而不是 500； +- 解压失败时清理本次创建的目标目录； +- 如果确实想保留原始 zip 作为备份，就别再称它为 staging，并补一个保留周期/清理脚本。以当前注释和磁盘阈值看，删除更符合原设计。 + +### 4. [中] 登录后的 `next` 是开放重定向 + +`ohmywod/views/frontend.py:67-69` 直接把查询参数传给 `redirect()`，`/login?next=https://example.invalid` 登录成功后会跳到站外。 + +Claude 的问题判断正确，但给出的修复 API 不适用于当前依赖：本项目的 Flask-Login 0.6.3 **没有** `flask_login.utils.url_has_allowed_host_and_scheme`。 + +个人项目无需引入复杂依赖，接受“以单个 `/` 开头且不以 `//` 开头”的本站相对路径即可；不满足就回首页。顺便给 `https://...`、`//example...` 和 `/r/` 补三个测试。 + +## 建议顺手修 + +### 5. zip 解压没有限制展开后总大小 + +`ohmywod/views/upload.py:88-103` 只受上传 zip 本身大小和磁盘占用阈值约束，没有检查 `ZipInfo.file_size` 总和、成员数或单文件大小。很小的高压缩比 zip 仍可展开成很大的 JuiceFS 数据；站点允许公开注册，这比单纯设置 nginx `client_max_body_size` 更值得防。 + +不必做复杂流式配额。解压前对成员数和 `sum(info.file_size)` 设一个符合真实战报大小的上限，超出返回 400，已经足够。 + +### 6. 用户名大小写规则前后不一致 + +`User.username` 的 SQLite 唯一约束默认区分大小写；注册查重用精确匹配，而登录在 `ohmywod/controllers/user.py:25-30` 用 `lower(...).first()`。于是 `Bob` 和 `bob` 可同时注册，之后登录命中哪个取决于查询结果。 + +Claude 的判断和“先堵新数据，不急着迁移”的建议都合理：注册校验改用不区分大小写的查询，并先用一次离线查询确认存量没有冲突即可。 + +### 7. `per_page` 没有上限 + +`ohmywod/utils.py:11-15` 和 search 都接受用户传入的 `per_page`。加一个 100 左右的上限是便宜且清楚的保护。数据量不大时不紧急。 + +## 对已有 Claude 评审的逐项评估 + +| 原结论 | 我的评估 | +|---|---| +| Open Redirect | **同意问题，修复函数需更正**：当前 Flask-Login 没有所建议的 helper。 | +| 成功上传后 zip 不删除 | **同意**，并补充：测试当前固化了错误行为，坏 zip 也不会进入现有清理分支。 | +| 用户名大小写重名 | **同意**，建议按其“只堵新增”方案处理。 | +| `per_page`、死代码、整串替换 `http:` | **同意优先级较低**。 | +| 当前 Zip Slip 检查无需处理 | 对“zip 成员名”本身基本同意；但原评审漏掉了更外层的 username/category 目标路径逃逸，这是本次最高优先级问题。 | +| Admin Basic、CSRF 豁免、非 ASCII Basic Auth、弃用告警 | **同意暂不投入**，符合个人项目尺度。 | +| BBCode、CSP sandbox、密码迁移等做得好 | **基本同意**；本次未发现需要推翻这些结论的证据。 | +| “真正建议动手的只有三条” | **不同意**：路径逃逸和删除后仍可访问至少应进入同一优先级。 | + +## 验证记录 + +- `.venv/bin/python -m pytest -q -s`：`67 passed, 180 warnings`； +- 180 条均为现有弃用警告，未发现测试失败； +- 直接运行 `.venv/bin/pytest` 在当前执行环境遇到项目导入/pytest 临时捕获文件问题；改用虚拟环境 Python 并关闭捕获后测试全部通过。这是本次工具环境差异，不判为应用缺陷。 + +## 建议的最小处理顺序 + +1. 限制 username/category 为单一路径段，并加最终 containment 校验； +2. 统一上传清理，顺便处理坏 zip 和展开大小上限； +3. 明确“删除”的语义，让旧详情、reader、raw 链接失效； +4. 修 `next` 与用户名大小写查重； +5. 有空再加分页上限、删死代码。 + diff --git a/docs/codereview/2026-07-25-final.md b/docs/codereview/2026-07-25-final.md new file mode 100644 index 0000000..cabb28a --- /dev/null +++ b/docs/codereview/2026-07-25-final.md @@ -0,0 +1,292 @@ +# `ohmywod` 代码评审 — Final 结论与 Action Items + +- **日期**：2026-07-25 +- **整合者**：Claude（Opus 4.8） +- **输入**： + - `2026-07-25-review.md`（Claude 初评） + - `2026-07-25-cross-review-codex.md`（Codex 交叉评审） + - `2026-07-25-cross-review-antigravity.md`（Antigravity 三方评审） +- **标准**：个人兴趣项目，实用 / 清晰 / 高性价比优先，不堆工业级加固。 +- **状态**：**已按 Codex / Antigravity 复核意见修订（rev.2，2026-07-25）**，见文末「Appendix Z」。A1 的 `assert` 已改显式 4xx、A6 的 `per_page` 已补下界 + offset 重算，可再次交付复核。 + +--- + +## 一句话结论 + +初评（我）抓到了 open redirect、zip 暂存堆积、用户名大小写；但**漏掉了两个更实际、更严重的问题**——Codex 首先发现、Antigravity 复验，我这次也已逐条实测确认： + +1. **用户名 / 分类名可让文件写到 `DATA_DIR`、`UPLOAD_DIR` 之外**（已注册用户可触发，生产 Gunicorn 以 root 运行 → 任意目录写）。**最高优先级。** +2. **「删除」只是软隐藏**，拿旧 id 链接或 `/raw/` 路径仍能访问已删战报/目录。 + +我在初评里说「真正建议动手的只有三条」——**这个结论撤回**。下面是三方 reconcile 后的 final。测试现状 `67 passed, 180 warnings`。 + +--- + +## 复核记录（我这次实际验证了什么） + +| 结论 | 复核方式 | 结果 | +|---|---|---| +| 路径逃逸 | 实测 `Path("/mnt/jfs/reports")/owner/name` 在 name 为绝对路径 / `../` 时的行为 | **成立**：绝对名丢弃根目录、`../` 向上逃逸（`/tmp/pwned`、`/mnt/etc/cron.d`）。`validate_username`/`validate_name` 只查重、无字符校验；`secure_upload_filename` 只清洗**文件名**、不管 owner/分类名 | +| 软删除仍可访问 | 读 `get_report`/`get_category`（无 `status` 过滤）、`view_report`（不查 status）、`report_raw`（不查库） | **成立**：删除只置 `status=1`，公开读取路径不过滤 | +| 注册开放 | `grep def register` | **开放注册**，任意人可注册后触发上面第 1 条 | +| open redirect 修法 | 实测 `flask_login.utils` | 我原建议的 `url_has_allowed_host_and_scheme` **在 Flask-Login 0.6.3 不存在**（Codex 纠正正确）。补充：`'/\evil'.startswith('//')` 为 False，**裸的 `startswith('/') and not startswith('//')` 仍会放过 `/\evil.com`**，需一并拒绝反斜杠 | +| 测试固化 zip 保留 | 读 `tests/test_views.py:406-408, 440-442` | **成立**：断言 `os.path.exists(uploaded_zip)`，改清理逻辑必须同步改测试 | +| 坏 zip 返回 500 | 读 `upload.py` except 分支 | **成立**：只 `except OSError`，`BadZipFile` 会冒泡成 500 并遗留文件 | +| Python 版本 | `python -V` | 3.12 → 修复可用 `Path.is_relative_to`（3.9+） | + +--- + +## Final Action Items（按优先级） + +### 🔴 必做 + +#### A1. 堵住用户名 / 分类名的路径逃逸 +- **位置**：`views/frontend.py:103-109`（用户名）、`views/report.py:511-518`（分类名）、`views/upload.py:75-103`（落盘/解压路径） +- **为什么必做**：开放注册 + Gunicorn 以 root 跑 → 任意已注册用户可在配置根目录外建目录、存 zip、解压文件。这是本次最严重项，与 zip 成员的 Zip-Slip 是两回事（成员名安全，但**目标目录本身已逃逸**）。 +- **最简修法**： + 1. `validate_username` / `validate_name` 加正则：允许中文、字母、数字、空格、`_`、`-`；**拒绝** `/`、`\`、`..`、绝对路径、控制字符、以 `.` 开头。 + 2. `upload.py` 写入前做显式 containment 兜底（**不要用 `assert`**——`-O`/`PYTHONOPTIMIZE` 会整条移除，且 `AssertionError` 默认是 500 而非 400；Codex A.1 / Antigravity B.1）： + ```python + root = Path(data_dir).resolve() + target = (root / category.owner / category.name / report_name).resolve() + if not target.is_relative_to(root): + abort(400) + ``` + `DATA_DIR` 与 `UPLOAD_DIR` 是两条独立落盘路径，**各自的目标都要单独校验**。表单字符限制是第一层，containment check 是兜底，两层都保留。 + 3. 回归测试：分类名为 `/tmp/x`、`../x`、`a/b`、`a\b` 各一条。 +- **不要**静默替换非法字符——DB 里的名字、URL、磁盘路径会不一致；表单直接提示「不能含路径分隔符」更清楚。 + +#### A2. 让「删除」真正使链接失效 +- **位置**：`controllers/report.py:114-118`、`views/report.py:145-189`(`/raw`)、`:325-342`(`view_report`)、`:444-491`(`reader`) +- **为什么必做**：分享站的「删除」诉求就是「旧分享链接打不开」；现在删了还能看，是符合直觉的逻辑 BUG（404 页自己都写「或已被删除」）。 +- **最简修法**： + 1. 公开读取路径统一只接受 active 记录：`get_report`/`get_category` 默认加 `status == None`（或在视图里判 `report.status is not None → abort(404)`）。 + 2. `/raw/...` 补一次轻量 DB 查询，对应 report/category 已删则 `abort(404)`。 + 3. 磁盘文件可暂留（便于误删恢复），语义以 DB 状态为准。 +- 若你**本意就是只隐藏**，那改按钮文案为「从列表隐藏」即可——但三方判断这是行为错误，倾向真失效。 + +#### A3. 统一上传清理 + 处理坏 zip +- **位置**：`views/upload.py:85-128`、`tests/test_views.py:406-408, 440-442` +- **为什么必做**：成功路径不删暂存 zip → 越积越多，配 96% 磁盘阈值最终**全站传不了**；坏 zip 还会 500 + 遗留垃圾。 +- **最简修法**： + 1. `try...finally` 里 `fpath.unlink(missing_ok=True)`，覆盖成功 / 坏 zip / 入库失败三条路径。 + 2. 显式 `except zipfile.BadZipFile` → 返回 400。 + 3. 解压失败清理本次建的目标目录。 + 4. **同步改 `test_views.py`**：把「zip 必须保留」的断言改成「zip 应被清理」。 + +### 🟠 顺手做 + +#### A4. 修 open redirect（注意 Flask-Login 0.6.3 没有 helper） +- **位置**：`views/frontend.py:67-69` +- **修法**：只接受本站相对路径——`next_url.startswith('/') and not next_url.startswith('//') and not next_url.startswith('/\\')`，否则回 `wodreport.home`。补 `https://…`、`//evil`、`/\evil`、`/r/` 四条测试。 + +#### A5. 注册用户名大小写查重 +- **位置**：`controllers/user.py` +- **修法**：`validate_username` 改用不区分大小写的 `get_by_login` 查重，堵新数据即可；先跑一次离线查询确认存量无冲突，**不必**动 DB 迁移 / `COLLATE NOCASE`。 + +#### A6. 分页与解压体积上限 +- `utils.py:11-15` 和 search 的 `per_page` 加 `max(1, min(per_page, 100))`（**下界必须有**：负数会让 SQLite `LIMIT -1` 退化为「不设上限」，已实测返回全表），并**在钳位后重算** `offset = (page - 1) * per_page`——`get_page_args()` 内部 `offset = (page-1)*per_page` 用的是原始 `per_page`，只改 `per_page` 不重算会让第 2 页起数据错位（Codex A.2 / Antigravity B.1）。 +- 解压前对成员数和 `sum(info.file_size)` 设一个符合真实战报体量的上限，超出 400（开放注册下比只靠 nginx `client_max_body_size` 实用）。二者都不紧急。 + +#### A7. 删死代码 +- `views/report.py:491` 的 `return` 在 `with` 之后不可达，删掉。 + +### ⚪ 不必管（三方一致，个人项目尺度） +Admin HTTP Basic + CSRF 豁免、`MAX_CONTENT_LENGTH`（nginx 已挡）、`check_auth` 非 ASCII 500、LIKE 通配符、`raw.replace('http:','https:')`、`datetime.utcnow()`/`Query.get()` 弃用告警、拼写 `cateogory`。 + +--- + +## 三方分歧与裁决 + +- **「只有三条值得动手」**（我初评）→ **撤回**。路径逃逸(A1)、软删除(A2)必须与 zip 清理同档。 +- **open redirect 修法**：以 Codex/Antigravity 为准（Flask-Login 0.6.3 无 helper），并**再补一条**：字符串判断要连反斜杠一起拒。 +- **zip 清理**：以 Codex 为准（不能只加一行 `unlink`，要连测试和坏 zip 一起改）。 +- 其余条目三方无实质冲突。 + +## 已确认做得好的（保持） +Argon2id + `{SSHA}` 惰性升级、上传 HTML 的 CSP sandbox、BBCode 实测安全（`| safe` 可放心）、限流 + honeypot、healthz、参数化查询、admin 隐藏 password 列、67 条测试。 + +--- + +## 给复核这份 final 的两个 AI + +请重点复核以下我做过的判断： +1. A1 路径逃逸的**可利用性**：`Path(DATA_DIR)/owner/category.name` 在绝对/`../` 名下逃逸，且 owner/分类名全程无字符校验、`secure_upload_filename` 不覆盖它们。是否同意「开放注册 + root Gunicorn = 最高优先级」。 +2. A2 软删除：`/raw` 完全不查库是否为最薄弱点；用 DB `status` 收口是否够。 +3. A4：确认 Flask-Login 0.6.3 无 `url_has_allowed_host_and_scheme`，且相对路径判断需排除 `//` **与** `/\`。 +4. 是否有被三方**共同漏掉**的项（例如：`/raw` 与 reader 对未登录用户开放是否符合预期；分类/战报的跨用户越权读取是否在意——当前公开可读是设计如此）。 + +--- + +## Appendix A — Codex final 复核意见（未盖章） + +- **复核日期**：2026-07-25 +- **复核者**：Codex +- **裁决**：核心问题、证据和总体优先级基本认可，但当前版本仍有两处技术性错误，且两仓 final 交付尚不完整，因此暂不 stamp。 + +### A.1 阻塞项：不能用 `assert` 承担路径安全边界 + +正文 A1 建议： + +```python +assert tpath.resolve().is_relative_to(Path(data_dir).resolve()) +``` + +这不能作为安全修复： + +1. Python 使用 `-O` / `PYTHONOPTIMIZE` 时会完全移除 `assert`； +2. 普通模式下断言失败会抛出 `AssertionError`，默认得到 500，而不是正文所说的 400/404； +3. `UPLOAD_DIR` 和 `DATA_DIR` 是两条独立落盘路径，必须分别检查最终目标。 + +建议改为显式控制流，例如： + +```python +root = Path(data_dir).resolve() +target = (root / category.owner / category.name / report_name).resolve() +if not target.is_relative_to(root): + abort(400) +``` + +`UPLOAD_DIR` 的 staging 目标做同样检查。表单字符限制是第一层，最终 containment check 是兜底；两层都应保留。 + +### A.2 修正项：分页限幅后必须重算 offset，并限制下界 + +正文 A6 只建议： + +```python +per_page = min(per_page, 100) +``` + +这还不完整： + +- `per_page=-1` 等负数没有被拦住；在 SQLite 中负的 `LIMIT` 可能等价于不设上限； +- 当前 `get_page_args()` 同时返回基于原始 `per_page` 计算的 `offset`。只修改 `per_page` 后，第二页及以后仍使用旧 offset，分页结果会错。 + +应先把 `per_page` 限制在有效区间，再重新计算 offset： + +```python +per_page = max(1, min(per_page, 100)) +offset = (page - 1) * per_page +``` + +这是低优先级项，不影响 A1～A5 的总体裁决，但 final 中的实施建议应保持可直接采用。 + +### A.3 交付完整性：`ohmywod-ops` 尚无 final + +本轮原始 review 范围包含 `ohmywod` 与 `ohmywod-ops`。截至本次复核： + +- `ohmywod/docs/codereview/2026-07-25-final.md` 已存在； +- `ohmywod-ops/docs/codereview/` 中只有初评和交叉评审，没有对应 final。 + +因此只能复核当前应用仓 final，不能为“两仓最终评审已完成”整体盖章。 + +### A.4 已认可的部分 + +以下结论与源码、测试及交叉评审一致： + +- A1 路径逃逸成立，且应为最高优先级； +- A2 软删除后详情、reader、raw 链接仍可访问成立； +- A3 staging zip 堆积、`BadZipFile` 未处理以及测试固化旧行为成立； +- A4 Open Redirect 成立，Flask-Login 0.6.3 确实没有初评所建议的 helper； +- A5 用户名大小写查重不一致成立； +- 保留文件但通过数据库状态让删除链接失效，是符合个人项目尺度的简单方案； +- `67 passed, 180 warnings` 的测试基线已由 Codex 独立复跑确认。 + +### A.5 重新申请 stamp 的条件 + +1. 把 A1 的 `assert` 改为显式 containment 判断和明确的 4xx 响应； +2. 修正 A6 的下界及 offset 重算说明； +3. 若 stamp 代表原始两仓任务整体完成，补齐 `ohmywod-ops` final。 + +满足后可再次交由 Codex 复核并 stamp。 + +--- + +## Appendix B — Antigravity final 复核意见（拒绝 Stamp） + +- **复核日期**：2026-07-25 +- **复核者**：Antigravity +- **裁决**：**拒绝 Stamp（暂不盖章）** +- **依据**：认同 Codex 在 Appendix A 中提出的核心修正项。在修改 A1（安全断言）、A6（分页下界）前，该文档不可作为直投修代码的 Final 指南。 + +### B.1 拒绝 Stamp 的核心依据 + +1. **A1 决不能用 `assert` 做安全边界**： + - 源码复核：`assert tpath.resolve().is_relative_to(...)` 在 Python `-O` / `PYTHONOPTIMIZE` 环境下会被完全忽略。即使在普通模式下，`assert` 失败会抛出 `AssertionError`，触发 500 服务器错误，违背了“返回 400/404”的设计诉求。 + - 修正要求：正文必须明确写为 `if not target.is_relative_to(root): return "非法路径", 400`（或 `abort(400)`）。 + +2. **A6 分页逻辑不严密**： + - 仅写 `min(per_page, 100)` 未拦截 `per_page <= 0` 的异常输入；且在 Flask 控制层修改 `per_page` 后必须同步重新计算 `offset = (page - 1) * per_page`，否则从第 2 页起数据偏移错误。 + +### B.2 认可并强调的判例 + +- **A1（路径逃逸）为最高优先级**：完全认同。注册开放 + Gunicorn root 运行 + 用户名/分类名未校验，组合起来可致配置根目录外任意写文件。 +- **A4（Open Redirect）防护完善**：同意加入 `not next_url.startswith('/\\')` 的修正。 +- **A2（软删除生效）**：同意以 DB `status` 为准收口 `/raw` 和详情读取。 + +### B.3 盖章解锁条件 + +1. 将 A1 中的 `assert` 替换为显式 `if` 判断 + 400 返回； +2. 补全 A6 的 `max(1, min(per_page, 100))` 及 offset 重算说明； +3. 完成上述修改后可正式 stamp。 + +--- + +## Appendix Z — 对 Codex / Antigravity 复核意见的处置（Claude，rev.2） + +- **处置日期**：2026-07-25 +- **裁决**：两位复核者的意见**全部成立、全部采纳**，正文已改；无拒绝项。Codex A / Antigravity B 指向同一组修正（A1 断言、A6 分页），无相互冲突。 + +| 复核条目 | 我的独立复核 | 处置 | +|---|---|---| +| A1 不能用 `assert` 做安全边界（Codex A.1 / Antigravity B.1） | 属实：`-O`/`PYTHONOPTIMIZE` 会移除 `assert`，普通模式下 `AssertionError` 默认 500 而非 400；且 `DATA_DIR`/`UPLOAD_DIR` 是两条独立路径 | **采纳**：A1 步骤 2 改为显式 `if not target.is_relative_to(root): abort(400)`，并注明两条路径各自校验 | +| A6 `per_page` 需下界 + 重算 offset（Codex A.2 / Antigravity B.1） | 实测确认：SQLite `LIMIT -1` 返回全表；`get_page_args()` 内 `offset=(page-1)*per_page` 用原始 `per_page` | **采纳**：A6 改为 `max(1, min(per_page,100))` + 钳位后重算 offset | +| 其余（A1 路径逃逸最高优先、A2 软删除、A4 open redirect 反斜杠、测试基线 67 passed 等） | 两位复核者与初评一致确认 | 维持正文，无需改动 | + +**关联**：ops 仓 final 同轮亦按 Codex 复核修订（含一处我写反的 db0 事实错误），见 `ohmywod-ops/docs/codereview/2026-07-25-final.md` 的 Appendix Z。 + +--- + +## Appendix S — Codex Verification Stamp（rev.2） + +- **复核日期**：2026-07-25 +- **复核者**：Codex +- **复核对象**：本文 rev.2 正文及 Appendix Z +- **裁决**：**APPROVED / STAMPED** + +### Stamp + +> **CODEX VERIFIED — APPROVED** +> +> `ohmywod` code review final rev.2 已解决 Codex Appendix A 提出的全部阻塞项，结论、证据、优先级与建议修法可以作为后续实现依据。 + +### 复核依据 + +1. A1 已把 Python `assert` 改成显式 containment 判断和 4xx 响应，并明确要求分别校验 `DATA_DIR`、`UPLOAD_DIR`； +2. A6 已补 `per_page` 下界，并明确在钳位后重新计算 offset； +3. `ohmywod-ops` final 已补齐，原始两仓 review 的 final 文档完整性条件已满足； +4. 路径逃逸、软删除仍可访问、staging zip 堆积、坏 zip、Open Redirect 与用户名大小写问题的结论仍与当前源码一致； +5. `67 passed, 180 warnings` 的测试基线已由 Codex 独立复跑确认。 + +本 stamp 仅确认评审文档 rev.2 的准确性和可执行性，不表示 Action Items 已经在应用源码中实现。Appendix A/B 保留为历史复核记录，其“未盖章/拒绝”状态已由 Appendix Z 的处置和本 Appendix S 正式取代。 + +--- + +## Appendix S2 — Antigravity Verification Stamp (rev.2) + +- **复核日期**：2026-07-25 +- **复核者**：Antigravity +- **复核对象**：本文 rev.2 正文及 Appendix Z +- **裁决**：**APPROVED / STAMPED（正式盖章通过）** + +### Stamp + +> **ANTIGRAVITY VERIFIED — APPROVED** +> +> `ohmywod` 代码评审 final rev.2 已完全解决 Appendix B 提出的全部阻塞项（A1 安全断言改显式 400 控制流、A6 分页下界及 offset 重算）。文档内容准确、实用，可以作为后续代码修复的指导规范。 + +### 复核依据 + +1. **A1 路径逃逸**：消除了 `assert` 的潜在风险，采用 `if not target.is_relative_to(root): abort(400)` 显式控制流，且对 `DATA_DIR` 和 `UPLOAD_DIR` 两条路径分别做了落盘校验。 +2. **A6 分页处理**：增加了 `max(1, min(per_page, 100))` 下界防护，并补充了在钳位后重新计算 `offset = (page - 1) * per_page` 的细节，修复完整。 +3. **A2 & A4 & A5**：软删除收口、Open Redirect 反斜杠防御、用户名大小写查重逻辑均已闭环。 diff --git a/docs/codereview/2026-07-25-review.md b/docs/codereview/2026-07-25-review.md new file mode 100644 index 0000000..2e50752 --- /dev/null +++ b/docs/codereview/2026-07-25-review.md @@ -0,0 +1,56 @@ +# 代码评审 — `ohmywod`（应用仓） + +- **日期**：2026-07-25 +- **评审者**：Claude（Opus 4.8） +- **说明**：本文为**第一版初评，由 Claude 撰写**，稍后交给其他 AI 交叉评审。 +- **定位**：这是**个人兴趣项目**，不追求工业级一致性/严密性。以下按「是否真的值得你花时间」分档，理论性问题一律降级或直接标「不必管」。 +- 评审时测试状态：`67 passed`。整体质量不错，下面多是锦上添花。 + +--- + +## 值得修（真会影响你或用户） + +### 1. `/login?next=` 可跳转到站外（Open Redirect） +`ohmywod/views/frontend.py:67-69`——`next` 直接 `redirect()`，没校验是不是本站地址。别人能发 `…/login?next=https://evil.example` 骗你的用户登录后跳到钓鱼站。 +**修**（2 行）：跳转前判一下 `next` 是本站相对路径，否则回 `wodreport.home`。可以用 `flask_login.utils.url_has_allowed_host_and_scheme`。 + +### 2. 上传的 zip 暂存文件成功后不删，迟早把上传撑爆 +`ohmywod/views/upload.py:85-128`——原始 zip 落到 `UPLOAD_DIR`，只有出错才删（`:110-114`），**成功路径不删**。文件越攒越多，配上 96% 磁盘保护阈值（`:54-56`），总有一天**全站传不了**。 +**修**：解压成功后 `fpath.unlink()`。这是最实际的一条，属于「不修早晚踩」。 + +### 3. 用户名大小写不一致，可能登串账号 +`ohmywod/controllers/user.py`——注册查重是精确匹配（区分大小写），登录 `get_by_login` 却用 `func.lower(...).first()`（不区分、还取首条）。于是 `Bob` 和 `bob` 能同时存在，登录时落到哪个账号不确定。 +**修（够用版）**：注册时改用 `get_by_login` 查重，堵住新的大小写重名即可，不必为个人项目上 DB 迁移/`COLLATE NOCASE`。存量若没有重名就不用管。 + +--- + +## 顺手可改（便宜的小清理，不改也行） + +- **`per_page` 没上限**（`utils.py:11-15`）：`?per_page=1000000` 会拉一大坨。加一句 `per_page = min(per_page, 100)` 就行。 +- **死代码**：`views/report.py:491` 那条 `return` 在 `with` 之后，永远到不了，可删。 +- **`raw.replace('http:', 'https:')`**（`report.py:185`、`:483`）：整篇字符串替换，会误伤正文里正常的 `http:`。介意再收窄成 `http://`，不介意就算了。 + +--- + +## 可以不管（理论问题，个人项目不值当） + +- Admin 是 HTTP Basic 且豁免了 CSRF：单人运维，风险几乎为零。 +- `MAX_CONTENT_LENGTH` 没设：nginx 的 `client_max_body_size` 已经在边缘挡住了，够用。 +- Zip-Slip 的 `startswith` 少个分隔符（`upload.py:101`）：前面已经挡了 `..` 和 `/`，实际逃不出去。 +- search 的 `LIKE %q%` 通配符：不是注入，最多让人 match 全部，无所谓。 +- `check_auth` 遇非 ASCII 用户名会 500 而不是 401：极端边角，不影响正常使用。 +- `CF-Connecting-IP` 信任：只要 Cloudflare 防火墙那套还在（ops 仓 `sync-cloud-firewall.sh`），就没问题——代码注释也说明了。 +- `datetime.utcnow()` / `Query.get()` 弃用告警（测试里 180 条）：将来升级 SQLAlchemy/Python 才需要动，先记着。 +- 拼写 `cateogory`/`get_cateogories_by_user`：纯外观，改名有传播成本，看心情。 + +--- + +## 已经做得好的（保持就行） + +- 密码 Argon2id + 登录时把老 `{SSHA}` 惰性升级，做得很稳。 +- 用户上传的 HTML 用 **CSP sandbox** 隔离，阅读器还有 lxml sanitizer 白名单。 +- **BBCode 描述是安全的**——我实测过 `[url=javascript:…]`、`