From 7cf07763ba0b1f8421e6a649f8e22bc4d58e21ce Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Tue, 4 Aug 2026 00:34:59 +0530 Subject: [PATCH] Close the Slack gate's flag-repetition bypass, and stop a NUL byte from silencing the bot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in `grapharc/slack/command.py`, the module that decides what text from a Slack workspace may become an argv. **Typing a flag twice walked past the agent opt-in.** The gate admits a command by reading a flag's value; argparse's `store` action then runs the *last* occurrence, and `_flag_value` returned the *first*. So the gate and the CLI read different values out of the same command line: plan 'ship it' --model openrouter/x/y \ --registry grapharc.examples.plan_docs:build_registry \ --registry grapharc.stdlib:build_registry was judged against the demo registry — which needs no opt-in — and executed against `grapharc.stdlib:build_registry`, which builds agent kinds that run tools on the host under an executor that, in its own words, "confines nothing". `GRAPHARC_SLACK_ALLOW_AGENT` was never consulted, and the `--approve` injection that would have put a human in front of the run was skipped in the same step, so nothing downstream caught it either. The single-flag form was refused correctly the whole time; the exploit was the second `--registry`, which needs no privilege and no special knowledge. Repeats of any admitted flag are refused outright now. That is the fail-closed reading and it retires the whole first-vs-last family rather than the one flag that exposed it: a Slack command has no legitimate reason to pass `--registry` or `--model` twice, and a gate that must choose which of two occurrences to believe is a gate that can be wrong. The carve-out is `repeatable_flags`, the options the CLI itself accumulates (`agent --allow`/`--deny`, argparse `action="append"`), where every occurrence reaches the run and there is no other value to diverge from. A duplicated `--model` falls to the same rule, opted in or not, so it cannot smuggle a backend past the spend gate. `_flag_value` reads the last occurrence regardless — the cheap half of the belt. With repeats refused there is only ever one, but a future caller assembling an argv some other way should not be able to reopen the gap. A sweep over the whole allowlist asserts the duplicated form of every gated flag, so a gate added later inherits the property instead of having to remember it. **A NUL byte in a path came back as silence.** `Path(raw).resolve()` raises `ValueError`, and `handle_text_live` catches only `SlackCommandError`, so `trace a\x00b` escaped the bolt listener as an unhandled exception and the requester got no reply at all — the one answer a chat bot must never give, since it is indistinguishable from the bot being down. A NUL anywhere in the request is a refusal now, in the same voice the core tools already use ("cannot name a file"), and `_confined` turns any `ValueError`/`OSError` out of the filesystem into a refusal too, keeping the guarantee for callers of its own. Folded in from the same report: the flag allowlist tested `token.startswith("--")`, so a single-dash token slipped it and was spent as a positional — `trace -h` was admitted with `-h` as the path. The allowlist is meant to be exhaustive; any leading dash is a flag now, and one not on the list is refused like any other. Fixes #61 Fixes #64 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 + docs/cookbook/07-slack.md | 1 + grapharc/slack/command.py | 82 ++++++++++++++++++++++-- tests/test_slack_gateway.py | 122 ++++++++++++++++++++++++++++++++++++ 4 files changed, 201 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3cad53..d274ccd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,3 +16,5 @@ Entries are newest-last within a release, matching the order they were written. - a bracket anywhere in a model's **prose** hijacked JSON extraction, because only the first `{`/`[` was ever tried. `Based on the context [lines 3-5]: {…}` was rejected as unparseable, and — worse — `Analysis (note [1]): {"supported": false}` returned a perfectly valid `[1]`, substituting a fabricated value for the verifier's actual answer. Every opener is tried now, and length alone turned out not to be a safe rank — a citation list like `[101, 205, 309, …]` *longer* than the verdict still won — so object spans are tried before array spans, each longest-first; junk still returns `None`, so fail-closed is unchanged. - a **bare backend name was read as a model name**, because `split_spec` only consulted the backend list when the spec contained a slash. `--model claude-cli` — the backend `models --check` reports as `usable` — shelled out to `claude -p --model claude-cli` and was refused by the CLI on *every* call, and `--model mock` named the paid subscription backend and spawned the real binary, so the double documented as "never reaches a provider" reached for one. A bare backend name now resolves to that backend (`claude-cli` to its own default model, `mock` to the scripted double, which ignores the model segment anyway); `openrouter`, `openai` and `ollama` front catalogues rather than a model, so those are refused with an example spelling instead of a guess about what to bill you for. The slash forms and bare *model* names are unchanged. - a failing `claude -p` **reported no reason at all**. The CLI exits non-zero with an empty stderr and puts its explanation in the JSON envelope on stdout; the gateway read only stderr, so the error was `claude -p exited 1: ` — a sentence that stops at the colon. Since the wrong-model bug above presented itself exactly that way, the two hid each other. stdout is read first now, and the recovered text also feeds the transient-vs-deterministic classifier, which was previously deciding from `""`. +- **repeating `--registry` walked a Slack user straight past the agent opt-in.** The gate reads a flag's value to decide admission and argparse's `store` action then runs the *last* occurrence, but `_flag_value` returned the *first* — so `plan … --registry grapharc.examples.plan_docs:build_registry --registry grapharc.stdlib:build_registry` was judged against the demo registry and executed against the one that builds agent kinds on the host, with `GRAPHARC_SLACK_ALLOW_AGENT` never consulted and the forced `--approve` skipped in the same step. No privilege and no special knowledge needed: typing the flag twice was the whole exploit. Repeats of any admitted flag are refused outright now — the fail-closed reading, which retires the entire first-vs-last family rather than the one flag that exposed it — with a carve-out for the options the CLI itself accumulates (`agent --allow`/`--deny`, argparse `action="append"`), where every occurrence reaches the run and nothing can diverge. A duplicated `--model` is refused on the same rule, opted in or not, and `_flag_value` reads the last occurrence regardless, so the two readers can no longer disagree. A sweep over the whole allowlist asserts the duplicated form of every gated flag, so a future gate cannot reopen the gap. +- a **NUL byte in a path came back as silence**, the worst answer a chat bot can give: `Path(raw).resolve()` raises `ValueError`, `handle_text_live` catches only `SlackCommandError`, so `trace a\x00b` escaped the bolt listener as an unhandled exception and the requester saw no reply at all — indistinguishable from the bot being down. A NUL anywhere in the request is now a refusal in the same voice the core tools already use ("cannot name a file"), and `_confined` turns any `ValueError`/`OSError` out of the filesystem into a refusal too, for callers of its own. Folded in from the same report: the flag allowlist tested `token.startswith("--")`, so a single-dash token slipped it and was spent as a positional — `trace -h` was admitted with `-h` as the path. Any leading dash is a flag now, and one not on the list is refused like any other. diff --git a/docs/cookbook/07-slack.md b/docs/cookbook/07-slack.md index 38cc625..8412280 100644 --- a/docs/cookbook/07-slack.md +++ b/docs/cookbook/07-slack.md @@ -27,6 +27,7 @@ an afterthought. The defaults: | The budget, policy and trace flags each command already has | `--registry` (imports an arbitrary module), `--config`, `--json`, `--no-color` | | `plan --registry`, for exactly the two registries the package ships | any other `--registry` value | | `agent`, only behind the double opt-in below | `--model` / `--reviewer-model`, unless the operator opts in | +| Each admitted flag, once; `agent --allow`/`--deny` accumulate as the CLI does | The same flag twice (`--registry --registry `), because the gate would judge one occurrence and the CLI would run the other | With `--model` off, every reachable command runs the scripted, spend-free path. The default answer to "can someone in Slack cost me money?" is **no**; diff --git a/grapharc/slack/command.py b/grapharc/slack/command.py index 11fef50..c594728 100644 --- a/grapharc/slack/command.py +++ b/grapharc/slack/command.py @@ -19,6 +19,16 @@ - **`--model` is refused unless the operator opted in**, because it reaches a paid backend. Without it every allowed command runs the scripted, spend-free path; the default answer to "can Slack cost me money?" is no. +- **A flag may not be repeated.** The gate admits a command by reading a flag's + value, and argparse's `store` action then runs the *last* occurrence — so any + reader that takes a different one is a bypass, and `--registry + --registry ` was exactly that: admitted against the benign value, + executed against the agent registry, with the forced `--approve` skipped in + the same step. Refusing the repeat is the fail-closed reading and it closes + the whole first-vs-last family at once, rather than the one flag that showed + it. The carve-out is `repeatable_flags`: options the CLI itself accumulates + (`agent --allow/--deny`, argparse `action="append"`), where every occurrence + reaches the run and there is no "other" value to diverge from. - **Every path must resolve inside the bot's working directory.** `trace ../../.env` is refused before a process is spawned, whether it arrives as a positional or as a flag value. @@ -54,6 +64,10 @@ class CommandSpec: # flag -> the exact values it may take. How `--registry` stays shut against # arbitrary imports while the registries this package ships stay reachable. choice_flags: dict[str, frozenset[str]] = field(default_factory=dict) + # flags the CLI accumulates (argparse `action="append"`), so a second + # occurrence adds to the run rather than replacing what the gate read. + # Every other flag is refused on its second occurrence. + repeatable_flags: frozenset[str] = frozenset() _BUDGET = {"--max-tokens": False, "--max-iterations": False, "--max-seconds": False} @@ -129,6 +143,9 @@ class CommandSpec: # `local` (no confinement) stays unreachable; `claude-cli` delegates to # Claude Code's own sandboxed loop, which the injection below tempers. choice_flags={"--executor": frozenset({"sandbox", "claude-cli"})}, + # Tool-name globs: the CLI appends them, so `--deny 'shell*' --deny + # 'net*'` denies both. Repeating them narrows the run, never widens it. + repeatable_flags=frozenset({"--allow", "--deny"}), ), "approve": CommandSpec( bool_flags=frozenset({"--deny"}), path_positionals=frozenset({0}) @@ -167,8 +184,26 @@ def _confined(raw: str, workdir: Path) -> None: Lexical resolution only — the target need not exist yet (`--trace` names a file the run will create). + + Every refusal here leaves as a `SlackCommandError`, including the ones the + filesystem raises: `Path.resolve()` throws `ValueError` on a NUL byte, and + the bolt listeners catch only `SlackCommandError`, so anything else escapes + the handler and the requester gets no reply at all — silence being the one + answer a chat bot must never give, since it is indistinguishable from being + down. `parse_command` screens NUL bytes out of the whole request before it + gets here; this keeps the guarantee for any other caller. """ - resolved = (workdir / raw).resolve() if not Path(raw).is_absolute() else Path(raw).resolve() + if "\x00" in raw: + # `repr`, not the raw string: a NUL echoed back into a Slack message + # is invisible, and an invisible character is exactly what the reader + # needs to see named. + raise SlackCommandError(f"path contains a NUL byte, which cannot name a file: {raw!r}") + try: + resolved = ( + (workdir / raw).resolve() if not Path(raw).is_absolute() else Path(raw).resolve() + ) + except (ValueError, OSError) as exc: + raise SlackCommandError(f"that is not a usable path: {exc}") from None if not resolved.is_relative_to(workdir.resolve()): raise SlackCommandError(f"path escapes the bot's working directory: `{raw}`") @@ -187,6 +222,14 @@ def parse_command( # with a backtick glued to the first and last token. No admissible # command starts or ends with one, so wrapping backticks are noise. text = text.strip().strip("`").strip() + # A NUL byte cannot name a file, cannot cross into `subprocess`, and makes + # `Path.resolve()` raise `ValueError` — an exception type the bolt handlers + # do not catch, so it would surface as no reply rather than as a refusal. + # Screen it out of the whole request, not just the path-shaped parts: it is + # never meaningful anywhere in a command, and this is the one place that + # sees the text before anything tries to use it. + if "\x00" in text: + raise SlackCommandError("that contains a NUL byte, which cannot name a file or a command") try: tokens = shlex.split(text) except ValueError as exc: @@ -217,10 +260,27 @@ def parse_command( argv = [name] positional_index = 0 index = 0 + seen_flags: set[str] = set() while index < len(rest): token = rest[index] - if token.startswith("--"): + # Any leading dash is a flag, not a positional. The allowlist is meant + # to be exhaustive, and testing for `--` let short options through it: + # `trace -h` was admitted and spent the path positional on `-h`. The + # CLI has one short option and no positional that begins with a dash, + # so treating the whole shape as a flag costs nothing and leaves the + # allowlist the only way in. + if token.startswith("-"): flag, eq, inline_value = token.partition("=") + # Repeats are refused before the flag is read, because reading one + # of several occurrences is what the gate cannot safely do — see + # the module docstring. An inadmissible flag was already refused on + # its first occurrence, so anything reaching here was admitted once. + if flag in seen_flags and flag not in spec.repeatable_flags: + raise SlackCommandError( + f"`{flag}` was given twice; from Slack a flag may appear only once, " + "because the gate and the CLI would not necessarily read the same one" + ) + seen_flags.add(flag) if flag in spec.bool_flags: if eq: raise SlackCommandError(f"`{flag}` takes no value") @@ -329,12 +389,22 @@ def _has_flag(argv: list[str], flag: str) -> bool: def _flag_value(argv: list[str], flag: str) -> str | None: + """The value the CLI will act on: the **last** occurrence, as argparse. + + `parse_command` already refuses a repeated flag, so there is only ever one + here. Reading it argparse's way anyway is the cheap half of the belt: this + function returning the *first* occurrence while `store` kept the last is + precisely how a second `--registry` walked past the agent opt-in, and a + future caller that assembles an argv some other way should not be able to + reopen that gap. + """ + value: str | None = None for index, token in enumerate(argv): if token == flag and index + 1 < len(argv): - return argv[index + 1] - if token.startswith(f"{flag}="): - return token.partition("=")[2] - return None + value = argv[index + 1] + elif token.startswith(f"{flag}="): + value = token.partition("=")[2] + return value def _default_trace() -> str: diff --git a/tests/test_slack_gateway.py b/tests/test_slack_gateway.py index 7c95e91..f8f3b82 100644 --- a/tests/test_slack_gateway.py +++ b/tests/test_slack_gateway.py @@ -184,6 +184,94 @@ def test_the_demo_plan_registries_stay_reachable_without_opt_ins(tmp_path): assert "--approve" not in argv # only the host-acting registry is parked +def test_a_repeated_registry_cannot_walk_the_agent_opt_in(tmp_path): + """The gate read the first `--registry`; argparse would have run the last. + + A benign registry in front of the stdlib one was admitted against the + benign value and executed against the agent one, with the forced + `--approve` skipped in the same step. Both orders, so neither "the gate + reads the last" nor "the gate reads the first" can pass this again. + """ + kwargs = dict(workdir=tmp_path, allow_model=True, allow_agent=False) + benign = "grapharc.examples.plan_docs:build_registry" + stdlib = "grapharc.stdlib:build_registry" + for first, second in ((benign, stdlib), (stdlib, benign)): + with pytest.raises(SlackCommandError, match="twice"): + parse_command( + f"plan 'ship it' --model openrouter/x/y " + f"--registry {first} --registry {second}", + **kwargs, + ) + # The single-flag refusal is untouched. + with pytest.raises(SlackCommandError, match="GRAPHARC_SLACK_ALLOW_AGENT"): + parse_command(f"plan 'ship it' --model openrouter/x/y --registry {stdlib}", **kwargs) + # And so is the single-flag admission of a benign registry. + argv = parse_command(f"plan 'ship it' --model openrouter/x/y --registry {benign}", **kwargs) + assert argv.count("--registry") == 1 + + +def test_a_repeated_model_cannot_smuggle_a_backend_past_the_spend_gate(tmp_path): + with pytest.raises(SlackCommandError, match="paid backend"): + parse_command("plan goal --model mock/x --model openrouter/a/b", workdir=tmp_path) + # Opted in, a second `--model` is still refused: the gate must never have + # to choose which of two values the CLI is going to use. + with pytest.raises(SlackCommandError, match="twice"): + parse_command( + "plan goal --model mock/x --model openrouter/a/b", + workdir=tmp_path, + allow_model=True, + ) + # The `=` form is the same flag, whichever way each occurrence is spelled. + with pytest.raises(SlackCommandError, match="twice"): + parse_command( + "plan goal --model mock/x --model=openrouter/a/b", + workdir=tmp_path, + allow_model=True, + ) + + +def test_every_gated_flag_is_refused_in_its_duplicated_form(tmp_path): + """Exhaustive over the allowlist, so a future gate cannot reopen the gap. + + Every admitted flag except the ones the CLI accumulates (`action="append"`) + must be refused when it appears twice — the gate reads one occurrence, and + a flag whose two occurrences could differ is a flag the gate cannot judge. + """ + from grapharc.slack.command import ALLOWED_COMMANDS + + checked = 0 + for name, spec in ALLOWED_COMMANDS.items(): + flags = set(spec.bool_flags) | set(spec.model_flags) | set(spec.value_flags) + flags |= set(spec.choice_flags) + for flag in sorted(flags - set(spec.repeatable_flags)): + if flag in spec.bool_flags: + text = f"{name} {flag} {flag}" + else: + if flag in spec.choice_flags: + value = sorted(spec.choice_flags[flag])[0] + elif spec.value_flags.get(flag, False): + value = "inside.jsonl" + else: + value = "1" + text = f"{name} {flag} {value} {flag} {value}" + with pytest.raises(SlackCommandError, match="twice"): + parse_command(text, workdir=tmp_path, allow_model=True, allow_agent=True) + checked += 1 + assert checked > 20, "the allowlist shrank; this sweep should still be broad" + + +def test_the_flags_the_cli_accumulates_stay_repeatable(tmp_path): + """`--allow`/`--deny` are argparse `append`: every occurrence reaches the run.""" + argv = parse_command( + "agent task --deny 'shell*' --deny 'net*' --allow 'read*' --allow 'list*'", + workdir=tmp_path, + allow_model=True, + allow_agent=True, + ) + assert argv.count("--deny") == 2 + assert argv.count("--allow") == 2 + + def test_a_command_pasted_with_code_backticks_still_parses(tmp_path): """Copying from a code-formatted Slack message brings the backticks along.""" argv = parse_command( @@ -224,6 +312,40 @@ def test_a_path_flag_value_may_not_escape_the_workdir_either_form(tmp_path): parse_command("plan goal --trace=../t.jsonl", workdir=tmp_path) +def test_a_nul_byte_is_a_refusal_not_an_exception(tmp_path): + """`Path.resolve()` raises `ValueError` on a NUL; the bot must still reply. + + `handle_text_live` catches `SlackCommandError` and nothing else, so a + `ValueError` out of the gate escaped the bolt listener and the requester + saw no reply at all — indistinguishable from the bot being down. + """ + from grapharc.slack.bot import handle_text + + for text in ("trace a\x00b", "plan goal --trace a\x00b", "plan a\x00b --model mock/x"): + with pytest.raises(SlackCommandError, match="NUL byte"): + parse_command(text, workdir=tmp_path, allow_model=True) + + config = SlackBotConfig(bot_token="xoxb-x", app_token="xapp-x", workdir=tmp_path) + reply = handle_text("trace a\x00b", config) + assert "NUL byte" in reply + + # `_confined` keeps the guarantee for any other caller of its own. + from grapharc.slack.command import _confined + + with pytest.raises(SlackCommandError, match="NUL byte"): + _confined("a\x00b", tmp_path) + + +def test_a_single_dash_token_is_refused_as_a_flag_not_taken_as_a_path(tmp_path): + """The flag allowlist is meant to be exhaustive; `--` let short options by.""" + with pytest.raises(SlackCommandError, match="not allowed"): + parse_command("trace -h", workdir=tmp_path) + with pytest.raises(SlackCommandError, match="not allowed"): + parse_command("run graph.toml -x", workdir=tmp_path) + with pytest.raises(SlackCommandError, match="not allowed"): + parse_command("metrics -", workdir=tmp_path) + + def test_a_path_inside_the_workdir_is_admitted_even_absolute(tmp_path): inside = tmp_path / "runs" / "t.jsonl" argv = parse_command(f"trace {inside}", workdir=tmp_path)