Fix expand tilde and invalid path chars on windows

The crate shellexpand contained an error for windows platforms, where tilde characters where only expanded correctly, if preceeded by a forward slash. Although, a patch is currently in the pipeline, the crate owner has not merged it for almost a year and the repo generally appears inactive.

Further, the colon ':' character is disallowed in filepaths on windows, as it is used on the NTFS filesystem to delimit alternative data streams (e.g. %HOMEPATH%\Downloads\readme.txt:Zone.Identifier). The violation occurs in tor_dirmgr::src::storage::sqlite::SqliteStore::save_blob_internal in the filename generation.

This patch therefore breaks backwards compatibility, but the given issues would prohibit windows portability as well.

The following unit tests that failed on windows are addressed by this patch:

• tor_config::src::path::test::expand_home
• tor_config::src::path::test::expand_cache
• tor_dirmgr::src::storage::sqlite::test::consensus
• tor_dirmgr::src::lib::test::load_and_store_internals
• tor_dirmgr::src::lib::test::make_consensus_request
• tor_dirmgr::src::lib::test::expand_response

crates/tor-config/src/path.rs

@@ -194,7 +194,6 @@ mod test {
         assert_eq!(p.path().unwrap().to_str(), Some("/usr/local/foo"));
     }
 
-    // FIXME: Add test for windows
     #[cfg(not(target_family = "windows"))]
     #[test]
     fn expand_home() {
@@ -209,7 +208,20 @@ mod test {
         assert_eq!(p.path().unwrap().to_str(), expected.to_str());
     }
 
-    // FIXME: Add test for windows
+    #[cfg(target_family = "windows")]
+    #[test]
+    fn expand_home() {
+        let p = CfgPath::new("~\\.arti\\config".to_string());
+        assert_eq!(p.to_string(), "~\\.arti\\config".to_string());
+
+        let expected = dirs::home_dir().unwrap().join(".arti\\config");
+        assert_eq!(p.path().unwrap().to_str(), expected.to_str());
+
+        let p = CfgPath::new("${USER_HOME}\\.arti\\config".to_string());
+        assert_eq!(p.to_string(), "${USER_HOME}\\.arti\\config".to_string());
+        assert_eq!(p.path().unwrap().to_str(), expected.to_str());
+    }
+
     #[cfg(not(target_family = "windows"))]
     #[test]
     fn expand_cache() {
@@ -220,6 +232,16 @@ mod test {
         assert_eq!(p.path().unwrap().to_str(), expected.to_str());
     }
 
+    #[cfg(target_family = "windows")]
+    #[test]
+    fn expand_cache() {
+        let p = CfgPath::new("${ARTI_CACHE}\\example".to_string());
+        assert_eq!(p.to_string(), "${ARTI_CACHE}\\example".to_string());
+
+        let expected = project_dirs().unwrap().cache_dir().join("example");
+        assert_eq!(p.path().unwrap().to_str(), expected.to_str());
+    }
+
     #[test]
     fn expand_bogus() {
         let p = CfgPath::new("${ARTI_WOMBAT}/example".to_string());

[Ian Jackson]

Were you aware of the Rust raw strings feature? I think you could write this like this, for example:

-        let p = CfgPath::new("~\\.arti\\config".to_string());
+        let p = CfgPath::new(r"~\.arti\config".to_string());

NB, untested. Whether to do that here is a matter of taste I think, and certainly use of the doubled toothpicks is no reason not to merge this.