From 261279ff802dcf0efb73374e93203b94daad69fb Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Sat, 3 Oct 2026 16:58:01 +0900 Subject: [PATCH] fix(lark): match broker subcommand denylist through separate option values (#6212) * fix(lark): match broker subcommand denylist through separate option values The opt-in DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS check dropped only `-`-prefixed tokens and compared the rest from the start, so the value in `--profile work config show` became the leading positional and a `config show` rule never matched. Real lark-cli 1.0.65 runs `config show` for that shape and for `config --profile work show`. The broker cannot know which options take a value, so a rule now matches when its tokens appear in order among the non-flag tokens. A contiguous match would still miss a value placed between the path tokens. Argument values that spell a denied path in order are refused too (fail-closed). * docs(changelog): reference #6212 for the lark broker denylist fix * docs(lark): call denylist entries command paths, not prefixes Entries are matched as in-order subsequences now, so the remaining 'prefix' wording in the BrokerConfig comment, the parse_deny_subcommands and run_lark_cli docstrings, and the broker README undersold the rule. Also rewrap the comment and docstring lines this PR had pushed past ~80 columns. --- CHANGELOG.md | 13 +++++ CHANGELOG_zh.md | 11 ++++ .../deerflow/integrations/lark_broker.py | 45 ++++++++++------ .../harness/deerflow/skills/AGENTS.md | 2 +- backend/tests/test_lark_broker.py | 53 +++++++++++++++++++ docker/lark-cli-broker/README.md | 13 +++-- 6 files changed, 114 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1049706aa..f42d2beec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2760,6 +2760,18 @@ This release closes that milestone with **301 merged pull requests**. stamped `skill_context_denied`, so durable context, skill allowed-tools, and autonomous secret bindings never activate the denied skill. ([#4541]) +- **lark:** The opt-in Lark broker subcommand denylist + (`DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS`) can no longer be bypassed by an + option value passed as its own token. Matching dropped only `-`-prefixed + tokens and compared the rest from the start, so the `work` in `--profile work + config show` became the leading positional and a `config show` rule never + matched — real `lark-cli` 1.0.65 still runs `config show` there. The broker + cannot know which options take a value, so a rule now matches when its tokens + appear in order among the non-flag tokens — which also catches values placed + between them (`config --profile work show`), a case a contiguous match would + still miss. Argument values that spell a denied path in order are refused too + (fail-closed). ([#6212]) + ### Documentation - **docs:** Fix the Apple Container verification instructions. The guide @@ -7629,5 +7641,6 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6140]: https://github.com/bytedance/deer-flow/pull/6140 [#6201]: https://github.com/bytedance/deer-flow/pull/6201 [#6202]: https://github.com/bytedance/deer-flow/pull/6202 +[#6212]: https://github.com/bytedance/deer-flow/pull/6212 [#6171]: https://github.com/bytedance/deer-flow/pull/6171 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 28263aff54..00ebf900b3 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -2337,6 +2337,16 @@ `read_file` 读取 `SKILL.md` 时会打上 `skill_context_denied` 标记，持久上 下文、技能 allowed-tools 与自主密钥绑定都不会激活被拒绝的技能。([#4541]) +- **Lark：** 可选的 Lark broker 子命令拒绝列表 + （`DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS`）不再能被以独立 token 传入的选项 + 值绕过。此前匹配只去掉以 `-` 开头的 token 并从头比较剩余部分，因此 + `--profile work config show` 中的 `work` 成为首个位置参数，`config show` + 规则永远匹配不上——而真实的 `lark-cli` 1.0.65 在这种写法下仍会执行 + `config show`。broker 无法得知哪些选项带值，因此规则现在只要其 token 按顺序 + 出现在非选项 token 中即视为匹配——这也覆盖了值夹在中间的情形 + （`config --profile work show`），而连续匹配仍会漏掉这种情况。参数值恰好按 + 顺序拼出被拒绝路径的调用也会被拒绝（fail-closed）。([#6212]) + ### 文档 - **文档：** 修正 Apple Container 的验证说明。 @@ -6376,3 +6386,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#6171]: https://github.com/bytedance/deer-flow/pull/6171 [#6201]: https://github.com/bytedance/deer-flow/pull/6201 [#6202]: https://github.com/bytedance/deer-flow/pull/6202 +[#6212]: https://github.com/bytedance/deer-flow/pull/6212 diff --git a/backend/packages/harness/deerflow/integrations/lark_broker.py b/backend/packages/harness/deerflow/integrations/lark_broker.py index 1eb40d73c6..c3a5fb5156 100644 --- a/backend/packages/harness/deerflow/integrations/lark_broker.py +++ b/backend/packages/harness/deerflow/integrations/lark_broker.py @@ -249,12 +249,13 @@ class BrokerConfig: port: int = LARK_BROKER_DEFAULT_PORT timeout_seconds: int = LARK_BROKER_DEFAULT_TIMEOUT_SECONDS # Opt-in denylist of ``lark-cli`` subcommand paths the broker refuses to run - # (issue #4338 hardening). Each entry is a space-joined command prefix, e.g. - # "config show" or "auth token", matched against the leading non-flag tokens - # of the request. Narrows the command surface a prompt-injected agent can - # reach — the broker already removes the credential *files*, but the full - # command surface stays reachable unless a secret-dumping subcommand is denied - # here. Empty by default (no behavior change). + # (issue #4338 hardening). Each entry is a space-joined command path, e.g. + # "config show" or "auth token", matched in order against the non-flag + # tokens of the request (see ``_denied_subcommand``). Narrows the command + # surface a prompt-injected agent can reach — the broker already removes the + # credential *files*, but the full command surface stays reachable unless a + # secret-dumping subcommand is denied here. Empty by default (no behavior + # change). deny_subcommands: tuple[tuple[str, ...], ...] = () def credential_env(self) -> dict[str, str]: @@ -272,33 +273,42 @@ class BrokerConfig: def parse_deny_subcommands(raw: str | None) -> tuple[tuple[str, ...], ...]: - """Parse the comma-separated denylist env into command-prefix tuples. + """Parse the comma-separated denylist env into command-path tuples. ``"config show, auth token"`` → ``(("config", "show"), ("auth", "token"))``. Blank/whitespace-only entries are dropped. """ if not raw: return () - prefixes: list[tuple[str, ...]] = [] + paths: list[tuple[str, ...]] = [] for entry in raw.split(","): tokens = tuple(entry.split()) if tokens: - prefixes.append(tokens) - return tuple(prefixes) + paths.append(tokens) + return tuple(paths) def _denied_subcommand(deny: tuple[tuple[str, ...], ...], args: list[str]) -> tuple[str, ...] | None: - """Return the matched denylist prefix if ``args`` is a denied subcommand. + """Return the matched denylist rule if ``args`` may run a denied subcommand. - Matches against the leading non-flag tokens (options and their values are - skipped) so ``config --json show`` is still caught by a ``config show`` rule. + The broker cannot know which ``lark-cli`` options take a value, so a value + passed as its own token (``--profile work``) is indistinguishable from a + subcommand name and may sit before or between the command-path tokens. A + rule therefore matches when its tokens appear *in order* among the non-flag + tokens, with anything in between: the command path the CLI resolves is + always such a subsequence, so ``--profile work config show`` and + ``config --profile work show`` are both caught by a ``config show`` rule. + The cost is a fail-closed refusal when argument values happen to spell a + denied path in order. """ if not deny: return None positional = [token for token in args if not token.startswith("-")] - for prefix in deny: - if positional[: len(prefix)] == list(prefix): - return prefix + for rule in deny: + # ``in`` advances the shared iterator: an ordered-subsequence test. + remaining = iter(positional) + if all(token in remaining for token in rule): + return rule return None @@ -315,7 +325,8 @@ def run_lark_cli(config: BrokerConfig, args: list[str], stdin: bytes) -> ExecRes ``args`` is passed as an argv list with ``shell=False`` so a sandbox-supplied argument can never be shell-interpreted into a second command. A configured - ``deny_subcommands`` prefix is refused before the binary is ever spawned. + ``deny_subcommands`` command path is refused before the binary is ever + spawned. """ denied = _denied_subcommand(config.deny_subcommands, args) if denied is not None: diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 7fe6e2165e..91cbb066a5 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -13,7 +13,7 @@ - `skills/describe.py` — `build_describe_skill_tool(catalog)` builds the `describe_skill` tool as a closure; `build_skill_search_setup(skills, enabled, ...)` produces a `SkillSearchSetup(describe_skill_tool, skill_names)` that is wired into both the LangGraph agent factory (`agent.py`) and the embedded client (`client.py`). - **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 prefixes, 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()` (TTL-cached provisioner `/api/capabilities` probe, tight timeout + longer negative caching on the bash hot path) selects broker vs. binary mode for both the bash env overlay and status. `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. +- **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()` (TTL-cached provisioner `/api/capabilities` probe, tight timeout + longer negative caching on the bash hot path) selects broker vs. binary mode for both the bash env overlay and status. `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`. - **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, bind its `required-secrets`, or apply its `allowed-tools`. 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`. 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/tests/test_lark_broker.py b/backend/tests/test_lark_broker.py index 48fbab4bad..b332643e45 100644 --- a/backend/tests/test_lark_broker.py +++ b/backend/tests/test_lark_broker.py @@ -623,6 +623,59 @@ def test_denied_subcommand_matches_through_leading_flags(tmp_path: Path) -> None assert result.exit_code == 126 +@pytest.mark.parametrize( + "args", + [ + ["--profile", "work", "config", "show"], + ["config", "--profile", "work", "show"], + ["--profile", "work", "config", "--profile", "work", "show", "--json"], + ], +) +def test_denied_subcommand_matches_through_separate_option_values(tmp_path: Path, args: list[str]) -> None: + # ``--profile`` is lark-cli's root value flag; lark-cli 1.0.65 runs + # ``config show`` for every shape above. The broker cannot tell which + # options take a value, so a value passed as its own token must not hide the + # denied path, whether it sits before the path or between its tokens. + config = BrokerConfig( + lark_cli_path=_fake_lark_cli(tmp_path), + config_dir="c", + data_dir="d", + deny_subcommands=(("config", "show"),), + ) + result = run_lark_cli(config, args, b"") + assert result.exit_code == 126 + assert result.stdout == b"" + + +def test_denied_subcommand_requires_rule_tokens_in_order() -> None: + deny = (("config", "show"),) + assert lark_broker._denied_subcommand(deny, ["show", "config"]) is None + assert lark_broker._denied_subcommand(deny, ["config", "list"]) is None + + +def test_exec_endpoint_refuses_denied_subcommand_behind_option_value(tmp_path: Path) -> None: + config = BrokerConfig( + lark_cli_path=_fake_lark_cli(tmp_path), + config_dir="c", + data_dir="d", + port=0, + deny_subcommands=(("config", "show"),), + ) + server = serve(config) + import threading + + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + status, body = _post_exec(*server.server_address[:2], {"args": ["--profile", "work", "config", "show"]}) + finally: + server.shutdown() + thread.join(timeout=5) + assert status == 200 + assert body["exit_code"] == 126 + assert body["stdout_b64"] == "" + + @_skip_windows def test_allowed_subcommand_still_runs_with_denylist(tmp_path: Path) -> None: config = BrokerConfig( diff --git a/docker/lark-cli-broker/README.md b/docker/lark-cli-broker/README.md index 950102acb5..c409494b66 100644 --- a/docker/lark-cli-broker/README.md +++ b/docker/lark-cli-broker/README.md @@ -129,10 +129,13 @@ paths still refer to the sidecar's filesystem, not the sandbox's. The broker removes the credential *files* from the sandbox, but the full `lark-cli` command surface stays reachable, so any subcommand that prints/exports tokens could still exfiltrate them. Set `DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS` -on the sidecar to a comma-separated list of command prefixes the broker should -refuse (matched against the leading non-flag tokens), e.g. -`DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS="config show, auth token"`. Denied calls -return exit `126` with a `subcommand ... is disabled` message and never spawn the -binary. Empty by default (no behavior change); confirm the deployed `lark-cli` +on the sidecar to a comma-separated list of command paths the broker should +refuse, e.g. `DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS="config show, auth token"`. +Denied calls return exit `126` with a `subcommand ... is disabled` message and +never spawn the binary. The broker does not know which options take a value, so +a rule matches when its tokens appear in order among the request's non-flag +tokens, even with other tokens in between: `--profile work config show` and +`config --profile work show` are both refused by `config show`. This fails closed — a +call whose argument values spell a denied path in order is refused too. Empty by default (no behavior change); confirm the deployed `lark-cli` version's subcommand surface has no trivial secret-dump command before enabling broker mode in production.