Merge branch 'direnv-rewrite'

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 {

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<Value> {
+        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 

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::{
-use std::{ffi::OsString, fmt::Debug, path::PathBuf};
+    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<PathBuf>,
+    allowed_directories: IndexSet<PathBuf>,
 
     //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<PathBuf, Vec<String>>,
+    added_env_vars: IndexMap<PathBuf, IndexSet<EnvKey>>,
 
     //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<PathBuf, Vec<(String, OsString)>>,
+    overwritten_env_vars: IndexMap<PathBuf, IndexMap<EnvKey, EnvVal>>,
 }
 
 impl DirectorySpecificEnvironment {
     pub fn new(allowed_directories: Option<Value>) -> 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<IndexMap<String, String>> {
+    pub fn overwritten_values_to_restore(&mut self) -> Result<IndexMap<EnvKey, EnvVal>> {
         let current_dir = std::env::current_dir()?;
-
-        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;
+        let mut new_overwritten_env_values = IndexMap::new();
         while let Some(wdir) = working_dir {
-                if wdir == directory.as_path() {
+            if let Some(val) = self.overwritten_env_vars.get(wdir) {
-                    re_add_keyvals = false;
+                new_overwritten_env_values.insert(wdir.to_path_buf(), val.clone());
-                    new_overwritten.insert(directory.clone(), keyvals.clone());
+            }
-                    break;
+            working_dir = working_dir
-                } else {
+                .expect("This should not be None because of the while condition")
-                    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 {
+        let mut keyvals_to_restore = IndexMap::new();
+        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(
+                    keyvals_to_restore.insert(k.clone(), v.clone());
-                        k.clone(),
-                        v.to_str()
-                            .ok_or_else(|| {
-                                Error::new(
-                                    ErrorKind::Other,
-                                    format!("{:?} is not valid unicode", v),
-                                )
-                            })?
-                            .to_string(),
-                    );
                 }
             }
         }
 
-        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<IndexMap<String, String>> {
+    pub fn env_vars_to_add(&mut self) -> std::io::Result<IndexMap<EnvKey, EnvVal>> {
         let current_dir = std::env::current_dir()?;
-
-        let mut vars_to_add = IndexMap::new();
-        for dir in &self.allowed_directories {
         let mut working_dir = Some(current_dir.as_path());
+        let mut vars_to_add = IndexMap::new();
 
         //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() {
+            if self.allowed_directories.contains(wdir) {
                 let toml_doc = std::fs::read_to_string(wdir.join(".nu-env").as_path())?
                     .parse::<toml::Value>()?;
 
-                    let vars_in_current_file = toml_doc
+                toml_doc
                     .get("env")
-                        .ok_or_else(|| {
+                    .ok_or_else(|| Error::new(ErrorKind::InvalidData, "env section missing"))?
-                            std::io::Error::new(
-                                std::io::ErrorKind::InvalidData,
-                                "No [env] section in .nu-env",
-                            )
-                        })?
                     .as_table()
-                        .ok_or_else(|| {
+                    .ok_or_else(|| Error::new(ErrorKind::InvalidData, "env section malformed"))?
-                            Error::new(ErrorKind::InvalidData, "Malformed [env] section in .nu-env")
+                    .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.
-                    self.overwritten_env_values.insert(
+                        if let Some(existing_val) = std::env::var_os(dir_env_key) {
-                        wdir.to_path_buf(),
+                            //This condition is to make sure variables in parent directories don't overwrite variables set by subdirectories.
-                        keys_in_current_nufile
+                            if !vars_to_add.contains_key(dir_env_key) {
-                            .iter()
+                                self.overwritten_env_vars
-                            .filter_map(|key| {
+                                    .entry(wdir.to_path_buf())
-                                if let Some(val) = std::env::var_os(key) {
+                                    .or_insert(IndexMap::new())
-                                    return Some((key.clone(), val));
+                                    .insert(dir_env_key.clone(), existing_val);
-                                }
-                                None
-                            })
-                            .collect(),
-                    );
 
-                    self.added_env_vars
+                                vars_to_add.insert(dir_env_key.clone(), dir_env_val);
-                        .insert(wdir.to_path_buf(), keys_in_current_nufile);
+                            }
-                    break;
                         } 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()
-                        .ok_or_else(|| {
+                .expect("This should not be None because of the while condition")
-                            Error::new(ErrorKind::NotFound, "Root directory has no parent")
-                        })?
                 .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<Vec<String>> {
+    //once they are marked for deletion, remove them from added_env_vars
+    pub fn env_vars_to_delete(&mut self) -> Result<IndexSet<EnvKey>> {
         let current_dir = std::env::current_dir()?;
-
-        //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 wdir == directory {
+            if let Some(vars_added_by_this_directory) = self.added_env_vars.get(wdir) {
-                        return vars_to_delete;
+                //If we are still in a directory, we should continue to track the vars it added.
-                    } else {
+                new_added_env_vars.insert(wdir.to_path_buf(), vars_added_by_this_directory.clone());
-                        working_dir = working_dir.expect("Root directory has no parent").parent();
+            }
+            working_dir = working_dir
+                .expect("This should not be None because of the while condition")
+                .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());
+        self.added_env_vars = new_added_env_vars;
-                vars_to_delete
-            },
-        );
 
         Ok(vars_to_delete)
     }

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<Value>;
@@ -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);
         };
     }
 

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<Mutex<Box<Environment>>>,
@@ -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) {