Merge pull request #640 from Jongy/handle-non-utf8-env

Handle non Unicode env keys/values

Author: epage

src/env.rs

@@ -1,4 +1,5 @@
 use std::env;
+use std::ffi::OsString;
 
 #[cfg(feature = "convert-case")]
 use convert_case::{Case, Casing};
@@ -7,6 +8,7 @@ use crate::error::Result;
 use crate::map::Map;
 use crate::source::Source;
 use crate::value::{Value, ValueKind};
+use crate::ConfigError;
 
 /// An environment source collects a dictionary of environment variables values into a hierarchical
 /// config Value type. We have to be aware how the config tree is created from the environment
@@ -243,10 +245,16 @@ impl Source for Environment {
             .as_ref()
             .map(|prefix| format!("{prefix}{prefix_separator}").to_lowercase());
 
-        let collector = |(key, value): (String, String)| {
+        let collector = |(key, value): (OsString, OsString)| {
+            let key = match key.into_string() {
+                Ok(key) => key,
+                // Key is not valid unicode, skip it
+                Err(_) => return Ok(()),
+            };
+
             // Treat empty environment variables as unset
             if self.ignore_empty && value.is_empty() {
-                return;
+                return Ok(());
             }
 
             let mut key = key.to_lowercase();
@@ -260,10 +268,18 @@ impl Source for Environment {
                     }
                 } else {
                     // Skip this key
-                    return;
+                    return Ok(());
                 }
             }
 
+            // At this point, we don't know if the key is required or not.
+            // Therefore if the value is not a valid unicode string, we error out.
+            let value = value.into_string().map_err(|os_string| {
+                ConfigError::Message(format!(
+                    "env variable {key:?} contains non-Unicode data: {os_string:?}"
+                ))
+            })?;
+
             // If separator is given replace with `.`
             if !separator.is_empty() {
                 key = key.replace(separator, ".");
@@ -308,12 +324,18 @@ impl Source for Environment {
             };
 
             m.insert(key, Value::new(Some(&uri), value));
+
+            Ok(())
         };
 
         match &self.source {
-            Some(source) => source.clone().into_iter().for_each(collector),
-            None => env::vars().for_each(collector),
-        }
+            Some(source) => source
+                .clone()
+                .into_iter()
+                .map(|(key, value)| (key.into(), value.into()))
+                .try_for_each(collector),
+            None => env::vars_os().try_for_each(collector),
+        }?;
 
         Ok(m)
     }

tests/testsuite/env.rs

@@ -721,3 +721,86 @@ fn test_parse_uint_default() {
     let config: TestUint = config.try_deserialize().unwrap();
     assert_eq!(config.int_val, 42);
 }
+
+#[cfg(any(unix, windows))]
+#[cfg(test)]
+mod unicode_tests {
+    use std::ffi::OsString;
+
+    use super::*;
+
+    fn make_invalid_unicode_os_string() -> OsString {
+        let string = {
+            #[cfg(unix)]
+            {
+                use std::os::unix::ffi::OsStringExt;
+
+                OsString::from_vec(vec![0xff])
+            }
+            #[cfg(windows)]
+            {
+                use std::os::windows::ffi::OsStringExt;
+
+                OsString::from_wide(&[0xd800]) // unpaired high surrogate
+            }
+        };
+
+        assert!(string.to_str().is_none());
+
+        string
+    }
+
+    #[test]
+    fn test_invalid_unicode_key_ignored() {
+        temp_env::with_vars(
+            vec![
+                (make_invalid_unicode_os_string(), Some("abc")),
+                ("A_B_C".into(), Some("abc")),
+            ],
+            || {
+                let vars = Environment::default().collect().unwrap();
+
+                assert!(vars.contains_key("a_b_c"));
+            },
+        );
+    }
+
+    #[test]
+    fn test_invalid_unicode_value_filtered() {
+        temp_env::with_vars(
+            vec![
+                ("invalid_value1", Some(make_invalid_unicode_os_string())),
+                ("valid_value2", Some("valid".into())),
+            ],
+            || {
+                let vars = Environment::with_prefix("valid")
+                    .keep_prefix(true)
+                    .collect()
+                    .unwrap();
+
+                assert!(!vars.contains_key("invalid_value1"));
+                assert!(vars.contains_key("valid_value2"));
+            },
+        );
+    }
+
+    #[test]
+    fn test_invalid_unicode_value_not_filtered() {
+        temp_env::with_vars(
+            vec![("invalid_value1", Some(make_invalid_unicode_os_string()))],
+            || {
+                let result = Environment::default().collect();
+
+                #[cfg(unix)]
+                let expected =
+                    str![[r#"env variable "invalid_value1" contains non-Unicode data: "/xFF""#]];
+                #[cfg(windows)]
+                let expected = str![[
+                    r#"env variable "invalid_value1" contains non-Unicode data: "/u{d800}""#
+                ]];
+
+                assert_data_eq!(result.unwrap_err().to_string(), expected);
+            },
+        );
+    }
+}