From c714629fafb003d9af8bf59186d0a04278b7e97d Mon Sep 17 00:00:00 2001 From: NanHan Qing <157583847+nanhanq1@users.noreply.github.com> Date: Wed, 30 Sep 2026 14:56:03 +0800 Subject: [PATCH] fix(sandbox): expose DEERFLOW_USER_ID to every bash command (#6058) * fix(sandbox): expose DEERFLOW_USER_ID to bash commands A skill's SKILL.md can trigger scripts, but those scripts run as bash subprocesses inside the sandbox and cannot reach the Gateway auth context, so get_effective_user_id() is unavailable to them and user-scoped skills are impossible to write. The maintainer settled the direction on #3919: inject the id as DEERFLOW_USER_ID. Every bash command now begins with 'export DEERFLOW_USER_ID=; ' (or 'unset' when the resolved id is unusable), mirroring the existing DEERFLOW_CHANNEL_USER_ID channel-identity prefix (#3914). The id rides the command string rather than execute_command(env=...), because a non-empty env switches AioSandbox to the bash.exec API reserved for request-scoped secrets. The earlier attempt (#4029) was closed for making every command carry the new export without updating the exact-command assertions in tests/test_channel_user_id_env.py. Those assertions are updated here, and the composition of the two prefixes is pinned by a dedicated test. * fix(sandbox): fix the DEERFLOW_USER_ID unit test and cover review findings Follow-up to the review on this PR. Unit test (CI `backend-unit-tests (2)`): the change moved the command contract but left the exact-command assertion in tests/test_skill_request_scoped_secrets.py::test_local_bash_forwards_env_and_timeout expecting the bare `echo hi`. The local validation run used a `-k` filter that did not select that file, so the breakage was invisible there. The assertion now pins the composed command, and `_is_windows` is patched explicitly so it no longer depends on the host OS. [P2] Windows local sandbox: `export` is invalid under the PowerShell/cmd.exe fallback, but skipping the prefix outright left the promise unmet on Windows. The id is published through `execute_command(env=...)` there instead: LocalSandbox layers `env` into the per-process environment and, unlike AioSandbox, has no persistent session that could carry a stale value. It is deliberately kept out of `injected_env` so the identifier does not enter the secret-redaction set. Docstring: `_user_identity_prefix` now carries the same "informational, not authenticated identity" caveat the channel variable has in backend/app/channels/AGENTS.md. The value is authorization-grade at the Gateway boundary, but any bash command can overwrite its own environment, so skills must not treat the shell variable as proof of identity. Docs: the en/zh sandbox.mdx identity sections and the channels/AGENTS.md identity paragraph now describe the second, outermost prefix, its ordering against the channel prefix, and the Windows behaviour. --- backend/app/channels/AGENTS.md | 2 +- .../harness/deerflow/sandbox/tools.py | 86 +++++++- backend/tests/test_channel_user_id_env.py | 193 ++++++++++++++++-- backend/tests/test_github_token_plumbing.py | 3 +- .../test_skill_request_scoped_secrets.py | 50 ++++- .../content/en/harness/subagents/sandbox.mdx | 6 + .../content/zh/harness/subagents/sandbox.mdx | 6 + 7 files changed, 318 insertions(+), 28 deletions(-) diff --git a/backend/app/channels/AGENTS.md b/backend/app/channels/AGENTS.md index 50ef8d4a09..99fba291c6 100644 --- a/backend/app/channels/AGENTS.md +++ b/backend/app/channels/AGENTS.md @@ -26,7 +26,7 @@ Bridges external messaging platforms (Feishu, Slack, Telegram, Discord, DingTalk - Feishu/Lark inbound image/file downloads read at most 20,000,001 bytes and reject anything above 20,000,000 bytes before persistence or sandbox sync. Oversize, unsafe-path, and path-resolution failures rewrite only that attachment placeholder to `Failed to obtain the [type]` so later attachments in the same message can still load. - WeChat iLink inbound image/file downloads stream with an in-flight cap derived from `channels.wechat.max_inbound_image_bytes` / `max_inbound_file_bytes` (20 MB / 50 MB defaults) that aborts before the full read, and the payload-supplied `media.full_url` must pass a scheme + dot-boundary host-suffix allowlist (`channels.wechat.allowed_media_hosts`, defaulting to the `qq.com` family plus the configured `cdn_base_url` host) before anything is fetched. The limits are PLAINTEXT limits while the stream measures CIPHERTEXT, so the in-flight cap is the PKCS#7-padded size of exactly-limit plaintext (`WechatChannel._stream_cap_for`); the exact post-decryption check stays as the authority. There is deliberately **no URL re-fetch fallback**: `_read_wechat_inbound_file` (manager) only reads the locally staged `path` — the channel always stages one before publishing — so nothing fetches a WeChat URL outside the channel-side gate. WeCom media URLs (frame-supplied `url`, no per-channel size config) get the same style of host gate — `qq.com` suffixes plus the COS shape `ww-aibot-img-.cos..myqcloud.com` WeCom serves signed links from, where the numeric suffix must be one of the verified WeCom-owned Tencent Cloud APPIDs (`_WECOM_MEDIA_COS_APPIDS`, currently only the `1258476243` observed in Tencent's published callback examples) because bucket names are user-chosen and any Tencent Cloud account can register a lookalike `ww-aibot-img-*` bucket; other accounts go through operator suffixes from `channels.wecom.allowed_media_hosts` resolved from the live channel at read time — and the manager-level `MAX_INBOUND_URL_FILE_BYTES` (50 MB) streaming cap. Both URL downloads and `_download_cdn_bytes` request `Accept-Encoding: identity`, refuse a response with any residual `Content-Encoding` before reading, and iterate `aiter_raw()`: httpx's transparent decoding would allocate the full decompressed body before the byte cap sees a chunk. No URL-based inbound-media log line may contain any part of the URL (signed links carry credentials in path and query): failure labels use the attachment filename or host only (`manager._inbound_file_label`), and reader failures log a sanitized exception summary — class name plus HTTP status (`manager._reader_error_summary`, `wechat._media_download_error_summary`), never the URL-bearing message or traceback. The rule extends to the HTTP client libraries' own records: httpx emits `HTTP Request: GET ` at INFO via the bare `httpx` logger, and urllib3 — enumerated against the installed source; every remaining emitter logs host:port only (connection establishment/reset) or one absolute URL (the header-parse warning) that the generic pattern rewrites — renders a request target in five shapes, each needing a dedicated redaction pattern because the generic absolute-URL pass cannot see a target with no scheme: `Redirecting -> ` at INFO via `urllib3.poolmanager` and DEBUG via `urllib3.connectionpool` (either slot may be an origin-form relative Location, which pool/connection callers pass or the Location header carries; the shape keeps the `^Redirecting ` prefix anchor because `-> /path` arrows also appear in non-URL logs — sandbox mount mappings — and must pass through untouched, while both slots consume through end-of-line: the raw Location header may carry interior spaces, and the recursive urlopen frame can pass it on as the next request target. A slot is kept verbatim only when the generic pass consumes it whole (`logging_config._url_pass_consumes_slot` asks that pass itself, so the kept set cannot drift from its `host`/`rest` stop conditions) — every other RFC 3986 relative-reference form (slash-less paths, query-only, fragments, network-path), non-hierarchical scheme, space-carrying slot, quote-at-boundary slot (`https://h/a')b?sig=…`, where `rest` stops at the closing mark), and empty-host absolute (`https:///path?sig=…`, which `host` never matches) collapses, so neither `download?sign=…` nor `other page?sig=…` leaks its signed query), the per-request `scheme://host:port "METHOD /path?query HTTP/x.x"` DEBUG line, `Retry: ` DEBUG and `Retrying (…) after connection broken by '': ` **WARNING** — above the INFO root, so reachable at the default log level — via `urllib3.connectionpool`, and `Incremented Retry for (url='')` DEBUG via `urllib3.util.retry` (origin-form on the request path, absolute on the redirect path). The generic pass's `rest` treats a quote as a closing mark only at a boundary — followed by whitespace, a closing parenthesis, or end of string — so urllib3's `Incremented Retry for (url='…')` keeps its `')` scaffolding while a quote embedded in a URL stays consumed and redacted. Logger filters only see records emitted through that exact logger — child loggers neither inherit them nor trigger them on propagation — so `UrlRedactionFilter` is attached to the `httpx` logger AND to every root handler by `configure_logging` (handler-level filters see propagated records), which also keeps Telegram's token-bearing Bot API paths (`api.telegram.org/bot/...`, emitted by python-telegram-bot's httpx client) out of the logs. 2. `ChannelManager._dispatch_loop()` consumes from queue -3. For user-owned channel connections, incoming messages carry `connection_id`, `owner_user_id`, and `workspace_id`; `owner_user_id` becomes the DeerFlow run `user_id`, while the raw platform user id remains `channel_user_id`. The Gateway accepts `channel_user_id` only from an internally authenticated channel caller's top-level `body.context`, clears it from both free-form `body.config` sections, and writes it into runtime context only (never `configurable`, which is checkpointed). `bash_tool` exposes it to sandbox commands as the fixed env var `DEERFLOW_CHANNEL_USER_ID` — via a shell-quoted command-string prefix, NOT the `execute_command(env=...)` channel, which is reserved for request-scoped secrets and would switch `AioSandbox` onto the `bash.exec` path (image >= 1.9.3, fresh session per call). Per-call injection keeps group-chat identity correct (one thread/sandbox, many senders) **without depending on the AIO shell's session semantics**: every IM-channel command carries an explicit `export VAR=; ` (valid id) or `unset VAR; ` (empty / non-str / over the 256-char cap). The AIO no-env path reuses a persistent shell session (the reason for the class lock, #1433), so a bare command could otherwise resolve a stale id an earlier sender exported; the `unset` closes the window the length/type guard would open (a dropped id would inherit the previous sender's value). Non-IM runs (no `channel_user_id` in context) are left untouched. Not injected on the Windows local sandbox (its PowerShell/cmd.exe fallback has no `export`/`unset`). Propagates across `task` delegation: `task_tool` captures the dispatching turn's id and the subagent executor forwards it into the subagent's runtime context, same as the guardrail attribution fields. The runtime-context value is authorization-grade at the Gateway/guardrail boundary, but the exported shell variable remains informational because any bash command can overwrite its own environment; skills must not treat the shell variable itself as authenticated identity. Tests: `tests/test_gateway_services.py`, `tests/test_channel_user_id_env.py` +3. For user-owned channel connections, incoming messages carry `connection_id`, `owner_user_id`, and `workspace_id`; `owner_user_id` becomes the DeerFlow run `user_id`, while the raw platform user id remains `channel_user_id`. The Gateway accepts `channel_user_id` only from an internally authenticated channel caller's top-level `body.context`, clears it from both free-form `body.config` sections, and writes it into runtime context only (never `configurable`, which is checkpointed). `bash_tool` exposes it to sandbox commands as the fixed env var `DEERFLOW_CHANNEL_USER_ID` — via a shell-quoted command-string prefix, NOT the `execute_command(env=...)` channel, which is reserved for request-scoped secrets and would switch `AioSandbox` onto the `bash.exec` path (image >= 1.9.3, fresh session per call). Per-call injection keeps group-chat identity correct (one thread/sandbox, many senders) **without depending on the AIO shell's session semantics**: every IM-channel command carries an explicit `export VAR=; ` (valid id) or `unset VAR; ` (empty / non-str / over the 256-char cap). The AIO no-env path reuses a persistent shell session (the reason for the class lock, #1433), so a bare command could otherwise resolve a stale id an earlier sender exported; the `unset` closes the window the length/type guard would open (a dropped id would inherit the previous sender's value). Non-IM runs (no `channel_user_id` in context) are left untouched. A second, always-present prefix carries the authenticated DeerFlow user id (#3919) as the fixed env var `DEERFLOW_USER_ID`, emitted **outermost** of the two (`export DEERFLOW_USER_ID=; export DEERFLOW_CHANNEL_USER_ID=; cd ...`), so a reader sees which user the command runs as before which IM sender it came from; unlike the channel id it is never skipped for a non-IM run, because `resolve_runtime_user_id` falls back to `default` (same `export`/`unset` discipline and 256-char cap). Neither prefix is emitted on the Windows local sandbox (its PowerShell/cmd.exe fallback has no `export`/`unset`): there the user id is published through `execute_command(env=...)` instead — `LocalSandbox` layers `env` into the per-process environment and, unlike `AioSandbox`, has no persistent session to keep honest — while the channel id is simply absent. Propagates across `task` delegation: `task_tool` captures the dispatching turn's id and the subagent executor forwards it into the subagent's runtime context, same as the guardrail attribution fields. The runtime-context value is authorization-grade at the Gateway/guardrail boundary, but both exported shell variables (`DEERFLOW_CHANNEL_USER_ID`, `DEERFLOW_USER_ID`) remain informational because any bash command can overwrite its own environment, and anything the command sources or launches (a dependency, a sourced rc file on the host-bash path) can silently re-export a different id before a skill reads it — only the outer prefix on the next `bash_tool` call re-asserts the true value; skills must not treat the shell variable itself as authenticated identity. Tests: `tests/test_gateway_services.py`, `tests/test_channel_user_id_env.py` 4. For chat: look up/create thread through Gateway's LangGraph-compatible API - First-use creation is serialized by a waiter-aware keyed lock over `(channel_name, chat_id, topic_id)`. Holders and queued callers share one lock generation through failure and cancellation; idle entries are reclaimed only after the final participant leaves. This prevents a late inbound message from bypassing a queued creator and splitting one conversation across duplicate Gateway threads. 5. Feishu/Telegram chat: `runs.stream()` → accumulate AI text → publish multiple outbound updates (`is_final=False`) → publish final outbound (`is_final=True`) diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 9f831da0ba..0262d0f9c5 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -2053,6 +2053,17 @@ _CHANNEL_USER_ID_CONTEXT_KEY = "channel_user_id" # this is corrupt and must not bloat every sandbox command string. _CHANNEL_USER_ID_MAX_LEN = 256 +# Fixed env var exposing the authenticated DeerFlow user id to sandbox +# commands, so skill scripts and the subprocesses they launch can scope their +# work to the current user instead of guessing or hard-coding (#3919). An +# identifier, not a secret. +USER_ID_ENV = "DEERFLOW_USER_ID" + +# Same defensive bound as the channel identity: a real user id is short +# (``make_safe_user_id`` output), so anything past this is corrupt and must not +# bloat every sandbox command string. +_USER_ID_MAX_LEN = 256 + def _is_windows() -> bool: return os.name == "nt" @@ -2089,6 +2100,58 @@ def _channel_identity_prefix(runtime: Runtime) -> str | None: return f"unset {CHANNEL_USER_ID_ENV}; " +def _resolved_user_id(runtime: Runtime) -> str | None: + """Return the effective user id when it is publishable, else ``None``. + + ``resolve_runtime_user_id`` is the authorization-grade source (see its + docstring): server-owned for external callers, channel-authenticated for + internal ones. The guards are the same defensive bound the channel identity + uses — a real id is short (``make_safe_user_id`` output), so an empty / + non-str / over-cap value is corrupt and must not reach a command. + """ + user_id = resolve_runtime_user_id(runtime) + if isinstance(user_id, str) and 0 < len(user_id) <= _USER_ID_MAX_LEN: + return user_id + return None + + +def _user_identity_prefix(runtime: Runtime) -> str: + """Build the command prefix that publishes the effective user id to bash. + + Unlike :func:`_channel_identity_prefix`, this always returns a prefix. + ``resolve_runtime_user_id`` falls back to ``DEFAULT_USER_ID``, so there is + no "not applicable" run: every command is attributable to a user. Stating + the value on every command is also what keeps a skill script correct + regardless of what an earlier command exported into a reused shell session + — the same per-call discipline the channel identity needs. + + - usable id (non-empty str within the length cap) → ``export VAR=; `` + - unusable id (empty / non-str / over the cap) → ``unset VAR; `` + + The id deliberately rides the command string instead of the + ``execute_command(env=...)`` channel: a non-empty ``env`` switches + ``AioSandbox`` to the ``bash.exec`` API (fresh session per call, image + >= 1.9.3 required), which is reserved for request-scoped secrets. The value + is an identifier, not a secret, so keeping it in the audit-visible command + string is fine. + + **Informational, not authenticated identity.** The exported shell variable + is a convenience for skill scripts — exactly like + ``DEERFLOW_CHANNEL_USER_ID`` — not a credential and not proof of who is + acting. Any bash command can overwrite its own environment, and anything the + command sources or launches (a dependency, a sourced rc file on the + host-bash path) can silently re-export a different id before a skill's + scoping logic reads it; only the outer prefix on the *next* ``bash_tool`` + call re-asserts the true value. Skills that need user-scoped *authorization* + must resolve the identity server-side via ``resolve_runtime_user_id`` rather + than trusting this variable. + """ + user_id = _resolved_user_id(runtime) + if user_id is not None: + return f"export {USER_ID_ENV}={shlex.quote(user_id)}; " + return f"unset {USER_ID_ENV}; " + + def _github_env_from_runtime(runtime: Runtime) -> dict[str, str] | None: """Build a per-call env overlay carrying a GitHub App installation token. @@ -2194,6 +2257,7 @@ def bash_tool(runtime: Runtime, command: str, description: str = "") -> str: # never placed in the command string. injected_env = read_active_secrets(getattr(runtime, "context", None)) or None identity_prefix = _channel_identity_prefix(runtime) + user_prefix = _user_identity_prefix(runtime) github_env = _github_env_from_runtime(runtime) lark_cli_env = _lark_cli_env_from_runtime(runtime, command, sandbox_paths=not is_local_sandbox(runtime)) if github_env: @@ -2209,9 +2273,20 @@ def bash_tool(runtime: Runtime, command: str, description: str = "") -> str: command = replace_virtual_paths_in_command(command, thread_data) command = _apply_cwd_prefix(command, thread_data) # POSIX-only: the Windows local sandbox may execute via - # PowerShell/cmd.exe where `export` is not valid syntax. - if identity_prefix and not _is_windows(): - command = identity_prefix + command + # PowerShell/cmd.exe where `export` is not valid syntax, so the id + # is published through the subprocess environment instead — + # LocalSandbox layers `env` into the per-process environment, and + # unlike AioSandbox it has no persistent shell session that could + # carry a stale value forward. Deliberately kept out of + # `injected_env`, which doubles as the secret-redaction set: an + # identifier is not a secret and must stay readable in output. + local_env = injected_env + if not _is_windows(): + command = user_prefix + (identity_prefix or "") + command + else: + windows_user_id = _resolved_user_id(runtime) + if windows_user_id is not None: + local_env = {**(injected_env or {}), USER_ID_ENV: windows_user_id} try: from deerflow.config.app_config import get_app_config @@ -2225,7 +2300,7 @@ def bash_tool(runtime: Runtime, command: str, description: str = "") -> str: sandbox, command, runtime=runtime, - env=injected_env, + env=local_env, timeout=command_timeout, ) return _truncate_bash_output( @@ -2234,8 +2309,7 @@ def bash_tool(runtime: Runtime, command: str, description: str = "") -> str: ) ensure_thread_directories_exist(runtime) command = f"cd {VIRTUAL_PATH_PREFIX}/workspace; {command}" - if identity_prefix: - command = identity_prefix + command + command = user_prefix + (identity_prefix or "") + command try: from deerflow.config.app_config import get_app_config diff --git a/backend/tests/test_channel_user_id_env.py b/backend/tests/test_channel_user_id_env.py index 82220dfbd1..765556f8b3 100644 --- a/backend/tests/test_channel_user_id_env.py +++ b/backend/tests/test_channel_user_id_env.py @@ -1,21 +1,28 @@ -"""Tests for exposing the IM-channel platform user id to sandbox commands (#3914). +"""Tests for exposing caller identity to sandbox commands (#3914, #3919). Two halves: - Gateway: only an internally authenticated caller's top-level ``body.context`` may supply ``channel_user_id``; free-form RunnableConfig values are cleared. -- Sandbox: ``bash_tool`` exposes the id as the fixed env var - ``DEERFLOW_CHANNEL_USER_ID`` via an ``export`` prefix on the command string. - It must NOT ride the ``env=`` parameter: on ``AioSandbox`` a non-empty env - switches execution to the ``bash.exec`` API, which requires image >= 1.9.3 - and abandons the persistent shell session — that channel is reserved for - request-scoped secrets. +- Sandbox: ``bash_tool`` exposes both identities as fixed env vars via an + ``export`` prefix on the command string — ``DEERFLOW_CHANNEL_USER_ID`` for the + IM platform sender (#3914) and ``DEERFLOW_USER_ID`` for the authenticated + DeerFlow user (#3919). Neither must ride the ``env=`` parameter: on + ``AioSandbox`` a non-empty env switches execution to the ``bash.exec`` API, + which requires image >= 1.9.3 and abandons the persistent shell session — + that channel is reserved for request-scoped secrets. + +The two differ in applicability: the channel id is absent for non-IM runs so +its prefix is skipped, whereas the user id always resolves (falling back to +``default``) so its prefix is always emitted, and it comes first. """ from types import SimpleNamespace from deerflow.sandbox.tools import ( CHANNEL_USER_ID_ENV, + USER_ID_ENV, _channel_identity_prefix, + _user_identity_prefix, bash_tool, ) @@ -25,11 +32,16 @@ _THREAD_DATA = { "outputs_path": "/tmp/deer-flow/threads/t1/user-data/outputs", } +# Pinned so expected command strings do not depend on whichever identity the +# ambient test fixture happens to install. +_USER_ID = "u-test" +_USER_PREFIX = f"export {USER_ID_ENV}={_USER_ID}; " + def _aio_runtime(context: dict) -> SimpleNamespace: return SimpleNamespace( state={"sandbox": {"sandbox_id": "aio-sandbox-1"}, "thread_data": _THREAD_DATA.copy()}, - context=context, + context={"user_id": _USER_ID, **context}, ) @@ -108,13 +120,15 @@ class TestBashToolChannelIdentityPrefix: sandbox = _run_bash(monkeypatch, _aio_runtime({"channel_user_id": "ou_feishu_123"})) assert len(sandbox.calls) == 1 - assert sandbox.calls[0]["command"] == f"export {CHANNEL_USER_ID_ENV}=ou_feishu_123; cd /mnt/user-data/workspace; echo hi" + assert sandbox.calls[0]["command"] == f"{_USER_PREFIX}export {CHANNEL_USER_ID_ENV}=ou_feishu_123; cd /mnt/user-data/workspace; echo hi" assert sandbox.calls[0]["env"] is None def test_no_channel_user_id_omits_identity_prefix(self, monkeypatch): sandbox = _run_bash(monkeypatch, _aio_runtime({"thread_id": "t1"})) - assert sandbox.calls[0]["command"] == "cd /mnt/user-data/workspace; echo hi" + # The user id is always present, so only the channel half is skipped. + assert sandbox.calls[0]["command"] == f"{_USER_PREFIX}cd /mnt/user-data/workspace; echo hi" + assert CHANNEL_USER_ID_ENV not in sandbox.calls[0]["command"] assert sandbox.calls[0]["env"] is None def test_per_call_identity_follows_current_context(self, monkeypatch): @@ -134,7 +148,7 @@ class TestBashToolChannelIdentityPrefix: assert command.endswith("; cd /mnt/user-data/workspace; echo hi") # shlex.quote wraps the value; the raw injection payload must not appear # as executable syntax outside the quoted region. - assert "export " + CHANNEL_USER_ID_ENV + "='x'\"'\"'; rm -rf /tmp/y; '\"'\"''; cd /mnt/user-data/workspace; echo hi" == command + assert _USER_PREFIX + "export " + CHANNEL_USER_ID_ENV + "='x'\"'\"'; rm -rf /tmp/y; '\"'\"''; cd /mnt/user-data/workspace; echo hi" == command def test_secrets_and_identity_compose(self, monkeypatch): """Active skill secrets keep the env= channel; the identity keeps the @@ -149,7 +163,7 @@ class TestBashToolChannelIdentityPrefix: call = sandbox.calls[0] assert call["env"] == {"ERP_TOKEN": "secret-value"} - assert call["command"] == f"export {CHANNEL_USER_ID_ENV}=ou_1; cd /mnt/user-data/workspace; echo hi" + assert call["command"] == f"{_USER_PREFIX}export {CHANNEL_USER_ID_ENV}=ou_1; cd /mnt/user-data/workspace; echo hi" assert "secret-value" not in call["command"] def test_non_im_run_leaves_command_untouched(self): @@ -176,8 +190,8 @@ class TestBashToolChannelIdentityPrefix: a = _run_bash(monkeypatch, _aio_runtime({"channel_user_id": "sender-a"})) b = _run_bash(monkeypatch, _aio_runtime({"channel_user_id": "b" * 5000})) - assert a.calls[0]["command"] == f"export {CHANNEL_USER_ID_ENV}=sender-a; cd /mnt/user-data/workspace; echo hi" - assert b.calls[0]["command"] == f"unset {CHANNEL_USER_ID_ENV}; cd /mnt/user-data/workspace; echo hi" + assert a.calls[0]["command"] == f"{_USER_PREFIX}export {CHANNEL_USER_ID_ENV}=sender-a; cd /mnt/user-data/workspace; echo hi" + assert b.calls[0]["command"] == f"{_USER_PREFIX}unset {CHANNEL_USER_ID_ENV}; cd /mnt/user-data/workspace; echo hi" assert b.calls[0]["env"] is None def test_windows_local_sandbox_skips_prefix(self, monkeypatch): @@ -186,7 +200,7 @@ class TestBashToolChannelIdentityPrefix: every IM-channel command.""" runtime = SimpleNamespace( state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()}, - context={"channel_user_id": "ou_1", "thread_id": "t1"}, + context={"channel_user_id": "ou_1", "thread_id": "t1", "user_id": _USER_ID}, ) sandbox = _CapturingSandbox() monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) @@ -202,7 +216,7 @@ class TestBashToolChannelIdentityPrefix: def test_posix_local_sandbox_gets_prefix(self, monkeypatch): runtime = SimpleNamespace( state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()}, - context={"channel_user_id": "ou_1", "thread_id": "t1"}, + context={"channel_user_id": "ou_1", "thread_id": "t1", "user_id": _USER_ID}, ) sandbox = _CapturingSandbox() monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) @@ -214,5 +228,150 @@ class TestBashToolChannelIdentityPrefix: assert len(sandbox.calls) == 1 command = sandbox.calls[0]["command"] - assert command.startswith(f"export {CHANNEL_USER_ID_ENV}=ou_1; ") + assert command.startswith(f"{_USER_PREFIX}export {CHANNEL_USER_ID_ENV}=ou_1; ") assert command.endswith("echo hi") + + +class TestBashToolUserIdentityPrefix: + """#3919 — the authenticated user id is published to every bash command.""" + + def test_user_id_exported_and_env_stays_none(self, monkeypatch): + """The id rides the command string; env must stay None so AioSandbox + keeps the persistent-shell path (same reason as the channel id).""" + sandbox = _run_bash(monkeypatch, _aio_runtime({"thread_id": "t1"})) + + assert len(sandbox.calls) == 1 + assert sandbox.calls[0]["command"] == f"{_USER_PREFIX}cd /mnt/user-data/workspace; echo hi" + assert sandbox.calls[0]["env"] is None + + def test_prefix_emitted_even_without_an_explicit_user(self, monkeypatch): + """A run that carries no ``user_id`` still resolves one (``default``), so + the variable is always defined rather than silently absent.""" + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "aio-sandbox-1"}, "thread_data": _THREAD_DATA.copy()}, + context={"thread_id": "t1"}, + ) + sandbox = _run_bash(monkeypatch, runtime) + + assert sandbox.calls[0]["command"].startswith(f"export {USER_ID_ENV}=") + + def test_user_prefix_precedes_channel_prefix(self, monkeypatch): + """Both are POSIX exports; the user id is the outer one, so a reader sees + which user the command runs as before which IM sender it came from.""" + sandbox = _run_bash(monkeypatch, _aio_runtime({"channel_user_id": "ou_1"})) + + command = sandbox.calls[0]["command"] + assert command.startswith(_USER_PREFIX) + assert command.index(USER_ID_ENV) < command.index(CHANNEL_USER_ID_ENV) + + def test_value_is_shell_quoted(self, monkeypatch): + """A hostile user id must not be able to inject shell syntax.""" + sandbox = _run_bash(monkeypatch, _aio_runtime({"user_id": "x'; rm -rf /tmp/y; '"})) + + command = sandbox.calls[0]["command"] + assert command.endswith("; cd /mnt/user-data/workspace; echo hi") + assert "export " + USER_ID_ENV + "='x'\"'\"'; rm -rf /tmp/y; '\"'\"''; cd /mnt/user-data/workspace; echo hi" == command + + def test_unusable_value_emits_unset_not_none(self, monkeypatch): + """A corrupt id must clear the variable rather than skip the prefix, so a + command cannot inherit a stale value from a reused shell session.""" + for bad in ("", 123, "x" * 5000, None): + monkeypatch.setattr( + "deerflow.sandbox.tools.resolve_runtime_user_id", + lambda runtime, _bad=bad: _bad, + ) + prefix = _user_identity_prefix(SimpleNamespace(context={})) + assert prefix == f"unset {USER_ID_ENV}; ", f"value={bad!r}" + + def test_composes_with_channel_prefix_and_secrets(self, monkeypatch): + """Secrets keep the env= channel; both identities keep the command-string + channel. They must not mix.""" + runtime = _aio_runtime( + { + "channel_user_id": "ou_1", + "__active_skill_secrets": {"ERP_TOKEN": "secret-value"}, + } + ) + sandbox = _run_bash(monkeypatch, runtime) + + call = sandbox.calls[0] + assert call["env"] == {"ERP_TOKEN": "secret-value"} + assert call["command"] == f"{_USER_PREFIX}export {CHANNEL_USER_ID_ENV}=ou_1; cd /mnt/user-data/workspace; echo hi" + assert "secret-value" not in call["command"] + + def test_posix_local_sandbox_gets_user_prefix(self, monkeypatch): + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()}, + context={"thread_id": "t1", "user_id": _USER_ID}, + ) + sandbox = _CapturingSandbox() + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_thread_directories_exist", lambda runtime: None) + monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True) + monkeypatch.setattr("deerflow.sandbox.tools._is_windows", lambda: False) + + bash_tool.func(runtime=runtime, description="test", command="echo hi") + + assert len(sandbox.calls) == 1 + assert sandbox.calls[0]["command"].startswith(_USER_PREFIX) + + def test_windows_local_sandbox_publishes_user_id_via_env(self, monkeypatch): + """POSIX ``export`` is invalid under the Windows local sandbox, so the id + moves to the ``env`` channel there rather than being dropped: the + contract is that every bash command can see the user id.""" + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()}, + context={"thread_id": "t1", "user_id": _USER_ID}, + ) + sandbox = _CapturingSandbox() + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_thread_directories_exist", lambda runtime: None) + monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True) + monkeypatch.setattr("deerflow.sandbox.tools._is_windows", lambda: True) + + bash_tool.func(runtime=runtime, description="test", command="echo hi") + + assert len(sandbox.calls) == 1 + call = sandbox.calls[0] + assert USER_ID_ENV not in call["command"] + assert "export" not in call["command"] + assert call["env"] == {USER_ID_ENV: _USER_ID} + + def test_windows_env_channel_keeps_secrets_and_user_id_separate(self, monkeypatch): + """Secrets stay in ``env`` for redaction; the id rides alongside them + without entering the redaction set, so echoing it stays readable.""" + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()}, + context={ + "thread_id": "t1", + "user_id": _USER_ID, + "__active_skill_secrets": {"ERP_TOKEN": "secret-value"}, + }, + ) + sandbox = _CapturingSandbox() + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_thread_directories_exist", lambda runtime: None) + monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True) + monkeypatch.setattr("deerflow.sandbox.tools._is_windows", lambda: True) + + bash_tool.func(runtime=runtime, description="test", command="echo hi") + + assert sandbox.calls[0]["env"] == {"ERP_TOKEN": "secret-value", USER_ID_ENV: _USER_ID} + + def test_windows_local_sandbox_omits_unusable_user_id(self, monkeypatch): + """A corrupt id is dropped rather than published — there is no portable + way to clear a variable across PowerShell/cmd/MSYS, so the env channel + carries nothing instead of carrying garbage.""" + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "local"}, "thread_data": _THREAD_DATA.copy()}, + context={"thread_id": "t1", "user_id": "x" * 5000}, + ) + sandbox = _CapturingSandbox() + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_thread_directories_exist", lambda runtime: None) + monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True) + monkeypatch.setattr("deerflow.sandbox.tools._is_windows", lambda: True) + + bash_tool.func(runtime=runtime, description="test", command="echo hi") + + assert sandbox.calls[0]["env"] is None diff --git a/backend/tests/test_github_token_plumbing.py b/backend/tests/test_github_token_plumbing.py index c9b2021e00..70839f8a00 100644 --- a/backend/tests/test_github_token_plumbing.py +++ b/backend/tests/test_github_token_plumbing.py @@ -425,6 +425,7 @@ def test_bash_tool_routes_subagent_command_to_its_shell_scope( state={"sandbox": {"sandbox_id": "aio:xyz"}}, context={ "thread_id": "t1", + "user_id": "u-test", "sandbox_command_scope_id": "subagent:task-1", }, config={}, @@ -464,7 +465,7 @@ def test_bash_tool_routes_subagent_command_to_its_shell_scope( assert result == "done" assert captured["scope_id"] == "subagent:task-1" - assert captured["command"] == "cd /mnt/user-data/workspace; ls" + assert captured["command"] == "export DEERFLOW_USER_ID=u-test; cd /mnt/user-data/workspace; ls" # --------------------------------------------------------------------------- diff --git a/backend/tests/test_skill_request_scoped_secrets.py b/backend/tests/test_skill_request_scoped_secrets.py index d56e653c22..060dde5937 100644 --- a/backend/tests/test_skill_request_scoped_secrets.py +++ b/backend/tests/test_skill_request_scoped_secrets.py @@ -1312,6 +1312,7 @@ class TestBashToolInjectsActiveSecrets: def test_local_bash_forwards_env_and_timeout(self, monkeypatch): from deerflow.sandbox import tools as tools_mod + from deerflow.sandbox.tools import USER_ID_ENV captured = {} @@ -1323,7 +1324,7 @@ class TestBashToolInjectsActiveSecrets: return "done" runtime = SimpleNamespace( - context={"__active_skill_secrets": {"ERP_TOKEN": "tok-456"}}, + context={"__active_skill_secrets": {"ERP_TOKEN": "tok-456"}, "user_id": "u-local"}, state={"sandbox": {"sandbox_id": "local:1"}}, ) thread_data = {"workspace_path": "/tmp/ws", "cwd": "/mnt/user-data/workspace"} @@ -1337,14 +1338,57 @@ class TestBashToolInjectsActiveSecrets: 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), + # Pinned so the assertion below does not depend on the host OS: + # the Windows local sandbox publishes the id through ``env`` rather + # than the command string (#3919). + patch.object(tools_mod, "_is_windows", return_value=False), + patch("deerflow.config.app_config.get_app_config", return_value=fake_cfg), + ): + out = tools_mod.bash_tool.func(runtime=runtime, command="echo hi", description="run local skill") + + assert out == "done" + assert captured["command"] == f"export {USER_ID_ENV}=u-local; echo hi" + assert captured["env"] == {"ERP_TOKEN": "tok-456"} + assert captured["timeout"] == 42 + + def test_local_bash_windows_publishes_user_id_via_env(self, monkeypatch): + """``export`` is invalid under the Windows local sandbox's + PowerShell/cmd.exe shells, so the id moves to the ``env`` channel there + instead of being dropped — the PR promises it on every bash command.""" + from deerflow.sandbox import tools as tools_mod + from deerflow.sandbox.tools import USER_ID_ENV + + captured = {} + + class FakeSandbox: + def execute_command(self, command, env=None, timeout=None): + captured["command"] = command + captured["env"] = env + return "done" + + runtime = SimpleNamespace( + context={"__active_skill_secrets": {"ERP_TOKEN": "tok-789"}, "user_id": "u-win"}, + 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.object(tools_mod, "_is_windows", return_value=True), patch("deerflow.config.app_config.get_app_config", return_value=fake_cfg), ): out = tools_mod.bash_tool.func(runtime=runtime, command="echo hi", description="run local skill") assert out == "done" assert captured["command"] == "echo hi" - assert captured["env"] == {"ERP_TOKEN": "tok-456"} - assert captured["timeout"] == 42 + assert captured["env"] == {"ERP_TOKEN": "tok-789", USER_ID_ENV: "u-win"} def test_remote_bash_does_not_forward_shared_timeout(self): from deerflow.sandbox import tools as tools_mod diff --git a/frontend/src/content/en/harness/subagents/sandbox.mdx b/frontend/src/content/en/harness/subagents/sandbox.mdx index ed611c63bf..d8769b3342 100644 --- a/frontend/src/content/en/harness/subagents/sandbox.mdx +++ b/frontend/src/content/en/harness/subagents/sandbox.mdx @@ -51,6 +51,12 @@ Delegating to `bash` under the local sandbox returns an explicit failure explain Conversations triggered through an IM channel carry the sender identity in the runtime context as `channel_user_id`. The `task` tool forwards it to the subagent, and every bash command the subagent runs is prefixed with `export DEERFLOW_CHANNEL_USER_ID=;` (or `unset DEERFLOW_CHANNEL_USER_ID;` when the IM id is present but empty or invalid; runs without an IM identity get no prefix). The value is at most 256 characters and shell-quoted, so delegated scripts still know who is acting. +Every bash command also carries the authenticated DeerFlow user id as `DEERFLOW_USER_ID`, emitted as the **outermost** prefix — `export DEERFLOW_USER_ID=; `, or `unset DEERFLOW_USER_ID; ` when the resolved id is empty, not a string, or longer than 256 characters. It precedes the channel prefix and the `cd` into the workspace, so reading left to right tells you which user the command runs as, then which IM sender it came from. Unlike the channel id this prefix is never skipped for a non-IM run: `resolve_runtime_user_id` falls back to `default`, so every command is attributable, and restating the value per call keeps a reused AIO shell session from resolving a stale one. + +On the **Windows local sandbox** neither prefix is emitted — `export`/`unset` are not valid syntax under its PowerShell/cmd.exe fallback. The user id is published through the subprocess environment instead, which `LocalSandbox` layers into the per-process environment (it has no persistent session to keep honest), while the channel id is simply absent. + +Treat both shell variables as **informational, not authenticated identity**. Any bash command can overwrite its own environment, so a skill that needs user-scoped authorization must resolve the identity server-side (`resolve_runtime_user_id`) rather than trusting `DEERFLOW_USER_ID`. + ## The subagent middleware chain `build_subagent_runtime_middlewares` assembles the chain. It shares a base with the Lead Agent and then appends subagent-specific middlewares. Roughly, from outermost to innermost: diff --git a/frontend/src/content/zh/harness/subagents/sandbox.mdx b/frontend/src/content/zh/harness/subagents/sandbox.mdx index c40a7d9ce1..8b5d8f57c3 100644 --- a/frontend/src/content/zh/harness/subagents/sandbox.mdx +++ b/frontend/src/content/zh/harness/subagents/sandbox.mdx @@ -51,6 +51,12 @@ AIO 镜像默认最多 10 个 shell 会话（`MAX_SHELL_SESSIONS`），超出时 通过 IM 渠道触发的对话会把发送者身份放进运行时上下文的 `channel_user_id`。`task` 工具把它转发给子 Agent，子 Agent 的每条 bash 命令前会加上 `export DEERFLOW_CHANNEL_USER_ID=<值>;`（IM 身份存在但为空或无效时是 `unset DEERFLOW_CHANNEL_USER_ID;`；没有 IM 身份的运行不加前缀），值最长 256 个字符并经过 shell 转义。因此委派出去的脚本仍然知道是谁在操作。 +每条 bash 命令还会带上已认证的 DeerFlow 用户 id，环境变量名为 `DEERFLOW_USER_ID`，并且作为**最外层**前缀出现——即 `export DEERFLOW_USER_ID=<值>; `，当解析出的 id 为空、不是字符串或超过 256 个字符时则是 `unset DEERFLOW_USER_ID; `。它排在渠道前缀和 `cd` 进工作目录之前，所以从左往右读就是「这条命令以哪个用户身份运行」、再是「来自哪个 IM 发送者」。与渠道 id 不同，这个前缀对非 IM 运行也不会省略：`resolve_runtime_user_id` 会回退到 `default`，因此每条命令都能归属到用户，而每次调用都重申一次也避免复用中的 AIO shell 会话解析到过期的值。 + +在 **Windows 本地沙箱**上两个前缀都不会出现——其 PowerShell/cmd.exe 回退路径不支持 `export`/`unset` 语法。用户 id 改为通过子进程环境传递，`LocalSandbox` 会把它分层并入进程环境（它没有需要保持一致的持久会话），而渠道 id 则完全缺失。 + +两个 shell 变量都应视为**信息性，而非已认证身份**。任何 bash 命令都能覆写自己的环境，因此需要按用户授权的技能必须在服务端解析身份（`resolve_runtime_user_id`），而不能信任 `DEERFLOW_USER_ID`。 + ## 子 Agent 的中间件链 子 Agent 的链由 `build_subagent_runtime_middlewares` 装配，与 Lead Agent 共享同一个基础部分，再追加子 Agent 专属的中间件。大致顺序（外到内）：