review: address all 4 Copilot review comments

- Error on missing shadow entry instead of silent no-op
- Validate password hash rejects ':', '\n', '\r'
- Centralize days_since_epoch() in shadow-core
- Assert field count and reserved field in test
This commit is contained in:
Pierre Warnier
2026-04-03 13:57:28 +02:00
parent 24347e9913
commit c3e0679acc
3 changed files with 54 additions and 36 deletions
+9
View File
@@ -106,6 +106,15 @@ impl ShadowEntry {
}
}
/// Current date as days since Unix epoch, for `last_change` updates.
pub fn days_since_epoch() -> i64 {
let secs = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.map(|d| i64::try_from(d.as_secs()).unwrap_or(i64::MAX))
.unwrap_or(0);
secs / 86400
}
/// Parse an optional numeric field — empty string becomes `None`.
fn parse_optional_field(field: &str) -> Result<Option<i64>, ShadowError> {
if field.is_empty() {
+43 -36
View File
@@ -171,6 +171,15 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let inactive = matches.get_one::<i64>(options::INACTIVE);
let new_password = matches.get_one::<String>(options::PASSWORD);
if let Some(pw) = new_password
&& pw.contains([':', '\n', '\r'])
{
return Err(UsermodError::CantUpdate(
"invalid password hash: must not contain ':', '\\n', or '\\r'".into(),
)
.into());
}
let login_changing = new_login.is_some();
if shadow_path.exists()
&& (do_lock
@@ -186,34 +195,40 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
let mut se = shadow::read_shadow_file(&shadow_path)
.map_err(|e| UsermodError::CantUpdate(format!("{e}")))?;
if let Some(s) = se.iter_mut().find(|e| e.name == *login) {
if do_lock {
s.lock();
}
if do_unlock {
s.unlock();
}
if let Some(exp) = expire {
s.expire_date = if exp == "-1" || exp.is_empty() {
None
} else {
Some(exp.parse::<i64>().map_err(|_| {
UsermodError::CantUpdate(format!(
"invalid expire date '{exp}' (expected days since epoch)"
))
})?)
};
}
if let Some(&i) = inactive {
s.inactive_days = if i < 0 { None } else { Some(i) };
}
if let Some(pw) = new_password {
s.passwd.clone_from(pw);
s.last_change = Some(days_since_epoch());
}
if let Some(new_name) = new_login {
s.name.clone_from(new_name);
}
let Some(s) = se.iter_mut().find(|e| e.name == *login) else {
drop(slock);
return Err(UsermodError::CantUpdate(format!(
"user '{login}' not found in shadow file"
))
.into());
};
if do_lock {
s.lock();
}
if do_unlock {
s.unlock();
}
if let Some(exp) = expire {
s.expire_date = if exp == "-1" || exp.is_empty() {
None
} else {
Some(exp.parse::<i64>().map_err(|_| {
UsermodError::CantUpdate(format!(
"invalid expire date '{exp}' (expected days since epoch)"
))
})?)
};
}
if let Some(&i) = inactive {
s.inactive_days = if i < 0 { None } else { Some(i) };
}
if let Some(pw) = new_password {
s.passwd.clone_from(pw);
s.last_change = Some(shadow::days_since_epoch());
}
if let Some(new_name) = new_login {
s.name.clone_from(new_name);
}
atomic::atomic_write(&shadow_path, |f| shadow::write_shadow(&se, f))
@@ -298,14 +313,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
Ok(())
}
fn days_since_epoch() -> i64 {
let now = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.map(|d| i64::try_from(d.as_secs()).unwrap_or(i64::MAX))
.unwrap_or(0);
now / 86400
}
/// Recursively chown all files and directories under `path` that are owned by
/// `old_uid` to `new_uid`. Files owned by other users are left untouched.
///
+2
View File
@@ -453,4 +453,6 @@ fn test_set_password_preserves_other_fields() {
assert_eq!(fields[5], "14", "warn_days should be preserved");
assert_eq!(fields[6], "30", "inactive_days should be preserved");
assert_eq!(fields[7], "20000", "expire_date should be preserved");
assert_eq!(fields.len(), 9, "shadow entry should have exactly 9 fields");
assert_eq!(fields[8], "", "reserved field should be empty");
}