From 46255af240e20c110171c08cf1e8c511a05245b1 Mon Sep 17 00:00:00 2001 From: Marcello Quarta <3270392+Maqix@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:43:37 +0000 Subject: [PATCH] fix(security): read a push line in a literal gh body as data The push splitter ends a segment at every newline, quoted or not, so the middle line of `gh pr create --body "notesgit pushmore"` was judged as a bare push (rule git-publish-push-bare) although bash hands the whole value to gh as one word. shell_normalizer_walk._gh_data_newlines_as_spaces reads one closed shape as its single-line spelling: the whole command is one `gh pr|issue|release ...` of plain words and whole literal quoted words (each closed, between blanks, no curly quote mark, no `$`/backtick/backslash in double quotes), and every multi-line quoted word is the value of -b/--body, -t/--title, --subject or -n/--notes. is_denied reads that view before it hands a source to the git-publish floor, so such a command is no publish and records no push_allowed audit. Every other text is returned unchanged and keeps its per-line reading. kiro-cli titles a shell call `Running: ` and the deny tiers judge that title as a subject of its own, so the helper reads through ONE exact leading `running: ` (the lowercased view) and keeps it: the title gets the reading of the command it displays. The constant moves from denied_rules to shell_normalizer_walk, below both readers, so _exception_applies and the helper share it. A second prefix, another spelling, or one that does not start the text is read as text. The gh-body reading only holds where the source IS the script the shell runs, and escape decoders break that. is_denied lowercases before the walk, so in `printf 'gh pr create --body \X27\x0a\x0a\X27' | bash` (also `\X22`, `echo -e`) the `\X27` that printf prints as text, and the shell reads as `X27`, became `\x27` and decoded to a quote. A printf FORMAT reads `\0047` as `\004` + `7`, and bash's `echo -e` reads `\47` as text, yet the walk decoded both to a quote. And a printf FORMAT is yielded with its conversions unfilled: in `printf 'gh pr create --body "n%s\x0a\x0a%s"' '"' ': "' | bash` the `%s` argument closes the body before the push line. The publish gate therefore spaces a source only when the command holds no backslash at all and the source holds no `%` other than `%%`; any other source keeps its per-line push reading, as on main. A nested source can also be rewritten before its evaluator reads it. In `echo 'gh pr create --body "xgit pu''sh origin main"' | tr -d '"' | bash` the walk yields the echo text, whose push line sits inside a gh body, but `tr` drops the quotes, so bash runs the push line as its own command. The gh-body reading applies only to the command the shell itself runs; every nested source keeps its per-line reading, as on main. The payload walk's own printf/echo decode reading, which main shares, is left as on main; #25197 tracks the printf/echo decode reading. Closes #20846 --- docs/system-specs/modules/security.md | 1 + src/kiro_crew/security/denied_rules.py | 6 +- src/kiro_crew/security/deny_gate.py | 24 +- src/kiro_crew/security/shell_normalizer.py | 2 + .../security/shell_normalizer_walk.py | 91 ++++++++ test/test_push_branch_gate.py | 215 ++++++++++++++++++ test/test_security_gate_liveness.py | 15 +- 7 files changed, 347 insertions(+), 7 deletions(-) diff --git a/docs/system-specs/modules/security.md b/docs/system-specs/modules/security.md index 2c7d5680f11..19b3aea386d 100644 --- a/docs/system-specs/modules/security.md +++ b/docs/system-specs/modules/security.md @@ -1866,6 +1866,7 @@ continue to apply. - `_PUSH_ALL_BRANCHES_OPTS` covers `--mirror`, `--all`, and Git's `--branches` alias are denied **outright** (they push every local branch, so a per-branch target check cannot vouch for them), kept in lockstep with the `--(mirror|all)` regex in `config/defaults.json`. - `_is_push_to_protected_branch()` splits the command with `_split_segments()` and validates **every** `push` segment / refspec (closing the `push origin feat && push origin main` bypass), normalizing `refs/heads/…` paths and `local:remote` refspecs; refspecs with shell/revision syntax (`$`, `` ` ``, `@{…}` — `_AMBIGUOUS_REFSPEC_RE`) are treated as ambiguous and denied. If a push was detected upstream but **no** clean segment parses, it denies to be safe. - **Force push:** a force flag (`--force` / `-f` / `--force-with-lease`) does not by itself make a feature-branch push protected (force-push to a feature branch is normal PR/rebase workflow), but force-push to a *protected* branch is still blocked because the target check fires regardless of flags. + - **A push line inside a literal `gh` text value is data.** The segment split ends at every newline, quoted or not, so on its own it reads the middle line of `gh pr create --body "notes⏎git push⏎more"` as a bare push. `shell_normalizer_walk._gh_data_newlines_as_spaces` reads that one shape as its single-line spelling: the whole command is one `gh pr|issue|release …` of plain words (`[A-Za-z0-9._/:=+,-]`, so no PowerShell `--%` stop-parsing token or `@` splat) and whole quoted words, every quote closes and stands between blanks, no quoted word holds a curly quote mark (PowerShell reads `‘ ’ ‚ ‛ “ ” „` as quotes too), a double-quoted word holds no `$`, backtick or backslash, and each multi-line quoted word is the value of `-b/--body`, `-t/--title`, `--subject` or `-n/--notes`. With no separator, comment, heredoc, escape or expansion outside the quotes, bash's and PowerShell's quote reading of that text is the walk's, and gh is the only program the shell runs (gh refuses an alias or extension named after a core command). `is_denied` reads that view before it hands any source to `_git_publish_floor_tags`, so such a command is no publish and records no `push_allowed` audit. That reading is withheld where the source is not the script the shell runs: any nested source the payload walk yields (`source != lower` in `deny_gate.is_denied`), since a filter such as `tr -d '"'` or `sed` between it and its evaluator may rewrite it first, a source with an unresolved printf conversion (`%s`, `%b`, ...; `%%` is a literal `%`), whose arguments can close the body before the push line, and any command holding a backslash at all (`undecoded` in `deny_gate.is_denied`), since every escape decoder can make a quote bash never prints: `str.lower` turns `\X27`, which printf and `echo -e` print as text, into a decoded quote, a printf format reads `\0047` as `\004` + `7`, and bash's `echo` prints `\47` as text. Any other text is returned unchanged and keeps its per-line reading: an env prefix, `cd … &&`, a pipe or redirect, another owner (`bash -c`, `sh -c`, `eval`, `echo`, `git commit -m`), a positional or non-text-flag value, and every quote trick (comment apostrophe, heredoc, backslash-newline, unbalanced or glued quote). kiro-cli titles a shell call `Running: ` and the deny tiers judge that title as a subject of its own, so one exact leading `running: ` (`shell_normalizer_walk._SHELL_TITLE_PREFIX`, the lowercased view; `denied_rules._exception_applies` reads through the same constant) is read through and kept: the title gets its command's reading. A second prefix, any other spelling, or one that does not start the text is read as text, so a title never gets a weaker verdict than the command it displays. On Windows the text is always returned unchanged (`_QUOTED_NEWLINE_MAY_END_COMMAND`): cmd.exe ends a command at a newline even inside double quotes, and nothing proves which shell runs an agent's command there. Pinned by `test_push_branch_gate.py::TestQuotedGhTextValueIsNotAPush`. - **Self-protection (argv-structural floor):** the controls that stop the agent disabling its own controls — credential minting, process termination, the dev-mode attestation flag, the ssh-family sandbox escape, and the `restart` / `update` / `cloud` / `gateway restart` lifecycle commands, and the `skills authority-retire` recovery — are enforced by dedicated argv predicates. They come in two shapes, and the difference is whether a catalog row exists to gate on: - **Row-backed, UNION with the regex tier** (`_SELF_PROTECTION_FLOOR_RULE_IDS`: the credential mint, the self-kill, the dev-mode confirm flag, and `sandbox-escape-ssh-self` — an ssh/scp/sftp/rsync TARGET resolving to this same machine re-enters the host outside the sandbox, so the `_is_ssh_to_self` predicate resolves the operand-position target: options, redirects, `user@`/URI/IPv6/numeric-IP spellings, and this host's own names, the last resolved in a background thread so the gate never blocks on DNS). On Linux a non-own IP literal in host position is refused until the netlink `RTM_GETADDR` address table has published (`_NETLINK_ADDRS_PUBLISHED`), because it could be an unlisted secondary of this machine; the gateway warms that table at boot without blocking (`warm_own_host_names`, run through `asyncio.to_thread`). Hosts-file aliases are read into a table (`hosts_file.py`): a file up to 64 KiB is parsed in the same call on a miss, a larger one is parsed by a background warm when the check runs on an event loop, and in the same call (read up to `_HOSTS_FILE_READ_CAP`, exactly as the warm reads it) on a thread with no running loop (`atomic_write.on_event_loop`), such as a stdio MCP server or the cron fire-time vet, where a pending answer would not be retried; a failed read answers fail closed; the table is keyed on the file's stat plus, for a file up to 64 KiB, a content digest on every platform, since a same-size rewrite inside one coarse ctime tick leaves the stat unchanged (a larger POSIX file keeps a stat-only key, so a same-size rewrite inside the ctime tick of its previous write is still served stale); on Windows a dotless name is refused while the hosts file is over 64 KiB, since its content cannot be verified. The refusal note names three labelled cases — **PENDING** (retry: classification is still running), **Still refused** (an unresolved name, an unreadable address list, or the target really is this machine), and **FORWARDED** port (a forwarded port on a loopback address is denied like the host). Enforced by **both** the row's `pattern` in the regex tier **and** the predicate, and the predicate runs only while the row is in the effective set, so an operator who disabled the row has disabled the floor with it. Neither half is sufficient alone, and the floor is deliberately **additive**, never a replacement. This gate is the **interim tier**, not closure of the escape: it sees only tool-routed command lines, so interpreter/script-file indirection (`bash escape.sh`, `python -c` + subprocess/paramiko) and non-ssh-family clients (`git clone ssh://localhost/…`, autossh) still reach the unsandboxed sshd — the sandbox has no network namespace, and the fix of record is an OS-level network fence there (issue #9806). - **Floor-only, UNGATED** (`_SELF_PROTECTION_UNGATED_FLOOR_IDS`: `self-protection-restart`, `self-protection-update`, `self-protection-gateway-restart`, `self-protection-cloud`, `self-protection-skills-authority-retire`). These have **no catalog row**: the four lifecycle floors' regex rows fired on the product's name appearing anywhere in a command and were deleted (see "Self-protection global options" above), and the `skills authority-retire` floor never had one: it denies the operator-terminal recovery that moves the auto-skill publication authority aside (`kirocrew skills authority-retire`), which an agent shell must not run because the authority is what keeps an agent from loosening its own skill governance. Like the other subcommand floors, it refuses that command's `--help` form too; the refusal note sends the operator to their own terminal, and `kirocrew skills --help` stays allowed. With no row there is no toggle, so the predicates run unconditionally — like the git-publish anti-obfuscation branches — and a refusal's first line reports the id itself, with the structural note on the second line. The ids are deliberately not catalog ids, and the two sets are pinned disjoint: gating a predicate on a row lookup was how a deleted row became a silently disabled floor (`is_denied` used to `continue` past an id that resolved to no pattern), so a subcommand floor may never rejoin the gated set without its row. `_LEGACY_RULE_ID_BY_PATTERN` carries no alias for those rows for the same reason — a governance pin persisted in either spelling of those rows resolves to nothing, which is the truth: an always-on floor has nothing left to pin. The map aliases only a prior spelling of a row that still exists (the bare-substring spelling of `reverse-shell-nc`, whose row is anchored to the `nc` command token), so a ceiling written against the older catalog keeps pinning that row across the upgrade. diff --git a/src/kiro_crew/security/denied_rules.py b/src/kiro_crew/security/denied_rules.py index ab73f757457..89063a791b3 100644 --- a/src/kiro_crew/security/denied_rules.py +++ b/src/kiro_crew/security/denied_rules.py @@ -59,7 +59,7 @@ BUILTIN_DENY_PATTERNS, ) from .diagnostics import RefusalDiagnostic, annotate_refusal, refusal_diagnostic -from .shell_normalizer import _SHELL_ACTIVE_CHARS +from .shell_normalizer import _SHELL_ACTIVE_CHARS, _SHELL_TITLE_PREFIX if TYPE_CHECKING: from collections.abc import Iterable @@ -488,10 +488,6 @@ def _exception_eligible(view: str) -> bool: return not _SHELL_ACTIVE_CHARS.intersection(view) -#: kiro-cli's display prefix on a shell call's title, lowercased like the views. -_SHELL_TITLE_PREFIX = "running: " - - def _exception_applies(view: str, exceptions: list[str]) -> bool: """Whether one of *exceptions* exonerates the lowercased *view*. diff --git a/src/kiro_crew/security/deny_gate.py b/src/kiro_crew/security/deny_gate.py index 6f5c47892ab..daa10566d11 100644 --- a/src/kiro_crew/security/deny_gate.py +++ b/src/kiro_crew/security/deny_gate.py @@ -627,7 +627,29 @@ def _reason( # complete bypass of the sole enforcement pushes have; gating once at the end # keeps the per-rule opt-out semantics exactly as written -- a rule an operator # disabled stays disabled at whatever depth it fires. - publish_sources = [source for source in payload_sources if _argv._is_git_publish(source)] + # A source whose every push line lies inside a literal gh text value is not + # a publish, so the floor never reads it and it records no ``push_allowed`` + # audit either. That reading holds only where the source IS the script the + # shell runs, so a source keeps the per-line reading whenever the command + # holds a backslash or a printf conversion. Every escape decoder here can + # make a quote the shell never sees: lowercasing turns ``\X27`` (text) into + # ``\x27`` (a quote), a printf FORMAT reads ``\0047`` as ``\004`` + ``7``, + # and bash's echo prints ``\47`` as text. A conversion its arguments fill + # (``%s`` -> ``"``) can close the body before the push line; ``%%`` is a + # literal ``%``. A command with neither is decoded by nothing. A NESTED + # source keeps the per-line reading too: a filter such as ``tr`` or ``sed`` + # may rewrite its text (drop the quotes) before the evaluator reads it. + _spaced = _submodule("shell_normalizer")._gh_data_newlines_as_spaces + undecoded = "\\" not in tool_name + publish_sources = [ + source + for source in payload_sources + if _argv._is_git_publish( + _spaced(source) + if source == lower and undecoded and "%" not in source.replace("%%", "") + else source + ) + ] if publish_sources: floor_tags: frozenset[str] = frozenset() for publish_source in publish_sources: diff --git a/src/kiro_crew/security/shell_normalizer.py b/src/kiro_crew/security/shell_normalizer.py index af7e497e7f9..441ff14be0b 100644 --- a/src/kiro_crew/security/shell_normalizer.py +++ b/src/kiro_crew/security/shell_normalizer.py @@ -81,6 +81,7 @@ _PROCESS_SUBSTITUTION_OPENERS, _SHELL_ACTIVE_CHARS, _SHELL_ASSIGN_RE, + _SHELL_TITLE_PREFIX, _SHELL_WRAPPER_CHARS, _VAR_USE_RE, _arms_case_context, @@ -90,6 +91,7 @@ _dequote_token, _ends_argv, _fold_line_continuations, + _gh_data_newlines_as_spaces, _is_computed_value, _is_shell_variable_reference, _iter_shell_chars, diff --git a/src/kiro_crew/security/shell_normalizer_walk.py b/src/kiro_crew/security/shell_normalizer_walk.py index 149c7484e25..98a8c145eb2 100644 --- a/src/kiro_crew/security/shell_normalizer_walk.py +++ b/src/kiro_crew/security/shell_normalizer_walk.py @@ -17,6 +17,7 @@ from __future__ import annotations +import os import re from typing import TYPE_CHECKING, NamedTuple @@ -1259,6 +1260,96 @@ def _split_push_command_segments(text: str) -> list[str]: rest = rest[resume_at:] +#: ``gh`` command groups whose body / title / notes flags only ever carry text +#: for the GitHub API. gh refuses an alias or extension named after a core +#: command, so these names cannot be redefined to run their arguments. +_GH_DATA_COMMAND_GROUPS = frozenset({"pr", "issue", "release"}) +#: The value-taking text flags of those groups (``-m`` is deliberately absent: +#: on ``gh pr merge`` it is the boolean ``--merge``, not a message). +_GH_DATA_FLAGS = frozenset({"-b", "--body", "-t", "--title", "--subject", "-n", "--notes"}) +#: An unquoted word of that command: no expansion, glob, redirect, comment, +#: escape or separator character can appear in one, in bash or in PowerShell +#: (no ``%``, so no ``--%`` stop-parsing token; no ``@``, so no splat). +_GH_PLAIN_WORD_RE = re.compile(r"[A-Za-z0-9._/:=+,-]+") +#: Characters PowerShell also reads as quote marks, so one inside an ASCII +#: quoted word could end it there while bash reads on. +_POWERSHELL_EXTRA_QUOTES = frozenset("\u2018\u2019\u201a\u201b\u201c\u201d\u201e") +#: cmd.exe ends a command at a newline even inside double quotes, and nothing +#: here proves which shell runs an agent's command on Windows, so there a quoted +#: newline is not proven data and every line keeps its own reading. +_QUOTED_NEWLINE_MAY_END_COMMAND = os.name == "nt" +#: kiro-cli's display prefix on a shell call's title, lowercased like the views. +_SHELL_TITLE_PREFIX = "running: " + + +def _gh_data_newlines_as_spaces(text: str) -> str: + """Return *text* with newlines inside a literal ``gh`` text-flag value spaced. + + The push splitter ends a segment at EVERY newline, quoted or not, so a line + of ``gh pr create --body "notes⏎git push⏎more"`` that reads ``git push`` was + judged as a bare push although bash hands the whole value to gh as one word. + This removes that reading for one shape only, where the quote walk below is + exactly bash's and PowerShell's: the whole text is ONE simple command ``gh + pr|issue|release ...`` of plain words (``_GH_PLAIN_WORD_RE``) and whole + quoted words, every quoted word closes, stands alone between blanks, holds + no ``_POWERSHELL_EXTRA_QUOTES`` mark, and no ``$``, backtick or backslash + when double-quoted, and every quoted word that spans lines is the value of a + ``_GH_DATA_FLAGS`` flag. With no separator, comment, heredoc, escape or + expansion outside the quotes, gh is the only program the shell runs. + + Quote state comes from :func:`_iter_shell_chars`; the checks above are read + over its steps, and the spaced view is rebuilt from them with each QUOTED + newline as a space. Any other text is returned unchanged, so the caller + reads it as before; the spaced view only ever equals the single-line + spelling of the same command. kiro-cli titles a shell call ``Running: + `` and the deny tiers judge that title too, so ONE exact leading + ``_SHELL_TITLE_PREFIX`` is read through and kept: the title gets the reading + of the command it displays. On Windows + (``_QUOTED_NEWLINE_MAY_END_COMMAND``) the text is always returned unchanged. + """ + if "\n" not in text or _QUOTED_NEWLINE_MAY_END_COMMAND: + return text + prefix = _SHELL_TITLE_PREFIX if text.startswith(_SHELL_TITLE_PREFIX) else "" + steps = list(_iter_shell_chars(text[len(prefix) :])) + words: list[tuple[str, bool]] = [] + start = 0 + for end in [*(i for i, s in enumerate(steps) if s.active and s.char in " \t"), len(steps)]: + if end > start: + word = _gh_text_word(steps[start:end], words[-1] if words else None) + if word is None: + return text + words.append(word) + start = end + 1 + if len(words) < 2 or words[0] != ("gh", False): + return text + if words[1][1] or words[1][0] not in _GH_DATA_COMMAND_GROUPS: + return text + return prefix + "".join(" " if s.char == "\n" else s.text for s in steps) + + +def _gh_text_word( + steps: "list[_ShellChar]", previous: "tuple[str, bool] | None" +) -> "tuple[str, bool] | None": + """One blank-delimited word of :func:`_gh_data_newlines_as_spaces` as ``(text, + quoted)``, or None when it is not a plain word or one whole literal quoted word.""" + raw = "".join(s.text for s in steps) + if all(s.active for s in steps): + return (raw, False) if _GH_PLAIN_WORD_RE.fullmatch(raw) else None + first, last = steps[0], steps[-1] + if not first.active or first.char not in "'\"" or len(steps) < 2 or last.state != 0: + return None + if any(s.state == 0 for s in steps[:-1]): + return None # the quote closed before the word ended: text glued to it + body = raw[1:-1] + if first.char == '"' and any(c in body for c in "$`\\"): + return None + if any(c in _POWERSHELL_EXTRA_QUOTES for c in body): + return None + if "\n" in body and (previous is None or previous[1] or previous[0] not in _GH_DATA_FLAGS): + return None + return (body, True) + + # Shell expansions that fuse text INTO a word, so the literal command hides the # real push target. Any of these inside a git-publish command is unverifiable # -> deny (fail closed): diff --git a/test/test_push_branch_gate.py b/test/test_push_branch_gate.py index a695e7ae944..374b9822263 100644 --- a/test/test_push_branch_gate.py +++ b/test/test_push_branch_gate.py @@ -1340,3 +1340,218 @@ def test_end_of_options_stops_option_parsing_for_later_dash_tokens(self): # scan's single-arg misread) and not fed to the unrecognised-option # fallback (which would over-deny a shape git accepts). assert not security._git_publish_floor_tags("git push -- origin -x") + + +class TestQuotedGhTextValueIsNotAPush: + """A push line inside a literal ``gh`` body / title / notes value is data. + + The push splitter ends a segment at every newline, so the middle line of + ``gh pr create --body "notes⏎git push⏎more"`` was judged as a bare push. Only + one provable shape is read as data: a single ``gh pr|issue|release`` command + of plain words and literal quoted words, the multi-line one being the value + of a text flag. Every other shape keeps its per-line reading. + """ + + ALLOWED = ( + f'gh pr create --body "notes\n{PUSH}\nmore"', + f"gh pr create --body 'notes\n{PUSH}\nmore'", + f'gh pr create -R o/r --head u:fix/x --title "t" --body "a\n{PUSH} origin main\nb"', + f'gh issue comment 12 -b "x\n{PUSH}\n"', + f'gh release create v1 --notes "n\n{PUSH} -f\n"', + ) + + DENIED = ( + # A real push outside the quotes, on its own line or after a separator. + f'gh pr create --body "notes"\n{PUSH}', + f'gh pr create --body "x\n"\n{PUSH} origin main', + f'gh pr create --body "x\n" ; {PUSH} origin main', + f'gh pr create --body "x\n" && {PUSH} origin ma$b', + f'gh pr create --body "x\n" && {PUSH} origin {{main,x}}', + f"gh pr create --body \"x\n\" && {PUSH} origin $'main'", + f'{PUSH} origin main\ngh pr create --body "a\n"', + # Quote tricks whose quote reading is not bash's. + f"gh pr create --body x # don't\n{PUSH}\n'", + f"gh pr create --body \"x\n\" # it's\n{PUSH}\n'", + f'gh pr create --body "$(echo)\n{PUSH}\n"', + f'gh pr create --body "a\\"\n{PUSH}\n"', + f'gh pr create --body "unterminated\n{PUSH}', + f"gh pr create --body x \\\n{PUSH}", + f'gh pr create --body-file - </dev/null', + f'(gh pr create --body "a\n{PUSH}\n")', + ) + + @pytest.fixture(autouse=True) + def _shell_reads_quoted_newlines_as_data(self, _floor_monkeypatch) -> None: + # The allowed shape holds where the shell is bash or PowerShell; the + # Windows reading is pinned on its own below, so the rows run alike on + # every CI platform. + from kiro_crew.security import shell_normalizer_walk + + _floor_monkeypatch.setattr(shell_normalizer_walk, "_QUOTED_NEWLINE_MAY_END_COMMAND", False) + + #: kiro-cli titles a shell call ``Running: `` and the deny tiers judge + #: that title as a subject of its own, so every row is also run as its title. + TITLES = ("", "Running: ") + + #: Not the one exact title prefix: each keeps the per-line reading. + PREFIX_LOOKALIKES = ( + f'xRunning: gh pr create --body "notes\n{PUSH}\nmore"', + f'Running:gh pr create --body "notes\n{PUSH}\nmore"', + f'Running:\tgh pr create --body "notes\n{PUSH}\nmore"', + f'Running: Running: gh pr create --body "notes\n{PUSH}\nmore"', + f'Running: gh pr create --body "x\n"\nRunning: {PUSH} origin main', + ) + + @pytest.mark.parametrize("title", TITLES) + @pytest.mark.parametrize("command", ALLOWED) + def test_a_push_line_in_a_literal_gh_text_value_is_allowed( + self, command: str, title: str, captured_sel_events + ) -> None: + assert is_denied(title + command) is None + # Not a publish at all, so no push_allowed audit is recorded for it. + assert not any(e.event_type == "push_allowed" for e in captured_sel_events) + + @pytest.mark.parametrize("title", TITLES) + @pytest.mark.parametrize("command", DENIED) + def test_every_other_shape_keeps_its_per_line_reading(self, command: str, title: str) -> None: + from kiro_crew.security import shell_normalizer + + subject = (title + command).lower() + assert shell_normalizer._gh_data_newlines_as_spaces(subject) == subject + assert is_denied(title + command), f"{title + command!r} was allowed" + assert _is_push_to_protected_branch(command) + + @pytest.mark.parametrize("command", PREFIX_LOOKALIKES) + def test_only_one_exact_title_prefix_is_read_through(self, command: str) -> None: + from kiro_crew.security import shell_normalizer + + assert shell_normalizer._gh_data_newlines_as_spaces(command.lower()) == command.lower() + assert is_denied(command), f"{command!r} was allowed" + + @pytest.mark.parametrize("title", TITLES) + @pytest.mark.parametrize("command", ALLOWED) + def test_on_windows_a_quoted_newline_keeps_the_per_line_reading( + self, command: str, title: str, monkeypatch + ) -> None: + # cmd.exe ends a command at a newline even inside double quotes, so on + # Windows the push line in the body may run, and it is judged as a push. + from kiro_crew.security import shell_normalizer_walk + + subject = (title + command).lower() + monkeypatch.setattr(shell_normalizer_walk, "_QUOTED_NEWLINE_MAY_END_COMMAND", True) + assert shell_normalizer_walk._gh_data_newlines_as_spaces(subject) == subject + assert is_denied(title + command), f"{title + command!r} was allowed" + + +class TestPrintfOutputIsNotItsSourceText: + """A ``printf`` / ``echo -e`` producer prints text its source does not spell. + + ``is_denied`` lowercases before the walk, so ``\\X27`` (printed as text, read + by the shell as ``X27``) became ``\\x27`` and decoded to a quote; and a printf + FORMAT's ``%s`` is filled by its arguments, so ``%s`` -> ``"`` closes the body + before the push line. The decoder also reads ``\\0047`` and echo's ``\\47`` as + quotes bash never prints. So only a command with no backslash and no + conversion gets the literal gh-body reading. + """ + + PROTECTED = ( + f"printf 'gh pr create --body \\X27\\x0a{PUSH} origin main\\x0a\\X27' | bash", + f"printf 'gh pr create --body \\X22\\x0a{PUSH} origin main\\x0a\\X22' | bash", + f"echo -e 'gh pr create --body \\X27\\x0a{PUSH} origin main\\x0a\\X27' | bash", + f"printf 'gh pr create --body \"n%s\\x0a{PUSH} origin main\\x0a%s\"' '\"' ': \"' | bash", + f"printf 'gh pr create --body \"n%b\\x0a{PUSH} origin main\\x0a%b\"' '\"' ': \"' | bash", + f'printf "gh pr create --body \'n%s\\x0a{PUSH} origin main\\x0a%s\'" "\'" ": \'" | bash', + # A printf FORMAT reads ``\\0047`` as ``\\004`` + ``7``, and bash's echo + # prints ``\\47`` as text: neither makes a quote, so the push line runs. + f"printf 'gh pr create --body \\0047\\x0a{PUSH} origin main\\x0a\\0047' | bash", + f"printf 'gh pr create --body \\0042\\x0a{PUSH} origin main\\x0a\\0042' | bash", + f"echo -e 'gh pr create --body \\47\\x0a{PUSH} origin main\\x0a\\47' | bash", + f"echo -e 'gh pr create --body \\42\\x0a{PUSH} origin main\\x0a\\42' | bash", + # Any backslash keeps the per-line reading, even where the printed text + # would be a real gh body: refused, as on main. + f"printf 'gh pr create --body \"100%%\\x0a{PUSH} origin main\\x0a\"' | bash", + ) + + FEATURE = ( + f"printf 'gh pr create --body \\X27\\x0a{PUSH} origin feat/x\\x0a\\X27' | bash", + f"printf 'gh pr create --body \"n%s\\x0a{PUSH} origin feat/x\\x0a%s\"' '\"' ': \"' | bash", + f"printf 'gh pr create --body \\0047\\x0a{PUSH} origin feat/x\\x0a\\0047' | bash", + f"echo -e 'gh pr create --body \\47\\x0a{PUSH} origin feat/x\\x0a\\47' | bash", + ) + + #: No backslash and no conversion: nothing decodes it, so the body is data. + LITERAL_BODY = f"gh pr create --body '100%%\n{PUSH} origin main\n'" + + @pytest.fixture(autouse=True) + def _shell_reads_quoted_newlines_as_data(self, _floor_monkeypatch) -> None: + from kiro_crew.security import shell_normalizer_walk + + _floor_monkeypatch.setattr(shell_normalizer_walk, "_QUOTED_NEWLINE_MAY_END_COMMAND", False) + + @pytest.mark.parametrize("command", PROTECTED) + def test_a_printed_quote_does_not_hide_a_protected_push(self, command: str) -> None: + assert is_denied(command), f"{command!r} was allowed" + + @pytest.mark.parametrize("command", FEATURE) + def test_the_feature_twin_is_an_audited_push(self, command: str, captured_sel_events) -> None: + assert is_denied(command) is None + assert any(e.event_type == "push_allowed" for e in captured_sel_events) + + def test_a_literal_percent_keeps_the_body_reading(self, captured_sel_events) -> None: + assert is_denied(self.LITERAL_BODY) is None + assert not any(e.event_type == "push_allowed" for e in captured_sel_events) + + +class TestFilteredPayloadKeepsItsLineReading: + """A filter between a payload and its evaluator may rewrite the payload. + + ``echo '' | tr -d '"' | bash`` hands the evaluator the echo text with + its quotes removed, so a push line the source text holds inside a gh body runs + as its own command. Only the command the shell itself runs gets the gh-body + reading; every nested source keeps its per-line reading, as on main. + """ + + _BODY = "gh pr create --body \"x\ngit pu''sh origin {}\n\"" + + PROTECTED = ( + f"echo '{_BODY.format('main')}' | tr -d '\"' | bash", + f"echo '{_BODY.format('main')}' | sed 's/\"//g' | sh", + f"echo '{_BODY.format('main')}' | tr -d '\"' | bash -s", + ) + + FEATURE = f"echo '{_BODY.format('feat/x')}' | tr -d '\"' | bash" + + @pytest.fixture(autouse=True) + def _shell_reads_quoted_newlines_as_data(self, _floor_monkeypatch) -> None: + from kiro_crew.security import shell_normalizer_walk + + _floor_monkeypatch.setattr(shell_normalizer_walk, "_QUOTED_NEWLINE_MAY_END_COMMAND", False) + + @pytest.mark.parametrize("command", PROTECTED) + def test_a_filtered_payload_does_not_hide_a_protected_push(self, command: str) -> None: + assert is_denied(command), f"{command!r} was allowed" + + def test_the_feature_twin_is_an_audited_push(self, captured_sel_events) -> None: + assert is_denied(self.FEATURE) is None + assert any(e.event_type == "push_allowed" for e in captured_sel_events) diff --git a/test/test_security_gate_liveness.py b/test/test_security_gate_liveness.py index b3dc6c25b54..fc0c2650d37 100644 --- a/test/test_security_gate_liveness.py +++ b/test/test_security_gate_liveness.py @@ -588,7 +588,20 @@ def _url_payload_command(n: int) -> str: #: their comments and their manifest lines; the exemption only narrows. #: Raised again, from 32,899, for ``redaction.py``'s hold on a whole SRI digest that a #: streamed delta cuts: it is counted on top of the ARN exemption above. -_PACKAGE_LINE_BUDGET = 32_909 +#: +#: Raised again, from 32,909, for ``shell_normalizer_walk._gh_data_newlines_as_spaces`` +#: and its caller in ``is_denied``: a ``git push`` line inside a literal ``gh pr|issue|release`` +#: text-flag value is read as the one word bash hands gh. One closed shape, read +#: over ``_iter_shell_chars`` steps, and every other command keeps its per-line +#: push reading. ``_SHELL_TITLE_PREFIX`` moves to that module so the helper reads +#: a ``Running: `` title through the constant ``denied_rules`` already uses. +#: Twelve more for the publish gate's guard on the gh-body reading: a command with a +#: backslash, or a source with an unresolved printf conversion, keeps its +#: per-line push reading, since every escape decoder can make a quote bash +#: never prints (``\X27`` lowercased, a printf format's ``\0047``, echo's ``\47``). +#: Four more: only the command the shell runs gets the gh-body reading, since a +#: filter such as ``tr`` may rewrite a nested payload before its evaluator. +_PACKAGE_LINE_BUDGET = 33_020 #: Ceiling on any ONE file in the package. This is what the bound is really for -- #: a package total says nothing about a single file growing back into a second