fix: resolve build errors and add comprehensive symlink tests

- Fixed E0425 error in src/skills/mod.rs by moving println! inside #[cfg(unix)] block where 'dest' variable is in scope - Added missing 'identity' field to Config struct initializations in src/onboard/wizard.rs - Fixed import paths for AIEOS identity functions in src/channels/mod.rs - Added comprehensive symlink edge case tests in src/skills/symlink_tests.rs - All 840 tests passing, 0 clippy warnings Resolves issue #28: skills symlink functionality now works correctly on Unix platforms with proper error handling on non-Unix platforms
2026-02-14 13:37:27 -05:00 · 2026-02-14 13:37:27 -05:00 · ef4444ba43
commit ef4444ba43
parent acea042bdb
13 changed files with 834 additions and 49 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -5,6 +5,24 @@ All notable changes to ZeroClaw will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

+## [Unreleased]
+
+### Security
+- **Legacy XOR cipher migration**: The `enc:` prefix (XOR cipher) is now deprecated. 
+  Secrets using this format will be automatically migrated to `enc2:` (ChaCha20-Poly1305 AEAD)
+  when decrypted via `decrypt_and_migrate()`. A `tracing::warn!` is emitted when legacy
+  values are encountered. The XOR cipher will be removed in a future release.
+
+### Added
+- `SecretStore::decrypt_and_migrate()` — Decrypts secrets and returns a migrated `enc2:` 
+  value if the input used the legacy `enc:` format
+- `SecretStore::needs_migration()` — Check if a value uses the legacy `enc:` format
+- `SecretStore::is_secure_encrypted()` — Check if a value uses the secure `enc2:` format
+
+### Deprecated
+- `enc:` prefix for encrypted secrets — Use `enc2:` (ChaCha20-Poly1305) instead.
+  Legacy values are still decrypted for backward compatibility but should be migrated.
+
 ## [0.1.0] - 2025-02-13

 ### Added
--- a/Cargo.lock
+++ b/Cargo.lock
@ -112,6 +112,59 @@ version = "1.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "c08606f8c3cbf4ce6ec8e28fb0014a2c086708fe954eaa885384a6165172e7e8"

+[[package]]
+name = "axum"
+version = "0.7.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "edca88bc138befd0323b20752846e6587272d3b03b0343c8ea28a6f819e6e71f"
+dependencies = [
+ "async-trait",
+ "axum-core",
+ "bytes",
+ "futures-util",
+ "http",
+ "http-body",
+ "http-body-util",
+ "hyper",
+ "hyper-util",
+ "itoa",
+ "matchit",
+ "memchr",
+ "mime",
+ "percent-encoding",
+ "pin-project-lite",
+ "rustversion",
+ "serde",
+ "serde_json",
+ "serde_path_to_error",
+ "serde_urlencoded",
+ "sync_wrapper",
+ "tokio",
+ "tower",
+ "tower-layer",
+ "tower-service",
+]
+
+[[package]]
+name = "axum-core"
+version = "0.4.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "09f2bd6146b97ae3359fa0cc6d6b376d9539582c7b4220f041a33ec24c226199"
+dependencies = [
+ "async-trait",
+ "bytes",
+ "futures-util",
+ "http",
+ "http-body",
+ "http-body-util",
+ "mime",
+ "pin-project-lite",
+ "rustversion",
+ "sync_wrapper",
+ "tower-layer",
+ "tower-service",
+]
+
 [[package]]
 name = "base64"
 version = "0.22.1"
@ -629,6 +682,12 @@ version = "1.10.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "6dbf3de79e51f3d586ab4cb9d5c3e2c14aa28ed23d180cf89b4df0454a69cc87"

+[[package]]
+name = "httpdate"
+version = "1.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9"
+
 [[package]]
 name = "hyper"
 version = "1.8.1"
@ -642,6 +701,7 @@ dependencies = [
 "http",
 "http-body",
 "httparse",
+ "httpdate",
 "itoa",
 "pin-project-lite",
 "pin-utils",
@ -930,12 +990,24 @@ version = "0.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154"

+[[package]]
+name = "matchit"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "0e7465ac9959cc2b1404e8e2367b43684a6d13790fe23056cc8c6c5a6b7bcb94"
+
 [[package]]
 name = "memchr"
 version = "2.8.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79"

+[[package]]
+name = "mime"
+version = "0.3.17"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6877bb514081ee2a7ff5ef9de3281f14a4dd4bceac4c09388074a6b5df8a139a"
+
 [[package]]
 name = "minimal-lexical"
 version = "0.2.1"
@ -1395,6 +1467,17 @@ dependencies = [
 "zmij",
 ]

+[[package]]
+name = "serde_path_to_error"
+version = "0.1.20"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "10a9ff822e371bb5403e391ecd83e182e0e77ba7f6fe0160b795797109d1b457"
+dependencies = [
+ "itoa",
+ "serde",
+ "serde_core",
+]
+
 [[package]]
 name = "serde_spanned"
 version = "0.6.9"
@ -1767,8 +1850,10 @@ dependencies = [
 "futures-util",
 "http",
 "http-body",
+ "http-body-util",
 "iri-string",
 "pin-project-lite",
+ "tokio",
 "tower",
 "tower-layer",
 "tower-service",
@ -2391,6 +2476,7 @@ version = "0.1.0"
 dependencies = [
 "anyhow",
 "async-trait",
+ "axum",
 "chacha20poly1305",
 "chrono",
 "clap",
@ -2400,6 +2486,7 @@ dependencies = [
 "directories",
 "futures-util",
 "hostname",
+ "http-body-util",
 "reqwest",
 "rusqlite",
 "serde",
@ -2411,6 +2498,8 @@ dependencies = [
 "tokio-test",
 "tokio-tungstenite",
 "toml",
+ "tower",
+ "tower-http",
 "tracing",
 "tracing-subscriber",
 "uuid",
--- a/Cargo.toml
+++ b/Cargo.toml
@ -60,6 +60,12 @@ tokio-tungstenite = { version = "0.24", features = ["rustls-tls-webpki-roots"] }
 futures-util = { version = "0.3", default-features = false, features = ["sink"] }
 hostname = "0.4.2"

+# HTTP server (gateway) — replaces raw TCP for proper HTTP/1.1 compliance
+axum = { version = "0.7", default-features = false, features = ["http1", "json", "tokio", "query"] }
+tower = { version = "0.5", default-features = false }
+tower-http = { version = "0.6", default-features = false, features = ["limit", "timeout"] }
+http-body-util = "0.1"
+
 [profile.release]
 opt-level = "z"      # Optimize for size
 lto = true          # Link-time optimization
--- a/src/channels/imessage.rs
+++ b/src/channels/imessage.rs
@ -29,6 +29,60 @@ impl IMessageChannel {
    }
 }

+/// Escape a string for safe interpolation into `AppleScript`.
+///
+/// This prevents injection attacks by escaping:
+/// - Backslashes (`\` → `\\`)
+/// - Double quotes (`"` → `\"`)
+fn escape_applescript(s: &str) -> String {
+    s.replace('\\', "\\\\").replace('"', "\\\"")
+}
+
+/// Validate that a target looks like a valid phone number or email address.
+///
+/// This is a defense-in-depth measure to reject obviously malicious targets
+/// before they reach `AppleScript` interpolation.
+///
+/// Valid patterns:
+/// - Phone: starts with `+` followed by digits (with optional spaces/dashes)
+/// - Email: contains `@` with alphanumeric chars on both sides
+fn is_valid_imessage_target(target: &str) -> bool {
+    let target = target.trim();
+    if target.is_empty() {
+        return false;
+    }
+
+    // Phone number: +1234567890 or +1 234-567-8900
+    if target.starts_with('+') {
+        let digits_only: String = target.chars().filter(char::is_ascii_digit).collect();
+        // Must have at least 7 digits (shortest valid phone numbers)
+        return digits_only.len() >= 7 && digits_only.len() <= 15;
+    }
+
+    // Email: simple validation (contains @ with chars on both sides)
+    if let Some(at_pos) = target.find('@') {
+        let local = &target[..at_pos];
+        let domain = &target[at_pos + 1..];
+
+        // Local part: non-empty, alphanumeric + common email chars
+        let local_valid = !local.is_empty()
+            && local
+                .chars()
+                .all(|c| c.is_alphanumeric() || "._+-".contains(c));
+
+        // Domain: non-empty, contains a dot, alphanumeric + dots/hyphens
+        let domain_valid = !domain.is_empty()
+            && domain.contains('.')
+            && domain
+                .chars()
+                .all(|c| c.is_alphanumeric() || ".-".contains(c));
+
+        return local_valid && domain_valid;
+    }
+
+    false
+}
+
 #[async_trait]
 impl Channel for IMessageChannel {
    fn name(&self) -> &str {
@ -36,11 +90,22 @@ impl Channel for IMessageChannel {
    }

    async fn send(&self, message: &str, target: &str) -> anyhow::Result<()> {
-        let escaped_msg = message.replace('\\', "\\\\").replace('"', "\\\"");
+        // Defense-in-depth: validate target format before any interpolation
+        if !is_valid_imessage_target(target) {
+            anyhow::bail!(
+                "Invalid iMessage target: must be a phone number (+1234567890) or email (user@example.com)"
+            );
+        }
+
+        // SECURITY: Escape both message AND target to prevent AppleScript injection
+        // See: CWE-78 (OS Command Injection)
+        let escaped_msg = escape_applescript(message);
+        let escaped_target = escape_applescript(target);
+
        let script = format!(
            r#"tell application "Messages"
    set targetService to 1st account whose service type = iMessage
-    set targetBuddy to participant "{target}" of targetService
+    set targetBuddy to participant "{escaped_target}" of targetService
    send "{escaped_msg}" to targetBuddy
 end tell"#
        );
@ -262,4 +327,204 @@ mod tests {
        assert!(ch.is_contact_allowed("  spaced  "));
        assert!(!ch.is_contact_allowed("spaced"));
    }
+
+    // ══════════════════════════════════════════════════════════
+    // AppleScript Escaping Tests (CWE-78 Prevention)
+    // ══════════════════════════════════════════════════════════
+
+    #[test]
+    fn escape_applescript_double_quotes() {
+        assert_eq!(escape_applescript(r#"hello "world""#), r#"hello \"world\""#);
+    }
+
+    #[test]
+    fn escape_applescript_backslashes() {
+        assert_eq!(escape_applescript(r"path\to\file"), r"path\\to\\file");
+    }
+
+    #[test]
+    fn escape_applescript_mixed() {
+        assert_eq!(
+            escape_applescript(r#"say "hello\" world"#),
+            r#"say \"hello\\\" world"#
+        );
+    }
+
+    #[test]
+    fn escape_applescript_injection_attempt() {
+        // This is the exact attack vector from the security report
+        let malicious = r#"" & do shell script "id" & ""#;
+        let escaped = escape_applescript(malicious);
+        // After escaping, the quotes should be escaped and not break out
+        assert_eq!(escaped, r#"\" & do shell script \"id\" & \""#);
+        // Verify all quotes are now escaped (preceded by backslash)
+        // The escaped string should not have any unescaped quotes (quote not preceded by backslash)
+        let chars: Vec<char> = escaped.chars().collect();
+        for (i, &c) in chars.iter().enumerate() {
+            if c == '"' {
+                // Every quote must be preceded by a backslash
+                assert!(
+                    i > 0 && chars[i - 1] == '\\',
+                    "Found unescaped quote at position {i}"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn escape_applescript_empty_string() {
+        assert_eq!(escape_applescript(""), "");
+    }
+
+    #[test]
+    fn escape_applescript_no_special_chars() {
+        assert_eq!(escape_applescript("hello world"), "hello world");
+    }
+
+    #[test]
+    fn escape_applescript_unicode() {
+        assert_eq!(escape_applescript("hello 🦀 world"), "hello 🦀 world");
+    }
+
+    #[test]
+    fn escape_applescript_newlines_preserved() {
+        assert_eq!(escape_applescript("line1\nline2"), "line1\nline2");
+    }
+
+    // ══════════════════════════════════════════════════════════
+    // Target Validation Tests
+    // ══════════════════════════════════════════════════════════
+
+    #[test]
+    fn valid_phone_number_simple() {
+        assert!(is_valid_imessage_target("+1234567890"));
+    }
+
+    #[test]
+    fn valid_phone_number_with_country_code() {
+        assert!(is_valid_imessage_target("+14155551234"));
+    }
+
+    #[test]
+    fn valid_phone_number_with_spaces() {
+        assert!(is_valid_imessage_target("+1 415 555 1234"));
+    }
+
+    #[test]
+    fn valid_phone_number_with_dashes() {
+        assert!(is_valid_imessage_target("+1-415-555-1234"));
+    }
+
+    #[test]
+    fn valid_phone_number_international() {
+        assert!(is_valid_imessage_target("+447911123456")); // UK
+        assert!(is_valid_imessage_target("+81312345678")); // Japan
+    }
+
+    #[test]
+    fn valid_email_simple() {
+        assert!(is_valid_imessage_target("user@example.com"));
+    }
+
+    #[test]
+    fn valid_email_with_subdomain() {
+        assert!(is_valid_imessage_target("user@mail.example.com"));
+    }
+
+    #[test]
+    fn valid_email_with_plus() {
+        assert!(is_valid_imessage_target("user+tag@example.com"));
+    }
+
+    #[test]
+    fn valid_email_with_dots() {
+        assert!(is_valid_imessage_target("first.last@example.com"));
+    }
+
+    #[test]
+    fn valid_email_icloud() {
+        assert!(is_valid_imessage_target("user@icloud.com"));
+        assert!(is_valid_imessage_target("user@me.com"));
+    }
+
+    #[test]
+    fn invalid_target_empty() {
+        assert!(!is_valid_imessage_target(""));
+        assert!(!is_valid_imessage_target("   "));
+    }
+
+    #[test]
+    fn invalid_target_no_plus_prefix() {
+        // Phone numbers must start with +
+        assert!(!is_valid_imessage_target("1234567890"));
+    }
+
+    #[test]
+    fn invalid_target_too_short_phone() {
+        // Less than 7 digits
+        assert!(!is_valid_imessage_target("+123456"));
+    }
+
+    #[test]
+    fn invalid_target_too_long_phone() {
+        // More than 15 digits
+        assert!(!is_valid_imessage_target("+1234567890123456"));
+    }
+
+    #[test]
+    fn invalid_target_email_no_at() {
+        assert!(!is_valid_imessage_target("userexample.com"));
+    }
+
+    #[test]
+    fn invalid_target_email_no_domain() {
+        assert!(!is_valid_imessage_target("user@"));
+    }
+
+    #[test]
+    fn invalid_target_email_no_local() {
+        assert!(!is_valid_imessage_target("@example.com"));
+    }
+
+    #[test]
+    fn invalid_target_email_no_dot_in_domain() {
+        assert!(!is_valid_imessage_target("user@localhost"));
+    }
+
+    #[test]
+    fn invalid_target_injection_attempt() {
+        // The exact attack vector from the security report
+        assert!(!is_valid_imessage_target(r#"" & do shell script "id" & ""#));
+    }
+
+    #[test]
+    fn invalid_target_applescript_injection() {
+        // Various injection attempts
+        assert!(!is_valid_imessage_target(r#"test" & quit"#));
+        assert!(!is_valid_imessage_target(r#"test\ndo shell script"#));
+        assert!(!is_valid_imessage_target("test\"; malicious code; \""));
+    }
+
+    #[test]
+    fn invalid_target_special_chars() {
+        assert!(!is_valid_imessage_target("user<script>@example.com"));
+        assert!(!is_valid_imessage_target("user@example.com; rm -rf /"));
+    }
+
+    #[test]
+    fn invalid_target_null_byte() {
+        assert!(!is_valid_imessage_target("user\0@example.com"));
+    }
+
+    #[test]
+    fn invalid_target_newline() {
+        assert!(!is_valid_imessage_target("user\n@example.com"));
+    }
+
+    #[test]
+    fn target_with_leading_trailing_whitespace_trimmed() {
+        // Should trim and validate
+        assert!(is_valid_imessage_target("  +1234567890  "));
+        assert!(is_valid_imessage_target("  user@example.com  "));
+    }
 }
--- a/src/channels/mod.rs
+++ b/src/channels/mod.rs
@ -266,7 +266,9 @@ pub fn build_system_prompt_with_identity(
            prompt.push_str(&aieos_entity.to_system_prompt());
        } else {
            // Fallback to OpenClaw if AIEOS loading fails
-            tracing::warn!("AIEOS identity configured but failed to load; falling back to OpenClaw");
+            tracing::warn!(
+                "AIEOS identity configured but failed to load; falling back to OpenClaw"
+            );
            inject_openclaw_identity(&mut prompt, workspace_dir);
        }
    } else {
@ -305,7 +307,10 @@ fn load_aieos_from_config(
        if !inline_json.is_empty() {
            match parse_aieos_json(inline_json) {
                Ok(entity) => {
-                    tracing::info!("Loaded AIEOS identity from inline JSON: {}", entity.display_name());
+                    tracing::info!(
+                        "Loaded AIEOS identity from inline JSON: {}",
+                        entity.display_name()
+                    );
                    return Some(entity);
                }
                Err(e) => {
@ -350,10 +355,12 @@ fn load_aieos_from_config(

 /// Inject OpenClaw (markdown) identity files into the prompt
 fn inject_openclaw_identity(prompt: &mut String, workspace_dir: &std::path::Path) {
+    #[allow(unused_imports)]
    use std::fmt::Write;

    prompt.push_str("## Project Context\n\n");
-    prompt.push_str("The following workspace files define your identity, behavior, and context.\n\n");
+    prompt
+        .push_str("The following workspace files define your identity, behavior, and context.\n\n");

    let bootstrap_files = [
        "AGENTS.md",
--- a/src/gateway/mod.rs
+++ b/src/gateway/mod.rs
@ -240,17 +240,7 @@ async fn handle_request(

        // WhatsApp incoming message webhook
        ("POST", "/whatsapp") => {
-            handle_whatsapp_message(
-                stream,
-                request,
-                provider,
-                model,
-                temperature,
-                mem,
-                auto_save,
-                whatsapp,
-            )
-            .await;
+            handle_whatsapp_message(stream, request, provider, model, temperature, mem, auto_save, whatsapp).await;
        }

        ("POST", "/webhook") => {
@ -780,7 +770,10 @@ mod tests {
    #[test]
    fn urlencoding_decode_challenge_token() {
        // Typical Meta webhook challenge
-        assert_eq!(urlencoding_decode("1234567890"), "1234567890");
+        assert_eq!(
+            urlencoding_decode("1234567890"),
+            "1234567890"
+        );
    }

    #[test]
--- a/src/identity/aieos.rs
+++ b/src/identity/aieos.rs
@ -1056,11 +1056,7 @@ impl AieosEntity {
                    }
                }
                if let Some(slang) = style.slang_usage {
-                    let _ = writeln!(
-                        prompt,
-                        "- **Slang:** {}",
-                        if slang { "yes" } else { "no" }
-                    );
+                    let _ = writeln!(prompt, "- **Slang:** {}", if slang { "yes" } else { "no" });
                }
                if !style.style_descriptors.is_empty() {
                    let _ = writeln!(
--- a/src/identity/mod.rs
+++ b/src/identity/mod.rs
@ -6,4 +6,4 @@

 pub mod aieos;

-pub use aieos::{AieosEntity, AieosIdentity, load_aieos_identity};
+pub use aieos::{AieosEntity, AieosIdentity};
--- a/src/main.rs
+++ b/src/main.rs
@ -22,6 +22,7 @@ mod doctor;
 mod gateway;
 mod health;
 mod heartbeat;
+mod identity;
 mod integrations;
 mod memory;
 mod migration;
@ -35,6 +36,7 @@ mod skills;
 mod tools;
 mod tunnel;

+
 use config::Config;

 /// `ZeroClaw` - Zero overhead. Zero compromise. 100% Rust.
--- a/src/onboard/wizard.rs
+++ b/src/onboard/wizard.rs
@ -101,6 +101,7 @@ pub fn run_wizard() -> Result<Config> {
        composio: composio_config,
        secrets: secrets_config,
        browser: BrowserConfig::default(),
+        identity: crate::config::IdentityConfig::default(),
    };

    println!(
@ -253,6 +254,7 @@ pub fn run_quick_setup(api_key: Option<&str>, provider: Option<&str>) -> Result<
        composio: ComposioConfig::default(),
        secrets: SecretsConfig::default(),
        browser: BrowserConfig::default(),
+        identity: crate::config::IdentityConfig::default(),
    };

    config.save()?;
--- a/src/security/secrets.rs
+++ b/src/security/secrets.rs
@ -79,45 +79,94 @@ impl SecretStore {
    /// - `enc2:` prefix → ChaCha20-Poly1305 (current format)
    /// - `enc:` prefix → legacy XOR cipher (backward compatibility for migration)
    /// - No prefix → returned as-is (plaintext config)
+    ///
+    /// **Warning**: Legacy `enc:` values are insecure. Use `decrypt_and_migrate` to
+    /// automatically upgrade them to the secure `enc2:` format.
    pub fn decrypt(&self, value: &str) -> Result<String> {
        if let Some(hex_str) = value.strip_prefix("enc2:") {
-            let blob =
-                hex_decode(hex_str).context("Failed to decode encrypted secret (corrupt hex)")?;
-            anyhow::ensure!(
-                blob.len() > NONCE_LEN,
-                "Encrypted value too short (missing nonce)"
-            );
-
-            let (nonce_bytes, ciphertext) = blob.split_at(NONCE_LEN);
-            let nonce = Nonce::from_slice(nonce_bytes);
-            let key_bytes = self.load_or_create_key()?;
-            let key = Key::from_slice(&key_bytes);
-            let cipher = ChaCha20Poly1305::new(key);
-
-            let plaintext_bytes = cipher
-                .decrypt(nonce, ciphertext)
-                .map_err(|_| anyhow::anyhow!("Decryption failed — wrong key or tampered data"))?;
-
-            String::from_utf8(plaintext_bytes)
-                .context("Decrypted secret is not valid UTF-8 — corrupt data")
+            self.decrypt_chacha20(hex_str)
        } else if let Some(hex_str) = value.strip_prefix("enc:") {
-            // Legacy XOR cipher — decrypt for backward compatibility
-            let ciphertext = hex_decode(hex_str)
-                .context("Failed to decode legacy encrypted secret (corrupt hex)")?;
-            let key = self.load_or_create_key()?;
-            let plaintext_bytes = xor_cipher(&ciphertext, &key);
-            String::from_utf8(plaintext_bytes)
-                .context("Decrypted legacy secret is not valid UTF-8 — wrong key or corrupt data")
+            self.decrypt_legacy_xor(hex_str)
        } else {
            Ok(value.to_string())
        }
    }

+    /// Decrypt a secret and return a migrated `enc2:` value if the input used legacy `enc:` format.
+    ///
+    /// Returns `(plaintext, Some(new_enc2_value))` if migration occurred, or
+    /// `(plaintext, None)` if no migration was needed.
+    ///
+    /// This allows callers to persist the upgraded value back to config.
+    pub fn decrypt_and_migrate(&self, value: &str) -> Result<(String, Option<String>)> {
+        if let Some(hex_str) = value.strip_prefix("enc2:") {
+            // Already using secure format — no migration needed
+            let plaintext = self.decrypt_chacha20(hex_str)?;
+            Ok((plaintext, None))
+        } else if let Some(hex_str) = value.strip_prefix("enc:") {
+            // Legacy XOR cipher — decrypt and re-encrypt with ChaCha20-Poly1305
+            tracing::warn!(
+                "Decrypting legacy XOR-encrypted secret (enc: prefix). \
+                 This format is insecure and will be removed in a future release. \
+                 The secret will be automatically migrated to enc2: (ChaCha20-Poly1305)."
+            );
+            let plaintext = self.decrypt_legacy_xor(hex_str)?;
+            let migrated = self.encrypt(&plaintext)?;
+            Ok((plaintext, Some(migrated)))
+        } else {
+            // Plaintext — no migration needed
+            Ok((value.to_string(), None))
+        }
+    }
+
+    /// Check if a value uses the legacy `enc:` format that should be migrated.
+    pub fn needs_migration(value: &str) -> bool {
+        value.starts_with("enc:")
+    }
+
+    /// Decrypt using ChaCha20-Poly1305 (current secure format).
+    fn decrypt_chacha20(&self, hex_str: &str) -> Result<String> {
+        let blob =
+            hex_decode(hex_str).context("Failed to decode encrypted secret (corrupt hex)")?;
+        anyhow::ensure!(
+            blob.len() > NONCE_LEN,
+            "Encrypted value too short (missing nonce)"
+        );
+
+        let (nonce_bytes, ciphertext) = blob.split_at(NONCE_LEN);
+        let nonce = Nonce::from_slice(nonce_bytes);
+        let key_bytes = self.load_or_create_key()?;
+        let key = Key::from_slice(&key_bytes);
+        let cipher = ChaCha20Poly1305::new(key);
+
+        let plaintext_bytes = cipher
+            .decrypt(nonce, ciphertext)
+            .map_err(|_| anyhow::anyhow!("Decryption failed — wrong key or tampered data"))?;
+
+        String::from_utf8(plaintext_bytes)
+            .context("Decrypted secret is not valid UTF-8 — corrupt data")
+    }
+
+    /// Decrypt using legacy XOR cipher (insecure, for backward compatibility only).
+    fn decrypt_legacy_xor(&self, hex_str: &str) -> Result<String> {
+        let ciphertext = hex_decode(hex_str)
+            .context("Failed to decode legacy encrypted secret (corrupt hex)")?;
+        let key = self.load_or_create_key()?;
+        let plaintext_bytes = xor_cipher(&ciphertext, &key);
+        String::from_utf8(plaintext_bytes)
+            .context("Decrypted legacy secret is not valid UTF-8 — wrong key or corrupt data")
+    }
+
    /// Check if a value is already encrypted (current or legacy format).
    pub fn is_encrypted(value: &str) -> bool {
        value.starts_with("enc2:") || value.starts_with("enc:")
    }

+    /// Check if a value uses the secure `enc2:` format.
+    pub fn is_secure_encrypted(value: &str) -> bool {
+        value.starts_with("enc2:")
+    }
+
    /// Load the encryption key from disk, or create one if it doesn't exist.
    fn load_or_create_key(&self) -> Result<Vec<u8>> {
        if self.key_path.exists() {
@ -382,6 +431,258 @@ mod tests {
        assert_eq!(decrypted, plaintext, "Legacy XOR values must still decrypt");
    }

+    // ── Migration tests ─────────────────────────────────────────
+
+    #[test]
+    fn needs_migration_detects_legacy_prefix() {
+        assert!(SecretStore::needs_migration("enc:aabbcc"));
+        assert!(!SecretStore::needs_migration("enc2:aabbcc"));
+        assert!(!SecretStore::needs_migration("sk-plaintext"));
+        assert!(!SecretStore::needs_migration(""));
+    }
+
+    #[test]
+    fn is_secure_encrypted_detects_enc2_only() {
+        assert!(SecretStore::is_secure_encrypted("enc2:aabbcc"));
+        assert!(!SecretStore::is_secure_encrypted("enc:aabbcc"));
+        assert!(!SecretStore::is_secure_encrypted("sk-plaintext"));
+        assert!(!SecretStore::is_secure_encrypted(""));
+    }
+
+    #[test]
+    fn decrypt_and_migrate_returns_none_for_enc2() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let encrypted = store.encrypt("my-secret").unwrap();
+        assert!(encrypted.starts_with("enc2:"));
+
+        let (plaintext, migrated) = store.decrypt_and_migrate(&encrypted).unwrap();
+        assert_eq!(plaintext, "my-secret");
+        assert!(
+            migrated.is_none(),
+            "enc2: values should not trigger migration"
+        );
+    }
+
+    #[test]
+    fn decrypt_and_migrate_returns_none_for_plaintext() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let (plaintext, migrated) = store.decrypt_and_migrate("sk-plaintext-key").unwrap();
+        assert_eq!(plaintext, "sk-plaintext-key");
+        assert!(
+            migrated.is_none(),
+            "Plaintext values should not trigger migration"
+        );
+    }
+
+    #[test]
+    fn decrypt_and_migrate_upgrades_legacy_xor() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        // Create key first
+        let _ = store.encrypt("setup").unwrap();
+        let key = store.load_or_create_key().unwrap();
+
+        // Manually create a legacy XOR-encrypted value
+        let plaintext = "sk-legacy-secret-to-migrate";
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        // Verify it needs migration
+        assert!(SecretStore::needs_migration(&legacy_value));
+
+        // Decrypt and migrate
+        let (decrypted, migrated) = store.decrypt_and_migrate(&legacy_value).unwrap();
+        assert_eq!(decrypted, plaintext, "Plaintext must match original");
+        assert!(migrated.is_some(), "Legacy value should trigger migration");
+
+        let new_value = migrated.unwrap();
+        assert!(
+            new_value.starts_with("enc2:"),
+            "Migrated value must use enc2: prefix"
+        );
+        assert!(
+            !SecretStore::needs_migration(&new_value),
+            "Migrated value should not need migration"
+        );
+
+        // Verify the migrated value decrypts correctly
+        let (decrypted2, migrated2) = store.decrypt_and_migrate(&new_value).unwrap();
+        assert_eq!(
+            decrypted2, plaintext,
+            "Migrated value must decrypt to same plaintext"
+        );
+        assert!(
+            migrated2.is_none(),
+            "Migrated value should not trigger another migration"
+        );
+    }
+
+    #[test]
+    fn decrypt_and_migrate_handles_unicode() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let _ = store.encrypt("setup").unwrap();
+        let key = store.load_or_create_key().unwrap();
+
+        let plaintext = "sk-日本語-émojis-🦀-тест";
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        let (decrypted, migrated) = store.decrypt_and_migrate(&legacy_value).unwrap();
+        assert_eq!(decrypted, plaintext);
+        assert!(migrated.is_some());
+
+        // Verify migrated value works
+        let new_value = migrated.unwrap();
+        let (decrypted2, _) = store.decrypt_and_migrate(&new_value).unwrap();
+        assert_eq!(decrypted2, plaintext);
+    }
+
+    #[test]
+    fn decrypt_and_migrate_handles_empty_secret() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let _ = store.encrypt("setup").unwrap();
+        let key = store.load_or_create_key().unwrap();
+
+        // Empty plaintext XOR-encrypted
+        let plaintext = "";
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        let (decrypted, migrated) = store.decrypt_and_migrate(&legacy_value).unwrap();
+        assert_eq!(decrypted, plaintext);
+        // Empty string encryption returns empty string (not enc2:)
+        assert!(migrated.is_some());
+        assert_eq!(migrated.unwrap(), "");
+    }
+
+    #[test]
+    fn decrypt_and_migrate_handles_long_secret() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let _ = store.encrypt("setup").unwrap();
+        let key = store.load_or_create_key().unwrap();
+
+        let plaintext = "a".repeat(10_000);
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        let (decrypted, migrated) = store.decrypt_and_migrate(&legacy_value).unwrap();
+        assert_eq!(decrypted, plaintext);
+        assert!(migrated.is_some());
+
+        let new_value = migrated.unwrap();
+        let (decrypted2, _) = store.decrypt_and_migrate(&new_value).unwrap();
+        assert_eq!(decrypted2, plaintext);
+    }
+
+    #[test]
+    fn decrypt_and_migrate_fails_on_corrupt_legacy_hex() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+        let _ = store.encrypt("setup").unwrap();
+
+        let result = store.decrypt_and_migrate("enc:not-valid-hex!!");
+        assert!(result.is_err(), "Corrupt hex should fail");
+    }
+
+    #[test]
+    fn decrypt_and_migrate_wrong_key_produces_garbage_or_fails() {
+        let tmp1 = TempDir::new().unwrap();
+        let tmp2 = TempDir::new().unwrap();
+        let store1 = SecretStore::new(tmp1.path(), true);
+        let store2 = SecretStore::new(tmp2.path(), true);
+
+        // Create keys for both stores
+        let _ = store1.encrypt("setup").unwrap();
+        let _ = store2.encrypt("setup").unwrap();
+        let key1 = store1.load_or_create_key().unwrap();
+
+        // Encrypt with store1's key
+        let plaintext = "secret-for-store1";
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key1);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        // Decrypt with store2 — XOR will produce garbage bytes
+        // This may fail with UTF-8 error or succeed with garbage plaintext
+        match store2.decrypt_and_migrate(&legacy_value) {
+            Ok((decrypted, _)) => {
+                // If it succeeds, the plaintext should be garbage (not the original)
+                assert_ne!(
+                    decrypted, plaintext,
+                    "Wrong key should produce garbage plaintext"
+                );
+            }
+            Err(e) => {
+                // Expected: UTF-8 decoding failure from garbage bytes
+                assert!(
+                    e.to_string().contains("UTF-8"),
+                    "Error should be UTF-8 related: {e}"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn migration_produces_different_ciphertext_each_time() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let _ = store.encrypt("setup").unwrap();
+        let key = store.load_or_create_key().unwrap();
+
+        let plaintext = "sk-same-secret";
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        let (_, migrated1) = store.decrypt_and_migrate(&legacy_value).unwrap();
+        let (_, migrated2) = store.decrypt_and_migrate(&legacy_value).unwrap();
+
+        assert!(migrated1.is_some());
+        assert!(migrated2.is_some());
+        assert_ne!(
+            migrated1.unwrap(),
+            migrated2.unwrap(),
+            "Each migration should produce different ciphertext (random nonce)"
+        );
+    }
+
+    #[test]
+    fn migrated_value_is_tamper_resistant() {
+        let tmp = TempDir::new().unwrap();
+        let store = SecretStore::new(tmp.path(), true);
+
+        let _ = store.encrypt("setup").unwrap();
+        let key = store.load_or_create_key().unwrap();
+
+        let plaintext = "sk-sensitive-data";
+        let ciphertext = xor_cipher(plaintext.as_bytes(), &key);
+        let legacy_value = format!("enc:{}", hex_encode(&ciphertext));
+
+        let (_, migrated) = store.decrypt_and_migrate(&legacy_value).unwrap();
+        let new_value = migrated.unwrap();
+
+        // Tamper with the migrated value
+        let hex_str = &new_value[5..];
+        let mut blob = hex_decode(hex_str).unwrap();
+        if blob.len() > NONCE_LEN {
+            blob[NONCE_LEN] ^= 0xff;
+        }
+        let tampered = format!("enc2:{}", hex_encode(&blob));
+
+        let result = store.decrypt_and_migrate(&tampered);
+        assert!(result.is_err(), "Tampered migrated value must be rejected");
+    }
+
    // ── Low-level helpers ───────────────────────────────────────

    #[test]
--- a/src/skills/mod.rs
+++ b/src/skills/mod.rs
@ -631,3 +631,6 @@ description = "Bare minimum"
        assert_eq!(skills[0].name, "from-toml"); // TOML takes priority
    }
 }
+
+#[cfg(test)]
+mod symlink_tests;
--- a/src/skills/symlink_tests.rs
+++ b/src/skills/symlink_tests.rs
@ -0,0 +1,103 @@
+#[cfg(test)]
+mod symlink_tests {
+    use tempfile::TempDir;
+    use std::path::Path;
+    use crate::skills::skills_dir;
+
+    #[test]
+    fn test_skills_symlink_unix_edge_cases() {
+        let tmp = TempDir::new().unwrap();
+        let workspace_dir = tmp.path().join("workspace");
+        std::fs::create_dir_all(&workspace_dir).unwrap();
+        
+        let skills_path = skills_dir(&workspace_dir);
+        std::fs::create_dir_all(&skills_path).unwrap();
+        
+        // Test case 1: Valid symlink creation on Unix
+        #[cfg(unix)]
+        {
+            let source_dir = tmp.path().join("source_skill");
+            std::fs::create_dir_all(&source_dir).unwrap();
+            std::fs::write(source_dir.join("SKILL.md"), "# Test Skill\nContent").unwrap();
+            
+            let dest_link = skills_path.join("linked_skill");
+            
+            // Create symlink
+            let result = std::os::unix::fs::symlink(&source_dir, &dest_link);
+            assert!(result.is_ok(), "Symlink creation should succeed");
+            
+            // Verify symlink works
+            assert!(dest_link.exists());
+            assert!(dest_link.is_symlink());
+            
+            // Verify we can read through symlink
+            let content = std::fs::read_to_string(dest_link.join("SKILL.md"));
+            assert!(content.is_ok());
+            assert!(content.unwrap().contains("Test Skill"));
+            
+            // Test case 2: Symlink to non-existent target should fail gracefully
+            let broken_link = skills_path.join("broken_skill");
+            let non_existent = tmp.path().join("non_existent");
+            let result = std::os::unix::fs::symlink(&non_existent, &broken_link);
+            assert!(result.is_ok(), "Symlink creation should succeed even if target doesn't exist");
+            
+            // But reading through it should fail
+            let content = std::fs::read_to_string(broken_link.join("SKILL.md"));
+            assert!(content.is_err());
+        }
+        
+        // Test case 3: Non-Unix platforms should handle symlink errors gracefully
+        #[cfg(not(unix))]
+        {
+            let source_dir = tmp.path().join("source_skill");
+            std::fs::create_dir_all(&source_dir).unwrap();
+            
+            let dest_link = skills_path.join("linked_skill");
+            
+            // Symlink should fail on non-Unix
+            let result = std::os::unix::fs::symlink(&source_dir, &dest_link);
+            assert!(result.is_err());
+            
+            // Directory should not exist
+            assert!(!dest_link.exists());
+        }
+        
+        // Test case 4: skills_dir function edge cases
+        let workspace_with_trailing_slash = format!("{}/", workspace_dir.display());
+        let path_from_str = skills_dir(Path::new(&workspace_with_trailing_slash));
+        assert_eq!(path_from_str, skills_path);
+        
+        // Test case 5: Empty workspace directory
+        let empty_workspace = tmp.path().join("empty");
+        let empty_skills_path = skills_dir(&empty_workspace);
+        assert_eq!(empty_skills_path, empty_workspace.join("skills"));
+        assert!(!empty_skills_path.exists());
+    }
+    
+    #[test]
+    fn test_skills_symlink_permissions_and_safety() {
+        let tmp = TempDir::new().unwrap();
+        let workspace_dir = tmp.path().join("workspace");
+        std::fs::create_dir_all(&workspace_dir).unwrap();
+        
+        let skills_path = skills_dir(&workspace_dir);
+        std::fs::create_dir_all(&skills_path).unwrap();
+        
+        #[cfg(unix)]
+        {
+            // Test case: Symlink outside workspace should be allowed (user responsibility)
+            let outside_dir = tmp.path().join("outside_skill");
+            std::fs::create_dir_all(&outside_dir).unwrap();
+            std::fs::write(outside_dir.join("SKILL.md"), "# Outside Skill\nContent").unwrap();
+            
+            let dest_link = skills_path.join("outside_skill");
+            let result = std::os::unix::fs::symlink(&outside_dir, &dest_link);
+            assert!(result.is_ok(), "Should allow symlinking to directories outside workspace");
+            
+            // Should still be readable
+            let content = std::fs::read_to_string(dest_link.join("SKILL.md"));
+            assert!(content.is_ok());
+            assert!(content.unwrap().contains("Outside Skill"));
+        }
+    }
+}