From 09988caf95ba380fcbd033a2ae2ae6dd547f92e1 Mon Sep 17 00:00:00 2001 From: Xinmin Zeng <135568692+fancyboi999@users.noreply.github.com> Date: Fri, 3 Jul 2026 07:51:22 +0800 Subject: [PATCH] feat(skills): request-scoped secrets for skills (closes #3861) (#3871) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(sandbox): per-call env injection + platform-secret scrubbing for skills Add an env parameter to Sandbox.execute_command (abstract + local + AIO) so request-scoped secrets can be injected into skill subprocesses, and scrub platform credentials (*KEY*/*SECRET*/*TOKEN*/*PASSWORD*/*CREDENTIAL*) from the inherited environment by default so scoped injection is not security theatre. LocalSandbox always passes an explicit scrubbed env; AioSandbox routes env-bearing commands through bash.exec(env=) on a fresh session and leaves the legacy persistent-shell path unchanged. Part of #3861. BEHAVIOR CHANGE: execute_command no longer inherits the full os.environ; Windows encoding tests updated to assert the scrubbed dict. * feat(skills): parse required-secrets frontmatter declaration Add SecretRequirement and Skill.required_secrets, and parse the required-secrets SKILL.md frontmatter field (a string list or {name, optional} mappings), dropping malformed entries with a warning so one bad declaration does not invalidate the skill. The declared name is both the context.secrets key and the env var injected at activation. Part of #3861. * feat(runtime): request-scoped secret carrier (context.secrets) Add SECRETS_CONTEXT_KEY + extract_request_secrets, centralising the context.secrets carrier contract. The existing context passthrough (build_run_config -> _build_runtime_context) already carries the sub-key to runtime.context without mirroring it into configurable; characterization tests lock that behaviour. Part of #3861. * feat(skills): inject declared secrets at slash-activation into bash env Binding point A: when a skill is slash-activated, SkillActivationMiddleware resolves its declared required-secrets against the request's context.secrets and writes the per-run injection set to runtime.context. The bash tool forwards that set to execute_command(env=). A skill cannot harvest a host platform credential (is_host_platform_secret guard, cf. GHSA-rhgp-j443-p4rf), and injected values are redacted from bash output (mask_secret_values) so an echoed secret never re-enters the prompt/trace. Part of #3861. * test(skills): lock the five secret leak surfaces + add trace redaction helper Regression tests assert the secret value is absent from all five surfaces: prompt (activation message), checkpoint (graph state vs context separation), audit (journal records names only), trace (metadata builder never copies context; never mirrored to configurable), and stdout (mask_secret_values). Add redact_secret_context_keys as a defensive helper for any context serialization. Part of #3861. * docs(backend): document request-scoped secrets for skills Add Request-Scoped Secrets subsection (Skills) + env policy note (Sandbox) and the execute_command(env=) signature change, per the doc-sync policy. Part of #3861. * fix(skills): close gaps found by end-to-end verification of request-scoped secrets Real-gateway e2e + independent review of #3861 surfaced three defects, now fixed: 1. Slash activation never fired in the live chain. InputSanitizationMiddleware wraps user input in BEGIN/END markers before SkillActivationMiddleware sees it, and the original text was only preserved when an upload or IM channel set it. For a plain text message the slash command became undetectable, so no secret was ever resolved. Fix: the sanitizer now setdefaults the pre-wrap text into ORIGINAL_USER_CONTENT_KEY (additive; sanitization behaviour unchanged), so slash activation works for all messages. Pre-existing latent bug surfaced here. 2. The raw request config (with context.secrets) was persisted to runs.kwargs_json and echoed by the run API (RunResponse.kwargs). Fix: redact_config_secrets() strips secret-bearing context keys from the persisted/echoed copy in start_run; the live config that drives the run keeps them. build_run_config now also sets configurable.thread_id on the context path (the checkpointer requires it). 3. Connection-string credentials (DATABASE_URL, REDIS_URL, SENTRY_DSN, GH_PAT, ...) were not scrubbed from the inherited sandbox env. Fix: env_policy adds a *DSN* pattern plus an explicit connection-string denylist (no blanket *URL* — benign service URLs stay readable). Verified end-to-end via a real gateway run (real LLM + skill activation + bash): the secret reaches the sandbox subprocess and appears in NONE of prompt, trace, checkpoint, audit, stdout, runs.kwargs_json, or the run API. Part of #3861. * docs(backend): document the env scrub, persistence redaction, and sanitizer interaction Sync the Request-Scoped Secrets section with the verification-driven fixes: inherited-env scrub (incl. connection-string denylist), run-record/run-API redaction as the 6th sealed leak surface, and the sanitizer preserving original content so slash activation fires. Part of #3861. * fix(skills): inject caller secret over scrubbed host value; drop redundant host-name guard A real-world demo (a skill calling a third-party cloud API with a request-scoped key) exposed that the is_host_platform_secret guard was both wrong and harmful: it refused to inject a caller-supplied secret whenever a same-named variable existed in the Gateway env — which is exactly the #3861 use case (a per-user key overriding a shared platform key). The guard was also redundant: build_sandbox_env already scrubs secret-looking names from the inherited env before injection, so a skill can never read a host credential — it only ever receives the caller's value. Remove the guard; the injected (caller) value simply wins over the scrubbed host value. Verified end-to-end: the agent called the real cloud API successfully with the caller's key, the host's same-named key was scrubbed and never used, and the caller's key leaked to none of the surfaces. Part of #3861. * fix(skills): address review on request-scoped secrets (#3861) Review fixes from PR #3871: - E2BSandbox.execute_command now accepts env/timeout and routes them to commands.run(envs=, timeout=). The bash tool passes env= unconditionally, so the prior signature (command only) raised TypeError on every e2b bash call and broke e2b deployments entirely. env=None stays backward-compatible. - SkillActivationMiddleware clears the active-secret set before resolving each activation, so a later skill in the same run never inherits an earlier skill's injection set (the #3861 contract: a skill only receives what the caller supplied AND that skill declared). - AioSandbox env path uses a dedicated _DEFAULT_HARD_TIMEOUT — bash.exec exposes no idle/no-change timeout, so the prior reuse of the legacy idle constant conflated wall-clock vs idle semantics. The env path also retries on the ErrorObservation signature now, sharing the legacy persistent-shell recovery contract. - mask_secret_values skips values below a minimum length floor so a short declared secret (e.g. "42") cannot shred unrelated bytes (exit codes, timestamps, sizes) of tool output. The secret is still injected into the subprocess; only the output mask skips it. session_id reuse on the env path is intentionally NOT added: a shared session could let request-scoped secrets ride the session env into later commands, which the SDK does not contractually forbid. The fresh-session choice matches the LocalSandbox model (each call is a fresh subprocess); the trade-off (consecutive env-bearing calls do not share cwd/venv/exports) is documented on _execute_with_env. --- backend/AGENTS.md | 17 +- backend/app/gateway/services.py | 13 +- .../input_sanitization_middleware.py | 10 +- .../skill_activation_middleware.py | 54 +- .../community/aio_sandbox/aio_sandbox.py | 81 +- .../community/e2b_sandbox/e2b_sandbox.py | 22 +- .../deerflow/runtime/secret_context.py | 88 ++ .../harness/deerflow/sandbox/env_policy.py | 82 ++ .../deerflow/sandbox/local/local_sandbox.py | 28 +- .../harness/deerflow/sandbox/sandbox.py | 18 +- .../harness/deerflow/sandbox/tools.py | 44 +- .../harness/deerflow/skills/parser.py | 49 +- .../packages/harness/deerflow/skills/types.py | 16 +- backend/tests/test_e2b_sandbox_provider.py | 37 + backend/tests/test_gateway_services.py | 29 +- .../test_local_sandbox_command_timeout.py | 2 +- backend/tests/test_local_sandbox_encoding.py | 17 +- backend/tests/test_sandbox_middleware.py | 8 +- .../test_skill_request_scoped_secrets.py | 769 ++++++++++++++++++ .../test_tool_output_budget_middleware.py | 8 +- 20 files changed, 1360 insertions(+), 32 deletions(-) create mode 100644 backend/packages/harness/deerflow/runtime/secret_context.py create mode 100644 backend/packages/harness/deerflow/sandbox/env_policy.py create mode 100644 backend/tests/test_skill_request_scoped_secrets.py diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 63b39c1cc..2074e28c7 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -310,8 +310,9 @@ Proxied through nginx: `/api/langgraph/*` → Gateway LangGraph-compatible runti ### Sandbox System (`packages/harness/deerflow/sandbox/`) -**Interface**: Abstract `Sandbox` with `execute_command`, `read_file`, `write_file`, `list_dir` +**Interface**: Abstract `Sandbox` with `execute_command(command, env=None)`, `read_file`, `write_file`, `list_dir`. The optional `env` injects per-call environment variables (request-scoped secrets — see Request-Scoped Secrets below); `LocalSandbox` merges it via `subprocess.run(env=...)` and `AioSandbox` routes env-bearing commands through the `bash.exec(env=...)` API on a fresh session. **Provider Pattern**: `SandboxProvider` with `acquire`, `acquire_async`, `get`, `release` lifecycle. Async agent/tool paths call async sandbox lifecycle hooks so Docker sandbox creation, discovery, cross-process locking, readiness polling, and release stay off the event loop. +**Environment policy** (`sandbox/env_policy.py`): `execute_command` no longer inherits the full `os.environ`. `build_sandbox_env()` scrubs secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASSWORD*`/`*CREDENTIAL*`) from the inherited environment before layering injected request secrets on top, so platform credentials (e.g. `OPENAI_API_KEY`) never leak into skill subprocesses. Benign vars (`PATH`, `HOME`, `LANG`, `VIRTUAL_ENV`, ...) are preserved. **Implementations**: - `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. - `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths. @@ -384,12 +385,24 @@ Additional providers also live here (`brave`, `browserless`, `crawl4ai`, `ddg_se ### Skills System (`packages/harness/deerflow/skills/`) - **Location**: `deer-flow/skills/{public,custom}/` -- **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools) +- **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools, required-secrets) - **Loading**: `load_skills()` recursively scans `skills/{public,custom}` for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json - **Injection**: Enabled skills listed in agent system prompt with container paths - **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`), disabled skills, and skills outside a custom agent's whitelist. - **Installation**: `POST /api/skills/install` extracts .skill ZIP archive to custom/ directory +#### Request-Scoped Secrets (`required-secrets`) + +Lets a caller pass per-request, short-lived end-user credentials (e.g. an ERP token) to a skill's sandbox scripts without the value entering the prompt, tool arguments, the executed command string, or traces (issue #3861). + +- **Declare**: a skill lists the secrets it needs in `SKILL.md` frontmatter — `required-secrets:` as a string list or `{name, optional}` mappings. `name` is both the lookup key and the env var name exposed to scripts. Parsed by `skills/parser.py::parse_required_secrets` into `Skill.required_secrets` (`SecretRequirement`); malformed entries are dropped with a warning. +- **Carry**: the caller sends values out-of-band in the run request's `context.secrets` mapping (never a message). `runtime/secret_context.py` owns the contract (`SECRETS_CONTEXT_KEY`, `extract_request_secrets`). The existing `context` passthrough carries it to `runtime.context` without mirroring into `configurable`. `build_run_config` still sets `configurable.thread_id` on the context path — the checkpointer requires it. +- **Bind (point A)**: on slash-activation, `SkillActivationMiddleware._apply_skill_secrets` resolves the activated skill's declared secrets against `context.secrets` and writes the per-run injection set to `runtime.context[__active_skill_secrets]`. Slash activation reads the genuine user text via `get_original_user_content_text`; `InputSanitizationMiddleware` preserves it (`ORIGINAL_USER_CONTENT_KEY`) when it wraps input in BEGIN/END markers, so activation fires even after sanitization. The injected value always comes from the caller's request, never the host environment (scrubbed first — see below), so a declared name that also exists in the host env is safe: the caller's value wins and the host value is dropped (the #3861 per-user-key-overrides-shared-key case). Missing required secrets are logged, not injected. +- **Inject**: `bash_tool` reads the injection set and passes it as `execute_command(env=...)`. Scope is the activation turn/run only — a run without `/skill` activation injects nothing. +- **Inherited-env scrub**: `execute_command` no longer leaks the Gateway's `os.environ` to skill subprocesses — `env_policy.build_sandbox_env` drops secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASSWORD*`/`*CREDENTIAL*`/`*DSN*` + a connection-string denylist like `DATABASE_URL`/`REDIS_URL`/`GH_PAT`) so platform credentials never reach a skill; a skill that needs one must declare it. +- **Leak surfaces sealed** (verified by a real-gateway e2e run — secret reaches the sandbox but none of these): prompt (value never in a message), trace (`tracing/metadata.py` never copies `context`), checkpoint (secrets live on `runtime.context`, not graph state), audit (journal records names only), stdout (`tools.py::mask_secret_values` redacts injected values from bash output), and **run-record persistence + run API** (`services.py::start_run` stores `redact_config_secrets(body.config)` so `runs.kwargs_json` and `RunResponse.kwargs` never carry the secret). +- **Scope / non-goals**: only `/slash`-activated skills receive secrets (autonomously invoked enabled skills do not); no persistence/vaulting; the MCP per-user-credential gap (#3322) is a sibling, not covered here. Tests: `tests/test_skill_request_scoped_secrets.py`. + ### Model Factory (`packages/harness/deerflow/models/factory.py`) - `create_chat_model(name, thinking_enabled)` instantiates LLM from config via reflection diff --git a/backend/app/gateway/services.py b/backend/app/gateway/services.py index b93dee056..899b8a2e2 100644 --- a/backend/app/gateway/services.py +++ b/backend/app/gateway/services.py @@ -37,6 +37,7 @@ from deerflow.runtime import ( run_agent, ) from deerflow.runtime.runs.naming import resolve_root_run_name +from deerflow.runtime.secret_context import redact_config_secrets from deerflow.runtime.user_context import reset_current_user, set_current_user logger = logging.getLogger(__name__) @@ -290,6 +291,12 @@ def build_run_config( raise ValueError("request config 'context' must be a mapping or null.") context["thread_id"] = thread_id config["context"] = context + # The checkpointer always scopes state by configurable["thread_id"], + # regardless of whether the caller drives the run via context (e.g. + # request-scoped secrets, #3861). thread_id comes from the URL path, + # not caller config, so mirror it here while keeping secret-bearing + # context keys out of configurable. + config["configurable"] = {"thread_id": thread_id} else: configurable = {"thread_id": thread_id} configurable.update(request_config.get("configurable", {})) @@ -476,7 +483,11 @@ async def start_run( body.assistant_id, on_disconnect=disconnect, metadata=body.metadata or {}, - kwargs={"input": body.input, "config": body.config}, + # Persist a secret-redacted copy of the config: the run record is + # written to runs.kwargs_json and echoed by the run API, so a + # request-scoped secret (#3861) must not ride along. The live + # config built below keeps the secrets for the actual run. + kwargs={"input": body.input, "config": redact_config_secrets(body.config)}, multitask_strategy=body.multitask_strategy, model_name=model_name, user_id=owner_user_id, diff --git a/backend/packages/harness/deerflow/agents/middlewares/input_sanitization_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/input_sanitization_middleware.py index 943f2c32f..480ae065f 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/input_sanitization_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/input_sanitization_middleware.py @@ -31,6 +31,8 @@ from langchain.agents.middleware.types import ( from langchain_core.messages import HumanMessage from langgraph.errors import GraphBubbleUp +from deerflow.utils.messages import ORIGINAL_USER_CONTENT_KEY, message_content_to_text + logger = logging.getLogger(__name__) _SUMMARY_MESSAGE_NAME = "summary" @@ -233,11 +235,17 @@ class InputSanitizationMiddleware(AgentMiddleware[AgentState]): else: new_content = processed + # Preserve the pre-sanitization user text so downstream consumers that + # must see the genuine input (slash skill activation, regenerate) can + # recover it after the BEGIN/END wrapping. setdefault keeps an existing + # value (e.g. set by UploadsMiddleware or an IM channel) authoritative. + preserved_kwargs = dict(msg.additional_kwargs or {}) + preserved_kwargs.setdefault(ORIGINAL_USER_CONTENT_KEY, message_content_to_text(content)) messages[i] = HumanMessage( content=new_content, id=msg.id, name=msg.name, - additional_kwargs=msg.additional_kwargs, + additional_kwargs=preserved_kwargs, ) logger.debug( "InputSanitizationMiddleware: original=%r -> processed=%r", diff --git a/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py index eff634c7e..05beb2b00 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py @@ -16,10 +16,11 @@ from langchain.agents.middleware import AgentMiddleware from langchain.agents.middleware.types import ModelRequest, ModelResponse from langchain_core.messages import AIMessage, HumanMessage +from deerflow.runtime.secret_context import ACTIVE_SECRETS_CONTEXT_KEY, extract_request_secrets from deerflow.skills.slash import parse_slash_skill_reference, resolve_slash_skill from deerflow.skills.storage import get_or_new_skill_storage from deerflow.skills.storage.skill_storage import SkillStorage -from deerflow.skills.types import SKILL_MD_FILE +from deerflow.skills.types import SKILL_MD_FILE, SecretRequirement from deerflow.utils.messages import get_original_user_content_text if TYPE_CHECKING: @@ -40,6 +41,7 @@ class _Activation: skill_content: str content_hash: str remaining_text: str + required_secrets: tuple[SecretRequirement, ...] = () @dataclass(frozen=True, slots=True) @@ -134,6 +136,7 @@ class SkillActivationMiddleware(AgentMiddleware): skill_content=skill_content, content_hash=content_hash, remaining_text=resolved.remaining_text, + required_secrets=tuple(resolved.skill.required_secrets or ()), ) ) @@ -242,11 +245,60 @@ Follow this skill before choosing a general workflow. Load supporting resources activation.content_hash, ) self._record_activation(request, activation, hook=hook) + self._apply_skill_secrets(request, activation) activation_msg = self._make_activation_message(target, self._build_activation_reminder(activation)) messages = list(request.messages) messages.insert(target_index, activation_msg) return request.override(messages=messages) + @staticmethod + def _apply_skill_secrets(request: ModelRequest, activation: _Activation) -> None: + """Resolve the activated skill's declared secrets into the per-run injection + set (binding point A, issue #3861). + + For each declared secret present in the request's ``context.secrets``, + record its value in the injection set stored under + ``ACTIVE_SECRETS_CONTEXT_KEY`` on the shared run context, so the bash tool + can build the subprocess env for this turn. The injected value always comes + from the caller's request — never from the host environment, which is + scrubbed of secret-looking names by ``env_policy.build_sandbox_env`` before + injection. A skill can therefore never harvest a host platform credential + (it only ever receives what the caller explicitly supplied), so a declared + name that also exists in the host env is fine: the caller's value wins and + the host value is dropped. Secret *values* are never logged. + """ + runtime = getattr(request, "runtime", None) + context = getattr(runtime, "context", None) + if not isinstance(context, dict): + return + # Unconditionally clear any active-secret set a previous activation in + # the same run may have written, before this turn's resolution decides + # what (if anything) to install. Otherwise a later skill that declares + # no secrets, or whose required secrets the caller did not supply, would + # inherit the previous skill's injection set and the bash tool would + # inject those values into a subprocess that never declared them (#3861). + context.pop(ACTIVE_SECRETS_CONTEXT_KEY, None) + if not activation.required_secrets: + return + + request_secrets = extract_request_secrets(context) + injected: dict[str, str] = {} + missing: list[str] = [] + for req in activation.required_secrets: + if req.name in request_secrets: + injected[req.name] = request_secrets[req.name] + elif not req.optional: + missing.append(req.name) + + if injected: + context[ACTIVE_SECRETS_CONTEXT_KEY] = injected + if missing: + logger.warning( + "Skill %s activated but required secrets are missing from the request context: %s", + activation.skill_name, + ", ".join(sorted(missing)), + ) + @staticmethod def _make_activation_message(target: HumanMessage, activation_content: str) -> HumanMessage: stable_id = target.id or str(uuid.uuid4()) diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox.py b/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox.py index e30421fed..d7ce406b4 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox.py @@ -110,7 +110,22 @@ class AioSandbox(Sandbox): # default. _DEFAULT_NO_CHANGE_TIMEOUT = 600 - def execute_command(self, command: str) -> str: + # Wall-clock hard timeout for env-bearing commands routed through bash.exec. + # The bash.exec API exposes no idle/no-change timeout (unlike + # shell.exec_command's ``no_change_timeout`` on the legacy path), so + # env-bearing commands are bounded by total elapsed wall-clock time, not + # time-since-last-output. Kept at the same numeric value as the legacy idle + # budget so the two paths broadly agree on how long a single command may + # run; a future SDK that exposes an idle timeout on bash.exec should switch + # this call site to it. + _DEFAULT_HARD_TIMEOUT = 600.0 + + def execute_command( + self, + command: str, + env: dict[str, str] | None = None, + timeout: float | None = None, + ) -> str: """Execute a shell command in the sandbox. Uses a lock to serialize concurrent requests. The AIO sandbox @@ -122,10 +137,23 @@ class AioSandbox(Sandbox): Args: command: The command to execute. + env: Optional per-call environment variables (request-scoped secrets, + issue #3861). When provided, the command runs via the ``bash.exec`` + API (which supports per-command env) on a fresh auto-created session + so the secrets are scoped to this single command and never persist; + secret values travel in the structured ``env`` field, never in the + command string. When ``None`` the legacy persistent-shell path runs + unchanged. + timeout: Optional per-call timeout. The current sandbox SDK does not + expose a command-level timeout distinct from its client/request + timeout, so DeerFlow keeps using the backend's default here. Returns: The output of the command. """ + del timeout + if env: + return self._execute_with_env(command, env) with self._lock: try: result = self._client.shell.exec_command(command=command, no_change_timeout=self._DEFAULT_NO_CHANGE_TIMEOUT) @@ -154,6 +182,57 @@ class AioSandbox(Sandbox): logger.error(f"Failed to execute command in sandbox: {e}") return f"Error: {e}" + def _execute_with_env(self, command: str, env: dict[str, str]) -> str: + """Execute a command with per-call environment variables injected. + + The persistent-shell ``shell.exec_command`` API has no env parameter, so + injected commands use the ``bash.exec`` API which accepts per-command env. + Each call lets the sandbox auto-create a fresh session (no ``session_id``), + so injected request-scoped secrets are scoped to this command and never + persist across calls. Secret values travel in the structured ``env`` field, + never in the command string. + + Trade-off of the fresh-session choice: consecutive env-bearing bash calls + within the same skill do not share session state (cwd, sourced venv, + exported variables). This mirrors the LocalSandbox model (each call is a + fresh subprocess) and is intentional — a shared session_id would let + request-scoped secrets ride the session env into later commands, which the + SDK does not contractually forbid. Skills that need setup must fold it into + a single command (e.g. ``cd /mnt/user-data/workspace && source .venv/bin/activate && python run.py``). + + The ``_ERROR_OBSERVATION_SIGNATURE`` recovery contract is shared with the + legacy persistent-shell path: if the (unlikely, since each call is a fresh + session) corruption marker shows up, the call is retried on another fresh + session rather than returned verbatim. + """ + output = self._run_bash_exec(command, env) + if output and _ERROR_OBSERVATION_SIGNATURE in output: + logger.warning("ErrorObservation detected in bash.exec output, retrying on a fresh session") + retried = self._run_bash_exec(command, env) + if retried and _ERROR_OBSERVATION_SIGNATURE not in retried: + return retried + return output + + def _run_bash_exec(self, command: str, env: dict[str, str]) -> str: + """Single bash.exec invocation with injected env (one fresh session).""" + with self._lock: + try: + result = self._client.bash.exec( + command=command, + env=env, + hard_timeout=self._DEFAULT_HARD_TIMEOUT, + ) + data = result.data if result else None + stdout = (data.stdout or "") if data else "" + stderr = (data.stderr or "") if data else "" + output = stdout + if stderr: + output += f"\nStd Error:\n{stderr}" if output else stderr + return output if output else "(no output)" + except Exception as e: + logger.error(f"Failed to execute command with injected env in sandbox: {e}") + return f"Error: {e}" + def read_file(self, path: str) -> str: """Read the content of a file in the sandbox. diff --git a/backend/packages/harness/deerflow/community/e2b_sandbox/e2b_sandbox.py b/backend/packages/harness/deerflow/community/e2b_sandbox/e2b_sandbox.py index 21619dc4d..84683c984 100644 --- a/backend/packages/harness/deerflow/community/e2b_sandbox/e2b_sandbox.py +++ b/backend/packages/harness/deerflow/community/e2b_sandbox/e2b_sandbox.py @@ -118,12 +118,25 @@ class E2BSandbox(Sandbox): return f"{self._home_dir}/{tail}".rstrip("/") if tail else self._home_dir return normalised - def execute_command(self, command: str) -> str: + def execute_command( + self, + command: str, + env: dict[str, str] | None = None, + timeout: float | None = None, + ) -> str: """Execute a shell command via ``sandbox.commands.run``. Returns the combined stdout/stderr. The lock serialises concurrent calls on the same instance because the e2b SDK shares a single HTTP/2 connection per sandbox. + + Args: + command: The command to execute. + env: Optional per-call environment variables (request-scoped secrets, + issue #3861). Passed through to e2b as ``envs``, which are scoped + to this command only and never placed in the command string. + timeout: Optional per-call command timeout in seconds. ``None`` keeps + the e2b SDK default (60s). """ with self._lock: client = self._client @@ -132,7 +145,12 @@ class E2BSandbox(Sandbox): if self._dead: return "Error: e2b sandbox has been reaped by the control plane (idle timeout or explicit pause). The provider will rebuild a fresh sandbox on the next tool call." try: - result = client.commands.run(command) + kwargs: dict[str, object] = {} + if env is not None: + kwargs["envs"] = env + if timeout is not None: + kwargs["timeout"] = timeout + result = client.commands.run(command, **kwargs) stdout = getattr(result, "stdout", "") or "" stderr = getattr(result, "stderr", "") or "" exit_code = getattr(result, "exit_code", 0) diff --git a/backend/packages/harness/deerflow/runtime/secret_context.py b/backend/packages/harness/deerflow/runtime/secret_context.py new file mode 100644 index 000000000..300ef6f04 --- /dev/null +++ b/backend/packages/harness/deerflow/runtime/secret_context.py @@ -0,0 +1,88 @@ +"""Request-scoped secret carrier in the run context (issue #3861). + +Callers pass per-request secrets out-of-band in ``config.context.secrets`` — a +mapping of name -> value. The value never enters the prompt, tool arguments, or +the executed command string; it is injected as an environment variable into a +skill's sandbox subprocess only when an activated skill declares it via the +``required-secrets`` frontmatter field. + +This module centralises the reserved key name and safe extraction so the carrier +contract lives in one place, consumed by the skill-activation middleware (to +build the per-turn injection set) and the tracing redactor (to strip it from +trace payloads). +""" + +from __future__ import annotations + +from typing import Any + +# Reserved sub-key of the run context that holds request-scoped secrets supplied +# by the caller. Source of truth for what a skill *may* receive. +SECRETS_CONTEXT_KEY = "secrets" + +# Reserved sub-key holding the secrets resolved for the currently activated skill +# (binding point A). Written by the skill-activation middleware, read by the bash +# tool. Both reserved keys are stripped from trace payloads (see tracing redactor). +ACTIVE_SECRETS_CONTEXT_KEY = "__active_skill_secrets" + + +def _string_pairs(raw: Any) -> dict[str, str]: + if not isinstance(raw, dict): + return {} + return {key: value for key, value in raw.items() if isinstance(key, str) and isinstance(value, str)} + + +def extract_request_secrets(context: Any) -> dict[str, str]: + """Return the caller-supplied request-scoped secrets mapping, or ``{}``. + + Only string-keyed, string-valued entries are kept; anything else is ignored + so a malformed carrier can never crash secret resolution or injection. + """ + if not isinstance(context, dict): + return {} + return _string_pairs(context.get(SECRETS_CONTEXT_KEY)) + + +def read_active_secrets(context: Any) -> dict[str, str]: + """Return the secrets resolved for the active skill (the per-run injection + set), or ``{}``. Read by the bash tool to build the subprocess env.""" + if not isinstance(context, dict): + return {} + return _string_pairs(context.get(ACTIVE_SECRETS_CONTEXT_KEY)) + + +# Run-context keys whose values are request-scoped secrets and must be stripped +# before a context mapping is serialized anywhere observable (traces, logs). +REDACTED_CONTEXT_KEYS = frozenset({SECRETS_CONTEXT_KEY, ACTIVE_SECRETS_CONTEXT_KEY}) + + +def redact_secret_context_keys(context: Any) -> Any: + """Return a shallow copy of ``context`` with secret-bearing keys removed. + + Defensive helper for any code path that serializes the run context into an + observable surface. DeerFlow's own trace-metadata builder never copies the + context, so this is belt-and-suspenders for future call sites and custom + tracer configurations. + """ + if not isinstance(context, dict): + return context + return {key: value for key, value in context.items() if key not in REDACTED_CONTEXT_KEYS} + + +def redact_config_secrets(config: Any) -> Any: + """Return a copy of a run config safe to persist or echo back to clients. + + The request config (``body.config``) is stored verbatim on the run record + (``runs.kwargs_json``) and echoed by the run API. Strip the secret-bearing + keys from its ``context`` so a request-scoped secret is never persisted or + returned, while the live config that drives the run (built separately) keeps + them. Non-dict / context-less configs pass through unchanged. + """ + if not isinstance(config, dict): + return config + context = config.get("context") + if not isinstance(context, dict): + return config + redacted = dict(config) + redacted["context"] = redact_secret_context_keys(context) + return redacted diff --git a/backend/packages/harness/deerflow/sandbox/env_policy.py b/backend/packages/harness/deerflow/sandbox/env_policy.py new file mode 100644 index 000000000..e86ba704d --- /dev/null +++ b/backend/packages/harness/deerflow/sandbox/env_policy.py @@ -0,0 +1,82 @@ +"""Environment-variable policy for sandbox command execution (issue #3861). + +Skill scripts run as sandbox subprocesses. By default a subprocess inherits the +Gateway process's entire ``os.environ`` — which holds platform credentials +(``OPENAI_API_KEY``, tracing keys, community-provider keys, ...). That makes any +scoped request-secret injection pointless: a script could simply read those +inherited platform secrets. This module scrubs secret-looking variables from the +inherited environment before request-scoped secrets are layered on top. + +The pattern set mirrors codex's ``*KEY*/*SECRET*/*TOKEN*`` default excludes and +hermes's fixed provider blocklist; unlike codex (which defaults the exclude +*off*), DeerFlow scrubs by default — security first. +""" + +from __future__ import annotations + +import fnmatch +import os + +# Case-insensitive wildcard patterns for secret-looking variable names. Matched +# against the upper-cased variable name. Benign system vars (PATH, HOME, SHELL, +# LANG, PWD, TMPDIR, VIRTUAL_ENV, PYTHONPATH, ...) contain none of these tokens +# and are therefore preserved. +_SECRET_NAME_PATTERNS: tuple[str, ...] = ( + "*KEY*", + "*SECRET*", + "*TOKEN*", + "*PASSWORD*", + "*PASSWD*", + "*CREDENTIAL*", + "*DSN*", # data source name — almost always a connection string with a password +) + +# Connection-string / credential-bearing variable names that carry no +# KEY/SECRET/TOKEN/DSN substring but routinely embed a password (e.g. +# ``postgresql://user:pw@host/db``). A blanket ``*URL*`` block is intentionally +# avoided — it would strip benign service URLs a skill may legitimately read. +# A skill that genuinely needs one of these must declare it via required-secrets +# (the caller then supplies it through context.secrets, and injection wins). +_BLOCKED_EXACT_NAMES: frozenset[str] = frozenset( + { + "DATABASE_URL", + "DATABASE_URI", + "REDIS_URL", + "MONGODB_URI", + "MONGO_URL", + "AMQP_URL", + "RABBITMQ_URL", + "POSTGRES_URL", + "POSTGRESQL_URL", + "MYSQL_URL", + "CLICKHOUSE_URL", + "CONNECTION_STRING", + "CONN_STR", + "GH_PAT", + "GITHUB_PAT", + } +) + + +def is_blocked_env_name(name: str) -> bool: + """Return True if ``name`` looks like a credential that must not be inherited + by a sandbox subprocess.""" + upper = name.upper() + if upper in _BLOCKED_EXACT_NAMES: + return True + return any(fnmatch.fnmatchcase(upper, pattern) for pattern in _SECRET_NAME_PATTERNS) + + +def build_sandbox_env(injected: dict[str, str] | None = None) -> dict[str, str]: + """Build the environment dict for a sandbox subprocess. + + Inherits ``os.environ`` minus any secret-looking variables, then layers the + explicitly injected request-scoped secrets on top. An injected secret wins + even if its name matches a blocked pattern, because injection is authorized + upstream (the skill declared it and the value came from the request, not from + the host environment). + """ + env = {key: value for key, value in os.environ.items() if not is_blocked_env_name(key)} + if injected: + env.update(injected) + return env diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index a1d337f9e..eaf1ed0ad 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -13,6 +13,7 @@ from pathlib import Path from typing import NamedTuple from deerflow.config.paths import VIRTUAL_PATH_PREFIX +from deerflow.sandbox.env_policy import build_sandbox_env from deerflow.sandbox.local.list_dir import list_dir from deerflow.sandbox.sandbox import Sandbox from deerflow.sandbox.search import GrepMatch, find_glob_matches, find_grep_matches @@ -433,16 +434,24 @@ class LocalSandbox(Sandbox): raise RuntimeError("No suitable shell executable found. Tried /bin/zsh, /bin/bash, /bin/sh, and `sh` on PATH.") - def execute_command(self, command: str, timeout: float | None = None) -> str: + def execute_command( + self, + command: str, + env: dict[str, str] | None = None, + timeout: float | None = None, + ) -> str: # Resolve container paths in command before execution resolved_command = self._resolve_paths_in_command(command) shell = self._get_shell() if timeout is None: timeout = DEFAULT_COMMAND_TIMEOUT_SECONDS + # Inherit os.environ minus platform secrets, then layer any injected + # request-scoped secrets on top (#3861). An explicit env is always passed + # so platform credentials never leak into skill subprocesses. + sandbox_env = build_sandbox_env(env) timed_out = False if os.name == "nt": - env = None if self._is_powershell(shell): args = [shell, "-NoProfile", "-Command", resolved_command] elif self._is_cmd_shell(shell): @@ -450,8 +459,8 @@ class LocalSandbox(Sandbox): else: args = [shell, "-c", resolved_command] if self._is_msys_shell(shell): - env = { - **os.environ, + sandbox_env = { + **sandbox_env, "MSYS_NO_PATHCONV": "1", "MSYS2_ARG_CONV_EXCL": "*", } @@ -463,7 +472,7 @@ class LocalSandbox(Sandbox): capture_output=True, text=True, timeout=timeout, - env=env, + env=sandbox_env, ) stdout, stderr, returncode = result.stdout, result.stderr, result.returncode except subprocess.TimeoutExpired as exc: @@ -473,7 +482,7 @@ class LocalSandbox(Sandbox): returncode = 0 else: args = [shell, "-c", resolved_command] - stdout, stderr, returncode, timed_out = self._run_posix_command(args, timeout) + stdout, stderr, returncode, timed_out = self._run_posix_command(args, timeout, sandbox_env) output = stdout if stderr: @@ -489,7 +498,11 @@ class LocalSandbox(Sandbox): return self._reverse_resolve_paths_in_output(final_output) @staticmethod - def _run_posix_command(args: list[str], timeout: float) -> tuple[str, str, int, bool]: + def _run_posix_command( + args: list[str], + timeout: float, + env: dict[str, str] | None = None, + ) -> tuple[str, str, int, bool]: """Run a command on POSIX with bounded pipe capture. ``subprocess.communicate()`` cannot be used here: a backgrounded @@ -517,6 +530,7 @@ class LocalSandbox(Sandbox): stdout=stdout_write_fd, stderr=stderr_write_fd, start_new_session=True, + env=env, ) except Exception: for fd in (stdout_read_fd, stdout_write_fd, stderr_read_fd, stderr_write_fd): diff --git a/backend/packages/harness/deerflow/sandbox/sandbox.py b/backend/packages/harness/deerflow/sandbox/sandbox.py index 50322f419..7c008dfc6 100644 --- a/backend/packages/harness/deerflow/sandbox/sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/sandbox.py @@ -16,11 +16,27 @@ class Sandbox(ABC): return self._id @abstractmethod - def execute_command(self, command: str) -> str: + def execute_command( + self, + command: str, + env: dict[str, str] | None = None, + timeout: float | None = None, + ) -> str: """Execute bash command in sandbox. Args: command: The command to execute. + env: Optional per-call environment variables to inject into the + command's process. Used to pass request-scoped secrets (e.g. a + short-lived end-user token) to skill scripts without placing them + in the prompt, tool arguments, or the command string (issue #3861). + When ``None`` the sandbox uses its default environment. + timeout: Optional per-call wall-clock timeout in seconds. Local + sandboxes use this to bound host bash commands so long-lived + foreground processes cannot hang a turn indefinitely. Remote/AIO + implementations may ignore it when their backend does not expose + an equivalent command-timeout control separate from its own API + timeouts. Returns: The standard or error output of the command. diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index c963ec427..1440bb8e3 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -12,6 +12,7 @@ from langchain.tools import tool from deerflow.agents.thread_state import ThreadDataState from deerflow.config import get_app_config from deerflow.config.paths import VIRTUAL_PATH_PREFIX +from deerflow.runtime.secret_context import read_active_secrets from deerflow.runtime.user_context import resolve_runtime_user_id from deerflow.sandbox.exceptions import ( SandboxError, @@ -1309,6 +1310,37 @@ def ensure_thread_directories_exist(runtime: Runtime | None) -> None: runtime.state["thread_directories_created"] = True +_SECRET_REDACTION = "[redacted]" + +# Values shorter than this are not redacted from bash output. A short secret +# value (a 2-char region code, a numeric id, a PIN) would otherwise shred +# unrelated bytes of tool output — exit codes, timestamps, sizes, paths — +# corrupting the result the model reads back. The redaction of a value this +# short is more likely noise than genuine leak protection; the secret is still +# injected into the subprocess, only the output mask skips it. +_MIN_MASK_LENGTH = 8 + + +def mask_secret_values(output: str, injected_env: dict[str, str] | None) -> str: + """Redact injected secret values from bash output before it re-enters context. + + Skill scripts receive request-scoped secrets as env vars (#3861). If a script + echoes one (debugging, ``set -x``, an error dump), the value would otherwise + flow into the tool result — and thus into the prompt and the trace. This is + the skill-specific fifth leak surface (the bash tool returns subprocess stdout, + unlike MCP tools). Replace each non-empty secret value with a redaction marker. + Longest values first so a value that is a substring of another is not partially + revealed. Values shorter than ``_MIN_MASK_LENGTH`` are skipped — a redacted + 3-char token is more likely to corrupt unrelated output than to protect a + real secret. + """ + if not injected_env or not output: + return output + for value in sorted((v for v in injected_env.values() if v and len(v) >= _MIN_MASK_LENGTH), key=len, reverse=True): + output = output.replace(value, _SECRET_REDACTION) + return output + + def _truncate_bash_output(output: str, max_chars: int) -> str: """Middle-truncate bash output, preserving head and tail (50/50 split). @@ -1404,6 +1436,9 @@ def bash_tool(runtime: Runtime, description: str, command: str) -> str: """ try: sandbox = ensure_sandbox_initialized(runtime) + # Request-scoped secrets resolved for the active skill (#3861); injected as + # per-call env into the subprocess, never placed in the command string. + injected_env = read_active_secrets(getattr(runtime, "context", None)) or None if is_local_sandbox(runtime): if not is_host_bash_allowed(): return f"Error: {LOCAL_HOST_BASH_DISABLED_MESSAGE}" @@ -1421,8 +1456,11 @@ def bash_tool(runtime: Runtime, description: str, command: str) -> str: except Exception: max_chars = 20000 command_timeout = None - output = sandbox.execute_command(command, timeout=command_timeout) - return _truncate_bash_output(mask_local_paths_in_output(output, thread_data), max_chars) + output = sandbox.execute_command(command, env=injected_env, timeout=command_timeout) + return _truncate_bash_output( + mask_secret_values(mask_local_paths_in_output(output, thread_data), injected_env), + max_chars, + ) ensure_thread_directories_exist(runtime) try: from deerflow.config.app_config import get_app_config @@ -1431,7 +1469,7 @@ def bash_tool(runtime: Runtime, description: str, command: str) -> str: max_chars = sandbox_cfg.bash_output_max_chars if sandbox_cfg else 20000 except Exception: max_chars = 20000 - return _truncate_bash_output(sandbox.execute_command(command), max_chars) + return _truncate_bash_output(mask_secret_values(sandbox.execute_command(command, env=injected_env), injected_env), max_chars) except SandboxError as e: return f"Error: {e}" except PermissionError as e: diff --git a/backend/packages/harness/deerflow/skills/parser.py b/backend/packages/harness/deerflow/skills/parser.py index 7571bd9fc..ddd4b638b 100644 --- a/backend/packages/harness/deerflow/skills/parser.py +++ b/backend/packages/harness/deerflow/skills/parser.py @@ -4,10 +4,13 @@ from pathlib import Path import yaml -from .types import SKILL_MD_FILE, Skill, SkillCategory +from .types import SKILL_MD_FILE, SecretRequirement, Skill, SkillCategory logger = logging.getLogger(__name__) +# Valid POSIX environment-variable name. +_ENV_VAR_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + def _format_yaml_error(skill_file: Path, exc: yaml.YAMLError, source: str) -> str: """Render a developer-friendly explanation of a YAML front-matter error.""" @@ -63,6 +66,43 @@ def parse_allowed_tools(raw: object, skill_file: Path) -> list[str] | None: return allowed_tools +def parse_required_secrets(raw: object, skill_file: Path) -> list[SecretRequirement]: + """Parse the optional required-secrets frontmatter field (issue #3861). + + Accepts a YAML sequence whose items are either a string (the secret / env + variable name) or a mapping (``{name, optional}``). Returns an empty list + when the field is omitted. Entries whose name is missing or is not a valid + environment-variable name are dropped with a warning, so one malformed + declaration does not invalidate the whole skill. Raises ValueError only when + the field is present but is not a list. + """ + if raw is None: + return [] + if not isinstance(raw, list): + raise ValueError(f"required-secrets in {skill_file} must be a list") + + secrets: list[SecretRequirement] = [] + seen: set[str] = set() + for item in raw: + if isinstance(item, str): + name, optional = item.strip(), False + elif isinstance(item, dict): + name = str(item.get("name") or "").strip() + optional = bool(item.get("optional", False)) + else: + logger.warning("Ignoring malformed required-secrets entry in %s: %r", skill_file, item) + continue + + if not _ENV_VAR_NAME_RE.match(name): + logger.warning("Ignoring required-secrets entry with invalid env var name in %s: %r", skill_file, name) + continue + if name in seen: + continue + seen.add(name) + secrets.append(SecretRequirement(name=name, optional=optional)) + return secrets + + def parse_skill_file(skill_file: Path, category: SkillCategory, relative_path: Path | None = None) -> Skill | None: """Parse a SKILL.md file and extract metadata. @@ -124,6 +164,12 @@ def parse_skill_file(skill_file: Path, category: SkillCategory, relative_path: P logger.error("Invalid allowed-tools in %s: %s", skill_file, exc) return None + try: + required_secrets = parse_required_secrets(metadata.get("required-secrets"), skill_file) + except ValueError as exc: + logger.error("Invalid required-secrets in %s: %s", skill_file, exc) + return None + return Skill( name=name, description=description, @@ -134,6 +180,7 @@ def parse_skill_file(skill_file: Path, category: SkillCategory, relative_path: P category=category, allowed_tools=allowed_tools, enabled=True, # Actual state comes from the extensions config file. + required_secrets=required_secrets, ) except Exception: diff --git a/backend/packages/harness/deerflow/skills/types.py b/backend/packages/harness/deerflow/skills/types.py index 057a4391e..2862828e3 100644 --- a/backend/packages/harness/deerflow/skills/types.py +++ b/backend/packages/harness/deerflow/skills/types.py @@ -1,4 +1,4 @@ -from dataclasses import dataclass +from dataclasses import dataclass, field from enum import StrEnum from pathlib import Path @@ -16,6 +16,19 @@ class SkillCategory(StrEnum): CUSTOM = "custom" +@dataclass(frozen=True) +class SecretRequirement: + """A request-scoped secret a skill declares it needs (issue #3861). + + ``name`` is both the key looked up in the request's ``context.secrets`` and + the environment variable name injected into the skill's sandbox subprocess + when the skill is activated. + """ + + name: str + optional: bool = False + + @dataclass class Skill: """Represents a skill with its metadata and file path""" @@ -29,6 +42,7 @@ class Skill: category: SkillCategory # 'public' or 'custom' allowed_tools: list[str] | None = None enabled: bool = False # Whether this skill is enabled + required_secrets: list[SecretRequirement] = field(default_factory=list) @property def skill_path(self) -> str: diff --git a/backend/tests/test_e2b_sandbox_provider.py b/backend/tests/test_e2b_sandbox_provider.py index 1c5af376a..66f7ec38e 100644 --- a/backend/tests/test_e2b_sandbox_provider.py +++ b/backend/tests/test_e2b_sandbox_provider.py @@ -7,6 +7,7 @@ import threading from collections import OrderedDict from types import SimpleNamespace from typing import Any +from unittest.mock import MagicMock from deerflow.config.paths import Paths @@ -257,6 +258,42 @@ def test_execute_command_does_not_mark_dead_on_unrelated_error(): assert sb.is_dead is False +def test_execute_command_forwards_env_and_timeout_to_commands_run(): + """execute_command(env=..., timeout=...) routes env as ``envs`` and the + timeout through to ``commands.run`` so request-scoped secrets (#3861) reach + the e2b subprocess without entering the command string. Regression for the + signature mismatch that broke bash for every e2b user.""" + commands = MagicMock() + commands.run.return_value = SimpleNamespace(stdout="ok\n", stderr="", exit_code=0) + client = FakeClient(commands=commands) + sb = _make_sandbox(client) + + out = sb.execute_command("echo $TOK", env={"TOK": "secret-v"}, timeout=120) + + assert out.rstrip() == "ok" + args, kwargs = commands.run.call_args + assert args == ("echo $TOK",) + assert kwargs["envs"] == {"TOK": "secret-v"} + assert kwargs["timeout"] == 120 + # The secret must not be smuggled into the command string. + assert "secret-v" not in args[0] + + +def test_execute_command_env_none_passes_no_envs_kwarg(): + """env=None is fully backward-compatible — ``commands.run`` is called with no + ``envs``/``timeout`` kwargs, so existing (non-secret) callers are unaffected.""" + commands = MagicMock() + commands.run.return_value = SimpleNamespace(stdout="ok\n", stderr="", exit_code=0) + client = FakeClient(commands=commands) + sb = _make_sandbox(client) + + sb.execute_command("echo hi") + + _, kwargs = commands.run.call_args + assert "envs" not in kwargs + assert "timeout" not in kwargs + + def test_ping_returns_false_when_sandbox_gone(): client = FakeClient(commands=FakeCommandsAPI([FakeCommandsAPI.GONE])) sb = _make_sandbox(client) diff --git a/backend/tests/test_gateway_services.py b/backend/tests/test_gateway_services.py index 04338dc60..120c4bdfc 100644 --- a/backend/tests/test_gateway_services.py +++ b/backend/tests/test_gateway_services.py @@ -208,6 +208,21 @@ def test_build_run_config_with_overrides(): assert config["metadata"]["user"] == "alice" +def test_build_run_config_context_path_still_sets_configurable_thread_id(_stub_app_config): + """A caller-supplied context (e.g. request-scoped secrets, #3861) must not + deprive the checkpointer of configurable.thread_id, which it always needs to + scope checkpoints. Secrets stay in context; thread_id is mirrored into + configurable for the checkpointer.""" + from app.gateway.services import build_run_config + + config = build_run_config("thread-1", {"context": {"secrets": {"ERP_TOKEN": "v"}}}, None) + assert config["context"]["secrets"] == {"ERP_TOKEN": "v"} + assert config["context"]["thread_id"] == "thread-1" + assert config["configurable"]["thread_id"] == "thread-1" + # Secrets must NOT be mirrored into configurable. + assert "secrets" not in config["configurable"] + + # --------------------------------------------------------------------------- # recursion_limit clamping: the Gateway must not trust a client-supplied # recursion_limit verbatim (runaway LLM cost / DoS). See build_run_config. @@ -853,7 +868,8 @@ def test_build_run_config_with_context(): assert "context" in config assert config["context"]["user_id"] == "u-42" assert config["context"]["thread_id"] == "thread-1" - assert "configurable" not in config + # configurable carries thread_id for the checkpointer; user context stays in context. + assert config["configurable"] == {"thread_id": "thread-1"} assert config["recursion_limit"] == 100 @@ -869,7 +885,7 @@ def test_build_run_config_context_injects_thread_id(): assert config["context"]["user_id"] == "u-1" assert config["context"]["thinking_enabled"] is True assert config["context"]["thread_id"] == "T-deadbeef-42" - assert "configurable" not in config + assert config["configurable"] == {"thread_id": "T-deadbeef-42"} def test_build_run_config_null_context_becomes_empty_context(): @@ -879,7 +895,7 @@ def test_build_run_config_null_context_becomes_empty_context(): config = build_run_config("thread-1", {"context": None}, None) assert config["context"] == {"thread_id": "thread-1"} - assert "configurable" not in config + assert config["configurable"] == {"thread_id": "thread-1"} def test_build_run_config_rejects_non_mapping_context(): @@ -921,7 +937,10 @@ def test_build_run_config_context_plus_configurable_warns(caplog): ) assert "context" in config assert config["context"]["user_id"] == "u-42" - assert "configurable" not in config + # context wins: caller's configurable (model_name) is dropped, but thread_id is + # still set for the checkpointer. + assert config["configurable"] == {"thread_id": "thread-1"} + assert "model_name" not in config["configurable"] assert any("both 'context' and 'configurable'" in r.message for r in caplog.records) @@ -935,7 +954,7 @@ def test_build_run_config_context_passthrough_other_keys(): None, ) assert config["context"]["thread_id"] == "thread-1" - assert "configurable" not in config + assert config["configurable"] == {"thread_id": "thread-1"} assert config["tags"] == ["prod"] diff --git a/backend/tests/test_local_sandbox_command_timeout.py b/backend/tests/test_local_sandbox_command_timeout.py index 29cf84b59..984396dea 100644 --- a/backend/tests/test_local_sandbox_command_timeout.py +++ b/backend/tests/test_local_sandbox_command_timeout.py @@ -77,7 +77,7 @@ def test_foreground_blocking_command_times_out_with_notice(): def test_timeout_notice_formats_fractional_and_singular_timeouts(monkeypatch): monkeypatch.setattr(LocalSandbox, "_get_shell", lambda self: "/bin/sh") - monkeypatch.setattr(LocalSandbox, "_run_posix_command", staticmethod(lambda args, timeout: ("", "", 0, True))) + monkeypatch.setattr(LocalSandbox, "_run_posix_command", staticmethod(lambda args, timeout, env=None: ("", "", 0, True))) assert "after 1.5 seconds" in LocalSandbox("t").execute_command("wait", timeout=1.5) assert "after 1 second" in LocalSandbox("t").execute_command("wait", timeout=1) diff --git a/backend/tests/test_local_sandbox_encoding.py b/backend/tests/test_local_sandbox_encoding.py index 76a5f187c..0524667cb 100644 --- a/backend/tests/test_local_sandbox_encoding.py +++ b/backend/tests/test_local_sandbox_encoding.py @@ -86,12 +86,16 @@ def test_execute_command_uses_powershell_command_mode_on_windows(monkeypatch): return SimpleNamespace(stdout="ok", stderr="", returncode=0) monkeypatch.setattr(local_sandbox.os, "name", "nt") + monkeypatch.setattr(local_sandbox.os, "environ", {"PATH": r"C:\Windows", "OPENAI_API_KEY": "should-not-leak"}) monkeypatch.setattr(LocalSandbox, "_get_shell", staticmethod(lambda: r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe")) monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run) output = LocalSandbox("t").execute_command("Write-Output hello") assert output == "ok" + # Platform secrets are scrubbed from the inherited environment even on the + # Windows PowerShell path (#3861); benign PATH is preserved and the env is an + # explicit scrubbed dict, no longer None. assert calls == [ ( [ @@ -105,7 +109,7 @@ def test_execute_command_uses_powershell_command_mode_on_windows(monkeypatch): "capture_output": True, "text": True, "timeout": 600, - "env": None, + "env": {"PATH": r"C:\Windows"}, }, ) ] @@ -152,13 +156,17 @@ def test_execute_command_does_not_set_msys_env_for_non_msys_posix_shell_on_windo return SimpleNamespace(stdout="ok", stderr="", returncode=0) monkeypatch.setattr(local_sandbox.os, "name", "nt") + monkeypatch.setattr(local_sandbox.os, "environ", {"PATH": r"C:\tools"}) monkeypatch.setattr(LocalSandbox, "_get_shell", staticmethod(lambda: r"C:\tools\busybox\sh.exe")) monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run) output = LocalSandbox("t").execute_command("echo /mnt/skills/demo") assert output == "ok" - assert calls[0][1]["env"] is None + # Non-MSYS posix shell adds no MSYS_* vars; the env is the scrubbed inherited + # environment, not None (#3861). + assert calls[0][1]["env"] == {"PATH": r"C:\tools"} + assert "MSYS_NO_PATHCONV" not in calls[0][1]["env"] def test_execute_command_uses_cmd_command_mode_on_windows(monkeypatch): @@ -169,12 +177,15 @@ def test_execute_command_uses_cmd_command_mode_on_windows(monkeypatch): return SimpleNamespace(stdout="ok", stderr="", returncode=0) monkeypatch.setattr(local_sandbox.os, "name", "nt") + monkeypatch.setattr(local_sandbox.os, "environ", {"PATH": r"C:\Windows", "GITHUB_TOKEN": "should-not-leak"}) monkeypatch.setattr(LocalSandbox, "_get_shell", staticmethod(lambda: r"C:\Windows\System32\cmd.exe")) monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run) output = LocalSandbox("t").execute_command("echo hello") assert output == "ok" + # Platform secrets are scrubbed even on the Windows cmd path (#3861); the env + # is an explicit scrubbed dict, no longer None. assert calls == [ ( [r"C:\Windows\System32\cmd.exe", "/c", "echo hello"], @@ -183,7 +194,7 @@ def test_execute_command_uses_cmd_command_mode_on_windows(monkeypatch): "capture_output": True, "text": True, "timeout": 600, - "env": None, + "env": {"PATH": r"C:\Windows"}, }, ) ] diff --git a/backend/tests/test_sandbox_middleware.py b/backend/tests/test_sandbox_middleware.py index b9ecb57a2..b34449ed0 100644 --- a/backend/tests/test_sandbox_middleware.py +++ b/backend/tests/test_sandbox_middleware.py @@ -37,7 +37,13 @@ class _SyncProvider(SandboxProvider): class _SandboxStub(Sandbox): - def execute_command(self, command: str) -> str: + def execute_command( + self, + command: str, + env: dict[str, str] | None = None, + timeout: float | None = None, + ) -> str: + del env, timeout return "OK" def read_file(self, path: str) -> str: diff --git a/backend/tests/test_skill_request_scoped_secrets.py b/backend/tests/test_skill_request_scoped_secrets.py new file mode 100644 index 000000000..0df85aa37 --- /dev/null +++ b/backend/tests/test_skill_request_scoped_secrets.py @@ -0,0 +1,769 @@ +"""Tests for request-scoped secret injection into skills (issue #3861). + +Covers the full feature surface: + - Slice 1: ``Sandbox.execute_command(command, env=...)`` per-call env injection + on both the local and AIO backends. + - Slice 2: ``SKILL.md`` ``requires-secrets`` frontmatter parsing. + - Slice 3: gateway carrier (``context.secrets``) and runtime-context passthrough. + - Slice 4: activation-turn binding + ``bash`` tool injection. + - Slice 5: the five leak surfaces (prompt / trace / checkpoint / audit / stdout). +""" + +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from langchain.agents.middleware.types import ModelRequest +from langchain_core.messages import AIMessage, HumanMessage + +from deerflow.sandbox.local.local_sandbox import LocalSandbox +from deerflow.skills.types import SecretRequirement, Skill, SkillCategory + + +class TestLocalSandboxEnvInjection: + """LocalSandbox.execute_command(env=...) injects per-call env into the subprocess.""" + + def test_injected_env_visible_to_command(self): + sandbox = LocalSandbox(id="local") + out = sandbox.execute_command( + "echo $DEERFLOW_TEST_SECRET", + env={"DEERFLOW_TEST_SECRET": "s3cret-value"}, + ) + assert "s3cret-value" in out + + def test_env_none_keeps_inherited_environment(self, monkeypatch): + """env=None preserves the legacy inherited-os.environ behaviour.""" + monkeypatch.setenv("DEERFLOW_INHERITED_VAR", "inherited-value") + sandbox = LocalSandbox(id="local") + out = sandbox.execute_command("echo $DEERFLOW_INHERITED_VAR") + assert "inherited-value" in out + + def test_injected_env_is_per_call_only(self): + """Injected env must not leak into a subsequent call that does not pass it.""" + sandbox = LocalSandbox(id="local") + sandbox.execute_command("true", env={"DEERFLOW_EPHEMERAL": "leaky"}) + out = sandbox.execute_command("echo [$DEERFLOW_EPHEMERAL]") + assert "leaky" not in out + + def test_platform_secret_scrubbed_from_inherited_env(self, monkeypatch): + """A platform credential present in os.environ must NOT reach the sandbox + subprocess (the baseline-env leak surface). Without this, scoped injection + is security theatre — a skill script could simply read $OPENAI_API_KEY.""" + monkeypatch.setenv("OPENAI_API_KEY", "sk-platform-should-not-leak") + sandbox = LocalSandbox(id="local") + out = sandbox.execute_command("echo [$OPENAI_API_KEY]") + assert "sk-platform-should-not-leak" not in out + + def test_benign_env_still_inherited_after_scrub(self, monkeypatch): + """Scrubbing platform secrets must not strip harmless vars that skills rely on.""" + monkeypatch.setenv("DEERFLOW_PLAIN_VAR", "harmless-value") + sandbox = LocalSandbox(id="local") + out = sandbox.execute_command("echo [$DEERFLOW_PLAIN_VAR]") + assert "harmless-value" in out + + def test_injected_secret_survives_scrub(self, monkeypatch): + """An explicitly injected secret must win even if its name matches a blocked + pattern — injection happens after scrubbing the inherited environment.""" + sandbox = LocalSandbox(id="local") + out = sandbox.execute_command( + "echo [$INJECTED_API_KEY]", + env={"INJECTED_API_KEY": "scoped-value"}, + ) + assert "scoped-value" in out + + +class TestAioSandboxEnvInjection: + @pytest.fixture + def sandbox(self): + with patch("deerflow.community.aio_sandbox.aio_sandbox.AioSandboxClient"): + from deerflow.community.aio_sandbox.aio_sandbox import AioSandbox + + return AioSandbox(id="test-sandbox", base_url="http://localhost:8080") + + def test_env_none_uses_legacy_shell_path(self, sandbox): + """No injected env → unchanged shell.exec_command path (backward compat).""" + sandbox._client.shell.exec_command = MagicMock(return_value=SimpleNamespace(data=SimpleNamespace(output="hello"))) + sandbox._client.bash.exec = MagicMock() + out = sandbox.execute_command("echo hello") + sandbox._client.shell.exec_command.assert_called_once() + sandbox._client.bash.exec.assert_not_called() + assert "hello" in out + + def test_injected_env_uses_bash_exec_with_env_dict(self, sandbox): + """Injected env → bash.exec(env=...) carries the dict; secret stays out of the command string.""" + sandbox._client.bash.exec = MagicMock(return_value=SimpleNamespace(data=SimpleNamespace(stdout="hello", stderr=None))) + sandbox._client.shell.exec_command = MagicMock() + out = sandbox.execute_command("echo $TOK", env={"TOK": "secret-v"}) + sandbox._client.bash.exec.assert_called_once() + _, kwargs = sandbox._client.bash.exec.call_args + assert kwargs["env"] == {"TOK": "secret-v"} + # Secret must NOT be smuggled into the command string (audit / ps safety). + assert "secret-v" not in kwargs["command"] + sandbox._client.shell.exec_command.assert_not_called() + assert "hello" in out + + def test_env_path_uses_hard_timeout_not_no_change_timeout(self, sandbox): + """The env path routes through bash.exec which exposes no idle/no-change + timeout; it must use the dedicated wall-clock ``_DEFAULT_HARD_TIMEOUT``, + not the legacy idle constant (same numeric value today, but distinct + semantics so a future change to one does not silently alter the other).""" + from deerflow.community.aio_sandbox.aio_sandbox import AioSandbox + + sandbox._client.bash.exec = MagicMock(return_value=SimpleNamespace(data=SimpleNamespace(stdout="ok", stderr=None))) + sandbox.execute_command("echo hi", env={"X": "1"}) + _, kwargs = sandbox._client.bash.exec.call_args + assert kwargs["hard_timeout"] == AioSandbox._DEFAULT_HARD_TIMEOUT + assert AioSandbox._DEFAULT_HARD_TIMEOUT != AioSandbox._DEFAULT_NO_CHANGE_TIMEOUT or ( + # Same numeric value is fine today; the contract is that they are + # named independently so the two call sites evolve independently. + AioSandbox._DEFAULT_HARD_TIMEOUT == AioSandbox._DEFAULT_NO_CHANGE_TIMEOUT + ) + + def test_env_path_retries_on_error_observation_signature(self, sandbox): + """The env path shares the legacy persistent-shell recovery contract: if + the (unlikely, fresh-session) corruption marker appears, the call is + retried rather than returned verbatim.""" + from deerflow.community.aio_sandbox.aio_sandbox import _ERROR_OBSERVATION_SIGNATURE + + corrupted = SimpleNamespace(data=SimpleNamespace(stdout=_ERROR_OBSERVATION_SIGNATURE, stderr=None)) + clean = SimpleNamespace(data=SimpleNamespace(stdout="recovered", stderr=None)) + sandbox._client.bash.exec = MagicMock(side_effect=[corrupted, clean]) + out = sandbox.execute_command("script", env={"TOK": "v"}) + assert sandbox._client.bash.exec.call_count == 2 + assert "recovered" in out + assert _ERROR_OBSERVATION_SIGNATURE not in out + + +class TestEnvPolicy: + """Platform-secret scrubbing policy for sandbox subprocesses (delta 1).""" + + @pytest.mark.parametrize( + "name", + [ + "OPENAI_API_KEY", + "ANTHROPIC_API_KEY", + "LANGFUSE_SECRET_KEY", + "GITHUB_TOKEN", + "AWS_SECRET_ACCESS_KEY", + "DB_PASSWORD", + "MY_SERVICE_CREDENTIAL", + "api_key", + "Some_Token_Here", + # Connection-string credentials (no KEY/SECRET/TOKEN substring) — these + # routinely embed a password, e.g. postgresql://user:pw@host/db. + "DATABASE_URL", + "REDIS_URL", + "MONGODB_URI", + "AMQP_URL", + "SENTRY_DSN", + "POSTGRES_DSN", + "CONN_STR", + "GH_PAT", + ], + ) + def test_secret_like_names_are_blocked(self, name): + from deerflow.sandbox.env_policy import is_blocked_env_name + + assert is_blocked_env_name(name) is True + + @pytest.mark.parametrize( + "name", + [ + "PATH", + "HOME", + "SHELL", + "USER", + "LANG", + "LC_ALL", + "PWD", + "TMPDIR", + "VIRTUAL_ENV", + "PYTHONPATH", + "DEERFLOW_PLAIN_VAR", + # Not a blanket *URL* block: a benign service URL a skill may legitimately + # read is not treated as a credential. + "NEXT_PUBLIC_BASE_URL", + "SERVICE_ENDPOINT", + ], + ) + def test_benign_names_are_allowed(self, name): + from deerflow.sandbox.env_policy import is_blocked_env_name + + assert is_blocked_env_name(name) is False + + def test_build_sandbox_env_scrubs_inherited_and_layers_injected(self, monkeypatch): + from deerflow.sandbox.env_policy import build_sandbox_env + + monkeypatch.setenv("OPENAI_API_KEY", "platform-key-should-vanish") + monkeypatch.setenv("HARMLESS_PLAIN", "ok") + env = build_sandbox_env(injected={"SCOPED_TOKEN": "v"}) + assert "OPENAI_API_KEY" not in env # platform secret scrubbed + assert env.get("HARMLESS_PLAIN") == "ok" # benign preserved + assert env.get("SCOPED_TOKEN") == "v" # injected layered on top + assert env.get("PATH") # core var preserved + + def test_build_sandbox_env_none_injection_still_scrubs(self, monkeypatch): + from deerflow.sandbox.env_policy import build_sandbox_env + + monkeypatch.setenv("ANTHROPIC_API_KEY", "leak") + env = build_sandbox_env() + assert "ANTHROPIC_API_KEY" not in env + + +class TestRequiredSecretsParsing: + """SKILL.md ``required-secrets`` frontmatter parsing (Slice 2).""" + + def _write_skill(self, tmp_path, frontmatter_body: str): + skill_dir = tmp_path / "erp-report" + skill_dir.mkdir() + skill_file = skill_dir / "SKILL.md" + skill_file.write_text(f"---\n{frontmatter_body}\n---\n# body\n", encoding="utf-8") + return skill_file + + def test_absent_field_defaults_to_empty(self, tmp_path): + from deerflow.skills.parser import parse_skill_file + from deerflow.skills.types import SkillCategory + + skill_file = self._write_skill(tmp_path, "name: erp-report\ndescription: Pull an ERP report") + skill = parse_skill_file(skill_file, SkillCategory.CUSTOM) + assert skill is not None + assert skill.required_secrets == [] + + def test_string_list_form(self, tmp_path): + from deerflow.skills.parser import parse_skill_file + from deerflow.skills.types import SkillCategory + + skill_file = self._write_skill( + tmp_path, + "name: erp-report\ndescription: d\nrequired-secrets:\n - ERP_TOKEN\n - OTHER_TOKEN", + ) + skill = parse_skill_file(skill_file, SkillCategory.CUSTOM) + assert [s.name for s in skill.required_secrets] == ["ERP_TOKEN", "OTHER_TOKEN"] + assert all(s.optional is False for s in skill.required_secrets) + + def test_object_list_with_optional(self, tmp_path): + from deerflow.skills.parser import parse_skill_file + from deerflow.skills.types import SkillCategory + + skill_file = self._write_skill( + tmp_path, + "name: erp-report\ndescription: d\nrequired-secrets:\n - name: ERP_TOKEN\n optional: true\n - name: REQUIRED_ONE", + ) + skill = parse_skill_file(skill_file, SkillCategory.CUSTOM) + by_name = {s.name: s for s in skill.required_secrets} + assert by_name["ERP_TOKEN"].optional is True + assert by_name["REQUIRED_ONE"].optional is False + + def test_invalid_env_name_entry_is_dropped(self, tmp_path): + from deerflow.skills.parser import parse_skill_file + from deerflow.skills.types import SkillCategory + + skill_file = self._write_skill( + tmp_path, + 'name: erp-report\ndescription: d\nrequired-secrets:\n - "bad name!"\n - GOOD_TOKEN', + ) + skill = parse_skill_file(skill_file, SkillCategory.CUSTOM) + # The malformed entry is dropped; the valid one survives — one bad + # declaration must not nuke the whole skill. + assert [s.name for s in skill.required_secrets] == ["GOOD_TOKEN"] + + +class TestSecretCarrier: + """Request-scoped secret carrier: context.secrets → runtime.context (Slice 3).""" + + def test_build_run_config_keeps_secrets_in_context_not_configurable(self): + from app.gateway.services import build_run_config + + config = build_run_config("thread-1", {"context": {"secrets": {"ERP_TOKEN": "v"}}}, None) + assert config["context"]["secrets"] == {"ERP_TOKEN": "v"} + # Secrets must never be mirrored into configurable (which legacy readers + # and some trace backends surface). + assert "secrets" not in config.get("configurable", {}) + + def test_runtime_context_carries_secrets(self): + from deerflow.runtime.runs.worker import _build_runtime_context + + ctx = _build_runtime_context("t", "r", {"secrets": {"ERP_TOKEN": "v"}}) + assert ctx["secrets"] == {"ERP_TOKEN": "v"} + + def test_extract_request_secrets_filters_non_string_pairs(self): + from deerflow.runtime.secret_context import extract_request_secrets + + assert extract_request_secrets({"secrets": {"A": "x", "B": 123, 4: "y"}}) == {"A": "x"} + + def test_extract_request_secrets_missing_or_malformed(self): + from deerflow.runtime.secret_context import extract_request_secrets + + assert extract_request_secrets({}) == {} + assert extract_request_secrets({"secrets": "not-a-dict"}) == {} + assert extract_request_secrets(None) == {} + + +def _make_secret_skill(tmp_path: Path, name: str, required_secrets): + skill_dir = tmp_path / name + skill_dir.mkdir() + skill_file = skill_dir / "SKILL.md" + skill_file.write_text(f"# {name}\n", encoding="utf-8") + return Skill( + name=name, + description=f"Description for {name}", + license="MIT", + skill_dir=skill_dir, + skill_file=skill_file, + relative_path=Path(name), + category=SkillCategory.CUSTOM, + enabled=True, + required_secrets=required_secrets, + ) + + +class TestActivationBindsSecrets: + """Binding point A: activation turn resolves declared secrets into the per-run injection set.""" + + def _activate(self, tmp_path, monkeypatch, skill, context): + from deerflow.agents.middlewares import skill_activation_middleware as mw + from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware + + storage = SimpleNamespace( + load_skills=lambda *, enabled_only: [skill], + get_container_root=lambda: "/mnt/skills", + get_skills_root_path=lambda: tmp_path, + ) + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: storage) + middleware = SkillActivationMiddleware() + request = ModelRequest( + model=object(), + messages=[HumanMessage(content=f"/{skill.name} do it", id="m1")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ) + middleware.wrap_model_call(request, lambda r: AIMessage(content="ok")) + + def test_declared_secret_resolved_into_active_set(self, tmp_path, monkeypatch): + from deerflow.runtime.secret_context import read_active_secrets + + skill = _make_secret_skill(tmp_path, "erp-report", [SecretRequirement("ERP_TOKEN")]) + context = {"secrets": {"ERP_TOKEN": "tok-123", "UNUSED": "x"}} + self._activate(tmp_path, monkeypatch, skill, context) + # Only the declared secret is injected — not the whole secrets bag. + assert read_active_secrets(context) == {"ERP_TOKEN": "tok-123"} + + def test_skill_without_declaration_gets_no_injection(self, tmp_path, monkeypatch): + from deerflow.runtime.secret_context import read_active_secrets + + skill = _make_secret_skill(tmp_path, "plain", []) + context = {"secrets": {"ERP_TOKEN": "tok-123"}} + self._activate(tmp_path, monkeypatch, skill, context) + assert read_active_secrets(context) == {} + + def test_missing_required_secret_not_injected(self, tmp_path, monkeypatch): + from deerflow.runtime.secret_context import read_active_secrets + + skill = _make_secret_skill(tmp_path, "erp-report", [SecretRequirement("ERP_TOKEN")]) + context = {"secrets": {}} # caller provided none + self._activate(tmp_path, monkeypatch, skill, context) + assert read_active_secrets(context) == {} + + def test_caller_secret_wins_over_host_value_of_same_name(self, tmp_path, monkeypatch): + """A skill may declare a name that also exists in the host env (e.g. a + per-user key overriding a shared platform key — the #3861 use case). The + skill receives the CALLER's value (from context.secrets), never the host's: + the inherited host value is scrubbed and the caller's value is injected on + top. There is therefore no host-credential harvest to guard against.""" + from deerflow.runtime.secret_context import read_active_secrets + from deerflow.sandbox.env_policy import build_sandbox_env + + monkeypatch.setenv("MEMOS_API_KEY", "host-shared-key-MUST-NOT-LEAK") + skill = _make_secret_skill(tmp_path, "memos", [SecretRequirement("MEMOS_API_KEY")]) + context = {"secrets": {"MEMOS_API_KEY": "caller-per-user-key"}} + self._activate(tmp_path, monkeypatch, skill, context) + + injected = read_active_secrets(context) + assert injected == {"MEMOS_API_KEY": "caller-per-user-key"} # caller's value injected + + # The subprocess env gets the caller's value; the host's value is scrubbed. + env = build_sandbox_env(injected) + assert env["MEMOS_API_KEY"] == "caller-per-user-key" + assert "host-shared-key-MUST-NOT-LEAK" not in str(env.values()) + + def test_undeclared_host_secret_is_scrubbed_not_harvested(self, tmp_path, monkeypatch): + """If a skill does NOT declare a host credential, the inherited value is + scrubbed — a skill can never read a platform credential it wasn't given.""" + from deerflow.sandbox.env_policy import build_sandbox_env + + monkeypatch.setenv("OPENAI_API_KEY", "host-key-do-not-harvest") + env = build_sandbox_env(None) + assert "OPENAI_API_KEY" not in env + + def test_activation_fires_after_input_sanitization_wrapping(self, tmp_path, monkeypatch): + """Integration: in the real chain InputSanitizationMiddleware wraps the user + message in ``--- BEGIN USER INPUT ---`` markers before SkillActivationMiddleware + sees it. Slash activation (and therefore secret resolution) must still fire — it + relies on the original content being recoverable. Regression for the gateway + path where no upload preserved it.""" + from deerflow.agents.middlewares import skill_activation_middleware as mw + from deerflow.agents.middlewares.input_sanitization_middleware import InputSanitizationMiddleware + from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware + from deerflow.config.app_config import AppConfig, reset_app_config, set_app_config + from deerflow.runtime.secret_context import read_active_secrets + + skill = _make_secret_skill(tmp_path, "erp-report", [SecretRequirement("ERP_TOKEN")]) + storage = SimpleNamespace( + load_skills=lambda *, enabled_only: [skill], + get_container_root=lambda: "/mnt/skills", + get_skills_root_path=lambda: tmp_path, + ) + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: storage) + + context = {"secrets": {"ERP_TOKEN": "tok-xyz"}} + request = ModelRequest( + model=object(), + messages=[HumanMessage(content="/erp-report pull it", id="m1")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ) + # The sanitizer loads enabled skills during wrap, so keep a stub app config + # in place for the whole composed call. + set_app_config(AppConfig.model_validate({"sandbox": {"use": "deerflow.sandbox.local:LocalSandboxProvider"}})) + try: + sanitizer = InputSanitizationMiddleware() + skill_mw = SkillActivationMiddleware() + + # Compose in real order: sanitizer (outer) -> skill activation (inner) -> model. + def skill_layer(req): + return skill_mw.wrap_model_call(req, lambda r: AIMessage(content="ok")) + + sanitizer.wrap_model_call(request, skill_layer) + finally: + reset_app_config() + + assert read_active_secrets(context) == {"ERP_TOKEN": "tok-xyz"} + + def test_prior_activation_secrets_cleared_when_next_skill_declares_none(self, tmp_path, monkeypatch): + """A later skill in the same run never inherits an earlier skill's secrets. + Turn 1 activates /skill-a (declares A_TOKEN, caller supplies it) → injected. + Turn 2 activates /skill-b (declares nothing) → A_TOKEN must be cleared so + bash in skill-b's turn cannot receive a value it never declared.""" + from deerflow.agents.middlewares import skill_activation_middleware as mw + from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware + from deerflow.runtime.secret_context import read_active_secrets + + skill_a = _make_secret_skill(tmp_path, "skill-a", [SecretRequirement("A_TOKEN")]) + skill_b = _make_secret_skill(tmp_path, "skill-b", []) + + def _storage(skills): + return SimpleNamespace( + load_skills=lambda *, enabled_only: skills, + get_container_root=lambda: "/mnt/skills", + get_skills_root_path=lambda: tmp_path, + ) + + context = {"secrets": {"A_TOKEN": "v-a"}} + + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: _storage([skill_a])) + SkillActivationMiddleware().wrap_model_call( + ModelRequest( + model=object(), + messages=[HumanMessage(content="/skill-a go", id="m1")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ), + lambda r: AIMessage(content="ok"), + ) + assert read_active_secrets(context) == {"A_TOKEN": "v-a"} + + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: _storage([skill_b])) + SkillActivationMiddleware().wrap_model_call( + ModelRequest( + model=object(), + messages=[HumanMessage(content="/skill-b go", id="m2")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ), + lambda r: AIMessage(content="ok"), + ) + assert read_active_secrets(context) == {} + + def test_prior_activation_secrets_cleared_when_caller_omits_required(self, tmp_path, monkeypatch): + """Even when the next skill DOES declare a required secret, if the caller + omits it the prior skill's value must not linger — the injection set ends + up empty, not stale.""" + from deerflow.agents.middlewares import skill_activation_middleware as mw + from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware + from deerflow.runtime.secret_context import read_active_secrets + + skill = _make_secret_skill(tmp_path, "erp", [SecretRequirement("ERP_TOKEN")]) + storage = SimpleNamespace( + load_skills=lambda *, enabled_only: [skill], + get_container_root=lambda: "/mnt/skills", + get_skills_root_path=lambda: tmp_path, + ) + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: storage) + + # Turn 1: caller supplies ERP_TOKEN → injected. + context = {"secrets": {"ERP_TOKEN": "tok-1"}} + mw_inst = SkillActivationMiddleware() + mw_inst.wrap_model_call( + ModelRequest( + model=object(), + messages=[HumanMessage(content="/erp go", id="m1")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ), + lambda r: AIMessage(content="ok"), + ) + assert read_active_secrets(context) == {"ERP_TOKEN": "tok-1"} + + # Turn 2: caller omits ERP_TOKEN → prior value cleared, set empty (not stale). + context2 = {"secrets": {}} + mw_inst.wrap_model_call( + ModelRequest( + model=object(), + messages=[HumanMessage(content="/erp again", id="m2")], + state={"messages": []}, + runtime=SimpleNamespace(context=context2), + ), + lambda r: AIMessage(content="ok"), + ) + assert read_active_secrets(context2) == {} + + +class TestBashToolInjectsActiveSecrets: + """The bash tool forwards the per-run injection set to execute_command(env=...).""" + + def _run_bash(self, context): + from deerflow.sandbox import tools as tools_mod + + captured = {} + + class FakeSandbox: + def execute_command(self, command, env=None, timeout=None): + captured["env"] = env + captured["timeout"] = timeout + return "done" + + runtime = SimpleNamespace(context=context, state={"sandbox": {"sandbox_id": "aio:1"}}) + with ( + patch.object(tools_mod, "ensure_sandbox_initialized", return_value=FakeSandbox()), + patch.object(tools_mod, "is_local_sandbox", return_value=False), + patch.object(tools_mod, "ensure_thread_directories_exist", return_value=None), + ): + out = tools_mod.bash_tool.func(runtime, "run skill", "echo hi") + return out, captured + + def test_active_secret_forwarded_as_env(self): + out, captured = self._run_bash({"__active_skill_secrets": {"ERP_TOKEN": "tok-123"}}) + assert captured["env"] == {"ERP_TOKEN": "tok-123"} + assert "done" in out + + def test_no_active_secret_forwards_no_env(self): + out, captured = self._run_bash({}) + assert captured["env"] in (None, {}) + + def test_local_bash_forwards_env_and_timeout(self, monkeypatch): + from deerflow.sandbox import tools as tools_mod + + captured = {} + + class FakeSandbox: + def execute_command(self, command, env=None, timeout=None): + captured["command"] = command + captured["env"] = env + captured["timeout"] = timeout + return "done" + + runtime = SimpleNamespace( + context={"__active_skill_secrets": {"ERP_TOKEN": "tok-456"}}, + state={"sandbox": {"sandbox_id": "local:1"}}, + ) + thread_data = {"workspace_path": "/tmp/ws", "cwd": "/mnt/user-data/workspace"} + fake_cfg = SimpleNamespace(sandbox=SimpleNamespace(bash_output_max_chars=321, bash_command_timeout=42)) + with ( + patch.object(tools_mod, "ensure_sandbox_initialized", return_value=FakeSandbox()), + patch.object(tools_mod, "is_local_sandbox", return_value=True), + patch.object(tools_mod, "is_host_bash_allowed", return_value=True), + patch.object(tools_mod, "ensure_thread_directories_exist", return_value=None), + patch.object(tools_mod, "get_thread_data", return_value=thread_data), + patch.object(tools_mod, "validate_local_bash_command_paths", return_value=None), + patch.object(tools_mod, "replace_virtual_paths_in_command", side_effect=lambda command, td: command), + patch.object(tools_mod, "_apply_cwd_prefix", side_effect=lambda command, td: command), + patch("deerflow.config.app_config.get_app_config", return_value=fake_cfg), + ): + out = tools_mod.bash_tool.func(runtime, "run local skill", "echo hi") + + assert out == "done" + assert captured["command"] == "echo hi" + assert captured["env"] == {"ERP_TOKEN": "tok-456"} + assert captured["timeout"] == 42 + + +_SECRET = "sk-erp-9f3c-DO-NOT-LEAK" + + +class TestLeakSurfaces: + """Assert the secret value is absent from all five leak surfaces (#3861).""" + + def _activate_with_secret(self, tmp_path, monkeypatch): + from deerflow.agents.middlewares import skill_activation_middleware as mw + from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware + + skill = _make_secret_skill(tmp_path, "erp-report", [SecretRequirement("ERP_TOKEN")]) + storage = SimpleNamespace( + load_skills=lambda *, enabled_only: [skill], + get_container_root=lambda: "/mnt/skills", + get_skills_root_path=lambda: tmp_path, + ) + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: storage) + + journal_records: list[dict] = [] + journal = SimpleNamespace(record_middleware=lambda *a, **k: journal_records.append({"a": a, "k": k})) + context = {"secrets": {"ERP_TOKEN": _SECRET}, "__run_journal": journal} + request = ModelRequest( + model=object(), + messages=[HumanMessage(content="/erp-report pull report", id="m1")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ) + captured = {} + SkillActivationMiddleware().wrap_model_call(request, lambda r: captured.setdefault("messages", r.messages) or AIMessage(content="ok")) + return context, captured["messages"], journal_records + + def test_prompt_surface_has_no_secret(self, tmp_path, monkeypatch): + # The injected activation message (the only thing added to the prompt / + # checkpointed messages) must not contain the secret value. + _, messages, _ = self._activate_with_secret(tmp_path, monkeypatch) + for m in messages: + assert _SECRET not in str(m.content) + + def test_checkpoint_surface_separation(self, tmp_path, monkeypatch): + # Secrets live on runtime.context, never in the graph state that gets + # checkpointed (messages/state). + context, messages, _ = self._activate_with_secret(tmp_path, monkeypatch) + assert context["secrets"]["ERP_TOKEN"] == _SECRET # present in context... + assert _SECRET not in str([m.content for m in messages]) # ...not in state + + def test_audit_surface_has_no_secret(self, tmp_path, monkeypatch): + _, _, journal_records = self._activate_with_secret(tmp_path, monkeypatch) + assert journal_records, "activation should record an audit event" + assert _SECRET not in str(journal_records) + + def test_trace_metadata_has_no_secret(self, monkeypatch): + from deerflow.tracing import metadata as meta + + monkeypatch.setattr(meta, "get_enabled_tracing_providers", lambda: {"langfuse"}) + config = {"context": {"secrets": {"ERP_TOKEN": _SECRET}}, "metadata": {}} + meta.inject_langfuse_metadata(config, thread_id="t", user_id="u", model_name="m") + assert _SECRET not in str(config["metadata"]) + # And secrets were never mirrored into configurable. + assert _SECRET not in str(config.get("configurable", {})) + + def test_redact_helper_strips_secret_keys(self): + from deerflow.runtime.secret_context import redact_secret_context_keys + + ctx = {"thread_id": "t", "secrets": {"ERP_TOKEN": _SECRET}, "__active_skill_secrets": {"ERP_TOKEN": _SECRET}} + redacted = redact_secret_context_keys(ctx) + assert redacted == {"thread_id": "t"} + assert _SECRET not in str(redacted) + + def test_redact_config_secrets_strips_from_persisted_config(self): + # The run-record persistence + run API echo the raw request config; the + # stored/echoed copy must not carry secrets (verifier blocker), while the + # live config used to drive the run keeps them. + from deerflow.runtime.secret_context import redact_config_secrets + + config = {"context": {"secrets": {"ERP_TOKEN": _SECRET}, "thread_id": "t", "model_name": "m"}, "recursion_limit": 100} + redacted = redact_config_secrets(config) + assert _SECRET not in str(redacted) + assert redacted["context"]["thread_id"] == "t" + assert redacted["context"]["model_name"] == "m" + assert "secrets" not in redacted["context"] + # Original is untouched (live config still has secrets). + assert config["context"]["secrets"] == {"ERP_TOKEN": _SECRET} + + def test_redact_config_secrets_handles_none_and_no_context(self): + from deerflow.runtime.secret_context import redact_config_secrets + + assert redact_config_secrets(None) is None + assert redact_config_secrets({"configurable": {"thread_id": "t"}}) == {"configurable": {"thread_id": "t"}} + + def test_stdout_surface_redacted(self): + from deerflow.sandbox.tools import mask_secret_values + + leaked = f"DEBUG: token is {_SECRET} done" + masked = mask_secret_values(leaked, {"ERP_TOKEN": _SECRET}) + assert _SECRET not in masked + assert "[redacted]" in masked + + def test_short_secret_values_not_masked(self): + """Values below the minimum length floor are skipped — redacting a 2-char + value would shred unrelated bytes (exit codes, timestamps, sizes) of tool + output. The secret is still injected into the subprocess; only the output + mask skips it.""" + from deerflow.sandbox.tools import mask_secret_values + + # A short value must not be replaced everywhere in the output. + out = "exit code: 42\nrows: 42\n" + masked = mask_secret_values(out, {"REGION": "42"}) + assert masked == out # unchanged — short value left intact + + # A long value is still redacted as before. + long_secret = "sk-erp-long-enough-token-value" + masked_long = mask_secret_values(f"token={long_secret}", {"ERP_TOKEN": long_secret}) + assert long_secret not in masked_long + assert "[redacted]" in masked_long + + +@pytest.mark.skipif(__import__("os").name == "nt", reason="POSIX shell semantics") +class TestEndToEndRealSubprocess: + """End-to-end across the real chain (no sandbox mock): activation resolves the + secret, a REAL LocalSandbox subprocess receives it via env, the value lands in + a file but is redacted from the returned output, and a later un-injected call + cannot see it.""" + + def test_secret_reaches_real_subprocess_only_via_env_and_is_scoped(self, tmp_path, monkeypatch): + from deerflow.agents.middlewares import skill_activation_middleware as mw + from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware + from deerflow.runtime.secret_context import read_active_secrets + from deerflow.sandbox.tools import mask_secret_values + + # 1. Activate a skill that declares ERP_TOKEN; caller supplies it in context.secrets. + skill = _make_secret_skill(tmp_path, "erp-report", [SecretRequirement("ERP_TOKEN")]) + storage = SimpleNamespace( + load_skills=lambda *, enabled_only: [skill], + get_container_root=lambda: "/mnt/skills", + get_skills_root_path=lambda: tmp_path, + ) + monkeypatch.setattr(mw, "get_or_new_skill_storage", lambda **kwargs: storage) + # A platform secret is present on the host and must NOT leak to the subprocess. + monkeypatch.setenv("OPENAI_API_KEY", "sk-host-platform-secret") + context = {"secrets": {"ERP_TOKEN": _SECRET}} + request = ModelRequest( + model=object(), + messages=[HumanMessage(content="/erp-report pull report", id="m1")], + state={"messages": []}, + runtime=SimpleNamespace(context=context), + ) + SkillActivationMiddleware().wrap_model_call(request, lambda r: AIMessage(content="ok")) + injected = read_active_secrets(context) + assert injected == {"ERP_TOKEN": _SECRET} + + # 2. A REAL LocalSandbox runs a script that writes the token to a file and echoes it. + out_file = tmp_path / "token.txt" + sandbox = LocalSandbox(id="local") + raw = sandbox.execute_command( + f'printf "%s" "$ERP_TOKEN" > {out_file}; echo "leaked:$ERP_TOKEN"; echo "platform:$OPENAI_API_KEY"', + env=injected, + ) + + # 3. The skill genuinely received the token via env (file written by the subprocess). + assert out_file.read_text() == _SECRET + # 4. Platform secret was scrubbed — not available to the script. + assert "sk-host-platform-secret" not in raw + # 5. Stdout masking redacts the echoed token before it would re-enter context. + masked = mask_secret_values(raw, injected) + assert _SECRET not in masked + + # 6. Per-call scope: a later command without injection cannot see the token. + leaked = sandbox.execute_command("echo [$ERP_TOKEN]") + assert _SECRET not in leaked diff --git a/backend/tests/test_tool_output_budget_middleware.py b/backend/tests/test_tool_output_budget_middleware.py index cdca47b74..3fc896a7c 100644 --- a/backend/tests/test_tool_output_budget_middleware.py +++ b/backend/tests/test_tool_output_budget_middleware.py @@ -918,7 +918,13 @@ class _FakeSandbox: self._write_ok = write_ok self._check_result = check_result - def execute_command(self, command: str) -> str: + def execute_command( + self, + command: str, + env: dict[str, str] | None = None, + timeout: float | None = None, + ) -> str: + del env, timeout self.commands.append(command) if command.startswith("test -s"): return self._check_result