diff --git a/CODESTYLE.md b/CODESTYLE.md index da7f86f5a..4ff129457 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -32,6 +32,7 @@ Each language defines a **clean-compile** verification: the combination of build ### Operations and Failure Messages - **Let the real operation be the check, not a prediction of it.** Attempt the read, the write, or the call itself, and turn its failure into a clean refusal. Do not probe first with an existence, type, or permission test. Where the operation could block or succeed on the wrong kind of target, judge what it opened instead. A probe is a second model of the operation. It misjudges each case its author did not foresee, such as a dangling link, a link loop, or a platform's own permission model. The operation cannot disagree with itself. +- **Classify a subprocess failure by which command failed and the status code it reports, never by the wording of its message.** That wording varies by tool version and locale, and a status code does not, even one printed inside the message. Where one command's status code cannot separate an expected failure from an unexpected one, split it into real operations, one per failure. - **A failure message states the error, the path or other target, and the remedy, and claims nothing about the state the failure left.** The error the operation raised is the one fact the code holds at that point. A description of what the failure left behind is a prediction. It reads false once an unforeseen state reaches the message, such as a file written in full that then fails to close. A remedy such as "fix what it names, then re-run" needs no such claim. ### Command-Line Arguments diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index e10c489f1..7e27b3005 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -179,7 +179,22 @@ text says, because the harm it covers was never in the text. A command naming git or gh that holds a carriage return is denied, since the shell reads one as part of a word. So is one holding a word that a backslash-newline splits. So is one holding an escaped backslash before a line break. The shell reads that as a line break where the hook - would join the lines, and as a continuation inside a double-quoted wrapper body. Two comments whose + would join the lines, and as a continuation inside a double-quoted wrapper body. Neither deny + reads a backslash-newline the outer shell keeps as text, where the hook can read it as a shell + running that text would. That is one inside single quotes in command text, including command + text in a `$(...)` substitution. It is also one ending a quoted heredoc's data line, unless the + delimiter line follows. It is not one inside `$'...'`, or single quotes within double quotes, a + backquote, or `${...}`. A Markdown hard line break in a quoted heredoc or a single-quoted body + is one. The single-quoted part must be its word's only quoted, escaped, or expanded part, with no + escaped word break just before it. Where the text names git, gh, or a push, it must also hold no + further quoting. Those names are read in the text as written, with its lines joined, and with + each one a line break. Each backslash-newline in that text must then sit in its command text or + end a comment there. The command is then read again as a shell running that text reads it, + joining the lines except where one ends a comment. It is also read with each one joined and with each one a line break, and denied where + any reading is. Every reading keeps each escaped backslash pair before a line break, which the + shell reads as one literal backslash. Nor does either read a single unquoted + backslash-newline between a command separator such as `&&` or `|` and a non-operator character, + which splits no word. Two comments whose apostrophes pair up as one quoted span are read as the shell reads them, as comments. The lines between them are then read as commands. Some words are read as they stand. They are a directory option's literal or double-quoted value where the guard can read its quoting, a commit option's double-quoted value, a diff --git a/host-setup/agent-safety/claude/gh-write-guard.py b/host-setup/agent-safety/claude/gh-write-guard.py index 9f49b25d4..99dfb7f68 100755 --- a/host-setup/agent-safety/claude/gh-write-guard.py +++ b/host-setup/agent-safety/claude/gh-write-guard.py @@ -4107,8 +4107,8 @@ def _push_arg_lists(cmd): _CD_BUILTINS = ("cd", "pushd", "popd", "chdir") _UNRESOLVED = object() -_SPLIT_WORD = re.compile(r"(?<=\S)\\\r?\n(?=\S)") -_ESCAPED_LINE_BREAK = re.compile(r"(?()"): + raise ValueError("partly quoted heredoc delimiter") + tag = next(t for t in heredoc.group(2, 3, 4) if t is not None) + quoted = heredoc.group(4) is None or "\\" in heredoc.group(0) + pending.append((tag, bool(heredoc.group(1)), quoted)) + i = heredoc.end() + elif ch == "#" and word_start: + end = cmd.find("\n", i) + if end < 0: + return n + ctx[end] = "comment" + i = end + elif word_start and _CASE_KEYWORD.match(cmd, i): + cases += 1 if cmd.startswith("case", i) else -1 if cases else 0 + i += 4 + else: + if ch == "(": + depth += 1 + elif ch == ")" and depth: + depth -= 1 + elif ch == ")" and closer and not cases: + return i + 1 + i += 1 + if closer: + raise ValueError("unterminated group") + return n + + command(0, None) + return ctx + + +_PLAIN_WORD_PART = re.compile(r"[\w./:=@%+,-]*") +_NESTED_QUOTING = ("'", '"', "`", "<<") + + +def _literal_span_kept(cmd, start, stop, word, inner): + """True where the readings of the `literal` text `cmd[start:stop]` show what a shell running it runs. + + A span in a word with any other quoted, escaped, or expanded part is never kept, since the shell runs the whole word. + Nor is one with an escaped word break just before it. + A span naming no git, gh, or push, as written, joined, or with line breaks, is kept whatever its own scan found. + Any other span is kept only where its own scan finished and it holds no quoting a further shell would unwrap. + Each backslash-newline in it must then sit in command text or end a comment there, as `inner` places it. + """ + if word: + head = cmd.rfind(" ", 0, start - 1) + 1 + for c in "\t\n;&|<>(": + head = max(head, cmd.rfind(c, 0, start - 1) + 1) + tail = _PLAIN_WORD_PART.match(cmd, stop + 1).end() + if ( + not _PLAIN_WORD_PART.fullmatch(cmd, head, start - 1) + or cmd[head - 2 : head - 1] == "\\" + or (tail < len(cmd) and cmd[tail] not in " \t\n;&|<>()") + ): + return False + text = cmd[start:stop] + if not any( + _names_git_or_gh(t) or _names_push(t) + for t in (text, text.replace("\\\n", ""), text.replace("\\\n", "\n")) + ): + return True + if inner is None or any(q in text for q in _NESTED_QUOTING): + return False + return all( + inner.get(m.end() - 1) in ("command", "comment") for m in _LINE_BREAK_RUN.finditer(text) + ) + + +def _line_break_reading(cmd): + """Read each backslash-newline in `cmd`, returning (denied, readings). + + `denied` is True where one in a context the shell joins splits a word, or follows an escaped backslash. + A split directly after an unquoted command separator such as `&&` or `|` splits no word, so it is left to the fold. + A `literal` one is not denied where `_literal_span_kept` keeps its span, and is read as any other one otherwise. + `readings` holds `cmd` read as a shell running a kept span may read it, or is empty where `cmd` holds none. + One reading takes each kept backslash-newline as the span's own scan places it. + A shell running the span reads one ending a comment there as a line break, and joins any other. + The other readings join each one, or read each one as a line break. + Each escaped backslash pair before a kept newline stays in all of them, read as one literal backslash. + A space parts those pairs from each line break a reading puts after them, so that break is not read again. + A scan that does not finish leaves every backslash-newline in a context the shell joins. + """ + try: + ctx = _line_break_contexts(cmd) + except (ValueError, RecursionError): + ctx = {} + spans = {} + edits = [] + for m in _LINE_BREAK_RUN.finditer(cmd): + nl = m.end() - 1 + odd = (nl - m.start()) % 2 == 1 + kind = ctx.get(nl) + if isinstance(kind, tuple): + _, start, stop, word = kind + if (start, stop) not in spans: + try: + inner = _line_break_contexts(cmd[start:stop]) + except (ValueError, RecursionError): + inner = None + kept = _literal_span_kept(cmd, start, stop, word, inner) + spans[start, stop] = (inner or {}) if kept else None + inner = spans[start, stop] + if inner is not None: + pairs = cmd[m.start() : nl - 1 if odd else nl] + broken = f"{pairs} \n" if pairs else "\n" + joined = pairs if odd else broken + ends_comment = inner.get(nl - start) == "comment" + edits.append( + (m.start(), nl + 1, joined, broken if ends_comment else joined, broken) + ) + continue + kind = None + if not odd: + return True, () + before, after = cmd[nl - 2 : nl - 1], cmd[nl + 1 : nl + 2] + if not before.strip() or not after.strip(): + continue + k = nl - 1 + while k and cmd[k - 1] in "();<>|&": + k -= 1 + glued = ( + kind == "command" + and m.start() == nl - 1 + and cmd[k : nl - 1] in _GLUED_SEPARATORS + and cmd[k - 1 : k] != "\\" + and after not in _SHELL_OP_CHARS + ) + if not glued: + return True, () + if not edits: + return False, () + readings = [] + for pick in (3, 2, 4): + out, at = [], 0 + for edit in edits: + out.append(cmd[at : edit[0]] + edit[pick]) + at = edit[1] + readings.append("".join(out) + cmd[at:]) + return False, tuple(dict.fromkeys(readings)) + + def _opaque_cd(toks): """True when something in the command may change the directory or the repository without a literal `cd`. @@ -8645,9 +8872,12 @@ def classify( "than as a space or a line break, so the guard cannot tell where its words end. " "Failing closed. Write the command with plain newlines." + _handoff(cmd) ) - if (_SPLIT_WORD.search(cmd) or _ESCAPED_LINE_BREAK.search(cmd)) and ( - _names_git_or_gh(cmd) or _names_push(cmd) - ): + denied, readings = ( + _line_break_reading(cmd) + if _LINE_BREAK_RUN.search(cmd) and (_names_git_or_gh(cmd) or _names_push(cmd)) + else (False, ()) + ) + if denied: return "deny", ( "This command has a word split by a backslash-newline, or an escaped backslash before " "a line break. The shell joins the first into one word the guard cannot read. It reads " @@ -8655,6 +8885,23 @@ def classify( "inside a double-quoted wrapper body. A hidden cd could redirect a push, or a split value add a " "subcommand. Write each word whole on one line." + _handoff(cmd) ) + for reading in readings: + dec, reason = classify( + reading, + cwd, + origin, + current_branch, + rules_lookup, + environ, + primary_checkout_lookup, + ref_resolver, + config_lookup, + ) + if dec == "deny": + return "deny", ( + "Read as a shell running its quoted text reads it, with a backslash-newline there " + "joined or read as a line break, this command is denied. " + reason + ) # Fold shell line-continuations so a multi-line Bash invocation, such as `gh pr merge 5 \ --admin`, parses as one command. # Only backslash-newline is joined, so a real newline between commands still separates them. cmd = re.sub(r"\\\r?\n", " ", cmd) @@ -9004,6 +9251,151 @@ def _nested_wrapper(body, depth): "deny", "an escaped backslash before a newline is no line continuation, so the next line is read", ), + ( + "gh pr comment 5 --body 'a\\\nb'", + "allow", + "a backslash-newline inside single quotes is literal, so it splits no word", + ), + ( + "gh pr comment 5 --body 'a\\\\\nb'", + "allow", + "an escaped backslash before a newline inside single quotes is literal too", + ), + ( + "gh pr create --title x --body \"$(cat <<'EOF'\nline one\\\nline two\nEOF\n)\"", + "allow", + "a Markdown hard line break in a quoted heredoc inside a substitution is data", + ), + ( + "git commit -F- <<'EOF'\nsubject\n\nline one\\\nline two\nEOF", + "allow", + "a line ending in a backslash in a quoted heredoc is data", + ), + ( + 'git commit -F- <<"EOF"\nline one\\\\\nline two\nEOF', + "allow", + "an escaped backslash ending a line in a double-quoted-delimiter heredoc is data", + ), + ( + "git add x &&\\\ngit commit -m y", + "allow", + "a line continuation glued to && splits no word", + ), + ( + "git log --oneline |\\\nhead -5", + "allow", + "a line continuation glued to a pipe splits no word", + ), + ( + "git add x &\\\n& git commit -m y", + "deny", + "a line continuation inside && joins the operator the guard reads apart", + ), + ( + "git status 2>\\\n&1", + "deny", + "a line continuation after a redirection operator still fails closed", + ), + ( + 'git commit -m "a\\\nb" --no-verify', + "deny", + "a backslash-newline inside double quotes splits a word, as the shell joins it there", + ), + ( + "git commit -F- < run_audit(tree, {"bin/Run_Backup": "100755"}, frozenset({"bin/Run_Backup"})) +def _probe_run(add: Any, commit: Any = None, control: Any = None) -> Any: + """A subprocess.run stand-in answering the invalid-byte probe's own git add of the bad name with `add`.""" + real_run = subprocess.run + + def run(cmd: list[str], *args: Any, **kwargs: Any) -> Any: + cwd = kwargs.get("cwd") + if control is not None and cmd == ["git", "add", "--", "control.txt"]: + return control(cmd) + if cmd[:3] == ["git", "add", "--"] and cwd and cmd[3].startswith("bad-"): + return add(cmd) + if commit is not None and cmd[:2] == ["git", "commit"] and "add invalid-utf8 name" in cmd: + return commit(cmd) + return real_run(cmd, *args, **kwargs) + + return run + + +def _fail(cmd: list[str]) -> Any: + raise subprocess.CalledProcessError(128, cmd) + + +class TestSelftestInvalidByteProbe(unittest.TestCase): + def _selftest(self, run: Any, windows: bool) -> tuple[int, str]: + out = io.StringIO() + with ( + mock.patch.object(audit, "_IS_WINDOWS", windows), + mock.patch.object(subprocess, "run", run), + contextlib.redirect_stdout(out), + ): + result = audit._selftest() + return result, out.getvalue() + + def _require_probe_file(self, text: str) -> None: + if "host cannot create it" in text or "no candidate byte" in text: + self.skipTest("this host cannot exercise the invalid-byte probe") + + def test_a_failing_add_of_the_probe_file_skips_on_windows(self) -> None: + result, text = self._selftest(_probe_run(_fail), windows=True) + self._require_probe_file(text) + self.assertIn("skip hub_tracked: non-UTF-8 filename (git cannot add it)", text) + self.assertEqual(result, 0, text) + + def _error_of(self, run: Any, windows: bool) -> subprocess.CalledProcessError: + out = io.StringIO() + with ( + mock.patch.object(audit, "_IS_WINDOWS", windows), + mock.patch.object(subprocess, "run", run), + contextlib.redirect_stdout(out), + ): + try: + audit._selftest() + except subprocess.CalledProcessError as error: + return error + self._require_probe_file(out.getvalue()) + self.fail("the git failure did not propagate") + + def test_a_failing_control_add_fails_loudly_on_windows(self) -> None: + error = self._error_of( + _probe_run(lambda cmd: subprocess.CompletedProcess(cmd, 0), control=_fail), + windows=True, + ) + self.assertEqual(error.cmd, ["git", "add", "--", "control.txt"]) + + def test_a_failing_add_of_the_probe_file_fails_loudly_elsewhere(self) -> None: + error = self._error_of(_probe_run(_fail), windows=False) + self.assertEqual(error.cmd[:3], ["git", "add", "--"]) + + def test_another_git_failure_still_fails_loudly(self) -> None: + run = _probe_run( + lambda cmd: subprocess.CompletedProcess(cmd, 0), commit=lambda cmd: _fail(cmd) + ) + error = self._error_of(run, windows=True) + self.assertEqual(error.cmd[:2], ["git", "commit"]) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_git_env.py b/tests/test_git_env.py index de0453bf8..1feb957b1 100755 --- a/tests/test_git_env.py +++ b/tests/test_git_env.py @@ -57,10 +57,6 @@ def test_a_variable_set_inside_the_context_does_not_outlive_it(self) -> None: self.assertNotIn("GIT_INDEX_FILE", os.environ) -@unittest.skipIf( - sys.platform == "win32", - "audit.py --selftest fails on Windows at its invalid-byte filename probe", -) class TestAuditSelftestIgnoresAmbientGitDir(unittest.TestCase): def test_the_selftest_leaves_the_ambient_repository_unchanged(self) -> None: with tempfile.TemporaryDirectory() as tmp: