From 9182ad08609c69f0852df010e2327d86fd77a50a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 19 Sep 2026 21:21:28 -0700 Subject: [PATCH] test: trim egress shorthand tests to two invariants, ignore positionals in the flag scan Fold the salvaged parametrized positive/negative tables and the redundant env-file/real-chain e2e tests into one parametrized invariant (every pflag spelling of -e collides, legitimate args do not) plus one production-entry test through DockerEnvironment. Args that do not start with "-" are positionals under pflag and are never env flags, so the shorthand scanner skips them instead of reading a container name like "de" as "-d -e". --- tests/tools/test_docker_environment.py | 114 ++++--------------------- tools/environments/docker_egress.py | 2 + 2 files changed, 20 insertions(+), 96 deletions(-) diff --git a/tests/tools/test_docker_environment.py b/tests/tools/test_docker_environment.py index c8e1456323..2aa8853324 100644 --- a/tests/tools/test_docker_environment.py +++ b/tests/tools/test_docker_environment.py @@ -1033,73 +1033,35 @@ def test_forward_env_non_suffixed_egress_name_refuses_under_egress(monkeypatch): _make_dummy_env(forward_env=["AWS_SECRET_ACCESS_KEY"]) -_CRITICAL = {"HTTPS_PROXY", "AWS_SECRET_ACCESS_KEY"} - - @pytest.mark.parametrize( - "extra_args", + "extra_args, collides", [ - # Baseline forms the scanner already caught. - ["-e", "HTTPS_PROXY=http://evil"], - ["-e=HTTPS_PROXY=http://evil"], - ["--env", "HTTPS_PROXY=http://evil"], - ["--env=HTTPS_PROXY=http://evil"], - # pflag joined shorthand: docker parses "-eNAME=v" as "-e NAME=v". - ["-eHTTPS_PROXY=http://evil"], + # pflag joined shorthand: docker parses "-eNAME=v" as "-e NAME=v" (#115887). + (["-eHTTPS_PROXY=http://evil"], True), # Name-only passthrough shorthand injects the host's real variable. - ["-eAWS_SECRET_ACCESS_KEY"], + (["-eAWS_SECRET_ACCESS_KEY"], True), # Chained after docker run's boolean shorthands (-d -i -t -P -q). - ["-iteHTTPS_PROXY=http://evil"], - ["-deHTTPS_PROXY=http://evil"], + (["-iteHTTPS_PROXY=http://evil"], True), # A chain ending in bare "e" takes the next arg as its value. - ["-ite", "HTTPS_PROXY=http://evil"], - ["-dit", "-eHTTPS_PROXY=http://evil"], - # A value-taking flag's operand is not a positional: the env flag - # after it is still parsed by docker and must still collide. - ["--entrypoint", "/bin/sh", "-e", "HTTPS_PROXY=x"], - ["-w", "/app", "-eHTTPS_PROXY=x"], - ["--name", "ctr", "--env", "AWS_SECRET_ACCESS_KEY"], + (["-ite", "HTTPS_PROXY=http://evil"], True), + (["--env=HTTPS_PROXY=http://evil"], True), + (["--", "--env-file", "f"], False), # pflag terminator: rest is image/cmd + # Legitimate args must keep working under enforcement. + (["-eFOO=bar"], False), + (["-dit", "-p8080:80", "-v/tmp:/tmp", "--name", "de", "-e", "FOO=bar"], False), ], ) -def test_extra_args_egress_collision_shorthand_forms(extra_args): +def test_extra_args_egress_collision_follows_docker_shorthand_parsing(extra_args, collides): """Docker's pflag CLI accepts a value-taking shorthand joined to its value and chained after boolean shorthands; every spelling of ``-e`` that injects - a critical env name must collide.""" - collisions = _extra_args_egress_collisions(extra_args, _CRITICAL) - assert collisions, f"{extra_args} injects a critical env name under docker's own parsing" - - -@pytest.mark.parametrize( - "extra_args", - [ - ["-dit"], # boolean chain only - ["-p8080:80"], # non-env value shorthand, joined - ["-v/tmp:/tmp"], # volume shorthand, joined - ["-w/app"], # workdir shorthand, joined - ["-eFOO=bar"], # joined env, non-critical name - ["-e", "FOO=bar"], # two-arg env, non-critical - ["--", "-e", "HTTPS_PROXY=x"], # pflag terminator: rest is image/cmd - ["-x"], # unknown shorthand; docker errors - ["--entrypoint=/bin/sh"], - ["--name", "ctr"], - ], -) -def test_extra_args_egress_collision_non_env_forms(extra_args): - """Args that cannot inject env under docker's parsing must not collide.""" - assert _extra_args_egress_collisions(extra_args, _CRITICAL) == [] - - -def test_extra_args_env_file_shorthand_and_terminator(): - """--env-file collides unconditionally; the scan honors the terminator.""" - assert _extra_args_egress_collisions(["--env-file", "f"], _CRITICAL) == ["--env-file"] - assert _extra_args_egress_collisions(["--env-file=f"], _CRITICAL) == ["--env-file"] - assert _extra_args_egress_collisions( - ["--", "--env-file", "f"], _CRITICAL) == [] + a critical env name must collide, and nothing else may (#115887).""" + critical = {"HTTPS_PROXY", "AWS_SECRET_ACCESS_KEY"} + assert bool(_extra_args_egress_collisions(extra_args, critical)) is collides def test_extra_args_joined_shorthand_refuses_under_egress(monkeypatch): - """End to end: a combined ``-eNAME=v`` in docker_extra_args must trip the - enforced-egress guard exactly like the spaced ``-e NAME=v`` form.""" + """End to end: a boolean-chained ``-iteNAME=v`` in docker_extra_args must trip + the enforced-egress guard exactly like the spaced ``-e NAME=v`` form.""" monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") monkeypatch.setattr( docker_env, @@ -1109,47 +1071,7 @@ def test_extra_args_joined_shorthand_refuses_under_egress(monkeypatch): _mock_subprocess_run(monkeypatch) with pytest.raises(RuntimeError, match="docker_extra_args.*HTTPS_PROXY"): - _make_dummy_env(extra_args=["-eHTTPS_PROXY=http://10.0.0.9:3128"]) - - -def test_extra_args_joined_shorthand_real_egress_chain(monkeypatch, tmp_path): - """Deepest e2e: the real ``_egress_proxy_args_for_docker`` plumbing (config, - iron-proxy status, token mappings) produces the critical-name set, and a - joined ``-e=v`` shorthand collides before ``docker run`` is invoked. - A mapped credential name (OPENAI_API_KEY) is critical by suffix, so this - also covers real-credential injection, not only proxy-control vars.""" - import agent.proxy_sources.iron_proxy as ip - from hermes_cli import config as hc - - monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") - ca = tmp_path / "hermes-egress-ca.crt" - ca.write_text("x") - proxy_cfg = tmp_path / "proxy.yaml" - proxy_cfg.write_text("x") - monkeypatch.setattr( - hc, "load_config", - lambda: {"proxy": {"enabled": True, "enforce_on_docker": True}}) - monkeypatch.setattr( - ip, "get_status", - lambda: ip.ProxyStatus( - enabled=True, config_path=proxy_cfg, ca_cert_path=ca, - pid=4321, listening=True)) - monkeypatch.setattr( - ip, "load_mappings", - lambda: [ip.TokenMapping( - proxy_token="tok", real_env_name="OPENAI_API_KEY", - upstream_hosts=("api.openai.com",))]) - calls = _mock_subprocess_run(monkeypatch) - - with pytest.raises(RuntimeError, match="docker_extra_args.*OPENAI_API_KEY"): - _make_dummy_env(extra_args=["-eOPENAI_API_KEY=sk-real"]) - - run_calls = [ - c for c in calls - if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run" - and "sleep" not in c[0] - ] - assert not run_calls, "the collision must refuse before docker run" + _make_dummy_env(extra_args=["-iteHTTPS_PROXY=http://10.0.0.9:3128"]) def test_reuse_starts_stopped_container_before_attaching(monkeypatch): diff --git a/tools/environments/docker_egress.py b/tools/environments/docker_egress.py index 33e2453cdb..37344ce598 100644 --- a/tools/environments/docker_egress.py +++ b/tools/environments/docker_egress.py @@ -161,6 +161,8 @@ def _env_flag_value(arg: str) -> tuple[str, str | None] | None: if flag in ("--env", "--env-file"): return flag[2:], inline if sep else None return None + if not arg.startswith("-"): + return None # positional (image/command), never a flag # A single dash heads a shorthand chain: every letter before the last must # be a boolean shorthand, and the last takes the rest of the arg (or the # next arg when bare) as its value.