diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index ba0bca9ca..af08f5402 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -173,8 +173,10 @@ def _extract_skill_name_from_skills_path(path: str) -> str | None: if not relative: return None # Expected patterns: "public//...", "custom//...", "legacy//..." - # or "/..." (direct skill access) - parts = relative.split("/") + # or "/..." (direct skill access). Empty segments are dropped so a + # directory entry ("public/", as `ls` emits for dirs) is still recognized as + # a category root rather than yielding an empty skill name. + parts = [part for part in relative.split("/") if part] if len(parts) >= 2 and parts[0] in ("public", "custom", "legacy"): return parts[1] if len(parts) == 1 and parts[0] in ("public", "custom", "legacy"): @@ -241,6 +243,39 @@ def _is_disabled_skill_path(path: str, *, user_id: str | None = None) -> bool: return True +def _drop_disabled_skill_paths(paths: list[str], *, user_id: str | None = None) -> list[str]: + """Filter out paths that belong to a disabled skill. + + ``_is_disabled_skill_path`` gates the *requested* path, which is enough for + ``read_file`` but not for the tools that descend: ``ls``, ``glob`` and + ``grep`` return paths other than the one they were given, so a root anywhere + above a disabled skill still surfaces its files. This applies the same check + to the results. + + The enabled-state lookup re-reads ``extensions_config.json`` (or the per-user + skill state) on every call, so the verdict is memoized per skill — a 100-match + grep must not become 100 config reads. + """ + skills_prefix = _get_skills_container_path() + verdicts: dict[tuple[str, str], bool] = {} + kept: list[str] = [] + for path in paths: + skill_name = _extract_skill_name_from_skills_path(path) + if skill_name is None: + kept.append(path) + continue + # Leading segment (the category, or the skill itself in the direct + # layout) plus the name identifies the skill the same way + # _is_disabled_skill_path does, so paths sharing a key share a verdict. + category = path[len(skills_prefix) :].lstrip("/").split("/")[0] + key = (category, skill_name) + if key not in verdicts: + verdicts[key] = _is_disabled_skill_path(path, user_id=user_id) + if not verdicts[key]: + kept.append(path) + return kept + + def _resolve_skills_path(path: str) -> str: """Resolve a virtual skills path to a host filesystem path. @@ -1740,8 +1775,9 @@ def ls_tool(runtime: Runtime, description: str, path: str) -> str: path: The **absolute** path to the directory to list. """ try: + user_id = resolve_runtime_user_id(runtime) # Block access to disabled skill directories - if _is_disabled_skill_path(path, user_id=resolve_runtime_user_id(runtime)): + if _is_disabled_skill_path(path, user_id=user_id): skill_name = _extract_skill_name_from_skills_path(path) or "unknown" return f"Error: Skill '{skill_name}' is disabled. Access to its files is blocked. Enable the skill in settings before using it." sandbox = ensure_sandbox_initialized(runtime) @@ -1767,6 +1803,12 @@ def ls_tool(runtime: Runtime, description: str, path: str) -> str: output = "\n".join(children) if thread_data is not None: output = mask_local_paths_in_output(output, thread_data) + # The gate above only covers `path` itself; the listing descends into + # children, so a root above a disabled skill still exposes its files. + entries = _drop_disabled_skill_paths(output.splitlines(), user_id=user_id) + if not entries: + return "(empty)" + output = "\n".join(entries) try: from deerflow.config.app_config import get_app_config @@ -1811,6 +1853,11 @@ def glob_tool( max_results: Maximum number of paths to return. Default is 200. """ try: + user_id = resolve_runtime_user_id(runtime) + # Block access to disabled skill directories + if _is_disabled_skill_path(path, user_id=user_id): + skill_name = _extract_skill_name_from_skills_path(path) or "unknown" + return f"Error: Skill '{skill_name}' is disabled. Access to its files is blocked. Enable the skill in settings before using it." sandbox = ensure_sandbox_initialized(runtime) ensure_thread_directories_exist(runtime) requested_path = path @@ -1829,6 +1876,9 @@ def glob_tool( matches, truncated = sandbox.glob(path, pattern, include_dirs=include_dirs, max_results=effective_max_results) if thread_data is not None: matches = [mask_local_paths_in_output(match, thread_data) for match in matches] + # The gate above only covers `path` itself; the search descends into it, + # so a root above a disabled skill still surfaces its files. + matches = _drop_disabled_skill_paths(matches, user_id=user_id) return _format_glob_results(requested_path, matches, truncated) except SandboxError as e: return f"Error: {e}" @@ -1887,6 +1937,11 @@ def grep_tool( max_results: Maximum number of matching lines to return. Default is 100. """ try: + user_id = resolve_runtime_user_id(runtime) + # Block access to disabled skill directories + if _is_disabled_skill_path(path, user_id=user_id): + skill_name = _extract_skill_name_from_skills_path(path) or "unknown" + return f"Error: Skill '{skill_name}' is disabled. Access to its files is blocked. Enable the skill in settings before using it." sandbox = ensure_sandbox_initialized(runtime) ensure_thread_directories_exist(runtime) requested_path = path @@ -1919,6 +1974,10 @@ def grep_tool( ) for match in matches ] + # The gate above only covers `path` itself; the search descends into it, + # so a root above a disabled skill still surfaces its file contents. + allowed = set(_drop_disabled_skill_paths([match.path for match in matches], user_id=user_id)) + matches = [match for match in matches if match.path in allowed] return _format_grep_results(requested_path, matches, truncated) except SandboxError as e: return f"Error: {e}" diff --git a/backend/tests/test_sandbox_search_tools.py b/backend/tests/test_sandbox_search_tools.py index 4e7b0fb08..e3438c823 100644 --- a/backend/tests/test_sandbox_search_tools.py +++ b/backend/tests/test_sandbox_search_tools.py @@ -1,7 +1,9 @@ +import json from types import SimpleNamespace from unittest.mock import patch from deerflow.community.aio_sandbox.aio_sandbox import AioSandbox +from deerflow.config.paths import Paths from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping from deerflow.sandbox.search import GrepMatch, find_glob_matches, find_grep_matches from deerflow.sandbox.tools import glob_tool, grep_tool, ls_tool @@ -500,16 +502,33 @@ def test_ls_tool_skills_path_uses_sandbox_mapping_user_id_not_contextvar(tmp_pat ) monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + # Listing a category root descends into the skills below it, so the + # disabled-skill gate now resolves each one's enabled state. That lookup + # needs app config; without it the gate fails closed (see PR #3889) and the + # listing would be empty for a reason unrelated to what this test asserts. + skills_root = tmp_path / "skills" + (skills_root / "custom").mkdir(parents=True) + app_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=False), + ) + # Leave contextvar unset → get_effective_user_id() returns "default" # Before the fix, _resolve_skills_path would resolve to default_custom (empty) # After the fix, the sandbox PathMapping resolves to user-abc_custom (has my-skill) token = set_current_user(SimpleNamespace(id="default")) # contextvar says "default" try: - result = ls_tool.func( - runtime=_make_runtime(tmp_path), - description="list custom skills", - path="/mnt/skills/custom", - ) + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=base_dir)): + with patch("deerflow.config.get_app_config", return_value=app_config): + result = ls_tool.func( + runtime=_make_runtime(tmp_path), + description="list custom skills", + path="/mnt/skills/custom", + ) # Must show user-abc's skill (sandbox mapping), NOT default's empty dir (contextvar) assert "my-skill" in result @@ -536,3 +555,248 @@ def test_ls_tool_filters_upload_staging_files(tmp_path, monkeypatch) -> None: assert "/mnt/user-data/uploads/report.txt" in result assert "/mnt/user-data/uploads/.upload-note.txt" in result assert ".upload-active.part" not in result + + +def _make_skills_sandbox(tmp_path, monkeypatch, *, disabled: str): + """Skills tree with one disabled and one enabled public skill. + + Drives the real `_is_disabled_skill_path` gate through a real + extensions_config.json rather than stubbing the gate out. + """ + skills_dir = tmp_path / "skills" + for name, body in [(disabled, "SECRET_PROCEDURE = step-1-step-2\n"), ("open-skill", "PUBLIC_PROCEDURE = hello\n")]: + (skills_dir / "public" / name).mkdir(parents=True) + (skills_dir / "public" / name / "SKILL.md").write_text(f"---\nname: {name}\n---\n\n{body}", encoding="utf-8") + + ext = tmp_path / "extensions_config.json" + ext.write_text( + json.dumps({"mcpServers": {}, "skills": {disabled: {"enabled": False}, "open-skill": {"enabled": True}}}), + encoding="utf-8", + ) + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(ext)) + + sandbox = LocalSandbox( + id="local", + path_mappings=[PathMapping(container_path="/mnt/skills", local_path=str(skills_dir), read_only=True)], + ) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + return sandbox + + +def test_glob_tool_blocks_disabled_skill_root(tmp_path, monkeypatch) -> None: + """glob must refuse a disabled skill's own directory, like ls and read_file do.""" + runtime = _make_runtime(tmp_path) + _make_skills_sandbox(tmp_path, monkeypatch, disabled="secret-skill") + + result = glob_tool.func( + runtime=runtime, + description="list skill files", + pattern="**/*.md", + path="/mnt/skills/public/secret-skill", + ) + + assert "Skill 'secret-skill' is disabled" in result + assert "SKILL.md" not in result + + +def test_grep_tool_blocks_disabled_skill_root(tmp_path, monkeypatch) -> None: + """grep must refuse a disabled skill's own directory, like ls and read_file do.""" + runtime = _make_runtime(tmp_path) + _make_skills_sandbox(tmp_path, monkeypatch, disabled="secret-skill") + + result = grep_tool.func( + runtime=runtime, + description="search skill files", + pattern="SECRET_PROCEDURE", + path="/mnt/skills/public/secret-skill", + ) + + assert "Skill 'secret-skill' is disabled" in result + assert "SECRET_PROCEDURE = step-1-step-2" not in result + + +def test_glob_tool_does_not_surface_disabled_skill_from_ancestor_root(tmp_path, monkeypatch) -> None: + """A root above the skill must not surface it: glob descends past the path gate.""" + runtime = _make_runtime(tmp_path) + _make_skills_sandbox(tmp_path, monkeypatch, disabled="secret-skill") + + result = glob_tool.func( + runtime=runtime, + description="find skills", + pattern="**/SKILL.md", + path="/mnt/skills", + ) + + assert "secret-skill" not in result + # ...while the enabled sibling is still returned. + assert "/mnt/skills/public/open-skill/SKILL.md" in result + + +def test_grep_tool_does_not_surface_disabled_skill_content_from_ancestor_root(tmp_path, monkeypatch) -> None: + """The strongest leak: grep from /mnt/skills printed a disabled skill's file contents.""" + runtime = _make_runtime(tmp_path) + _make_skills_sandbox(tmp_path, monkeypatch, disabled="secret-skill") + + result = grep_tool.func( + runtime=runtime, + description="search skills", + pattern="PROCEDURE", + path="/mnt/skills", + ) + + assert "SECRET_PROCEDURE = step-1-step-2" not in result + assert "secret-skill" not in result + # ...while the enabled sibling still matches. + assert "PUBLIC_PROCEDURE = hello" in result + + +def test_ls_tool_does_not_surface_disabled_skill_from_category_root(tmp_path, monkeypatch) -> None: + """ls gates the requested path but descends two levels, so the category root leaked.""" + runtime = _make_runtime(tmp_path) + _make_skills_sandbox(tmp_path, monkeypatch, disabled="secret-skill") + + result = ls_tool.func( + runtime=runtime, + description="list public skills", + path="/mnt/skills/public", + ) + + assert "secret-skill" not in result + # ...while the enabled sibling is still listed. + assert "open-skill" in result + + +def test_ls_tool_keeps_category_dirs_when_listing_skills_root(tmp_path, monkeypatch) -> None: + """`ls /mnt/skills` lists dirs with a trailing slash ("public/"), which the + skill-name extractor must read as a category root, not as a skill named "". + + An empty name skips the `skill_name is None` short-circuit and falls through + to a config read; it currently lands on "keep" only because unknown skills + default to enabled. This pins the intended outcome directly: category dirs + stay visible while the disabled skill below them does not. + """ + runtime = _make_runtime(tmp_path) + _make_skills_sandbox(tmp_path, monkeypatch, disabled="secret-skill") + + result = ls_tool.func( + runtime=runtime, + description="list skills root", + path="/mnt/skills", + ) + + assert "/mnt/skills/public" in result + assert "open-skill" in result + assert "secret-skill" not in result + + +def test_extract_skill_name_treats_category_dir_with_trailing_slash_as_root() -> None: + """LocalSandbox.list_dir appends "/" to directories, so the gate sees + "/mnt/skills/public/" — which must resolve to None (category root), not "". + """ + from deerflow.sandbox.tools import _extract_skill_name_from_skills_path as extract + + # Changed direction: trailing-slash category roots used to yield "". + assert extract("/mnt/skills/public/") is None + assert extract("/mnt/skills/custom/") is None + assert extract("/mnt/skills/legacy/") is None + # Unchanged directions: real skills still resolve, with or without the slash. + assert extract("/mnt/skills/public") is None + assert extract("/mnt/skills/public/bootstrap") == "bootstrap" + assert extract("/mnt/skills/public/bootstrap/") == "bootstrap" + assert extract("/mnt/skills/public/bootstrap/SKILL.md") == "bootstrap" + assert extract("/mnt/skills/my-skill/") == "my-skill" + assert extract("/mnt/user-data/workspace/file.md") is None + + +def _make_custom_skills_sandbox(tmp_path, monkeypatch, *, user_id: str, disabled: str): + """Per-user CUSTOM skills tree with one disabled and one enabled skill. + + CUSTOM/LEGACY enabled state lives in the per-user ``_skill_states.json`` + (``UserScopedSkillStorage``), a different store from the public skills' + ``extensions_config.json`` — so the public fixture above does not exercise + this branch of ``_is_disabled_skill_path``. + """ + from deerflow.skills.storage import reset_skill_storage + + base_dir = tmp_path / ".deer-flow" + user_skills = base_dir / "users" / user_id / "skills" + user_custom = user_skills / "custom" + for name, body in [(disabled, "SECRET_PROCEDURE = step-1-step-2\n"), ("open-custom", "PUBLIC_PROCEDURE = hello\n")]: + (user_custom / name).mkdir(parents=True) + (user_custom / name / "SKILL.md").write_text(f"---\nname: {name}\n---\n\n{body}", encoding="utf-8") + + (user_skills / "_skill_states.json").write_text( + json.dumps({disabled: {"enabled": False}, "open-custom": {"enabled": True}}), + encoding="utf-8", + ) + + skills_root = tmp_path / "skills" + (skills_root / "public").mkdir(parents=True) + (skills_root / "custom").mkdir(parents=True) + app_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=False), + ) + + sandbox = LocalSandbox( + id=f"local:{user_id}:thread-1", + path_mappings=[PathMapping(container_path="/mnt/skills/custom", local_path=str(user_custom), read_only=True)], + ) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + # The storage cache is keyed by user id, not by base_dir: a cached instance + # from another test would read the wrong _skill_states.json. + reset_skill_storage() + monkeypatch.setattr("deerflow.sandbox.tools.resolve_runtime_user_id", lambda runtime: user_id) + return base_dir, app_config + + +def test_grep_tool_does_not_surface_disabled_custom_skill(tmp_path, monkeypatch) -> None: + """CUSTOM skills resolve enabled state through the per-user _skill_states.json, + not extensions_config.json — the store the public-skill tests never touch.""" + from deerflow.skills.storage import reset_skill_storage + + runtime = _make_runtime(tmp_path) + base_dir, app_config = _make_custom_skills_sandbox(tmp_path, monkeypatch, user_id="user-abc", disabled="secret-custom") + + try: + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=base_dir)): + with patch("deerflow.config.get_app_config", return_value=app_config): + result = grep_tool.func( + runtime=runtime, + description="search custom skills", + pattern="PROCEDURE", + path="/mnt/skills/custom", + ) + finally: + reset_skill_storage() + + assert "SECRET_PROCEDURE = step-1-step-2" not in result + assert "secret-custom" not in result + # ...while the enabled sibling still matches. + assert "PUBLIC_PROCEDURE = hello" in result + + +def test_ls_tool_does_not_surface_disabled_custom_skill(tmp_path, monkeypatch) -> None: + """Same per-user store, via the descending ls listing.""" + from deerflow.skills.storage import reset_skill_storage + + runtime = _make_runtime(tmp_path) + base_dir, app_config = _make_custom_skills_sandbox(tmp_path, monkeypatch, user_id="user-abc", disabled="secret-custom") + + try: + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=base_dir)): + with patch("deerflow.config.get_app_config", return_value=app_config): + result = ls_tool.func( + runtime=runtime, + description="list custom skills", + path="/mnt/skills/custom", + ) + finally: + reset_skill_storage() + + assert "secret-custom" not in result + assert "open-custom" in result