fix(security): skip symbolic links during skill updates

Modified `src/skills/update.rs` to ensure symbolic links are not
followed when updating a skill from a local source. This prevents
potential information disclosure of sensitive host files.

Consistent with the hardening previously applied to the installation
path in `src/skills/install.rs`.

Added regression test in `tests/test_update_security.rs`.

Author: yacosta738

.agents/journal/sentinel.md

@@ -31,3 +31,8 @@ tool that are known to contain user-provided credentials or sensitive environmen
 **Vulnerability:** String-based path checks (like `starts_with('/')`) or platform-specific `Path::is_absolute()` calls failed to detect Windows-style absolute paths (e.g., `C:/...`) when running on Unix systems, leading to potential path traversal vulnerabilities in skill archives.
 **Learning:** Rust's `std::path::Path` behavior depends on the host operating system. On Unix, a path starting with a drive letter is considered relative, which allows it to pass simple `is_absolute()` checks and be joined to a base directory, potentially escaping it if not carefully validated.
 **Prevention:** Use `Path::components()` to explicitly reject `RootDir`, `Prefix`, and `ParentDir` components. For cross-platform safety on Unix, also manually check for Windows drive letter patterns (e.g., `filename.as_bytes()[1] == b':'`) in untrusted archive entry paths.
+
+## 2025-05-19 - Inconsistent Symlink Handling in Skill Updates
+**Vulnerability:** While `src/skills/install.rs` was previously hardened against symlink following, `src/skills/update.rs` used a separate implementation of directory copying (`copy_dir_all`) that still followed symbolic links. This allowed information disclosure during local skill updates even after the installation path was secured.
+**Learning:** Security fixes must be applied comprehensively across all code paths performing similar operations. Code duplication (like having multiple recursive copy functions) often leads to inconsistent security postures where one path is hardened but another remains vulnerable.
+**Prevention:** Consolidate critical filesystem operations into shared, hardened utilities. If duplication is necessary, use grep to find all instances of risky patterns (like `fs::copy` in a loop) when patching a vulnerability.

src/skills/update.rs

@@ -195,6 +195,13 @@ fn copy_dir_all(src: &Path, dst: &Path) -> std::io::Result<()> {
     for entry in fs::read_dir(src)? {
         let entry = entry?;
         let ty = entry.file_type()?;
+
+        // SECURITY: Skip symbolic links to prevent information disclosure of host files
+        // during local skill updates.
+        if ty.is_symlink() {
+            continue;
+        }
+
         let src_path = entry.path();
         let dst_path = dst.join(entry.file_name());
         if ty.is_dir() {

tests/test_update_security.rs

@@ -0,0 +1,82 @@
+use std::fs;
+use std::process::Command;
+use tempfile::TempDir;
+
+#[cfg(unix)]
+use std::os::unix::fs::symlink;
+
+fn agentsync_bin() -> &'static str {
+    env!("CARGO_BIN_EXE_agentsync")
+}
+
+#[test]
+#[cfg(unix)]
+fn test_update_skill_skips_symlinks() {
+    let temp_dir = TempDir::new().unwrap();
+    let project_root = temp_dir.path().join("project");
+    fs::create_dir_all(&project_root).unwrap();
+
+    // 1. Install an initial skill
+    let initial_src = temp_dir.path().join("initial_src");
+    fs::create_dir_all(&initial_src).unwrap();
+    fs::write(
+        initial_src.join("SKILL.md"),
+        "---\nname: test-skill\nversion: 1.0.0\n---\n",
+    )
+    .unwrap();
+
+    let install_out = Command::new(agentsync_bin())
+        .current_dir(&project_root)
+        .arg("skill")
+        .arg("install")
+        .arg("test-skill")
+        .arg("--source")
+        .arg(initial_src.to_str().unwrap())
+        .output()
+        .expect("failed to run install");
+    assert!(install_out.status.success());
+
+    // 2. Create a sensitive file outside
+    let sensitive_file = temp_dir.path().join("sensitive.txt");
+    fs::write(&sensitive_file, "SECRET_CONTENT").unwrap();
+
+    // 3. Create malicious update source
+    let malicious_src = temp_dir.path().join("malicious_src");
+    fs::create_dir_all(&malicious_src).unwrap();
+    fs::write(
+        malicious_src.join("SKILL.md"),
+        "---\nname: test-skill\nversion: 1.1.0\n---\n",
+    )
+    .unwrap();
+
+    // Create a symlink to the sensitive file
+    symlink(&sensitive_file, malicious_src.join("leak.txt")).unwrap();
+
+    // 4. Run update
+    let update_out = Command::new(agentsync_bin())
+        .current_dir(&project_root)
+        .arg("skill")
+        .arg("update")
+        .arg("test-skill")
+        .arg("--source")
+        .arg(malicious_src.to_str().unwrap())
+        .output()
+        .expect("failed to run update");
+
+    assert!(
+        update_out.status.success(),
+        "Update failed: {}",
+        String::from_utf8_lossy(&update_out.stderr)
+    );
+
+    // 5. Verify the leak
+    let leaked_path = project_root.join(".agents/skills/test-skill/leak.txt");
+
+    // After the fix, leak.txt should NOT exist because symlinks are skipped.
+    // Before the fix, leak.txt WILL exist and contain "SECRET_CONTENT" because fs::copy follows symlinks.
+    assert!(
+        !leaked_path.exists(),
+        "SECURITY VULNERABILITY: Symlink was followed during update, leaking content to {}",
+        leaked_path.display()
+    );
+}