From c11713c539d555d943719a4c80ed86c9e0eeb4cc Mon Sep 17 00:00:00 2001 From: Aari Date: Fri, 10 Jul 2026 07:40:19 +0800 Subject: [PATCH] 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. --- .../harness/deerflow/sandbox/env_policy.py | 12 ++++++ .../test_skill_request_scoped_secrets.py | 38 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/backend/packages/harness/deerflow/sandbox/env_policy.py b/backend/packages/harness/deerflow/sandbox/env_policy.py index e86ba704d..d0603041f 100644 --- a/backend/packages/harness/deerflow/sandbox/env_policy.py +++ b/backend/packages/harness/deerflow/sandbox/env_policy.py @@ -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", } ) diff --git a/backend/tests/test_skill_request_scoped_secrets.py b/backend/tests/test_skill_request_scoped_secrets.py index 56f6ff0e4..f741c1997 100644 --- a/backend/tests/test_skill_request_scoped_secrets.py +++ b/backend/tests/test_skill_request_scoped_secrets.py @@ -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