From 16877b92fc1aa5c0241a96313d6c400a5933f12d Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Sat, 10 Oct 2026 09:58:26 +0900 Subject: [PATCH] fix(skills): confine review_skill_package local targets to the caller (#6580) * fix(skills): confine review_skill_package local targets to the caller Local path targets were allowed anywhere under the Gateway cwd or /tmp. Every documented deployment keeps DEER_FLOW_HOME under the cwd, so a user who knew another user's id could pass .deer-flow/users//skills/custom/ and get that skill's SKILL.md and references/ back in the model response. The tool is always available, so skill tool policy could not remove it. Local targets are now confined to the configured skills root and the caller's own user directory (resolved from the runtime user id through make_safe_user_id, like the user-scoped skill storage). skill:// and inline:// targets are unchanged. * test(skills): pin dot-dot and sandbox-planted symlink review targets * docs(changelog): reference #6580 * fix(skills): ignore a review skills root that contains per-user directories A skills root configured at or above DEER_FLOW_HOME would re-admit every user's directory as a local review target. Drop such a root and log a warning instead of trusting it silently. * test(skills): use the platform temp dir for the shared-temp review case A hardcoded /tmp does not exist on Windows dev hosts. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 11 ++ CHANGELOG_zh.md | 9 + .../harness/deerflow/skills/AGENTS.md | 2 +- .../builtins/review_skill_package_tool.py | 28 ++- .../tests/test_review_skill_package_tool.py | 175 +++++++++++++++--- 5 files changed, 194 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 988c0f3239..ed540b4c74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4303,6 +4303,16 @@ This release closes that milestone with **439 merged pull requests**. package. Previously a `hooks/install.jse` carrying one stray byte received no static analysis and no executable review. ([#6321]) +- **skills:** Stop `review_skill_package` from reading other users' skills. + Local path targets were allowed anywhere under the Gateway cwd or `/tmp`, + and every documented deployment keeps `DEER_FLOW_HOME` under the cwd, so a + user who knew another user's id could pass + `.deer-flow/users//skills/custom/` and get that skill's + `SKILL.md` and `references/` back in the model response. The tool is + always available, so skill tool policy could not remove it. Local targets + are now confined to the configured skills root and the caller's own user + directory; `skill://` and `inline://` targets are unchanged. ([#6580]) + ### Documentation - **docs:** Fix the Apple Container verification instructions. The guide @@ -9416,4 +9426,5 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6537]: https://github.com/bytedance/deer-flow/pull/6537 [#6543]: https://github.com/bytedance/deer-flow/pull/6543 [#6556]: https://github.com/bytedance/deer-flow/pull/6556 +[#6580]: https://github.com/bytedance/deer-flow/pull/6580 [#6582]: https://github.com/bytedance/deer-flow/pull/6582 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 1e4a59d2b6..9334891d11 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -3345,6 +3345,14 @@ 文件。此前带一个杂散字节的 `hooks/install.jse` 既得不到静态分析，也 不经过可执行代码审查。([#6321]) +- **技能：** `review_skill_package` 不再能读取其他用户的技能。此前本地路径 + 目标只要位于 Gateway 工作目录或 `/tmp` 之下即被放行，而所有文档化的部署都把 + `DEER_FLOW_HOME` 放在工作目录之下，因此知道他人用户 id 的用户可以传入 + `.deer-flow/users//skills/custom/`，在模型响应中拿到该技能的 + `SKILL.md` 与 `references/` 内容。该工具始终可用，技能工具策略也无法移除它。 + 现在本地目标仅限于配置的技能根目录和调用者自己的用户目录；`skill://` 与 + `inline://` 目标不受影响。([#6580]) + ### 文档 - **文档：** 修正 Apple Container 的验证说明。 @@ -7603,4 +7611,5 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#6520]: https://github.com/bytedance/deer-flow/pull/6520 [#6537]: https://github.com/bytedance/deer-flow/pull/6537 [#6556]: https://github.com/bytedance/deer-flow/pull/6556 +[#6580]: https://github.com/bytedance/deer-flow/pull/6580 [#6582]: https://github.com/bytedance/deer-flow/pull/6582 diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 80c0370be3..4a91c0df29 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -22,7 +22,7 @@ - **SkillScan**: `packages/harness/deerflow/skills/skillscan/` is the native deterministic scanner for `.skill` archives and agent-managed skill writes. It runs offline before the LLM scanner, emits structured findings (`rule_id`, `severity`, `file`, `line`, `message`, `remediation`, redacted `evidence` — category/analyzer are encoded in the `rule_id` prefix), blocks `CRITICAL`, and passes warning findings into `scan_skill_content()`. The moderation adapter must normalize both plain-text responses and LangChain Responses API text blocks before parsing the required JSON decision. `scan_archive_preflight()` / `scan_skill_dir()` are pure sync functions (dispatch off the event loop); `enforce_static_scan()` applies the blocking policy and the `skill_scan.enabled` kill switch. `skills/package_files.py` is the single definition of code files (`scripts/` members, code suffixes, extensionless `#!` files) and executable magic bytes; the installer, export guard, and SkillScan all import it, so do not re-derive either rule locally. Files that fail NUL-free UTF-8 decoding are treated as binaries unless they are code files: interpreters still run those, so they raise `package-undecodable-script` (HIGH) and are analyzed over a lossy decode, keeping one stray byte from hiding a file or downgrading a `CRITICAL` match; one with executable magic skips the text rules, which only misread its string tables. The Python instance-client signal deliberately follows only a one-level, same-scope evidence chain (PR #4265 review): a proven imported constructor bound to a simple name, optional name-to-name alias propagation, rebinding invalidation, and a constructor-supported outbound method or context-manager use; bare canonical-looking names never fall back to module identity. Nested scopes never inherit client handles and inherit only constructor aliases proven stable by a binding-only enclosing-scope prepass. Comprehensions, walrus-bearing statements, annotations, executable expressions inside complex binding targets, unsupported operations, and ambiguous flows produce no finding from this signal; skipped constructs invalidate all names they may bind, while representative false negatives are pinned by `test_python_declared_false_negatives_stay_unreported`. Compound bodies are walked from isolated copies so wrapping code in `if True:` is not a bypass, while copied scope entries, binding-only prepasses, and AST visits consume a deterministic work budget and the walk stops after its first sink. Budget or recursion exhaustion skips only this best-effort signal and retains deterministic findings already collected for the file. Do not add Semgrep/OpenGrep or YAML rule-engine dependencies to the core path; Phase 1 rule specs live in Python constants next to their analyzers in `skillscan/orchestrator.py`. - **SkillScan HTTP hosts**: recognize HTTP/HTTPS schemes case-insensitively in both URL regexes and prefix checks, without changing source evidence. Use URL authority parsing for lowercase hosts and bracketed IPv6 loopback; userinfo must never hide an external host. Malformed URLs remain outbound for exfiltration checks. Cleartext HTTP detection covers IPv6 literals; cloud-metadata hostname matching is case-insensitive. Coverage: `tests/test_skillscan_native.py`. - **Python secret mappings**: `secret-env-assignment` scans literal container values and conditional branches under secret-like bindings. Dictionary keys are structural labels and contribute candidates only when their folded literal matches the shared `_SECRET_TOKEN_PATTERNS`; they must not inherit credential evidence from the enclosing variable name. Opaque keys without a recognized token format stay unreported. Regressions in `tests/test_skillscan_native.py` cover runtime-loaded mappings, nested labels, credential-value locations, and literal/concatenated token keys; `tests/test_skill_review_core.py` pins the CI CLI gate. -- **Skill Review Core**: `packages/harness/deerflow/skills/review/` provides read-only package snapshots, deterministic facts, resource/eval analysis, report rendering, and the CLI (`python -m deerflow.skills.review.cli`). It reuses the shared frontmatter helper and SkillScan; it must not import `app.*`, execute target scripts, install dependencies, or call networks. JSON contracts live in `contracts/skill_review/`. The analyzer hands SkillScan a byte-faithful copy of every non-symlink snapshot file (binary entries carry `content_base64`), so package rules such as `package-executable-binary` cover binaries; only eval-fixture `SKILL.md` samples are withheld, while scripts and other files under `evals/fixtures/` are scanned. Materialization uses exclusive create, so a duplicate archive member or a case-folded name fails the scan closed (`not_assessed: skillscan`) instead of overwriting an earlier file, as does an entry without bytes in an untruncated snapshot. The `review_skill_package` built-in tool labels results with `review_subject_entry` and never `skill_context_entry`, so reviewing a target does not activate it, bind its `required-secrets`, or apply its `allowed-tools`. Its model-visible `ToolMessage.content` is a compact JSON payload with untrusted control tags neutralized; the full raw review payload, including Markdown renders, stays in `ToolMessage.artifact`. CI should run the CLI with `--fail-on error --fail-on-incomplete` so blocker/error findings and truncated/not-assessed packages fail the gate. The public `skills/public/skill-reviewer` skill owns semantic readiness review and suggestions only; mutation and runtime experiments remain owned by `skill-creator`. +- **Skill Review Core**: `packages/harness/deerflow/skills/review/` provides read-only package snapshots, deterministic facts, resource/eval analysis, report rendering, and the CLI (`python -m deerflow.skills.review.cli`). It reuses the shared frontmatter helper and SkillScan; it must not import `app.*`, execute target scripts, install dependencies, or call networks. JSON contracts live in `contracts/skill_review/`. The analyzer hands SkillScan a byte-faithful copy of every non-symlink snapshot file (binary entries carry `content_base64`), so package rules such as `package-executable-binary` cover binaries; only eval-fixture `SKILL.md` samples are withheld, while scripts and other files under `evals/fixtures/` are scanned. Materialization uses exclusive create, so a duplicate archive member or a case-folded name fails the scan closed (`not_assessed: skillscan`) instead of overwriting an earlier file, as does an entry without bytes in an untruncated snapshot. The `review_skill_package` built-in tool labels results with `review_subject_entry` and never `skill_context_entry`, so reviewing a target does not activate it, bind its `required-secrets`, or apply its `allowed-tools`. Its model-visible `ToolMessage.content` is a compact JSON payload with untrusted control tags neutralized; the full raw review payload, including Markdown renders, stays in `ToolMessage.artifact`. Local path targets are host paths, so the tool confines them to the configured skills root and the caller's own `{base_dir}/users/{user_id}/`; never re-add the Gateway cwd or `/tmp`, because `DEER_FLOW_HOME` sits under the cwd and those roots expose other users' skills. A skills root that contains `{base_dir}/users/` is dropped with a warning. CI should run the CLI with `--fail-on error --fail-on-incomplete` so blocker/error findings and truncated/not-assessed packages fail the gate. The public `skills/public/skill-reviewer` skill owns semantic readiness review and suggestions only; mutation and runtime experiments remain owned by `skill-creator`. #### Request-Scoped Secrets (`required-secrets`) diff --git a/backend/packages/harness/deerflow/tools/builtins/review_skill_package_tool.py b/backend/packages/harness/deerflow/tools/builtins/review_skill_package_tool.py index b83b22b3ee..5907bb686f 100644 --- a/backend/packages/harness/deerflow/tools/builtins/review_skill_package_tool.py +++ b/backend/packages/harness/deerflow/tools/builtins/review_skill_package_tool.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging from pathlib import Path from typing import Literal @@ -9,6 +10,7 @@ from langchain_core.messages import ToolMessage from langchain_core.tools import tool from langgraph.types import Command +from deerflow.config.paths import get_paths, make_safe_user_id from deerflow.runtime.user_context import resolve_runtime_user_id from deerflow.skills.review.analyzer import analyze_skill_package from deerflow.skills.review.models import stable_json_dumps @@ -17,6 +19,8 @@ from deerflow.skills.review.renderer import build_static_report, render_report_m from deerflow.skills.storage import get_or_new_skill_storage, get_or_new_user_skill_storage from deerflow.tools.types import Runtime +logger = logging.getLogger(__name__) + Profile = Literal["deerflow", "agentskills"] IncludeContent = Literal["none", "facts-only", "semantic-review"] @@ -103,26 +107,36 @@ def _snapshot_for_target(target: str, *, runtime: Runtime, inline_content: str | raise ValueError("inline_content is required for inline:// targets") return build_inline_snapshot(inline_content, name_hint=target) + user_id = resolve_runtime_user_id(runtime) if target.startswith("skill://"): - user_id = resolve_runtime_user_id(runtime) storage = get_or_new_user_skill_storage(user_id) return InstalledSkillReader.from_target(target, storage=storage).read() path = Path(target).expanduser() - _ensure_local_target_allowed(path) + _ensure_local_target_allowed(path, user_id=user_id) if path.suffix == ".skill": return ArchivePackageReader(path).read() return LocalDirectoryReader(path).read() -def _ensure_local_target_allowed(path: Path) -> None: +def _ensure_local_target_allowed(path: Path, *, user_id: str) -> None: resolved = path.resolve() - allowed_roots: list[Path] = [Path.cwd().resolve(), Path("/tmp").resolve()] + # Never allow the Gateway cwd or /tmp: DEER_FLOW_HOME sits under the cwd in + # every documented deployment, so those roots expose other users' data. + paths = get_paths() + allowed_roots: list[Path] = [paths.user_dir(make_safe_user_id(user_id)).resolve()] try: storage = get_or_new_skill_storage() - allowed_roots.append(storage.get_skills_root_path().resolve()) + skills_root = storage.get_skills_root_path().resolve() except Exception: - pass + skills_root = None + if skills_root is not None: + # The same applies to a skills root configured at or above DEER_FLOW_HOME. + users_root = (paths.base_dir / "users").resolve() + if users_root.is_relative_to(skills_root): + logger.warning("Ignoring skills root %s as a review target root: it contains per-user directories", skills_root) + else: + allowed_roots.append(skills_root) for root in allowed_roots: try: @@ -131,7 +145,7 @@ def _ensure_local_target_allowed(path: Path) -> None: continue _ensure_local_target_is_package_or_archive(resolved) return - raise ValueError("Local review targets must be under the current workspace, /tmp, or the configured skills root") + raise ValueError("Local review targets must be under the caller's user directory or the configured skills root") def _ensure_local_target_is_package_or_archive(path: Path) -> None: diff --git a/backend/tests/test_review_skill_package_tool.py b/backend/tests/test_review_skill_package_tool.py index 013b56ede5..59bd31239b 100644 --- a/backend/tests/test_review_skill_package_tool.py +++ b/backend/tests/test_review_skill_package_tool.py @@ -1,15 +1,20 @@ import json +import tempfile +from pathlib import Path from types import SimpleNamespace +import pytest + +from deerflow.config.paths import Paths, make_safe_user_id from deerflow.skills.storage.local_skill_storage import LocalSkillStorage from deerflow.tools.builtins.review_skill_package_tool import review_skill_package -def _runtime() -> SimpleNamespace: +def _runtime(user_id: str = "default") -> SimpleNamespace: return SimpleNamespace( state={}, - context={"thread_id": "thread-1", "user_id": "default"}, - config={"configurable": {"thread_id": "thread-1", "user_id": "default"}}, + context={"thread_id": "thread-1", "user_id": user_id}, + config={"configurable": {"thread_id": "thread-1", "user_id": user_id}}, tool_call_id="tool-1", ) @@ -92,29 +97,30 @@ def test_review_skill_package_rejects_unsafe_local_path(): assert "Local review targets must be under" in message.content -def test_review_skill_package_rejects_local_directory_without_skill_md(tmp_path, monkeypatch): +@pytest.fixture +def deployment(tmp_path, monkeypatch): + """Mirror documented deployments: DEER_FLOW_HOME and the skills root sit under the Gateway cwd.""" monkeypatch.chdir(tmp_path) - (tmp_path / "notes.txt").write_text("workspace note", encoding="utf-8") + paths = Paths(tmp_path / ".deer-flow") + storage = LocalSkillStorage(host_path=str(tmp_path / "skills"), container_path="/mnt/skills") + monkeypatch.setattr("deerflow.tools.builtins.review_skill_package_tool.get_paths", lambda: paths) + monkeypatch.setattr("deerflow.tools.builtins.review_skill_package_tool.get_or_new_skill_storage", lambda: storage) + return paths + + +def _write_package(package_dir, content: str) -> None: + package_dir.mkdir(parents=True) + (package_dir / "SKILL.md").write_text(content, encoding="utf-8") + + +@pytest.mark.parametrize("user_id", ["alice", "feishu:ou_alice"]) +def test_review_skill_package_allows_callers_own_user_package(deployment, user_id): + package = deployment.user_custom_skills_dir(make_safe_user_id(user_id)) / "demo-skill" + _write_package(package, _skill_content()) command = review_skill_package.func( - target=".", - runtime=_runtime(), - ) - - message = command.update["messages"][0] - assert message.status == "error" - assert "directories containing a root SKILL.md" in message.content - - -def test_review_skill_package_allows_local_skill_package(tmp_path, monkeypatch): - package = tmp_path / "demo" - package.mkdir() - (package / "SKILL.md").write_text(_skill_content(), encoding="utf-8") - monkeypatch.chdir(tmp_path) - - command = review_skill_package.func( - target="demo", - runtime=_runtime(), + target=str(package), + runtime=_runtime(user_id), include_content="facts-only", ) @@ -122,3 +128,126 @@ def test_review_skill_package_allows_local_skill_package(tmp_path, monkeypatch): payload = json.loads(message.content) assert message.status == "success" assert payload["facts"]["subject"]["declared_name"] == "demo-skill" + + +def test_review_skill_package_allows_shared_skills_root_package(deployment, tmp_path): + package = tmp_path / "skills" / "public" / "demo-skill" + _write_package(package, _skill_content()) + + command = review_skill_package.func( + target=str(package), + runtime=_runtime("alice"), + include_content="facts-only", + ) + + assert command.update["messages"][0].status == "success" + + +@pytest.mark.parametrize( + "target_for", + [ + pytest.param(lambda paths, victim: str(victim), id="absolute"), + pytest.param(lambda paths, victim: f"{paths.user_dir('alice')}/../bob/skills/custom/{victim.name}", id="dot-dot"), + pytest.param(lambda paths, victim: ".deer-flow/users/bob/skills/custom/secret-skill", id="cwd-relative"), + ], +) +def test_review_skill_package_rejects_another_users_package(deployment, target_for): + victim_package = deployment.user_custom_skills_dir("bob") / "secret-skill" + _write_package(victim_package, _skill_content("secret-skill") + "BOB_PRIVATE_BODY\n") + deployment.user_dir("alice").mkdir(parents=True) + + command = review_skill_package.func( + target=target_for(deployment, victim_package), + runtime=_runtime("alice"), + ) + + message = command.update["messages"][0] + assert message.status == "error" + assert "Local review targets must be under" in message.content + assert "BOB_PRIVATE_BODY" not in message.content + + +@pytest.mark.parametrize("skills_root_for", [lambda home: home, lambda home: home.parent], ids=["deer-flow-home", "ancestor"]) +def test_review_skill_package_ignores_skills_root_that_contains_user_dirs(deployment, monkeypatch, caplog, skills_root_for): + storage = LocalSkillStorage(host_path=str(skills_root_for(deployment.base_dir)), container_path="/mnt/skills") + monkeypatch.setattr("deerflow.tools.builtins.review_skill_package_tool.get_or_new_skill_storage", lambda: storage) + package = deployment.user_custom_skills_dir("bob") / "secret-skill" + _write_package(package, _skill_content("secret-skill")) + + with caplog.at_level("WARNING", logger="deerflow.tools.builtins.review_skill_package_tool"): + command = review_skill_package.func( + target=str(package), + runtime=_runtime("alice"), + include_content="facts-only", + ) + + message = command.update["messages"][0] + assert message.status == "error" + assert "Local review targets must be under" in message.content + assert "secret-skill" not in message.content + assert "contains per-user directories" in caplog.text + + +def test_review_skill_package_rejects_symlink_planted_in_callers_outputs(deployment): + """A sandbox can write symlinks into the caller's own outputs; the resolved target decides.""" + victim_package = deployment.user_custom_skills_dir("bob") / "secret-skill" + _write_package(victim_package, _skill_content("secret-skill") + "BOB_PRIVATE_BODY\n") + outputs_dir = deployment.sandbox_outputs_dir("thread-1", user_id="alice") + outputs_dir.mkdir(parents=True) + link = outputs_dir / "borrowed-skill" + link.symlink_to(victim_package, target_is_directory=True) + + command = review_skill_package.func( + target=str(link), + runtime=_runtime("alice"), + ) + + message = command.update["messages"][0] + assert message.status == "error" + assert "Local review targets must be under" in message.content + assert "BOB_PRIVATE_BODY" not in message.content + + +def test_review_skill_package_rejects_package_only_under_cwd(deployment, tmp_path): + _write_package(tmp_path / "demo", _skill_content()) + + command = review_skill_package.func( + target="demo", + runtime=_runtime("alice"), + include_content="facts-only", + ) + + message = command.update["messages"][0] + assert message.status == "error" + assert "Local review targets must be under" in message.content + + +def test_review_skill_package_rejects_local_directory_without_skill_md(deployment): + user_dir = deployment.user_dir("alice") + user_dir.mkdir(parents=True) + (user_dir / "notes.txt").write_text("workspace note", encoding="utf-8") + + command = review_skill_package.func( + target=str(user_dir), + runtime=_runtime("alice"), + ) + + message = command.update["messages"][0] + assert message.status == "error" + assert "directories containing a root SKILL.md" in message.content + + +def test_review_skill_package_rejects_package_under_shared_tmp(deployment): + with tempfile.TemporaryDirectory(dir=tempfile.gettempdir()) as shared_tmp: + package = Path(shared_tmp) / "demo" + _write_package(package, _skill_content()) + + command = review_skill_package.func( + target=str(package), + runtime=_runtime("alice"), + include_content="facts-only", + ) + + message = command.update["messages"][0] + assert message.status == "error" + assert "Local review targets must be under" in message.content