Close the Slack gate's flag-repetition bypass, and stop a NUL byte from silencing the bot

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.

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 <demo> --registry <stdlib>`), 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**;

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 <shipped demo>
+  --registry <stdlib>` 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:

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)