From 8decfd327ea7bcef2d60598e8b8428b63908acc5 Mon Sep 17 00:00:00 2001 From: AochenShen99 Date: Thu, 28 May 2026 15:48:32 +0800 Subject: [PATCH] Fix custom skill install permissions (#3241) * Fix custom skill install permissions * Fix skill upload test portability * Keep custom skill writes sandbox readable * Clear sandbox write bits on skill permissions * Limit custom skill write permission updates --- .../harness/deerflow/skills/installer.py | 2 + .../harness/deerflow/skills/permissions.py | 34 +++++++++ .../skills/storage/local_skill_storage.py | 2 + .../tests/test_local_skill_storage_write.py | 15 ++++ backend/tests/test_skill_permissions.py | 63 ++++++++++++++++ backend/tests/test_skills_custom_router.py | 74 +++++++++++++++++++ backend/tests/test_skills_installer.py | 20 +++++ 7 files changed, 210 insertions(+) create mode 100644 backend/packages/harness/deerflow/skills/permissions.py create mode 100644 backend/tests/test_skill_permissions.py diff --git a/backend/packages/harness/deerflow/skills/installer.py b/backend/packages/harness/deerflow/skills/installer.py index 536ba0f88..fe78ca633 100644 --- a/backend/packages/harness/deerflow/skills/installer.py +++ b/backend/packages/harness/deerflow/skills/installer.py @@ -13,6 +13,7 @@ import stat import zipfile from pathlib import Path, PurePosixPath, PureWindowsPath +from deerflow.skills.permissions import make_skill_tree_sandbox_readable from deerflow.skills.security_scanner import scan_skill_content logger = logging.getLogger(__name__) @@ -139,6 +140,7 @@ def _move_staged_skill_into_reserved_target(staging_target: Path, target: Path) reserved = True for child in staging_target.iterdir(): shutil.move(str(child), target / child.name) + make_skill_tree_sandbox_readable(target) installed = True except FileExistsError as e: raise SkillAlreadyExistsError(f"Skill '{target.name}' already exists") from e diff --git a/backend/packages/harness/deerflow/skills/permissions.py b/backend/packages/harness/deerflow/skills/permissions.py new file mode 100644 index 000000000..b060598b6 --- /dev/null +++ b/backend/packages/harness/deerflow/skills/permissions.py @@ -0,0 +1,34 @@ +"""Filesystem permission helpers for installed skill trees.""" + +import stat +from pathlib import Path + + +def make_skill_path_sandbox_readable(path: Path) -> None: + if path.is_symlink(): + return + mode = stat.S_IMODE(path.stat().st_mode) + without_sandbox_write = mode & ~(stat.S_IWGRP | stat.S_IWOTH) + if path.is_dir(): + path.chmod(without_sandbox_write | 0o555) + elif path.is_file(): + path.chmod(without_sandbox_write | 0o444) + + +def make_skill_tree_sandbox_readable(target: Path) -> None: + make_skill_path_sandbox_readable(target) + for path in target.rglob("*"): + make_skill_path_sandbox_readable(path) + + +def make_skill_written_path_sandbox_readable(skill_root: Path, target: Path) -> None: + resolved_root = skill_root.resolve() + resolved_target = target.resolve() + resolved_target.relative_to(resolved_root) + + make_skill_path_sandbox_readable(resolved_root) + current = resolved_root + for part in resolved_target.parent.relative_to(resolved_root).parts: + current = current / part + make_skill_path_sandbox_readable(current) + make_skill_path_sandbox_readable(resolved_target) diff --git a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py index 4b7dffde4..69114d51f 100644 --- a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py @@ -13,6 +13,7 @@ from datetime import UTC, datetime from pathlib import Path from deerflow.config.runtime_paths import resolve_path +from deerflow.skills.permissions import make_skill_written_path_sandbox_readable from deerflow.skills.storage.skill_storage import SKILL_MD_FILE, SkillStorage from deerflow.skills.types import SkillCategory @@ -90,6 +91,7 @@ class LocalSkillStorage(SkillStorage): tmp_file.write(content) tmp_path = Path(tmp_file.name) tmp_path.replace(target) + make_skill_written_path_sandbox_readable(self.get_custom_skill_dir(name), target) async def ainstall_skill_from_archive(self, archive_path: str | Path) -> dict: import zipfile diff --git a/backend/tests/test_local_skill_storage_write.py b/backend/tests/test_local_skill_storage_write.py index ce68c6e88..3abd4a63c 100644 --- a/backend/tests/test_local_skill_storage_write.py +++ b/backend/tests/test_local_skill_storage_write.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import stat import pytest @@ -43,6 +44,20 @@ def test_write_is_atomic_overwrite(tmp_path, storage): assert (tmp_path / "custom" / "demo-skill" / "SKILL.md").read_text() == "second" +def test_write_makes_written_path_sandbox_readable(tmp_path, storage): + skill_dir = tmp_path / "custom" / "demo-skill" + skill_dir.mkdir(parents=True) + skill_dir.chmod(0o700) + + storage.write_custom_skill("demo-skill", "references/ref.md", "# ref") + + ref_dir = skill_dir / "references" + ref_file = ref_dir / "ref.md" + assert stat.S_IMODE(skill_dir.stat().st_mode) & 0o055 == 0o055 + assert stat.S_IMODE(ref_dir.stat().st_mode) & 0o055 == 0o055 + assert stat.S_IMODE(ref_file.stat().st_mode) & 0o044 == 0o044 + + # --------------------------------------------------------------------------- # Empty / blank path # --------------------------------------------------------------------------- diff --git a/backend/tests/test_skill_permissions.py b/backend/tests/test_skill_permissions.py new file mode 100644 index 000000000..dc32c9c4c --- /dev/null +++ b/backend/tests/test_skill_permissions.py @@ -0,0 +1,63 @@ +import stat + +from deerflow.skills.permissions import make_skill_tree_sandbox_readable, make_skill_written_path_sandbox_readable + + +def _mode(path): + return stat.S_IMODE(path.stat().st_mode) + + +def test_skill_tree_readability_includes_hidden_paths_and_removes_sandbox_write(tmp_path): + root = tmp_path / "demo-skill" + hidden_dir = root / ".hidden" + scripts_dir = root / "scripts" + hidden_dir.mkdir(parents=True) + scripts_dir.mkdir() + env_file = root / ".env" + hidden_file = hidden_dir / ".secret" + script_file = scripts_dir / "run.sh" + env_file.write_text("secret", encoding="utf-8") + hidden_file.write_text("secret", encoding="utf-8") + script_file.write_text("#!/bin/sh\n", encoding="utf-8") + + root.chmod(0o777) + hidden_dir.chmod(0o777) + scripts_dir.chmod(0o777) + env_file.chmod(0o666) + hidden_file.chmod(0o600) + script_file.chmod(0o777) + + make_skill_tree_sandbox_readable(root) + + assert _mode(root) == 0o755 + assert _mode(hidden_dir) == 0o755 + assert _mode(scripts_dir) == 0o755 + assert _mode(env_file) == 0o644 + assert _mode(hidden_file) == 0o644 + assert _mode(script_file) == 0o755 + + +def test_written_path_readability_is_limited_to_written_path(tmp_path): + root = tmp_path / "demo-skill" + ref_dir = root / "references" + sibling_dir = root / "templates" + ref_dir.mkdir(parents=True) + sibling_dir.mkdir() + target = ref_dir / "guide.md" + sibling = sibling_dir / "note.md" + target.write_text("guide", encoding="utf-8") + sibling.write_text("note", encoding="utf-8") + + root.chmod(0o700) + ref_dir.chmod(0o700) + target.chmod(0o600) + sibling_dir.chmod(0o700) + sibling.chmod(0o600) + + make_skill_written_path_sandbox_readable(root, target) + + assert _mode(root) == 0o755 + assert _mode(ref_dir) == 0o755 + assert _mode(target) == 0o644 + assert _mode(sibling_dir) == 0o700 + assert _mode(sibling) == 0o600 diff --git a/backend/tests/test_skills_custom_router.py b/backend/tests/test_skills_custom_router.py index e8a86d8ab..b6f6bb304 100644 --- a/backend/tests/test_skills_custom_router.py +++ b/backend/tests/test_skills_custom_router.py @@ -1,14 +1,18 @@ import errno import json +import stat import zipfile +from io import BytesIO from pathlib import Path from types import SimpleNamespace +from _router_auth_helpers import make_authed_test_app from fastapi import FastAPI from fastapi.testclient import TestClient from app.gateway.deps import get_config from app.gateway.routers import skills as skills_router +from app.gateway.routers import uploads as uploads_router from deerflow.skills.storage import get_or_new_skill_storage from deerflow.skills.types import Skill @@ -53,6 +57,15 @@ def _make_skill_archive(tmp_path: Path, name: str, content: str | None = None) - return archive +def _make_skill_archive_bytes(name: str, content: str | None = None) -> bytes: + buffer = BytesIO() + skill_content = content or _skill_content(name) + with zipfile.ZipFile(buffer, "w") as zf: + zf.writestr(f"{name}/SKILL.md", skill_content) + zf.writestr(f"{name}/references/guide.md", "# Guide\n") + return buffer.getvalue() + + def test_install_skill_archive_runs_security_scan(monkeypatch, tmp_path): skills_root = tmp_path / "skills" (skills_root / "custom").mkdir(parents=True) @@ -101,6 +114,65 @@ def test_install_skill_archive_runs_security_scan(monkeypatch, tmp_path): assert refresh_calls == ["refresh"] +def test_uploaded_skill_archive_installs_sandbox_readable_tree(monkeypatch, tmp_path): + home = tmp_path / "home" + skills_root = tmp_path / "skills" + skills_root.mkdir() + refresh_calls = [] + + async def _scan(*args, **kwargs): + from deerflow.skills.security_scanner import ScanResult + + return ScanResult(decision="allow", reason="ok") + + async def _refresh(): + refresh_calls.append("refresh") + + config = SimpleNamespace( + skills=SimpleNamespace(get_skills_path=lambda: skills_root, container_path="/mnt/skills", use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage"), + skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None), + uploads=SimpleNamespace(auto_convert_documents=False), + ) + provider = SimpleNamespace(uses_thread_data_mounts=True) + + monkeypatch.setenv("DEER_FLOW_HOME", str(home)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + monkeypatch.setattr(uploads_router, "get_sandbox_provider", lambda: provider) + monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _scan) + monkeypatch.setattr(skills_router, "refresh_skills_system_prompt_cache_async", _refresh) + + app = make_authed_test_app() + app.state.config = config + app.dependency_overrides[get_config] = lambda: config + app.include_router(uploads_router.router) + app.include_router(skills_router.router) + + thread_id = "thread-uploaded-skill" + archive_bytes = _make_skill_archive_bytes("uploaded-skill") + + with TestClient(app) as client: + upload_response = client.post( + f"/api/threads/{thread_id}/uploads", + files=[("files", ("uploaded-skill.skill", archive_bytes, "application/octet-stream"))], + ) + assert upload_response.status_code == 200 + uploaded_file = upload_response.json()["files"][0] + uploaded_path = Path(uploaded_file["path"]) + assert uploaded_path.is_file() + + install_response = client.post("/api/skills/install", json={"thread_id": thread_id, "path": uploaded_file["virtual_path"]}) + + assert install_response.status_code == 200 + assert install_response.json()["skill_name"] == "uploaded-skill" + installed_dir = skills_root / "custom" / "uploaded-skill" + nested_dir = installed_dir / "references" + assert stat.S_IMODE(installed_dir.stat().st_mode) & 0o055 == 0o055 + assert stat.S_IMODE(nested_dir.stat().st_mode) & 0o055 == 0o055 + assert stat.S_IMODE((installed_dir / "SKILL.md").stat().st_mode) & 0o044 == 0o044 + assert stat.S_IMODE((nested_dir / "guide.md").stat().st_mode) & 0o044 == 0o044 + assert refresh_calls == ["refresh"] + + def test_install_skill_archive_security_scan_block_returns_400(monkeypatch, tmp_path): skills_root = tmp_path / "skills" (skills_root / "custom").mkdir(parents=True) @@ -175,6 +247,7 @@ def test_custom_skills_router_lifecycle(monkeypatch, tmp_path): ) assert update_response.status_code == 200 assert update_response.json()["description"] == "Edited skill" + assert stat.S_IMODE((custom_dir / "SKILL.md").stat().st_mode) & 0o044 == 0o044 history_response = client.get("/api/skills/custom/demo-skill/history") assert history_response.status_code == 200 @@ -183,6 +256,7 @@ def test_custom_skills_router_lifecycle(monkeypatch, tmp_path): rollback_response = client.post("/api/skills/custom/demo-skill/rollback", json={"history_index": -1}) assert rollback_response.status_code == 200 assert rollback_response.json()["description"] == "Demo skill" + assert stat.S_IMODE((custom_dir / "SKILL.md").stat().st_mode) & 0o044 == 0o044 assert refresh_calls == ["refresh", "refresh"] diff --git a/backend/tests/test_skills_installer.py b/backend/tests/test_skills_installer.py index 101f1b2a8..7c21bc288 100644 --- a/backend/tests/test_skills_installer.py +++ b/backend/tests/test_skills_installer.py @@ -198,6 +198,26 @@ class TestInstallSkillFromArchive: assert result["skill_name"] == "test-skill" assert (skills_root / "custom" / "test-skill" / "SKILL.md").exists() + def test_installed_skill_tree_is_readable_by_sandbox_mount(self, tmp_path): + zip_path = tmp_path / "test-skill.skill" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("test-skill/SKILL.md", "---\nname: test-skill\ndescription: A test skill\n---\n\n# test-skill\n") + zf.writestr("test-skill/references/guide.md", "# Guide\n") + skills_root = tmp_path / "skills" + skills_root.mkdir() + + get_or_new_skill_storage(skills_path=skills_root).install_skill_from_archive(zip_path) + + installed_dir = skills_root / "custom" / "test-skill" + nested_dir = installed_dir / "references" + skill_file = installed_dir / "SKILL.md" + guide_file = nested_dir / "guide.md" + + assert stat.S_IMODE(installed_dir.stat().st_mode) & 0o055 == 0o055 + assert stat.S_IMODE(nested_dir.stat().st_mode) & 0o055 == 0o055 + assert stat.S_IMODE(skill_file.stat().st_mode) & 0o044 == 0o044 + assert stat.S_IMODE(guide_file.stat().st_mode) & 0o044 == 0o044 + def test_scans_skill_markdown_before_install(self, tmp_path, monkeypatch): zip_path = self._make_skill_zip(tmp_path) skills_root = tmp_path / "skills"