Enhance path validation to prevent symlink escape vulnerabilities (#2051)

Co-authored-by: 李冠辰 <liguanchen@xiaomi.com>
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>

src/openhuman/security/policy.rs

@@ -774,9 +774,92 @@ impl SecurityPolicy {
             }
         }
 
+        // Symlink-safe check (#1927). The string-level checks above can be
+        // bypassed by creating a symlink inside the workspace that points to
+        // a forbidden tree (e.g. `evil -> /etc/shadow`). Canonicalize the
+        // path and re-validate `workspace_only` containment + forbidden_paths
+        // against the resolved location.
+        if let Some(canonical) = self.try_canonicalize_under_workspace(path) {
+            let workspace_root = self
+                .workspace_dir
+                .canonicalize()
+                .unwrap_or_else(|_| self.workspace_dir.clone());
+            if self.workspace_only && !canonical.starts_with(&workspace_root) {
+                log::trace!(
+                    "[security:policy] path blocked: symlink escapes workspace (requested={}, resolved={}, workspace={})",
+                    path,
+                    canonical.display(),
+                    workspace_root.display()
+                );
+                return false;
+            }
+            // If the resolved path stays inside the workspace, trust the
+            // workspace boundary over forbidden_paths — otherwise a workspace
+            // that lives under e.g. `/tmp` (common in tests and sandboxes)
+            // would block every legitimate access. forbidden_paths is meant
+            // to catch escapes *outside* the workspace, which the workspace
+            // containment check above already validates.
+            let inside_workspace = canonical.starts_with(&workspace_root);
+            if !inside_workspace {
+                for forbidden in &self.forbidden_paths {
+                    let forbidden_expanded = if let Some(stripped) = forbidden.strip_prefix("~/") {
+                        std::env::var("HOME")
+                            .ok()
+                            .map(|h| PathBuf::from(h).join(stripped))
+                            .unwrap_or_else(|| PathBuf::from(forbidden))
+                    } else {
+                        PathBuf::from(forbidden)
+                    };
+                    let forbidden_canonical = forbidden_expanded
+                        .canonicalize()
+                        .unwrap_or(forbidden_expanded);
+                    if canonical.starts_with(&forbidden_canonical) {
+                        log::trace!(
+                        "[security:policy] path blocked: symlink resolves to forbidden tree (requested={}, resolved={}, forbidden={})",
+                        path,
+                        canonical.display(),
+                        forbidden_canonical.display()
+                    );
+                        return false;
+                    }
+                }
+            }
+        }
+
         true
     }
 
+    /// Resolve a user-supplied path under the workspace, canonicalizing it
+    /// (or its parent) when present on disk. Used by [`Self::is_path_allowed`]
+    /// to defend against symlink-based escapes that pass the string-level
+    /// checks. Returns `None` only when neither the path nor its parent can
+    /// be resolved on disk — in that case the caller falls back to the
+    /// string-level checks alone (which is the safe default for fresh paths
+    /// whose entire chain does not yet exist).
+    fn try_canonicalize_under_workspace(&self, path: &str) -> Option<PathBuf> {
+        let expanded = if let Some(stripped) = path.strip_prefix("~/") {
+            std::env::var("HOME")
+                .ok()
+                .map(|h| PathBuf::from(h).join(stripped))?
+        } else {
+            PathBuf::from(path)
+        };
+        let absolute = if expanded.is_absolute() {
+            expanded
+        } else {
+            self.workspace_dir.join(&expanded)
+        };
+        if let Ok(canonical) = absolute.canonicalize() {
+            return Some(canonical);
+        }
+        // Path itself does not exist (e.g. a write-to-new-file call). Try
+        // canonicalizing the parent + appending the basename so we still
+        // catch parent chains that resolve via symlink to a forbidden tree.
+        let parent = absolute.parent()?;
+        let name = absolute.file_name()?;
+        parent.canonicalize().ok().map(|p| p.join(name))
+    }
+
     /// Validate that a resolved path is still inside the workspace.
     /// Call this AFTER joining `workspace_dir` + relative path and canonicalizing.
     pub fn is_resolved_path_allowed(&self, resolved: &Path) -> bool {

src/openhuman/security/policy_tests.rs

@@ -393,6 +393,91 @@ fn dotfile_in_workspace_allowed() {
     assert!(p.is_path_allowed(".env"));
 }
 
+// -- is_path_allowed — symlink safety (#1927) ---------------------
+
+#[cfg(unix)]
+#[test]
+fn symlink_inside_workspace_escaping_outside_is_blocked() {
+    use std::os::unix::fs::symlink;
+
+    let workspace = tempfile::tempdir().expect("workspace tempdir");
+    let outside = tempfile::tempdir().expect("outside tempdir");
+    let target = outside.path().join("secret.txt");
+    std::fs::write(&target, "secret").expect("write secret");
+
+    let link = workspace.path().join("evil");
+    symlink(&target, &link).expect("create symlink");
+
+    let p = SecurityPolicy {
+        workspace_dir: workspace.path().to_path_buf(),
+        workspace_only: true,
+        forbidden_paths: vec![],
+        ..SecurityPolicy::default()
+    };
+
+    // String-level checks pass: "evil" has no "..", isn't absolute, and is
+    // not in forbidden_paths. The canonicalize step must catch the symlink
+    // pointing outside the workspace root.
+    assert!(
+        !p.is_path_allowed("evil"),
+        "symlink that escapes the workspace must be blocked"
+    );
+}
+
+#[cfg(unix)]
+#[test]
+fn symlink_to_forbidden_tree_is_blocked() {
+    use std::os::unix::fs::symlink;
+
+    let workspace = tempfile::tempdir().expect("workspace tempdir");
+    let forbidden = tempfile::tempdir().expect("forbidden tempdir");
+    let target = forbidden.path().join("secret");
+    std::fs::write(&target, "x").expect("write secret");
+
+    let link = workspace.path().join("link-to-forbidden");
+    symlink(&target, &link).expect("create symlink");
+
+    let p = SecurityPolicy {
+        workspace_dir: workspace.path().to_path_buf(),
+        // Disable workspace_only so the assertion isolates the forbidden_paths
+        // path (the symlink escapes the workspace, which would also trip
+        // workspace_only — but here we want to prove the forbidden_paths
+        // check itself canonicalizes).
+        workspace_only: false,
+        forbidden_paths: vec![forbidden.path().to_string_lossy().to_string()],
+        ..SecurityPolicy::default()
+    };
+
+    // The string "link-to-forbidden" does not start with the forbidden
+    // tempdir path, so the string-level check passes. Canonical resolution
+    // must catch that it resolves into the forbidden tree.
+    assert!(
+        !p.is_path_allowed("link-to-forbidden"),
+        "symlink that resolves into a forbidden tree must be blocked"
+    );
+}
+
+#[test]
+fn write_to_not_yet_existing_path_in_workspace_still_allowed() {
+    // After adding the symlink-safe canonicalize step, writing to a
+    // not-yet-existing path inside the workspace must still pass — the
+    // parent-dir fallback canonicalizes the parent and confirms it is
+    // inside the workspace root.
+    let workspace = tempfile::tempdir().expect("workspace tempdir");
+    let p = SecurityPolicy {
+        workspace_dir: workspace.path().to_path_buf(),
+        workspace_only: true,
+        forbidden_paths: vec![],
+        ..SecurityPolicy::default()
+    };
+
+    assert!(p.is_path_allowed("new-file.txt"));
+    // Whole parent chain missing too — helper returns None, and we fall
+    // back to the string-level checks (which would pass for a
+    // workspace-relative non-traversal path).
+    assert!(p.is_path_allowed("not-yet-existing/subdir/file.txt"));
+}
+
 // -- from_config --------------------------------------------------
 
 #[test]

src/openhuman/tools/impl/filesystem/file_read.rs

@@ -323,7 +323,16 @@ mod tests {
         let result = tool.execute(json!({"path": "escape.txt"})).await.unwrap();
 
         assert!(result.is_error);
-        assert!(result.output().contains("escapes workspace"));
+        // After the symlink-safe canonical check landed in
+        // SecurityPolicy::is_path_allowed (#1927), the policy layer blocks
+        // the escape before file_read's own resolved-path check runs — the
+        // error becomes "Path not allowed by security policy". Either
+        // message signals defense-in-depth worked.
+        let out = result.output();
+        assert!(
+            out.contains("escapes workspace") || out.contains("not allowed"),
+            "expected escape/not-allowed error, got: {out}"
+        );
 
         let _ = tokio::fs::remove_dir_all(&root).await;
     }

src/openhuman/tools/impl/filesystem/file_write.rs

@@ -319,7 +319,14 @@ mod tests {
             .unwrap();
 
         assert!(result.is_error);
-        assert!(result.output().contains("escapes workspace"));
+        // SecurityPolicy now blocks symlink escapes at the is_path_allowed
+        // layer (#1927) — error becomes "Path not allowed by security
+        // policy" rather than the deeper "escapes workspace" message.
+        let out = result.output();
+        assert!(
+            out.contains("escapes workspace") || out.contains("not allowed"),
+            "expected escape/not-allowed error, got: {out}"
+        );
         assert!(!outside.join("hijack.txt").exists());
 
         let _ = tokio::fs::remove_dir_all(&root).await;
@@ -395,9 +402,13 @@ mod tests {
             .unwrap();
 
         assert!(result.is_error, "writing through symlink must be blocked");
+        // The symlink-safe is_path_allowed check (#1927) blocks at the
+        // policy layer before the tool's own symlink-target detection
+        // runs; accept either error message.
+        let out = result.output();
         assert!(
-            result.output().contains("symlink"),
-            "error should mention symlink"
+            out.contains("symlink") || out.contains("not allowed"),
+            "error should mention symlink or policy block, got: {out}"
         );
 
         // Verify original file was not modified