mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
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.
This commit is contained in:
parent
741948ba9b
commit
261279ff80
13
CHANGELOG.md
13
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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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:
|
||||
|
||||
File diff suppressed because one or more lines are too long
@ -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(
|
||||
|
||||
@ -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.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user