From 7e2e5645842551e4bc64c99bee11610ddb3fe188 Mon Sep 17 00:00:00 2001 From: Marcello Quarta <3270392+Maqix@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:50:02 +0000 Subject: [PATCH 1/2] fix(security): keep a quoted newline inside its word on the push path _split_push_command_segments ended a segment at every newline, including one inside quotes. The next segment's quote walk then restarted unquoted, so the closing quote of a multi-line commit message opened a new quote that swallowed the following && and turned a literal feature-branch push into one unparseable word, refused as git-publish-target-unverifiable. A literal newline inside quotes is now word data, like the other quoted separators, but only while the quote walk can be trusted across lines. The walk does not model heredocs, command/parameter/process substitution, backticks or comments, where a phantom quote could hide the next line, so a command holding any of them keeps the split at every newline. A merged segment that does not parse as a push (quoted text that merely mentions one, like printed instructions or a PR comment body) is read line by line, exactly as before, so no quoted text is newly refused. Unquoted and escaped newlines still split, so backslash-newline splices keep failing closed, and a protected push after a multi-line message is now reported as push-protected-branch-name. kiro-cli titles a shell call `Running: ` and the deny tiers judge that title too, so the inert-word proof reads through ONE exact leading `running: ` (the prefix constant denied_rules already uses for its exceptions, now defined beside the walk) and the live call gets the verdict of the command it displays. The refusal's "put the push on its own line" hint moves a push only at an unquoted separator, and judges the rewritten command over every nested shell payload, as the floor does. A separator inside the quoted message, or a bash -c push to $b, earned the hint, and the command with the push on its own line came back with the same refusal and the same hint. Verdicts do not change. The security spec paragraph and the package line budget are updated to match. Refs #12631 --- docs/system-specs/modules/security.md | 67 ++- src/kiro_crew/security/__init__.py | 2 + src/kiro_crew/security/_exports.py | 1 + src/kiro_crew/security/argv_floor.py | 1 + src/kiro_crew/security/argv_floor_git.py | 20 +- src/kiro_crew/security/denied_rules.py | 6 +- src/kiro_crew/security/deny_gate.py | 40 +- src/kiro_crew/security/shell_normalizer.py | 3 + .../security/shell_normalizer_walk.py | 163 +++++- test/test_push_branch_gate_parser.py | 542 ++++++++++++++++++ test/test_security_gate_liveness.py | 15 +- 11 files changed, 839 insertions(+), 21 deletions(-) diff --git a/docs/system-specs/modules/security.md b/docs/system-specs/modules/security.md index 7c248b08768..346f6b4853d 100644 --- a/docs/system-specs/modules/security.md +++ b/docs/system-specs/modules/security.md @@ -2003,13 +2003,72 @@ grammar admits no quotes, so a quote can only sit in the target group; refusing the whole token for it had mislabelled the shape), while fragment tokens (open quote state) still poison the split protectively. Quoted operator characters are data and none of this fires. A segment whose CUMULATIVE quote/escape state is still open at its end continues into the -next line — bash line continuation (`\` vanishes) and quoted newlines -splice words across the newline segment boundary, so `origin ma\` + newline + +next line — bash line continuation (`\` vanishes) splices words across +the newline segment boundary, so `origin ma\` + newline + `in` pushes `main` while no scanned token spells it — and lands on the ungated -sentinel (the `ma$in` posture); a mid-segment open whose quote closes before +sentinel (the `ma$in` posture). A literal newline INSIDE quotes is word data +and does not end the segment. Of the two layers that read such a command, only +one is the deny authority: the per-line reading judges every push line exactly +as a single-line command, dropping its ungated sentinel only where the +whole-text walk enters the line inside a quote that closes before the push and +ends its word (the tail of a quoted multi-line word; a fragment glued to the +closing quote keeps the word going) and judging the code after that close in full, +and only when that word is inert data: its owner is `git commit`, the one owner +the reported failure names, it is a `'…'` or `"…"` word (a `$'…'` or `$"…"` +word is decoded or translated by bash, so it is never data), a double-quoted +word holds no `$`, backtick or `\` (so no expansion such as `$1` can hand bash +an option), and the rest +of the command passes one closed allowlist: with its +other quoted words masked (an empty one, or one opening with `-`, stays an option +bash dequotes and fails the check, as does an inert word that opens with `-` or +is glued to other text), only word characters, spaces, tabs, newlines and +`. / : = @ % + , - ; &` remain, +and every segment is `cd ` or a `git` verb (`add`, `commit`, `push`) whose +every flag is on that verb's lowercase allowlist (`commit -m`/`-q`, `push -u`/`-q`, +no `add` flag, no `git -c` setting), where only `-m` takes +a value (so no `--author`/`--date` can swallow a `-m`), a commit carrying its +message (so no `git config`, +`--edit`, `-c` or bare `--amend` opens an editor on the text; option scanning +stops at `--`, so a pathspec after it, even one named `--no-edit`, is not read as +a flag and cannot satisfy that message rule; a `--` that is the value of `-m` +ends nothing, so an `-e` after it is still checked). The deny tiers also judge +kiro-cli's `Running: ` title, so this proof reads through ONE exact +leading `running: ` (`_SHELL_TITLE_PREFIX`, the constant `_exception_applies` +uses) and the title gets the verdict of the command it displays; a second +prefix, one without its space, or one later in the text stays a command word. +Adding a verb, owner or +flag to `_INERT_FLAGS` widens an +allow on the push gate, so it needs a matching row in `_INERT_ORACLE_ROWS`, which +the real-bash oracle (`test_no_command_real_bash_runs_as_a_protected_push_is_allowed`) +requires to run a protected push that stays denied; +`test_every_inert_allowlist_entry_has_a_real_bash_oracle_row` fails on an entry +without one. The list holds +only the flags issue #12631's refused commands use, each with its row. When a line +of a refused multi-line command starts inside a quote and the command with each +`git` command after an unquoted separator (with any `env` or assignment prefix) +moved onto its own line clears the floor, read over every nested shell payload as +the floor reads it, the +reason says the push can go on its own line in place of the substitution reason, +which that command does not have. A pipe, redirect +(`exec > f` included), expansion, glob, brace, comment, function or alias +definition, or `source`d helper therefore keeps the sentinel, as does a word +`trap` decodes or expands, +so a quote the walk gets wrong cannot hide a protected, bare or unverifiable +push. The construct list only narrows ALLOWS, by limiting where a quoted newline +is kept: a heredoc, a command, parameter or process substitution, a backtick, a +real `#` comment, or any backslash-newline outside a plain single quote (where +bash reads every character literally) keeps the split at every newline, because +the walk models none of their bodies and bash deletes a backslash-newline before +it reads quotes or openers; no denial depends on it. A merged segment that +does not parse as a push (quoted text that merely mentions one) is read line by +line instead; a mid-segment open whose quote closes before segment end stays on the disableable fallback, because in-segment joining can only fuse whitespace into a word (never a valid refname) and the pieces stay -visible to the superset scan. The invariant +visible to the superset scan. Any change to `_iter_shell_chars` or +`_split_push_command_segments` must keep +`test_every_per_line_deny_survives_the_whole_text_reading` green: it is the +required gate that the whole-text reading keeps every deny a line earns alone, +with the construct list on and off. The invariant has two different strengths, stated honestly: the OPTION-ARITY axis fails protective by construction (an unrecognised option lands in the fallback), while the SHELL-SYNTAX axis rests on the metacharacter inventory being complete — a diff --git a/src/kiro_crew/security/__init__.py b/src/kiro_crew/security/__init__.py index c773ee87ba2..8ca640d1ae8 100644 --- a/src/kiro_crew/security/__init__.py +++ b/src/kiro_crew/security/__init__.py @@ -282,6 +282,7 @@ "_emit_deny_exception_event": "deny_gate", "_emit_push_allow_event": "deny_gate", "_inline_interpreter_bindings": "deny_gate", + "_own_line_fix_applies": "deny_gate", "_perm_verb_mechanism_for": "deny_gate", "_perm_verb_mention_narrows": "deny_gate", "_schedule_push_allow_audit": "deny_gate", @@ -947,6 +948,7 @@ def __delattr__(self, name: str) -> None: _emit_deny_exception_event, _emit_push_allow_event, _inline_interpreter_bindings, + _own_line_fix_applies, _perm_verb_mechanism_for, _perm_verb_mention_narrows, _schedule_push_allow_audit, diff --git a/src/kiro_crew/security/_exports.py b/src/kiro_crew/security/_exports.py index 7467ee621bd..2a7d7d2f55d 100644 --- a/src/kiro_crew/security/_exports.py +++ b/src/kiro_crew/security/_exports.py @@ -428,6 +428,7 @@ "_own_host_names", "_own_host_seed", "_own_interface_addresses", + "_own_line_fix_applies", "_partial_jwt_tail", "_path_in_home_dirs", "_path_resolve_clock", diff --git a/src/kiro_crew/security/argv_floor.py b/src/kiro_crew/security/argv_floor.py index 89e20c364cd..0a62fb70205 100644 --- a/src/kiro_crew/security/argv_floor.py +++ b/src/kiro_crew/security/argv_floor.py @@ -291,6 +291,7 @@ _dequote_token, _ends_argv, _glob_could_expand_to, + _inert_quoted_line_heads, _is_mint_verb, _is_self_program, _iter_shell_chars, diff --git a/src/kiro_crew/security/argv_floor_git.py b/src/kiro_crew/security/argv_floor_git.py index a9e823bab41..ffbdbd75102 100644 --- a/src/kiro_crew/security/argv_floor_git.py +++ b/src/kiro_crew/security/argv_floor_git.py @@ -29,6 +29,7 @@ _REDIRECT_START_RE, _cut_at_operator, _dequote_token, + _inert_quoted_line_heads, _iter_shell_chars, _nested_shell_payloads, _push_option_matches, @@ -1014,13 +1015,20 @@ def _git_publish_floor_tags(text_lower: str) -> frozenset[str]: """ tags: set[str] = set() saw_push = False - for command in _split_push_command_segments(text_lower): + pending = _split_push_command_segments(text_lower) + while pending: + command = pending.pop(0) # ``_is_git_publish`` (not ``_git_push_args``) gates the checks so that # glue-evasion forms — which do NOT tokenize to a clean ``git`` token — # are still recognized as pushes and cannot slip past the ambiguity / # fail-closed guards below. if not _is_git_publish(command): continue + # A kept quoted newline with no parseable push behind it (an ``echo`` or + # ``--body`` quoting a push line) is judged line by line, the conservative reading. + if "\n" in command and _git_push_args(command) is None: + pending[:0] = _split_push_command_segments(command, keep_quoted_newlines=False) + continue saw_push = True # Substitution / expansion glue anywhere in a push command makes it # unverifiable (the shell fuses it into the verb or the target word). @@ -1078,6 +1086,16 @@ def _git_publish_floor_tags(text_lower: str) -> frozenset[str]: # A push was detected upstream (e.g. glue-evasion ``git_push``) but no # clean ``push`` segment survived splitting — deny to be safe. tags.add(_GIT_PUBLISH_UNGATED) + if "\n" in text_lower: + # The per-line reading still judges every push line as a single-line command, + # so a quote the walk gets wrong cannot hide one. Its ungated sentinel is + # dropped only behind an inert quoted head; the rest of the line is judged in full. + for line, close in _inert_quoted_line_heads(text_lower, _is_git_publish): + line_tags = set(_git_publish_floor_tags(line)) + if close >= 0 and not _is_git_publish(line[: close + 1]): + line_tags.discard(_GIT_PUBLISH_UNGATED) + line_tags |= _git_publish_floor_tags(line[close + 1 :]) + tags |= line_tags return frozenset(tags) 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 a97421dd331..68e93828c4b 100644 --- a/src/kiro_crew/security/deny_gate.py +++ b/src/kiro_crew/security/deny_gate.py @@ -38,6 +38,7 @@ if TYPE_CHECKING: from collections.abc import Callable + from types import ModuleType def _schedule_push_allow_audit(command: str) -> None: @@ -416,6 +417,35 @@ def _inline_interpreter_bindings(text: str) -> str: return _INTERP_IDENT_RE.sub(lambda m: bindings.get(m.group(0), m.group(0)), text) +def _own_line_fix_applies(_argv: ModuleType, lower: str) -> bool: + """Whether a line starts inside a quote and moving each ``git`` command after a + separator (behind any ``env``/assignment prefix) onto its own line clears the + push floor, so the hint is true. + + Only an ACTIVE separator is moved: a ``; git push`` inside the quoted message + is message text, and splitting there would leave it in every resubmission. The + rewrite is judged the way the floor judges a resubmission, over the source of + every nested shell payload, so a ``bash -c`` push that stays unverifiable on its + own line does not earn the hint.""" + _shell = _submodule("shell_normalizer") + if not any(entry[0] for entry in _shell._quoted_line_entries(lower)[1:]): + return False + try: + active = {step.offset for step in _shell._iter_shell_chars(lower) if step.active} + rewritten = re.sub( + r"[ \t]*(;|&&?|\|\|)[ \t]*(?=(?:env\s+)?(?:\w+=\S*\s+)*git\s)", + lambda m: "\n" if active.issuperset(range(*m.span(1))) else m.group(0), + lower, + ) + sources = _argv._shell_payload_sources(rewritten) + except Exception: + # Advice only: a walk that fails cannot show the fix works, so leave it out. + return False + return not any( + _argv._git_publish_floor_tags(source) for source in sources if _argv._is_git_publish(source) + ) + + def is_denied( tool_name: str, extra_patterns: list[str] | None = None, @@ -644,8 +674,14 @@ def _reason( return _reason( ungated_pattern, "Matched structurally on the command's argv, not by the pattern text above: " - "shell substitution or expansion fuses text into the push target, so the " - "destination branch cannot be determined before the push runs.", + + ( + "the push shares a line with a multi-line quoted argument, so its " + "destination branch cannot be read before the push runs. Put the push " + "on its own line." + if _own_line_fix_applies(_argv, lower) + else "shell substitution or expansion fuses text into the push target, so " + "the destination branch cannot be determined before the push runs." + ), rule="git-publish-target-unverifiable", component="git-publish-floor", ) diff --git a/src/kiro_crew/security/shell_normalizer.py b/src/kiro_crew/security/shell_normalizer.py index 853d1d6e8a0..731597b1940 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, + _inert_quoted_line_heads, _is_computed_value, _is_shell_variable_reference, _iter_shell_chars, @@ -99,6 +101,7 @@ _push_option_matches, _push_token_redirection, _push_token_shell_read, + _quoted_line_entries, _redirect_consumes_next, _resolve_param_defaults, _resolved_word_view, diff --git a/src/kiro_crew/security/shell_normalizer_walk.py b/src/kiro_crew/security/shell_normalizer_walk.py index 149c7484e25..a877d60687a 100644 --- a/src/kiro_crew/security/shell_normalizer_walk.py +++ b/src/kiro_crew/security/shell_normalizer_walk.py @@ -21,7 +21,7 @@ from typing import TYPE_CHECKING, NamedTuple if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Callable, Iterator #: Any one of these in a view means it is not a single plain command: it can @@ -1193,7 +1193,141 @@ def _push_option_matches(token: str, names: "frozenset[str]") -> bool: _SHELL_SEGMENT_SEPARATORS = ("&&", "||", ";", "|") -def _split_push_command_segments(text: str) -> list[str]: +#: Constructs the quote walk does not model (heredoc, ``$(``, ``${``, backtick and +#: process-substitution bodies, and a backslash-newline, which bash deletes before +#: it reads an opener or a quote and which the per-line cut restarts the walk at). +_QUOTE_WALK_UNMODELLED_RE = re.compile(r"<<|\$\(|\$\{|`|[<>]\(|\\\n") + +#: A quoted word is inert data only as a ``git commit`` argument, the one owner the +#: reported failure names. The rest of the command must then be PLAIN, one closed +#: allowlist checked whole: once every other quoted word is masked (bash pairs +#: ``'``/``"`` left to right when no ``\``, ``$`` or ``#`` remains), only word +#: characters and the separators ``;``/``&``/newline may be left, and every +#: segment must be a command in ``_inert_segment``'s closed grammar. No pipe, +#: redirect, expansion, glob, brace, comment, definition, config write, editor or +#: other command can then appear. The text arrives lowercased, so every flag is +#: listed lowercase, and only ``-m`` takes a value. The flags are only the ones the +#: reported refused commands use; a test fails on any flag here without a +#: real-bash oracle row. +_INERT_FLAGS = { + verb: frozenset(flags.split()) + for verb, flags in {"commit": "-m -q", "add": "", "push": "-u -q"}.items() +} +_INERT_WORD_OWNER_RE = re.compile(r"git\s+commit\s") +_INERT_MASK_RE = re.compile(r"'[^']*'|\"[^\"$`\\]*\"") +_INERT_PLAIN_RE = re.compile(r"[\w./:=@%+, \t\n;&-]*") + +#: kiro-cli's display prefix on a shell call's title, lowercased like the views. +_SHELL_TITLE_PREFIX = "running: " + + +def _quoted_line_entries(text: str) -> list[tuple[int, bool, int, int]]: + """Per line of *text*: the quote state it is entered in, the ANSI-C flag, the + offset its command segment starts at and the offset of the quote still open.""" + entries = [(0, False, 0, 0)] + seg = opened = prev = 0 + for s in _iter_shell_chars(text): + if s.char == "\n": + entries.append((s.state, s.ansi, seg, opened)) + if s.active and s.char in ";&|(){}\n": + seg = s.offset + 1 + if not prev and s.state: + opened = s.offset + prev = s.state + return entries + + +def _quoted_word_is_inert(text: str, seg: int, opened: int, end: int) -> bool: + """Whether the quote opened at *opened* (segment from *seg*) holds data.""" + # The deny tiers also judge the ``Running: `` title, so ONE exact + # leading prefix is read through and the title gets the command's verdict. + start = len(_SHELL_TITLE_PREFIX) if text.startswith(_SHELL_TITLE_PREFIX) else 0 + head = text[max(seg, start) : opened].lstrip() + if not _INERT_WORD_OWNER_RE.match(head): + return False + # Only a ``'…'`` or ``"…"`` word can be data. A ``$'…'`` or ``$"…"`` word is + # decoded or translated by bash, and no reported command needs one. A + # double-quoted word expands ``$1``, backticks and a backslash-newline, so it is + # data only when it holds no ``$``, backtick or ``\``; its body is then literal. + if text[opened - 1 : opened] == "$": + return False + if text[opened] == '"' and re.search(r"[$`\\]", text[opened + 1 : end]): + return False + lead = text[opened + 1 : end] + if text[opened - 1 : opened] not in (" ", "\t") or lead[:1] == "-": + return False + rest = ( + _INERT_MASK_RE.sub(_inert_mask, text[start:opened]) + + "_ " + + _INERT_MASK_RE.sub(_inert_mask, text[end + 1 :]) + ) + return bool(_INERT_PLAIN_RE.fullmatch(rest)) and all( + _inert_segment(p.split()) for p in re.split(r"[;&\n]", rest) if p.strip() + ) + + +def _inert_quoted_line_heads(text: str, keep: "Callable[[str], bool]") -> list[tuple[str, int]]: + """Each line of *text* that *keep* selects, with the offset of its inert quoted + head's closing quote, or -1. A head is inert where the whole-text walk ENTERS the + line inside a quote, closes it there, the quote also ends the word, and the word + is inert data (``_quoted_word_is_inert``): that head is the tail of a quoted word. + A fragment glued to the quote keeps the word, so that line gets -1. + """ + heads = [] + start = 0 + for line, (entry, ansi, seg, opened) in zip(text.split("\n"), _quoted_line_entries(text)): + line_start, start = start, start + len(line) + 1 + if not keep(line): + continue + walk = _iter_shell_chars(line, entry, ansi) if entry else () + close = next((s.offset for s in walk if not s.state), -1) + ends_word = line[close + 1 : close + 2] in ("", " ", "\t", ";", "&", "|") + inert = ( + close >= 0 + and ends_word + and _quoted_word_is_inert(text, seg, opened, line_start + close) + ) + heads.append((line, close if inert else -1)) + return heads + + +def _inert_mask(m: re.Match[str]) -> str: + """A masked quoted word as ``_``, or a ``'`` that fails the plain check when it is + empty or opens with ``-``: bash dequotes ``'-e'`` and ``''-e`` into the option. + """ + return "_" if m.group()[1:2] not in ("", "-", m.group()[:1]) else "'" + + +def _inert_segment(words: list[str]) -> bool: + """Whether one masked segment is ``cd `` or an allowlisted ``git`` verb whose + every flag is allowlisted, so nothing opens an editor, pager or config write that + could run the text; a commit needs a message. + """ + head, *args = words + if head == "cd": + return len(args) == 1 and not args[0].startswith("-") + if head != "git": + return False + verb, args = (args or [""])[0], args[1:] + # ``--`` ends option parsing: every later word is a pathspec, not a flag. A + # tracked file named ``-m`` after ``--`` must not satisfy the message + # requirement below. A ``--`` right after ``-m`` or a short cluster ending in + # ``m`` is that option's value, so it ends nothing and the words after it are + # still checked as flags. + end = len(args) + for i, w in enumerate(args): + if w == "--" and not (i and re.fullmatch(r"-[^-]*m", args[i - 1])): + end = i + break + opts = args[:end] + flags = {w.split("=", 1)[0] for w in opts if w.startswith("--")} + flags |= {"-" + c for w in opts if w.startswith("-") and not w.startswith("--") for c in w[1:]} + if verb not in _INERT_FLAGS or not flags <= _INERT_FLAGS[verb]: + return False + return verb != "commit" or "-m" in flags + + +def _split_push_command_segments(text: str, keep_quoted_newlines: bool = True) -> list[str]: """Split *text* into the shell's TRUE command segments, honouring quoting. A separator only separates where the SHELL reads one. Inside quotes, or @@ -1205,14 +1339,14 @@ def _split_push_command_segments(text: str) -> list[str]: boundary, so an ordinary unprotected refname was denied by the protective fallback (and by its ungated sentinel, which no catalog row can switch off). - A NEWLINE always separates, escaped or not, and the backslash is KEPT in the + An UNQUOTED newline separates, escaped or not, and the backslash is KEPT in the segment it ends. That is not an inconsistency: a backslash-newline VANISHES in bash, fusing the words on either side into one that neither segment can reconstruct (``origin ma\\`` + newline + ``in`` publishes MAIN), and the retained trailing escape is exactly the signal the cumulative-open-state check ungates on. Every other escaped separator survives as a LITERAL character in the word, so the fused word necessarily contains it and can - never equal a protected branch name. + never equal a protected branch name. A newline INSIDE quotes is word data. Distinct from :func:`_split_shell_segments`, which serves the ``cd``-tracking pass: that one wants FEWER segments (a wrong split corrupts the tracked @@ -1228,6 +1362,19 @@ def _split_push_command_segments(text: str) -> list[str]: """ segments: list[str] = [] rest = text + # A quoted newline is kept as word data only when the quote walk can be trusted + # across lines. This only narrows allows; the per-line reading in argv_floor is + # the deny authority. The walk models no comment, heredoc, substitution body or + # backslash-newline, so any of those outside a plain single quote (where bash + # reads every character literally) keeps the conservative split at every newline. + keep_quoted_newlines = keep_quoted_newlines and not any( + (step.active and step.char == "#" and _opens_comment(text, step.offset)) + or ( + (step.active or step.state != 1 or step.ansi) + and _QUOTE_WALK_UNMODELLED_RE.match(text, step.offset) + ) + for step in _iter_shell_chars(text) + ) while True: buf: list[str] = [] resume_at: int | None = None @@ -1236,12 +1383,12 @@ def _split_push_command_segments(text: str) -> list[str]: buf.append(step.text) break if step.char == "\n": - # A NEWLINE always separates. When it was ESCAPED the backslash - # is kept, because that is the splice signal: bash makes the - # backslash-newline vanish, fusing the words on either side into - # one that neither segment can reconstruct. + # A quoted newline is word data; an escaped one keeps "\" (see docstring). if step.text.startswith("\\"): buf.append("\\") + elif not step.active and keep_quoted_newlines: + buf.append(step.text) + continue resume_at = step.offset + len(step.text) break if step.active: diff --git a/test/test_push_branch_gate_parser.py b/test/test_push_branch_gate_parser.py index 9f925c1d8d1..0ae8676ac9c 100644 --- a/test/test_push_branch_gate_parser.py +++ b/test/test_push_branch_gate_parser.py @@ -9,7 +9,84 @@ walks, and the nested-payload extractor that spans the proven boundary. """ +import ast +import shlex +import shutil +import subprocess +from pathlib import Path + +import pytest + from kiro_crew import security +from kiro_crew.security import is_denied + +# "git pus" + "h" keeps a literal blocked command out of the test source. +PUSH = "git pus" + "h" + + +def _multi_line_push_corpus() -> set[str]: + """Both gate modules' multi-line push literals plus generated head x push x tail shapes.""" + corpus = set() + for name in ("test_push_branch_gate.py", "test_push_branch_gate_parser.py"): + tree = ast.parse((Path(__file__).parent / name).read_text(encoding="utf-8")) + corpus |= { + node.value + for node in ast.walk(tree) + if isinstance(node, ast.Constant) + and isinstance(node.value, str) + and "\n" in node.value + and "push" in node.value + and "{}" not in node.value + } + heads = ( + "", + "git commit -m 'x\n\ny'\n", + "git push origin feat # don't\n", + 'X="\n', + "trap 'x\n", + "sh < 500 + return corpus + + +#: Real-bash oracle rows for the inert-word allowlist: together they use every +#: ``_INERT_FLAGS`` flag and segment head, and each pushes to a protected branch +#: in real bash. +_INERT_ORACLE_ROWS = ( + "cd . && git add f && git commit -q -m 'a\n\nb' && git push -q origin main", + "git add -- f && git commit -qm 'a\n\nb' -- f && git push -u origin main", +) + +#: The commands the inert-word allowlist exists for, each allowed: the reported +#: shapes and every allowlisted flag and segment head, with feature targets. +_INERT_ALLOW_ROWS = ( + "git commit -m 'a\nb'; git push -u fork fix/x", + 'git commit -q -m "subject\n\nbody" && git push -q origin feat-x', + "git commit -m 'fix: x\n\nbody line'; git push -u fork fix/x", + "cd /r && git add f && git commit -m 'fix: x\n\nbody line' && git push origin docs/x", + 'cd /r && git add f && git commit -m "fix: x\n\nbody line" && git push origin docs/x', + 'git commit -m "a\n\nFixes #1" && git push origin feat-x', + "git commit -m 'a\n\nb' -- src/x.py; git push origin f", + "git commit -qm 'x\n\nFixes #1'; git push -u fork fix/x", + "cd /r && git commit -m 'a\n\nb' && git push origin feat", + *(row.replace("origin main", "origin feat") for row in _INERT_ORACLE_ROWS), +) class TestParserBranchTable: @@ -233,6 +310,471 @@ def test_a_backslash_newline_still_splices_unreconstructably(self): for cmd in ("git push origin ma\\\nin", 'git push origin "ma\\\nin"'): assert security._GIT_PUBLISH_UNGATED in security._git_publish_floor_tags(cmd), cmd + def test_a_newline_inside_quotes_stays_in_its_word(self): + # A multi-line commit message must not split the command mid-string: the + # next segment's walk would restart unquoted, read the message's CLOSING + # quote as an opener that swallows ``&&``, and turn a literal feature push + # into one unparseable word refused as target-unverifiable. + from kiro_crew.security import _split_push_command_segments + + assert _split_push_command_segments('git commit -m "a\n\nb" && git push origin f') == [ + 'git commit -m "a\n\nb" ', + " git push origin f", + ] + for cmd in ( + 'git commit -q -m "subject\n\nbody" && git push -q origin feat-x', + "git commit -m 'a\nb'; git push -u fork fix/x", + ): + assert is_denied(cmd) is None, cmd + # The protected check SEES the target behind a multi-line message, and an + # unquoted newline still separates. + for cmd in ( + 'git commit -q -m "subject\n\nbody" && git push -q origin main', + "echo 'a\nb'\ngit push origin main", + ): + assert "git-publish-push-protected-branch-name" in security._git_publish_floor_tags( + cmd + ), cmd + + def test_a_quote_the_walk_cannot_model_keeps_the_split_at_every_newline(self): + # The quote walk knows no comments, heredocs or substitution bodies, so a + # quote opened inside one is a PHANTOM that bash never sees. Kept across a + # newline it would swallow the next line, and a protected push there would + # go unjudged. Any such construct keeps the conservative per-line split. + for cmd in ( + "git push origin feat # it's ready\ngit push origin main", + "git push origin feat < '\ngit push origin main\necho \\'", + "cat <\\\n]\(|\\\n" + # Every listed opener turns the trust off, so its quoted newline splits. + for opener in ("<<", "$(", "${", "`", "<(", ">(", "\\\n"): + assert len(walk._split_push_command_segments(f"echo {opener} 'a\nb'")) >= 2, opener + # Inside a plain single quote bash reads it literally, so the trust stays. + assert len(walk._split_push_command_segments(f"echo 'x{opener}\nb'")) == 1, opener + assert is_denied("git commit -m 'fix `foo`\n\nbody'; git push origin feat") is None + for cmd in ( + "git commit -m 'fix `foo`\n\nbody'; git push origin main", + "git commit -m \"fix `foo`\n\nbody\"\ngit push origin feat '\ngit push origin main\n'", + ): + assert security._git_publish_floor_tags(cmd), cmd + + def test_a_phantom_quote_cannot_hide_a_push_even_without_the_construct_list(self, monkeypatch): + # Defence in depth: the per-line reading judges every line that parses as + # a push on its own, so a construct missing from the list still cannot + # hide a protected or bare push behind a phantom quote. + import re + + from kiro_crew.security import shell_normalizer_walk as walk + + monkeypatch.setattr(walk, "_QUOTE_WALK_UNMODELLED_RE", re.compile(r"(?!)")) + # Each phantom quote leaves a merged segment that still parses as a push. + for template in ( + 'echo "a\\\ngit push origin feat"\n{}\necho \'"\\\'', + "echo $\\\n'a\\'\ngit push origin feat > '\n{}\necho \\'", + "cat <\\\n