docs: add code review fixes plan
This commit is contained in:
voson
201
plans/code-review-fixes-2026-04-11.md
Normal file
201
plans/code-review-fixes-2026-04-11.md
Normal file
@@ -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<BTreeMap<String, String>>`，键为 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`。
|
||||
Reference in New Issue
Block a user