Do not pass non-string env vars to externals (#4748)

* Do not pass non-string env vars to externals

Also misc cleanup

* Add note to default config

* Add a test

* Ensure PATH/Path conversion list <-> string

crates/nu-command/src/env/env_command.rs

@@ -1,7 +1,9 @@
 use nu_engine::env_to_string;
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Command, EngineState, Stack};
-use nu_protocol::{Category, Example, IntoPipelineData, PipelineData, Signature, Value};
+use nu_protocol::{
+    Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Value,
+};
 
 #[derive(Clone)]
 pub struct Env;
@@ -27,7 +29,6 @@ impl Command for Env {
         _input: PipelineData,
     ) -> Result<nu_protocol::PipelineData, nu_protocol::ShellError> {
         let span = call.head;
-        let config = stack.get_config().unwrap_or_default();
 
         let mut env_vars: Vec<(String, Value)> =
             stack.get_env_vars(engine_state).into_iter().collect();
@@ -39,7 +40,12 @@ impl Command for Env {
             let mut cols = vec![];
             let mut vals = vec![];
 
-            let raw = env_to_string(&name, val.clone(), engine_state, stack, &config)?;
+            let raw_val = match env_to_string(&name, &val, engine_state, stack) {
+                Ok(raw) => Value::string(raw, span),
+                Err(ShellError::EnvVarNotAString(..)) => Value::nothing(span),
+                Err(e) => return Err(e),
+            };
+
             let val_type = val.get_type();
 
             cols.push("name".into());
@@ -52,7 +58,7 @@ impl Command for Env {
             vals.push(val);
 
             cols.push("raw".into());
-            vals.push(Value::string(raw, span));
+            vals.push(raw_val);
 
             values.push(Value::Record { cols, vals, span });
         }

crates/nu-command/src/system/exec.rs

@@ -76,8 +76,7 @@ fn exec(
     let args: Vec<Spanned<String>> = call.rest(engine_state, stack, 1)?;
 
     let cwd = current_dir(engine_state, stack)?;
-    let config = stack.get_config()?;
+    let env_vars = env_to_strings(engine_state, stack)?;
-    let env_vars = env_to_strings(engine_state, stack, &config)?;
     let current_dir = current_dir(engine_state, stack)?;
 
     let external_command = ExternalCommand {

crates/nu-command/src/system/run_external.rs

@@ -55,8 +55,7 @@ impl Command for External {
         let redirect_stderr = call.has_flag("redirect-stderr");
 
         // Translate environment variables from Values to Strings
-        let config = stack.get_config().unwrap_or_default();
+        let env_vars_str = env_to_strings(engine_state, stack)?;
-        let env_vars_str = env_to_strings(engine_state, stack, &config)?;
 
         fn value_as_spanned(value: Value) -> Result<Spanned<String>, ShellError> {
             let span = value.span()?;

crates/nu-command/src/viewers/griddle.rs

@@ -66,7 +66,7 @@ prints out the list properly."#
         let separator_param: Option<String> = call.get_flag(engine_state, stack, "separator")?;
         let config = stack.get_config().unwrap_or_default();
         let env_str = match stack.get_env_var(engine_state, "LS_COLORS") {
-            Some(v) => Some(env_to_string("LS_COLORS", v, engine_state, stack, &config)?),
+            Some(v) => Some(env_to_string("LS_COLORS", &v, engine_state, stack)?),
             None => None,
         };
         let use_grid_icons = config.use_grid_icons;

crates/nu-engine/src/env.rs

@@ -3,15 +3,10 @@ use std::path::{Path, PathBuf};
 
 use nu_protocol::ast::PathMember;
 use nu_protocol::engine::{EngineState, Stack};
-use nu_protocol::{Config, PipelineData, ShellError, Span, Value};
+use nu_protocol::{PipelineData, ShellError, Span, Value};
 
 use crate::eval_block;
 
-#[cfg(windows)]
-const ENV_SEP: &str = ";";
-#[cfg(not(windows))]
-const ENV_SEP: &str = ":";
-
 #[cfg(windows)]
 const ENV_PATH_NAME: &str = "Path";
 #[cfg(windows)]
@@ -52,6 +47,23 @@ pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Opti
         }
     }
 
+    #[cfg(not(windows))]
+    {
+        error = error.or_else(|| ensure_path(&mut new_scope, ENV_PATH_NAME));
+    }
+
+    #[cfg(windows)]
+    {
+        let first_result = ensure_path(&mut new_scope, ENV_PATH_NAME);
+        if first_result.is_some() {
+            let second_result = ensure_path(&mut new_scope, ENV_PATH_NAME_SECONDARY);
+
+            if second_result.is_some() {
+                error = error.or(first_result);
+            }
+        }
+    }
+
     for (k, v) in new_scope {
         engine_state.env_vars.insert(k, v);
     }
@@ -60,18 +72,51 @@ pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Opti
 }
 
 /// Translate one environment variable from Value to String
+///
+/// Returns Ok(None) if the env var is not
 pub fn env_to_string(
     env_name: &str,
-    value: Value,
+    value: &Value,
     engine_state: &EngineState,
     stack: &Stack,
-    config: &Config,
 ) -> Result<String, ShellError> {
-    match get_converted_value(engine_state, stack, env_name, &value, "to_string") {
+    match get_converted_value(engine_state, stack, env_name, value, "to_string") {
         ConversionResult::Ok(v) => Ok(v.as_string()?),
         ConversionResult::ConversionError(e) => Err(e),
         ConversionResult::GeneralError(e) => Err(e),
-        ConversionResult::CellPathError => Ok(value.into_string(ENV_SEP, config)),
+        ConversionResult::CellPathError => match value.as_string() {
+            Ok(s) => Ok(s),
+            Err(_) => {
+                if env_name == ENV_PATH_NAME {
+                    // Try to convert PATH/Path list to a string
+                    match value {
+                        Value::List { vals, .. } => {
+                            let paths = vals
+                                .iter()
+                                .map(|v| v.as_string())
+                                .collect::<Result<Vec<_>, _>>()?;
+
+                            match std::env::join_paths(paths) {
+                                Ok(p) => Ok(p.to_string_lossy().to_string()),
+                                Err(_) => Err(ShellError::EnvVarNotAString(
+                                    env_name.to_string(),
+                                    value.span()?,
+                                )),
+                            }
+                        }
+                        _ => Err(ShellError::EnvVarNotAString(
+                            env_name.to_string(),
+                            value.span()?,
+                        )),
+                    }
+                } else {
+                    Err(ShellError::EnvVarNotAString(
+                        env_name.to_string(),
+                        value.span()?,
+                    ))
+                }
+            }
+        },
     }
 }
 
@@ -79,13 +124,17 @@ pub fn env_to_string(
 pub fn env_to_strings(
     engine_state: &EngineState,
     stack: &Stack,
-    config: &Config,
 ) -> Result<HashMap<String, String>, ShellError> {
     let env_vars = stack.get_env_vars(engine_state);
     let mut env_vars_str = HashMap::new();
     for (env_name, val) in env_vars {
-        let val_str = env_to_string(&env_name, val, engine_state, stack, config)?;
+        match env_to_string(&env_name, &val, engine_state, stack) {
-        env_vars_str.insert(env_name, val_str);
+            Ok(val_str) => {
+                env_vars_str.insert(env_name, val_str);
+            }
+            Err(ShellError::EnvVarNotAString(..)) => {} // ignore non-string values
+            Err(e) => return Err(e),
+        }
     }
 
     Ok(env_vars_str)
@@ -93,16 +142,16 @@ pub fn env_to_strings(
 
 /// Shorthand for env_to_string() for PWD with custom error
 pub fn current_dir_str(engine_state: &EngineState, stack: &Stack) -> Result<String, ShellError> {
-    let config = stack.get_config()?;
     if let Some(pwd) = stack.get_env_var(engine_state, "PWD") {
-        match env_to_string("PWD", pwd, engine_state, stack, &config) {
+        match env_to_string("PWD", &pwd, engine_state, stack) {
             Ok(cwd) => {
                 if Path::new(&cwd).is_absolute() {
                     Ok(cwd)
                 } else {
-                    Err(ShellError::LabeledError(
+                    Err(ShellError::SpannedLabeledError(
                             "Invalid current directory".to_string(),
-                            format!("The 'PWD' environment variable must be set to an absolute path. Found: '{}'", cwd)
+                            format!("The 'PWD' environment variable must be set to an absolute path. Found: '{}'", cwd),
+                            pwd.span()?
                     ))
                 }
             }
@@ -149,21 +198,7 @@ pub fn path_str(
         }
     }?;
 
-    if let Value::String { val, .. } = pathval {
+    env_to_string(pathname, &pathval, engine_state, stack)
-        Ok(val)
-    } else {
-        match get_converted_value(engine_state, stack, pathname, &pathval, "to_string") {
-        ConversionResult::Ok(v) => Ok(v.as_string()?),
-        ConversionResult::ConversionError(e) => Err(e),
-        ConversionResult::GeneralError(e) => Err(e),
-        ConversionResult::CellPathError => Err(ShellError::SpannedLabeledErrorHelp(
-            format!("Missing environment conversion of {} to string", pathname),
-            "Could not convert to string".to_string(),
-            pathval.span()?,
-            "The 'to_string' field of ENV_CONVERSIONS environment variable must be set up correctly".to_string()
-        ))
-        }
-    }
 }
 
 fn get_converted_value(
@@ -226,7 +261,6 @@ fn get_converted_value(
                     Err(e) => ConversionResult::ConversionError(e),
                 }
             } else {
-                // This one is OK to fail: We want to know if custom conversion is working
                 ConversionResult::ConversionError(ShellError::MissingParameter(
                     "block input".into(),
                     from_span,
@@ -239,3 +273,56 @@ fn get_converted_value(
         ConversionResult::CellPathError
     }
 }
+
+fn ensure_path(scope: &mut HashMap<String, Value>, env_path_name: &str) -> Option<ShellError> {
+    let mut error = None;
+
+    // If PATH/Path is still a string, force-convert it to a list
+    match scope.get(env_path_name) {
+        Some(Value::String { val, span }) => {
+            // Force-split path into a list
+            let span = *span;
+            let paths = std::env::split_paths(val)
+                .map(|p| Value::String {
+                    val: p.to_string_lossy().to_string(),
+                    span,
+                })
+                .collect();
+
+            scope.insert(env_path_name.to_string(), Value::List { vals: paths, span });
+        }
+        Some(Value::List { vals, span }) => {
+            // Must be a list of strings
+            if !vals.iter().all(|v| matches!(v, Value::String { .. })) {
+                error = error.or_else(|| {
+                    Some(ShellError::SpannedLabeledError(
+                        format!("Wrong {} environment variable value", env_path_name),
+                        format!("{} must be a list of strings", env_path_name),
+                        *span,
+                    ))
+                });
+            }
+        }
+        Some(val) => {
+            // All other values are errors
+            let span = match val.span() {
+                Ok(sp) => sp,
+                Err(e) => {
+                    error = error.or(Some(e));
+                    Span::test_data() // FIXME: any better span to use here?
+                }
+            };
+
+            error = error.or_else(|| {
+                Some(ShellError::SpannedLabeledError(
+                    format!("Wrong {} environment variable value", env_path_name),
+                    format!("{} must be a list of strings", env_path_name),
+                    span,
+                ))
+            });
+        }
+        None => { /* not preset, do nothing */ }
+    }
+
+    error
+}

crates/nu-protocol/src/shell_error.rs

@@ -112,6 +112,17 @@ pub enum ShellError {
     #[diagnostic(code(nu::shell::cant_convert), url(docsrs))]
     CantConvert(String, String, #[label("can't convert {1} to {0}")] Span),
 
+    #[error("{0} is not representable as a string.")]
+    #[diagnostic(
+        code(nu::shell::env_var_not_a_string),
+        url(docsrs),
+        help(
+            r#"The '{0}' environment variable must be a string or be convertible to a string.
+Either make sure {0} is a string, or add a 'to_string' entry for it in ENV_CONVERSIONS."#
+        )
+    )]
+    EnvVarNotAString(String, #[label("value not representable as a string")] Span),
+
     #[error("Division by zero.")]
     #[diagnostic(code(nu::shell::division_by_zero), url(docsrs))]
     DivisionByZero(#[label("division by zero")] Span),

docs/sample_config/default_config.nu

@@ -28,6 +28,7 @@ let-env PROMPT_MULTILINE_INDICATOR = "::: "
 # Specifies how environment variables are:
 # - converted from a string to a value on Nushell startup (from_string)
 # - converted from a value back to a string when running extrnal commands (to_string)
+# Note: The conversions happen *after* config.nu is loaded
 let-env ENV_CONVERSIONS = {
   "PATH": {
     from_string: { |s| $s | split row (char esep) }

src/tests/test_env.rs

@@ -14,3 +14,11 @@ fn shorthand_env_2() -> TestResult {
 fn shorthand_env_3() -> TestResult {
     run_test(r#"FOO=BAZ BAR=MOO $env.FOO"#, "BAZ")
 }
+
+#[test]
+fn convert_non_string_env_var_to_nothing() -> TestResult {
+    run_test(
+        r#"let-env FOO = true; env | where name == FOO | get raw.0 | describe"#,
+        "nothing",
+    )
+}