From a041af542f126e96e859497392f8959f24af5c18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E5=88=9A=20/=20Bryan=20Li?= <166219058+lisir202446@users.noreply.github.com> Date: Sat, 24 Oct 2026 08:53:50 +0800 Subject: [PATCH] fix(skillscan): keep a live client handle through a branch that rebinds it later (#7128) * fix(skillscan): keep a live client handle through a branch that rebinds it later The instance-client signal dropped every name a compound statement could bind before walking any of its parts, so an outbound call that really happened first disappeared when a later statement in that same body rebound the name (issue #4296, case 1). Dropping up front is what keeps a benign skill from being hard-blocked for the parts that can *follow* a sibling's rebinding -- a `finally` after a handler replaced the handle, an `else` after a loop body -- so those keep it. Parts that cannot follow a sibling (an `if`/`elif`/`else` branch, the one `match` case that runs, a `try` body, a loop body) are now walked with the enclosing handle intact and apply their own bindings in source order, the rule the top level already uses. Names the statement binds as it dispatches -- a loop target, an `except ... as` name, a `match` capture -- stay dropped before every part. * fix(skillscan): walk a compound header before dropping its dispatch bindings A loop target, an `except ... as` name and a `match` capture bind only after that header has been evaluated, so dropping them first hid a real sink written in the header itself: `for s in s.post(...)` with `s` a live handle reported nothing, while the same call under a differently named target was found. Move the drop to after the header loop and before the parts, which is all `_branching_parts` needs; the body still starts with those names gone, so the loop-value case stays unreported. Raised in review on #7128. Pins the two header shapes, the inverse that keeps the body conservative, and the three paths the review called out as unpinned: the mutually-exclusive two-level `if`/`elif` case, a `while` body, and a `try` `else` after a body rebind. --------- Co-authored-by: Willem Jiang --- .../harness/deerflow/skills/AGENTS.md | 2 +- .../deerflow/skills/skillscan/orchestrator.py | 91 +++++++++++++++---- backend/tests/test_skillscan_native.py | 38 +++++++- 3 files changed, 108 insertions(+), 23 deletions(-) diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index d5dd94eab6..e48320b83e 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -29,7 +29,7 @@ - **Slash activation**: `/skill-name task` loads that enabled skill's `SKILL.md` for the current model call only. The resolver rejects leading whitespace, missing separators, reserved command syntax (`/new`, `/help`, `/bootstrap`, `/status`, `/models`, `/memory`, `/goal`, `/agent`, and the exact composer-only alias `/context compact`), disabled skills, and skills outside a custom agent's whitelist. A custom skill named `context` remains activatable for other task text. - **Installation**: `POST /api/skills/install` extracts .skill ZIP archive to custom/ directory - **Managed integrations**: Lark/Feishu CLI support installs one global official `lark-*` pack as read-only `SkillCategory.INTEGRATION` entries under `/mnt/skills/integrations/lark-cli/...`; enabled flags, app configuration, and OAuth data remain per-user. Install resolves the newest `larksuite/cli` release from GitHub (`releases/latest`) at install time (falling back to a bottom-line pinned version if the lookup fails) rather than hard-coding the pack version; integrity relies on the official host + structural archive guards + a recorded hash of the effective installed tree after shared guidance injection (not a pinned archive-byte SHA, which GitHub does not keep stable). The Gateway image still installs a pinned `@larksuite/cli` binary, so `get_lark_integration_status` surfaces `latest_available_version` and `runtime_version_mismatch` for the UI. AIO installs additionally verify and publish official Linux amd64/arm64 binaries under `{DEER_FLOW_HOME}/integrations/lark-cli/sandbox-cli`, mounted read-only at `/mnt/integrations/lark-cli/runtime`; `/mnt/integrations/lark-cli/config` (app credentials, incl. the long-lived `appSecret`) is mounted **read-only** into the sandbox, its empty `config/locks` subdirectory is over-mounted writable for `lark-cli` coordination files, and `/mnt/integrations/lark-cli/data` (refreshable OAuth tokens) stays writable, all mapping to owner-only per-user directories. **Sandbox trust boundary:** the credential-bearing config and data dirs are still *readable* by arbitrary sandbox processes (the agent's `bash` tool, or code reached via prompt-injection in a tool result), so the app secret and tokens are exposed to sandbox-side code even though they never reach the browser — the read-only config mount only prevents in-sandbox tampering, not read/exfiltration. The sidecar credential-broker (Pattern B, issue #4338) is the fix that removes these plaintext mounts from sandbox execution: set `LARK_CLI_BROKER_IMAGE` on the provisioner (see `docker/lark-cli-broker/`) and the Gateway sends `provision_lark_cli_broker` on sandbox create. The provisioner then runs a `lark-cli-broker` sidecar that owns the per-user `config`/`config/locks`/`data` mounts (mounted into the **sidecar only**, at `/var/lark/{config,config/locks,data}` with only the nested locks mount writable) and serves the `lark-cli` command surface on Pod loopback (`http://127.0.0.1:8788`); a shim init container (`install-shim`) writes a forwarding `lark-cli` into the shared runtime `emptyDir`, so the sandbox gets `DEERFLOW_LARK_BROKER_URL` + a shim on PATH but **no** credential files. The on-PATH `bin/lark-cli` is a `/bin/sh` launcher that resolves a Python 3 interpreter and execs the Python shim body (`bin/lark-cli-shim.py`) beside it, so broker mode does not silently ENOEXEC on a sandbox image without a `#!/usr/bin/env python3`-resolvable interpreter — it fails loudly (exit 127, actionable message) and can be pinned with `DEERFLOW_LARK_BROKER_PYTHON`. The broker runs `lark-cli` in the sidecar's cwd and cannot see the sandbox filesystem, so cwd is intentionally **not** forwarded and file-I/O subcommands relative to the sandbox cwd are unsupported (command surface only). An optional `DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS` denylist (comma-separated command paths, matched in order through interleaved option values, forwarded from the provisioner) lets the broker refuse secret-dumping subcommands before spawning the binary. `lark_cli_env_overlay(broker=True)` therefore omits `LARKSUITE_CLI_CONFIG_DIR`/`DATA_DIR`; `sandbox_lark_broker_active()` caches confirmed provisioner capabilities for admission and fails closed on unknowns; bash overlays use the admitted sandbox's actual mode. Configured broker provisioning rejects direct credential mounts. `DEER_FLOW_LARK_CLI_SANDBOX_RUNTIME_DIR` supplies a validated, symlink-free pre-staged runtime for air-gapped deployments. For the remote provisioner (K8s), the runtime binary is otherwise provisioned by an optional init container + shared `emptyDir` (Pattern A): set `LARK_CLI_INIT_IMAGE` on the provisioner (see `docker/lark-cli-init/`) and the Gateway sends `provision_lark_cli_runtime` on sandbox create once the pack is installed, so remote installs skip the Gateway-side GitHub download entirely. Broker (Pattern B) supersedes the init-container binary (Pattern A) when both images are configured. `get_lark_integration_status(check_runtime=True)` surfaces `sandbox_runtime_mode` (`none` / `gateway-download` / `init-container` / `broker`) and `sandbox_runtime_ready` (remote modes read the provisioner `GET /api/capabilities`: `lark_cli_init_image` / `lark_cli_broker_image`), plus `sandbox_runtime_probed` separating a confirmed probe result from the conservative unprobed fallback (mutation responses carry a probed status built from a credential snapshot taken under the credential lock), so a green UI can't hide a chat-time `lark-cli: command not found`. Cheap status probes are explicitly not live-verified; users authorize or reconnect through the browser device-flow endpoints instead of running terminal commands. -- **SkillScan**: `packages/harness/deerflow/skills/skillscan/` is the native deterministic scanner for `.skill` archives and agent-managed skill writes. It runs offline before the LLM scanner, emits structured findings (`rule_id`, `severity`, `file`, `line`, `message`, `remediation`, redacted `evidence` — category/analyzer are encoded in the `rule_id` prefix), blocks `CRITICAL`, and passes warning findings into `scan_skill_content()`. The moderation adapter must normalize both plain-text responses and LangChain Responses API text blocks before parsing the required JSON decision. `scan_archive_preflight()` / `scan_skill_dir()` are pure sync functions (dispatch off the event loop); `enforce_static_scan()` applies the blocking policy and the `skill_scan.enabled` kill switch. `skills/package_files.py` is the single definition of code files (`scripts/` members, code suffixes, extensionless `#!` files) and executable magic bytes; the installer, export guard, and SkillScan all import it, so do not re-derive either rule locally. Files that fail NUL-free UTF-8 decoding are treated as binaries unless they are code files: interpreters still run those, so they raise `package-undecodable-script` (HIGH) and are analyzed over a lossy decode, keeping one stray byte from hiding a file or downgrading a `CRITICAL` match; one with executable magic skips the text rules, which only misread its string tables. The Python instance-client signal deliberately follows only a one-level, same-scope evidence chain (PR #4265 review): a proven imported constructor bound to a simple name, optional name-to-name alias propagation, rebinding invalidation, and a constructor-supported outbound method or context-manager use; bare canonical-looking names never fall back to module identity. Nested scopes never inherit client handles and inherit only constructor aliases proven stable by a binding-only enclosing-scope prepass. Comprehensions, walrus-bearing statements, annotations, executable expressions inside complex binding targets, unsupported operations, and ambiguous flows produce no finding from this signal; skipped constructs invalidate all names they may bind, while representative false negatives are pinned by `test_python_declared_false_negatives_stay_unreported`. Compound bodies are walked from isolated copies so wrapping code in `if True:` is not a bypass, while copied scope entries, binding-only prepasses, and AST visits consume a deterministic work budget and the walk stops after its first sink. Budget or recursion exhaustion skips only this best-effort signal and retains deterministic findings already collected for the file. Do not add Semgrep/OpenGrep or YAML rule-engine dependencies to the core path; Phase 1 rule specs live in Python constants next to their analyzers in `skillscan/orchestrator.py`. +- **SkillScan**: `packages/harness/deerflow/skills/skillscan/` is the native deterministic scanner for `.skill` archives and agent-managed skill writes. It runs offline before the LLM scanner, emits structured findings (`rule_id`, `severity`, `file`, `line`, `message`, `remediation`, redacted `evidence` — category/analyzer are encoded in the `rule_id` prefix), blocks `CRITICAL`, and passes warning findings into `scan_skill_content()`. The moderation adapter must normalize both plain-text responses and LangChain Responses API text blocks before parsing the required JSON decision. `scan_archive_preflight()` / `scan_skill_dir()` are pure sync functions (dispatch off the event loop); `enforce_static_scan()` applies the blocking policy and the `skill_scan.enabled` kill switch. `skills/package_files.py` is the single definition of code files (`scripts/` members, code suffixes, extensionless `#!` files) and executable magic bytes; the installer, export guard, and SkillScan all import it, so do not re-derive either rule locally. Files that fail NUL-free UTF-8 decoding are treated as binaries unless they are code files: interpreters still run those, so they raise `package-undecodable-script` (HIGH) and are analyzed over a lossy decode, keeping one stray byte from hiding a file or downgrading a `CRITICAL` match; one with executable magic skips the text rules, which only misread its string tables. The Python instance-client signal deliberately follows only a one-level, same-scope evidence chain (PR #4265 review): a proven imported constructor bound to a simple name, optional name-to-name alias propagation, rebinding invalidation, and a constructor-supported outbound method or context-manager use; bare canonical-looking names never fall back to module identity. Nested scopes never inherit client handles and inherit only constructor aliases proven stable by a binding-only enclosing-scope prepass. Comprehensions, walrus-bearing statements, annotations, executable expressions inside complex binding targets, unsupported operations, and flows whose order cannot be decided produce no finding from this signal; representative false negatives are pinned by `test_python_declared_false_negatives_stay_unreported`. Compound bodies are walked from isolated copies so wrapping code in `if True:` is not a bypass: a part that cannot follow a sibling's rebinding (an `if` branch, the one `match` case, a `try` or loop body) applies its own bindings in source order, while a part that can (a loop or `try` `else`, a handler, a `finally`) starts with those names dropped; dispatch-time bindings (a loop target, `except ... as`, a `match` capture) are always dropped first, and nothing bound inside a body reaches the code after the statement. Copied scope entries, binding-only prepasses, and AST visits consume a deterministic work budget and the walk stops after its first sink. Budget or recursion exhaustion skips only this best-effort signal and retains deterministic findings already collected for the file. Do not add Semgrep/OpenGrep or YAML rule-engine dependencies to the core path; Phase 1 rule specs live in Python constants next to their analyzers in `skillscan/orchestrator.py`. - **SkillScan HTTP hosts**: recognize HTTP/HTTPS schemes case-insensitively in both URL regexes and prefix checks, without changing source evidence. Use URL authority parsing for lowercase hosts and bracketed IPv6 loopback; userinfo must never hide an external host. Malformed URLs remain outbound for exfiltration checks. Cleartext HTTP detection covers IPv6 literals; cloud-metadata hostname matching is case-insensitive. Coverage: `tests/test_skillscan_native.py`. - **Python secret mappings**: `secret-env-assignment` scans literal container values and conditional branches under secret-like bindings. Dictionary keys are structural labels and contribute candidates only when their folded literal matches the shared `_SECRET_TOKEN_PATTERNS`; they must not inherit credential evidence from the enclosing variable name. Opaque keys without a recognized token format stay unreported. Regressions in `tests/test_skillscan_native.py` cover runtime-loaded mappings, nested labels, credential-value locations, and literal/concatenated token keys; `tests/test_skill_review_core.py` pins the CI CLI gate. - **Skill Review Core**: `packages/harness/deerflow/skills/review/` provides read-only package snapshots, deterministic facts, resource/eval analysis, report rendering, and the CLI (`python -m deerflow.skills.review.cli`). It reuses the shared frontmatter helper and SkillScan; it must not import `app.*`, execute target scripts, install dependencies, or call networks. JSON contracts live in `contracts/skill_review/`. The analyzer hands SkillScan a byte-faithful copy of every non-symlink snapshot file (binary entries carry `content_base64`), so package rules such as `package-executable-binary` cover binaries; only eval-fixture `SKILL.md` samples are withheld, while scripts and other files under `evals/fixtures/` are scanned. Materialization uses exclusive create, so a duplicate archive member or a case-folded name fails the scan closed (`not_assessed: skillscan`) instead of overwriting an earlier file, as does an entry without bytes in an untruncated snapshot. The `review_skill_package` built-in tool labels results with `review_subject_entry` and never `skill_context_entry`, so reviewing a target does not activate it or bind its `required-secrets`. Its model-visible `ToolMessage.content` is a compact JSON payload with untrusted control tags neutralized; the full raw review payload, including Markdown renders, stays in `ToolMessage.artifact`. Local path targets are host paths, so the tool confines them to the configured skills root and the caller's own `{base_dir}/users/{user_id}/`; never re-add the Gateway cwd or `/tmp`, because `DEER_FLOW_HOME` sits under the cwd and those roots expose other users' skills. A skills root that contains `{base_dir}/users/` is dropped with a warning. CI should run the CLI with `--fail-on error --fail-on-incomplete` so blocker/error findings and truncated/not-assessed packages fail the gate. The public `skills/public/skill-reviewer` skill owns semantic readiness review and suggestions only; mutation and runtime experiments remain owned by `skill-creator`. diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 36e1e7ab92..22ed7ec585 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -1565,7 +1565,12 @@ def _call_is_network_sink(call_name: str) -> bool: # narrowing the model has to change that test rather than change behaviour silently. # # Compound bodies are still walked from isolated entry-state copies so `if True:` is not a -# universal bypass, but ambiguous bindings are dropped rather than joined. Every AST visit +# universal bypass. A part that cannot follow a sibling's binding -- an `if` branch, the one +# `match` case that runs, a `try` or loop body -- keeps the enclosing handle live and applies its +# own rebinding in source order; parts that can follow a sibling (a loop or `try` `else`, a +# handler, a `finally`) still start with those names dropped, so a rebind on one path cannot be +# read as live on the next. Names the statement binds as it dispatches -- a loop target, an +# `except ... as` name, a `match` capture -- are always dropped first. Every AST visit # and copied scope entry consumes a deterministic work budget, and the walk stops as soon # as it finds one sink. This bounds the branch-copy cost on untrusted source. @@ -1734,19 +1739,31 @@ def _walk_client_scope(node: ast.AST, scope: _ClientScope, inherited: _ClientSco def _walk_client_branching(node: ast.AST, scope: _ClientScope, inherited: _ClientScope, analysis: _ClientAnalysis) -> None: - """Analyze a compound statement without deciding which of its parts run, or in what order. + """Analyze a compound statement part by part, dropping only the bindings that can already be stale. - Every name the statement may bind is dropped *before* any body is walked, not after. Dropping - afterwards would be wrong for the parts that run after a sibling has already rebound the name -- - a `finally` after a handler replaced the handle, a later `except*` clause, a second loop - iteration -- and each of those disagreements is a benign file hard-blocked as `CRITICAL`. - Ordering the parts instead of dropping is the control-flow interpreter this signal is not. + A name is dropped before a part is walked when some *other* part of the same statement can run + before it and rebind that name: an `else` after a loop body, a `finally` after a handler replaced + the handle, a later `except*` clause. Reading such an earlier path's rebind as still live is a + benign file hard-blocked as `CRITICAL`. - Bodies are still walked, each from its own copy, so a construction inside one branch cannot leak - into a sibling and a sink inside a branch is still seen. What is lost is a name that the same - statement both calls and rebinds; that is the documented false negative. + A part that cannot follow a sibling -- a branch of an `if`/`elif`/`else` chain, the one `match` + case that runs, a `try` body, a loop body -- is walked with the enclosing handle intact, and the + part then applies its own rebinding in source order. That is the rule the top level already uses, + where a rebind after a call cannot retract it, and it is what stops a call that really happened + from disappearing because a later statement in that same part rebinds the name (issue #4296, + case 1). + + Names the statement binds as it dispatches -- a loop target, an `except ... as` name, a `match` + capture -- are dropped after the header is walked and before every part, because the header is + evaluated while those bindings do not exist yet: a sink in `for s in s.post(...)` really happens + under the enclosing handle. Everything the statement may bind is then dropped from the enclosing + scope once its parts are walked, so a binding made inside a part never reaches the code that + follows the statement. """ - _drop_client_bindings(scope, _branching_bound_names(node, analysis)) + entry_names = _branching_entry_bound_names(node, analysis) + # Names rebound by ordinary statements inside the parts: which part ran is undecided here, so they + # stay dropped for the parts that can follow a sibling and live for the parts that cannot. + sibling_names = _branching_bound_names(node, analysis) - entry_names for header in _branching_header_exprs(node): walrus_names = _walrus_target_names(header, analysis) if walrus_names: @@ -1755,11 +1772,13 @@ def _walk_client_branching(node: ast.AST, scope: _ClientScope, inherited: _Clien _walk_client_scope(header, scope, inherited, analysis) if analysis.found is not None: return - for body in _branching_bodies(node): - branch_scope = scope.copy_without(analysis) + _drop_client_bindings(scope, entry_names) + for body, follows_sibling in _branching_parts(node): + branch_scope = scope.copy_without(analysis, sibling_names if follows_sibling else None) _walk_client_statements(body, branch_scope, inherited, analysis) if analysis.found is not None: return + _drop_client_bindings(scope, entry_names | sibling_names) def _branching_header_exprs(node: ast.AST) -> list[ast.AST]: @@ -1774,22 +1793,56 @@ def _branching_header_exprs(node: ast.AST) -> list[ast.AST]: return [] # `try` has no header; handler types run only when an exception was raised -def _branching_bodies(node: ast.AST) -> list[list[ast.AST]]: +def _branching_parts(node: ast.AST) -> list[tuple[list[ast.AST], bool]]: + """The statement's parts, each tagged with whether a sibling part can run before it. + + `False` marks a part that starts from the state at the statement itself: the branches of an + `if`/`elif`/`else` chain are mutually exclusive, only one `match` case runs, and a `try` body or a + loop body is the first thing its statement runs. A binding made in some other part cannot be the + value this part starts from, so an enclosing handle stays live here. + + `True` marks a part that can follow a sibling's binding: a loop or `try` `else`, a handler after + its `try` body, a later `except*` clause, and a `finally` after any of them. + """ if isinstance(node, (ast.Try, ast.TryStar)): # A handler's `type` expression and its body run on the same path, so they share one copy. handlers = [[*([handler.type] if handler.type is not None else []), *handler.body] for handler in node.handlers] - return [node.body, *handlers, node.orelse, node.finalbody] + return [(node.body, False), *((body, True) for body in [*handlers, node.orelse, node.finalbody])] if isinstance(node, ast.Match): - return [[*([case.guard] if case.guard is not None else []), *case.body] for case in node.cases] + return [([*([case.guard] if case.guard is not None else []), *case.body], False) for case in node.cases] if isinstance(node, ast.If): - return [node.body, node.orelse] + return [(node.body, False), (node.orelse, False)] if isinstance(node, (ast.For, ast.AsyncFor)): - return [node.body, node.orelse] + return [(node.body, False), (node.orelse, True)] if isinstance(node, ast.While): - return [node.body, node.orelse] + return [(node.body, False), (node.orelse, True)] return [] +def _branching_bodies(node: ast.AST) -> list[list[ast.AST]]: + return [body for body, _follows_sibling in _branching_parts(node)] + + +def _branching_entry_bound_names(node: ast.AST, analysis: _ClientAnalysis) -> set[str]: + """Names the statement binds as it dispatches, never from a statement inside one of its parts. + + These are not order-dependent: a loop target, an exception name and a `match` capture hold a new + value before any body of the statement runs, so an enclosing handle under the same name is already + gone for every part walked below. Dropping them here rather than with the rest is what lets a + later sink in the same part still be seen. + """ + names: set[str] = set() + declared: set[str] = set() + if isinstance(node, (ast.For, ast.AsyncFor)): + names.update(_client_assignment_target_names(node.target)) + if isinstance(node, (ast.Try, ast.TryStar)): + names.update(handler.name for handler in node.handlers if handler.name) + if isinstance(node, ast.Match): + for case in node.cases: + _collect_client_scope_bindings(case.pattern, names, declared, analysis) + return names - declared + + def _branching_bound_names(node: ast.AST, analysis: _ClientAnalysis) -> set[str]: """Every name the statement may bind, including the loop/handler/capture targets themselves.""" names: set[str] = set() diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index 2f5950cf27..ab5149e406 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -1535,6 +1535,41 @@ def test_python_client_handle_binding_matches_runtime(tmp_path: Path, source: st # treating both as executed is exactly the over-reporting that hard-blocks benign files. The # prelude takes the `else` path, so the runtime shows which receiver really answers. ("flag = False\ns = config\n", "import os\nimport requests\n\nif flag:\n s = requests.Session()\nelse:\n s.post(host, json=dict(os.environ))\n", ["config"], False), + # Issue #4296 case 1, now reported: a part that cannot follow a sibling keeps the enclosing + # handle, and the rebind on the later line cannot retract the call already made on the first. + ("flag = True\n", "import os\nimport requests\n\ns = requests.Session()\nif flag:\n s.post(host, json=dict(os.environ))\n s = config\n", ["client"], True), + # The same two statements in the other order really are benign, so the case above is not + # passing because an `if` body is walked without ordering its bindings. + ("flag = True\n", "import os\nimport requests\n\ns = requests.Session()\nif flag:\n s = config\n s.post(host, json=dict(os.environ))\n", ["config"], False), + # A loop body is the first thing its statement runs, so the same rule applies there. + ("", "import os\nimport requests\n\ns = requests.Session()\nfor _ in [1]:\n s.post(host, json=dict(os.environ))\n s = config\n", ["client"], True), + # The loop `else` is different: it always follows the body, so a handle the body replaced must + # not be read as live there. + ("", "import os\nimport requests\n\ns = requests.Session()\nfor _ in [1]:\n s = config\nelse:\n s.post(host, json=dict(os.environ))\n", ["config"], False), + # A `try` body starts from the state at the statement, and a `finally` does not. + ("", "import os\nimport requests\n\ns = requests.Session()\ntry:\n s.post(host, json=dict(os.environ))\n s = config\nexcept Exception:\n pass\n", ["client"], True), + ("", "import os\nimport requests\n\ns = requests.Session()\ntry:\n s = config\nfinally:\n s.post(host, json=dict(os.environ))\n", ["config"], False), + # Exactly one `match` case runs, so a case body cannot follow another case's binding. + ("value = 1\n", "import os\nimport requests\n\ns = requests.Session()\nmatch value:\n case 1:\n s.post(host, json=dict(os.environ))\n s = config\n", ["client"], True), + # A capture target is not a sibling binding: it rebinds the name as the statement dispatches, + # so the enclosing handle is already gone before the matched body runs. + ("value = [1]\n", "import os\nimport requests\n\ns = requests.Session()\nmatch value:\n case [s]:\n s.post(host, json=dict(os.environ))\n", [], False), + # A handler's `as` name is the same case: it shadows the handle for its own body. + ("", "import os\nimport requests\n\ns = requests.Session()\ntry:\n float('x')\nexcept Exception as s:\n s.post(host, json=dict(os.environ))\n", [], False), + # The header is evaluated *before* the dispatch-time binding exists, so a sink there runs + # under the enclosing handle even when the target/capture reuses that name. + ("", "import os\nimport requests\n\ns = requests.Session()\nfor s in s.post(host, json=dict(os.environ)):\n pass\n", ["client"], True), + ("", "import os\nimport requests\n\ns = requests.Session()\nmatch s.post(host, json=dict(os.environ)):\n case [s]:\n pass\n", ["client"], True), + # ... and the inverse, which is why the dispatch names are still dropped before the body: + # inside the body `s` is the loop value, not the handle. + ("", "import os\nimport requests\n\ns = requests.Session()\nfor s in [1]:\n s.post(host, json=dict(os.environ))\n", [], False), + # Two levels of branching: the branches are mutually exclusive, so a rebind in the `if` body + # cannot retract a sink in the `elif` body. + ("a = False\nb = True\n", "import os\nimport requests\n\ns = requests.Session()\nif a:\n s = config\nelif b:\n s.post(host, json=dict(os.environ))\n", ["client"], True), + # A `while` body is the first thing its statement runs, same rule as `for`. + ("flag = True\n", "import os\nimport requests\n\ns = requests.Session()\nwhile flag:\n s.post(host, json=dict(os.environ))\n flag = False\n s = config\n", ["client"], True), + # A `try` `else` always follows the body, so the body's rebind must reach it. + ("", "import os\nimport requests\n\ns = requests.Session()\ntry:\n s = config\nexcept Exception:\n pass\nelse:\n s.post(host, json=dict(os.environ))\n", ["config"], False), ], ) def test_python_branch_bodies_are_walked_without_leaking_bindings(tmp_path: Path, prelude: str, source: str, receivers: list[str], is_exfil: bool) -> None: @@ -1557,9 +1592,6 @@ def test_python_import_over_a_live_handle_drops_it(tmp_path: Path) -> None: @pytest.mark.parametrize( "source", [ - # A name the compound statement both calls and rebinds. Which value survives depends on the - # path taken, so the handle is dropped rather than resolved. - "import os\nimport requests\n\ns = requests.Session()\nif flag:\n s.post(host, json=dict(os.environ))\n s = config\n", # A construction on a branch that really runs, observed after the statement. "import os\nimport requests\n\nif flag:\n s = requests.Session()\ns.post(host, json=dict(os.environ))\n", # A walrus that really executes: whether and when it runs is undecidable in general, so the