mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-25 07:28:07 +00:00
fix(sandbox): fail fast when the AIO image lacks bash.exec for env injection (#3922)
Images older than all-in-one-sandbox 1.9.x have no /v1/bash/* routes, so every env-bearing command (skills declaring required-secrets) surfaced a raw nginx 404 that the model kept retrying. Detect the 404, remember the capability gap per sandbox instance, and return an actionable error that names the minimum image version and the sandbox.image remediation. No fallback through the legacy shell path on purpose: /v1/shell/exec has no env parameter, and every workaround puts the secret values back into the command string or on disk — the exact leak surfaces the request-scoped secrets design closed. Closes #3921
This commit is contained in:
parent
9d7e131340
commit
48477d868b
@ -400,6 +400,7 @@ Lets a caller pass per-request, short-lived end-user credentials (e.g. an ERP to
|
||||
- **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.
|
||||
- **AIO image requirement**: on `AioSandbox` the env path uses the `bash.exec` API (`POST /v1/bash/exec`), which upstream all-in-one-sandbox only ships since `1.9.3` — older images (including a `latest` tag frozen on the `1.0.0.x` line) 404 the whole `/v1/bash/*` namespace. `AioSandbox` detects the 404, remembers the capability gap on the instance, and fails fast with an actionable upgrade error instead of letting the model retry raw 404s; there is deliberately **no** fallback through the legacy shell path because none keeps the secret values out of the command string (#3921). Regression tests: `tests/test_aio_sandbox.py::TestBashExecUnsupportedFailFast`.
|
||||
- **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`.
|
||||
|
||||
@ -455,6 +455,7 @@ If you rebuild the runtime from scratch instead of extending the published image
|
||||
|
||||
- `sandbox.get_context()`, including `home_dir`
|
||||
- `shell.exec_command(...)`
|
||||
- `bash.exec(...)` — only exercised for per-command environment injection (skills that declare `required-secrets`). The `/v1/bash/*` routes exist since upstream all-in-one-sandbox `1.9.3`; on older images (including a `latest` tag still frozen on the `1.0.0.x` line) DeerFlow fails fast with an actionable error instead of surfacing the raw 404. Pin `sandbox.image` to `1.9.3` or newer (e.g. `1.11.0`) and recreate the sandbox container to use `required-secrets` with the AIO sandbox.
|
||||
- `file.read_file(...)`
|
||||
- `file.write_file(...)`, including base64 writes for binary content
|
||||
- streamed `file.download_file(...)`
|
||||
|
||||
@ -6,6 +6,7 @@ import threading
|
||||
import uuid
|
||||
|
||||
from agent_sandbox import Sandbox as AioSandboxClient
|
||||
from agent_sandbox.core.api_error import ApiError
|
||||
|
||||
from deerflow.config.paths import VIRTUAL_PATH_PREFIX
|
||||
from deerflow.sandbox.sandbox import Sandbox
|
||||
@ -17,6 +18,19 @@ _MAX_DOWNLOAD_SIZE = 100 * 1024 * 1024 # 100 MB
|
||||
|
||||
_ERROR_OBSERVATION_SIGNATURE = "'ErrorObservation' object has no attribute 'exit_code'"
|
||||
|
||||
# Env-bearing commands require the bash.exec API (POST /v1/bash/exec), which the
|
||||
# all-in-one-sandbox image only ships since 1.9.x. Older images (including any
|
||||
# ``latest`` tag frozen on the 1.0.0.x line) answer 404 for the whole /v1/bash/*
|
||||
# namespace. That raw 404 is useless to the model (it just retries), so the
|
||||
# sandbox fails fast with this operator-facing message instead (#3921).
|
||||
_BASH_EXEC_UNSUPPORTED_ERROR = (
|
||||
"Error: this sandbox image does not support per-command environment injection "
|
||||
"(POST /v1/bash/exec returned 404), which is required to run skills that declare "
|
||||
"required-secrets. This is a deployment issue that retrying cannot fix: upgrade the "
|
||||
"sandbox image to all-in-one-sandbox >= 1.9.3 (set `sandbox.image` in config.yaml, "
|
||||
"e.g. pin the tag `1.11.0`) and recreate the sandbox container, then try again."
|
||||
)
|
||||
|
||||
|
||||
class AioSandbox(Sandbox):
|
||||
"""Sandbox implementation using the agent-infra/sandbox Docker container.
|
||||
@ -40,6 +54,9 @@ class AioSandbox(Sandbox):
|
||||
self._home_dir = home_dir
|
||||
self._lock = threading.Lock()
|
||||
self._closed = False
|
||||
# Set to True after bash.exec answers 404 (image predates /v1/bash/*),
|
||||
# so later env-bearing calls fail fast instead of re-hitting HTTP (#3921).
|
||||
self._bash_exec_unsupported = False
|
||||
|
||||
@property
|
||||
def base_url(self) -> str:
|
||||
@ -204,7 +221,14 @@ class AioSandbox(Sandbox):
|
||||
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.
|
||||
|
||||
Images older than all-in-one-sandbox 1.9.x have no ``/v1/bash/*`` routes;
|
||||
there is no fallback on the legacy shell path that would keep the secret
|
||||
values out of the command string, so the only safe behaviour is to fail
|
||||
fast with an actionable error (#3921).
|
||||
"""
|
||||
if self._bash_exec_unsupported:
|
||||
return _BASH_EXEC_UNSUPPORTED_ERROR
|
||||
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")
|
||||
@ -229,6 +253,13 @@ class AioSandbox(Sandbox):
|
||||
if stderr:
|
||||
output += f"\nStd Error:\n{stderr}" if output else stderr
|
||||
return output if output else "(no output)"
|
||||
except ApiError as e:
|
||||
if e.status_code == 404:
|
||||
self._bash_exec_unsupported = True
|
||||
logger.error("Sandbox %s does not support bash.exec (/v1/bash/exec returned 404); env-bearing commands are unavailable until the sandbox image is upgraded to all-in-one-sandbox >= 1.9.3", self.id)
|
||||
return _BASH_EXEC_UNSUPPORTED_ERROR
|
||||
logger.error(f"Failed to execute command with injected env in sandbox: {e}")
|
||||
return f"Error: {e}"
|
||||
except Exception as e:
|
||||
logger.error(f"Failed to execute command with injected env in sandbox: {e}")
|
||||
return f"Error: {e}"
|
||||
|
||||
@ -161,6 +161,90 @@ class TestErrorObservationRetry:
|
||||
assert call_count == 1
|
||||
|
||||
|
||||
class TestBashExecUnsupportedFailFast:
|
||||
"""Regression tests for #3921: sandbox images older than all-in-one-sandbox
|
||||
1.9.x have no ``/v1/bash/exec`` route, so every env-bearing command (skills
|
||||
declaring ``required-secrets``) hit a bare nginx 404 that the model kept
|
||||
retrying. The sandbox must fail fast with an actionable, operator-facing
|
||||
error instead."""
|
||||
|
||||
def _api_error_404(self):
|
||||
from agent_sandbox.core.api_error import ApiError
|
||||
|
||||
return ApiError(
|
||||
headers={"server": "nginx/1.18.0 (Ubuntu)"},
|
||||
status_code=404,
|
||||
body={"success": False, "message": "Not Found", "data": None},
|
||||
)
|
||||
|
||||
def test_bash_exec_404_returns_actionable_error(self, sandbox):
|
||||
"""A 404 from bash.exec must explain the image capability gap and the
|
||||
remediation (upgrade image), not surface the raw nginx error."""
|
||||
sandbox._client.bash.exec = MagicMock(side_effect=self._api_error_404())
|
||||
|
||||
out = sandbox.execute_command("echo $TOK", env={"TOK": "secret-v"})
|
||||
|
||||
assert out.startswith("Error:")
|
||||
# Actionable: names the missing capability and the minimum image version.
|
||||
assert "/v1/bash/exec" in out
|
||||
assert "1.9.3" in out
|
||||
assert "required-secrets" in out
|
||||
# Not the raw upstream 404 body the model can't act on.
|
||||
assert "nginx" not in out
|
||||
|
||||
def test_bash_exec_404_is_cached_and_stops_retry_storm(self, sandbox):
|
||||
"""After one 404 the capability gap is remembered on the instance:
|
||||
follow-up env-bearing calls return the same actionable error without
|
||||
another HTTP round-trip (the original bug produced 4 consecutive 404s
|
||||
as the model retried variants of the command)."""
|
||||
sandbox._client.bash.exec = MagicMock(side_effect=self._api_error_404())
|
||||
|
||||
first = sandbox.execute_command("cmd-1", env={"TOK": "v"})
|
||||
second = sandbox.execute_command("cmd-2", env={"TOK": "v"})
|
||||
|
||||
assert sandbox._client.bash.exec.call_count == 1
|
||||
assert first == second
|
||||
assert "1.9.3" in second
|
||||
|
||||
def test_bash_exec_non_404_error_is_not_cached(self, sandbox):
|
||||
"""Transient failures (e.g. 500) must not permanently disable the env
|
||||
path — the next env-bearing call should try bash.exec again."""
|
||||
from agent_sandbox.core.api_error import ApiError
|
||||
|
||||
sandbox._client.bash.exec = MagicMock(side_effect=ApiError(status_code=500, body="boom"))
|
||||
|
||||
first = sandbox.execute_command("cmd-1", env={"TOK": "v"})
|
||||
second = sandbox.execute_command("cmd-2", env={"TOK": "v"})
|
||||
|
||||
assert sandbox._client.bash.exec.call_count == 2
|
||||
assert first.startswith("Error:")
|
||||
assert "1.9.3" not in first
|
||||
assert second.startswith("Error:")
|
||||
|
||||
def test_env_less_path_unaffected_after_404(self, sandbox):
|
||||
"""The legacy persistent-shell path must keep working on an image
|
||||
without bash.exec — only env injection is unavailable there."""
|
||||
sandbox._client.bash.exec = MagicMock(side_effect=self._api_error_404())
|
||||
sandbox._client.shell.exec_command = MagicMock(return_value=SimpleNamespace(data=SimpleNamespace(output="plain ok")))
|
||||
|
||||
sandbox.execute_command("cmd", env={"TOK": "v"})
|
||||
out = sandbox.execute_command("echo plain")
|
||||
|
||||
assert out == "plain ok"
|
||||
sandbox._client.shell.exec_command.assert_called_once()
|
||||
|
||||
def test_bash_exec_success_does_not_mark_unsupported(self, sandbox):
|
||||
"""A healthy bash.exec keeps the env path fully enabled."""
|
||||
sandbox._client.bash.exec = MagicMock(return_value=SimpleNamespace(data=SimpleNamespace(stdout="ok", stderr=None)))
|
||||
|
||||
first = sandbox.execute_command("cmd-1", env={"TOK": "v"})
|
||||
second = sandbox.execute_command("cmd-2", env={"TOK": "v"})
|
||||
|
||||
assert first == "ok"
|
||||
assert second == "ok"
|
||||
assert sandbox._client.bash.exec.call_count == 2
|
||||
|
||||
|
||||
class TestListDirSerialization:
|
||||
"""Verify that list_dir also acquires the lock."""
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user