From 15183883741196fe1bf01f5609bc74b201a1312a Mon Sep 17 00:00:00 2001 From: voson Date: Sat, 4 Apr 2026 17:58:12 +0800 Subject: [PATCH] chore(release): secrets-mcp 0.4.0 Bump version for the N:N entry_secrets data model and related MCP/Web changes. Remove superseded SQL migration artifacts; rely on auto-migrate. Add structured errors, taxonomy normalization, and web i18n helpers. Made-with: Cursor --- .gitignore | 5 +- AGENTS.md | 32 +- Cargo.lock | 3 +- Cargo.toml | 1 + README.md | 19 +- crates/secrets-core/Cargo.toml | 1 + crates/secrets-core/src/error.rs | 139 ++++ crates/secrets-core/src/lib.rs | 2 + crates/secrets-core/src/service/add.rs | 137 +++- crates/secrets-core/src/service/delete.rs | 569 ++++--------- crates/secrets-core/src/service/env_map.rs | 40 +- crates/secrets-core/src/service/import.rs | 1 + crates/secrets-core/src/service/search.rs | 41 +- crates/secrets-core/src/service/update.rs | 62 +- crates/secrets-core/src/taxonomy.rs | 111 +++ crates/secrets-mcp/Cargo.toml | 2 +- crates/secrets-mcp/src/error.rs | 36 + crates/secrets-mcp/src/main.rs | 1 + crates/secrets-mcp/src/tools.rs | 62 +- crates/secrets-mcp/src/web.rs | 645 +++++++++++++-- crates/secrets-mcp/templates/audit.html | 65 +- crates/secrets-mcp/templates/entries.html | 910 ++++++++++++++++++--- crates/secrets-mcp/templates/i18n.js | 76 ++ migrations/001_nn_schema.sql | 126 --- migrations/002_data_cleanup.sql | 67 -- scripts/cleanup-orphan-user-ids.sql | 22 - scripts/migrate-db-prod-to-nn-test.sh | 81 -- scripts/migrate-v0.3.0.sql | 194 ----- scripts/sync-test-to-prod.sh | 95 +++ 29 files changed, 2285 insertions(+), 1260 deletions(-) create mode 100644 crates/secrets-core/src/error.rs create mode 100644 crates/secrets-core/src/taxonomy.rs create mode 100644 crates/secrets-mcp/src/error.rs create mode 100644 crates/secrets-mcp/templates/i18n.js delete mode 100644 migrations/001_nn_schema.sql delete mode 100644 migrations/002_data_cleanup.sql delete mode 100644 scripts/cleanup-orphan-user-ids.sql delete mode 100755 scripts/migrate-db-prod-to-nn-test.sh delete mode 100644 scripts/migrate-v0.3.0.sql create mode 100755 scripts/sync-test-to-prod.sh diff --git a/.gitignore b/.gitignore index 00776a9..ab4bfc1 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,6 @@ .env .DS_Store .cursor/ -# Google OAuth 下载的 JSON 凭据文件 -client_secret_*.apps.googleusercontent.com.json *.pem -tmp/ \ No newline at end of file +tmp/ +client_secret_*.apps.googleusercontent.com.json \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 781c0db..d17254a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,13 +55,24 @@ entries ( ```sql secrets ( id UUID PRIMARY KEY DEFAULT uuidv7(), - entry_id UUID NOT NULL REFERENCES entries(id) ON DELETE CASCADE, - field_name VARCHAR(256) NOT NULL, + user_id UUID, + name VARCHAR(256) NOT NULL, + type VARCHAR(64) NOT NULL DEFAULT 'text', encrypted BYTEA NOT NULL DEFAULT '\x', version BIGINT NOT NULL DEFAULT 1, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), - updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), - UNIQUE(entry_id, field_name) + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +) +-- 唯一：UNIQUE(user_id, name) WHERE user_id IS NOT NULL +``` + +```sql +entry_secrets ( + entry_id UUID NOT NULL REFERENCES entries(id) ON DELETE CASCADE, + secret_id UUID NOT NULL REFERENCES secrets(id) ON DELETE CASCADE, + sort_order INT NOT NULL DEFAULT 0, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + PRIMARY KEY(entry_id, secret_id) ) ``` @@ -108,17 +119,20 @@ oauth_accounts ( | 字段 | 含义 | 示例 | |------|------|------| | `folder` | 隔离空间（参与唯一键） | `refining` | -| `type` | 软分类（不参与唯一键） | `server`, `service`, `key`, `person` | +| `type` | 软分类（不参与唯一键） | `server`, `service`, `person`, `document` | | `name` | 标识名 | `gitea`, `aliyun` | | `notes` | 非敏感说明 | 自由文本 | | `tags` | 标签 | `["aliyun","prod"]` | -| `metadata` | 明文描述 | `ip`、`url`、`key_ref` | -| `secrets.field_name` | 加密字段名（明文） | `token`, `ssh_key` | +| `metadata` | 明文描述 | `ip`、`url`、`subtype` | +| `secrets.name` | 密钥名称（调用方提供） | `token`, `ssh_key`, `password` | +| `secrets.type` | 密钥类型（调用方提供，默认 `text`） | `text`, `password`, `key` | | `secrets.encrypted` | 密文 | AES-GCM | -### PEM 共享（`key_ref`） +### 共享密钥（N:N 关联） -建议将共享 PEM 存为 **`type=key`** 的 entry；其它记录在 `metadata.key_ref` 指向目标 entry 的 `name`（支持 `folder/name` 格式消歧）。删除被引用 key 时，服务会自动迁移为单副本 + 重定向（复制到首个引用方，其余引用方改指向新 owner）；解析逻辑见 `secrets_core::service::env_map`。 +多个 entry 可共享同一 secret 字段，通过 `entry_secrets` 中间表关联。 +添加条目时通过 `link_secret_names` 参数指定要关联的已有 secret name（按 `(user_id, name)` 精确匹配）。 +删除 entry 时仅解除关联，secret 本身若仍被引用则保留；不再被任何 entry 引用时自动清理。 ## 代码规范 diff --git a/Cargo.lock b/Cargo.lock index 4ce2755..c00190c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1960,6 +1960,7 @@ dependencies = [ "sha2", "sqlx", "tempfile", + "thiserror", "tokio", "toml", "tracing", @@ -1968,7 +1969,7 @@ dependencies = [ [[package]] name = "secrets-mcp" -version = "0.3.9" +version = "0.4.0" dependencies = [ "anyhow", "askama", diff --git a/Cargo.toml b/Cargo.toml index e103bf6..0e34dfe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,7 @@ rand = "^0.10.0" # Utils anyhow = "^1.0.102" +thiserror = "^2" chrono = { version = "^0.4.44", features = ["serde"] } uuid = { version = "^1.22.0", features = ["serde"] } tracing = "^0.1" diff --git a/README.md b/README.md index 68b4d81..69fdaaf 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,7 @@ SECRETS_ENV=production - **`secrets_search`**：发现条目（可按 query / folder / type / name 过滤）；不要求加密头。 - **`secrets_get` / `secrets_update` / `secrets_delete`（按 name）/ `secrets_history` / `secrets_rollback`**：仅 `name` 且全局唯一则直接命中；若多条同名，返回消歧错误，需在参数中补 **`folder`**。 - **`secrets_delete`**：`dry_run=true` 时与真实删除相同的消歧规则——唯一则预览一条，多条则报错并要求 `folder`。 -- **共享 key 自动迁移删除**：删除仍被 `metadata.key_ref` 引用的 key 条目时，系统会自动迁移：把密文复制到首个引用方，并将其余引用方的 `key_ref` 重定向到新 owner，然后继续删除。 +- **共享密钥**：N:N 关联下，删除 entry 仅解除关联，被共享的 secret 若仍被其他 entry 引用则保留；无引用时自动清理。 ## 加密架构（混合 E2EE） @@ -151,25 +151,28 @@ flowchart LR ## 数据模型 -主表 **`entries`**（`folder`、`type`、`name`、`notes`、`tags`、`metadata`，多租户时带 `user_id`）+ 子表 **`secrets`**（每行一个加密字段：`field_name`、`encrypted`）。**唯一性**：`UNIQUE(user_id, folder, name)`（`user_id` 为空时为遗留行唯一 `(folder, name)`）。另有 `entries_history`、`secrets_history`、`audit_log`，以及 **`users`**（含 `key_salt`、`key_check`、`key_params`、`api_key`）、**`oauth_accounts`**。首次连库自动迁移建表（`secrets-core` 的 `migrate`）；已有库可对照 [`scripts/migrate-v0.3.0.sql`](scripts/migrate-v0.3.0.sql) 做列重命名与索引重建。**Web 登录会话**（tower-sessions）使用同一 `SECRETS_DATABASE_URL`，进程启动时对会话存储执行迁移（见 `secrets-mcp` 中 `PostgresStore::migrate`），无需额外环境变量。 +主表 **`entries`**（`folder`、`type`、`name`、`notes`、`tags`、`metadata`，多租户时带 `user_id`）+ 子表 **`secrets`**（每行一个加密字段：`name`、`type`、`encrypted`，通过 `entry_secrets` 中间表与 entry 建立 N:N 关联）。**唯一性**：`UNIQUE(user_id, folder, name)`（`user_id` 为空时为遗留行唯一 `(folder, name)`）。另有 `entries_history`、`secrets_history`、`audit_log`，以及 **`users`**（含 `key_salt`、`key_check`、`key_params`、`api_key`）、**`oauth_accounts`**。首次连库自动迁移建表（`secrets-core` 的 `migrate`）；已有库可对照 [`scripts/migrate-v0.3.0.sql`](scripts/migrate-v0.3.0.sql) 做列重命名与索引重建。**Web 登录会话**（tower-sessions）使用同一 `SECRETS_DATABASE_URL`，进程启动时对会话存储执行迁移（见 `secrets-mcp` 中 `PostgresStore::migrate`），无需额外环境变量。 | 位置 | 字段 | 说明 | |------|------|------| | entries | folder | 组织/隔离空间，如 `refining`、`ricnsmart`；参与唯一键 | -| entries | type | 软分类，如 `server`、`service`、`key`、`person`（可扩展，不参与唯一键） | +| entries | type | 软分类，如 `server`、`service`、`person`、`document`（可扩展，不参与唯一键） | | entries | name | 人类可读标识；与 `folder` 一起在用户内唯一 | | entries | notes | 非敏感说明文本 | -| entries | metadata | 明文 JSON（ip、url、`key_ref` 等） | -| secrets | field_name | 明文字段名，便于 schema 展示 | +| entries | metadata | 明文 JSON（ip、url、subtype 等） | +| secrets | name | 密钥名称（调用方提供） | +| secrets | type | 密钥类型（调用方提供，默认 `text`） | | secrets | encrypted | AES-GCM 密文（含 nonce） | | users | key_salt | PBKDF2 salt（32B），首次设置密码短语时写入 | | users | key_check | 派生密钥加密已知常量，用于验证密码短语 | | users | key_params | 派生算法参数，如 `{"alg":"pbkdf2-sha256","iterations":600000}` | -### PEM 共享（`key_ref`） +### 共享密钥（N:N 关联） -同一 PEM 可被多条 `server` 等记录引用：建议将 PEM 存为 **`type=key`** 的 entry，在其它条目的 `metadata.key_ref` 中写目标 entry 的 `name`（支持 `folder/name` 格式消歧）；轮换时只更新该目标记录即可。 -删除共享 key 时，系统会自动迁移引用：将密文复制到首个引用方（单副本），其余引用方的 `key_ref` 自动重定向到该新 owner，再删除原 key 记录。 +多个条目可共享同一密文字段，通过 `entry_secrets` 中间表实现 N:N 关联： +- 添加条目时可通过 `link_secret_names` 参数关联已有的 secret（按 `(user_id, name)` 精确匹配查找） +- 同一 secret 可被多个 entry 引用，删除某 entry 不会级联删除被共享的 secret +- 当 secret 不再被任何 entry 引用时，自动清理（`NOT EXISTS` 子查询） ## 审计日志 diff --git a/crates/secrets-core/Cargo.toml b/crates/secrets-core/Cargo.toml index 1772c31..da0b3bf 100644 --- a/crates/secrets-core/Cargo.toml +++ b/crates/secrets-core/Cargo.toml @@ -10,6 +10,7 @@ path = "src/lib.rs" [dependencies] aes-gcm.workspace = true anyhow.workspace = true +thiserror.workspace = true chrono.workspace = true rand.workspace = true serde.workspace = true diff --git a/crates/secrets-core/src/error.rs b/crates/secrets-core/src/error.rs new file mode 100644 index 0000000..767ea1f --- /dev/null +++ b/crates/secrets-core/src/error.rs @@ -0,0 +1,139 @@ +use sqlx::error::DatabaseError; + +/// Structured business errors for the secrets service. +/// +/// These replace ad-hoc `anyhow` strings for expected failure modes, +/// allowing MCP and Web layers to map to appropriate protocol-level errors. +#[derive(Debug, thiserror::Error)] +pub enum AppError { + #[error("A secret with the name '{secret_name}' already exists for this user")] + ConflictSecretName { secret_name: String }, + + #[error("An entry with folder='{folder}' and name='{name}' already exists")] + ConflictEntryName { folder: String, name: String }, + + #[error("Entry not found")] + NotFoundEntry, + + #[error("Validation failed: {message}")] + Validation { message: String }, + + #[error("Concurrent modification detected")] + ConcurrentModification, + + #[error(transparent)] + Internal(#[from] anyhow::Error), +} + +impl AppError { + /// Try to convert a sqlx database error into a structured `AppError`. + /// + /// The caller should provide the context (which table was being written, + /// what values were being inserted) so we can produce a meaningful error. + pub fn from_db_error(err: sqlx::Error, ctx: DbErrorContext<'_>) -> Self { + if let sqlx::Error::Database(ref db_err) = err + && db_err.code().as_deref() == Some("23505") + { + return Self::from_unique_violation(db_err.as_ref(), ctx); + } + AppError::Internal(err.into()) + } + + fn from_unique_violation(db_err: &dyn DatabaseError, ctx: DbErrorContext<'_>) -> Self { + let constraint = db_err.constraint(); + + match constraint { + Some("idx_secrets_unique_user_name") => AppError::ConflictSecretName { + secret_name: ctx.secret_name.unwrap_or("unknown").to_string(), + }, + Some("idx_entries_unique_user") | Some("idx_entries_unique_legacy") => { + AppError::ConflictEntryName { + folder: ctx.folder.unwrap_or("").to_string(), + name: ctx.name.unwrap_or("unknown").to_string(), + } + } + _ => { + // Fall back to message-based detection for unnamed constraints + let msg = db_err.message(); + if msg.contains("secrets") { + AppError::ConflictSecretName { + secret_name: ctx.secret_name.unwrap_or("unknown").to_string(), + } + } else { + AppError::ConflictEntryName { + folder: ctx.folder.unwrap_or("").to_string(), + name: ctx.name.unwrap_or("unknown").to_string(), + } + } + } + } + } +} + +/// Context hints used when converting a database error to `AppError`. +#[derive(Debug, Default, Clone, Copy)] +pub struct DbErrorContext<'a> { + pub secret_name: Option<&'a str>, + pub folder: Option<&'a str>, + pub name: Option<&'a str>, +} + +impl<'a> DbErrorContext<'a> { + pub fn secret_name(name: &'a str) -> Self { + Self { + secret_name: Some(name), + ..Default::default() + } + } + + pub fn entry(folder: &'a str, name: &'a str) -> Self { + Self { + folder: Some(folder), + name: Some(name), + ..Default::default() + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn app_error_display_messages() { + let err = AppError::ConflictSecretName { + secret_name: "token".to_string(), + }; + assert!(err.to_string().contains("token")); + + let err = AppError::ConflictEntryName { + folder: "refining".to_string(), + name: "gitea".to_string(), + }; + assert!(err.to_string().contains("refining")); + assert!(err.to_string().contains("gitea")); + + let err = AppError::NotFoundEntry; + assert_eq!(err.to_string(), "Entry not found"); + + let err = AppError::Validation { + message: "too long".to_string(), + }; + assert!(err.to_string().contains("too long")); + + let err = AppError::ConcurrentModification; + assert!(err.to_string().contains("Concurrent modification")); + } + + #[test] + fn db_error_context_helpers() { + let ctx = DbErrorContext::secret_name("my_key"); + assert_eq!(ctx.secret_name, Some("my_key")); + assert!(ctx.folder.is_none()); + + let ctx = DbErrorContext::entry("prod", "db-creds"); + assert_eq!(ctx.folder, Some("prod")); + assert_eq!(ctx.name, Some("db-creds")); + assert!(ctx.secret_name.is_none()); + } +} diff --git a/crates/secrets-core/src/lib.rs b/crates/secrets-core/src/lib.rs index 5c81284..b38e1a3 100644 --- a/crates/secrets-core/src/lib.rs +++ b/crates/secrets-core/src/lib.rs @@ -2,5 +2,7 @@ pub mod audit; pub mod config; pub mod crypto; pub mod db; +pub mod error; pub mod models; pub mod service; +pub mod taxonomy; diff --git a/crates/secrets-core/src/service/add.rs b/crates/secrets-core/src/service/add.rs index 65b77cf..4e1a53f 100644 --- a/crates/secrets-core/src/service/add.rs +++ b/crates/secrets-core/src/service/add.rs @@ -7,7 +7,9 @@ use uuid::Uuid; use crate::crypto; use crate::db; +use crate::error::{AppError, DbErrorContext}; use crate::models::EntryRow; +use crate::taxonomy; // ── Key/value parsing helpers ───────────────────────────────────────────────── @@ -177,13 +179,19 @@ pub struct AddParams<'a> { pub tags: &'a [String], pub meta_entries: &'a [String], pub secret_entries: &'a [String], + pub secret_types: &'a std::collections::HashMap, pub link_secret_names: &'a [String], /// Optional user_id for multi-user isolation (None = single-user CLI mode) pub user_id: Option, } pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> Result { - let metadata = build_json(params.meta_entries)?; + let Value::Object(mut metadata_map) = build_json(params.meta_entries)? else { + unreachable!("build_json always returns a JSON object"); + }; + let normalized_entry_type = + taxonomy::normalize_entry_type_and_metadata(params.entry_type, &mut metadata_map); + let metadata = Value::Object(metadata_map); let secret_json = build_json(params.secret_entries)?; let meta_keys = collect_key_paths(params.meta_entries)?; let secret_keys = collect_key_paths(params.secret_entries)?; @@ -224,7 +232,7 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> entry_id: ex.id, user_id: params.user_id, folder: params.folder, - entry_type: params.entry_type, + entry_type: &normalized_entry_type, name: params.name, version: ex.version, action: "add", @@ -254,7 +262,7 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> ) .bind(uid) .bind(params.folder) - .bind(params.entry_type) + .bind(&normalized_entry_type) .bind(params.name) .bind(params.notes) .bind(params.tags) @@ -277,7 +285,7 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> RETURNING id"#, ) .bind(params.folder) - .bind(params.entry_type) + .bind(&normalized_entry_type) .bind(params.name) .bind(params.notes) .bind(params.tags) @@ -299,7 +307,7 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> entry_id, user_id: params.user_id, folder: params.folder, - entry_type: params.entry_type, + entry_type: &normalized_entry_type, name: params.name, version: current_entry_version, action: "create", @@ -345,30 +353,42 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> } } + let orphan_candidates: Vec = existing_fields.iter().map(|f| f.id).collect(); + sqlx::query("DELETE FROM entry_secrets WHERE entry_id = $1") .bind(entry_id) .execute(&mut *tx) .await?; - sqlx::query( - "DELETE FROM secrets s \ - WHERE NOT EXISTS (SELECT 1 FROM entry_secrets es WHERE es.secret_id = s.id)", - ) - .execute(&mut *tx) - .await?; + if !orphan_candidates.is_empty() { + sqlx::query( + "DELETE FROM secrets s \ + WHERE s.id = ANY($1) \ + AND NOT EXISTS (SELECT 1 FROM entry_secrets es WHERE es.secret_id = s.id)", + ) + .bind(&orphan_candidates) + .execute(&mut *tx) + .await?; + } } for (field_name, field_value) in &flat_fields { let encrypted = crypto::encrypt_json(master_key, field_value)?; + let secret_type = params + .secret_types + .get(field_name) + .map(|s| s.as_str()) + .unwrap_or("text"); let secret_id: Uuid = sqlx::query_scalar( "INSERT INTO secrets (user_id, name, type, encrypted) VALUES ($1, $2, $3, $4) RETURNING id", ) .bind(params.user_id) .bind(field_name) - .bind(infer_secret_type(field_name)) + .bind(secret_type) .bind(&encrypted) .fetch_one(&mut *tx) - .await?; + .await + .map_err(|e| AppError::from_db_error(e, DbErrorContext::secret_name(field_name)))?; sqlx::query("INSERT INTO entry_secrets (entry_id, secret_id) VALUES ($1, $2)") .bind(entry_id) .bind(secret_id) @@ -414,7 +434,7 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> params.user_id, "add", params.folder, - params.entry_type, + &normalized_entry_type, params.name, serde_json::json!({ "tags": params.tags, @@ -429,32 +449,13 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) -> Ok(AddResult { name: params.name.to_string(), folder: params.folder.to_string(), - entry_type: params.entry_type.to_string(), + entry_type: normalized_entry_type, tags: params.tags.to_vec(), meta_keys, secret_keys, }) } -pub(crate) fn infer_secret_type(name: &str) -> &'static str { - match name { - "ssh_key" => "pem", - "password" => "password", - "phone" | "phone_2" => "phone", - "webhook_url" | "address" => "url", - "access_key_id" - | "access_key_secret" - | "global_api_key" - | "api_key" - | "secret_key" - | "personal_access_token" - | "runner_token" - | "GOOGLE_CLIENT_ID" - | "GOOGLE_CLIENT_SECRET" => "token", - _ => "text", - } -} - fn validate_link_secret_names( link_secret_names: &[String], new_secret_names: &BTreeSet, @@ -601,6 +602,7 @@ mod tests { tags: &[], meta_entries: &[], secret_entries: &[], + secret_types: &Default::default(), link_secret_names: std::slice::from_ref(&secret_name), user_id: None, }, @@ -647,6 +649,7 @@ mod tests { tags: &[], meta_entries: &[], secret_entries: &[], + secret_types: &Default::default(), link_secret_names: std::slice::from_ref(&secret_name), user_id: None, }, @@ -697,6 +700,7 @@ mod tests { tags: &[], meta_entries: &[], secret_entries: &[], + secret_types: &Default::default(), link_secret_names: std::slice::from_ref(&secret_name), user_id: None, }, @@ -709,4 +713,69 @@ mod tests { cleanup_test_rows(&pool, &marker).await?; Ok(()) } + + #[tokio::test] + async fn add_duplicate_secret_name_returns_conflict_error() -> Result<()> { + let Some(pool) = maybe_test_pool().await else { + return Ok(()); + }; + let suffix = Uuid::from_u128(rand::random()).to_string(); + let marker = format!("dup_secret_{}", &suffix[..8]); + let entry_name = format!("{}_entry", marker); + let secret_name = "shared_token"; + + cleanup_test_rows(&pool, &marker).await?; + + // First add succeeds + run( + &pool, + AddParams { + name: &entry_name, + folder: &marker, + entry_type: "service", + notes: "", + tags: &[], + meta_entries: &[], + secret_entries: &[format!("{}=value1", secret_name)], + secret_types: &Default::default(), + link_secret_names: &[], + user_id: None, + }, + &[0_u8; 32], + ) + .await?; + + // Second add with same secret name under same user_id should fail with ConflictSecretName + let entry_name2 = format!("{}_entry2", marker); + let err = run( + &pool, + AddParams { + name: &entry_name2, + folder: &marker, + entry_type: "service", + notes: "", + tags: &[], + meta_entries: &[], + secret_entries: &[format!("{}=value2", secret_name)], + secret_types: &Default::default(), + link_secret_names: &[], + user_id: None, + }, + &[0_u8; 32], + ) + .await + .expect_err("must fail on duplicate secret name"); + + let app_err = err + .downcast_ref::() + .expect("error should be AppError"); + assert!( + matches!(app_err, crate::error::AppError::ConflictSecretName { .. }), + "expected ConflictSecretName, got: {}", + app_err + ); + + cleanup_test_rows(&pool, &marker).await?; + Ok(()) + } } diff --git a/crates/secrets-core/src/service/delete.rs b/crates/secrets-core/src/service/delete.rs index b162cc4..86ef975 100644 --- a/crates/secrets-core/src/service/delete.rs +++ b/crates/secrets-core/src/service/delete.rs @@ -17,7 +17,6 @@ pub struct DeletedEntry { #[derive(Debug, serde::Serialize)] pub struct DeleteResult { pub deleted: Vec, - pub migrated: Vec, pub dry_run: bool, } @@ -32,174 +31,6 @@ pub struct DeleteParams<'a> { pub user_id: Option, } -#[derive(Debug, sqlx::FromRow)] -struct KeyReferrer { - id: Uuid, - folder: String, - #[sqlx(rename = "type")] - entry_type: String, - name: String, -} - -fn ref_label(r: &KeyReferrer) -> String { - format!("{}/{} ({})", r.folder, r.name, r.entry_type) -} - -fn ref_path(r: &KeyReferrer) -> String { - format!("{}/{}", r.folder, r.name) -} - -async fn fetch_key_referrers_pool( - pool: &PgPool, - key_entry_id: Uuid, - key_folder: &str, - key_name: &str, - user_id: Option, -) -> Result> { - let qualified = format!("{}/{}", key_folder, key_name); - let refs: Vec = if let Some(uid) = user_id { - sqlx::query_as( - "SELECT id, folder, type, name FROM entries \ - WHERE user_id = $1 AND id <> $2 \ - AND (metadata->>'key_ref' = $3 OR metadata->>'key_ref' = $4) \ - ORDER BY folder, type, name", - ) - .bind(uid) - .bind(key_entry_id) - .bind(key_name) - .bind(&qualified) - .fetch_all(pool) - .await? - } else { - sqlx::query_as( - "SELECT id, folder, type, name FROM entries \ - WHERE user_id IS NULL AND id <> $1 \ - AND (metadata->>'key_ref' = $2 OR metadata->>'key_ref' = $3) \ - ORDER BY folder, type, name", - ) - .bind(key_entry_id) - .bind(key_name) - .bind(&qualified) - .fetch_all(pool) - .await? - }; - Ok(refs) -} - -async fn migrate_key_refs_if_needed( - tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, - key_row: &EntryRow, - key_name: &str, - user_id: Option, - dry_run: bool, -) -> Result> { - let qualified = format!("{}/{}", key_row.folder, key_name); - let refs: Vec = if let Some(uid) = user_id { - sqlx::query_as( - "SELECT id, folder, type, name FROM entries \ - WHERE user_id = $1 AND id <> $2 \ - AND (metadata->>'key_ref' = $3 OR metadata->>'key_ref' = $4) \ - ORDER BY folder, type, name", - ) - .bind(uid) - .bind(key_row.id) - .bind(key_name) - .bind(&qualified) - .fetch_all(&mut **tx) - .await? - } else { - sqlx::query_as( - "SELECT id, folder, type, name FROM entries \ - WHERE user_id IS NULL AND id <> $1 \ - AND (metadata->>'key_ref' = $2 OR metadata->>'key_ref' = $3) \ - ORDER BY folder, type, name", - ) - .bind(key_row.id) - .bind(key_name) - .bind(&qualified) - .fetch_all(&mut **tx) - .await? - }; - - if refs.is_empty() { - return Ok(vec![]); - } - if dry_run { - return Ok(refs.iter().map(ref_label).collect()); - } - - let owner = &refs[0]; - let owner_path = ref_path(owner); - let key_fields: Vec = sqlx::query_as( - "SELECT s.id, s.name, s.encrypted \ - FROM entry_secrets es \ - JOIN secrets s ON s.id = es.secret_id \ - WHERE es.entry_id = $1", - ) - .bind(key_row.id) - .fetch_all(&mut **tx) - .await?; - - for f in &key_fields { - sqlx::query("INSERT INTO entry_secrets (entry_id, secret_id) VALUES ($1, $2) ON CONFLICT DO NOTHING") - .bind(owner.id) - .bind(f.id) - .execute(&mut **tx) - .await?; - } - - sqlx::query( - "UPDATE entries SET metadata = metadata - 'key_ref', \ - version = version + 1, updated_at = NOW() WHERE id = $1", - ) - .bind(owner.id) - .execute(&mut **tx) - .await?; - - crate::audit::log_tx( - tx, - user_id, - "key_migrate", - &owner.folder, - &owner.entry_type, - &owner.name, - json!({ - "from_key": format!("{}/{}", key_row.folder, key_name), - "role": "new_owner", - "redirect_target": owner_path, - }), - ) - .await; - - for r in refs.iter().skip(1) { - sqlx::query( - "UPDATE entries SET metadata = jsonb_set(metadata, '{key_ref}', to_jsonb($2::text), true), \ - version = version + 1, updated_at = NOW() WHERE id = $1", - ) - .bind(r.id) - .bind(&owner_path) - .execute(&mut **tx) - .await?; - - crate::audit::log_tx( - tx, - user_id, - "key_migrate", - &r.folder, - &r.entry_type, - &r.name, - json!({ - "from_key": format!("{}/{}", key_row.folder, key_name), - "role": "redirected_ref", - "redirect_to": owner_path, - }), - ) - .await; - } - - Ok(refs.iter().map(ref_label).collect()) -} - /// Delete a single entry by id (multi-tenant: `user_id` must match). pub async fn delete_by_id(pool: &PgPool, entry_id: Uuid, user_id: Uuid) -> Result { let mut tx = pool.begin().await?; @@ -224,8 +55,6 @@ pub async fn delete_by_id(pool: &PgPool, entry_id: Uuid, user_id: Uuid) -> Resul let entry_type = row.entry_type.clone(); let name = row.name.clone(); let entry_row: EntryRow = (&row).into(); - let migrated = - migrate_key_refs_if_needed(&mut tx, &entry_row, &name, Some(user_id), false).await?; snapshot_and_delete( &mut tx, @@ -254,7 +83,6 @@ pub async fn delete_by_id(pool: &PgPool, entry_id: Uuid, user_id: Uuid) -> Resul folder, entry_type, }], - migrated, dry_run: false, }) } @@ -294,6 +122,7 @@ async fn delete_one( // - 2+ matches → disambiguation error (same as non-dry-run) #[derive(sqlx::FromRow)] struct DryRunRow { + #[allow(dead_code)] id: Uuid, folder: String, #[sqlx(rename = "type")] @@ -339,20 +168,16 @@ async fn delete_one( return match rows.len() { 0 => Ok(DeleteResult { deleted: vec![], - migrated: vec![], dry_run: true, }), 1 => { let row = rows.into_iter().next().unwrap(); - let refs = - fetch_key_referrers_pool(pool, row.id, &row.folder, name, user_id).await?; Ok(DeleteResult { deleted: vec![DeletedEntry { name: name.to_string(), folder: row.folder, entry_type: row.entry_type, }], - migrated: refs.iter().map(ref_label).collect(), dry_run: true, }) } @@ -417,7 +242,6 @@ async fn delete_one( tx.rollback().await?; return Ok(DeleteResult { deleted: vec![], - migrated: vec![], dry_run: false, }); } @@ -437,7 +261,6 @@ async fn delete_one( let folder = row.folder.clone(); let entry_type = row.entry_type.clone(); - let migrated = migrate_key_refs_if_needed(&mut tx, &row, name, user_id, false).await?; snapshot_and_delete(&mut tx, &folder, &entry_type, name, &row, user_id).await?; crate::audit::log_tx( &mut tx, @@ -457,7 +280,6 @@ async fn delete_one( folder, entry_type, }], - migrated, dry_run: false, }) } @@ -497,33 +319,29 @@ async fn delete_bulk( } if entry_type.is_some() { conditions.push(format!("type = ${}", idx)); + idx += 1; } let where_clause = format!("WHERE {}", conditions.join(" AND ")); - let sql = format!( - "SELECT id, version, folder, type, name, metadata, tags, notes \ - FROM entries {where_clause} ORDER BY type, name" - ); - - let mut q = sqlx::query_as::<_, FullEntryRow>(&sql); - if let Some(uid) = user_id { - q = q.bind(uid); - } - if let Some(f) = folder { - q = q.bind(f); - } - if let Some(t) = entry_type { - q = q.bind(t); - } - let rows = q.fetch_all(pool).await?; + let _ = idx; // used only for placeholder numbering in conditions if dry_run { - let mut migrated: Vec = Vec::new(); - for row in &rows { - let refs = - fetch_key_referrers_pool(pool, row.id, &row.folder, &row.name, user_id).await?; - migrated.extend(refs.iter().map(ref_label)); + let sql = format!( + "SELECT id, version, folder, type, name, metadata, tags, notes \ + FROM entries {where_clause} ORDER BY type, name" + ); + let mut q = sqlx::query_as::<_, FullEntryRow>(&sql); + if let Some(uid) = user_id { + q = q.bind(uid); } + if let Some(f) = folder { + q = q.bind(f); + } + if let Some(t) = entry_type { + q = q.bind(t); + } + let rows = q.fetch_all(pool).await?; + let deleted = rows .iter() .map(|r| DeletedEntry { @@ -534,15 +352,31 @@ async fn delete_bulk( .collect(); return Ok(DeleteResult { deleted, - migrated, dry_run: true, }); } + let mut tx = pool.begin().await?; + + let sql = format!( + "SELECT id, version, folder, type, name, metadata, tags, notes \ + FROM entries {where_clause} ORDER BY type, name FOR UPDATE" + ); + let mut q = sqlx::query_as::<_, FullEntryRow>(&sql); + if let Some(uid) = user_id { + q = q.bind(uid); + } + if let Some(f) = folder { + q = q.bind(f); + } + if let Some(t) = entry_type { + q = q.bind(t); + } + let rows = q.fetch_all(&mut *tx).await?; + let mut deleted = Vec::with_capacity(rows.len()); - let mut migrated: Vec = Vec::new(); for row in &rows { - let entry_row = EntryRow { + let entry_row: EntryRow = EntryRow { id: row.id, version: row.version, folder: row.folder.clone(), @@ -551,9 +385,6 @@ async fn delete_bulk( metadata: row.metadata.clone(), notes: row.notes.clone(), }; - let mut tx = pool.begin().await?; - let m = migrate_key_refs_if_needed(&mut tx, &entry_row, &row.name, user_id, false).await?; - migrated.extend(m); snapshot_and_delete( &mut tx, &row.folder, @@ -573,7 +404,6 @@ async fn delete_bulk( json!({"bulk": true}), ) .await; - tx.commit().await?; deleted.push(DeletedEntry { name: row.name.clone(), folder: row.folder.clone(), @@ -581,9 +411,10 @@ async fn delete_bulk( }); } + tx.commit().await?; + Ok(DeleteResult { deleted, - migrated, dry_run: false, }) } @@ -646,12 +477,17 @@ async fn snapshot_and_delete( .execute(&mut **tx) .await?; - sqlx::query( - "DELETE FROM secrets s \ - WHERE NOT EXISTS (SELECT 1 FROM entry_secrets es WHERE es.secret_id = s.id)", - ) - .execute(&mut **tx) - .await?; + let secret_ids: Vec = fields.iter().map(|f| f.id).collect(); + if !secret_ids.is_empty() { + sqlx::query( + "DELETE FROM secrets s \ + WHERE s.id = ANY($1) \ + AND NOT EXISTS (SELECT 1 FROM entry_secrets es WHERE es.secret_id = s.id)", + ) + .bind(&secret_ids) + .execute(&mut **tx) + .await?; + } Ok(()) } @@ -659,280 +495,153 @@ async fn snapshot_and_delete( #[cfg(test)] mod tests { use super::*; - use serde_json::json; + use sqlx::PgPool; async fn maybe_test_pool() -> Option { let Ok(url) = std::env::var("SECRETS_DATABASE_URL") else { - eprintln!("skip delete migration tests: SECRETS_DATABASE_URL is not set"); + eprintln!("skip delete tests: SECRETS_DATABASE_URL is not set"); return None; }; let Ok(pool) = PgPool::connect(&url).await else { - eprintln!("skip delete migration tests: cannot connect to database"); + eprintln!("skip delete tests: cannot connect to database"); return None; }; if let Err(e) = crate::db::migrate(&pool).await { - eprintln!("skip delete migration tests: migrate failed: {e}"); + eprintln!("skip delete tests: migrate failed: {e}"); return None; } Some(pool) } - async fn insert_entry( - pool: &PgPool, - id: Uuid, - user_id: Uuid, - folder: &str, - entry_type: &str, - name: &str, - metadata: serde_json::Value, - ) -> Result<()> { + async fn cleanup_single_user_rows(pool: &PgPool, marker: &str) -> Result<()> { sqlx::query( - "INSERT INTO entries (id, user_id, folder, type, name, notes, tags, metadata, version) \ - VALUES ($1, $2, $3, $4, $5, '', ARRAY[]::text[], $6, 1)", + "DELETE FROM entries WHERE user_id IS NULL AND (name LIKE $1 OR folder LIKE $1)", ) - .bind(id) - .bind(user_id) - .bind(folder) - .bind(entry_type) - .bind(name) - .bind(metadata) + .bind(format!("%{marker}%")) + .execute(pool) + .await?; + sqlx::query( + "DELETE FROM secrets WHERE user_id IS NULL AND name LIKE $1 \ + AND NOT EXISTS (SELECT 1 FROM entry_secrets es WHERE es.secret_id = secrets.id)", + ) + .bind(format!("%{marker}%")) .execute(pool) .await?; Ok(()) } - async fn insert_secret_for_entry( - pool: &PgPool, - user_id: Uuid, - entry_id: Uuid, - name: &str, - secret_type: &str, - encrypted: Vec, - ) -> Result<()> { - let secret_id: Uuid = sqlx::query_scalar( - "INSERT INTO secrets (user_id, name, type, encrypted) VALUES ($1, $2, $3, $4) RETURNING id", - ) - .bind(user_id) - .bind(name) - .bind(secret_type) - .bind(encrypted) - .fetch_one(pool) - .await?; - sqlx::query("INSERT INTO entry_secrets (entry_id, secret_id) VALUES ($1, $2)") - .bind(entry_id) - .bind(secret_id) - .execute(pool) - .await?; - Ok(()) - } - #[tokio::test] - async fn delete_shared_key_dry_run_reports_migration_without_writes() -> Result<()> { + async fn delete_dry_run_reports_matching_entry_without_writes() -> Result<()> { let Some(pool) = maybe_test_pool().await else { return Ok(()); }; + let suffix = Uuid::from_u128(rand::random()).to_string(); + let marker = format!("delete_dry_{}", &suffix[..8]); + let entry_name = format!("{}_entry", marker); - let user_id = Uuid::from_u128(rand::random()); - let key_id = Uuid::from_u128(rand::random()); - let ref_a = Uuid::from_u128(rand::random()); - let ref_b = Uuid::from_u128(rand::random()); + cleanup_single_user_rows(&pool, &marker).await?; - insert_entry( - &pool, - key_id, - user_id, - "kfolder", - "key", - "shared-key", - json!({}), - ) - .await?; - insert_secret_for_entry(&pool, user_id, key_id, "pem", "pem", vec![1_u8, 2, 3]).await?; - - insert_entry( - &pool, - ref_a, - user_id, - "afolder", - "server", - "srv-a", - json!({"key_ref":"kfolder/shared-key"}), - ) - .await?; - insert_entry( - &pool, - ref_b, - user_id, - "bfolder", - "server", - "srv-b", - json!({"key_ref":"shared-key"}), + sqlx::query( + "INSERT INTO entries (user_id, folder, type, name, notes, tags, metadata) \ + VALUES (NULL, $1, 'service', $2, '', '{}', '{}')", ) + .bind(&marker) + .bind(&entry_name) + .execute(&pool) .await?; let result = run( &pool, DeleteParams { - name: Some("shared-key"), - folder: Some("kfolder"), + name: Some(&entry_name), + folder: Some(&marker), entry_type: None, dry_run: true, - user_id: Some(user_id), + user_id: None, }, ) .await?; assert!(result.dry_run); assert_eq!(result.deleted.len(), 1); - assert_eq!(result.migrated.len(), 2); + assert_eq!(result.deleted[0].name, entry_name); - let key_exists: bool = sqlx::query_scalar( - "SELECT EXISTS(SELECT 1 FROM entries WHERE id = $1 AND user_id = $2)", + let still_exists: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM entries WHERE user_id IS NULL AND folder = $1 AND name = $2)", ) - .bind(key_id) - .bind(user_id) + .bind(&marker) + .bind(&entry_name) .fetch_one(&pool) .await?; - assert!(key_exists); + assert!(still_exists); - let ref_a_key_ref: Option = - sqlx::query_scalar("SELECT metadata->>'key_ref' FROM entries WHERE id = $1") - .bind(ref_a) - .fetch_one(&pool) - .await?; - let ref_b_key_ref: Option = - sqlx::query_scalar("SELECT metadata->>'key_ref' FROM entries WHERE id = $1") - .bind(ref_b) - .fetch_one(&pool) - .await?; - assert_eq!(ref_a_key_ref.as_deref(), Some("kfolder/shared-key")); - assert_eq!(ref_b_key_ref.as_deref(), Some("shared-key")); - - sqlx::query("DELETE FROM entries WHERE user_id = $1") - .bind(user_id) - .execute(&pool) - .await?; + cleanup_single_user_rows(&pool, &marker).await?; Ok(()) } #[tokio::test] - async fn delete_shared_key_auto_migrates_single_copy_and_redirects_refs() -> Result<()> { + async fn delete_by_id_removes_entry_and_orphan_secret() -> Result<()> { let Some(pool) = maybe_test_pool().await else { return Ok(()); }; - + let suffix = Uuid::from_u128(rand::random()).to_string(); + let marker = format!("delete_id_{}", &suffix[..8]); let user_id = Uuid::from_u128(rand::random()); - let key_id = Uuid::from_u128(rand::random()); - let ref_a = Uuid::from_u128(rand::random()); - let ref_b = Uuid::from_u128(rand::random()); - let ref_c = Uuid::from_u128(rand::random()); + let entry_name = format!("{}_entry", marker); + let secret_name = format!("{}_secret", marker); - insert_entry( - &pool, - key_id, - user_id, - "kfolder", - "key", - "shared-key", - json!({}), - ) - .await?; - insert_secret_for_entry(&pool, user_id, key_id, "pem", "pem", vec![7_u8, 8, 9]).await?; - - // owner candidate (sorted first by folder) - insert_entry( - &pool, - ref_a, - user_id, - "afolder", - "server", - "srv-a", - json!({"key_ref":"kfolder/shared-key"}), - ) - .await?; - insert_entry( - &pool, - ref_b, - user_id, - "bfolder", - "server", - "srv-b", - json!({"key_ref":"shared-key"}), - ) - .await?; - insert_entry( - &pool, - ref_c, - user_id, - "cfolder", - "service", - "svc-c", - json!({"key_ref":"kfolder/shared-key"}), - ) - .await?; - - let result = run( - &pool, - DeleteParams { - name: Some("shared-key"), - folder: Some("kfolder"), - entry_type: None, - dry_run: false, - user_id: Some(user_id), - }, - ) - .await?; - - assert!(!result.dry_run); - assert_eq!(result.deleted.len(), 1); - assert_eq!(result.migrated.len(), 3); - - let key_exists: bool = sqlx::query_scalar( - "SELECT EXISTS(SELECT 1 FROM entries WHERE id = $1 AND user_id = $2)", - ) - .bind(key_id) - .bind(user_id) - .fetch_one(&pool) - .await?; - assert!(!key_exists); - - let owner_key_ref: Option = - sqlx::query_scalar("SELECT metadata->>'key_ref' FROM entries WHERE id = $1") - .bind(ref_a) - .fetch_one(&pool) - .await?; - let ref_b_key_ref: Option = - sqlx::query_scalar("SELECT metadata->>'key_ref' FROM entries WHERE id = $1") - .bind(ref_b) - .fetch_one(&pool) - .await?; - let ref_c_key_ref: Option = - sqlx::query_scalar("SELECT metadata->>'key_ref' FROM entries WHERE id = $1") - .bind(ref_c) - .fetch_one(&pool) - .await?; - - assert_eq!(owner_key_ref, None); - assert_eq!(ref_b_key_ref.as_deref(), Some("afolder/srv-a")); - assert_eq!(ref_c_key_ref.as_deref(), Some("afolder/srv-a")); - - let owner_has_copied: bool = sqlx::query_scalar( - "SELECT EXISTS( \ - SELECT 1 \ - FROM entry_secrets es \ - JOIN secrets s ON s.id = es.secret_id \ - WHERE es.entry_id = $1 AND s.name = 'pem' \ - )", - ) - .bind(ref_a) - .fetch_one(&pool) - .await?; - assert!(owner_has_copied); - - sqlx::query("DELETE FROM entries WHERE user_id = $1") + sqlx::query("DELETE FROM entries WHERE user_id = $1 AND folder = $2") .bind(user_id) + .bind(&marker) .execute(&pool) .await?; + sqlx::query("DELETE FROM secrets WHERE user_id = $1 AND name = $2") + .bind(user_id) + .bind(&secret_name) + .execute(&pool) + .await?; + + let entry_id: Uuid = sqlx::query_scalar( + "INSERT INTO entries (user_id, folder, type, name, notes, tags, metadata) \ + VALUES ($1, $2, 'service', $3, '', '{}', '{}') RETURNING id", + ) + .bind(user_id) + .bind(&marker) + .bind(&entry_name) + .fetch_one(&pool) + .await?; + let secret_id: Uuid = sqlx::query_scalar( + "INSERT INTO secrets (user_id, name, type, encrypted) VALUES ($1, $2, 'text', $3) RETURNING id", + ) + .bind(user_id) + .bind(&secret_name) + .bind(vec![1_u8, 2, 3]) + .fetch_one(&pool) + .await?; + sqlx::query("INSERT INTO entry_secrets (entry_id, secret_id) VALUES ($1, $2)") + .bind(entry_id) + .bind(secret_id) + .execute(&pool) + .await?; + + let result = delete_by_id(&pool, entry_id, user_id).await?; + assert!(!result.dry_run); + assert_eq!(result.deleted.len(), 1); + assert_eq!(result.deleted[0].name, entry_name); + + let entry_exists: bool = + sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM entries WHERE id = $1)") + .bind(entry_id) + .fetch_one(&pool) + .await?; + let secret_exists: bool = + sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM secrets WHERE id = $1)") + .bind(secret_id) + .fetch_one(&pool) + .await?; + assert!(!entry_exists); + assert!(!secret_exists); + Ok(()) } } diff --git a/crates/secrets-core/src/service/env_map.rs b/crates/secrets-core/src/service/env_map.rs index 37cd1d2..9ab0f6c 100644 --- a/crates/secrets-core/src/service/env_map.rs +++ b/crates/secrets-core/src/service/env_map.rs @@ -40,7 +40,7 @@ async fn build_entry_env_map( only_fields: &[String], prefix: &str, master_key: &[u8; 32], - user_id: Option, + _user_id: Option, ) -> Result> { let entry_ids = vec![entry.id]; let secrets_map = fetch_secrets_for_entries(pool, &entry_ids).await?; @@ -68,44 +68,6 @@ async fn build_entry_env_map( map.insert(key, json_to_env_string(&decrypted)); } - // Resolve key_ref. Supported formats: "name" or "folder/name". - if let Some(key_ref) = entry.metadata.get("key_ref").and_then(|v| v.as_str()) { - let (ref_folder, ref_name) = if let Some((f, n)) = key_ref.split_once('/') { - (Some(f), n) - } else { - (None, key_ref) - }; - let key_entries = - fetch_entries(pool, ref_folder, None, Some(ref_name), &[], None, user_id).await?; - - if key_entries.len() > 1 { - anyhow::bail!( - "key_ref '{}' matched {} entries; qualify with folder/name to resolve the ambiguity", - key_ref, - key_entries.len() - ); - } - - if let Some(key_entry) = key_entries.first() { - let key_ids = vec![key_entry.id]; - let key_fields_map = fetch_secrets_for_entries(pool, &key_ids).await?; - let empty = vec![]; - let key_fields = key_fields_map.get(&key_entry.id).unwrap_or(&empty); - let key_prefix = env_prefix(key_entry, prefix); - for f in key_fields { - let decrypted = crypto::decrypt_json(master_key, &f.encrypted)?; - let key_var = format!( - "{}_{}", - key_prefix, - f.name.to_uppercase().replace(['-', '.'], "_") - ); - map.insert(key_var, json_to_env_string(&decrypted)); - } - } else { - tracing::warn!(key_ref, ?user_id, "key_ref target not found"); - } - } - Ok(map) } diff --git a/crates/secrets-core/src/service/import.rs b/crates/secrets-core/src/service/import.rs index 77c5318..82a5634 100644 --- a/crates/secrets-core/src/service/import.rs +++ b/crates/secrets-core/src/service/import.rs @@ -85,6 +85,7 @@ pub async fn run( tags: &entry.tags, meta_entries: &meta_entries, secret_entries: &secret_entries, + secret_types: &Default::default(), link_secret_names: &[], user_id: params.user_id, }, diff --git a/crates/secrets-core/src/service/search.rs b/crates/secrets-core/src/service/search.rs index 0e9cdbf..2f2cc23 100644 --- a/crates/secrets-core/src/service/search.rs +++ b/crates/secrets-core/src/service/search.rs @@ -8,10 +8,23 @@ use crate::models::{Entry, SecretField}; pub const FETCH_ALL_LIMIT: u32 = 100_000; +/// Build an ILIKE pattern for fuzzy matching, escaping `%` and `_` literals. +pub fn ilike_pattern(value: &str) -> String { + format!( + "%{}%", + value + .replace('\\', "\\\\") + .replace('%', "\\%") + .replace('_', "\\_") + ) +} + pub struct SearchParams<'a> { pub folder: Option<&'a str>, pub entry_type: Option<&'a str>, pub name: Option<&'a str>, + /// Fuzzy match on `entries.name` only (ILIKE with escaped `%`/`_`). + pub name_query: Option<&'a str>, pub tags: &'a [String], pub query: Option<&'a str>, pub sort: &'a str, @@ -51,11 +64,15 @@ pub async fn count_entries(pool: &PgPool, a: &SearchParams<'_>) -> Result { if let Some(v) = a.name { q = q.bind(v); } + if let Some(v) = a.name_query { + let pattern = ilike_pattern(v); + q = q.bind(pattern); + } for tag in a.tags { q = q.bind(tag); } if let Some(v) = a.query { - let pattern = format!("%{}%", v.replace('%', "\\%").replace('_', "\\_")); + let pattern = ilike_pattern(v); q = q.bind(pattern); } let n = q.fetch_one(pool).await?; @@ -86,6 +103,10 @@ fn entry_where_clause_and_next_idx(a: &SearchParams<'_>) -> (String, i32) { conditions.push(format!("name = ${}", idx)); idx += 1; } + if a.name_query.is_some() { + conditions.push(format!("name ILIKE ${} ESCAPE '\\'", idx)); + idx += 1; + } if !a.tags.is_empty() { let placeholders: Vec = a .tags @@ -135,6 +156,7 @@ pub async fn run(pool: &PgPool, params: SearchParams<'_>) -> Result, @@ -148,6 +170,7 @@ pub async fn fetch_entries( folder, entry_type, name, + name_query: None, tags, query, sort: "name", @@ -189,11 +212,15 @@ async fn fetch_entries_paged(pool: &PgPool, a: &SearchParams<'_>) -> Result { pub meta_entries: &'a [String], pub remove_meta: &'a [String], pub secret_entries: &'a [String], + pub secret_types: &'a std::collections::HashMap, pub remove_secrets: &'a [String], pub user_id: Option, } @@ -90,10 +93,7 @@ pub async fn run( let row = match rows.len() { 0 => { tx.rollback().await?; - anyhow::bail!( - "Not found: '{}'. Use `add` to create it first.", - params.name - ) + return Err(AppError::NotFoundEntry.into()); } 1 => rows.into_iter().next().unwrap(), _ => { @@ -167,10 +167,7 @@ pub async fn run( if result.rows_affected() == 0 { tx.rollback().await?; - anyhow::bail!( - "Concurrent modification detected for '{}'. Please retry.", - params.name - ); + return Err(AppError::ConcurrentModification.into()); } for entry in params.secret_entries { @@ -224,15 +221,21 @@ pub async fn run( .execute(&mut *tx) .await?; } else { + let secret_type = params + .secret_types + .get(field_name) + .map(|s| s.as_str()) + .unwrap_or("text"); let secret_id: Uuid = sqlx::query_scalar( "INSERT INTO secrets (user_id, name, type, encrypted) VALUES ($1, $2, $3, $4) RETURNING id", ) .bind(params.user_id) - .bind(field_name) - .bind(infer_secret_type(field_name)) + .bind(field_name.to_string()) + .bind(secret_type) .bind(&encrypted) .fetch_one(&mut *tx) - .await?; + .await + .map_err(|e| AppError::from_db_error(e, DbErrorContext::secret_name(field_name)))?; sqlx::query("INSERT INTO entry_secrets (entry_id, secret_id) VALUES ($1, $2)") .bind(row.id) .bind(secret_id) @@ -347,13 +350,13 @@ pub async fn update_fields_by_id( user_id: Uuid, params: UpdateEntryFieldsByIdParams<'_>, ) -> Result<()> { - if params.folder.len() > 128 { + if params.folder.chars().count() > 128 { anyhow::bail!("folder must be at most 128 characters"); } - if params.entry_type.len() > 64 { + if params.entry_type.chars().count() > 64 { anyhow::bail!("type must be at most 64 characters"); } - if params.name.len() > 256 { + if params.name.chars().count() > 256 { anyhow::bail!("name must be at most 256 characters"); } @@ -372,7 +375,7 @@ pub async fn update_fields_by_id( Some(r) => r, None => { tx.rollback().await?; - anyhow::bail!("Entry not found"); + return Err(AppError::NotFoundEntry.into()); } }; @@ -395,17 +398,25 @@ pub async fn update_fields_by_id( tracing::warn!(error = %e, "failed to snapshot entry history before web update"); } + let mut metadata_map = match params.metadata { + Value::Object(m) => m.clone(), + _ => Map::new(), + }; + let normalized_type = + taxonomy::normalize_entry_type_and_metadata(params.entry_type, &mut metadata_map); + let normalized_metadata = Value::Object(metadata_map); + let res = sqlx::query( "UPDATE entries SET folder = $1, type = $2, name = $3, notes = $4, tags = $5, metadata = $6, \ version = version + 1, updated_at = NOW() \ WHERE id = $7 AND version = $8", ) .bind(params.folder) - .bind(params.entry_type) + .bind(&normalized_type) .bind(params.name) .bind(params.notes) .bind(params.tags) - .bind(params.metadata) + .bind(&normalized_metadata) .bind(row.id) .bind(row.version) .execute(&mut *tx) @@ -414,16 +425,17 @@ pub async fn update_fields_by_id( if let sqlx::Error::Database(ref d) = e && d.code().as_deref() == Some("23505") { - return anyhow::anyhow!( - "An entry with this folder and name already exists for your account." - ); + return AppError::ConflictEntryName { + folder: params.folder.to_string(), + name: params.name.to_string(), + }; } - e.into() + AppError::Internal(e.into()) })?; if res.rows_affected() == 0 { tx.rollback().await?; - anyhow::bail!("Concurrent modification detected. Please refresh and try again."); + return Err(AppError::ConcurrentModification.into()); } crate::audit::log_tx( @@ -431,7 +443,7 @@ pub async fn update_fields_by_id( Some(user_id), "update", params.folder, - params.entry_type, + &normalized_type, params.name, serde_json::json!({ "source": "web", diff --git a/crates/secrets-core/src/taxonomy.rs b/crates/secrets-core/src/taxonomy.rs new file mode 100644 index 0000000..dcd9b48 --- /dev/null +++ b/crates/secrets-core/src/taxonomy.rs @@ -0,0 +1,111 @@ +use serde_json::{Map, Value}; + +fn normalize_token(input: &str) -> String { + input.trim().to_lowercase().replace('_', "-") +} + +fn normalize_subtype_token(input: &str) -> String { + normalize_token(input) +} + +fn map_legacy_entry_type(input: &str) -> Option<(&'static str, &'static str)> { + match input { + "log-ingestion-endpoint" => Some(("service", "log-ingestion")), + "cloud-api" => Some(("service", "cloud-api")), + "git-server" => Some(("service", "git")), + "mqtt-broker" => Some(("service", "mqtt-broker")), + "database" => Some(("service", "database")), + "monitoring-dashboard" => Some(("service", "monitoring")), + "dns-api" => Some(("service", "dns-api")), + "notification-webhook" => Some(("service", "webhook")), + "api-endpoint" => Some(("service", "api-endpoint")), + "credential" | "credential-key" => Some(("service", "credential")), + "key" => Some(("service", "credential")), + _ => None, + } +} + +/// Normalize entry `type` and optionally backfill `metadata.subtype` for legacy values. +/// +/// This keeps backward compatibility: +/// - stable primary types stay unchanged +/// - known legacy long-tail types are mapped to `service` + `metadata.subtype` +/// - unknown values are kept (normalized to kebab-case) instead of hard failing +pub fn normalize_entry_type_and_metadata( + entry_type: &str, + metadata: &mut Map, +) -> String { + let original_raw = entry_type.trim(); + let normalized = normalize_token(original_raw); + if normalized.is_empty() { + return String::new(); + } + + if let Some((mapped_type, mapped_subtype)) = map_legacy_entry_type(&normalized) { + if !metadata.contains_key("subtype") { + metadata.insert( + "subtype".to_string(), + Value::String(mapped_subtype.to_string()), + ); + } + if !metadata.contains_key("_original_type") && original_raw != mapped_type { + metadata.insert( + "_original_type".to_string(), + Value::String(original_raw.to_string()), + ); + } + return mapped_type.to_string(); + } + + if let Some(subtype) = metadata.get_mut("subtype") + && let Some(s) = subtype.as_str() + { + *subtype = Value::String(normalize_subtype_token(s)); + } + + normalized +} + +/// Canonical secret type options for UI dropdowns. +pub const SECRET_TYPE_OPTIONS: &[&str] = &[ + "text", "password", "token", "api-key", "ssh-key", "url", "phone", "id-card", +]; + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::{Map, Value}; + + #[test] + fn normalize_entry_type_maps_legacy_type_and_backfills_metadata() { + let mut metadata = Map::new(); + let normalized = normalize_entry_type_and_metadata("git-server", &mut metadata); + + assert_eq!(normalized, "service"); + assert_eq!( + metadata.get("subtype"), + Some(&Value::String("git".to_string())) + ); + assert_eq!( + metadata.get("_original_type"), + Some(&Value::String("git-server".to_string())) + ); + } + + #[test] + fn normalize_entry_type_normalizes_existing_subtype() { + let mut metadata = Map::new(); + metadata.insert( + "subtype".to_string(), + Value::String("Cloud_API".to_string()), + ); + + let normalized = normalize_entry_type_and_metadata("service", &mut metadata); + + assert_eq!(normalized, "service"); + assert_eq!( + metadata.get("subtype"), + Some(&Value::String("cloud-api".to_string())) + ); + } +} diff --git a/crates/secrets-mcp/Cargo.toml b/crates/secrets-mcp/Cargo.toml index b1eed8c..f21a50e 100644 --- a/crates/secrets-mcp/Cargo.toml +++ b/crates/secrets-mcp/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "secrets-mcp" -version = "0.3.9" +version = "0.4.0" edition.workspace = true [[bin]] diff --git a/crates/secrets-mcp/src/error.rs b/crates/secrets-mcp/src/error.rs new file mode 100644 index 0000000..1d90647 --- /dev/null +++ b/crates/secrets-mcp/src/error.rs @@ -0,0 +1,36 @@ +use secrets_core::error::AppError; + +/// Map a structured `AppError` to an MCP protocol error. +/// +/// This replaces the previous pattern of swallowing all errors into `-32603`. +pub fn app_error_to_mcp(err: &AppError) -> rmcp::ErrorData { + match err { + AppError::ConflictSecretName { secret_name } => rmcp::ErrorData::invalid_request( + format!( + "A secret with the name '{secret_name}' already exists for your account. \ + Secret names must be unique per user." + ), + None, + ), + AppError::ConflictEntryName { folder, name } => rmcp::ErrorData::invalid_request( + format!( + "An entry with folder='{folder}' and name='{name}' already exists. \ + The combination of folder and name must be unique." + ), + None, + ), + AppError::NotFoundEntry => rmcp::ErrorData::invalid_request( + "Entry not found. Use secrets_find to discover existing entries.", + None, + ), + AppError::Validation { message } => rmcp::ErrorData::invalid_request(message.clone(), None), + AppError::ConcurrentModification => rmcp::ErrorData::invalid_request( + "The entry was modified by another request. Please refresh and try again.", + None, + ), + AppError::Internal(_) => rmcp::ErrorData::internal_error( + "Request failed due to a server error. Check service logs if you need details.", + None, + ), + } +} diff --git a/crates/secrets-mcp/src/main.rs b/crates/secrets-mcp/src/main.rs index 134823b..f3452e0 100644 --- a/crates/secrets-mcp/src/main.rs +++ b/crates/secrets-mcp/src/main.rs @@ -1,4 +1,5 @@ mod auth; +mod error; mod logging; mod oauth; mod tools; diff --git a/crates/secrets-mcp/src/tools.rs b/crates/secrets-mcp/src/tools.rs index 24aa0c0..9c18afc 100644 --- a/crates/secrets-mcp/src/tools.rs +++ b/crates/secrets-mcp/src/tools.rs @@ -31,6 +31,7 @@ use secrets_core::service::{ }; use crate::auth::AuthUser; +use crate::error; // ── MCP client-facing errors (no internal details) ─────────────────────────── @@ -50,6 +51,17 @@ fn mcp_err_internal_logged( ) } +fn mcp_err_from_anyhow( + tool: &'static str, + user_id: Option, + err: anyhow::Error, +) -> rmcp::ErrorData { + if let Some(app_err) = err.downcast_ref::() { + return error::app_error_to_mcp(app_err); + } + mcp_err_internal_logged(tool, user_id, err) +} + fn mcp_err_invalid_encryption_key_logged(err: impl std::fmt::Display) -> rmcp::ErrorData { tracing::warn!(error = %err, "invalid X-Encryption-Key"); rmcp::ErrorData::invalid_request( @@ -162,11 +174,17 @@ struct FindInput { query: Option, #[schemars(description = "Exact folder filter (e.g. 'refining', 'ricnsmart')")] folder: Option, - #[schemars(description = "Exact type filter (e.g. 'server', 'service', 'person', 'key')")] + #[schemars( + description = "Exact type filter (recommended: 'server', 'service', 'person', 'document')" + )] #[serde(rename = "type")] entry_type: Option, - #[schemars(description = "Exact name filter")] + #[schemars(description = "Exact name filter. For fuzzy matching use name_query instead.")] name: Option, + #[schemars( + description = "Fuzzy name filter (ILIKE, case-insensitive partial match). Use this instead of 'name' when you don't know the exact name." + )] + name_query: Option, #[schemars(description = "Tag filters (all must match)")] tags: Option>, #[schemars(description = "Max results (default 20)")] @@ -179,11 +197,17 @@ struct SearchInput { query: Option, #[schemars(description = "Folder filter (e.g. 'refining', 'personal', 'family')")] folder: Option, - #[schemars(description = "Type filter (e.g. 'server', 'service', 'person', 'key')")] + #[schemars( + description = "Type filter (recommended: 'server', 'service', 'person', 'document')" + )] #[serde(rename = "type")] entry_type: Option, - #[schemars(description = "Exact name to match")] + #[schemars(description = "Exact name to match. For fuzzy matching use name_query instead.")] name: Option, + #[schemars( + description = "Fuzzy name filter (ILIKE, case-insensitive partial match). Use this instead of 'name' when you don't know the exact name." + )] + name_query: Option, #[schemars(description = "Tag filters (all must match)")] tags: Option>, #[schemars(description = "Return only summary fields (name/tags/notes/updated_at)")] @@ -211,7 +235,7 @@ struct AddInput { #[schemars(description = "Folder for organization (optional, e.g. 'personal', 'refining')")] folder: Option, #[schemars( - description = "Type/category of this entry (optional, e.g. 'server', 'person', 'key')" + description = "Type/category of this entry (optional, recommended: 'server', 'service', 'person', 'document')" )] #[serde(rename = "type")] entry_type: Option, @@ -233,6 +257,10 @@ struct AddInput { description = "Secret fields as a JSON object {\"key\": \"value\"}. Merged with 'secrets' if both provided. Reminder: non-sensitive endpoint/address fields should go to metadata.address." )] secrets_obj: Option>, + #[schemars( + description = "Secret types as {\"secret_name\": \"type\"}. Keys must match secret field names. Missing keys default to \"text\"." + )] + secret_types: Option>, #[schemars( description = "Link existing secrets by secret name. Names must resolve uniquely under current user." )] @@ -273,6 +301,10 @@ struct UpdateInput { description = "Secret fields to update/add as a JSON object {\"key\": \"value\"}. Merged with 'secrets' if both provided. Reminder: non-sensitive endpoint/address fields should go to metadata.address." )] secrets_obj: Option>, + #[schemars( + description = "Secret types as {\"secret_name\": \"type\"}. Keys must match secret field names. Missing keys default to \"text\"." + )] + secret_types: Option>, #[schemars(description = "Secret field keys to remove")] remove_secrets: Option>, } @@ -412,6 +444,7 @@ impl SecretsService { folder = input.folder.as_deref(), entry_type = input.entry_type.as_deref(), name = input.name.as_deref(), + name_query = input.name_query.as_deref(), query = input.query.as_deref(), "tool call start", ); @@ -422,6 +455,7 @@ impl SecretsService { folder: input.folder.as_deref(), entry_type: input.entry_type.as_deref(), name: input.name.as_deref(), + name_query: input.name_query.as_deref(), tags: &tags, query: input.query.as_deref(), sort: "name", @@ -499,6 +533,7 @@ impl SecretsService { folder = input.folder.as_deref(), entry_type = input.entry_type.as_deref(), name = input.name.as_deref(), + name_query = input.name_query.as_deref(), query = input.query.as_deref(), "tool call start", ); @@ -509,6 +544,7 @@ impl SecretsService { folder: input.folder.as_deref(), entry_type: input.entry_type.as_deref(), name: input.name.as_deref(), + name_query: input.name_query.as_deref(), tags: &tags, query: input.query.as_deref(), sort: input.sort.as_deref().unwrap_or("name"), @@ -667,6 +703,11 @@ impl SecretsService { if let Some(obj) = input.secrets_obj { secrets.extend(map_to_kv_strings(obj)); } + let secret_types = input.secret_types.unwrap_or_default(); + let secret_types_map: std::collections::HashMap = secret_types + .into_iter() + .filter_map(|(k, v)| v.as_str().map(|s| (k, s.to_string()))) + .collect(); let link_secret_names = input.link_secret_names.unwrap_or_default(); let folder = input.folder.as_deref().unwrap_or(""); let entry_type = input.entry_type.as_deref().unwrap_or(""); @@ -682,13 +723,14 @@ impl SecretsService { tags: &tags, meta_entries: &meta, secret_entries: &secrets, + secret_types: &secret_types_map, link_secret_names: &link_secret_names, user_id: Some(user_id), }, &user_key, ) .await - .map_err(|e| mcp_err_internal_logged("secrets_add", Some(user_id), e))?; + .map_err(|e| mcp_err_from_anyhow("secrets_add", Some(user_id), e))?; tracing::info!( tool = "secrets_add", @@ -745,6 +787,11 @@ impl SecretsService { if let Some(obj) = input.secrets_obj { secrets.extend(map_to_kv_strings(obj)); } + let secret_types = input.secret_types.unwrap_or_default(); + let secret_types_map: std::collections::HashMap = secret_types + .into_iter() + .filter_map(|(k, v)| v.as_str().map(|s| (k, s.to_string()))) + .collect(); let remove_secrets = input.remove_secrets.unwrap_or_default(); let result = svc_update( @@ -758,13 +805,14 @@ impl SecretsService { meta_entries: &meta, remove_meta: &remove_meta, secret_entries: &secrets, + secret_types: &secret_types_map, remove_secrets: &remove_secrets, user_id: Some(user_id), }, &user_key, ) .await - .map_err(|e| mcp_err_internal_logged("secrets_update", Some(user_id), e))?; + .map_err(|e| mcp_err_from_anyhow("secrets_update", Some(user_id), e))?; tracing::info!( tool = "secrets_update", diff --git a/crates/secrets-mcp/src/web.rs b/crates/secrets-mcp/src/web.rs index f536fa5..732f221 100644 --- a/crates/secrets-mcp/src/web.rs +++ b/crates/secrets-mcp/src/web.rs @@ -17,11 +17,12 @@ use uuid::Uuid; use secrets_core::audit::log_login; use secrets_core::crypto::hex; +use secrets_core::error::AppError; use secrets_core::service::{ api_key::{ensure_api_key, regenerate_api_key}, audit_log::list_for_user, delete::delete_by_id, - search::{SearchParams, count_entries, fetch_secret_schemas, list_entries}, + search::{SearchParams, fetch_secret_schemas, ilike_pattern, list_entries}, update::{UpdateEntryFieldsByIdParams, update_fields_by_id}, user::{ OAuthProfile, bind_oauth_account, find_or_create_user, get_user_by_id, @@ -88,15 +89,16 @@ struct EntriesPageTemplate { user_name: String, user_email: String, entries: Vec, - total_count: i64, - shown_count: usize, - limit: u32, + folder_tabs: Vec, + type_options: Vec, + secret_type_options_json: String, filter_folder: String, + filter_name: String, filter_type: String, version: &'static str, } -/// Non-sensitive fields only (no `secrets` / ciphertext). +/// Non-sensitive entry fields; `secrets` lists field names/types only (no ciphertext). struct EntryListItemView { id: String, folder: String, @@ -104,24 +106,37 @@ struct EntryListItemView { name: String, notes: String, tags: String, - metadata: String, + /// Compact JSON for `data-entry-metadata` (dialog editor). + metadata_json: String, + /// Secret field summaries for table + dialog chips. secrets: Vec, - /// RFC3339 UTC for `