fix(sandbox): scrub MYSQL_PWD and REDISCLI_AUTH from the inherited skill env (#4018)

* fix(sandbox): scrub MYSQL_PWD and REDISCLI_AUTH from the inherited skill env

env_policy blocks the MySQL and Redis connection strings (MYSQL_URL, REDIS_URL)
because they embed a password, but not the password variables the same clients
read directly: mysql/libmysqlclient takes MYSQL_PWD and redis-cli takes
REDISCLI_AUTH with no further configuration. Both names carry no
KEY/SECRET/TOKEN/PASSWORD/PASSWD substring, so they were inherited by every
skill subprocess -- the leak surface this module exists to close (#3861).

They need exact entries rather than a pattern: *PWD* would also strip PWD and
OLDPWD. REDIS_AUTH is included as the common non-canonical variant.

* Apply suggestions from code review

- Distinguish REDIS_AUTH (defensive, non-canonical) from MYSQL_PWD and
  REDISCLI_AUTH (documented no-flag credential sources) in the comment.
- Pin that request-scoped injection still overrides the host value for the
  newly blocked names, keeping required-secrets a working escape hatch.
This commit is contained in:
Aari 2026-07-10 07:40:19 +08:00 committed by GitHub
parent 488ec17899
commit c11713c539
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 50 additions and 0 deletions

View File

@ -37,6 +37,15 @@ _SECRET_NAME_PATTERNS: tuple[str, ...] = (
# 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).
#
# The same reasoning covers the password variables 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``, ...)
_BLOCKED_EXACT_NAMES: frozenset[str] = frozenset(
{
"DATABASE_URL",
@ -54,6 +63,9 @@ _BLOCKED_EXACT_NAMES: frozenset[str] = frozenset(
"CONN_STR",
"GH_PAT",
"GITHUB_PAT",
"MYSQL_PWD",
"REDISCLI_AUTH",
"REDIS_AUTH",
}
)

View File

@ -160,6 +160,13 @@ class TestEnvPolicy:
"POSTGRES_DSN",
"CONN_STR",
"GH_PAT",
# Password vars for services whose connection strings are already blocked
# above. These carry no KEY/SECRET/TOKEN/PASSWORD/PASSWD substring, and a
# blanket ``*PWD*`` / ``*AUTH*`` pattern would strip benign vars (``PWD``,
# ``OLDPWD``), so they need exact entries.
"MYSQL_PWD", # read directly by mysql / libmysqlclient
"REDISCLI_AUTH", # read directly by redis-cli
"REDIS_AUTH",
],
)
def test_secret_like_names_are_blocked(self, name):
@ -177,6 +184,7 @@ class TestEnvPolicy:
"LANG",
"LC_ALL",
"PWD",
"OLDPWD",
"TMPDIR",
"VIRTUAL_ENV",
"PYTHONPATH",
@ -192,6 +200,36 @@ class TestEnvPolicy:
assert is_blocked_env_name(name) is False
def test_db_password_vars_do_not_reach_the_subprocess_env(self, monkeypatch):
"""The URL forms are scrubbed; the password vars for the same services must be too.
``mysql`` reads ``MYSQL_PWD`` and ``redis-cli`` reads ``REDISCLI_AUTH`` as the
password with no further configuration, so inheriting them hands a skill
subprocess the credential the connection-string block already withholds.
"""
from deerflow.sandbox.env_policy import build_sandbox_env
monkeypatch.setenv("MYSQL_URL", "mysql://user:pw@host/db")
monkeypatch.setenv("MYSQL_PWD", "prod-db-password")
monkeypatch.setenv("REDISCLI_AUTH", "prod-redis-auth")
env = build_sandbox_env()
assert "MYSQL_URL" not in env
assert "MYSQL_PWD" not in env
assert "REDISCLI_AUTH" not in env
assert env.get("PWD") # the working directory must survive the added entries
def test_injection_still_wins_for_the_newly_blocked_names(self, monkeypatch):
"""``required-secrets`` stays the escape hatch for the names added here.
The request-scoped value must also override the host's, which is the
per-user-key-overrides-shared-key case from #3861.
"""
from deerflow.sandbox.env_policy import build_sandbox_env
monkeypatch.setenv("MYSQL_PWD", "host-value-must-not-leak")
env = build_sandbox_env(injected={"MYSQL_PWD": "request-scoped-value"})
assert env["MYSQL_PWD"] == "request-scoped-value"
def test_build_sandbox_env_scrubs_inherited_and_layers_injected(self, monkeypatch):
from deerflow.sandbox.env_policy import build_sandbox_env