Implement --preserve-env-var=VAR (#1167)

squell · web-flow · commit 55846efd8116 · 2025-06-30T11:41:16.000+02:00
diff --git a/src/sudo/cli/mod.rs b/src/sudo/cli/mod.rs
@@ -355,7 +355,7 @@ pub struct SudoRunOptions {
     // -B
     pub bell: bool,
     // -E
-    pub preserve_env: PreserveEnv,
+    /* ignored, part of env_var_list */
     // -k
     pub reset_timestamp: bool,
     // -n
@@ -384,7 +384,6 @@ impl TryFrom<SudoOptions> for SudoRunOptions {
 
     fn try_from(mut opts: SudoOptions) -> Result<Self, Self::Error> {
         let bell = mem::take(&mut opts.bell);
-        let preserve_env = mem::take(&mut opts.preserve_env);
         let reset_timestamp = mem::take(&mut opts.reset_timestamp);
         let non_interactive = mem::take(&mut opts.non_interactive);
         let stdin = mem::take(&mut opts.stdin);
@@ -424,7 +423,6 @@ impl TryFrom<SudoOptions> for SudoRunOptions {
 
         Ok(Self {
             bell,
-            preserve_env,
             reset_timestamp,
             non_interactive,
             stdin,
@@ -455,7 +453,7 @@ struct SudoOptions {
     // -U
     other_user: Option<SudoString>,
     // -E
-    preserve_env: PreserveEnv,
+    /* ignored, part of env_var_list */
     // -s
     shell: bool,
     // -S
@@ -488,29 +486,6 @@ struct SudoOptions {
     positional_args: Vec<String>,
 }
 
-#[derive(Default, Debug, Clone, PartialEq)]
-pub enum PreserveEnv {
-    #[default]
-    Nothing,
-    Everything,
-    Only(Vec<String>),
-}
-
-impl PreserveEnv {
-    #[cfg(test)]
-    pub fn try_into_only(self) -> Result<Vec<String>, Self> {
-        if let Self::Only(v) = self {
-            Ok(v)
-        } else {
-            Err(self)
-        }
-    }
-
-    pub fn is_nothing(&self) -> bool {
-        matches!(self, Self::Nothing)
-    }
-}
-
 #[derive(Debug, Clone, PartialEq)]
 pub enum List {
     Once,
@@ -666,7 +641,9 @@ impl SudoOptions {
                         options.bell = true;
                     }
                     "-E" | "--preserve-env" => {
-                        options.preserve_env = PreserveEnv::Everything;
+                        eprintln_ignore_io_error!(
+                            "warning: preserving the entire environment is not supported, `{flag}` is ignored"
+                        )
                     }
                     "-e" | "--edit" => {
                         options.edit = true;
@@ -716,15 +693,13 @@ impl SudoOptions {
                         options.chdir = Some(SudoPath::from_cli_string(value));
                     }
                     "-E" | "--preserve-env" => {
-                        let split_value = || value.split(',').map(str::to_string);
-                        match &mut options.preserve_env {
-                            PreserveEnv::Nothing => {
-                                options.preserve_env = PreserveEnv::Only(split_value().collect())
-                            }
-                            PreserveEnv::Everything => {}
-                            PreserveEnv::Only(list) => list.extend(split_value()),
-                        }
-                        // options.preserve_env = value.split(',').map(str::to_string).collect()
+                        options
+                            .env_var_list
+                            .extend(value.split(',').filter_map(|var| {
+                                std::env::var(var)
+                                    .ok()
+                                    .map(|value| (var.to_string(), value))
+                            }));
                     }
                     "-g" | "--group" => {
                         options.group = Some(SudoString::from_cli_string(value));
@@ -787,12 +762,6 @@ impl<T> IsAbsent for Vec<T> {
     }
 }
 
-impl IsAbsent for PreserveEnv {
-    fn is_absent(&self) -> bool {
-        self.is_nothing()
-    }
-}
-
 fn ensure_is_absent(context: &str, thing: &dyn IsAbsent, name: &str) -> Result<(), String> {
     if thing.is_absent() {
         Ok(())
@@ -822,7 +791,6 @@ fn reject_all(context: &str, opts: SudoOptions) -> Result<(), String> {
         login,
         non_interactive,
         other_user,
-        preserve_env,
         shell,
         stdin,
         prompt,
@@ -848,7 +816,6 @@ fn reject_all(context: &str, opts: SudoOptions) -> Result<(), String> {
         tuple!(login),
         tuple!(non_interactive),
         tuple!(other_user),
-        tuple!(preserve_env),
         tuple!(remove_timestamp),
         tuple!(reset_timestamp),
         tuple!(shell),
diff --git a/src/sudo/cli/tests.rs b/src/sudo/cli/tests.rs
@@ -1,7 +1,5 @@
 use crate::common::SudoPath;
 
-use crate::sudo::cli::PreserveEnv;
-
 use super::{SudoAction, SudoOptions};
 use pretty_assertions::assert_eq;
 
@@ -19,71 +17,56 @@ fn short_preserve_env_with_var_fails() {
 /// Passing '--preserve-env' with an argument fills 'preserve_env', 'short_preserve_env' stays 'false'
 #[test]
 fn preserve_env_with_var() {
-    let cmd = SudoOptions::try_parse_from(["sudo", "--preserve-env=some_argument"]).unwrap();
+    let cmd = SudoOptions::try_parse_from(["sudo", "--preserve-env=HOME"]).unwrap();
     assert_eq!(
-        ["some_argument"],
-        cmd.preserve_env.try_into_only().unwrap().as_slice(),
+        [("HOME".to_string(), std::env::var("HOME").unwrap())],
+        cmd.env_var_list.as_slice()
     );
 }
 
 /// Passing '--preserve-env' with several arguments fills 'preserve_env', 'short_preserve_env' stays 'false'
 #[test]
 fn preserve_env_with_several_vars() {
-    let cmd = SudoOptions::try_parse_from([
-        "sudo",
-        "--preserve-env=some_argument,another_argument,a_third_one",
-    ])
-    .unwrap();
+    let cmd = SudoOptions::try_parse_from(["sudo", "--preserve-env=PATH,HOME"]).unwrap();
     assert_eq!(
-        ["some_argument", "another_argument", "a_third_one"],
-        cmd.preserve_env.try_into_only().unwrap().as_slice(),
+        [
+            ("PATH".to_string(), std::env::var("PATH").unwrap()),
+            ("HOME".to_string(), std::env::var("HOME").unwrap()),
+        ],
+        cmd.env_var_list.as_slice()
     );
 }
 
-#[test]
-fn preserve_env_boolean() {
-    let cmd = SudoOptions::try_parse_from(["sudo", "--preserve-env"]).unwrap();
-    assert_eq!(cmd.preserve_env, PreserveEnv::Everything);
-}
-
 #[test]
 fn preserve_env_boolean_and_list() {
-    let expected = PreserveEnv::Everything;
     let argss = [
-        ["sudo", "--preserve-env", "--preserve-env=some_argument"],
-        ["sudo", "--preserve-env=some_argument", "--preserve-env"],
+        ["sudo", "--preserve-env", "--preserve-env=HOME"],
+        ["sudo", "--preserve-env=HOME", "--preserve-env"],
     ];
 
     for args in argss {
         let cmd = SudoOptions::try_parse_from(args).unwrap();
-        assert_eq!(expected, cmd.preserve_env);
+        assert_eq!(
+            [("HOME".to_string(), std::env::var("HOME").unwrap())],
+            cmd.env_var_list.as_slice()
+        );
     }
 }
 
 #[test]
 fn preserve_env_repeated() {
-    let cmd = SudoOptions::try_parse_from([
-        "sudo",
-        "--preserve-env=some_argument",
-        "--preserve-env=another_argument",
-    ])
-    .unwrap();
+    let cmd = SudoOptions::try_parse_from(["sudo", "--preserve-env=PATH", "--preserve-env=HOME"])
+        .unwrap();
     assert_eq!(
-        ["some_argument", "another_argument"],
-        cmd.preserve_env.try_into_only().unwrap().as_slice()
+        ["PATH", "HOME"],
+        cmd.env_var_list
+            .into_iter()
+            .map(|x| x.0)
+            .collect::<Vec<_>>()
+            .as_slice()
     );
 }
 
-// `--preserve-env` only accepts a value with the syntax `--preserve-env=varname`
-// so this `--preserve-env` is acting like a boolean flag
-#[test]
-fn preserve_env_space() {
-    let cmd = SudoOptions::try_parse_from(["sudo", "--preserve-env", "true"]).unwrap();
-
-    assert_eq!(PreserveEnv::Everything, cmd.preserve_env);
-    assert_eq!(["true"], cmd.positional_args.as_slice());
-}
-
 /// Catch env variable that is given without hyphens in 'VAR=value' form in env_var_list.
 /// external_args stay empty.
 #[test]
diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs
@@ -59,12 +59,6 @@ pub fn run(mut cmd_opts: SudoRunOptions) -> Result<(), Error> {
 
     let user_requested_env_vars = std::mem::take(&mut cmd_opts.env_var_list);
 
-    if !cmd_opts.preserve_env.is_nothing() {
-        eprintln_ignore_io_error!(
-            "warning: `--preserve-env` has not yet been implemented and will be ignored"
-        )
-    }
-
     let mut context = Context::from_run_opts(cmd_opts, &mut policy)?;
 
     let policy = judge(policy, &context)?;
diff --git a/test-framework/sudo-compliance-tests/src/sudo.rs b/test-framework/sudo-compliance-tests/src/sudo.rs
@@ -9,6 +9,7 @@ mod flag_help;
 mod flag_list;
 mod flag_login;
 mod flag_non_interactive;
+mod flag_preserve_environment;
 mod flag_prompt;
 mod flag_shell;
 mod flag_user;
diff --git a/test-framework/sudo-compliance-tests/src/sudo/flag_preserve_environment.rs b/test-framework/sudo-compliance-tests/src/sudo/flag_preserve_environment.rs
@@ -0,0 +1,87 @@
+use sudo_test::{Command, Env};
+
+use crate::{helpers, SUDOERS_ALL_ALL_NOPASSWD};
+
+#[test]
+fn env_var_is_preserved() {
+    let name = "SHOULD_BE_PRESERVED";
+    let value = "42";
+    let env = Env([SUDOERS_ALL_ALL_NOPASSWD, "Defaults setenv"]).build();
+
+    let stdout = Command::new("env")
+        .arg(format!("{name}={value}"))
+        .args(["sudo", &format!("--preserve-env={name}"), "env"])
+        .output(&env)
+        .stdout();
+    let sudo_env = helpers::parse_env_output(&stdout);
+
+    assert_eq!(Some(value), sudo_env.get(name).copied());
+}
+
+#[test]
+fn preserve_and_env_var_can_coexist() {
+    let name = "SHOULD_BE_PRESERVED";
+    let value = "42";
+    let other_name = "ALSO_PRESERVED";
+    let other_value = "37";
+    let env = Env([SUDOERS_ALL_ALL_NOPASSWD, "Defaults setenv"]).build();
+
+    let stdout = Command::new("env")
+        .arg(format!("{name}={value}"))
+        .arg(format!("{other_name}={other_value}"))
+        .args([
+            "sudo",
+            &format!("{name}={value}"),
+            &format!("--preserve-env={other_name}"),
+            "env",
+        ])
+        .output(&env)
+        .stdout();
+    let sudo_env = helpers::parse_env_output(&stdout);
+
+    assert_eq!(Some(value), sudo_env.get(name).copied());
+    assert_eq!(Some(other_value), sudo_env.get(other_name).copied());
+}
+#[test]
+fn env_var_overrides_preserve() {
+    let name = "SHOULD_BE_PRESERVED";
+    let value = "42";
+    let other_value = "37";
+    let env = Env([SUDOERS_ALL_ALL_NOPASSWD, "Defaults setenv"]).build();
+
+    let stdout = Command::new("env")
+        .arg(format!("{name}={value}"))
+        .args([
+            "sudo",
+            &format!("--preserve-env={name}"),
+            &format!("{name}={other_value}"),
+            "env",
+        ])
+        .output(&env)
+        .stdout();
+    let sudo_env = helpers::parse_env_output(&stdout);
+
+    assert_eq!(Some(other_value), sudo_env.get(name).copied());
+}
+
+#[test]
+fn preserve_overrides_env_var() {
+    let name = "SHOULD_BE_PRESERVED";
+    let value = "42";
+    let other_value = "37";
+    let env = Env([SUDOERS_ALL_ALL_NOPASSWD, "Defaults setenv"]).build();
+
+    let stdout = Command::new("env")
+        .arg(format!("{name}={value}"))
+        .args([
+            "sudo",
+            &format!("{name}={other_value}"),
+            &format!("--preserve-env={name}"),
+            "env",
+        ])
+        .output(&env)
+        .stdout();
+    let sudo_env = helpers::parse_env_output(&stdout);
+
+    assert_eq!(Some(value), sudo_env.get(name).copied());
+}
