Skip to content

fix: Preserve existing secret-key in config when starting CLIProxyAPI#41

Open
zhuziyu wants to merge 1 commit intorouter-for-me:mainfrom
zhuziyu:main
Open

fix: Preserve existing secret-key in config when starting CLIProxyAPI#41
zhuziyu wants to merge 1 commit intorouter-for-me:mainfrom
zhuziyu:main

Conversation

@zhuziyu
Copy link
Copy Markdown

@zhuziyu zhuziyu commented Mar 31, 2026

No description provided.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the start_cliproxyapi and restart_cliproxyapi functions to check for an existing secret-key in the configuration before generating a new one. The review feedback identifies several unsafe .unwrap() calls that could cause the application to panic if the configuration file is malformed. It is recommended to use the entry API for more robust mapping operations and to refactor the duplicated logic between the two functions into a shared helper.

Comment on lines +1143 to 1164
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
);
}

// Ensure remote-management section exists
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
let rm = conf
.as_mapping_mut()
.unwrap()
.get_mut(&serde_yaml::Value::from("remote-management"))
.unwrap()
.as_mapping_mut()
.unwrap();
rm.insert(
serde_yaml::Value::from("secret-key"),
serde_yaml::Value::from(new_pass.as_str()),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of .unwrap() on the configuration mapping is unsafe as it will cause the application to panic if the config.yaml file is malformed or empty (e.g., if it parses to Value::Null). Additionally, the logic for ensuring the remote-management section exists can be simplified and made more robust using the entry API. Finally, this entire block of logic is duplicated in restart_cliproxyapi and should be refactored into a shared helper function to improve maintainability.

Suggested change
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
);
}
// Ensure remote-management section exists
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
let rm = conf
.as_mapping_mut()
.unwrap()
.get_mut(&serde_yaml::Value::from("remote-management"))
.unwrap()
.as_mapping_mut()
.unwrap();
rm.insert(
serde_yaml::Value::from("secret-key"),
serde_yaml::Value::from(new_pass.as_str()),
);
let rm = conf
.as_mapping_mut()
.ok_or_else(|| "Invalid config structure".to_string())?
.entry(serde_yaml::Value::from("remote-management"))
.or_insert_with(|| serde_yaml::Value::Mapping(Default::default()))
.as_mapping_mut()
.ok_or_else(|| "remote-management is not a mapping".to_string())?;
rm.insert(
serde_yaml::Value::from("secret-key"),
serde_yaml::Value::from(new_pass.as_str()),
);

Comment on lines +1282 to 1303
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
);
}

// Ensure remote-management section exists
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
let rm = conf
.as_mapping_mut()
.unwrap()
.get_mut(&serde_yaml::Value::from("remote-management"))
.unwrap()
.as_mapping_mut()
.unwrap();
rm.insert(
serde_yaml::Value::from("secret-key"),
serde_yaml::Value::from(new_pass.as_str()),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the implementation in start_cliproxyapi, the use of .unwrap() here is unsafe and can lead to panics if the configuration file structure is unexpected. Using the entry API provides a cleaner and safer way to manage the configuration mapping. This logic is identical to the one in start_cliproxyapi and should be refactored to avoid duplication.

Suggested change
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
);
}
// Ensure remote-management section exists
if !conf
.as_mapping()
.unwrap()
.contains_key(&serde_yaml::Value::from("remote-management"))
{
conf.as_mapping_mut().unwrap().insert(
serde_yaml::Value::from("remote-management"),
serde_yaml::Value::Mapping(Default::default()),
let rm = conf
.as_mapping_mut()
.unwrap()
.get_mut(&serde_yaml::Value::from("remote-management"))
.unwrap()
.as_mapping_mut()
.unwrap();
rm.insert(
serde_yaml::Value::from("secret-key"),
serde_yaml::Value::from(new_pass.as_str()),
);
let rm = conf
.as_mapping_mut()
.ok_or_else(|| "Invalid config structure".to_string())?
.entry(serde_yaml::Value::from("remote-management"))
.or_insert_with(|| serde_yaml::Value::Mapping(Default::default()))
.as_mapping_mut()
.ok_or_else(|| "remote-management is not a mapping".to_string())?;
rm.insert(
serde_yaml::Value::from("secret-key"),
serde_yaml::Value::from(new_pass.as_str()),
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant