KiCad - the KiCad Bridge

@@ -866,6 +866,20 @@ pub fn pkgdir(identifier: &str) -> String {     identifier.replace('.', "_") } +/// Can this identifier name a package folder under 3rdparty? It becomes a path segment that+/// install and uninstall delete: an absolute path or a drive prefix would replace the whole path+/// in Path::join, and a package zip's metadata.json chooses it. KiCad's schema allows letters,+/// digits, '-' and '.'; '_' is kept for packages already installed under it.+pub fn identifier_is_safe(identifier: &str) -> bool {+    !identifier.is_empty() && identifier.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_'))+}++fn invalid_identifier(identifier: &str) -> Value {+    json!({"success": false, "errorCode": "invalid_identifier", "identifier": identifier,+        "error": format!("'{identifier}' is not a package identifier: only letters, digits, '.', '-' and '_' are allowed"),+        "_hint": "A PCM identifier looks like inc.adom.wiki.<owner>.<page>. Nothing was deleted or written."})+}+ pub struct Extracted {     pub files: usize,     pub symbols: Vec<String>,@@ -877,6 +891,9 @@ pub struct Extracted { /// PCM's extract(): <folder>/<contents> -> <3rdparty>/<folder>/<pkgid>/<contents>. /// Zip-slip guarded; entries outside the known folders are ignored (as PCM does). pub fn extract(zip_bytes: &[u8], third: &Path, identifier: &str) -> Result<Extracted, String> {+    if !identifier_is_safe(identifier) {+        return Err(format!("'{identifier}' is not a package identifier"));+    }     let zf = ZipFile::parse(zip_bytes)?;     let total: u64 = zf.entries.iter().map(|e| e.size).sum();     if total > MAX_ZIP {@@ -1042,6 +1059,9 @@ pub fn install(ctx: &Ctx, args: &Value, fetch: Fetch) -> Value {         return json!({"success": false, "error": "identifier is required",             "_hint": "identifier like com.adom.kicad-bridge.sample-lib; repositoryUrl is the repository.json URL (optional for an installed package: its own repository is used)."});     };+    if !identifier_is_safe(&ident) {+        return invalid_identifier(&ident);+    }     let want = s("version");     let Some(paths) = paths(ctx) else {         return json!({"success": false, "error": "KiCad user/config directories unknown",@@ -1210,7 +1230,7 @@ pub fn list(ctx: &Ctx, args: &Value, fetch: Fetch) -> Value {             let p = e.get("package").cloned().unwrap_or(Value::Null);             let id = p.get("identifier").and_then(Value::as_str).unwrap_or("");             let current = e.get("current_version").and_then(Value::as_str).unwrap_or("");-            let on_disk = FOLDERS.iter().any(|f| paths.third.join(f).join(pkgdir(id)).exists());+            let on_disk = identifier_is_safe(id) && FOLDERS.iter().any(|f| paths.third.join(f).join(pkgdir(id)).exists());             let mut row = json!({"identifier": p.get("identifier"), "name": p.get("name"), "type": p.get("type"),                    "version": e.get("current_version"), "repository": e.get("repository_name"),                    "repositoryId": e.get("repository_id"), "pinned": e.get("pinned"), "onDisk": on_disk,@@ -1270,6 +1290,9 @@ pub fn uninstall(ctx: &Ctx, args: &Value) -> Value {     let Some(ident) = args.get("identifier").and_then(Value::as_str).map(str::trim).filter(|s| !s.is_empty()) else {         return json!({"success": false, "error": "identifier is required"});     };+    if !identifier_is_safe(ident) {+        return invalid_identifier(ident);+    }     let Some(paths) = paths(ctx) else {         return json!({"success": false, "error": "KiCad user/config directories unknown"});     };@@ -1474,6 +1497,92 @@ mod tests {         assert_eq!(u["recordRemoved"], json!(false));     } +    /// A package zip holding only metadata.json, with the identifier given.+    fn zip_with_identifier(identifier: &str) -> Vec<u8> {+        let name = b"metadata.json";+        let body = json!({"identifier": identifier, "name": "x", "type": "library",+            "versions": [{"version": "1.0.0", "status": "stable", "kicad_version": "9.0"}]}).to_string().into_bytes();+        let crc = {+            let mut c = !0u32;+            for b in &body {+                c ^= *b as u32;+                for _ in 0..8 {+                    c = if c & 1 == 1 { (c >> 1) ^ 0xEDB8_8320 } else { c >> 1 };+                }+            }+            !c+        };+        let size = (body.len() as u32).to_le_bytes();+        let header = |sig: &[u8], v: &mut Vec<u8>, version_made: bool| {+            v.extend_from_slice(sig);+            if version_made {+                v.extend_from_slice(&20u16.to_le_bytes());+            }+            v.extend_from_slice(&20u16.to_le_bytes());+            v.extend_from_slice(&[0; 8]); // flags, stored, time, date+            v.extend_from_slice(&crc.to_le_bytes());+            v.extend_from_slice(&size);+            v.extend_from_slice(&size);+            v.extend_from_slice(&(name.len() as u16).to_le_bytes());+            v.extend_from_slice(&[0; 2]);+        };+        let mut z = Vec::new();+        header(b"PK\x03\x04", &mut z, false);+        z.extend_from_slice(name);+        z.extend_from_slice(&body);+        let cd = z.len() as u32;+        header(b"PK\x01\x02", &mut z, true);+        z.extend_from_slice(&[0; 10]); // comment, disk, attributes+        z.extend_from_slice(&0u32.to_le_bytes()); // local header offset+        z.extend_from_slice(name);+        let cd_size = z.len() as u32 - cd;+        z.extend_from_slice(b"PK\x05\x06");+        z.extend_from_slice(&[0; 4]);+        z.extend_from_slice(&1u16.to_le_bytes());+        z.extend_from_slice(&1u16.to_le_bytes());+        z.extend_from_slice(&cd_size.to_le_bytes());+        z.extend_from_slice(&cd.to_le_bytes());+        z.extend_from_slice(&[0; 2]);+        z+    }++    #[test]+    fn a_path_as_identifier_deletes_nothing() {+        let t = tmp("identpath");+        let c = ctx(&t.0);+        let victim = t.0.join("victim");+        std::fs::create_dir_all(&victim).unwrap();+        std::fs::write(victim.join("keep.txt"), "keep").unwrap();+        let abs = victim.to_string_lossy().to_string();+        let unused = |u: &str, _t: Duration| -> Result<Vec<u8>, String> { Err(format!("no network in this test: {u}")) };++        // the identifier a package zip chooses for itself+        let zip = t.0.join("evil.zip");+        std::fs::write(&zip, zip_with_identifier(&abs)).unwrap();+        let r = install(&c, &json!({"zipPath": zip.to_string_lossy()}), &unused);+        assert_eq!(r["errorCode"], json!("invalid_identifier"), "{r}");++        // the same through the identifier argument+        let r = install(&c, &json!({"repositoryUrl": REPO_URL, "identifier": abs}), &unused);+        assert_eq!(r["errorCode"], json!("invalid_identifier"), "{r}");+        let r = uninstall(&c, &json!({"identifier": abs}));+        assert_eq!(r["errorCode"], json!("invalid_identifier"), "{r}");++        assert_eq!(std::fs::read_to_string(victim.join("keep.txt")).unwrap(), "keep", "the folder must survive");+        assert!(extract(ZIP, &t.0, &abs).is_err());+        assert!(!t.0.join("config/installed_packages.json").exists(), "nothing recorded");+    }++    #[test]+    fn identifier_rule() {+        for ok in ["inc.adom.wiki.adom.test-board", "com.adom.kicad-bridge.sample-lib", "with_underscore.lib", "A1"] {+            assert!(identifier_is_safe(ok), "{ok}");+        }+        for bad in ["", "/etc", "/Users/me/Documents", r"C:\Users\me", "C:foo", "a/b", r"a\b", "../up", "a b", "a\0b", "\u{e9}t\u{e9}"] {+            assert!(!identifier_is_safe(bad), "{bad}");+        }+    }+     #[test]     fn install_refusals() {         let t = tmp("refuse");