diff --git a/crates/nu-cli/src/cli.rs b/crates/nu-cli/src/cli.rs index 14b423c454..f9bdd85f48 100644 --- a/crates/nu-cli/src/cli.rs +++ b/crates/nu-cli/src/cli.rs @@ -237,7 +237,7 @@ pub fn create_default_context( syncer.load_environment(); let mut context = Context::basic()?; - syncer.sync_env_vars(&mut context); + syncer.sync_env_vars(&mut context)?; syncer.sync_path_vars(&mut context); { @@ -671,7 +671,7 @@ pub async fn cli( // TODO: make sure config is cached so we don't path this load every call // FIXME: we probably want to be a bit more graceful if we can't set the environment syncer.reload(); - syncer.sync_env_vars(&mut context); + syncer.sync_env_vars(&mut context)?; syncer.sync_path_vars(&mut context); match line { diff --git a/crates/nu-cli/src/env/TODO.org b/crates/nu-cli/src/env/TODO.org new file mode 100644 index 0000000000..bc0a9f9e0f --- /dev/null +++ b/crates/nu-cli/src/env/TODO.org @@ -0,0 +1,26 @@ +* TODO +** Run even if =env()= returns None +It happens if there is no env-section in .config/nu/config.toml. +This results in +#+begin_src rust +impl Env for Environment { + fn env(&self) -> Option { + if let Some(vars) = &self.environment_vars { + return Some(vars.clone()); + } + + None + } +#+end_src +returning =None=, which completely skips running the code for dealing with directory specific environment variables. +** Confirm intended behavior + - If you are in a directory, or a subdirectory to a directory with a .nu-file, the vars in that .nu-file are applied. + - If you leave a directory which set some variables, the variables are unset. + - If a directory contains a .nu with an environment variable that was already set, the old value will be overwritten. + - This holds even if the old value was set by a .nu in a parent directory. The overwritten value is restored when you leave the directory. +** Security + https://github.com/nushell/nushell/issues/1965 +** Nice errors +** Potential issues + - Currently the functionality to restore environment variables is disabled, as it is handled by other parts of nushell just fine. + - This functionality might need to be re-added if \ No newline at end of file diff --git a/crates/nu-cli/src/env/directory_specific_environment.rs b/crates/nu-cli/src/env/directory_specific_environment.rs index 27d7509cfe..870bd079ca 100644 --- a/crates/nu-cli/src/env/directory_specific_environment.rs +++ b/crates/nu-cli/src/env/directory_specific_environment.rs @@ -1,22 +1,28 @@ -use indexmap::IndexMap; +use indexmap::{IndexMap, IndexSet}; use nu_protocol::{Primitive, UntaggedValue, Value}; -use std::io::{Error, ErrorKind, Result}; -use std::{ffi::OsString, fmt::Debug, path::PathBuf}; +use std::{ + ffi::OsString, + fmt::Debug, + io::{Error, ErrorKind, Result}, + path::PathBuf, +}; +type EnvKey = String; +type EnvVal = OsString; #[derive(Debug, Default)] pub struct DirectorySpecificEnvironment { - allowed_directories: Vec, + allowed_directories: IndexSet, //Directory -> Env key. If an environment var has been added from a .nu in a directory, we track it here so we can remove it when the user leaves the directory. - added_env_vars: IndexMap>, + added_env_vars: IndexMap>, //Directory -> (env_key, value). If a .nu overwrites some existing environment variables, they are added here so that they can be restored later. - overwritten_env_values: IndexMap>, + overwritten_env_vars: IndexMap>, } impl DirectorySpecificEnvironment { pub fn new(allowed_directories: Option) -> DirectorySpecificEnvironment { - let mut allowed_directories = if let Some(Value { + let allowed_directories = if let Some(Value { value: UntaggedValue::Table(ref wrapped_directories), tag: _, }) = allowed_directories @@ -35,157 +41,122 @@ impl DirectorySpecificEnvironment { }) .collect() } else { - vec![] + IndexSet::new() }; - allowed_directories.sort(); DirectorySpecificEnvironment { allowed_directories, added_env_vars: IndexMap::new(), - overwritten_env_values: IndexMap::new(), + overwritten_env_vars: IndexMap::new(), } } //If we are no longer in a directory, we restore the values it overwrote. - pub fn overwritten_values_to_restore(&mut self) -> Result> { + pub fn overwritten_values_to_restore(&mut self) -> Result> { let current_dir = std::env::current_dir()?; + let mut working_dir = Some(current_dir.as_path()); + + let mut new_overwritten_env_values = IndexMap::new(); + while let Some(wdir) = working_dir { + if let Some(val) = self.overwritten_env_vars.get(wdir) { + new_overwritten_env_values.insert(wdir.to_path_buf(), val.clone()); + } + working_dir = working_dir + .expect("This should not be None because of the while condition") + .parent(); + } let mut keyvals_to_restore = IndexMap::new(); - let mut new_overwritten = IndexMap::new(); - - for (directory, keyvals) in &self.overwritten_env_values { - let mut working_dir = Some(current_dir.as_path()); - - let mut re_add_keyvals = true; - while let Some(wdir) = working_dir { - if wdir == directory.as_path() { - re_add_keyvals = false; - new_overwritten.insert(directory.clone(), keyvals.clone()); - break; - } else { - working_dir = working_dir //Keep going up in the directory structure with .parent() - .ok_or_else(|| { - Error::new(ErrorKind::NotFound, "Root directory has no parent") - })? - .parent(); - } - } - if re_add_keyvals { + for (dir, keyvals) in &self.overwritten_env_vars { + if !new_overwritten_env_values.contains_key(dir) { for (k, v) in keyvals { - keyvals_to_restore.insert( - k.clone(), - v.to_str() - .ok_or_else(|| { - Error::new( - ErrorKind::Other, - format!("{:?} is not valid unicode", v), - ) - })? - .to_string(), - ); + keyvals_to_restore.insert(k.clone(), v.clone()); } } } - self.overwritten_env_values = new_overwritten; + self.overwritten_env_vars = new_overwritten_env_values; Ok(keyvals_to_restore) } - pub fn env_vars_to_add(&mut self) -> Result> { + pub fn env_vars_to_add(&mut self) -> std::io::Result> { let current_dir = std::env::current_dir()?; - + let mut working_dir = Some(current_dir.as_path()); let mut vars_to_add = IndexMap::new(); - for dir in &self.allowed_directories { - let mut working_dir = Some(current_dir.as_path()); - //Start in the current directory, then traverse towards the root with working_dir to see if we are in a subdirectory of a valid directory. - while let Some(wdir) = working_dir { - if wdir == dir.as_path() { - let toml_doc = std::fs::read_to_string(wdir.join(".nu-env").as_path())? - .parse::()?; + //Start in the current directory, then traverse towards the root with working_dir to see if we are in a subdirectory of a valid directory. + while let Some(wdir) = working_dir { + if self.allowed_directories.contains(wdir) { + let toml_doc = std::fs::read_to_string(wdir.join(".nu-env").as_path())? + .parse::()?; - let vars_in_current_file = toml_doc - .get("env") - .ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - "No [env] section in .nu-env", - ) - })? - .as_table() - .ok_or_else(|| { - Error::new(ErrorKind::InvalidData, "Malformed [env] section in .nu-env") - })?; + toml_doc + .get("env") + .ok_or_else(|| Error::new(ErrorKind::InvalidData, "env section missing"))? + .as_table() + .ok_or_else(|| Error::new(ErrorKind::InvalidData, "env section malformed"))? + .iter() + .for_each(|(dir_env_key, dir_env_val)| { + let dir_env_val: EnvVal = dir_env_val.as_str().unwrap().into(); - let mut keys_in_current_nufile = vec![]; - for (k, v) in vars_in_current_file { - vars_to_add.insert( - k.clone(), - v.as_str() - .ok_or_else(|| { - Error::new( - ErrorKind::InvalidData, - format!("Could not read environment variable: {}", v), - ) - })? - .to_string(), - ); //This is used to add variables to the environment - keys_in_current_nufile.push(k.clone()); //this is used to keep track of which directory added which variables - } + //If we are about to overwrite any environment variables, we save them first so they can be restored later. + if let Some(existing_val) = std::env::var_os(dir_env_key) { + //This condition is to make sure variables in parent directories don't overwrite variables set by subdirectories. + if !vars_to_add.contains_key(dir_env_key) { + self.overwritten_env_vars + .entry(wdir.to_path_buf()) + .or_insert(IndexMap::new()) + .insert(dir_env_key.clone(), existing_val); - //If we are about to overwrite any environment variables, we save them first so they can be restored later. - self.overwritten_env_values.insert( - wdir.to_path_buf(), - keys_in_current_nufile - .iter() - .filter_map(|key| { - if let Some(val) = std::env::var_os(key) { - return Some((key.clone(), val)); - } - None - }) - .collect(), - ); - - self.added_env_vars - .insert(wdir.to_path_buf(), keys_in_current_nufile); - break; - } else { - working_dir = working_dir //Keep going up in the directory structure with .parent() - .ok_or_else(|| { - Error::new(ErrorKind::NotFound, "Root directory has no parent") - })? - .parent(); - } + vars_to_add.insert(dir_env_key.clone(), dir_env_val); + } + } else { + //Otherwise, we just track that we added it here + self.added_env_vars + .entry(wdir.to_path_buf()) + .or_insert(IndexSet::new()) + .insert(dir_env_key.clone()); + vars_to_add.insert(dir_env_key.clone(), dir_env_val); + } + }); } + + working_dir = working_dir //Keep going up in the directory structure with .parent() + .expect("This should not be None because of the while condition") + .parent(); } Ok(vars_to_add) } //If the user has left directories which added env vars through .nu, we clear those vars - pub fn env_vars_to_delete(&mut self) -> Result> { + //once they are marked for deletion, remove them from added_env_vars + pub fn env_vars_to_delete(&mut self) -> Result> { let current_dir = std::env::current_dir()?; + let mut working_dir = Some(current_dir.as_path()); - //Gather up all environment variables that should be deleted. - //If we are not in a directory or one of its subdirectories, mark the env_vals it maps to for removal. - let vars_to_delete = self.added_env_vars.iter().fold( - Vec::new(), - |mut vars_to_delete, (directory, env_vars)| { - let mut working_dir = Some(current_dir.as_path()); + //We start from the current directory and go towards the root. We retain the variables set by directories we are in. + let mut new_added_env_vars = IndexMap::new(); + while let Some(wdir) = working_dir { + if let Some(vars_added_by_this_directory) = self.added_env_vars.get(wdir) { + //If we are still in a directory, we should continue to track the vars it added. + new_added_env_vars.insert(wdir.to_path_buf(), vars_added_by_this_directory.clone()); + } + working_dir = working_dir + .expect("This should not be None because of the while condition") + .parent(); + } - while let Some(wdir) = working_dir { - if wdir == directory { - return vars_to_delete; - } else { - working_dir = working_dir.expect("Root directory has no parent").parent(); - } + // Gather up all environment variables that should be deleted. + let mut vars_to_delete = IndexSet::new(); + for (dir, added_keys) in &self.added_env_vars { + if !new_added_env_vars.contains_key(dir) { + for k in added_keys { + vars_to_delete.insert(k.clone()); } - //only delete vars from directories we are not in - vars_to_delete.extend(env_vars.clone()); - vars_to_delete - }, - ); + } + } + self.added_env_vars = new_added_env_vars; Ok(vars_to_delete) } diff --git a/crates/nu-cli/src/env/environment.rs b/crates/nu-cli/src/env/environment.rs index 41e3270a61..78128e171e 100644 --- a/crates/nu-cli/src/env/environment.rs +++ b/crates/nu-cli/src/env/environment.rs @@ -1,9 +1,10 @@ use crate::data::config::Conf; +use std::io::Write; use crate::env::directory_specific_environment::*; use indexmap::{indexmap, IndexSet}; use nu_protocol::{UntaggedValue, Value}; use std::ffi::OsString; -use std::fmt::Debug; +use std::{fs::OpenOptions, fmt::Debug}; pub trait Env: Debug + Send { fn env(&self) -> Option; @@ -62,17 +63,18 @@ impl Environment { self.direnv.env_vars_to_delete()?.iter().for_each(|k| { self.remove_env(&k); }); + + // self.direnv + // .overwritten_values_to_restore()? + // .iter() + // .for_each(|(k, v)| { + // self.add_env(&k, &v.to_string_lossy(), true); + // }); + self.direnv.env_vars_to_add()?.iter().for_each(|(k, v)| { - self.add_env(&k, &v, true); + // std::env::set_var(k, v); + self.add_env(&k, &v.to_string_lossy(), true); }); - - self.direnv - .overwritten_values_to_restore()? - .iter() - .for_each(|(k, v)| { - self.add_env(&k, &v, true); - }); - Ok(()) } @@ -83,6 +85,7 @@ impl Environment { }) = self.environment_vars { envs.entries.remove(key); + std::env::remove_var(key); }; } diff --git a/crates/nu-cli/src/env/environment_syncer.rs b/crates/nu-cli/src/env/environment_syncer.rs index 7c378ea47e..a37a0dc1ea 100644 --- a/crates/nu-cli/src/env/environment_syncer.rs +++ b/crates/nu-cli/src/env/environment_syncer.rs @@ -3,6 +3,7 @@ use crate::data::config::{Conf, NuConfig}; use crate::env::environment::{Env, Environment}; use parking_lot::Mutex; use std::sync::Arc; +use nu_errors::ShellError; pub struct EnvironmentSyncer { pub env: Arc>>, @@ -41,7 +42,7 @@ impl EnvironmentSyncer { environment.morph(&*self.config); } - pub fn sync_env_vars(&mut self, ctx: &mut Context) { + pub fn sync_env_vars(&mut self, ctx: &mut Context) -> Result<(), ShellError> { let mut environment = self.env.lock(); environment.maintain_directory_environment().ok(); @@ -52,7 +53,6 @@ impl EnvironmentSyncer { // that aren't loaded from config. environment.add_env(&name, &value, false); - // clear the env var from the session // we are about to replace them ctx.with_host(|host| host.env_rm(std::ffi::OsString::from(name))); @@ -72,6 +72,7 @@ impl EnvironmentSyncer { } } } + Ok(()) } pub fn sync_path_vars(&mut self, ctx: &mut Context) {