fix(config): prevent API provider loss during alias updates and migration
- Validate provider integrity before saving alias changes - Reject rendered YAML that differs from the expected configuration - Abort migration when existing config cannot be read or parsed - Remove lossy config recovery fallback - Add regression tests for provider loss, locked files, malformed config, and repeated updates
This commit is contained in:
1 parent
88ea6f1c9a
commit
9042421292
7 files changed
+166
-105
No files matched your search
@@ -5,6 +5,47 @@ fn alias_config_is_unchanged(current: &str, latest: &str) -> Result<bool, String
|
||||
Ok(changes.oauth_model_aliases.is_none() && !changes.update_config_yaml)
|
||||
}
|
||||
|
||||
fn validate_alias_api_access_preserved(current: &str, updated: &str) -> Result<(), String> {
|
||||
let parse = |content: &str| {
|
||||
serde_norway::from_str::<serde_norway::Value>(content)
|
||||
.map_err(|error| format!("解析内核 YAML 配置失败: {error}"))
|
||||
};
|
||||
let current = parse(current)?;
|
||||
let updated = parse(updated)?;
|
||||
for section in MODEL_ALIAS_CONFIG_SECTIONS
|
||||
.iter()
|
||||
.copied()
|
||||
.chain(["vertex-api-key", "xai-api-key", "interactions-api-key"])
|
||||
{
|
||||
let without_models = |root: &serde_norway::Value| -> Result<_, String> {
|
||||
let mut value = root
|
||||
.get(section)
|
||||
.cloned()
|
||||
.unwrap_or(serde_norway::Value::Null);
|
||||
if value.is_null() {
|
||||
return Ok(serde_norway::Value::Sequence(Vec::new()));
|
||||
}
|
||||
let providers = value
|
||||
.as_sequence_mut()
|
||||
.ok_or_else(|| format!("{section} 必须是数组,已拒绝保存别名"))?;
|
||||
if MODEL_ALIAS_CONFIG_SECTIONS.contains(§ion) {
|
||||
for provider in providers {
|
||||
if let Some(provider) = provider.as_mapping_mut() {
|
||||
provider.remove(yaml_key("models"));
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(value)
|
||||
};
|
||||
if without_models(¤t)? != without_models(&updated)? {
|
||||
return Err(format!(
|
||||
"别名更新意外改变了 API 接入配置({section}),已拒绝写入"
|
||||
));
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn put_management_alias_config_changes(
|
||||
config: &GuiConfigFile,
|
||||
current: &str,
|
||||
@@ -25,6 +66,7 @@ pub(crate) async fn commit_management_alias_config_changes<T>(
|
||||
) -> Result<T, String> {
|
||||
static SAVE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());
|
||||
let _guard = SAVE_LOCK.lock().await;
|
||||
validate_alias_api_access_preserved(current, updated)?;
|
||||
let changes = management_alias_config_changes(current, updated)?;
|
||||
let latest = fetch_management_config_yaml(config).await?;
|
||||
if !alias_config_is_unchanged(current, &latest)? {
|
||||
|
||||
@@ -2279,8 +2279,15 @@ pub(crate) fn render_updated_core_yaml(
|
||||
document.set(updated);
|
||||
let rendered = expand_top_level_flow_style_collections(&document.get_string(), document.get())?;
|
||||
let rendered = indent_indentationless_yaml_sequences(&rendered);
|
||||
serde_norway::from_str::<serde_norway::Value>(&rendered)
|
||||
let validated = serde_norway::from_str::<serde_norway::Value>(&rendered)
|
||||
.map_err(|error| format!("验证更新后的内核配置失败: {error}"))?;
|
||||
if &validated != document.get() {
|
||||
let path = first_yaml_mismatch_path(document.get(), &validated, &mut Vec::new())
|
||||
.unwrap_or_else(|| "<unknown>".to_string());
|
||||
return Err(format!(
|
||||
"更新后的内核配置与预期值不一致(路径: {path}),已拒绝写入"
|
||||
));
|
||||
}
|
||||
Ok(rendered)
|
||||
}
|
||||
|
||||
|
||||
@@ -276,17 +276,6 @@ pub(crate) fn merge_core_config_fields(
|
||||
merge_core_config_value(template, current_value)
|
||||
}
|
||||
|
||||
pub(crate) fn merge_core_config_fields_tolerant(
|
||||
template: &str,
|
||||
current: &str,
|
||||
) -> Result<String, String> {
|
||||
let current = match serde_norway::from_str::<serde_norway::Value>(current) {
|
||||
Ok(current) if current.is_mapping() => current,
|
||||
_ => recover_parseable_top_level_yaml_fields(current),
|
||||
};
|
||||
merge_core_config_value(template, Some(current))
|
||||
}
|
||||
|
||||
pub(crate) fn merge_core_config_value(
|
||||
template: &str,
|
||||
current: Option<serde_norway::Value>,
|
||||
@@ -311,75 +300,6 @@ pub(crate) fn merge_core_config_value(
|
||||
Ok(rendered)
|
||||
}
|
||||
|
||||
pub(crate) fn recover_parseable_top_level_yaml_fields(content: &str) -> serde_norway::Value {
|
||||
let lines = yaml_line_ranges(content);
|
||||
let boundaries = lines
|
||||
.iter()
|
||||
.copied()
|
||||
.filter(|range| is_top_level_yaml_boundary(yaml_line_content(content, *range)))
|
||||
.collect::<Vec<_>>();
|
||||
let mut recovered = serde_norway::Mapping::new();
|
||||
|
||||
for (index, (start, _)) in boundaries.iter().copied().enumerate() {
|
||||
let line = yaml_line_content(content, boundaries[index]);
|
||||
if !is_top_level_yaml_mapping_field(line) {
|
||||
continue;
|
||||
}
|
||||
let end = boundaries
|
||||
.get(index + 1)
|
||||
.map(|(next_start, _)| *next_start)
|
||||
.unwrap_or(content.len());
|
||||
let Ok(value) = serde_norway::from_str::<serde_norway::Value>(&content[start..end]) else {
|
||||
continue;
|
||||
};
|
||||
let Some(mapping) = value.as_mapping() else {
|
||||
continue;
|
||||
};
|
||||
for (key, value) in mapping {
|
||||
recovered.insert(key.clone(), value.clone());
|
||||
}
|
||||
}
|
||||
|
||||
serde_norway::Value::Mapping(recovered)
|
||||
}
|
||||
|
||||
pub(crate) fn is_top_level_yaml_boundary(line: &str) -> bool {
|
||||
if line.is_empty()
|
||||
|| line.chars().next().is_some_and(char::is_whitespace)
|
||||
|| line.starts_with('#')
|
||||
|| is_indentationless_yaml_sequence_item(line)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
!matches!(line.trim(), "---" | "...") && !line.starts_with('%')
|
||||
}
|
||||
|
||||
pub(crate) fn is_top_level_yaml_mapping_field(line: &str) -> bool {
|
||||
let mut single_quoted = false;
|
||||
let mut double_quoted = false;
|
||||
let mut escaped = false;
|
||||
for (index, character) in line.char_indices() {
|
||||
if escaped {
|
||||
escaped = false;
|
||||
continue;
|
||||
}
|
||||
if double_quoted && character == '\\' {
|
||||
escaped = true;
|
||||
continue;
|
||||
}
|
||||
match character {
|
||||
'\'' if !double_quoted => single_quoted = !single_quoted,
|
||||
'"' if !single_quoted => double_quoted = !double_quoted,
|
||||
':' if !single_quoted && !double_quoted => {
|
||||
let rest = &line[index + character.len_utf8()..];
|
||||
return rest.is_empty() || rest.chars().next().is_some_and(char::is_whitespace);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
pub(crate) fn patch_core_network_yaml(
|
||||
content: &str,
|
||||
config: &GuiConfigFile,
|
||||
|
||||
@@ -1834,9 +1834,16 @@ pub(crate) fn migrate_core_config_for_update(
|
||||
return Ok(());
|
||||
}
|
||||
let old_config_path = source_dir.join(CORE_CONFIG_FILE);
|
||||
if !old_config_path.is_file() {
|
||||
return Ok(());
|
||||
}
|
||||
let old_config = match fs::read_to_string(&old_config_path) {
|
||||
Ok(content) => content,
|
||||
Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()),
|
||||
Err(error) => {
|
||||
return Err(format!(
|
||||
"读取旧版内核配置失败,为避免配置丢失已取消更新 {}: {error}",
|
||||
path_to_string(&old_config_path)
|
||||
));
|
||||
}
|
||||
};
|
||||
|
||||
let template_path = target_dir.join(CORE_EXAMPLE_CONFIG_FILE);
|
||||
if !template_path.is_file() {
|
||||
@@ -1845,23 +1852,13 @@ pub(crate) fn migrate_core_config_for_update(
|
||||
path_to_string(&template_path)
|
||||
));
|
||||
}
|
||||
let old_config = match fs::read(&old_config_path) {
|
||||
Ok(content) => String::from_utf8_lossy(&content).into_owned(),
|
||||
Err(error) => {
|
||||
eprintln!(
|
||||
"读取旧版内核配置失败,将使用新版默认配置继续更新 {}: {error}",
|
||||
path_to_string(&old_config_path)
|
||||
);
|
||||
String::new()
|
||||
}
|
||||
};
|
||||
let template = fs::read_to_string(&template_path).map_err(|error| {
|
||||
format!(
|
||||
"读取新版内核配置模板失败 {}: {error}",
|
||||
path_to_string(&template_path)
|
||||
)
|
||||
})?;
|
||||
let migrated = merge_core_config_fields_tolerant(&template, &old_config)?;
|
||||
let migrated = merge_core_config_fields(&template, Some(&old_config))?;
|
||||
let config_path = target_dir.join(CORE_CONFIG_FILE);
|
||||
fs::write(&config_path, migrated).map_err(|error| {
|
||||
format!(
|
||||
|
||||
@@ -16,6 +16,42 @@ enum Failure {
|
||||
ForeignChange,
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn alias_save_rejects_loss_of_api_access_before_any_write() {
|
||||
let initial = format!("{CURRENT}codex-api-key:\n - api-key: test-key\n base-url: https://example.test\n models: [{{name: model-a}}]\nopenai-compatibility:\n - name: preserved\n disabled: true\n api-key-entries: [{{api-key: other-test-key}}]\n models: [{{name: model-b}}]\n");
|
||||
for section in ["codex-api-key", "openai-compatibility"] {
|
||||
for mode in ["missing", "empty", "credential"] {
|
||||
let mut corrupted = yaml_json(&initial);
|
||||
match mode {
|
||||
"missing" => {
|
||||
corrupted.as_object_mut().unwrap().remove(section);
|
||||
}
|
||||
"empty" => corrupted[section] = serde_json::json!([]),
|
||||
_ => {
|
||||
corrupted[section][0]
|
||||
.as_object_mut()
|
||||
.unwrap()
|
||||
.retain(|key, _| !key.starts_with("api-key"));
|
||||
}
|
||||
}
|
||||
let core = MockCore::new(&initial, Failure::None);
|
||||
let result = put_management_alias_config_changes(
|
||||
&core.config,
|
||||
&initial,
|
||||
&serde_norway::to_string(&corrupted).unwrap(),
|
||||
)
|
||||
.await;
|
||||
let (persisted, requests) = core.finish();
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"accepted provider loss: {section}/{mode}"
|
||||
);
|
||||
assert_eq!(persisted, yaml_json(&initial));
|
||||
assert!(!requests.iter().any(|request| request.starts_with("PUT")));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
struct MockCore {
|
||||
config: GuiConfigFile,
|
||||
finished: Arc<AtomicBool>,
|
||||
|
||||
@@ -556,7 +556,7 @@ fn replacing_a_core_migrates_old_fields_into_the_new_template() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn replacing_a_core_discards_invalid_fields_and_continues_migration() {
|
||||
fn replacing_a_core_rejects_invalid_config_without_overwriting_files() {
|
||||
let root = agent_test_home("core-config-migrate-invalid");
|
||||
let source = root.join("source");
|
||||
let target = root.join("target");
|
||||
@@ -574,16 +574,46 @@ fn replacing_a_core_discards_invalid_fields_and_continues_migration() {
|
||||
.unwrap();
|
||||
fs::write(target.join(CORE_CONFIG_FILE), "staged: untouched\n").unwrap();
|
||||
|
||||
migrate_core_config_for_update(&source, &target).unwrap();
|
||||
let original = fs::read(source.join(CORE_CONFIG_FILE)).unwrap();
|
||||
assert!(migrate_core_config_for_update(&source, &target).is_err());
|
||||
assert_eq!(fs::read(source.join(CORE_CONFIG_FILE)).unwrap(), original);
|
||||
assert_eq!(
|
||||
fs::read_to_string(target.join(CORE_CONFIG_FILE)).unwrap(),
|
||||
"staged: untouched\n"
|
||||
);
|
||||
fs::remove_dir_all(root).unwrap();
|
||||
}
|
||||
|
||||
let migrated = fs::read_to_string(target.join(CORE_CONFIG_FILE)).unwrap();
|
||||
let document = serde_norway::from_str::<serde_norway::Value>(&migrated).unwrap();
|
||||
assert_eq!(document["host"], "127.0.0.1");
|
||||
assert_eq!(document["broken"], "new-default");
|
||||
assert_eq!(document["port"], 9527);
|
||||
assert_eq!(document["api-keys"][0], "old-a");
|
||||
assert_eq!(document["api-keys"][1], "old-b");
|
||||
assert_eq!(document["new-option"], true);
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn replacing_a_core_rejects_locked_config_without_using_defaults() {
|
||||
use std::os::windows::fs::OpenOptionsExt;
|
||||
let root = agent_test_home("core-config-locked");
|
||||
let source = root.join("source");
|
||||
let target = root.join("target");
|
||||
fs::create_dir_all(&source).unwrap();
|
||||
fs::create_dir_all(&target).unwrap();
|
||||
let original = "codex-api-key:\n - api-key: test-key\n base-url: https://example.test\n";
|
||||
let source_path = source.join(CORE_CONFIG_FILE);
|
||||
fs::write(&source_path, original).unwrap();
|
||||
fs::write(target.join(CORE_EXAMPLE_CONFIG_FILE), "port: 8317\n").unwrap();
|
||||
fs::write(target.join(CORE_CONFIG_FILE), "staged: untouched\n").unwrap();
|
||||
let locked = fs::OpenOptions::new()
|
||||
.read(true)
|
||||
.share_mode(0)
|
||||
.open(&source_path)
|
||||
.unwrap();
|
||||
let result = migrate_core_config_for_update(&source, &target);
|
||||
drop(locked);
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"unreadable configuration must not be replaced by defaults"
|
||||
);
|
||||
assert_eq!(fs::read_to_string(source_path).unwrap(), original);
|
||||
assert_eq!(
|
||||
fs::read_to_string(target.join(CORE_CONFIG_FILE)).unwrap(),
|
||||
"staged: untouched\n"
|
||||
);
|
||||
fs::remove_dir_all(root).unwrap();
|
||||
}
|
||||
|
||||
|
||||
@@ -5,6 +5,35 @@ fn json(content: &str) -> serde_json::Value {
|
||||
serde_json::to_value(serde_norway::from_str::<serde_norway::Value>(content).unwrap()).unwrap()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn desktop_routes_preserve_api_access_when_switching_models() {
|
||||
let input = "codex-api-key:\n - api-key: codex-test-key\n base-url: https://codex.example.test\n proxy-url: socks5://127.0.0.1:1080\n models: [{name: model-a}, {name: model-b}]\nopenai-compatibility:\n - name: other\n disabled: true\n api-key-entries: [{api-key: other-test-key}]\n models: [{name: other-model}]\npayload:\n override:\n - models: [{name: other-model}]\n params: {custom: retained}\n";
|
||||
let before = json(input);
|
||||
let mut content = input.to_string();
|
||||
for selected in ["model-a", "model-b", "model-b"] {
|
||||
content = ensure_claude_desktop_model_aliases_in_yaml(
|
||||
&content,
|
||||
&ClaudeDesktopModelMappings::all(selected),
|
||||
&test_agent_models(&["model-a", "model-b"]),
|
||||
)
|
||||
.unwrap();
|
||||
let mut after = json(&content);
|
||||
let models = after["codex-api-key"][0]["models"].as_array_mut().unwrap();
|
||||
assert_eq!(models.len(), 5);
|
||||
for route in [
|
||||
CLAUDE_DESKTOP_OPUS_MODEL_ID,
|
||||
CLAUDE_DESKTOP_SONNET_MODEL_ID,
|
||||
CLAUDE_DESKTOP_HAIKU_MODEL_ID,
|
||||
] {
|
||||
assert!(models
|
||||
.iter()
|
||||
.any(|model| model["alias"] == route && model["name"] == selected));
|
||||
}
|
||||
models.retain(|model| model.get("display-name").is_none());
|
||||
assert_eq!(after, before);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn desktop_routes_move_from_disabled_provider_to_enabled_source() {
|
||||
let models = test_agent_models(&["grok-4.6"]);
|
||||
|
||||
Reference in new issue
Block a user