From c3e0679acc9bf09dc82b441d6835d7c50c1850c8 Mon Sep 17 00:00:00 2001
From: Pierre Warnier <pierre@warnier.net>
Date: Fri, 3 Apr 2026 13:57:28 +0200
Subject: [PATCH] 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
---
 src/shadow-core/src/shadow.rs |  9 ++++
 src/uu/usermod/src/usermod.rs | 79 +++++++++++++++++++----------------
 tests/by-util/test_usermod.rs |  2 +
 3 files changed, 54 insertions(+), 36 deletions(-)

diff --git a/src/shadow-core/src/shadow.rs b/src/shadow-core/src/shadow.rs
index 776021a..4db51b3 100644
--- a/src/shadow-core/src/shadow.rs
+++ b/src/shadow-core/src/shadow.rs
@@ -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() {
diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs
index 8fec016..a58bdc1 100644
--- a/src/uu/usermod/src/usermod.rs
+++ b/src/uu/usermod/src/usermod.rs
@@ -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.
 ///
diff --git a/tests/by-util/test_usermod.rs b/tests/by-util/test_usermod.rs
index c66d4a3..02b7cad 100644
--- a/tests/by-util/test_usermod.rs
+++ b/tests/by-util/test_usermod.rs
@@ -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");
 }