Repository: memorysafety/sudo-rs
Pull request #815 (2024-01-10 11:36:41)

Add path information to parse errors originating from included files.
Closing #806 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) Report
Attention: `31 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`2fa9234`)](https://app.codecov.io/gh/memorysafety/sudo-rs/commit/2fa92348d88c2b34c92a9d33845fd0533421d319?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) 57.36% compared to head [(`7fcc153`)](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) 57.27%.
> Report is 2 commits behind head on main.

| [Files](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) | Patch % | Lines |
|---|---|---|
| [src/sudoers/mod.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL3N1ZG9lcnMvbW9kLnJz) | 39.47% | [23 Missing :warning: ](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) |
| [src/sudo/mod.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL3N1ZG8vbW9kLnJz) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) |
| [src/visudo/mod.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL3Zpc3Vkby9tb2QucnM=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #815      +/-   ##
==========================================
- Coverage   57.36%   57.27%   -0.10%     
==========================================
  Files          74       74              
  Lines       10559    10583      +24     
==========================================
+ Hits         6057     6061       +4     
- Misses       4502     4522      +20     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/815?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety).


Patch:
diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs
index e1e686ae..288dfabd 100644
--- a/src/sudo/mod.rs
+++ b/src/sudo/mod.rs
@@ -46,8 +46,14 @@ impl PolicyPlugin for SudoersPolicy {
         let (sudoers, syntax_errors) = crate::sudoers::Sudoers::open(sudoers_path)
             .map_err(|e| Error::Configuration(format!("{e}")))?;
 
-        for crate::sudoers::Error(pos, error) in syntax_errors {
-            diagnostic::diagnostic!("{error}", sudoers_path @ pos);
+        for crate::sudoers::Error {
+            source,
+            location,
+            message,
+        } in syntax_errors
+        {
+            let path = source.as_deref().unwrap_or(sudoers_path);
+            diagnostic::diagnostic!("{message}", path @ location);
         }
 
         Ok(sudoers)
diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs
index 45ff5243..1f4093a4 100644
--- a/src/sudoers/mod.rs
+++ b/src/sudoers/mod.rs
@@ -25,7 +25,11 @@ const INCLUDE_LIMIT: u8 = 128;
 
 /// Export some necessary symbols from modules
 pub use ast::TextEnum;
-pub struct Error(pub Option<basic_parser::Position>, pub String);
+pub struct Error {
+    pub source: Option<PathBuf>,
+    pub location: Option<basic_parser::Position>,
+    pub message: String,
+}
 
 #[derive(Default)]
 pub struct Sudoers {
@@ -587,12 +591,19 @@ fn analyze(
     }
 
     impl Sudoers {
-        fn include(&mut self, path: &Path, diagnostics: &mut Vec<Error>, count: &mut u8) {
+        fn include(
+            &mut self,
+            parent: &Path,
+            path: &Path,
+            diagnostics: &mut Vec<Error>,
+            count: &mut u8,
+        ) {
             if *count >= INCLUDE_LIMIT {
-                diagnostics.push(Error(
-                    None,
-                    format!("include file limit reached opening '{}'", path.display()),
-                ))
+                diagnostics.push(Error {
+                    source: Some(parent.to_owned()),
+                    location: None,
+                    message: format!("include file limit reached opening '{}'", path.display()),
+                })
             // FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
             // that includes another non-privileged sudoer files.
             } else {
@@ -609,7 +620,11 @@ fn analyze(
                             e.to_string()
                         };
 
-                        diagnostics.push(Error(None, message))
+                        diagnostics.push(Error {
+                            source: Some(parent.to_owned()),
+                            location: None,
+                            message,
+                        })
                     }
                 }
             }
@@ -641,6 +656,7 @@ fn analyze(
                         }
 
                         Sudo::Include(path) => self.include(
+                            cur_path,
                             &resolve_relative(cur_path, path),
                             diagnostics,
                             safety_count,
@@ -648,18 +664,20 @@ fn analyze(
 
                         Sudo::IncludeDir(path) => {
                             if path.contains("%h") {
-                                diagnostics.push(Error(
-                                    None,
-                                    format!("cannot open sudoers file {path}: percent escape %h in includedir is unsupported")));
+                                diagnostics.push(Error{
+                                    source: Some(cur_path.to_owned()),
+                                    location: None,
+                                    message: format!("cannot open sudoers file {path}: percent escape %h in includedir is unsupported")});
                                 continue;
                             }
 
                             let path = resolve_relative(cur_path, path);
                             let Ok(files) = std::fs::read_dir(&path) else {
-                                diagnostics.push(Error(
-                                    None,
-                                    format!("cannot open sudoers file {}", path.display()),
-                                ));
+                                diagnostics.push(Error {
+                                    source: Some(cur_path.to_owned()),
+                                    location: None,
+                                    message: format!("cannot open sudoers file {}", path.display()),
+                                });
                                 continue;
                             };
                             let mut safe_files = files
@@ -675,14 +693,16 @@ fn analyze(
                                 .collect::<Vec<_>>();
                             safe_files.sort();
                             for file in safe_files {
-                                self.include(file.as_ref(), diagnostics, safety_count)
+                                self.include(cur_path, file.as_ref(), diagnostics, safety_count)
                             }
                         }
                     },
 
-                    Err(basic_parser::Status::Fatal(pos, error)) => {
-                        diagnostics.push(Error(Some(pos), error))
-                    }
+                    Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
+                        source: Some(cur_path.to_owned()),
+                        location: Some(pos),
+                        message,
+                    }),
                     Err(_) => panic!("internal parser error"),
                 }
             }
@@ -756,7 +776,11 @@ fn sanitize_alias_table<T>(table: &Vec<Def<T>>, diagnostics: &mut Vec<Error>) ->
 
     impl<T> Visitor<'_, T> {
         fn complain(&mut self, text: String) {
-            self.diagnostics.push(Error(None, text))
+            self.diagnostics.push(Error {
+                source: None,
+                location: None,
+                message: text,
+            })
         }
 
         fn visit(&mut self, pos: usize) {
diff --git a/src/visudo/mod.rs b/src/visudo/mod.rs
index 75e27202..ebe5169c 100644
--- a/src/visudo/mod.rs
+++ b/src/visudo/mod.rs
@@ -107,7 +107,7 @@ fn check(file_arg: Option<&str>, perms: bool, owner: bool) -> io::Result<()> {
     }
 
     let mut stderr = io::stderr();
-    for crate::sudoers::Error(_position, message) in errors {
+    for crate::sudoers::Error { message, .. } in errors {
         writeln!(stderr, "syntax error: {message}")?;
     }
 
@@ -245,7 +245,7 @@ fn edit_sudoers_file(
 
         writeln!(stderr, "The provided sudoers file format is not recognized or contains syntax errors. Please review:\n")?;
 
-        for crate::sudoers::Error(_position, message) in errors {
+        for crate::sudoers::Error { message, .. } in errors {
             writeln!(stderr, "syntax error: {message}")?;
         }
 


Test patch:
diff --git a/src/sudoers/test/mod.rs b/src/sudoers/test/mod.rs
index 21482cf8..7d0d0ca2 100644
--- a/src/sudoers/test/mod.rs
+++ b/src/sudoers/test/mod.rs
@@ -382,7 +382,7 @@ fn gh676_percent_h_escape_unsupported() {
     );
     assert_eq!(errs.len(), 1);
     assert_eq!(
-        errs[0].1,
+        errs[0].message,
         "cannot open sudoers file /etc/%h: percent escape %h in includedir is unsupported"
     )
 }