mode: reject empty clauses and bare numeric modes in a list

src/uu/chmod/src/chmod.rs

@@ -325,11 +325,15 @@ impl Chmoder {
             // Where the clause being parsed starts inside `cmode_unwrapped`, so
             // that an error can be pointed back at it.
             let mut offset = 0;
+            let clauses = cmode_unwrapped.split(',').count();
             for mode in cmode_unwrapped.split(',') {
                 let clause_start = offset;
                 offset += mode.len() + 1; // past the clause and its comma
 
-                let result = if mode.chars().any(|c| c.is_ascii_digit()) {
+                let result = if let Err(error) = mode::check_clause(&cmode_unwrapped, mode, clauses)
+                {
+                    Err(error)
+                } else if mode.chars().any(|c| c.is_ascii_digit()) {
                     mode::parse_numeric(new_mode, mode, is_dir).map(|v| (v, v))
                 } else {
                     mode::parse_symbolic(new_mode, mode, mode::get_umask(), is_dir).map(|m| {

src/uucore/src/lib/features/mode.rs

@@ -359,6 +359,29 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) {
     (srwx, pos)
 }
 
+/// Check one comma-separated `clause` of `mode`, which has `clauses` clauses.
+///
+/// A clause cannot be empty (`u+x,`, `,u+x`, `u+x,,g+w`), and a numeric mode
+/// without an operator (`644`) must be the whole mode, so `644,u+x` and `1,2`
+/// are invalid. The error's span is relative to the clause.
+pub fn check_clause(mode: &str, clause: &str, clauses: usize) -> Result<(), ModeError> {
+    if clause.is_empty() {
+        return Err(ModeError::new(
+            format!("invalid mode ({mode})"),
+            0..0,
+            ModeErrorKind::MissingOperator,
+        ));
+    }
+    if clauses > 1 && clause.starts_with(|c: char| c.is_ascii_digit()) {
+        return Err(ModeError::new(
+            format!("invalid mode ({mode})"),
+            0..clause.len(),
+            ModeErrorKind::InvalidNumber,
+        ));
+    }
+    Ok(())
+}
+
 /// Modify a file mode based on a user-supplied string.
 /// Supports comma-separated mode strings like "ug+rwX,o+rX" (same as chmod).
 pub fn parse_chmod(
@@ -371,15 +394,14 @@ pub fn parse_chmod(
 
     // Split by commas and process each mode part sequentially
     let mut offset = 0;
+    let clauses = mode_string.split(',').count();
     for raw_part in mode_string.split(',') {
         let start = offset + (raw_part.len() - raw_part.trim_start().len());
         // Past the part and the comma that followed it.
         offset += raw_part.len() + 1;
 
         let mode_part = raw_part.trim();
-        if mode_part.is_empty() {
-            continue;
-        }
+        check_clause(mode_string, mode_part, clauses).map_err(|err| err.shift(start))?;
 
         new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) {
             parse_numeric(new_mode, mode_part, considering_dir)
@@ -468,7 +490,7 @@ mod tests {
         // Numeric mode with - operator (starting from 0, so nothing to remove)
         assert_eq!(parse("-4", false, 0).unwrap(), 0);
         // But if we first set a mode, then remove bits
-        assert_eq!(parse("644,-4", false, 0).unwrap(), 0o640);
+        assert_eq!(parse("=644,-4", false, 0).unwrap(), 0o640);
     }
 
     #[test]
@@ -505,17 +527,31 @@ mod tests {
 
     #[test]
     fn test_parse_mixed_numeric_and_symbolic() {
-        // Mix of numeric and symbolic modes
-        assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744);
-        assert_eq!(parse("u+rw,755", false, 0).unwrap(), 0o755);
+        // Mix of numeric and symbolic modes: in a list, a numeric mode needs
+        // an operator
+        assert_eq!(parse("=644,u+x", false, 0).unwrap(), 0o744);
+        assert_eq!(parse("u+rw,=755", false, 0).unwrap(), 0o755);
+        assert_eq!(parse("u+x,+644", false, 0).unwrap(), 0o744);
+
+        // A numeric mode without an operator must be the whole mode, as in GNU
+        assert!(parse("644,u+x", false, 0).is_err());
+        assert!(parse("u+rw,755", false, 0).is_err());
+        assert!(parse("1,2", false, 0).is_err());
     }
 
     #[test]
-    fn test_parse_empty_string() {
-        // Empty string should return 0
-        assert_eq!(parse("", false, 0).unwrap(), 0);
-        assert_eq!(parse("   ", false, 0).unwrap(), 0);
-        assert_eq!(parse(",,", false, 0).unwrap(), 0);
+    fn test_parse_empty_clauses() {
+        // An empty mode or an empty clause is invalid, as in GNU
+        assert!(parse("", false, 0).is_err());
+        assert!(parse("   ", false, 0).is_err());
+        assert!(parse(",,", false, 0).is_err());
+        assert!(parse("u+x,", false, 0).is_err());
+        assert!(parse(",u+x", false, 0).is_err());
+        assert!(parse("u+r,,g+w", false, 0).is_err());
+
+        // `=` and a lone operator are clauses, not empty ones
+        assert_eq!(parse("=,u+x", false, 0).unwrap(), 0o100);
+        assert_eq!(parse("+,-", false, 0).unwrap(), 0);
     }
 
     #[test]
@@ -555,17 +591,17 @@ mod tests {
         // Complex real-world examples
         assert_eq!(parse("u=rwx,g=rx,o=r", false, 0).unwrap(), 0o754);
         // To test removal, we need to first set permissions, then remove them
-        assert_eq!(parse("644,a-w", false, 0).unwrap(), 0o444);
-        assert_eq!(parse("644,g-r", false, 0).unwrap(), 0o604);
+        assert_eq!(parse("=644,a-w", false, 0).unwrap(), 0o444);
+        assert_eq!(parse("=644,g-r", false, 0).unwrap(), 0o604);
     }
 
     #[test]
     fn test_parse_sequential_application() {
         // Test that comma-separated modes are applied sequentially
         // First set to 644, then add execute for user
-        assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744);
+        assert_eq!(parse("=644,u+x", false, 0).unwrap(), 0o744);
 
         // First add user write, then set to 755 (should override)
-        assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755);
+        assert_eq!(parse("u+w,=755", false, 0).unwrap(), 0o755);
     }
 }

tests/by-util/test_chmod.rs

@@ -1865,3 +1865,27 @@ mod diagnostics {
             .no_output();
     }
 }
+
+#[test]
+fn test_chmod_rejects_empty_and_bare_numeric_clauses() {
+    // Rejected by GNU too: an empty clause, and a numeric mode without an
+    // operator in a list of clauses
+    for mode in ["u+x,", ",u+x", "u+r,,g+w", ",", "644,u+x", "u+x,644", "1,2"] {
+        let (at, mut ucmd) = at_and_ucmd!();
+        at.touch("file");
+        set_permissions(at.plus("file"), Permissions::from_mode(0o644)).unwrap();
+        ucmd.args(&[mode, "file"])
+            .fails_with_code(1)
+            .stderr_only(format!("chmod: invalid mode ({mode})\n"));
+        assert_eq!(at.metadata("file").mode() & 0o7777, 0o644, "{mode}");
+    }
+}
+
+#[test]
+fn test_chmod_numeric_mode_with_operator_in_a_list() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.touch("file");
+    set_permissions(at.plus("file"), Permissions::from_mode(0o600)).unwrap();
+    ucmd.args(&["u+x,=644,g+w", "file"]).succeeds().no_output();
+    assert_eq!(at.metadata("file").mode() & 0o7777, 0o664);
+}

tests/by-util/test_mkdir.rs

@@ -1263,3 +1263,16 @@ fn test_mkdir_concurrent_non_recursive() {
         );
     }
 }
+
+#[test]
+fn test_mkdir_mode_rejects_empty_and_bare_numeric_clauses() {
+    // Rejected by GNU too: an empty clause, and a numeric mode without an
+    // operator in a list of clauses
+    for mode in ["u+x,", ",u+x", "u+r,,g+w", ",", "644,u+x", "u+x,644", "1,2"] {
+        let (at, mut ucmd) = at_and_ucmd!();
+        ucmd.args(&["-m", mode, "dir"])
+            .fails_with_code(1)
+            .stderr_only(format!("mkdir: invalid mode '{mode}'\n"));
+        assert!(!at.dir_exists("dir"), "{mode}");
+    }
+}