fix(config): keep utf8 bom from breaking config reads and writes (#3962)

* fix(config): keep utf8 bom from breaking config reads and writes

refs #3678

* fix(config): only strip displaced boms when config is invalid

refs #3678

* fix(config): strip only boms the toml parser rejects

refs #3678

src/config/io.rs

@@ -93,9 +93,35 @@ fn platform_state_dir() -> PathBuf {
     }
 }
 
-fn read_optional_config(path: &Path) -> std::io::Result<Option<String>> {
+/// Normalize UTF-8 byte-order marks in config text.
+///
+/// TOML tolerates a single BOM at the very start of the document, but a BOM at
+/// the start of a later line makes the parser reject the whole file. A
+/// line-oriented edit can displace a leading BOM into the middle of the file,
+/// so drop line-start BOMs that the TOML parser actually rejects. A U+FEFF that
+/// is valid string data is kept, because its parse error would not point at it.
+fn normalize_utf8_bom(content: &str) -> String {
+    let content = content.strip_prefix('\u{feff}').unwrap_or(content);
+    if !content.contains('\u{feff}') {
+        return content.to_owned();
+    }
+
+    let mut normalized = content.to_owned();
+    while let Err(error) = normalized.parse::<toml::Value>() {
+        let Some(span) = error.span() else {
+            break;
+        };
+        if normalized.get(span.clone()) != Some("\u{feff}") {
+            break;
+        }
+        normalized.replace_range(span, "");
+    }
+    normalized
+}
+
+pub(super) fn read_optional_config(path: &Path) -> std::io::Result<Option<String>> {
     match std::fs::read_to_string(path) {
-        Ok(content) => Ok(Some(content)),
+        Ok(content) => Ok(Some(normalize_utf8_bom(&content))),
         Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None),
         Err(err) => Err(err),
     }
@@ -1108,4 +1134,68 @@ mouse_capture = false
         assert!(!removed);
         assert_eq!(updated, content);
     }
+
+    #[test]
+    fn normalize_utf8_bom_removes_a_leading_bom() {
+        let content = "\u{feff}onboarding = false\n[terminal]\n";
+        assert_eq!(
+            normalize_utf8_bom(content),
+            "onboarding = false\n[terminal]\n"
+        );
+    }
+
+    #[test]
+    fn normalize_utf8_bom_recovers_from_a_displaced_mid_file_bom() {
+        let content = "onboarding = false\n\u{feff}[terminal]\ndefault_shell = \"pwsh.exe\"\n";
+        let normalized = normalize_utf8_bom(content);
+        assert_eq!(
+            normalized,
+            "onboarding = false\n[terminal]\ndefault_shell = \"pwsh.exe\"\n"
+        );
+        assert!(normalized.parse::<toml::Value>().is_ok());
+    }
+
+    #[test]
+    fn normalize_utf8_bom_preserves_boms_in_multiline_basic_strings() {
+        let content = "[theme]\nname = \"\"\"\nfirst\n\u{feff}second\n\"\"\"\n";
+        assert!(content.parse::<toml::Value>().is_ok());
+        assert_eq!(normalize_utf8_bom(content), content);
+    }
+
+    #[test]
+    fn normalize_utf8_bom_preserves_boms_in_multiline_literal_strings() {
+        let content = "[theme]\nname = '''\nfirst\n\u{feff}second\n'''\n";
+        assert!(content.parse::<toml::Value>().is_ok());
+        assert_eq!(normalize_utf8_bom(content), content);
+    }
+
+    #[test]
+    fn normalize_utf8_bom_preserves_string_boms_despite_other_errors() {
+        let content = "[theme]\nname = \"\"\"\nfirst\n\u{feff}second\n\"\"\"\nbroken = \n";
+        assert!(content.parse::<toml::Value>().is_err());
+        assert_eq!(normalize_utf8_bom(content), content);
+    }
+
+    #[test]
+    fn config_load_recovers_from_a_mid_file_bom() {
+        let _guard = crate::config::test_config_env_lock().lock().unwrap();
+        let path = std::env::temp_dir().join(format!(
+            "herdr-config-mid-file-bom-{}.toml",
+            std::process::id()
+        ));
+        std::fs::write(
+            &path,
+            b"onboarding = false\n\xEF\xBB\xBF[terminal]\ndefault_shell = \"pwsh.exe\"\n",
+        )
+        .unwrap();
+        std::env::set_var(CONFIG_PATH_ENV_VAR, &path);
+
+        let loaded = Config::load();
+
+        std::env::remove_var(CONFIG_PATH_ENV_VAR);
+        let _ = std::fs::remove_file(path);
+
+        assert!(loaded.diagnostics.is_empty(), "{:?}", loaded.diagnostics);
+        assert_eq!(loaded.config.terminal.default_shell, "pwsh.exe");
+    }
 }

src/config/write.rs

@@ -55,9 +55,9 @@ pub(crate) fn update_file_at(
         std::fs::create_dir_all(parent)
             .map_err(|error| format!("failed to create config directory: {error}"))?;
     }
-    let content = match std::fs::read_to_string(path) {
-        Ok(content) => content,
-        Err(error) if error.kind() == std::io::ErrorKind::NotFound => String::new(),
+    let content = match super::io::read_optional_config(path) {
+        Ok(Some(content)) => content,
+        Ok(None) => String::new(),
         Err(error) => {
             return Err(format!(
                 "failed to read config before saving {description}: {error}"
@@ -73,3 +73,37 @@ pub(crate) fn write_edit(edit: ConfigEdit<'_>) -> Result<(), String> {
         edit.apply(content)
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn update_file_at_does_not_move_a_leading_bom_into_the_file() {
+        let dir = std::env::temp_dir().join(format!("herdr-config-bom-{}", std::process::id()));
+        std::fs::create_dir_all(&dir).unwrap();
+        let path = dir.join("config.toml");
+        std::fs::write(
+            &path,
+            b"\xEF\xBB\xBF[terminal]\ndefault_shell = \"pwsh.exe\"\n",
+        )
+        .unwrap();
+
+        update_file_at(&path, "onboarding setting", |content| {
+            crate::config::upsert_top_level_bool(content, "onboarding", false)
+        })
+        .unwrap();
+
+        let written = std::fs::read_to_string(&path).unwrap();
+        let _ = std::fs::remove_dir_all(dir);
+
+        assert!(
+            !written.contains('\u{feff}'),
+            "unexpected BOM in {written:?}"
+        );
+        assert!(
+            toml::from_str::<toml::Value>(&written).is_ok(),
+            "written config is not valid TOML: {written:?}"
+        );
+    }
+}