Compare commits
3 Commits
secrets-mc
...
secrets-mc
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
c3b1a0df1a | ||
|
|
d772066210 | ||
|
|
2c7dbf890b |
2
Cargo.lock
generated
2
Cargo.lock
generated
@@ -2065,7 +2065,7 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "secrets-mcp"
|
||||
version = "0.5.19"
|
||||
version = "0.5.21"
|
||||
dependencies = [
|
||||
"anyhow",
|
||||
"askama",
|
||||
|
||||
@@ -184,6 +184,9 @@ pub struct ExportEntry {
|
||||
/// Decrypted secret fields. None means no secrets in this export (--no-secrets).
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub secrets: Option<BTreeMap<String, Value>>,
|
||||
/// Per-secret types (`text`, `password`, `key`, …). Omitted in legacy exports; importers default to `"text"`.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub secret_types: Option<BTreeMap<String, String>>,
|
||||
}
|
||||
|
||||
// ── Multi-user models ──────────────────────────────────────────────────────────
|
||||
@@ -311,3 +314,44 @@ pub fn toml_to_json_value(v: &toml::Value) -> Value {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod export_entry_tests {
|
||||
use super::*;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
#[test]
|
||||
fn export_entry_roundtrip_includes_secret_types() {
|
||||
let mut secrets = BTreeMap::new();
|
||||
secrets.insert("k".to_string(), serde_json::json!("v"));
|
||||
let mut types = BTreeMap::new();
|
||||
types.insert("k".to_string(), "password".to_string());
|
||||
let e = ExportEntry {
|
||||
name: "n".to_string(),
|
||||
folder: "f".to_string(),
|
||||
entry_type: "t".to_string(),
|
||||
notes: "".to_string(),
|
||||
tags: vec![],
|
||||
metadata: serde_json::json!({}),
|
||||
secrets: Some(secrets),
|
||||
secret_types: Some(types),
|
||||
};
|
||||
let json = serde_json::to_string(&e).unwrap();
|
||||
let back: ExportEntry = serde_json::from_str(&json).unwrap();
|
||||
assert_eq!(
|
||||
back.secret_types
|
||||
.as_ref()
|
||||
.unwrap()
|
||||
.get("k")
|
||||
.map(String::as_str),
|
||||
Some("password")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn export_entry_legacy_json_without_secret_types_deserializes() {
|
||||
let json = r#"{"name":"a","folder":"","type":"","notes":"","tags":[],"metadata":{},"secrets":{"x":"y"}}"#;
|
||||
let e: ExportEntry = serde_json::from_str(json).unwrap();
|
||||
assert!(e.secret_types.is_none());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -161,6 +161,7 @@ pub fn flatten_json_fields(prefix: &str, value: &Value) -> Vec<(String, Value)>
|
||||
|
||||
#[derive(Debug, serde::Serialize)]
|
||||
pub struct AddResult {
|
||||
pub entry_id: Uuid,
|
||||
pub name: String,
|
||||
pub folder: String,
|
||||
#[serde(rename = "type")]
|
||||
@@ -477,6 +478,7 @@ pub async fn run(pool: &PgPool, params: AddParams<'_>, master_key: &[u8; 32]) ->
|
||||
tx.commit().await?;
|
||||
|
||||
Ok(AddResult {
|
||||
entry_id,
|
||||
name: params.name.to_string(),
|
||||
folder: params.folder.to_string(),
|
||||
entry_type: entry_type.to_string(),
|
||||
|
||||
@@ -47,11 +47,14 @@ pub async fn ensure_api_key(pool: &PgPool, user_id: Uuid) -> Result<String> {
|
||||
/// Generate a fresh API key for the user, replacing the old one.
|
||||
pub async fn regenerate_api_key(pool: &PgPool, user_id: Uuid) -> Result<String> {
|
||||
let new_key = generate_api_key();
|
||||
sqlx::query("UPDATE users SET api_key = $1 WHERE id = $2")
|
||||
let res = sqlx::query("UPDATE users SET api_key = $1 WHERE id = $2")
|
||||
.bind(&new_key)
|
||||
.bind(user_id)
|
||||
.execute(pool)
|
||||
.await?;
|
||||
if res.rows_affected() == 0 {
|
||||
return Err(AppError::NotFoundUser.into());
|
||||
}
|
||||
Ok(new_key)
|
||||
}
|
||||
|
||||
@@ -63,3 +66,30 @@ pub async fn validate_api_key(pool: &PgPool, raw_key: &str) -> Result<Option<Uui
|
||||
.await?;
|
||||
Ok(row.map(|(id,)| id))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use sqlx::PgPool;
|
||||
|
||||
use super::regenerate_api_key;
|
||||
use crate::error::AppError;
|
||||
|
||||
#[tokio::test]
|
||||
async fn regenerate_api_key_unknown_user_returns_not_found() {
|
||||
let Ok(url) = std::env::var("SECRETS_DATABASE_URL") else {
|
||||
return;
|
||||
};
|
||||
let Ok(pool) = PgPool::connect(&url).await else {
|
||||
return;
|
||||
};
|
||||
let id = uuid::Uuid::new_v4();
|
||||
let err = regenerate_api_key(&pool, id)
|
||||
.await
|
||||
.err()
|
||||
.expect("expected error");
|
||||
assert!(matches!(
|
||||
err.downcast_ref::<AppError>(),
|
||||
Some(AppError::NotFoundUser)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -45,18 +45,27 @@ pub async fn build_env_map(
|
||||
|
||||
for f in fields {
|
||||
let decrypted = crypto::decrypt_json(master_key, &f.encrypted)?;
|
||||
let key = format!(
|
||||
"{}_{}",
|
||||
effective_prefix,
|
||||
f.name.to_uppercase().replace(['-', '.'], "_")
|
||||
);
|
||||
combined.insert(key, json_to_env_string(&decrypted));
|
||||
let seg = secret_name_to_env_segment(&f.name);
|
||||
let key = format!("{}_{}", effective_prefix, seg);
|
||||
if let Some(_old) = combined.insert(key.clone(), json_to_env_string(&decrypted)) {
|
||||
anyhow::bail!(
|
||||
"environment variable name collision after normalization: '{}' (secret '{}')",
|
||||
key,
|
||||
f.name
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(combined)
|
||||
}
|
||||
|
||||
/// Map a secret field name to an env key segment: `.` → `__`, `-` → `_`, then uppercase.
|
||||
/// Avoids collisions between e.g. `db.password` and `db_password`.
|
||||
fn secret_name_to_env_segment(name: &str) -> String {
|
||||
name.replace('.', "__").replace('-', "_").to_uppercase()
|
||||
}
|
||||
|
||||
fn env_prefix(entry: &crate::models::Entry, prefix: &str) -> String {
|
||||
let name_part = entry.name.to_uppercase().replace(['-', '.', ' '], "_");
|
||||
if prefix.is_empty() {
|
||||
@@ -75,3 +84,39 @@ fn json_to_env_string(v: &Value) -> String {
|
||||
other => other.to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
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");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -44,21 +44,23 @@ pub async fn export(
|
||||
|
||||
let mut export_entries: Vec<ExportEntry> = Vec::with_capacity(entries.len());
|
||||
for entry in &entries {
|
||||
let secrets = if params.no_secrets {
|
||||
None
|
||||
let (secrets, secret_types) = if params.no_secrets {
|
||||
(None, None)
|
||||
} else {
|
||||
let fields = secrets_map.get(&entry.id).map(Vec::as_slice).unwrap_or(&[]);
|
||||
if fields.is_empty() {
|
||||
Some(BTreeMap::new())
|
||||
(Some(BTreeMap::new()), Some(BTreeMap::new()))
|
||||
} else {
|
||||
let mk = master_key
|
||||
.ok_or_else(|| anyhow::anyhow!("master key required to decrypt secrets"))?;
|
||||
let mut map = BTreeMap::new();
|
||||
let mut type_map = BTreeMap::new();
|
||||
for f in fields {
|
||||
let decrypted = crypto::decrypt_json(mk, &f.encrypted)?;
|
||||
map.insert(f.name.clone(), decrypted);
|
||||
type_map.insert(f.name.clone(), f.secret_type.clone());
|
||||
}
|
||||
Some(map)
|
||||
(Some(map), Some(type_map))
|
||||
}
|
||||
};
|
||||
|
||||
@@ -70,6 +72,7 @@ pub async fn export(
|
||||
tags: entry.tags.clone(),
|
||||
metadata: entry.metadata.clone(),
|
||||
secrets,
|
||||
secret_types,
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use anyhow::Result;
|
||||
use sqlx::PgPool;
|
||||
use std::collections::HashMap;
|
||||
use uuid::Uuid;
|
||||
|
||||
use crate::models::ExportFormat;
|
||||
@@ -80,6 +81,11 @@ pub async fn run(
|
||||
|
||||
let secret_entries = build_secret_entries(entry.secrets.as_ref());
|
||||
let meta_entries = build_meta_entries(&entry.metadata);
|
||||
let secret_types_map: HashMap<String, String> = entry
|
||||
.secret_types
|
||||
.as_ref()
|
||||
.map(|m| m.iter().map(|(k, v)| (k.clone(), v.clone())).collect())
|
||||
.unwrap_or_default();
|
||||
|
||||
match add_run(
|
||||
pool,
|
||||
@@ -91,7 +97,7 @@ pub async fn run(
|
||||
tags: &entry.tags,
|
||||
meta_entries: &meta_entries,
|
||||
secret_entries: &secret_entries,
|
||||
secret_types: &Default::default(),
|
||||
secret_types: &secret_types_map,
|
||||
link_secret_names: &[],
|
||||
user_id: params.user_id,
|
||||
},
|
||||
@@ -125,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<String, String> {
|
||||
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());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -30,58 +30,61 @@ pub async fn run(
|
||||
folder: String,
|
||||
#[sqlx(rename = "type")]
|
||||
entry_type: String,
|
||||
name: String,
|
||||
version: i64,
|
||||
action: String,
|
||||
tags: Vec<String>,
|
||||
metadata: Value,
|
||||
}
|
||||
|
||||
let live_entry: Option<EntryWriteRow> = if let Some(uid) = user_id {
|
||||
let mut tx = pool.begin().await?;
|
||||
|
||||
let live: Option<EntryWriteRow> = if let Some(uid) = user_id {
|
||||
sqlx::query_as(
|
||||
"SELECT id, version, folder, type, name, tags, metadata, notes, deleted_at FROM entries \
|
||||
WHERE id = $1 AND user_id = $2 AND deleted_at IS NULL",
|
||||
WHERE id = $1 AND user_id = $2 AND deleted_at IS NULL FOR UPDATE",
|
||||
)
|
||||
.bind(entry_id)
|
||||
.bind(uid)
|
||||
.fetch_optional(pool)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?
|
||||
} else {
|
||||
sqlx::query_as(
|
||||
"SELECT id, version, folder, type, name, tags, metadata, notes, deleted_at FROM entries \
|
||||
WHERE id = $1 AND user_id IS NULL AND deleted_at IS NULL",
|
||||
WHERE id = $1 AND user_id IS NULL AND deleted_at IS NULL FOR UPDATE",
|
||||
)
|
||||
.bind(entry_id)
|
||||
.fetch_optional(pool)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?
|
||||
};
|
||||
|
||||
let live_entry = live_entry.ok_or(AppError::NotFoundEntry)?;
|
||||
let lr = live.ok_or(AppError::NotFoundEntry)?;
|
||||
|
||||
let snap: Option<EntryHistoryRow> = if let Some(ver) = to_version {
|
||||
sqlx::query_as(
|
||||
"SELECT folder, type, version, action, tags, metadata \
|
||||
"SELECT folder, type, name, version, action, tags, metadata \
|
||||
FROM entries_history \
|
||||
WHERE entry_id = $1 AND version = $2 ORDER BY id ASC LIMIT 1",
|
||||
)
|
||||
.bind(entry_id)
|
||||
.bind(ver)
|
||||
.fetch_optional(pool)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?
|
||||
} else {
|
||||
sqlx::query_as(
|
||||
"SELECT folder, type, version, action, tags, metadata \
|
||||
"SELECT folder, type, name, version, action, tags, metadata \
|
||||
FROM entries_history \
|
||||
WHERE entry_id = $1 ORDER BY id DESC LIMIT 1",
|
||||
)
|
||||
.bind(entry_id)
|
||||
.fetch_optional(pool)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?
|
||||
};
|
||||
|
||||
let snap = snap.ok_or_else(|| {
|
||||
anyhow::anyhow!(
|
||||
"No history found for entry '{}'{}.",
|
||||
live_entry.name,
|
||||
lr.name,
|
||||
to_version
|
||||
.map(|v| format!(" at version {}", v))
|
||||
.unwrap_or_default()
|
||||
@@ -91,17 +94,7 @@ pub async fn run(
|
||||
let snap_secret_snapshot = db::entry_secret_snapshot_from_metadata(&snap.metadata);
|
||||
let snap_metadata = db::strip_secret_snapshot_from_metadata(&snap.metadata);
|
||||
|
||||
let mut tx = pool.begin().await?;
|
||||
|
||||
let live: Option<EntryWriteRow> = sqlx::query_as(
|
||||
"SELECT id, version, folder, type, name, tags, metadata, notes, deleted_at FROM entries \
|
||||
WHERE id = $1 AND deleted_at IS NULL FOR UPDATE",
|
||||
)
|
||||
.bind(entry_id)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?;
|
||||
|
||||
let live_entry_id = if let Some(ref lr) = live {
|
||||
let live_entry_id = {
|
||||
let history_metadata =
|
||||
match db::metadata_with_secret_snapshot(&mut tx, lr.id, &lr.metadata).await {
|
||||
Ok(v) => v,
|
||||
@@ -168,8 +161,8 @@ pub async fn run(
|
||||
)
|
||||
.bind(&snap.folder)
|
||||
.bind(&snap.entry_type)
|
||||
.bind(&live_entry.name)
|
||||
.bind(&live_entry.notes)
|
||||
.bind(&snap.name)
|
||||
.bind(&lr.notes)
|
||||
.bind(&snap.tags)
|
||||
.bind(&snap_metadata)
|
||||
.bind(lr.id)
|
||||
@@ -177,8 +170,6 @@ pub async fn run(
|
||||
.await?;
|
||||
|
||||
lr.id
|
||||
} else {
|
||||
return Err(AppError::NotFoundEntry.into());
|
||||
};
|
||||
|
||||
if let Some(secret_snapshot) = snap_secret_snapshot {
|
||||
@@ -191,7 +182,7 @@ pub async fn run(
|
||||
"rollback",
|
||||
&snap.folder,
|
||||
&snap.entry_type,
|
||||
&live_entry.name,
|
||||
&snap.name,
|
||||
serde_json::json!({
|
||||
"entry_id": entry_id,
|
||||
"restored_version": snap.version,
|
||||
@@ -203,7 +194,7 @@ pub async fn run(
|
||||
tx.commit().await?;
|
||||
|
||||
Ok(RollbackResult {
|
||||
name: live_entry.name,
|
||||
name: snap.name,
|
||||
folder: snap.folder,
|
||||
entry_type: snap.entry_type,
|
||||
restored_version: snap.version,
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
[package]
|
||||
name = "secrets-mcp"
|
||||
version = "0.5.19"
|
||||
version = "0.5.21"
|
||||
edition.workspace = true
|
||||
|
||||
[[bin]]
|
||||
|
||||
@@ -345,15 +345,6 @@ impl SecretsService {
|
||||
Self::extract_enc_key(ctx)
|
||||
}
|
||||
|
||||
/// Require both user_id and encryption key (header only, no arg fallback).
|
||||
fn require_user_and_key(
|
||||
ctx: &RequestContext<RoleServer>,
|
||||
) -> Result<(Uuid, [u8; 32]), rmcp::ErrorData> {
|
||||
let user_id = Self::require_user_id(ctx)?;
|
||||
let key = Self::extract_enc_key(ctx)?;
|
||||
Ok((user_id, key))
|
||||
}
|
||||
|
||||
/// Require both user_id and encryption key, preferring an explicit argument
|
||||
/// value over the X-Encryption-Key header.
|
||||
fn require_user_and_key_or_arg(
|
||||
@@ -801,10 +792,7 @@ impl SecretsService {
|
||||
|
||||
let total_count = secrets_core::service::search::count_entries(&self.pool, &count_params)
|
||||
.await
|
||||
.inspect_err(
|
||||
|e| tracing::warn!(tool = "secrets_find", error = %e, "count_entries failed"),
|
||||
)
|
||||
.unwrap_or(0);
|
||||
.map_err(|e| mcp_err_internal_logged("secrets_find", Some(user_id), e))?;
|
||||
let relation_map = get_relations_for_entries(
|
||||
&self.pool,
|
||||
&result
|
||||
@@ -1135,11 +1123,8 @@ impl SecretsService {
|
||||
.await
|
||||
.map_err(|e| mcp_err_from_anyhow("secrets_add", Some(user_id), e))?;
|
||||
|
||||
let created_entry = resolve_entry(&self.pool, &input.name, Some(folder), Some(user_id))
|
||||
.await
|
||||
.map_err(|e| mcp_err_internal_logged("secrets_add", Some(user_id), e))?;
|
||||
for parent_id in parent_ids {
|
||||
add_parent_relation(&self.pool, parent_id, created_entry.id, Some(user_id))
|
||||
add_parent_relation(&self.pool, parent_id, result.entry_id, Some(user_id))
|
||||
.await
|
||||
.map_err(|e| mcp_err_from_anyhow("secrets_add", Some(user_id), e))?;
|
||||
}
|
||||
@@ -1420,7 +1405,7 @@ impl SecretsService {
|
||||
}
|
||||
|
||||
#[tool(
|
||||
description = "Rollback an entry to a previous version. Requires X-Encryption-Key header. \
|
||||
description = "Rollback an entry to a previous version. Requires Bearer API key only (no encryption key). \
|
||||
Omit to_version to restore the most recent snapshot. \
|
||||
Optionally pass 'id' (from secrets_find) to target directly.",
|
||||
annotations(title = "Rollback Secret Entry", destructive_hint = true)
|
||||
@@ -1431,7 +1416,7 @@ impl SecretsService {
|
||||
ctx: RequestContext<RoleServer>,
|
||||
) -> Result<CallToolResult, rmcp::ErrorData> {
|
||||
let t = Instant::now();
|
||||
let (user_id, _user_key) = Self::require_user_and_key(&ctx)?;
|
||||
let user_id = Self::require_user_id(&ctx)?;
|
||||
tracing::info!(
|
||||
tool = "secrets_rollback",
|
||||
?user_id,
|
||||
|
||||
@@ -11,7 +11,7 @@ use secrets_core::service::{
|
||||
|
||||
use crate::AppState;
|
||||
|
||||
use super::{SESSION_KEY_VERSION, current_user_id, render_template, require_valid_user};
|
||||
use super::{SESSION_KEY_VERSION, load_session_user_strict, render_template, require_valid_user};
|
||||
|
||||
#[derive(Template)]
|
||||
#[template(path = "dashboard.html")]
|
||||
@@ -92,17 +92,11 @@ pub(super) async fn api_key_salt(
|
||||
State(state): State<AppState>,
|
||||
session: Session,
|
||||
) -> Result<Json<KeySaltResponse>, StatusCode> {
|
||||
let user_id = current_user_id(&session)
|
||||
.await
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
|
||||
let user = get_user_by_id(&state.pool, user_id)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
tracing::error!(error = %e, %user_id, "failed to load user for key-salt API");
|
||||
StatusCode::INTERNAL_SERVER_ERROR
|
||||
})?
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
let user = match load_session_user_strict(&state.pool, &session).await {
|
||||
Ok(Some(u)) => u,
|
||||
Ok(None) => return Err(StatusCode::UNAUTHORIZED),
|
||||
Err(()) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
|
||||
};
|
||||
|
||||
if user.key_salt.is_none() {
|
||||
return Ok(Json(KeySaltResponse {
|
||||
@@ -126,19 +120,14 @@ pub(super) async fn api_key_setup(
|
||||
session: Session,
|
||||
Json(body): Json<KeySetupRequest>,
|
||||
) -> Result<Json<KeySetupResponse>, StatusCode> {
|
||||
let user_id = current_user_id(&session)
|
||||
.await
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
let user = match load_session_user_strict(&state.pool, &session).await {
|
||||
Ok(Some(u)) => u,
|
||||
Ok(None) => return Err(StatusCode::UNAUTHORIZED),
|
||||
Err(()) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
|
||||
};
|
||||
let user_id = user.id;
|
||||
|
||||
// Guard: if a passphrase is already configured, reject and direct to /api/key-change
|
||||
let user = get_user_by_id(&state.pool, user_id)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
tracing::error!(error = %e, %user_id, "failed to load user for key-setup guard");
|
||||
StatusCode::INTERNAL_SERVER_ERROR
|
||||
})?
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
|
||||
if user.key_salt.is_some() {
|
||||
tracing::warn!(%user_id, "key-setup called but passphrase already configured; use /api/key-change");
|
||||
return Err(StatusCode::CONFLICT);
|
||||
@@ -175,17 +164,12 @@ pub(super) async fn api_key_change(
|
||||
session: Session,
|
||||
Json(body): Json<KeyChangeRequest>,
|
||||
) -> Result<Json<KeySetupResponse>, StatusCode> {
|
||||
let user_id = current_user_id(&session)
|
||||
.await
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
|
||||
let user = get_user_by_id(&state.pool, user_id)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
tracing::error!(error = %e, %user_id, "failed to load user for key-change");
|
||||
StatusCode::INTERNAL_SERVER_ERROR
|
||||
})?
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
let user = match load_session_user_strict(&state.pool, &session).await {
|
||||
Ok(Some(u)) => u,
|
||||
Ok(None) => return Err(StatusCode::UNAUTHORIZED),
|
||||
Err(()) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
|
||||
};
|
||||
let user_id = user.id;
|
||||
|
||||
// Must have an existing passphrase to change
|
||||
let existing_key_check = user.key_check.ok_or_else(|| {
|
||||
@@ -276,9 +260,12 @@ pub(super) async fn api_apikey_get(
|
||||
State(state): State<AppState>,
|
||||
session: Session,
|
||||
) -> Result<Json<ApiKeyResponse>, StatusCode> {
|
||||
let user_id = current_user_id(&session)
|
||||
.await
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
let user = match load_session_user_strict(&state.pool, &session).await {
|
||||
Ok(Some(u)) => u,
|
||||
Ok(None) => return Err(StatusCode::UNAUTHORIZED),
|
||||
Err(()) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
|
||||
};
|
||||
let user_id = user.id;
|
||||
|
||||
let api_key = ensure_api_key(&state.pool, user_id).await.map_err(|e| {
|
||||
tracing::error!(error = %e, %user_id, "ensure_api_key failed");
|
||||
@@ -292,9 +279,12 @@ pub(super) async fn api_apikey_regenerate(
|
||||
State(state): State<AppState>,
|
||||
session: Session,
|
||||
) -> Result<Json<ApiKeyResponse>, StatusCode> {
|
||||
let user_id = current_user_id(&session)
|
||||
.await
|
||||
.ok_or(StatusCode::UNAUTHORIZED)?;
|
||||
let user = match load_session_user_strict(&state.pool, &session).await {
|
||||
Ok(Some(u)) => u,
|
||||
Ok(None) => return Err(StatusCode::UNAUTHORIZED),
|
||||
Err(()) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
|
||||
};
|
||||
let user_id = user.id;
|
||||
|
||||
let api_key = regenerate_api_key(&state.pool, user_id)
|
||||
.await
|
||||
|
||||
@@ -25,8 +25,8 @@ use secrets_core::service::{
|
||||
use crate::AppState;
|
||||
|
||||
use super::{
|
||||
ENTRIES_PAGE_LIMIT, UiLang, current_user_id, paginate, render_template, request_ui_lang,
|
||||
require_valid_user, tr,
|
||||
ENTRIES_PAGE_LIMIT, UiLang, paginate, render_template, request_ui_lang, require_valid_user,
|
||||
require_valid_user_json, tr,
|
||||
};
|
||||
|
||||
// ── Template types ────────────────────────────────────────────────────────────
|
||||
@@ -616,10 +616,8 @@ pub(super) async fn api_entry_patch(
|
||||
Json(body): Json<EntryPatchBody>,
|
||||
) -> Result<Json<serde_json::Value>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
let folder = body.folder.trim();
|
||||
let entry_type = body.entry_type.trim();
|
||||
@@ -635,6 +633,71 @@ pub(super) async fn api_entry_patch(
|
||||
));
|
||||
}
|
||||
|
||||
if folder.chars().count() > crate::validation::MAX_FOLDER_LENGTH {
|
||||
return Err((
|
||||
StatusCode::BAD_REQUEST,
|
||||
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": 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": 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": format!(
|
||||
"{} {} {}",
|
||||
tr(
|
||||
lang,
|
||||
"notes 长度不能超过",
|
||||
"notes 長度不能超過",
|
||||
"notes must be at most"
|
||||
),
|
||||
crate::validation::MAX_NOTES_LENGTH,
|
||||
tr(lang, " 个字符", " 個字元", " characters")
|
||||
) })),
|
||||
));
|
||||
}
|
||||
|
||||
let tags: Vec<String> = body
|
||||
.tags
|
||||
.into_iter()
|
||||
@@ -683,10 +746,8 @@ pub(super) async fn api_entry_options(
|
||||
Query(q): Query<EntryOptionQuery>,
|
||||
) -> Result<Json<serde_json::Value>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
let query =
|
||||
q.q.as_deref()
|
||||
@@ -738,10 +799,8 @@ pub(super) async fn api_entry_delete(
|
||||
Path(entry_id): Path<Uuid>,
|
||||
) -> Result<Json<serde_json::Value>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
delete_by_id(&state.pool, entry_id, user_id)
|
||||
.await
|
||||
@@ -760,10 +819,8 @@ pub(super) async fn api_trash_restore(
|
||||
Path(entry_id): Path<Uuid>,
|
||||
) -> Result<Json<serde_json::Value>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
restore_deleted_by_id(&state.pool, entry_id, user_id)
|
||||
.await
|
||||
@@ -782,10 +839,8 @@ pub(super) async fn api_trash_purge(
|
||||
Path(entry_id): Path<Uuid>,
|
||||
) -> Result<Json<serde_json::Value>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
purge_deleted_by_id(&state.pool, entry_id, user_id)
|
||||
.await
|
||||
@@ -818,10 +873,8 @@ pub(super) async fn api_secret_check_name(
|
||||
Query(params): Query<SecretCheckNameQuery>,
|
||||
) -> Result<Json<SecretCheckNameResponse>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
let name = params.name.trim();
|
||||
if name.is_empty() {
|
||||
@@ -914,10 +967,8 @@ pub(super) async fn api_secret_patch(
|
||||
}
|
||||
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
let name = body.name.as_ref().map(|s| s.trim());
|
||||
let secret_type = body.secret_type.as_ref().map(|s| s.trim());
|
||||
@@ -1123,10 +1174,8 @@ pub(super) async fn api_entry_secret_unlink(
|
||||
}
|
||||
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
let mut tx = state
|
||||
.pool
|
||||
@@ -1216,10 +1265,8 @@ pub(super) async fn api_entry_secrets_decrypt(
|
||||
Path(entry_id): Path<Uuid>,
|
||||
) -> Result<Json<serde_json::Value>, EntryApiError> {
|
||||
let lang = request_ui_lang(&headers);
|
||||
let user_id = current_user_id(&session).await.ok_or((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
))?;
|
||||
let user = require_valid_user_json(&state.pool, &session, lang).await?;
|
||||
let user_id = user.id;
|
||||
|
||||
let master_key = require_encryption_key(&headers, lang)?;
|
||||
|
||||
|
||||
@@ -1,10 +1,11 @@
|
||||
use askama::Template;
|
||||
use axum::{
|
||||
Router,
|
||||
Json, Router,
|
||||
http::{HeaderMap, StatusCode, header},
|
||||
response::{Html, IntoResponse, Redirect, Response},
|
||||
routing::{get, patch, post},
|
||||
};
|
||||
use serde_json::json;
|
||||
use tower_sessions::Session;
|
||||
use uuid::Uuid;
|
||||
|
||||
@@ -34,7 +35,7 @@ const AUDIT_PAGE_LIMIT: i64 = 10;
|
||||
// ── UI language ───────────────────────────────────────────────────────────────
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum UiLang {
|
||||
pub(super) enum UiLang {
|
||||
ZhCn,
|
||||
ZhTw,
|
||||
En,
|
||||
@@ -143,6 +144,71 @@ async fn require_valid_user(
|
||||
Ok(user)
|
||||
}
|
||||
|
||||
/// `Ok(None)` — unauthenticated or session invalidated (including `key_version` mismatch).
|
||||
/// `Err(())` — database error loading the user.
|
||||
pub(super) async fn load_session_user_strict(
|
||||
pool: &sqlx::PgPool,
|
||||
session: &Session,
|
||||
) -> Result<Option<secrets_core::models::User>, ()> {
|
||||
let Some(user_id) = current_user_id(session).await else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let user = match secrets_core::service::user::get_user_by_id(pool, user_id).await {
|
||||
Err(e) => {
|
||||
tracing::error!(error = %e, %user_id, "load_session_user_strict: failed to load user");
|
||||
return Err(());
|
||||
}
|
||||
Ok(None) => {
|
||||
if let Err(e) = session.flush().await {
|
||||
tracing::warn!(error = %e, "failed to flush stale session");
|
||||
}
|
||||
return Ok(None);
|
||||
}
|
||||
Ok(Some(u)) => u,
|
||||
};
|
||||
|
||||
let session_kv: Option<i64> = match session.get::<i64>(SESSION_KEY_VERSION).await {
|
||||
Ok(v) => v,
|
||||
Err(e) => {
|
||||
tracing::warn!(error = %e, "failed to read key_version from session; treating as missing");
|
||||
None
|
||||
}
|
||||
};
|
||||
if let Some(kv) = session_kv
|
||||
&& kv != user.key_version
|
||||
{
|
||||
tracing::info!(%user_id, session_kv = kv, db_kv = user.key_version, "key_version mismatch; invalidating session (API)");
|
||||
if let Err(e) = session.flush().await {
|
||||
tracing::warn!(error = %e, "failed to flush outdated session");
|
||||
}
|
||||
return Ok(None);
|
||||
}
|
||||
|
||||
Ok(Some(user))
|
||||
}
|
||||
|
||||
/// JSON API equivalent of [`require_valid_user`]: returns `401` with a JSON body instead of redirecting.
|
||||
pub(super) async fn require_valid_user_json(
|
||||
pool: &sqlx::PgPool,
|
||||
session: &Session,
|
||||
lang: UiLang,
|
||||
) -> Result<secrets_core::models::User, (StatusCode, Json<serde_json::Value>)> {
|
||||
match load_session_user_strict(pool, session).await {
|
||||
Ok(Some(user)) => Ok(user),
|
||||
Ok(None) => Err((
|
||||
StatusCode::UNAUTHORIZED,
|
||||
Json(json!({ "error": tr(lang, "未登录", "尚未登入", "Not logged in") })),
|
||||
)),
|
||||
Err(()) => Err((
|
||||
StatusCode::INTERNAL_SERVER_ERROR,
|
||||
Json(
|
||||
json!({ "error": tr(lang, "操作失败,请稍后重试", "操作失敗,請稍後重試", "Operation failed, please try again later") }),
|
||||
),
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
fn request_user_agent(headers: &HeaderMap) -> Option<String> {
|
||||
headers
|
||||
.get(header::USER_AGENT)
|
||||
|
||||
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`。
|
||||
143
plans/merge-code-review-fixes-2026-04-11.md
Normal file
143
plans/merge-code-review-fixes-2026-04-11.md
Normal file
@@ -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` 为基础继续补测试,而不是尝试把两份实现整合成第三套逻辑。这样改动面最小,风险也最低。
|
||||
Reference in New Issue
Block a user