mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-09 21:49:37 +00:00
fix(skills): parse Responses API content blocks in moderation scanner (#4936)
* Fix skill moderation parsing for Responses API content blocks Normalize LangChain Responses API text blocks before parsing the security moderation decision, while preserving the existing fail-closed behavior for unavailable or invalid moderation results. Add regression coverage for mixed content blocks and document the compatibility boundary. Constraint: Responses API AIMessage content is list-shaped while Chat Completions content is string-shaped Rejected: Disable security scanning | would weaken the skill write safety boundary Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep moderation parsing provider-format tolerant without including reasoning or tool blocks in the decision payload Tested: 27 security scanner tests; ruff check; ruff format check Not-tested: Live moderation request against the configured external endpoint * Reuse shared LLM response text normalization Route skill moderation responses through the existing provider-format normalizer so only text and output_text blocks participate in JSON parsing. Strengthen regression coverage with reasoning and tool blocks that contain misleading text fields.\n\nConstraint: Responses API content is shared across multiple harness consumers\nRejected: Keep a private normalizer | duplicated provider-shape policy diverges and can reintroduce reasoning-block contamination\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Extend the shared normalizer when a new provider content shape is verified; do not add divergent local parsers\nTested: 118 related backend tests; regression test red against the previous parser; Ruff check and format check\nNot-tested: Live GitHub CLA status refresh * Restore trusted external skill package loading Skill discovery follows one-level package-directory symlinks, but activation path validation rejected the resolved external path. Restore that compatibility for configured custom-skill category roots while keeping file-level symlinks and deeper escapes blocked. Add regression coverage for local and user-scoped storage plus slash activation, and document the boundary. Constraint: Existing skill discovery follows directory symlinks and operator-managed external packages must remain loadable Rejected: Allow arbitrary resolved paths | would weaken the skill path trust boundary Confidence: high Scope-risk: moderate Directive: Keep the final SKILL.md file symlink-free and preserve one-level category-root validation Tested: 79 targeted skill storage, loader, slash activation, and user-scoped tests passed; Ruff check and format check passed; GitNexus staged change detection reported low risk Not-tested: Real symlink activation on this Windows host lacks SeCreateSymbolicLinkPrivilege and is skipped Related: Skill projection copies sources into sandbox-visible views * Exercise real filesystem symlink boundaries in skill storage tests Replace global Path.resolve/is_symlink mocks with real directory and file symlinks, preserving the Windows privilege skip. Add regression coverage for deeper custom-root escapes and symlinks under non-custom categories so the one-level allowance remains explicit. Constraint: Symlink creation requires SeCreateSymbolicLinkPrivilege on some Windows runners Rejected: Keep global path-method mocks | they validate the mock behavior rather than filesystem semantics Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep security-boundary tests on real filesystem primitives; skip only when the runner lacks symlink privilege Tested: 76 targeted loader/storage/slash tests; Ruff check; Ruff format check Not-tested: Windows symlink-enabled execution on this host Related: #4936 * Pin the actual nested symlink escape boundary Place the second symlink below a real custom package directory so the test reaches the one-level relative-parent guard instead of returning early on a non-symlink parent. Keep the public-category rejection coverage unchanged. Constraint: The security boundary depends on both symlink depth and category root Rejected: Link the outer package directory directly | the parent is not a symlink at validation time, so the depth guard is never evaluated Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep this regression tied to the exact relative_parent.parts depth check Tested: Targeted storage, loader, and slash suites; GitNexus staged detection Not-tested: Symlink-enabled execution on this Windows host Related: #4936 * Make the nested symlink regression reach the depth guard The test now validates the SKILL.md directly through the nested symlink, so the symlink is the immediate parent and the relative-parent depth check is executed. Constraint: Windows test execution may skip when symlink privilege is unavailable Rejected: Keep the extra nested path segment | it bypasses the symlink-depth guard through an early return Confidence: high Scope-risk: narrow Reversibility: clean Directive: Mutation tests must fail when the depth restriction is removed Tested: Targeted test (skipped on this Windows host without symlink privilege); Ruff check and format check Not-tested: Real symlink execution on Windows; Linux CI will exercise the case Related: #4936 * Keep sandbox projections fresh for linked external skill packages The storage layer intentionally accepts one-level custom package-directory symlinks, but projection freshness previously hashed only the link inode. Follow the permitted target tree during custom and legacy source-signature scans so edits to SKILL.md, scripts, references, or assets trigger a rebuild before sandbox use. Constraint: Preserve the existing one-level custom/legacy symlink boundary and do not follow public, integration, nested, or file symlinks Rejected: Invalidate projections only from /api/skills/reload | sandbox acquisition must also detect edits made directly in external targets Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep target-tree traversal limited to the storage paths that explicitly permit external package-directory links Tested: 77 projection, user-scoped storage, and lifecycle tests passed; Ruff check and format check passed; git diff --check passed; GitNexus staged detection reported low risk Not-tested: Real external symlink execution on this Windows host without SeCreateSymbolicLinkPrivilege; existing tests skip that platform limitation
This commit is contained in:
georgelichen
GitHub
parent
cc6a2657e7
commit
641a4147e7
@ -890,7 +890,7 @@ than a green status hiding a later `command not found`.
|
||||
|
||||
If a trusted operator manages the configured skills directory through an external mount such as MinIO, NFS, or CSI, an administrator can call `POST /api/skills/reload` after changing files. This invalidates skill prompt caches for the current Gateway process and waits up to the bounded refresh timeout so subsequent runs rescan the latest files; running tasks are unchanged. A loader-level filesystem failure returns a generic server error and preserves the last successfully loaded process cache rather than publishing an empty catalog. Uvicorn workers and Kubernetes Pods must each be targeted separately. Direct mount writes bypass the validation, SkillScan, and history applied by DeerFlow's install/edit APIs, so only operator-controlled systems should have write access.
|
||||
|
||||
Skill installs and agent-managed skill edits run through **SkillScan**, a native deterministic safety scanner before the LLM-based skill scanner. Phase 1 runs offline with no Semgrep/OpenGrep dependency, blocks high-confidence `CRITICAL` findings such as private keys or shell execution, and passes warning findings to the LLM scanner for contextual review. Python instance-client exfiltration checks follow a minimal same-scope evidence chain: a simple name bound to a known client constructor, optional name-to-name aliases, and an actual outbound method or context-manager use supported by that constructor. Constructor roots must be proven imports; bare canonical-looking names are not inferred as modules. Nested scopes do not inherit client handles and inherit only constructor import aliases that are never rebound in the enclosing scope. Comprehensions, walrus-bearing statements, annotations, complex binding targets, unsupported operations, and ambiguous branch flows produce no finding from this signal; skipped constructs conservatively invalidate every name they may bind so stale client state cannot create a finding. A deterministic work budget or recursion limit reached by this best-effort analysis does not discard findings already collected for the file. Set `skill_scan.enabled: false` in `config.yaml` to disable only the deterministic analyzers; safe archive extraction and the LLM scanner still run.
|
||||
Skill installs and agent-managed skill edits run through **SkillScan**, a native deterministic safety scanner before the LLM-based skill scanner. Phase 1 runs offline with no Semgrep/OpenGrep dependency, blocks high-confidence `CRITICAL` findings such as private keys or shell execution, and passes warning findings to the LLM scanner for contextual review. The moderation adapter normalizes both plain-text model responses and LangChain Responses API text blocks before parsing the required JSON decision. Python instance-client exfiltration checks follow a minimal same-scope evidence chain: a simple name bound to a known client constructor, optional name-to-name aliases, and an actual outbound method or context-manager use supported by that constructor. Constructor roots must be proven imports; bare canonical-looking names are not inferred as modules. Nested scopes do not inherit client handles and inherit only constructor import aliases that are never rebound in the enclosing scope. Comprehensions, walrus-bearing statements, annotations, complex binding targets, unsupported operations, and ambiguous branch flows produce no finding from this signal; skipped constructs conservatively invalidate every name they may bind so stale client state cannot create a finding. A deterministic work budget or recursion limit reached by this best-effort analysis does not discard findings already collected for the file. Set `skill_scan.enabled: false` in `config.yaml` to disable only the deterministic analyzers; safe archive extraction and the LLM scanner still run.
|
||||
|
||||
DeerFlow also ships with **skill-reviewer**, a public skill for read-only skill quality review. It uses the built-in `review_skill_package` tool to inspect installed skills, local packages, archives, or pasted `SKILL.md` content without activating the target skill, binding its secrets, executing its scripts, or installing it. The tool returns a compact, tag-neutralized JSON payload to the model context and keeps the full raw review payload in the tool artifact for programmatic consumers. The deterministic review core reuses DeerFlow parsing and SkillScan facts, emits versioned JSON contracts under `contracts/skill_review/`, and can be run from the backend CLI:
|
||||
|
||||
|
||||
@ -2,7 +2,7 @@
|
||||
|
||||
- **Location**: global public skills live under `deer-flow/skills/public/`; user-authored custom skills live under `{DEER_FLOW_HOME}/users/{user_id}/skills/custom/`; globally managed integration skills live under `{DEER_FLOW_HOME}/integrations/skills/{provider}/`; per-user integration credentials remain under `{DEER_FLOW_HOME}/users/{user_id}/integrations/{provider}/{config,data}`
|
||||
- **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools, required-secrets)
|
||||
- **Loading**: `load_skills()` recursively scans public, per-user custom, global integration, and legacy custom locations for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json plus per-user skill state for non-public categories; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. SkillScan has a deliberately narrower packaging rule: known eval fixtures are permitted as support data, while other nested `SKILL.md` files are reported as package defects. It parses runtime metadata and reads enabled state from extensions_config.json.
|
||||
- **Loading**: `load_skills()` recursively scans public, per-user custom, global integration, and legacy custom locations for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json plus per-user skill state for non-public categories; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. A custom skill directory may be a one-level symlink to an external directory for compatibility with operator-managed skill trees; activation still rejects a symlinked `SKILL.md` or deeper path escape. SkillScan has a deliberately narrower packaging rule: known eval fixtures are permitted as support data, while other nested `SKILL.md` files are reported as package defects. It parses runtime metadata and reads enabled state from extensions_config.json.
|
||||
- **External reload**: `POST /api/skills/reload` is an admin-only, process-local invalidation hook for trusted MinIO/NFS/CSI writes. `SkillStorage` instances do not cache a catalog — `load_skills()` scans on every call — so the route clears all `(app_config, user_id)` entries and the rendered prompt-section LRU, then waits up to the shared refresh timeout for the existing off-loop single-flight refresh. Each invalidation receives a generation-bound result handle; a successful scan atomically replaces the global enabled-skills cache, while a loader-level failure propagates to the HTTP waiter and preserves the last-known-good global cache. Per-user/config scans capture the refresh version and cannot repopulate shared caches if invalidation occurs while they are loading. A timed-out HTTP wait fails generically while the daemon refresh worker continues. Subsequent runs rescan after a successful reload; active runs keep their existing snapshot. Each Uvicorn worker/Kubernetes Pod must be targeted separately. Direct mount writes bypass install/edit validation, SkillScan, and history, so mounted roots are an operator-controlled trust boundary.
|
||||
- **Tool policy**: Agent `allowed-tools` declarations apply dynamically only to slash-activated skills and skills captured in `ThreadState.skill_context` through configured `read_file` loads; passive enabled skills and custom-agent/subagent skill allowlists remain discoverable without clamping the baseline toolset. Subagents render only skill discovery metadata at startup and reuse the same adjacent `SkillActivationMiddleware` + `SkillToolPolicyMiddleware` pair as the lead; their configured `skills` field limits discovery and activation instead of eagerly loading bodies or unioning policies. Slash policy is dominant for its run, preventing subsequently read skills from widening explicit authority; autonomous captured skills use the existing union only when no slash source exists. `tool_search` and `describe_skill` stay available as framework discovery infrastructure, while every discovered or promoted business tool still requires active-policy permission for schema visibility and execution; `task`, `list_background_tasks`, and `cancel_background_task` likewise require explicit declarations. Each active model call intentionally reloads the full live registry so enable/disable changes, frontmatter edits, and custom/public name-shadow winners take effect without a stale TTL or unsafe direct-path cache; all tool calls produced by that model step reuse the resulting source-and-path-signed decision. Registry failures and all-invalid active sets fail closed, while stale individual paths are skipped when another valid skill remains. This is best-effort behavioral scoping, not a hard security boundary: alternate loading paths are not captured and bounded autonomous context may evict entries.
|
||||
- **Sandbox projection**: `skills/projection.py` materializes enabled-only trees at `{base_dir}/skills_view/public` and `{base_dir}/users/{user_id}/skills_view/{custom,legacy,integrations}`. It copies files into the view (`_copy_into_view`) so a sandbox write cannot mutate the canonical skill inode; the operational trade-off is an O(total bytes) I/O and per-user storage multiplier across rebuilds, prioritized for write isolation over zero-copy hardlinks. Steady-state freshness checks combine source and view metadata tree digests, so in-sandbox view tampering is detected and repaired on the next acquire. Storage writes, archive installs, deletes, and toggles rebuild under a cross-process lock; Gateway boot ensures only the shared public view, while each user view is repaired lazily on first sandbox acquire. Managed integration packages are global, but their projected category is per-user because enabled state is isolated. Rebuilds stage a complete tree and reconcile it with per-file atomic replacement, so unrelated enabled skills remain continuously visible; disable/delete paths remove only the affected package before mutating to preserve fail-closed behavior. User projection rebuilds re-read global enable state from disk instead of the process singleton, so a toggle handled by another Gateway worker is reflected on the next acquire. Gateway public-skill toggles take the public projection lock before the shared `extensions_config_write_lock`, re-read an existing config from disk, persist the full model shape, and rebuild before responding; keep this as one worker-owned critical section so MCP writes cannot interleave and request cancellation cannot release either lock while the worker still runs. The shared public steady-state signature check runs without the global projection lock; stale/error paths take the lock and re-check before rebuilding or clearing. User-scope checks remain serialized per user. Category root inodes remain stable so live bind mounts observe content changes without sandbox recreation. Projection failures clear the affected view before raising.
|
||||
@ -13,7 +13,7 @@
|
||||
- **Slash activation**: `/skill-name task` loads that enabled skill's `SKILL.md` for the current model call only. The resolver rejects leading whitespace, missing separators, reserved channel commands (`/new`, `/help`, `/bootstrap`, `/status`, `/models`, `/memory`, `/goal`), disabled skills, and skills outside a custom agent's whitelist.
|
||||
- **Installation**: `POST /api/skills/install` extracts .skill ZIP archive to custom/ directory
|
||||
- **Managed integrations**: Lark/Feishu CLI support installs one global official `lark-*` pack as read-only `SkillCategory.INTEGRATION` entries under `/mnt/skills/integrations/lark-cli/...`; enabled flags, app configuration, and OAuth data remain per-user. Install resolves the newest `larksuite/cli` release from GitHub (`releases/latest`) at install time (falling back to a bottom-line pinned version if the lookup fails) rather than hard-coding the pack version; integrity relies on the official host + structural archive guards + a recorded hash of the effective installed tree after shared guidance injection (not a pinned archive-byte SHA, which GitHub does not keep stable). The Gateway image still installs a pinned `@larksuite/cli` binary, so `get_lark_integration_status` surfaces `latest_available_version` and `runtime_version_mismatch` for the UI. AIO installs additionally verify and publish official Linux amd64/arm64 binaries under `{DEER_FLOW_HOME}/integrations/lark-cli/sandbox-cli`, mounted read-only at `/mnt/integrations/lark-cli/runtime`; `/mnt/integrations/lark-cli/config` (app credentials, incl. the long-lived `appSecret`) is mounted **read-only** into the sandbox, its empty `config/locks` subdirectory is over-mounted writable for `lark-cli` coordination files, and `/mnt/integrations/lark-cli/data` (refreshable OAuth tokens) stays writable, all mapping to owner-only per-user directories. **Sandbox trust boundary:** the credential-bearing config and data dirs are still *readable* by arbitrary sandbox processes (the agent's `bash` tool, or code reached via prompt-injection in a tool result), so the app secret and tokens are exposed to sandbox-side code even though they never reach the browser — the read-only config mount only prevents in-sandbox tampering, not read/exfiltration. The sidecar credential-broker (Pattern B, issue #4338) is the fix that removes these plaintext mounts from sandbox execution: set `LARK_CLI_BROKER_IMAGE` on the provisioner (see `docker/lark-cli-broker/`) and the Gateway sends `provision_lark_cli_broker` on sandbox create. The provisioner then runs a `lark-cli-broker` sidecar that owns the per-user `config`/`config/locks`/`data` mounts (mounted into the **sidecar only**, at `/var/lark/{config,config/locks,data}` with only the nested locks mount writable) and serves the `lark-cli` command surface on Pod loopback (`http://127.0.0.1:8788`); a shim init container (`install-shim`) writes a forwarding `lark-cli` into the shared runtime `emptyDir`, so the sandbox gets `DEERFLOW_LARK_BROKER_URL` + a shim on PATH but **no** credential files. The on-PATH `bin/lark-cli` is a `/bin/sh` launcher that resolves a Python 3 interpreter and execs the Python shim body (`bin/lark-cli-shim.py`) beside it, so broker mode does not silently ENOEXEC on a sandbox image without a `#!/usr/bin/env python3`-resolvable interpreter — it fails loudly (exit 127, actionable message) and can be pinned with `DEERFLOW_LARK_BROKER_PYTHON`. The broker runs `lark-cli` in the sidecar's cwd and cannot see the sandbox filesystem, so cwd is intentionally **not** forwarded and file-I/O subcommands relative to the sandbox cwd are unsupported (command surface only). An optional `DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS` denylist (comma-separated command prefixes, forwarded from the provisioner) lets the broker refuse secret-dumping subcommands before spawning the binary. `lark_cli_env_overlay(broker=True)` therefore omits `LARKSUITE_CLI_CONFIG_DIR`/`DATA_DIR`; `sandbox_lark_broker_active()` (TTL-cached provisioner `/api/capabilities` probe, tight timeout + longer negative caching on the bash hot path) selects broker vs. binary mode for both the bash env overlay and status. `DEER_FLOW_LARK_CLI_SANDBOX_RUNTIME_DIR` supplies a validated, symlink-free pre-staged runtime for air-gapped deployments. For the remote provisioner (K8s), the runtime binary is otherwise provisioned by an optional init container + shared `emptyDir` (Pattern A): set `LARK_CLI_INIT_IMAGE` on the provisioner (see `docker/lark-cli-init/`) and the Gateway sends `provision_lark_cli_runtime` on sandbox create once the pack is installed, so remote installs skip the Gateway-side GitHub download entirely. Broker (Pattern B) supersedes the init-container binary (Pattern A) when both images are configured. `get_lark_integration_status(check_runtime=True)` surfaces `sandbox_runtime_mode` (`none` / `gateway-download` / `init-container` / `broker`) and `sandbox_runtime_ready` (remote modes read the provisioner `GET /api/capabilities`: `lark_cli_init_image` / `lark_cli_broker_image`) so a green UI can't hide a chat-time `lark-cli: command not found`. Cheap status probes are explicitly not live-verified; users authorize or reconnect through the browser device-flow endpoints instead of running terminal commands.
|
||||
- **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()`. `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. 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**: `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. 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`.
|
||||
- **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 `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`.
|
||||
|
||||
#### Request-Scoped Secrets (`required-secrets`)
|
||||
|
||||
@ -216,7 +216,13 @@ def _clear_projection_scope(scope_root: Path, *category_roots: Path) -> None:
|
||||
_manifest_path(scope_root).unlink(missing_ok=True)
|
||||
|
||||
|
||||
def _update_tree_digest(digest, root: Path, label: str) -> None:
|
||||
def _update_tree_digest(
|
||||
digest,
|
||||
root: Path,
|
||||
label: str,
|
||||
*,
|
||||
follow_package_directory_symlinks: bool = False,
|
||||
) -> None:
|
||||
"""Hash directory metadata (inode/mode/size/mtime), not file contents.
|
||||
|
||||
Trade-off: fast enough to run on every sandbox acquire (O(files), no
|
||||
@ -225,6 +231,11 @@ def _update_tree_digest(digest, root: Path, label: str) -> None:
|
||||
projection stale until the next explicit rebuild. Runtime writes through
|
||||
this codebase are covered regardless: the mutation path rebuilds under
|
||||
lock, and atomic-rename always changes the inode.
|
||||
|
||||
Custom skill roots may contain an operator-managed package directory
|
||||
symlink. Follow only those links directly below the category root so
|
||||
changes in their external target tree invalidate the projection, while
|
||||
nested and unrelated symlinks remain boundary markers.
|
||||
"""
|
||||
digest.update(f"root:{label}\0".encode())
|
||||
if not root.exists():
|
||||
@ -242,6 +253,9 @@ def _update_tree_digest(digest, root: Path, label: str) -> None:
|
||||
metadata = entry.stat(follow_symlinks=False)
|
||||
if entry.is_symlink():
|
||||
kind = "link"
|
||||
if follow_package_directory_symlinks and relative_root == Path(".") and entry.is_dir(follow_symlinks=True):
|
||||
digest.update(f"{label}:{relative.as_posix()}:target:{Path(entry.path).resolve(strict=False)}\0".encode())
|
||||
child_dirs.append((Path(entry.path), relative))
|
||||
elif entry.is_dir(follow_symlinks=False):
|
||||
kind = "dir"
|
||||
child_dirs.append((Path(entry.path), relative))
|
||||
@ -267,8 +281,18 @@ def _source_signature(storage: SkillStorage, scope: str) -> str:
|
||||
elif scope == "user":
|
||||
user_custom_root = storage.get_user_custom_root()
|
||||
integration_root = storage.get_user_integrations_root()
|
||||
_update_tree_digest(digest, user_custom_root, "custom")
|
||||
_update_tree_digest(digest, host_root / SkillCategory.CUSTOM.value, "legacy")
|
||||
_update_tree_digest(
|
||||
digest,
|
||||
user_custom_root,
|
||||
"custom",
|
||||
follow_package_directory_symlinks=True,
|
||||
)
|
||||
_update_tree_digest(
|
||||
digest,
|
||||
host_root / SkillCategory.CUSTOM.value,
|
||||
"legacy",
|
||||
follow_package_directory_symlinks=True,
|
||||
)
|
||||
_update_tree_digest(digest, integration_root, "integrations")
|
||||
# CUSTOM/LEGACY/INTEGRATION visibility is the intersection of the
|
||||
# per-user state and the global extensions default, so both belong in
|
||||
|
||||
@ -15,6 +15,7 @@ from deerflow.models import create_chat_model
|
||||
from deerflow.runtime.user_context import get_effective_user_id
|
||||
from deerflow.skills.types import SKILL_MD_FILE
|
||||
from deerflow.tracing import inject_langfuse_metadata
|
||||
from deerflow.utils.llm_text import extract_response_text
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@ -155,7 +156,7 @@ async def scan_skill_content(
|
||||
config=invoke_config,
|
||||
)
|
||||
model_responded = True
|
||||
raw = str(getattr(response, "content", "") or "")
|
||||
raw = extract_response_text(getattr(response, "content", ""))
|
||||
parsed = _extract_json_object(raw)
|
||||
if parsed:
|
||||
decision = str(parsed.get("decision", "")).lower()
|
||||
|
||||
@ -78,6 +78,23 @@ class SkillStorage(ABC):
|
||||
if parsed_name != name:
|
||||
raise ValueError(f"Frontmatter name '{parsed_name}' must match requested skill name '{name}'.")
|
||||
|
||||
@staticmethod
|
||||
def _is_external_skill_directory_symlink(skill_file: Path, custom_root: Path) -> bool:
|
||||
"""Allow one-level package-directory links without allowing file links.
|
||||
|
||||
The storage contract permits an externally managed skill package to be
|
||||
linked directly below the supplied custom-skill category root. Preserve
|
||||
that compatibility while rejecting arbitrary path escapes and symlinked
|
||||
``SKILL.md`` files.
|
||||
"""
|
||||
if skill_file.is_symlink() or not skill_file.parent.is_symlink():
|
||||
return False
|
||||
try:
|
||||
relative_parent = skill_file.parent.relative_to(custom_root)
|
||||
except ValueError:
|
||||
return False
|
||||
return len(relative_parent.parts) == 1 and skill_file.parent.resolve().is_dir()
|
||||
|
||||
def ensure_safe_support_path(self, name: str, relative_path: str) -> Path:
|
||||
"""Validate and return the resolved absolute path for a support file."""
|
||||
_ALLOWED_SUPPORT_SUBDIRS = {"references", "templates", "scripts", "assets"}
|
||||
@ -122,14 +139,23 @@ class SkillStorage(ABC):
|
||||
under the per-user custom root, because custom skills are stored in a
|
||||
separate directory tree that is not a sub-path of the global root.
|
||||
|
||||
A one-level symlinked package directory directly below the configured
|
||||
custom-skill category root is also accepted for operator-managed
|
||||
external skills; the final ``SKILL.md`` file itself must not be a
|
||||
symlink.
|
||||
|
||||
Raises:
|
||||
ValueError: if the resolved path escapes all allowed roots.
|
||||
"""
|
||||
if skill_file.is_symlink():
|
||||
raise ValueError("Resolved skill file must stay within the configured skills root.")
|
||||
resolved_file = skill_file.resolve()
|
||||
resolved_root = self.get_skills_root_path().resolve()
|
||||
try:
|
||||
resolved_file.relative_to(resolved_root)
|
||||
except ValueError as exc:
|
||||
if self._is_external_skill_directory_symlink(skill_file, self.get_skills_root_path() / SkillCategory.CUSTOM.value):
|
||||
return resolved_file
|
||||
raise ValueError("Resolved skill file must stay within the configured skills root.") from exc
|
||||
return resolved_file
|
||||
|
||||
|
||||
@ -412,7 +412,12 @@ class UserScopedSkillStorage(LocalSkillStorage):
|
||||
|
||||
Custom and managed integration skills live outside ``_host_root``, so
|
||||
the default implementation's single-root check would reject them.
|
||||
One-level package-directory symlinks under either configured custom-skill
|
||||
category root retain the operator-managed external-skill compatibility
|
||||
of the base storage.
|
||||
"""
|
||||
if skill_file.is_symlink():
|
||||
raise ValueError(f"Resolved skill file {skill_file} must stay within the configured skill roots and cannot be a symlink.")
|
||||
resolved_file = skill_file.resolve()
|
||||
allowed_roots = (
|
||||
self._host_root.resolve(),
|
||||
@ -425,6 +430,8 @@ class UserScopedSkillStorage(LocalSkillStorage):
|
||||
return resolved_file
|
||||
except ValueError:
|
||||
continue
|
||||
if any(self._is_external_skill_directory_symlink(skill_file, custom_root) for custom_root in (self._user_custom_root, self._global_custom_root)):
|
||||
return resolved_file
|
||||
raise ValueError(
|
||||
f"Resolved skill file {resolved_file} must stay within the global skills root "
|
||||
f"({self._host_root.resolve()}), the per-user custom root "
|
||||
|
||||
@ -115,6 +115,37 @@ async def test_scan_skill_content_passes_run_name_to_model(monkeypatch):
|
||||
assert model.kwargs["config"] == {"run_name": "security_agent"}
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_scan_skill_content_parses_responses_api_text_blocks(monkeypatch):
|
||||
_make_env(
|
||||
monkeypatch,
|
||||
[{"type": "text", "text": '{"decision":"allow","reason":"clean"}'}],
|
||||
)
|
||||
|
||||
result = await scan_skill_content(SKILL_CONTENT, executable=False)
|
||||
|
||||
assert result.decision == "allow"
|
||||
assert result.reason == "clean"
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_scan_skill_content_ignores_non_text_blocks_and_joins_text_blocks(monkeypatch):
|
||||
_make_env(
|
||||
monkeypatch,
|
||||
[
|
||||
{"type": "reasoning", "text": '{"decision":"block","reason":"fake"}'},
|
||||
{"type": "text", "text": '{"decision":"allow",'},
|
||||
{"type": "output_text", "text": '"reason":"clean"}'},
|
||||
{"type": "tool_call", "text": '{"decision":"block","reason":"fake"}'},
|
||||
],
|
||||
)
|
||||
|
||||
result = await scan_skill_content(SKILL_CONTENT, executable=False)
|
||||
|
||||
assert result.decision == "allow"
|
||||
assert result.reason == "clean"
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_scan_skill_content_blocks_when_model_unavailable(monkeypatch):
|
||||
config = SimpleNamespace(skill_evolution=SimpleNamespace(moderation_model_name=None))
|
||||
|
||||
@ -144,6 +144,29 @@ def test_atomic_custom_skill_rewrite_refreshes_projection(projection_env) -> Non
|
||||
assert target.stat().st_ino != old_inode
|
||||
|
||||
|
||||
def test_external_custom_skill_directory_target_update_refreshes_projection(projection_env, tmp_path: Path) -> None:
|
||||
env = projection_env
|
||||
external_skill_dir = tmp_path / "external-skills" / "linked-skill"
|
||||
source = _write_skill(external_skill_dir.parent, "linked-skill", "before")
|
||||
linked_skill_dir = env.storage.get_user_custom_root() / "linked-skill"
|
||||
linked_skill_dir.parent.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
linked_skill_dir.symlink_to(external_skill_dir, target_is_directory=True)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
projected = rebuild_skill_projections(env.storage)
|
||||
target = projected.custom / "linked-skill" / "SKILL.md"
|
||||
assert "before" in target.read_text(encoding="utf-8")
|
||||
|
||||
source.write_text(_skill_content("linked-skill", "after"), encoding="utf-8")
|
||||
ensure_skill_projections(env.storage)
|
||||
|
||||
assert "after" in target.read_text(encoding="utf-8")
|
||||
|
||||
|
||||
def test_custom_content_write_keeps_unrelated_skill_visible_during_rebuild(projection_env, monkeypatch) -> None:
|
||||
env = projection_env
|
||||
env.storage.write_custom_skill("alpha", "SKILL.md", _skill_content("alpha"))
|
||||
|
||||
@ -3,8 +3,11 @@
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
from deerflow.config.skills_config import SkillsConfig
|
||||
from deerflow.skills.storage import get_or_new_skill_storage
|
||||
from deerflow.skills.storage.local_skill_storage import LocalSkillStorage
|
||||
|
||||
|
||||
def _write_skill(skill_dir: Path, name: str, description: str) -> None:
|
||||
@ -63,6 +66,27 @@ def test_load_skills_discovers_nested_skills_and_sets_container_paths(tmp_path:
|
||||
assert team_skill.get_container_file_path() == "/mnt/skills/custom/team/helper/SKILL.md"
|
||||
|
||||
|
||||
def test_local_storage_accepts_external_custom_skill_directory_symlink(tmp_path: Path):
|
||||
skills_root = tmp_path / "skills"
|
||||
external_file = tmp_path / "external-skills" / "external-skill" / "SKILL.md"
|
||||
external_file.parent.mkdir(parents=True)
|
||||
external_file.write_text("---\nname: external-skill\ndescription: An external skill\n---\n", encoding="utf-8")
|
||||
|
||||
linked_dir = skills_root / "custom" / "external-skill"
|
||||
linked_file = linked_dir / "SKILL.md"
|
||||
linked_dir.parent.mkdir(parents=True)
|
||||
try:
|
||||
linked_dir.symlink_to(external_file.parent, target_is_directory=True)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
storage = LocalSkillStorage(host_path=str(skills_root))
|
||||
|
||||
assert storage.validate_skill_file_path(linked_file) == external_file
|
||||
|
||||
|
||||
def test_load_skills_stops_at_skill_package_boundary(tmp_path: Path):
|
||||
"""SKILL.md files inside an existing skill package are support data, not skills."""
|
||||
skills_root = tmp_path / "skills"
|
||||
|
||||
@ -3,6 +3,7 @@ import hashlib
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
from langchain.agents.middleware.types import ModelRequest
|
||||
from langchain_core.messages import AIMessage, HumanMessage, ToolMessage
|
||||
|
||||
@ -187,6 +188,59 @@ def test_skill_activation_middleware_reads_public_skill_from_real_user_scoped_st
|
||||
assert user_msg is original
|
||||
|
||||
|
||||
def test_skill_activation_middleware_reads_external_custom_skill_directory_symlink(monkeypatch, tmp_path):
|
||||
skills_root = tmp_path / "skills"
|
||||
skill_dir = tmp_path / "external-skills" / "external-skill"
|
||||
skill_dir.mkdir(parents=True)
|
||||
skill_content = "---\nname: external-skill\ndescription: An external skill\n---\n\n# External skill\n"
|
||||
(skill_dir / "SKILL.md").write_text(skill_content, encoding="utf-8")
|
||||
|
||||
user_custom_root = tmp_path / "users" / "test-user" / "skills" / "custom"
|
||||
user_custom_root.mkdir(parents=True)
|
||||
try:
|
||||
(user_custom_root / "external-skill").symlink_to(skill_dir, target_is_directory=True)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
app_config = SimpleNamespace(
|
||||
skills=SimpleNamespace(
|
||||
get_skills_path=lambda: skills_root,
|
||||
container_path="/mnt/skills",
|
||||
use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage",
|
||||
),
|
||||
)
|
||||
extensions_config = ExtensionsConfig()
|
||||
monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path))
|
||||
monkeypatch.setattr(ExtensionsConfig, "from_file", classmethod(lambda cls, config_path=None: extensions_config))
|
||||
monkeypatch.setattr("deerflow.config.extensions_config.get_extensions_config", lambda: extensions_config)
|
||||
|
||||
storage = UserScopedSkillStorage("test-user", host_path=str(skills_root), app_config=app_config)
|
||||
monkeypatch.setattr(middleware_module, "get_or_new_user_skill_storage", lambda user_id, **kwargs: storage)
|
||||
|
||||
middleware = SkillActivationMiddleware(
|
||||
app_config=app_config,
|
||||
user_id="test-user",
|
||||
slash_source_owner_token=_SLASH_SOURCE_OWNER_TOKEN,
|
||||
)
|
||||
original = HumanMessage(content="/external-skill Run the external skill", id="msg-external-symlink")
|
||||
request = _make_model_request([original])
|
||||
captured = {}
|
||||
|
||||
def handler(model_request: ModelRequest):
|
||||
captured["messages"] = model_request.messages
|
||||
return AIMessage(content="ok")
|
||||
|
||||
result = middleware.wrap_model_call(request, handler)
|
||||
|
||||
assert isinstance(result, AIMessage)
|
||||
assert result.content == "ok"
|
||||
activation_msg, user_msg = captured["messages"]
|
||||
assert "# External skill" in activation_msg.content
|
||||
assert user_msg is original
|
||||
|
||||
|
||||
def test_skill_activation_middleware_does_not_duplicate_existing_activation(monkeypatch, tmp_path):
|
||||
skill = _make_skill(tmp_path, "data-analysis", content="# Data Analysis\nUse pandas.")
|
||||
monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill]))
|
||||
@ -727,7 +781,12 @@ def test_skill_activation_middleware_rejects_skill_file_outside_skills_root(monk
|
||||
outside_dir.mkdir()
|
||||
outside_file = outside_dir / "SKILL.md"
|
||||
outside_file.write_text("# Leaked\nDo not read me.", encoding="utf-8")
|
||||
(skill_dir / "SKILL.md").symlink_to(outside_file)
|
||||
try:
|
||||
(skill_dir / "SKILL.md").symlink_to(outside_file)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
skill = Skill(
|
||||
name="data-analysis",
|
||||
description="Description for data-analysis",
|
||||
|
||||
@ -308,6 +308,80 @@ class TestPathSafety:
|
||||
with pytest.raises(ValueError, match="must stay within"):
|
||||
user_storage.validate_skill_file_path(skill_file)
|
||||
|
||||
def test_accepts_external_skill_directory_symlink_but_not_file_symlink(self, user_storage: UserScopedSkillStorage, tmp_path: Path):
|
||||
external_file = tmp_path / "external-skills" / "external-skill" / "SKILL.md"
|
||||
external_file.parent.mkdir(parents=True)
|
||||
external_file.write_text(_skill_content("external-skill"), encoding="utf-8")
|
||||
|
||||
linked_dir = user_storage.get_user_custom_root() / "external-skill"
|
||||
linked_file = linked_dir / "SKILL.md"
|
||||
linked_dir.parent.mkdir(parents=True)
|
||||
try:
|
||||
linked_dir.symlink_to(external_file.parent, target_is_directory=True)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
assert user_storage.validate_skill_file_path(linked_file) == external_file
|
||||
|
||||
file_link = user_storage.get_user_custom_root() / "file-link" / "SKILL.md"
|
||||
file_link.parent.mkdir(parents=True)
|
||||
try:
|
||||
file_link.symlink_to(external_file)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
with pytest.raises(ValueError, match="must stay within"):
|
||||
user_storage.validate_skill_file_path(file_link)
|
||||
|
||||
def test_rejects_file_symlink_even_when_target_stays_inside_allowed_root(self, user_storage: UserScopedSkillStorage, tmp_path: Path):
|
||||
target_file = user_storage.get_user_custom_root() / "real-skill" / "SKILL.md"
|
||||
target_file.parent.mkdir(parents=True)
|
||||
target_file.write_text(_skill_content("real-skill"), encoding="utf-8")
|
||||
linked_file = user_storage.get_user_custom_root() / "alias-skill" / "SKILL.md"
|
||||
linked_file.parent.mkdir(parents=True)
|
||||
try:
|
||||
linked_file.symlink_to(target_file)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
with pytest.raises(ValueError, match="must stay within"):
|
||||
user_storage.validate_skill_file_path(linked_file)
|
||||
|
||||
def test_rejects_deeper_and_non_custom_directory_symlinks(self, user_storage: UserScopedSkillStorage, skills_root: Path, tmp_path: Path):
|
||||
external_dir = tmp_path / "external-skills" / "nested"
|
||||
external_dir.mkdir(parents=True)
|
||||
external_file = external_dir / "SKILL.md"
|
||||
external_file.write_text(_skill_content("nested-skill"), encoding="utf-8")
|
||||
|
||||
deep_parent = user_storage.get_user_custom_root() / "outer"
|
||||
deep_parent.mkdir(parents=True)
|
||||
deep_link = deep_parent / "link"
|
||||
try:
|
||||
deep_link.symlink_to(external_dir, target_is_directory=True)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
public_link = skills_root / SkillCategory.PUBLIC.value / "external-skill"
|
||||
public_link.parent.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
public_link.symlink_to(external_dir, target_is_directory=True)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("Windows symlink creation requires SeCreateSymbolicLinkPrivilege")
|
||||
raise
|
||||
|
||||
with pytest.raises(ValueError, match="must stay within"):
|
||||
user_storage.validate_skill_file_path(deep_link / "SKILL.md")
|
||||
with pytest.raises(ValueError, match="must stay within"):
|
||||
user_storage.validate_skill_file_path(public_link / "SKILL.md")
|
||||
|
||||
def test_rejects_invalid_skill_name(self, user_storage: UserScopedSkillStorage):
|
||||
with pytest.raises(ValueError, match="hyphen-case"):
|
||||
user_storage.get_custom_skill_dir("../../escaped")
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user