mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-25 15:37:53 +00:00
fix(sandbox): stop glob/grep/ls from surfacing disabled skills' files (#4096)
* fix(sandbox): stop glob/grep/ls from surfacing disabled skills' files The disabled-skill gate checks the path a tool is given, but ls, glob and grep all descend from it and return other paths, so a root above a disabled skill still serves its files. glob and grep never called the gate at all; ls called it only on its own argument and still leaked from a category root. Add the entry gate to glob/grep, and filter what all three return through the existing fail-closed _is_disabled_skill_path. The verdict is memoized per skill because ExtensionsConfig.from_file() is uncached, so a per-match check would turn a 100-match grep into 100 config reads. * fix(sandbox): normalize trailing slashes in the disabled-skill path check Review follow-ups on the disabled-skill gate: - _extract_skill_name_from_skills_path returned "" instead of None for a category directory carrying a trailing slash. LocalSandbox.list_dir appends "/" to directories, so `ls /mnt/skills` yields "/mnt/skills/public/", giving parts ["public", ""]. The empty name skipped the `skill_name is None` short-circuit and fell through to a config read, landing on the right outcome only because unknown skills default to enabled. Drop empty segments so a trailing-slash category root takes the existing category-root branch. - ls_tool resolved the runtime user id twice per call; hoist it into a local, matching glob_tool/grep_tool. - Cover the CUSTOM path: custom/legacy skills resolve their enabled state through the per-user _skill_states.json, a different store from the public skills' extensions_config.json, and no automated test exercised it.
This commit is contained in:
Aari
GitHub
parent
5edc7a889e
commit
97e2268d52
@ -173,8 +173,10 @@ def _extract_skill_name_from_skills_path(path: str) -> str | None:
|
||||
if not relative:
|
||||
return None
|
||||
# Expected patterns: "public/<name>/...", "custom/<name>/...", "legacy/<name>/..."
|
||||
# or "<name>/..." (direct skill access)
|
||||
parts = relative.split("/")
|
||||
# or "<name>/..." (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}"
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user