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