From c3b1a0df1a18746131b28f80e0a1d7a6c18adcdf Mon Sep 17 00:00:00 2001 From: voson Date: Sat, 11 Apr 2026 17:04:21 +0800 Subject: [PATCH] =?UTF-8?q?merge:=20code-review=20fixes=20(d7720662=20base?= =?UTF-8?q?line=20+=209f8a=20=E6=96=87=E6=A1=88=E5=B8=B8=E9=87=8F=E5=8C=96?= =?UTF-8?q?=E3=80=81env=5Fprefix=20=E6=B5=8B=E8=AF=95=E3=80=81=E8=A1=A5?= =?UTF-8?q?=E5=85=85=E7=94=A8=E4=BE=8B);=20secrets-mcp=200.5.21?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - default 已 rebase 到 d7720662；合并说明见 plans/merge-code-review-fixes-2026-04-11.md - Web PATCH 长度错误用 validation 常量拼接；env_map 单测；import/api_key 单测 - rustfmt 收尾 --- Cargo.lock | 2 +- crates/secrets-core/src/service/api_key.rs | 27 ++++ crates/secrets-core/src/service/env_map.rs | 27 +++- crates/secrets-core/src/service/import.rs | 47 +++++++ crates/secrets-mcp/Cargo.toml | 2 +- crates/secrets-mcp/src/web/entries.rs | 56 ++++++-- plans/merge-code-review-fixes-2026-04-11.md | 143 ++++++++++++++++++++ 7 files changed, 289 insertions(+), 15 deletions(-) create mode 100644 plans/merge-code-review-fixes-2026-04-11.md diff --git a/Cargo.lock b/Cargo.lock index 3000cf6..2c4fcc9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2065,7 +2065,7 @@ dependencies = [ [[package]] name = "secrets-mcp" -version = "0.5.20" +version = "0.5.21" dependencies = [ "anyhow", "askama", diff --git a/crates/secrets-core/src/service/api_key.rs b/crates/secrets-core/src/service/api_key.rs index 1d7d635..abed3cc 100644 --- a/crates/secrets-core/src/service/api_key.rs +++ b/crates/secrets-core/src/service/api_key.rs @@ -66,3 +66,30 @@ pub async fn validate_api_key(pool: &PgPool, raw_key: &str) -> Result(), + Some(AppError::NotFoundUser) + )); + } +} diff --git a/crates/secrets-core/src/service/env_map.rs b/crates/secrets-core/src/service/env_map.rs index f8d9399..34a3c00 100644 --- a/crates/secrets-core/src/service/env_map.rs +++ b/crates/secrets-core/src/service/env_map.rs @@ -87,11 +87,36 @@ fn json_to_env_string(v: &Value) -> String { #[cfg(test)] mod tests { - use super::secret_name_to_env_segment; + use serde_json::Value; + + use super::{env_prefix, secret_name_to_env_segment}; + use crate::models::Entry; #[test] fn secret_name_env_segment_disambiguates_dot_from_underscore() { assert_eq!(secret_name_to_env_segment("db.password"), "DB__PASSWORD"); assert_eq!(secret_name_to_env_segment("db_password"), "DB_PASSWORD"); + assert_eq!(secret_name_to_env_segment("api-key"), "API_KEY"); + } + + #[test] + fn env_prefix_with_and_without_prefix() { + let entry = Entry { + id: uuid::Uuid::new_v4(), + user_id: None, + folder: "test".into(), + entry_type: "server".into(), + name: "my-server".into(), + notes: String::new(), + tags: vec![], + metadata: Value::Null, + version: 1, + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + deleted_at: None, + }; + assert_eq!(env_prefix(&entry, ""), "MY_SERVER"); + assert_eq!(env_prefix(&entry, "ALIYUN"), "ALIYUN_MY_SERVER"); + assert_eq!(env_prefix(&entry, "aliyun_"), "ALIYUN_MY_SERVER"); } } diff --git a/crates/secrets-core/src/service/import.rs b/crates/secrets-core/src/service/import.rs index 83b8e1f..7aeec75 100644 --- a/crates/secrets-core/src/service/import.rs +++ b/crates/secrets-core/src/service/import.rs @@ -131,3 +131,50 @@ pub async fn run( dry_run: params.dry_run, }) } + +#[cfg(test)] +mod tests { + use std::collections::{BTreeMap, HashMap}; + + use crate::models::ExportEntry; + + /// Mirrors the map built in `run` before `AddParams` (legacy files omit `secret_types`). + fn secret_types_for_add(entry: &ExportEntry) -> HashMap { + entry + .secret_types + .as_ref() + .map(|m| m.iter().map(|(k, v)| (k.clone(), v.clone())).collect()) + .unwrap_or_default() + } + + #[test] + fn secret_types_three_kinds_round_trip_for_add_params() { + let mut types = BTreeMap::new(); + types.insert("p".into(), "password".into()); + types.insert("k".into(), "key".into()); + types.insert("t".into(), "text".into()); + let entry = ExportEntry { + name: "n".into(), + folder: "f".into(), + entry_type: "ty".into(), + notes: "".into(), + tags: vec![], + metadata: serde_json::json!({}), + secrets: Some(BTreeMap::new()), + secret_types: Some(types), + }; + let m = secret_types_for_add(&entry); + assert_eq!(m.get("p").map(String::as_str), Some("password")); + assert_eq!(m.get("k").map(String::as_str), Some("key")); + assert_eq!(m.get("t").map(String::as_str), Some("text")); + } + + #[test] + fn secret_types_absent_defaults_to_empty_map_like_legacy_export() { + let json = + r#"{"name":"a","folder":"","type":"","notes":"","tags":[],"metadata":{},"secrets":{}}"#; + let entry: ExportEntry = serde_json::from_str(json).unwrap(); + assert!(entry.secret_types.is_none()); + assert!(secret_types_for_add(&entry).is_empty()); + } +} diff --git a/crates/secrets-mcp/Cargo.toml b/crates/secrets-mcp/Cargo.toml index ee90cd8..9907705 100644 --- a/crates/secrets-mcp/Cargo.toml +++ b/crates/secrets-mcp/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "secrets-mcp" -version = "0.5.20" +version = "0.5.21" edition.workspace = true [[bin]] diff --git a/crates/secrets-mcp/src/web/entries.rs b/crates/secrets-mcp/src/web/entries.rs index f1654c7..3fc1a2c 100644 --- a/crates/secrets-mcp/src/web/entries.rs +++ b/crates/secrets-mcp/src/web/entries.rs @@ -636,33 +636,65 @@ pub(super) async fn api_entry_patch( if folder.chars().count() > crate::validation::MAX_FOLDER_LENGTH { return Err(( StatusCode::BAD_REQUEST, - Json( - json!({ "error": tr(lang, "folder 长度不能超过 128 个字符", "folder 長度不能超過 128 個字元", "folder must be at most 128 characters") }), - ), + Json(json!({ "error": format!( + "{} {} {}", + tr( + lang, + "folder 长度不能超过", + "folder 長度不能超過", + "folder must be at most" + ), + crate::validation::MAX_FOLDER_LENGTH, + tr(lang, " 个字符", " 個字元", " characters") + ) })), )); } if entry_type.chars().count() > crate::validation::MAX_ENTRY_TYPE_LENGTH { return Err(( StatusCode::BAD_REQUEST, - Json( - json!({ "error": tr(lang, "type 长度不能超过 64 个字符", "type 長度不能超過 64 個字元", "type must be at most 64 characters") }), - ), + Json(json!({ "error": format!( + "{} {} {}", + tr( + lang, + "type 长度不能超过", + "type 長度不能超過", + "type must be at most" + ), + crate::validation::MAX_ENTRY_TYPE_LENGTH, + tr(lang, " 个字符", " 個字元", " characters") + ) })), )); } if name.chars().count() > crate::validation::MAX_NAME_LENGTH { return Err(( StatusCode::BAD_REQUEST, - Json( - json!({ "error": tr(lang, "name 长度不能超过 256 个字符", "name 長度不能超過 256 個字元", "name must be at most 256 characters") }), - ), + Json(json!({ "error": format!( + "{} {} {}", + tr( + lang, + "name 长度不能超过", + "name 長度不能超過", + "name must be at most" + ), + crate::validation::MAX_NAME_LENGTH, + tr(lang, " 个字符", " 個字元", " characters") + ) })), )); } if notes.chars().count() > crate::validation::MAX_NOTES_LENGTH { return Err(( StatusCode::BAD_REQUEST, - Json( - json!({ "error": tr(lang, "notes 长度不能超过 10000 个字符", "notes 長度不能超過 10000 個字元", "notes must be at most 10000 characters") }), - ), + Json(json!({ "error": format!( + "{} {} {}", + tr( + lang, + "notes 长度不能超过", + "notes 長度不能超過", + "notes must be at most" + ), + crate::validation::MAX_NOTES_LENGTH, + tr(lang, " 个字符", " 個字元", " characters") + ) })), )); } diff --git a/plans/merge-code-review-fixes-2026-04-11.md b/plans/merge-code-review-fixes-2026-04-11.md new file mode 100644 index 0000000..2b99f3e --- /dev/null +++ b/plans/merge-code-review-fixes-2026-04-11.md @@ -0,0 +1,143 @@ +# Code Review 修复方案合并计划 + +**日期**: 2026-04-11 +**来源**: 两个 AI 实现对比评估 +**比较对象**: +- `d7720662` (`/Users/voson/work/refining/secrets-cr-fixes-ws`) +- `9f8a68cd` (`/Users/voson/work/refining/secrets/plan-impl`) + +--- + +## 结论 + +以 **`d7720662`** 为主线采纳。 + +**原因**: +1. `rollback` 的 live row 加锁与 snapshot 读取都在事务内完成，更符合原计划里对 TOCTOU 的修复要求。 +2. Web JSON API 的 session 校验保留了按 `UiLang` 返回错误信息的行为，没有把错误消息退化成固定英文。 +3. `svc_add` 返回 `entry_id`，MCP 层直接使用返回值建立 parent relation，和计划第 5 项更一致。 + +--- + +## 采纳策略 + +### 1. 主线保留 `d7720662` + +保留以下实现，不从另一份实现回退： + +- `crates/secrets-core/src/service/rollback.rs` +- `crates/secrets-mcp/src/web/mod.rs` +- `crates/secrets-core/src/service/add.rs` +- `crates/secrets-mcp/src/tools.rs` + +### 2. 从 `9f8a68cd` 手动吸收的小改动 + +仅吸收下面两处，手动改写，不直接整文件 cherry-pick： + +1. `crates/secrets-mcp/src/web/entries.rs` + - 把长度校验报错文案改成基于 `crate::validation::*` 常量拼接，避免上限数字硬编码在文案里。 + +2. `crates/secrets-core/src/service/env_map.rs` + - 补 `env_prefix_with_and_without_prefix` 单测。 + +--- + +## 明确不采纳的实现 + +### 不采纳 `9f8a68cd` 的 `rollback.rs` + +原因： +- 它仍然先在事务外读取 `entries_history`，再开启事务并锁 live row。 +- 对“回滚到最近快照”的路径仍存在先读后锁的时间窗口。 + +### 不采纳 `9f8a68cd` 的 `web/mod.rs` + +原因： +- `load_session_user_strict()` / `require_valid_user_json()` 返回固定英文 JSON 错误。 +- 会丢失现有多语言错误语义。 + +### 不采纳 `9f8a68cd` 的 `AddResult.id` + +原因： +- 本轮计划里明确要求 `svc_add` 返回 `entry_id`。 +- `d7720662` 的字段命名与 MCP 使用方式更贴近计划要求。 + +--- + +## 与原计划的覆盖情况 + +两份实现都完成了大部分代码修改，但验证项整体没有补齐，当前更像“代码已改，测试仍不足”。 + +### 已基本完成 + +1. 导入/导出链路补 `secret_types` +2. env map `.` -> `__`，并在冲突时返回错误 +3. API key `rows_affected()` 检查 +4. MCP `secrets_add` 避免提交后二次回查竞态 +5. MCP `secrets_find` 不再把 count 错误吞成 `0` +6. MCP `secrets_rollback` 不再要求 encryption key +7. Web JSON API 引入 `key_version` 严格校验 +8. Web PATCH 补长度校验 + +### 尚需补齐的验证 + +1. export/import round-trip 测试 + - `password` / `key` / `text` 三种类型导出再导入后保持不变 + +2. legacy import 测试 + - 老格式缺失 `secret_types` 时默认回落到 `text` + +3. env map 测试 + - `db.password` vs `db_password` + - 带 `prefix` + - 多 entry 合并冲突 + +4. rollback 测试 + - 恢复字段是否符合预期 + - 并发更新 + 回滚不依赖过期值 + +5. `regenerate_api_key` 测试 + - 正常用户返回新 key + - 不存在用户返回错误 + +6. MCP tool 测试 + - `secrets_find` count 失败路径 + - `secrets_rollback` 无 encryption key 也可执行 + +7. Web session / validation 测试 + - `key_version` mismatch -> `401` + - 用户不存在 / session 损坏 -> 正确错误 + - `folder/type/name/notes` 超长 -> `400` + +--- + +## 执行步骤 + +1. 以 `d7720662` 对应实现为合并基线。 +2. 手动吸收 `9f8a68cd` 中 `web/entries.rs` 的常量化长度报错文案。 +3. 手动吸收 `9f8a68cd` 中 `env_map.rs` 的 `env_prefix_with_and_without_prefix` 测试。 +4. 不引入 `9f8a68cd` 的 `rollback.rs`、`web/mod.rs`、`AddResult.id` 方案。 +5. 针对原计划缺失的验证项补测试。 +6. 跑质量门禁： + +```bash +cargo fmt -- --check +cargo clippy --locked -- -D warnings +cargo test --locked +``` + +7. 跑发布前检查： + +```bash +./scripts/release-check.sh +``` + +8. 确认版本和 tag： + - `crates/secrets-mcp/Cargo.toml` 已 bump（合并执行时为 `0.5.21`，因 `crates/**` 有变更） + - `jj tag list` + +--- + +## 备注 + +如果后续要做最终合并，建议以 `d7720662` 为基础继续补测试，而不是尝试把两份实现整合成第三套逻辑。这样改动面最小，风险也最低。