Skip to content

Improvements and bug fixes #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Digiyang merged 2 commits into from
Feb 16, 2026
Merged

Improvements and bug fixes #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c67e78
Add input retry loops and default cipher selection
Digiyang Feb 16, 2026
f29c7b2
Sanitize User ID in file path construction
Digiyang Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/sequoia_openpgp/certificate_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ use crate::app::launch::clear_screen;
use crate::utils::create_directory::{create_file, create_secret_file};
use crate::utils::parse_iso8601_duration::parse_iso8601_duration;

/// Sanitize a user ID for use as a filename component.
/// Replaces `..`, `/`, and `\` with `_` to prevent path traversal.
fn sanitize_for_path(input: &str) -> String {
input.replace("..", "_").replace(['/', '\\'], "_")
}

#[derive(Debug, Clone, Copy)]
pub struct CertificateManager;

Expand Down Expand Up @@ -294,7 +300,7 @@ impl CertificateManager {
clear_screen();
let mut builder = CertBuilder::new();

let uid_clone = user_id.clone();
let safe_uid = sanitize_for_path(&user_id);
builder = builder.add_userid(user_id);

match validity.as_str() {
Expand All @@ -319,7 +325,7 @@ impl CertificateManager {
}

match cipher.as_str() {
"1" => builder = builder.set_cipher_suite(CipherSuite::Cv25519),
"1" | "" => builder = builder.set_cipher_suite(CipherSuite::Cv25519),
"2" => builder = builder.set_cipher_suite(CipherSuite::RSA2k),
"3" => builder = builder.set_cipher_suite(CipherSuite::RSA3k),
"4" => builder = builder.set_cipher_suite(CipherSuite::RSA4k),
Expand Down Expand Up @@ -352,11 +358,11 @@ impl CertificateManager {
let home_dir = home::home_dir()
.ok_or_else(|| anyhow::anyhow!("Could not determine home directory"))?;
let key_path =
format!("{}/.pgpman/secrets/{}.pgp", &home_dir.display(), uid_clone).replace(" ", "");
format!("{}/.pgpman/secrets/{}.pgp", &home_dir.display(), safe_uid).replace(" ", "");
let revcert_path = format!(
"{}/.pgpman/revocation/{}.rev",
&home_dir.display(),
uid_clone
safe_uid
)
.replace(" ", "");
// export key to path
Expand All @@ -381,7 +387,7 @@ impl CertificateManager {
let cert_path = format!(
"{}/.pgpman/certificates/{}.pgp",
&home_dir.display(),
uid_clone
safe_uid
)
.replace(" ", "");
{
Expand Down
165 changes: 97 additions & 68 deletions src/sequoia_openpgp/wrapper_fun.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use ratatui::{
};
use sequoia_openpgp::{crypto::Password, parse::Parse, Cert};

use crate::app::ui::{draw_input_prompt, show_user_selection_popup};
use crate::app::ui::{draw_input_prompt, show_input_popup, show_user_selection_popup};

use super::certificate_manager::CertificateManager;

Expand Down Expand Up @@ -47,15 +47,19 @@ pub fn generate_keypair_tui(
terminal: &mut Terminal<CrosstermBackend<std::io::Stdout>>,
) -> Result<(), anyhow::Error> {
let style = Style::default().fg(Color::Yellow);
let user_id = draw_input_prompt(
terminal,
&[Line::from(Span::styled(
"Enter user id (e.g 'Bob <Bob@domain.de>):",
style,
))],
true,
)?;
validate_not_empty(&user_id, "User ID")?;
let user_id_prompt = [Line::from(Span::styled(
"Enter user id (e.g 'Bob <Bob@domain.de>):",
style,
))];
let user_id = loop {
let input = draw_input_prompt(terminal, &user_id_prompt, true)?;
match validate_not_empty(&input, "User ID") {
Ok(()) => break input,
Err(e) => {
show_input_popup(terminal, &e.to_string())?;
}
}
};

let prompt_validity = vec![
"Please specify a validity duration for your key:",
Expand All @@ -72,7 +76,7 @@ pub fn generate_keypair_tui(
.collect();

let prompt_cipher = vec![
"Please select a cypher:",
"Please select a cypher (default: Cv25519):",
" (1) Cv25519",
" EdDSA and ECDH over Curve25519 with SHA512 and AES256",
" (2) RSA2k",
Expand All @@ -96,25 +100,28 @@ pub fn generate_keypair_tui(

let validity = draw_input_prompt(terminal, &validity_spans, true)?;
let cipher = draw_input_prompt(terminal, &cipher_spans, true)?;
let pw = draw_input_prompt(
terminal,
&[Line::from(Span::styled(
"Enter password (min. 8 chars, a passphrase is recommended):",
style,
))],
false,
)?;
validate_password(&pw)?;

let rpw = draw_input_prompt(
terminal,
&[Line::from(Span::styled("Repeat password:", style))],
false,
)?;

if pw != rpw {
return Err(anyhow::anyhow!("Passwords do not match!"));
}
let pw_prompt = [Line::from(Span::styled(
"Enter password (min. 8 chars, a passphrase is recommended):",
style,
))];
let pw = loop {
let input = draw_input_prompt(terminal, &pw_prompt, false)?;
match validate_password(&input) {
Ok(()) => break input,
Err(e) => {
show_input_popup(terminal, &e.to_string())?;
}
}
};

let rpw_prompt = [Line::from(Span::styled("Repeat password:", style))];
let rpw = loop {
let input = draw_input_prompt(terminal, &rpw_prompt, false)?;
if pw == input {
break input;
}
show_input_popup(terminal, "Passwords do not match!")?;
};

cert_manager.generate_keypair(user_id, validity, cipher, pw, rpw)?;
Ok(())
Expand All @@ -136,14 +143,18 @@ pub fn export_certificate_tui(
draw_input_prompt(terminal, &[Line::from("Enter key secret")], false)?.into();
match secret.decrypt_secret(&password) {
Ok(_) => {
let file_name = draw_input_prompt(
terminal,
&[Line::from(
"Enter exported certificate name (e.g. name.certificate.pgp):",
)],
true,
)?;
validate_filename(&file_name)?;
let file_name_prompt = [Line::from(
"Enter exported certificate name (e.g. name.certificate.pgp):",
)];
let file_name = loop {
let input = draw_input_prompt(terminal, &file_name_prompt, true)?;
match validate_filename(&input) {
Ok(()) => break input,
Err(e) => {
show_input_popup(terminal, &e.to_string())?;
}
}
};

cert_manager.export_certificate(cert_path, &file_name)?;
}
Expand Down Expand Up @@ -171,27 +182,33 @@ pub fn edit_password_tui(
let original_pw = Password::from(original_pw_str);
match secret.decrypt_secret(&original_pw) {
Ok(_) => {
let new_password = draw_input_prompt(
terminal,
&[Line::from(
"Enter new password (min. 8 chars, a passphrase is recommended):",
)],
false,
let new_pw_prompt = [Line::from(
"Enter new password (min. 8 chars, a passphrase is recommended):",
)];
let new_password = loop {
let input = draw_input_prompt(terminal, &new_pw_prompt, false)?;
match validate_password(&input) {
Ok(()) => break input,
Err(e) => {
show_input_popup(terminal, &e.to_string())?;
}
}
};

let repeat_pw_prompt = [Line::from("Repeat new password")];
let repeat_password = loop {
let input = draw_input_prompt(terminal, &repeat_pw_prompt, false)?;
if new_password == input {
break input;
}
show_input_popup(terminal, "Passwords do not match!")?;
};
cert_manager.edit_password(
certificate,
&original_pw,
new_password,
repeat_password,
)?;
validate_password(&new_password)?;

let repeat_password =
draw_input_prompt(terminal, &[Line::from("Repeat new password")], false)?;
if new_password == repeat_password {
cert_manager.edit_password(
certificate,
&original_pw,
new_password,
repeat_password,
)?;
} else {
return Err(anyhow::anyhow!("Passwords do not match!"));
}
}
Err(_) => {
return Err(anyhow::anyhow!("Wrong password!"));
Expand Down Expand Up @@ -259,8 +276,16 @@ pub fn add_user_tui(
let original_pw = Password::from(original_pw_str);
match secret.decrypt_secret(&original_pw) {
Ok(_) => {
let new_user = draw_input_prompt(terminal, &[Line::from("Enter new user:")], true)?;
validate_not_empty(&new_user, "User ID")?;
let new_user_prompt = [Line::from("Enter new user:")];
let new_user = loop {
let input = draw_input_prompt(terminal, &new_user_prompt, true)?;
match validate_not_empty(&input, "User ID") {
Ok(()) => break input,
Err(e) => {
show_input_popup(terminal, &e.to_string())?;
}
}
};
cert_manager.add_user(cert_path, &original_pw, new_user)?;
}
Err(_) => {
Expand All @@ -285,14 +310,18 @@ pub fn split_users_tui(
let should_continue = show_user_selection_popup(terminal, &mut users, &mut selected_items)?;

if let Some(true) = should_continue {
let file_name = draw_input_prompt(
terminal,
&[Line::from(
"Enter exported certificate name (e.g. name.certificate.pgp):",
)],
true,
)?;
validate_filename(&file_name)?;
let file_name_prompt = [Line::from(
"Enter exported certificate name (e.g. name.certificate.pgp):",
)];
let file_name = loop {
let input = draw_input_prompt(terminal, &file_name_prompt, true)?;
match validate_filename(&input) {
Ok(()) => break input,
Err(e) => {
show_input_popup(terminal, &e.to_string())?;
}
}
};

let selected_users: Vec<String> = users
.items
Expand Down
77 changes: 74 additions & 3 deletions tests/certificate_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,19 @@ fn manager() -> CertificateManager {
CertificateManager
}

/// Mirror the sanitization logic from `certificate_manager::sanitize_for_path`.
fn sanitize_for_path(input: &str) -> String {
input.replace("..", "_").replace(['/', '\\'], "_")
}

/// Helper to compute the paths that `generate_keypair` writes to in ~/.pgpman/.
fn pgpman_paths(uid: &str) -> (String, String, String) {
let home = home::home_dir().unwrap();
let safe = sanitize_for_path(uid);
(
format!("{}/.pgpman/secrets/{}.pgp", home.display(), uid).replace(' ', ""),
format!("{}/.pgpman/revocation/{}.rev", home.display(), uid).replace(' ', ""),
format!("{}/.pgpman/certificates/{}.pgp", home.display(), uid).replace(' ', ""),
format!("{}/.pgpman/secrets/{}.pgp", home.display(), safe).replace(' ', ""),
format!("{}/.pgpman/revocation/{}.rev", home.display(), safe).replace(' ', ""),
format!("{}/.pgpman/certificates/{}.pgp", home.display(), safe).replace(' ', ""),
)
}

Expand Down Expand Up @@ -301,3 +307,68 @@ fn generate_keypair_mismatched_passwords_fails() {
);
assert!(result.is_err(), "mismatched passwords should fail");
}

/// A User ID containing `../` must not escape ~/.pgpman/ directories.
#[test]
fn generate_keypair_sanitizes_path_traversal_in_uid() {
init_directory().unwrap();

let malicious_uid = "../evil <evil@test.invalid>";
let (sk_expected, rev_expected, cert_expected) = pgpman_paths(malicious_uid);

manager()
.generate_keypair(
malicious_uid.into(),
"1y".into(),
"1".into(),
"safepass".into(),
"safepass".into(),
)
.unwrap();

// All files must land inside the expected ~/.pgpman/ subdirectories
let home = home::home_dir().unwrap();
let secrets_dir = format!("{}/.pgpman/secrets", home.display());
let revocation_dir = format!("{}/.pgpman/revocation", home.display());
let certificates_dir = format!("{}/.pgpman/certificates", home.display());

assert!(
sk_expected.starts_with(&secrets_dir),
"secret key must be inside secrets dir"
);
assert!(
rev_expected.starts_with(&revocation_dir),
"revocation cert must be inside revocation dir"
);
assert!(
cert_expected.starts_with(&certificates_dir),
"public cert must be inside certificates dir"
);

// Files must actually exist at the sanitized paths
assert!(Path::new(&sk_expected).exists(), "secret key should exist");
assert!(
Path::new(&rev_expected).exists(),
"revocation cert should exist"
);
assert!(
Path::new(&cert_expected).exists(),
"public cert should exist"
);

// The filename component must not contain ".."
let sk_filename = Path::new(&sk_expected)
.file_name()
.unwrap()
.to_str()
.unwrap();
assert!(
!sk_filename.contains(".."),
"filename must not contain '..'"
);

// Clean up
let _ = fs::remove_file(&sk_expected);
let _ = fs::remove_file(&rev_expected);
let _ = fs::remove_file(&cert_expected);
}