From 64b907dd9cf000175706287ca42b73fc0a228432 Mon Sep 17 00:00:00 2001
From: Gil Forsyth <gil@forsyth.dev>
Date: Tue, 14 Feb 2023 09:21:20 -0500
Subject: [PATCH] fix(PATH): push PATH updates into `os.environ` on change

This issue was raised in
https://github.com/xonsh/xonsh/discussions/5050.

If users (or `xontribs`) make use of `subprocess`, the `PATH` available
to `subprocess` is the value of `PATH` in `os.environ`, which is set at
the first import of `os.environ`.

This means (meant) that if a user updates their `PATH` in `xonshrc`,
those changes aren't visible to a bare `subprocess` call.
(This is what `UPDATE_OS_ENVIRON` is nominally for, but since changes to
`PATH` are mutating a mutable object, we don't hit any of the
`__setitem__` checks in the env object).

So, instead, I've added an explicit call to update the `os.environ`
value of `PATH` after each mutating call to our `EnvPath` class, so this
should also work for the various `_PATH` variables (e.g. `LDD_LIBRARY_PATH`).
---
 news/fix_os_environ_path.rst | 25 +++++++++++++++++++++++++
 tests/test_environ.py        |  2 +-
 tests/test_tools.py          | 16 ++++++++++++++++
 xonsh/environ.py             |  2 +-
 xonsh/tools.py               | 21 +++++++++++++++++++++
 5 files changed, 64 insertions(+), 2 deletions(-)
 create mode 100644 news/fix_os_environ_path.rst

diff --git a/news/fix_os_environ_path.rst b/news/fix_os_environ_path.rst
new file mode 100644
index 0000000000..0ed9b7e4b6
--- /dev/null
+++ b/news/fix_os_environ_path.rst
@@ -0,0 +1,25 @@
+**Added:**
+
+* <news item>
+
+**Changed:**
+
+* <news item>
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* Changes to ``$PATH`` are propagated to ``os.environ`` if
+  ``UPDATE_OS_ENVIRON=True`` so user and ``xontrib`` usage of ``subprocess``
+  reflects the current ``xonsh`` value for ``$PATH``
+
+**Security:**
+
+* <news item>
diff --git a/tests/test_environ.py b/tests/test_environ.py
index 220bcec759..77bf2a10c1 100644
--- a/tests/test_environ.py
+++ b/tests/test_environ.py
@@ -488,7 +488,7 @@ def test_register_custom_var_env_path():
 
 
 def test_deregister_custom_var():
-    env = Env()
+    env = Env({"UPDATE_OS_ENVIRON": True})
 
     env.register("MY_SPECIAL_VAR", type="env_path")
     env.deregister("MY_SPECIAL_VAR")
diff --git a/tests/test_tools.py b/tests/test_tools.py
index cce1961042..80710ef9b8 100644
--- a/tests/test_tools.py
+++ b/tests/test_tools.py
@@ -2048,3 +2048,19 @@ def test_is_regex_false():
 )
 def test_is_tok_color_dict(val, exp):
     assert is_tok_color_dict(val) == exp
+
+
+def test_path_updates_propagate_to_os_environ(xession):
+    with xession.env.swap(
+        {"PATH": EnvPath(xession.env["PATH"]), "UPDATE_OS_ENVIRON": True}
+    ):
+        fakepath = "not/a/real/path/at/all"
+        xession.env["PATH"].insert(0, fakepath)
+        assert fakepath in os.environ["PATH"]
+
+        xession.env["PATH"].pop(0)
+        assert fakepath not in os.environ["PATH"]
+
+        xession.env["UPDATE_OS_ENVIRON"] = False
+        xession.env["PATH"].insert(0, fakepath)
+        assert fakepath not in os.environ["PATH"]
diff --git a/xonsh/environ.py b/xonsh/environ.py
index 7e2e5c891e..ad8f9f754d 100644
--- a/xonsh/environ.py
+++ b/xonsh/environ.py
@@ -2345,7 +2345,7 @@ def register(
         )
 
     def deregister(self, name):
-        """Deregister an enviornment variable and all its type handling,
+        """Deregister an environment variable and all its type handling,
         default value, doc.
 
         Parameters
diff --git a/xonsh/tools.py b/xonsh/tools.py
index e60c165a10..8ad2597b60 100644
--- a/xonsh/tools.py
+++ b/xonsh/tools.py
@@ -174,6 +174,13 @@ def findfirst(s, substrs):
     return i, result
 
 
+@lazyobject
+def path_events():
+    from xonsh.events import events
+
+    return events
+
+
 class EnvPath(cabc.MutableSequence):
     """A class that implements an environment path, which is a list of
     strings. Provides a custom method that expands all paths if the
@@ -219,16 +226,30 @@ def __getitem__(self, item):
             return _expandpath(self._l[item])
 
     def __setitem__(self, index, item):
+        old_value = repr(self)
         self._l.__setitem__(index, item)
+        self._update_env(old_value=old_value)
 
     def __len__(self):
         return len(self._l)
 
     def __delitem__(self, key):
+        old_value = repr(self)
         self._l.__delitem__(key)
+        self._update_env(old_value=old_value)
 
     def insert(self, index, value):
+        old_value = repr(self)
         self._l.insert(index, value)
+        self._update_env(old_value=old_value)
+
+    def _update_env(self, old_value=None):
+        new_value = repr(self)
+        if xsh.env.get("UPDATE_OS_ENVIRON"):
+            xsh.env["PATH"] = self
+        path_events.on_envvar_change.fire(
+            name="PATH", oldvalue=old_value, newvalue=new_value
+        )
 
     @property
     def paths(self):