From 2c7dbf890b830877e87900def5665376ec803340 Mon Sep 17 00:00:00 2001 From: voson Date: Sat, 11 Apr 2026 17:04:05 +0800 Subject: [PATCH] docs: add code review fixes plan --- plans/code-review-fixes-2026-04-11.md | 201 ++++++++++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 plans/code-review-fixes-2026-04-11.md diff --git a/plans/code-review-fixes-2026-04-11.md b/plans/code-review-fixes-2026-04-11.md new file mode 100644 index 0000000..7c0ceba --- /dev/null +++ b/plans/code-review-fixes-2026-04-11.md @@ -0,0 +1,201 @@ +# Code Review 修复计划 + +**日期**: 2026-04-11 +**来源**: 三份 code review 报告提炼合并 +**范围**: 7 项修复 + 1 项风险提示（不纳入本次修复） + +--- + +## 1. `secrets-core` 导入/导出链路 + +**目标**: 修复"导入丢失 secret type"。 + +**涉及文件**: +- `crates/secrets-core/src/models.rs` +- `crates/secrets-core/src/service/export.rs` +- `crates/secrets-core/src/service/import.rs` + +**实施项**: +1. 在 `ExportEntry` 增加可选字段 `secret_types: Option>`，键为 secret 名，值为 type。 +2. 修改导出逻辑，除了导出 `secrets` 的值，也把每个 secret 的 `type` 一并导出。 +3. 修改导入逻辑，把 `entry.secret_types` 传给 `AddParams.secret_types`，不再用 `&Default::default()`。 +4. 明确兼容旧导出文件：`secret_types` 缺失时继续默认 `"text"`。 + +**验证**: +1. 新增 round-trip 测试：带 `password` / `key` / `text` 三种类型的导出再导入，类型保持不变。 +2. 新增向后兼容测试：旧格式导入时仍成功，默认回落到 `"text"`。 + +--- + +## 2. `secrets-core` env map + +**目标**: 修复环境变量名碰撞。 + +**涉及文件**: +- `crates/secrets-core/src/service/env_map.rs` + +**实施项**: +1. 统一分隔符策略：把字段名里的 `.` 替换成 `__`（双下划线），保留原始 `_` 为单下划线，避免 `db.password` 和 `db_password` 碰撞。 +2. 如仍发生碰撞，显式返回错误，而不是 `HashMap::insert` 静默覆盖。 + +**验证**: +1. 添加测试覆盖： + - `db.password` vs `db_password` + - 带 `prefix` 的情况 + - 多 entry 合并时碰撞 +2. 确认输出变量名文档与实现一致。 + +--- + +## 3. `secrets-core` rollback + +**目标**: 修复回滚路径中的 TOCTOU 和"使用旧 live 值"问题。 + +**涉及文件**: +- `crates/secrets-core/src/service/rollback.rs` + +**实施项**: +1. 把首次读取 live entry 的逻辑移入事务内，并与 `FOR UPDATE` 合并，避免在加锁前基于过期数据做决策。 +2. 在拿到加锁后的 live row 后，再决定错误消息、审计字段和更新语句输入。 +3. 明确回滚语义： + - 若产品希望"完全回到快照"，则 `name/notes` 也从快照恢复。 + - 若希望保留当前标识，则代码和文档都要显式说明此设计决策。 +4. 避免混用事务前的 `live_entry` 与事务内的 `lr`。 + +**验证**: +1. 新增测试覆盖： + - 回滚时恢复字段是否符合预期 + - 并发更新 + 回滚场景不再依赖过期值 + +--- + +## 4. `secrets-core` API key + +**目标**: 修复 `regenerate_api_key` 返回未持久化 key。 + +**涉及文件**: +- `crates/secrets-core/src/service/api_key.rs` + +**实施项**: +1. 检查 `UPDATE` 的 `rows_affected()`。 +2. 若为 `0`，返回 `NotFoundUser` 或等价业务错误。 +3. 可选：改成 `UPDATE ... RETURNING api_key`，减少"先生成后判断"的分支。 + +**验证**: +1. 新增测试： + - 正常用户可返回新 key + - 不存在用户时返回错误，而不是返回伪成功 key + +--- + +## 5. `secrets-mcp` tools + +**目标**: 修复 MCP 层三个问题： +- `secrets_add` 提交后再回查 entry 的竞态 +- `secrets_find` 计数失败被吞成 0 +- `secrets_rollback` 冗余要求 encryption key + +**涉及文件**: +- `crates/secrets-mcp/src/tools.rs` +- 可能连带 `crates/secrets-core/src/service/add.rs` 的返回结构 + +**实施项**: +1. 让 `svc_add` 直接返回 `entry_id`，MCP 不再在提交后用 `name+folder+user_id` 二次 `resolve_entry`。 +2. `secrets_find` 中移除 `.unwrap_or(0)`： + - 要么把计数错误向上传播 + - 要么返回 `total_count: null` / `count_unavailable: true` +3. `secrets_rollback` 改为只要求用户身份，不要求 encryption key。 +4. 同步修正工具描述文案，避免 schema 与实际行为不一致。 + +**验证**: +1. `secrets_add`：父子关系新增时直接使用返回的 `entry_id`。 +2. `secrets_find`：模拟 count 失败时，结果不再伪装成 `0`。 +3. `secrets_rollback`：无 key 时可执行，工具描述与行为一致。 + +--- + +## 6. `secrets-mcp` Web 会话校验 + +**目标**: 让 JSON API 与页面路由的 `key_version` 校验对齐。 + +**涉及文件**: +- `crates/secrets-mcp/src/web/mod.rs` +- `crates/secrets-mcp/src/web/entries.rs` +- `crates/secrets-mcp/src/web/account.rs` +- 其他仅使用 `current_user_id()` 的 JSON handler + +**实施项**: +1. 抽一个适用于 JSON API 的用户校验辅助函数。 +2. 该函数应同时完成： + - session 取 `user_id` + - 加载用户 + - 比对 `SESSION_KEY_VERSION` 与 DB `key_version` +3. 页面路由继续保留 `require_valid_user`，JSON 路由统一改用等价校验。 +4. 统一失败语义： + - HTML 路由：重定向 `/login` + - JSON 路由：返回 `401` + +**验证**: +1. 新增测试覆盖： + - `key_version` 一致时 JSON API 正常 + - `key_version` 不一致时 JSON API 返回 `401` + - 用户删除/会话损坏时返回正确错误 + +--- + +## 7. Web API 输入校验 + +**目标**: 补齐 Web JSON API 的长度校验，避免 DB 约束错误落成 500。 + +**涉及文件**: +- `crates/secrets-mcp/src/web/entries.rs` +- 如有需要，抽公共 helper 到 `web/mod.rs` 或单独模块 + +**实施项**: +1. 在 `api_entry_patch` 对齐 MCP 的长度规则： + - `folder <= 128` + - `type <= 64` + - `name <= 256` + - `notes <= 10000` +2. 视情况复用 `validation` 常量，避免 Web/MCP 两套上限漂移。 +3. 保持错误返回为 `400`，而不是依赖数据库报错。 + +**验证**: +1. 为超长 `folder/type/name/notes` 分别补测试。 +2. 确认错误文案和现有本地化风格一致。 + +--- + +## 8. 暂不纳入本次修复（风险提示） + +- 调试日志记录加密密钥片段（`extract_enc_key_or_arg` 在 debug 级别记录 key 前后缀） +- `encryption_key` 作为工具参数带来的日志/对话暴露面 + +**处理方式**: 记录为"接口安全风险提示"，待后续单独决定是否收紧 debug 日志、调整工具描述或限制参数传 key 的路径。 + +--- + +## 实施顺序 + +1. `secrets-core` 导入/导出链路 +2. `secrets-core` env map +3. `secrets-core` rollback +4. `secrets-core` API key +5. `secrets-mcp` tools +6. `secrets-mcp` Web 会话校验 +7. `secrets-mcp` Web 输入校验 + +**原因**: 先修 core 语义和返回结构，再修上层接入。`svc_add` 返回结构、rollback 语义、export/import 格式都属于底层契约，适合先稳定。 + +--- + +## 验证与收尾 + +1. 先跑相关单元/集成测试。 +2. 再跑全量： + ```bash + cargo fmt -- --check + cargo clippy --locked -- -D warnings + cargo test --locked + ``` +3. 若涉及 `crates/**` 的实际改动，按 `AGENTS.md` 约定检查版本/tag，并执行 `./scripts/release-check.sh`。