From 04a85b307a2d3adb0a966d61dcbbfe1b718c9bef Mon Sep 17 00:00:00 2001 From: Aari Date: Fri, 10 Jul 2026 14:05:47 +0800 Subject: [PATCH] fix(sandbox): scrub abbreviated *_PASS vars and Postgres PGPASSFILE from skill env (#4026) * fix(sandbox): scrub abbreviated *_PASS vars and Postgres PGPASSFILE from skill env env_policy blocks the full PASSWORD/PASSWD spellings but not the ubiquitous abbreviated _PASS form (DB_PASS, SMTP_PASS, MYSQL_PASS, ...), whose value is the plaintext password itself, so every skill subprocess inherited it. Postgres's file-based credential sources PGPASSFILE (.pgpass locator) and PGSERVICEFILE (pg_service.conf, may carry a password) were likewise missed -- the psql analog of the MYSQL_PWD/REDISCLI_AUTH no-flag sources #4018 added. Collapse *PASSWORD*/*PASSWD* into a single *PASS* pattern (which subsumes both, covers the abbreviated family, and catches PGPASSFILE) and add PGSERVICEFILE to the exact-name denylist. *PASS* deliberately does not touch PWD/OLDPWD, which carry no PASS substring. Capability-handle vars (KUBECONFIG, DOCKER_CONFIG, GIT_ASKPASS, ...) are a broader, non-value-bearing class and are left out of this fix. test_secret_like_names_are_blocked already pins the contract; the new names are added to its parametrize list (red before, green after) and the benign-names test guards against over-scrubbing PWD/OLDPWD. * test(sandbox): pin the *_ASKPASS over-scrub that *PASS* introduces *PASS* also matches the credential-helper vars GIT_ASKPASS, SSH_ASKPASS and SUDO_ASKPASS, which main inherits. Each names a program whose purpose is to hand the caller a credential, so inheriting the pointer is the same leak class as inheriting the value -- a skill can simply invoke it. Scrubbing them is intended; pin all three in test_secret_like_names_are_blocked (red on main) so the behaviour is a decision rather than a side effect of the pattern's shape. test_benign_names_are_allowed could not have caught this: none of its 14 names carries a PASS substring, so it passes identically before and after the broadening and asserts nothing about it. Record that in its docstring -- the absence of a benign PASS-bearing name is a decision, and PWD/OLDPWD are the boundary the list does pin. Also note the incidental COMPASS_*/BYPASS_* over-scrub in the module comment (over-scrubbing is this module's fail-safe direction; required-secrets is the escape hatch), and list PGPASSFILE alongside PGSERVICEFILE in AGENTS.md. --- backend/AGENTS.md | 4 +-- .../harness/deerflow/sandbox/env_policy.py | 32 +++++++++++++++---- .../test_skill_request_scoped_secrets.py | 32 +++++++++++++++++++ 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 1c4c86e12..6bb7885b1 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -348,7 +348,7 @@ Proxied through nginx: `/api/langgraph/*` → Gateway LangGraph-compatible runti **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. +**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*`/`*PASS*`/`*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`. Legacy global-custom mounts are gated by the same user-scoped skill discovery rule used for prompt/list visibility; providers must not infer visibility from raw directory presence alone. - `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. Legacy global-custom mounts follow the same shared visibility helper as local and remote providers. @@ -458,7 +458,7 @@ Lets a caller pass per-request, short-lived end-user credentials (e.g. an ERP to - **Bind (point A+)**: `SkillActivationMiddleware._resolve_secret_bindings` recomputes the injection set (`runtime.context[__active_skill_secrets]`) on every model call from two unioned sources, then REPLACES the key. (1) *Slash*: the run's most recent `/skill` activation, persisted as a source on the run context (only the activated skill's **canonical container path**, never its declared secrets) so the whole tool loop after the activation call keeps the binding; a new activation replaces it. Slash reads the genuine user text via `get_original_user_content_text`; `InputSanitizationMiddleware` preserves it (`ORIGINAL_USER_CONTENT_KEY`), so activation fires even after sanitization. (2) *In-context* (autonomous invocation): skills the model actually loaded in this thread — `ThreadState.skill_context` entries. **Both sources resolve the live registry skill by normalized container path on every call** (`_resolve_registry_skill`) and bind only that skill's own declared secrets — enabled + allowlist checked for both; the `secrets-autonomous: false` opt-out (malformed values fail closed to `false`) additionally gates the in-context path but exempts explicit slash. Resolving by registry — not by trusting the source's stored data — is what makes a caller-forged `__slash_skill_secret_source` harmless (`runtime.context` is caller-mergeable; the gateway also strips caller `__`-keys in `build_run_config`), #3938. Authorization is three-gated regardless of activation style: skill **enabled** by the operator × values **supplied per-request** by the caller (`context.secrets`) × names **declared** in frontmatter (∩ semantics). Because the set is recomputed per call, a skill evicted from `skill_context` (capacity) or a caller that stops supplying a value loses injection on the next call. 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 once per binding change, not injected; binding changes are recorded as a `middleware:skill_secrets` journal event (skill and secret names only, never values). - **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. +- **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*`/`*PASS*`/`*CREDENTIAL*`/`*DSN*` + a connection-string denylist like `DATABASE_URL`/`REDIS_URL`/`GH_PAT`, plus no-flag credential sources like `MYSQL_PWD`/`REDISCLI_AUTH`/`PGPASSFILE`/`PGSERVICEFILE`) 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**: no persistence/vaulting — values are request-scoped and never stored server-side, so long-lived use means the caller re-supplies `context.secrets` on each request while the skill stays in `skill_context`; subagents do not inherit the injection set; the MCP per-user-credential gap (#3322) is a sibling, not covered here. Tests: `tests/test_skill_request_scoped_secrets.py`. diff --git a/backend/packages/harness/deerflow/sandbox/env_policy.py b/backend/packages/harness/deerflow/sandbox/env_policy.py index d0603041f..6381c7d7d 100644 --- a/backend/packages/harness/deerflow/sandbox/env_policy.py +++ b/backend/packages/harness/deerflow/sandbox/env_policy.py @@ -25,8 +25,22 @@ _SECRET_NAME_PATTERNS: tuple[str, ...] = ( "*KEY*", "*SECRET*", "*TOKEN*", - "*PASSWORD*", - "*PASSWD*", + # ``*PASS*`` subsumes the full ``PASSWORD``/``PASSWD`` spellings *and* the + # ubiquitous abbreviated form (``DB_PASS``, ``SMTP_PASS``, ``MYSQL_PASS``, ...), + # whose plaintext value is the password itself. It also covers ``PGPASSFILE`` + # (libpq's ``.pgpass`` locator). + # + # It deliberately also catches the ``*_ASKPASS`` credential helpers + # (``GIT_ASKPASS``, ``SSH_ASKPASS``, ``SUDO_ASKPASS``). Those name a *program* + # rather than a secret, but that program exists to hand the caller a + # credential — inheriting the pointer is the same leak class this module + # closes, so scrubbing them is intended, not incidental. + # + # Incidental names that merely contain ``PASS`` (``COMPASS_*``, ``BYPASS_*``) + # are scrubbed too. That is the fail-safe direction for this module: a skill + # that genuinely needs any scrubbed name declares it via required-secrets. + # Benign ``PWD``/``OLDPWD`` carry no ``PASS`` substring and are unaffected. + "*PASS*", "*CREDENTIAL*", "*DSN*", # data source name — almost always a connection string with a password ) @@ -38,14 +52,17 @@ _SECRET_NAME_PATTERNS: tuple[str, ...] = ( # 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). # -# The same reasoning covers the password variables those clients read directly. +# The same reasoning covers the credential sources those clients read directly. # ``MYSQL_PWD`` and ``REDISCLI_AUTH`` are the documented no-flag credential # sources for ``mysql`` and ``redis-cli``. ``REDIS_AUTH`` is *not* canonical for # any standard Redis client — it is blocked defensively because client libraries -# and deployment charts commonly set it. -# All three need exact entries: ``PWD``/``AUTH`` cannot be wildcarded, since -# ``*PWD*`` would strip ``PWD`` and ``OLDPWD``. (``*PASSWORD*``/``*PASSWD*`` -# already cover ``PGPASSWORD``, ``MYSQL_PASSWORD``, ``REDIS_PASSWORD``, ...) +# and deployment charts commonly set it. ``PGSERVICEFILE`` is the Postgres analog: +# libpq reads the ``pg_service.conf`` it points at (which may carry a password +# field) with no flag; its sibling ``PGPASSFILE`` is already caught by ``*PASS*``. +# These need exact entries: ``PWD``/``AUTH``/``SERVICEFILE`` cannot be wildcarded, +# since ``*PWD*`` would strip ``PWD``/``OLDPWD`` and no shared token is unique to +# them. (``*PASS*`` already covers ``PGPASSWORD``, ``MYSQL_PASSWORD``, ``DB_PASS``, +# ``PGPASSFILE``, ...) _BLOCKED_EXACT_NAMES: frozenset[str] = frozenset( { "DATABASE_URL", @@ -66,6 +83,7 @@ _BLOCKED_EXACT_NAMES: frozenset[str] = frozenset( "MYSQL_PWD", "REDISCLI_AUTH", "REDIS_AUTH", + "PGSERVICEFILE", } ) diff --git a/backend/tests/test_skill_request_scoped_secrets.py b/backend/tests/test_skill_request_scoped_secrets.py index f741c1997..b2d59603e 100644 --- a/backend/tests/test_skill_request_scoped_secrets.py +++ b/backend/tests/test_skill_request_scoped_secrets.py @@ -167,6 +167,28 @@ class TestEnvPolicy: "MYSQL_PWD", # read directly by mysql / libmysqlclient "REDISCLI_AUTH", # read directly by redis-cli "REDIS_AUTH", + # Abbreviated ``_PASS`` password vars: value-bearing plaintext passwords + # that the full-spelling ``*PASSWORD*`` / ``*PASSWD*`` patterns miss. + "DB_PASS", + "SMTP_PASS", + "MYSQL_PASS", + "REDIS_PASS", + "FTP_PASS", + "MAIL_PASS", + # Postgres file-based credential sources read by libpq/psql with no flag, + # the direct analog of MYSQL_PWD/REDISCLI_AUTH above. PGPASSFILE names a + # .pgpass (host:port:db:user:password); PGSERVICEFILE names a + # pg_service.conf that may carry a password field. + "PGPASSFILE", + "PGSERVICEFILE", + # Credential *helpers*: each names a program that dispenses a credential + # on demand. Inheriting the pointer is the same leak class as inheriting + # the value, so ``*PASS*`` scrubbing them is intended. Pinned here so the + # behaviour is a deliberate decision rather than a side effect of the + # pattern's shape. + "GIT_ASKPASS", + "SSH_ASKPASS", + "SUDO_ASKPASS", ], ) def test_secret_like_names_are_blocked(self, name): @@ -196,6 +218,16 @@ class TestEnvPolicy: ], ) def test_benign_names_are_allowed(self, name): + """Names here must survive the scrub. + + Note what this list does *not* contain: any name carrying a ``PASS`` + substring. That is deliberate, not an oversight — ``*PASS*`` scrubs every + such name, including the ``*_ASKPASS`` credential helpers pinned in + ``test_secret_like_names_are_blocked`` above. Over-scrubbing is this + module's fail-safe direction; a skill that needs a scrubbed name declares + it via ``required-secrets``. ``PWD``/``OLDPWD`` are the boundary this list + does pin: they carry no ``PASS`` substring and must never be stripped. + """ from deerflow.sandbox.env_policy import is_blocked_env_name assert is_blocked_env_name(name) is False