diff --git a/gateway/authz_mixin.py b/gateway/authz_mixin.py index fae4b74a5e..d7b3ca13ae 100644 --- a/gateway/authz_mixin.py +++ b/gateway/authz_mixin.py @@ -28,28 +28,12 @@ from gateway.whatsapp_identity import ( ) -def _auth_env(name: str, default: str = "") -> str: - """Read allowlist/auth env; prefer profile secret_scope under multiplex.""" - if not name: - return default - try: - from agent.secret_scope import get_secret - - val = get_secret(name) - if val is not None and str(val).strip(): - return str(val).strip() - except Exception: - pass - return (os.getenv(name) or default).strip() - - def _platform_gate_env(name: str, default: str = "") -> str: """Read a platform allow/deny gate env var with per-profile isolation. - Like ``_auth_env`` but authoritative under multiplex: when a profile - secret scope is installed AND multiplexing is active, a key absent from - the scope returns ``default`` instead of falling through to - ``os.environ``. Under multiplex the process env may hold ANOTHER + When a profile secret scope is installed AND multiplexing is active, a + key absent from the scope returns ``default`` instead of falling through + to ``os.environ``. Under multiplex the process env may hold ANOTHER profile's first-writer-bridged value (the YAML→env bridges in the Discord/Telegram adapters' ``_apply_yaml_config`` are first-writer-wins), so falling through would leak profile A's allowlist into profile B @@ -72,6 +56,19 @@ def _platform_gate_env(name: str, default: str = "") -> str: return (os.getenv(name) or default).strip() +def _auth_env(name: str, default: str = "") -> str: + """Read allowlist/auth env with per-profile isolation under multiplex. + + Same rules as ``_platform_gate_env``: a scoped miss under multiplex + returns ``default`` and does not fall through to ``os.environ``. The + process env may hold another profile's first-writer-bridged value, so + a fallthrough would leak allowlists and allow-all flags across profiles + (issue #72348). Single-profile deployments keep the legacy + ``os.getenv`` read. + """ + return _platform_gate_env(name, default) + + def _coerce_allow_set(raw) -> set[str]: """Parse allowlist values from config or env var into a set of strings. diff --git a/tests/agent/test_secret_scope_tier1_migration.py b/tests/agent/test_secret_scope_tier1_migration.py index cceb32e21f..78f598f261 100644 --- a/tests/agent/test_secret_scope_tier1_migration.py +++ b/tests/agent/test_secret_scope_tier1_migration.py @@ -94,6 +94,37 @@ class TestAuthzPlatformGateEnv: assert _platform_gate_env("GATEWAY_ALLOWED_USERS") == "42" +class TestAuthzAuthEnv: + """_auth_env must follow _platform_gate_env isolation (no os.environ + fallthrough on a scoped miss under multiplex).""" + + def test_scoped_value_wins(self, monkeypatch): + from gateway.authz_mixin import _auth_env + + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "111") + ss.set_multiplex_active(True) + with _Scope({"TELEGRAM_ALLOWED_USERS": "222"}): + assert _auth_env("TELEGRAM_ALLOWED_USERS") == "222" + + def test_scoped_miss_returns_default_not_env(self, monkeypatch): + from gateway.authz_mixin import _auth_env + + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "profile-A") + monkeypatch.setenv("GATEWAY_ALLOW_ALL_USERS", "true") + monkeypatch.setenv("TELEGRAM_ALLOW_ALL_USERS", "true") + ss.set_multiplex_active(True) + with _Scope({"UNRELATED": "x"}): + assert _auth_env("TELEGRAM_ALLOWED_USERS") == "" + assert _auth_env("GATEWAY_ALLOW_ALL_USERS") == "" + assert _auth_env("TELEGRAM_ALLOW_ALL_USERS") == "" + + def test_single_profile_legacy_env(self, monkeypatch): + from gateway.authz_mixin import _auth_env + + monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "42") + assert _auth_env("GATEWAY_ALLOWED_USERS") == "42" + + # ── Cluster B: matrix startup reads (Slack pattern) ──────────────────────── class TestMatrixStartupSecret: diff --git a/tests/gateway/test_qqbot_scope_paths.py b/tests/gateway/test_qqbot_scope_paths.py index 11a2ab70c0..30890e41d4 100644 --- a/tests/gateway/test_qqbot_scope_paths.py +++ b/tests/gateway/test_qqbot_scope_paths.py @@ -97,16 +97,6 @@ class TestAuthzAllowAllScope: finally: ss.reset_secret_scope(tok) - @pytest.mark.xfail( - reason=( - "gateway/authz_mixin.py still reads the platform allow-all flag via " - "_auth_env, which falls through to os.environ on a scoped miss; the " - "scope-authoritative gate (_platform_gate_env semantics) for the " - "remaining authz_mixin reads lands in a separate PR. Flips green " - "when that PR converts the allow-all read." - ), - strict=True, - ) def test_scope_does_not_inherit_environ_opt_in(self, monkeypatch): # The PRIMARY profile opted in via os.environ; the secondary profile's # scope has no opt-in. The secondary must NOT inherit the primary's @@ -157,6 +147,33 @@ class TestAuthzAllowlistScope: finally: ss.reset_secret_scope(tok) + def test_scope_miss_does_not_inherit_environ_allowlist(self, monkeypatch): + # Primary env lists the sender. Secondary scope has no allowlist + # key at all. The miss must deny, not borrow the process value. + monkeypatch.setenv("QQ_ALLOWED_USERS", "user-1") + runner = _make_qq_runner() + ss.set_multiplex_active(True) + tok = ss.set_secret_scope({}) + try: + assert runner._is_user_authorized(_qq_dm_source()) is False + finally: + ss.reset_secret_scope(tok) + + +class TestAuthzGatewayAllowAllScope: + def test_scope_miss_does_not_inherit_gateway_allow_all(self, monkeypatch): + # GATEWAY_ALLOW_ALL_USERS is the last-chance fail-open flag, also + # read through _auth_env. A secondary profile that never set it + # must not inherit the primary's process-env opt-in. + monkeypatch.setenv("GATEWAY_ALLOW_ALL_USERS", "true") + runner = _make_qq_runner() + ss.set_multiplex_active(True) + tok = ss.set_secret_scope({}) + try: + assert runner._is_user_authorized(_qq_dm_source()) is False + finally: + ss.reset_secret_scope(tok) + class TestStartupValidatorScope: @staticmethod diff --git a/tests/tools/test_bot_relay.py b/tests/tools/test_bot_relay.py index f2166d3f7b..b7b9f4a37f 100644 --- a/tests/tools/test_bot_relay.py +++ b/tests/tools/test_bot_relay.py @@ -151,6 +151,56 @@ def test_waiter_command_quotes_and_targets_reply_file(root): assert "rm -rf" not in cmd # sanity: single quoted -c payload +def test_roster_rejects_connection_id_outside_handle_charset(root): + bad = [ + {"profile": "researcher", "handle": "researcher", "connection_id": "vps'); print(1)"}, + {"profile": "researcher", "handle": "researcher", "connection_id": "foo'bar"}, + {"profile": "researcher", "handle": "researcher", "connection_id": "ssh vps"}, + {"profile": "researcher", "handle": "researcher", "connection_id": "a" * 65}, + ] + assert bot_relay.write_remote_roster(root, bad) == 0 + good = { + "profile": "researcher", + "handle": "researcher", + "connection_id": "ssh-vps", + } + assert bot_relay.write_remote_roster(root, [good]) == 1 + + +def test_waiter_command_repr_encodes_hostile_connection_id(root): + import ast + import shlex + + inj = "x'); open(r'/tmp/pwned','w').write('pwned'); print('x" + env = { + "id": "c" * 32, + "target_handle": "researcher", + "target_connection": inj, + } + cmd = bot_relay.waiter_command(root, env) + parts = shlex.split(cmd) + code = parts[parts.index("-c") + 1] + compile(code, "", "exec") + tree = ast.parse(code) + opens = [ + n + for n in ast.walk(tree) + if isinstance(n, ast.Call) + and isinstance(n.func, ast.Name) + and n.func.id == "open" + ] + # Only json.load(open(p, ...)) is a real open(); the payload must stay data. + assert len(opens) == 1 + + # A quote in the id used to SyntaxError the waiter. It must compile. + quoted = bot_relay.waiter_command( + root, + {"id": "a" * 32, "target_handle": "h", "target_connection": "foo'bar"}, + ) + qcode = shlex.split(quoted)[shlex.split(quoted).index("-c") + 1] + compile(qcode, "", "exec") + + # ── message_agent integration: relay route + legacy-SOUL gate fix ─────────── import textwrap diff --git a/tools/bot_relay.py b/tools/bot_relay.py index 04b023eb16..e0cc026484 100644 --- a/tools/bot_relay.py +++ b/tools/bot_relay.py @@ -124,7 +124,11 @@ def _normalize_roster_row(row: Any) -> Optional[dict]: return None if not handle: handle = "hermes" if profile == "default" else profile - if not _HANDLE_RE.match(handle) or not _HANDLE_RE.match(profile): + if ( + not _HANDLE_RE.match(handle) + or not _HANDLE_RE.match(profile) + or not _HANDLE_RE.match(connection_id) + ): return None out = { "profile": profile, @@ -486,22 +490,28 @@ def waiter_command(root: Path | str, envelope: dict) -> str: interpreter. """ reply_path = str(relay_root(root) / REPLIES_DIR / f"{envelope['id']}.json") - label = f"@{envelope['target_handle']} on {envelope['target_connection']}" + label = ( + f"@{envelope.get('target_handle', '')} " + f"on {envelope.get('target_connection', '')}" + ) + # Encode label with !r so roster fields cannot break out of the generated + # python -c source (quotes, parens, or extra statements in connection_id). code = ( "import json,os,sys,time\n" f"p = {reply_path!r}\n" + f"label = {label!r}\n" f"deadline = time.time() + {REPLY_WAIT_SECONDS}\n" "while time.time() < deadline:\n" " if os.path.exists(p):\n" " d = json.load(open(p, encoding='utf-8'))\n" " if d.get('error'):\n" - f" print('Delivery to {label} failed: ' + d['error'])\n" + " print('Delivery to ' + label + ' failed: ' + d['error'])\n" " sys.exit(1)\n" - f" print('Reply from {label}:')\n" + " print('Reply from ' + label + ':')\n" " print(d.get('reply') or '(empty reply)')\n" " sys.exit(0)\n" " time.sleep(2)\n" - f"print('No reply from {label} within {REPLY_WAIT_SECONDS}s. The message may " + f"print('No reply from ' + label + ' within {REPLY_WAIT_SECONDS}s. The message may " "still be delivered when the Desktop reconnects; do not resend blindly.')\n" "sys.exit(1)\n" )