From bc4a33aba79f0521aa4fb800a514e456b562cd43 Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Sat, 12 Sep 2026 12:24:46 +0900 Subject: [PATCH] fix(skills): stop persisting resolved secrets when toggling skills (#5357) Toggling a skill wrote resolved secrets into extensions_config.json. The Gateway skill toggle and DeerFlowClient.update_skill loaded the file with ExtensionsConfig.from_file(), which replaces every "$VAR" string with the environment value (and an unset variable with ""), then serialized that model back through to_file_dict(). A "$GITHUB_TOKEN" reference was persisted as the plaintext token, and an unset reference was erased for good. DeerFlowClient.update_mcp_config had the same flaw for every key other than mcpServers. Every writer now does a raw read-modify-write, the way the MCP router already did: read_raw_extensions_config reads the on-disk JSON, set_raw_skill_enabled changes only the target entry, and validate_raw_extensions_config checks the candidate the way the runtime will load it before the atomic write. When the file does not exist yet, the Gateway seeds it with the cached skill states only, never the resolved cached model. The MCP router's raw loader and candidate validation delegate to the same helpers, so the rule lives in one place, and to_file_dict() is removed so the unsafe serialization has no entry point left. Co-authored-by: Willem Jiang --- CHANGELOG.md | 12 ++ CHANGELOG_zh.md | 9 + backend/app/gateway/routers/mcp.py | 19 +- backend/app/gateway/routers/skills.py | 21 ++- .../deerflow/agents/middlewares/AGENTS.md | 2 +- backend/packages/harness/deerflow/client.py | 42 +++-- .../harness/deerflow/config/AGENTS.md | 2 + .../deerflow/config/extensions_config.py | 48 ++++- .../harness/deerflow/skills/AGENTS.md | 2 +- .../blocking_io/test_skills_update_router.py | 13 +- backend/tests/test_client.py | 83 +++++++++ backend/tests/test_configured_extensions.py | 44 +++-- .../test_extensions_config_raw_writes.py | 176 ++++++++++++++++++ backend/tests/test_skills_custom_router.py | 4 +- 14 files changed, 411 insertions(+), 66 deletions(-) create mode 100644 backend/tests/test_extensions_config_raw_writes.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cfaf3b60..23180fe44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -568,6 +568,17 @@ This section accumulates work toward the **2.1.0** milestone ### Fixed +- **skills:** Stop writing resolved secrets into `extensions_config.json` when a + skill is toggled. The Gateway skill toggle and `DeerFlowClient.update_skill` + loaded the file through `ExtensionsConfig.from_file()`, which replaces every + `$VAR` value with the environment value, and wrote that model back — so a + `"$GITHUB_TOKEN"` reference was persisted as the plaintext token and an unset + variable was permanently replaced with `""`. `DeerFlowClient.update_mcp_config` + did the same for every key other than `mcpServers`. These writers now edit the + raw on-disk JSON and validate the candidate the way the runtime loads it, so + placeholders and hand-written structure survive; the MCP router shares the same + raw loader. Files rewritten by an earlier toggle keep their plaintext values: + restore the `$VAR` references and rotate the exposed credentials. ([#5357]) - **gateway:** Honor `disable_clarification` and `github_token` only for internally-authenticated callers, the way `non_interactive` already was. Both keys were forwarded from `body.context` regardless of the caller and @@ -2735,3 +2746,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5321]: https://github.com/bytedance/deer-flow/pull/5321 [#5338]: https://github.com/bytedance/deer-flow/pull/5338 [#5353]: https://github.com/bytedance/deer-flow/pull/5353 +[#5357]: https://github.com/bytedance/deer-flow/pull/5357 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 97b284353..e1fc405be 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -397,6 +397,14 @@ ### 修复 +- **Skills：** 切换 skill 启用状态时不再把解析后的密钥写入 `extensions_config.json`。 + 此前 Gateway 的 skill 开关与 `DeerFlowClient.update_skill` 通过 + `ExtensionsConfig.from_file()` 读取配置（该方法会把所有 `$VAR` 值替换为环境变量的 + 实际值），再把模型整体写回，于是 `"$GITHUB_TOKEN"` 引用会被持久化为明文令牌，未设置 + 的变量则被永久写成 `""`。`DeerFlowClient.update_mcp_config` 对 `mcpServers` 以外的 + 所有键也存在同样问题。现在这些写入方直接修改磁盘上的原始 JSON，并按运行时的加载方式 + 校验候选配置后再写入，占位符与手写结构保持不变；MCP 路由也复用同一个原始读取函数。 + 已被旧版本改写过的文件仍保留明文值，请恢复 `$VAR` 引用并轮换已暴露的凭据。([#5357]) - **Gateway：** `disable_clarification` 与 `github_token` 现在与 `non_interactive` 一样，仅对内部认证的调用方生效。此前这两个键无论调用方身份都会从 `body.context` 透传，而且不会从被逐字复制进 run config 的自由格式 `body.config` 中清除，因此任何 @@ -2110,3 +2118,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5321]: https://github.com/bytedance/deer-flow/pull/5321 [#5338]: https://github.com/bytedance/deer-flow/pull/5338 [#5353]: https://github.com/bytedance/deer-flow/pull/5353 +[#5357]: https://github.com/bytedance/deer-flow/pull/5357 diff --git a/backend/app/gateway/routers/mcp.py b/backend/app/gateway/routers/mcp.py index 577a0312b..a1bebcab0 100644 --- a/backend/app/gateway/routers/mcp.py +++ b/backend/app/gateway/routers/mcp.py @@ -1,5 +1,4 @@ import asyncio -import json import logging import os import re @@ -20,7 +19,9 @@ from deerflow.config.extensions_config import ( extensions_config_write_lock, get_extensions_config, normalize_mcp_transport_alias, + read_raw_extensions_config, reload_extensions_config, + validate_raw_extensions_config, ) from deerflow.config.runtime_paths import project_root from deerflow.constants import DEFAULT_MCP_SESSION_INIT_TIMEOUT @@ -1165,8 +1166,7 @@ def _mcp_server_response_from_raw(server_name: str, raw_server: Any) -> McpServe def _validate_extensions_config_candidate(raw_data: dict) -> None: """Reject a runtime-invalid candidate without changing its placeholders.""" try: - resolved_data = ExtensionsConfig.resolve_env_variables(raw_data) - ExtensionsConfig.model_validate(resolved_data) + validate_raw_extensions_config(raw_data) except ValidationError as exc: _raise_invalid_mcp_configuration(_validation_error_summary(exc), cause=exc) @@ -1296,16 +1296,9 @@ def _mcp_config_path(*, create: bool) -> Path: def _load_raw_extensions_config(config_path: Path, *, create: bool) -> dict: if config_path.exists(): try: - with open(config_path, encoding="utf-8") as f: - raw_data = json.load(f) - except json.JSONDecodeError as exc: - _raise_invalid_mcp_configuration( - f"Extensions configuration is not valid JSON: {exc.msg} at line {exc.lineno} column {exc.colno}", - cause=exc, - ) - if not isinstance(raw_data, dict): - _raise_invalid_mcp_configuration("Extensions configuration must be a JSON object") - return raw_data + return read_raw_extensions_config(config_path) + except ValueError as exc: + _raise_invalid_mcp_configuration(str(exc), cause=exc) if not create: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, diff --git a/backend/app/gateway/routers/skills.py b/backend/app/gateway/routers/skills.py index 02077e2f3..f54fd22ba 100644 --- a/backend/app/gateway/routers/skills.py +++ b/backend/app/gateway/routers/skills.py @@ -17,12 +17,14 @@ from deerflow.agents.lead_agent.prompt import clear_skills_system_prompt_cache, from deerflow.config.app_config import AppConfig from deerflow.config.extensions_config import ( ExtensionsConfig, - SkillStateConfig, atomic_write_extensions_config, extensions_config_file_lock, extensions_config_write_lock, get_extensions_config, + read_raw_extensions_config, reload_extensions_config, + set_raw_skill_enabled, + validate_raw_extensions_config, ) from deerflow.runtime.user_context import get_effective_user_id from deerflow.skills import Skill @@ -653,13 +655,18 @@ def _write_extensions_skill_state( with projection_update: with extensions_config_write_lock, extensions_config_file_lock(config_path): # The projection lock is cross-process, but the singleton cache is - # not. Existing files are therefore re-read under the lock; a new - # file starts from a deep snapshot of the cached defaults. - extensions_config = ExtensionsConfig.from_file(config_path) if config_path.exists() else get_extensions_config().model_copy(deep=True) - extensions_config.skills[skill_name] = SkillStateConfig(enabled=enabled) + # not. Existing files are therefore re-read under the lock, raw, so + # $VAR placeholders are not persisted as resolved secrets. A new + # file starts from the cached skill states only: the cached model + # holds resolved values and must never be serialized. + if config_path.exists(): + raw_config = read_raw_extensions_config(config_path) + else: + raw_config = {"skills": {name: {"enabled": state.enabled} for name, state in get_extensions_config().skills.items()}} + set_raw_skill_enabled(raw_config, skill_name, enabled) - config_data = extensions_config.to_file_dict() - atomic_write_extensions_config(config_path, config_data) + validate_raw_extensions_config(raw_config) + atomic_write_extensions_config(config_path, raw_config) logger.info(f"Skills configuration updated and saved to: {config_path}") reload_extensions_config() diff --git a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md index 0d828b9a3..cf28892e7 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md @@ -104,7 +104,7 @@ Before changing a later authorization phase, read the [authorization RFC](../../ fallback identity, cleanup/LRU/reset, severity ordering, and test invariants. 30. **TokenBudgetMiddleware** - *(optional, if `token_budget.enabled`)* Enforces per-run token limits 31. **Custom middlewares** - *(optional)* Any `custom_middlewares` passed to `build_middlewares` are injected here, before config-declared extensions and the terminal-response/safety/clarification tail -32. **Configured extension middlewares** - `extensions.middlewares` in `config.yaml` or `extensions_config.json` optionally accepts `module.path:ClassName` strings or `{class, kwargs}` objects. `deerflow.reflection.resolve_class` loads `AgentMiddleware` classes; import, class, and constructor errors fail agent creation. `kwargs` must be JSON-compatible; YAML dates/timestamps become ISO strings. Order: built-ins/custom and loop/token guards → extensions → terminal-response/safety/clarification tail. Subagents share the list before their safety tail; separate lead/subagent lists are unsupported. Trusted operator config only: paths instantiate arbitrary code. Gateway skill/MCP toggles preserve it via `to_file_dict()`; adding an API write path requires explicit trust-boundary review. +32. **Configured extension middlewares** - `extensions.middlewares` in `config.yaml` or `extensions_config.json` optionally accepts `module.path:ClassName` strings or `{class, kwargs}` objects. `deerflow.reflection.resolve_class` loads `AgentMiddleware` classes; import, class, and constructor errors fail agent creation. `kwargs` must be JSON-compatible; YAML dates/timestamps become ISO strings. Order: built-ins/custom and loop/token guards → extensions → terminal-response/safety/clarification tail. Subagents share the list before their safety tail; separate lead/subagent lists are unsupported. Trusted operator config only: paths instantiate arbitrary code. Gateway skill/MCP toggles preserve it in raw JSON; adding an API write path requires explicit trust-boundary review. 33. **TerminalResponseMiddleware** - When a provider returns an empty terminal `AIMessage` after tool execution, injects a hidden recovery prompt and retries the model once; a second empty response is replaced in checkpoint state by a visible error fallback marked for the run worker, so the run finishes as an error instead of a silent success 34. **ModelLengthFinishReasonMiddleware** - Records `stop_reason=model_length_capped` when provider-specific length detectors match a terminal `AIMessage` without tool-call intent (`finish_reason=length` / `MAX_TOKENS`, or `stop_reason=max_tokens`), preserving the original assistant content and never reparsing textual tool-call-like envelopes 35. **SafetyFinishReasonMiddleware** - *(optional, if `safety_finish_reason.enabled`)* Suppresses tool execution when the provider safety-terminated the response (e.g. `finish_reason=content_filter`); registered after terminal-response/custom/configured middlewares so LangChain's reverse-order `after_model` dispatch runs it first diff --git a/backend/packages/harness/deerflow/client.py b/backend/packages/harness/deerflow/client.py index fe0f48f0d..b1a35b6cf 100644 --- a/backend/packages/harness/deerflow/client.py +++ b/backend/packages/harness/deerflow/client.py @@ -41,12 +41,14 @@ from deerflow.config.agents_config import AGENT_NAME_PATTERN from deerflow.config.app_config import get_app_config, reload_app_config from deerflow.config.extensions_config import ( ExtensionsConfig, - SkillStateConfig, atomic_write_extensions_config, extensions_config_file_lock, extensions_config_write_lock, get_extensions_config, + read_raw_extensions_config, reload_extensions_config, + set_raw_skill_enabled, + validate_raw_extensions_config, ) from deerflow.config.paths import get_paths from deerflow.config.subagent_runtime_config import SubagentRuntimeConfig @@ -260,6 +262,19 @@ class DeerFlowClient: """Write JSON to *path* atomically (temp file + replace).""" atomic_write_extensions_config(path, data) + @classmethod + def _write_skill_enabled_state(cls, config_path: Path, name: str, enabled: bool) -> None: + """Persist one skill state and reload; callers hold the extensions config locks. + + Works on the raw file so ``$VAR`` placeholders are never written back as + resolved secrets. + """ + config_data = read_raw_extensions_config(config_path) + set_raw_skill_enabled(config_data, name, enabled) + validate_raw_extensions_config(config_data) + cls._atomic_write_json(config_path, config_data) + reload_extensions_config() + def _get_runnable_config(self, thread_id: str, **overrides) -> RunnableConfig: """Build a RunnableConfig for agent invocation.""" configurable = { @@ -1268,6 +1283,7 @@ class DeerFlowClient: ``McpConfigResponse`` schema. Raises: + ValueError: If the resulting config would not load; nothing is written. OSError: If the config file cannot be written. """ config_path = ExtensionsConfig.resolve_config_path() @@ -1277,10 +1293,11 @@ class DeerFlowClient: with extensions_config_write_lock, extensions_config_file_lock(config_path): # The singleton is process-local, so re-read the shared file under # the cross-process lock before merging the replacement MCP map. - current_config = ExtensionsConfig.from_file(config_path) - config_data = current_config.to_file_dict() + # Read it raw so sibling keys keep their $VAR placeholders. + config_data = read_raw_extensions_config(config_path) config_data["mcpServers"] = mcp_servers + validate_raw_extensions_config(config_data) self._atomic_write_json(config_path, config_data) reloaded = reload_extensions_config() @@ -1324,7 +1341,8 @@ class DeerFlowClient: Updated skill info dict. Raises: - ValueError: If the skill is not found. + ValueError: If the skill is not found, or extensions_config.json + is invalid (nothing is written). OSError: If the config file cannot be written. """ storage = get_or_new_user_skill_storage(get_effective_user_id(), app_config=self._app_config) @@ -1348,14 +1366,8 @@ class DeerFlowClient: with skill_projection_mutation(storage, "public", remove_names=removal_names): with extensions_config_write_lock, extensions_config_file_lock(config_path): # The projection lock is cross-process, but the singleton - # cache is not. Reload from disk under the config lock. - extensions_config = ExtensionsConfig.from_file(config_path) - extensions_config.skills[name] = SkillStateConfig(enabled=enabled) - - config_data = extensions_config.to_file_dict() - - self._atomic_write_json(config_path, config_data) - reload_extensions_config() + # cache is not. Reload raw from disk under the config lock. + self._write_skill_enabled_state(config_path, name, enabled) else: # CUSTOM / LEGACY: write per-user state from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage @@ -1368,11 +1380,7 @@ class DeerFlowClient: if config_path is None: raise FileNotFoundError("Cannot locate extensions_config.json. Set DEER_FLOW_EXTENSIONS_CONFIG_PATH or ensure it exists in the project root.") with extensions_config_write_lock, extensions_config_file_lock(config_path): - extensions_config = ExtensionsConfig.from_file(config_path) - extensions_config.skills[name] = SkillStateConfig(enabled=enabled) - config_data = extensions_config.to_file_dict() - self._atomic_write_json(config_path, config_data) - reload_extensions_config() + self._write_skill_enabled_state(config_path, name, enabled) # Invalidate the prompt cache for this caller (and for all users if # the changed skill is PUBLIC, since PUBLIC state is shared). Mirrors diff --git a/backend/packages/harness/deerflow/config/AGENTS.md b/backend/packages/harness/deerflow/config/AGENTS.md index f2bd85c1c..ddb225d8b 100644 --- a/backend/packages/harness/deerflow/config/AGENTS.md +++ b/backend/packages/harness/deerflow/config/AGENTS.md @@ -87,3 +87,5 @@ Extensions are optional only in the fallback *search* mode (priority 3-4 above): - `middlewares` - `AgentMiddleware` entries for lead and subagent runtime extension: class-path strings or `{class, kwargs}` objects. `kwargs` values must be JSON types; YAML dates and timestamps are coerced to ISO strings so they match JSON. `config.yaml -> extensions` can override these fields after validation; overrides are replace-per-field, not list concatenation. Gateway API endpoints and `DeerFlowClient` methods can modify MCP servers and skill state at runtime; their `extensions_config.json` writes use the shared atomic replacement helper, while `middlewares` remains an operator-controlled config-file extension point. + +Values beginning with `$` are resolved from the environment when the file is loaded, and an unset variable becomes `""`. Runtime writers (MCP router, skill toggle, `DeerFlowClient`) therefore read the raw file with `read_raw_extensions_config`, merge into it (`set_raw_skill_enabled` for skill state), check the candidate with `validate_raw_extensions_config`, and write that raw dict. They never serialize an `ExtensionsConfig` model back to disk: its resolved values would persist secrets in plaintext and erase the references. When the file does not exist yet, the Gateway skill toggle seeds only the cached skill states. `tests/test_extensions_config_raw_writes.py` and the placeholder tests in `tests/test_client.py` pin this. diff --git a/backend/packages/harness/deerflow/config/extensions_config.py b/backend/packages/harness/deerflow/config/extensions_config.py index 09a5218a5..8853d2c94 100644 --- a/backend/packages/harness/deerflow/config/extensions_config.py +++ b/backend/packages/harness/deerflow/config/extensions_config.py @@ -415,10 +415,6 @@ class ExtensionsConfig(BaseModel): raise ValueError(f"MCP task server name must contain 1 to {MCP_TASK_SERVER_NAME_MAX_LENGTH} characters") return self - def to_file_dict(self) -> dict[str, Any]: - """Serialize in the public extensions_config.json shape.""" - return self.model_dump(by_alias=True) - @classmethod def resolve_config_path(cls, config_path: str | None = None) -> Path | None: """Resolve the extensions config file path. @@ -507,6 +503,9 @@ class ExtensionsConfig(BaseModel): Returns: ExtensionsConfig: The loaded config, or empty config if file not found. + Its ``$VAR`` strings are already resolved, so it must never be + serialized back to disk; writers use + :func:`read_raw_extensions_config` instead. """ resolved_path = cls.resolve_config_path(config_path) if resolved_path is None: @@ -699,6 +698,47 @@ def atomic_write_extensions_config(path: Path, data: dict[str, Any]) -> None: ) +def read_raw_extensions_config(path: Path) -> dict[str, Any]: + """Read the on-disk config object with ``$VAR`` placeholders left intact. + + This is the only safe merge source for a read-modify-write. + ``ExtensionsConfig.from_file()`` resolves placeholders into live values and + unset variables into ``""``, so writing its model back would persist + secrets in plaintext and erase the references. Raises ``FileNotFoundError`` + when *path* does not exist, and ``ValueError`` for a malformed document; + that message omits the path so API callers can surface it as-is. + """ + try: + with open(path, encoding="utf-8") as f: + raw_data = json.load(f) + except json.JSONDecodeError as e: + raise ValueError(f"Extensions configuration is not valid JSON: {e.msg} at line {e.lineno} column {e.colno}") from e + if not isinstance(raw_data, dict): + raise ValueError("Extensions configuration must be a JSON object") + return raw_data + + +def validate_raw_extensions_config(raw_data: dict[str, Any]) -> ExtensionsConfig: + """Validate a raw write candidate exactly as the runtime will load it. + + Resolution works on a copy, so *raw_data* keeps its placeholders and can be + written as-is once this returns. + """ + return ExtensionsConfig.model_validate(ExtensionsConfig.resolve_env_variables(raw_data)) + + +def set_raw_skill_enabled(raw_data: dict[str, Any], skill_name: str, enabled: bool) -> None: + """Set one skill's enabled state in a raw config, leaving everything else as written.""" + skills = raw_data.setdefault("skills", {}) + if not isinstance(skills, dict): + raise ValueError("Extensions config `skills` must be a JSON object") + entry = skills.get(skill_name) + if isinstance(entry, dict): + entry["enabled"] = enabled + else: + skills[skill_name] = {"enabled": enabled} + + def get_extensions_config() -> ExtensionsConfig: """Get the extensions config instance. diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 72e52eeb3..2c746fef6 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -5,7 +5,7 @@ - **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 skill allowlists do not clamp the baseline tool set. A lead custom Agent's explicit `skills` list is additionally enforced at the sandbox filesystem layer (see Sandbox projection); subagent skill lists still scope discovery and activation only because concurrently delegated subagents share the lead thread sandbox. 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. The dynamic `allowed-tools` policy remains best-effort behavioral scoping: alternate loading paths are not captured and bounded autonomous context may evict entries. -- **Sandbox projection**: `skills/projection.py` materializes enabled-only shared trees at `{base_dir}/skills_view/public` and `{base_dir}/users/{user_id}/skills_view/{custom,legacy,integrations}`. A lead Agent with an explicit `skills` allowlist (including `[]`) gets the intersection of enabled public/user-visible skills and that allowlist at `{base_dir}/users/{user_id}/threads/{thread_id}/skills_view/{public,custom,legacy,integrations}`. `skills=None` keeps the shared zero-copy mount until a thread has used an explicit policy; later unrestricted runs repopulate the same stable thread root with all enabled skills. Rebuilds sign source state, view state, and normalized policy in a manifest, revoke every old category before adding the new policy, stage copies in temporary directories, and atomically replace files. Category root inodes stay stable for live bind mounts; concurrent readers can briefly see fewer skills during a policy change, never a skill revoked by the new policy. Policy-scoped copies reject absolute symlinks and relative symlinks that resolve outside their own skill package, preventing a permitted package from linking back to an omitted source. 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/thread 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 shared scopes under a cross-process lock; Gateway boot ensures only the shared public view, user views are repaired lazily on acquire, and Agent thread views are recomputed before sandbox reuse. Managed integration packages are global, but their projected category is per-user because enabled state is isolated. 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 and thread scope checks are serialized per scope. Projection failures clear the affected view before raising. +- **Sandbox projection**: `skills/projection.py` materializes enabled-only shared trees at `{base_dir}/skills_view/public` and `{base_dir}/users/{user_id}/skills_view/{custom,legacy,integrations}`. A lead Agent with an explicit `skills` allowlist (including `[]`) gets the intersection of enabled public/user-visible skills and that allowlist at `{base_dir}/users/{user_id}/threads/{thread_id}/skills_view/{public,custom,legacy,integrations}`. `skills=None` keeps the shared zero-copy mount until a thread has used an explicit policy; later unrestricted runs repopulate the same stable thread root with all enabled skills. Rebuilds sign source state, view state, and normalized policy in a manifest, revoke every old category before adding the new policy, stage copies in temporary directories, and atomically replace files. Category root inodes stay stable for live bind mounts; concurrent readers can briefly see fewer skills during a policy change, never a skill revoked by the new policy. Policy-scoped copies reject absolute symlinks and relative symlinks that resolve outside their own skill package, preventing a permitted package from linking back to an omitted source. 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/thread 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 shared scopes under a cross-process lock; Gateway boot ensures only the shared public view, user views are repaired lazily on acquire, and Agent thread views are recomputed before sandbox reuse. Managed integration packages are global, but their projected category is per-user because enabled state is isolated. 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 raw from disk, change only that skill's entry, 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 and thread scope checks are serialized per scope. Projection failures clear the affected view before raising. - **Injection (legacy / default)**: Enabled skills are listed in the agent system prompt with full metadata and container paths (`` block). Controlled by `skills.deferred_discovery: false` (default). - **Deferred discovery** (`skills.deferred_discovery: true`): Skills are listed by name only in a compact `` block, keeping the system prompt prefix-cache friendly. The agent calls the `describe_skill` tool at runtime to fetch full metadata for skills it wants to use, then loads the SKILL.md via `read_file`. Two new modules support this path: - `skills/catalog.py` — `SkillCatalog` (immutable, searchable; query forms: `select:a,b`, `+prefix`, free-text regex); `select:` returns all requested skills without a result cap; other modes cap at `MAX_RESULTS=5`. diff --git a/backend/tests/blocking_io/test_skills_update_router.py b/backend/tests/blocking_io/test_skills_update_router.py index 0bbc3695d..5b2417605 100644 --- a/backend/tests/blocking_io/test_skills_update_router.py +++ b/backend/tests/blocking_io/test_skills_update_router.py @@ -110,13 +110,14 @@ async def test_update_skill_writes_from_snapshot_without_mutating_singleton(tmp_ assert "demo-skill" not in shared_config.skills config_text = await asyncio.to_thread(config_path.read_text, encoding="utf-8") written = json.loads(config_text) - assert written["skills"] == { - "existing-skill": {"enabled": True}, - "demo-skill": {"enabled": False}, + # A new file is seeded with the cached skill states only. The cached model + # holds $VAR-resolved values, so none of its other fields are serialized. + assert written == { + "skills": { + "existing-skill": {"enabled": True}, + "demo-skill": {"enabled": False}, + } } - # to_file_dict() serializes the full shape, so unrelated top-level keys survive. - assert written["mcpServers"] == {} - assert "middlewares" in written async def test_update_skill_persists_state_when_source_omits_skills(tmp_path: Path, monkeypatch) -> None: diff --git a/backend/tests/test_client.py b/backend/tests/test_client.py index cf48ef981..5ac8a1183 100644 --- a/backend/tests/test_client.py +++ b/backend/tests/test_client.py @@ -1898,6 +1898,51 @@ class TestMcpConfig: finally: tmp_path.unlink() + def test_update_mcp_config_preserves_raw_sibling_keys(self, client, tmp_path, monkeypatch): + """Only ``mcpServers`` is replaced; every other key keeps its on-disk ``$VAR`` form.""" + monkeypatch.setenv("DEERFLOW_TEST_GH_TOKEN", "ghp_live_secret_value") + config_file = tmp_path / "extensions_config.json" + config_file.write_text( + json.dumps( + { + "mcpServers": {"old": {"type": "stdio", "command": "npx"}}, + "mcpInterceptors": {"auth": "$DEERFLOW_TEST_GH_TOKEN"}, + "skills": {"kept": {"enabled": False}}, + } + ), + encoding="utf-8", + ) + + with ( + patch("deerflow.client.ExtensionsConfig.resolve_config_path", return_value=config_file), + patch("deerflow.client.reload_extensions_config", return_value=ExtensionsConfig()), + ): + client.update_mcp_config({"new": {"type": "stdio", "command": "uvx", "env": {"TOKEN": "$DEERFLOW_TEST_GH_TOKEN"}}}) + + written_text = config_file.read_text(encoding="utf-8") + assert json.loads(written_text) == { + "mcpServers": {"new": {"type": "stdio", "command": "uvx", "env": {"TOKEN": "$DEERFLOW_TEST_GH_TOKEN"}}}, + "mcpInterceptors": {"auth": "$DEERFLOW_TEST_GH_TOKEN"}, + "skills": {"kept": {"enabled": False}}, + } + assert "ghp_live_secret_value" not in written_text + + def test_update_mcp_config_rejects_invalid_candidate_without_writing(self, client, tmp_path): + config_file = tmp_path / "extensions_config.json" + original = json.dumps({"mcpServers": {}, "skills": {"kept": {"enabled": False}}}) + config_file.write_text(original, encoding="utf-8") + reload = MagicMock() + + with ( + patch("deerflow.client.ExtensionsConfig.resolve_config_path", return_value=config_file), + patch("deerflow.client.reload_extensions_config", reload), + pytest.raises(ValueError), + ): + client.update_mcp_config({"bad": {"enabled": "not-a-bool"}}) + + assert config_file.read_text(encoding="utf-8") == original + reload.assert_not_called() + # --------------------------------------------------------------------------- # Skills management @@ -1982,6 +2027,44 @@ class TestSkillsManagement: finally: tmp_path.unlink() + @staticmethod + def _config_with_placeholders() -> dict: + return { + "mcpServers": {"github": {"type": "stdio", "command": "npx", "env": {"GITHUB_TOKEN": "$DEERFLOW_TEST_GH_TOKEN", "OPTIONAL": "$DEERFLOW_TEST_UNSET_VAR"}}}, + "mcpInterceptors": {"auth": "$DEERFLOW_TEST_GH_TOKEN"}, + "skills": {}, + } + + @pytest.mark.parametrize("category", ["public", "custom"]) + def test_update_skill_preserves_env_placeholders(self, client, tmp_path, monkeypatch, category): + """Toggling a skill must not persist resolved ``$VAR`` values or blank unset ones. + + ``public`` covers the shared-state path; ``custom`` with non-user-scoped + storage covers the fallback that also writes ``extensions_config.json``. + """ + monkeypatch.setenv("DEERFLOW_TEST_GH_TOKEN", "ghp_live_secret_value") + monkeypatch.delenv("DEERFLOW_TEST_UNSET_VAR", raising=False) + config_file = tmp_path / "extensions_config.json" + config_file.write_text(json.dumps(self._config_with_placeholders()), encoding="utf-8") + + skill = self._make_skill(enabled=True) + skill.category = category + storage = MagicMock() + storage.load_skills.side_effect = [[skill], [self._make_skill(enabled=False)]] + + with ( + patch("deerflow.client.get_or_new_user_skill_storage", return_value=storage), + patch("deerflow.client.ExtensionsConfig.resolve_config_path", return_value=config_file), + patch("deerflow.client.reload_extensions_config"), + ): + client.update_skill("test-skill", enabled=False) + + expected = self._config_with_placeholders() + expected["skills"]["test-skill"] = {"enabled": False} + written_text = config_file.read_text(encoding="utf-8") + assert json.loads(written_text) == expected + assert "ghp_live_secret_value" not in written_text + def test_update_skill_not_found(self, client): with patch("deerflow.skills.storage.local_skill_storage.LocalSkillStorage.load_skills", return_value=[]): with pytest.raises(ValueError, match="not found"): diff --git a/backend/tests/test_configured_extensions.py b/backend/tests/test_configured_extensions.py index c6e851853..95be61ffe 100644 --- a/backend/tests/test_configured_extensions.py +++ b/backend/tests/test_configured_extensions.py @@ -9,7 +9,14 @@ from langchain.agents.middleware import AgentMiddleware from pydantic import ValidationError from deerflow.agents.middlewares.configured_extensions import load_configured_extension_middlewares -from deerflow.config.extensions_config import ConfiguredMiddlewareSpec, ExtensionsConfig +from deerflow.config.extensions_config import ( + ConfiguredMiddlewareSpec, + ExtensionsConfig, + atomic_write_extensions_config, + read_raw_extensions_config, + set_raw_skill_enabled, + validate_raw_extensions_config, +) class RecordingMiddleware(AgentMiddleware): @@ -122,7 +129,7 @@ def test_kwargs_yaml_date_normalizes_to_iso_string(): spec = ConfiguredMiddlewareSpec.model_validate({"class": "pkg:Mw", "kwargs": {"cutoff": date(2026, 1, 1)}}) assert spec.kwargs == {"cutoff": "2026-01-01"} - json.dumps(ExtensionsConfig(middlewares=[spec]).to_file_dict()) + assert json.loads(json.dumps(spec.kwargs)) == {"cutoff": "2026-01-01"} def test_kwargs_yaml_datetime_normalizes_to_iso_string(): @@ -141,21 +148,28 @@ def test_kwargs_reject_nan(): ConfiguredMiddlewareSpec.model_validate({"class": "pkg:Mw", "kwargs": {"n": float("nan")}}) -def test_to_file_dict_round_trips_kwargs_entries(): - config = ExtensionsConfig.model_validate( - { - "middlewares": [ - "pkg:Plain", - {"class": "pkg:WithArgs", "kwargs": {"max_tool_calls": 5}}, - ] - } - ) +def test_raw_file_round_trips_kwargs_entries(tmp_path, monkeypatch): + monkeypatch.setenv("DEERFLOW_TEST_MIDDLEWARE_TOKEN", "test-secret") + config_path = tmp_path / "extensions_config.json" + raw = { + "middlewares": [ + "pkg:Plain", + {"class": "pkg:WithArgs", "kwargs": {"max_tool_calls": 5, "token": "$DEERFLOW_TEST_MIDDLEWARE_TOKEN"}}, + ] + } + config_path.write_text(json.dumps(raw), encoding="utf-8") - dumped = config.to_file_dict() - restored = ExtensionsConfig.model_validate(dumped) + candidate = read_raw_extensions_config(config_path) + set_raw_skill_enabled(candidate, "demo", False) + validate_raw_extensions_config(candidate) + atomic_write_extensions_config(config_path, candidate) + dumped = read_raw_extensions_config(config_path) + restored = ExtensionsConfig.from_file(config_path) + assert dumped["middlewares"] == raw["middlewares"] + assert dumped["skills"] == {"demo": {"enabled": False}} assert dumped["middlewares"][0] == "pkg:Plain" assert dumped["middlewares"][1]["class"] == "pkg:WithArgs" - assert dumped["middlewares"][1]["kwargs"] == {"max_tool_calls": 5} + assert dumped["middlewares"][1]["kwargs"] == {"max_tool_calls": 5, "token": "$DEERFLOW_TEST_MIDDLEWARE_TOKEN"} assert restored.middlewares[1].class_path == "pkg:WithArgs" - assert restored.middlewares[1].kwargs == {"max_tool_calls": 5} + assert restored.middlewares[1].kwargs == {"max_tool_calls": 5, "token": "test-secret"} diff --git a/backend/tests/test_extensions_config_raw_writes.py b/backend/tests/test_extensions_config_raw_writes.py new file mode 100644 index 000000000..f825b2813 --- /dev/null +++ b/backend/tests/test_extensions_config_raw_writes.py @@ -0,0 +1,176 @@ +"""Runtime writers must round-trip ``extensions_config.json`` without expanding ``$VAR``. + +``ExtensionsConfig.from_file()`` resolves every ``$VAR`` string into the live +environment value, and an unset variable into ``""``. Serializing that model +back to disk therefore persists secrets in plaintext and permanently erases +references to variables that are not set in the writing process. Every +read-modify-write must operate on the raw on-disk JSON instead; the MCP router +already does, and these tests pin the skill-state writers to the same contract. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from app.gateway.routers import skills as skills_router +from deerflow.config.extensions_config import ( + ExtensionsConfig, + McpServerConfig, + SkillStateConfig, + read_raw_extensions_config, + set_raw_skill_enabled, + validate_raw_extensions_config, +) + +SECRET = "ghp_live_secret_value" + + +def _raw_config_with_placeholders() -> dict: + return { + "mcpServers": { + "github": { + "enabled": True, + "type": "stdio", + "command": "npx", + "args": ["-y", "@modelcontextprotocol/server-github"], + "env": {"GITHUB_TOKEN": "$DEERFLOW_TEST_GH_TOKEN", "OPTIONAL": "$DEERFLOW_TEST_UNSET_VAR"}, + }, + "remote": { + "type": "http", + "url": "https://mcp.example.com/mcp", + "headers": {"Authorization": "$DEERFLOW_TEST_GH_TOKEN"}, + }, + }, + "mcpInterceptors": {"auth": "$DEERFLOW_TEST_GH_TOKEN"}, + "skills": {"existing-skill": {"enabled": True}}, + } + + +@pytest.fixture +def placeholder_env(monkeypatch): + monkeypatch.setenv("DEERFLOW_TEST_GH_TOKEN", SECRET) + monkeypatch.delenv("DEERFLOW_TEST_UNSET_VAR", raising=False) + + +def _write_json(path: Path, data: object) -> None: + path.write_text(json.dumps(data, indent=2), encoding="utf-8") + + +# --------------------------------------------------------------------------- +# Harness helpers +# --------------------------------------------------------------------------- + + +def test_read_raw_keeps_placeholders(tmp_path: Path, placeholder_env) -> None: + config_path = tmp_path / "extensions_config.json" + _write_json(config_path, _raw_config_with_placeholders()) + + assert read_raw_extensions_config(config_path) == _raw_config_with_placeholders() + + +def test_read_raw_rejects_invalid_json(tmp_path: Path) -> None: + config_path = tmp_path / "extensions_config.json" + config_path.write_text("{not json", encoding="utf-8") + + with pytest.raises(ValueError, match="not valid JSON"): + read_raw_extensions_config(config_path) + + +def test_read_raw_rejects_non_object(tmp_path: Path) -> None: + config_path = tmp_path / "extensions_config.json" + _write_json(config_path, ["not", "an", "object"]) + + with pytest.raises(ValueError, match="JSON object"): + read_raw_extensions_config(config_path) + + +def test_validate_raw_resolves_like_runtime_without_mutating_input(placeholder_env) -> None: + raw = _raw_config_with_placeholders() + + validated = validate_raw_extensions_config(raw) + + assert validated.mcp_servers["github"].env == {"GITHUB_TOKEN": SECRET, "OPTIONAL": ""} + assert raw == _raw_config_with_placeholders() + + +def test_validate_raw_rejects_runtime_invalid_candidate() -> None: + with pytest.raises(ValidationError): + validate_raw_extensions_config({"mcpServers": []}) + + +def test_set_raw_skill_enabled_creates_skills_map() -> None: + raw: dict = {"mcpServers": {}} + + set_raw_skill_enabled(raw, "demo-skill", False) + + assert raw == {"mcpServers": {}, "skills": {"demo-skill": {"enabled": False}}} + + +def test_set_raw_skill_enabled_keeps_sibling_entry_keys() -> None: + raw: dict = {"skills": {"demo-skill": {"enabled": True, "note": "operator comment"}}} + + set_raw_skill_enabled(raw, "demo-skill", False) + + assert raw["skills"]["demo-skill"] == {"enabled": False, "note": "operator comment"} + + +def test_set_raw_skill_enabled_rejects_non_object_skills() -> None: + with pytest.raises(ValueError, match="skills"): + set_raw_skill_enabled({"skills": ["demo-skill"]}, "demo-skill", False) + + +# --------------------------------------------------------------------------- +# Gateway skill toggle writer +# --------------------------------------------------------------------------- + + +def _patch_gateway_writer(monkeypatch, config_path: Path, cached: ExtensionsConfig | None = None) -> None: + monkeypatch.setattr(skills_router.ExtensionsConfig, "resolve_config_path", staticmethod(lambda _path=None: config_path)) + monkeypatch.setattr(skills_router, "reload_extensions_config", lambda: None) + monkeypatch.setattr(skills_router, "get_extensions_config", lambda: cached or ExtensionsConfig()) + + +def test_gateway_skill_toggle_preserves_placeholders(tmp_path: Path, monkeypatch, placeholder_env) -> None: + config_path = tmp_path / "extensions_config.json" + _write_json(config_path, _raw_config_with_placeholders()) + _patch_gateway_writer(monkeypatch, config_path) + + skills_router._write_extensions_skill_state(None, "demo-skill", False, rebuild_public_projection=False) + + expected = _raw_config_with_placeholders() + expected["skills"]["demo-skill"] = {"enabled": False} + written_text = config_path.read_text(encoding="utf-8") + assert json.loads(written_text) == expected + assert SECRET not in written_text + + +def test_gateway_skill_toggle_new_file_does_not_serialize_resolved_cache(tmp_path: Path, monkeypatch, placeholder_env) -> None: + config_path = tmp_path / "extensions_config.json" + cached = ExtensionsConfig( + mcp_servers={"github": McpServerConfig(command="npx", env={"GITHUB_TOKEN": SECRET})}, + skills={"existing-skill": SkillStateConfig(enabled=False)}, + ) + _patch_gateway_writer(monkeypatch, config_path, cached) + + skills_router._write_extensions_skill_state(None, "demo-skill", True, rebuild_public_projection=False) + + written_text = config_path.read_text(encoding="utf-8") + assert json.loads(written_text) == {"skills": {"existing-skill": {"enabled": False}, "demo-skill": {"enabled": True}}} + assert SECRET not in written_text + assert "demo-skill" not in cached.skills + + +def test_gateway_skill_toggle_leaves_invalid_config_untouched(tmp_path: Path, monkeypatch) -> None: + config_path = tmp_path / "extensions_config.json" + original = json.dumps({"mcpServers": [], "skills": {}}) + config_path.write_text(original, encoding="utf-8") + _patch_gateway_writer(monkeypatch, config_path) + + with pytest.raises(ValidationError): + skills_router._write_extensions_skill_state(None, "demo-skill", False, rebuild_public_projection=False) + + assert config_path.read_text(encoding="utf-8") == original diff --git a/backend/tests/test_skills_custom_router.py b/backend/tests/test_skills_custom_router.py index c0ab9c875..3d1f5568e 100644 --- a/backend/tests/test_skills_custom_router.py +++ b/backend/tests/test_skills_custom_router.py @@ -1027,10 +1027,10 @@ def test_public_skill_toggle_creates_missing_extensions_config(monkeypatch, tmp_ assert response.status_code == 200, response.text assert response.json()["enabled"] is False + # Only skill states are seeded; the cached model is never serialized because + # its $VAR values are already resolved. assert json.loads(config_path.read_text(encoding="utf-8")) == { - "mcpServers": {}, "skills": {"public-skill": {"enabled": False}}, - "middlewares": [], }