diff --git a/emrg/tools/bash_tool.py b/emrg/tools/bash_tool.py index dc0a7a5d..e7350f38 100644 --- a/emrg/tools/bash_tool.py +++ b/emrg/tools/bash_tool.py @@ -240,6 +240,29 @@ def _translate_windows_heredocs(cmd: str) -> tuple[str, str | None]: # `git config` reads unless it writes: read flags, or no positional key. _GIT_CONFIG_READ_FLAGS = frozenset({"--get", "--get-all", "--get-regexp", "-l", "--list", "--get-urlmatch"}) +# The flags that write. Naming them explicitly is the difference between a +# decision about the flag and an accident of the positional rule below: `--add +# k v` blocks because it leaves two positionals, `--replace-all k v r` for the +# same reason — but `--unset k` leaves one and read as a read, `--edit` leaves +# none. Measured on master with git 2.50.1, six spellings wrote while read-only +# said ALLOW: `--unset`, `--unset-all`, `--edit`, `-e`, `--remove-section`, and +# the subcommand `edit`. The mirror image was also live: `git config get k` — a +# pure read in git's subcommand spelling — was refused as a write. +_GIT_CONFIG_WRITE_FLAGS = frozenset({"--unset", "--unset-all", "--add", + "--replace-all", "--rename-section", + "--remove-section", "--edit", "-e"}) +# git 2.46+ accepts the same operations as subcommands without the `--`. Both +# spellings must land on the same verdict; the lists are kept apart on purpose, +# because `set`/`unset`/`edit` in *command* position are writes while the same +# words as a config *key* are not our business. Only the one-word subcommands +# actually need this branch (`unset k` is still caught below by leaving two +# positionals); `git config edit` leaked precisely because its flag form has no +# argument to count, so the rule that saved the others was not a rule at all. +_GIT_CONFIG_WRITE_SUBCOMMANDS = frozenset({"set", "unset", "unset-all", "add", + "replace-all", "rename-section", + "remove-section", "edit"}) +_GIT_CONFIG_READ_SUBCOMMANDS = frozenset({"get", "get-all", "get-regexp", + "get-urlmatch", "list"}) # git global options that take a SEPARATE argument — the parser must skip both # the option and its value to find the verb (`git -C . checkout .`). _GIT_GLOBAL_WITH_VALUE = frozenset({"-C", "-c", "--exec-path", "--git-dir", @@ -895,12 +918,21 @@ def _shape_decided_verdict(verb: str, rest: list[str]) -> str | None: # (no positional argument at all) is the listing read. return None if not positional else verb if verb == "config": + if any(t in _GIT_CONFIG_WRITE_FLAGS for t in rest): + return verb if any(t in _GIT_CONFIG_READ_FLAGS for t in rest): return None - # `git config ` with no value is a read; `key=value` or `--set` - # writes. A lone positional key is ambiguous, so treat a single - # positional as a read and anything that looks like an assignment as - # a write. + # git 2.46+ spells the same operations as subcommands (`git config unset + # k`); the first positional decides which, and only a word that is one of + # them counts — a config key is not a subcommand. + subcommand = next(iter(positional), None) + if subcommand in _GIT_CONFIG_WRITE_SUBCOMMANDS: + return verb + if subcommand in _GIT_CONFIG_READ_SUBCOMMANDS: + return None + # `git config ` with no value is a read; `key=value` writes. A lone + # positional key is ambiguous, so treat a single positional as a read and + # anything that looks like an assignment as a write. if any("=" in t for t in positional): return verb return None if len(positional) <= 1 else verb diff --git a/tests/test_git_config_shapes.py b/tests/test_git_config_shapes.py new file mode 100644 index 00000000..8989c87d --- /dev/null +++ b/tests/test_git_config_shapes.py @@ -0,0 +1,138 @@ +"""`git config` decides by flag or subcommand — never by counting positionals. + +The defect these tests exist for (issue #1253, measured on master with git +2.50.1): six spellings of `git config` wrote while the read-only guard said +ALLOW — `--unset`, `--unset-all`, `--edit`, `-e`, `--remove-section`, and the +subcommand spelling `edit`. They leaked for the same reason: the config branch +asked "how many positional tokens are there?" instead of "what does this flag +do?". `git config --add k v` blocked only because it leaves two positionals and +`--unset k` leaves one, which is an accident of the rule, not a decision about +the flag — so the next write flag someone adds would leak the same way. + +The mirror image was live at the same time: `git config get k`, a pure read in +git's 2.46+ subcommand spelling, was refused as a write, because two positionals +also look like an assignment. + +Every case below is driven through `_check_sandbox(cmd, "read-only")` — the +guard's own entry point, in the tier where the question is asked — and the two +lists are asserted in BOTH directions, so a change that blocks everything or +allows everything fails rather than passing half the file. +""" + +from __future__ import annotations + +import pytest + +from emrg.tools.bash_tool import ( + _GIT_CONFIG_READ_SUBCOMMANDS, + _GIT_CONFIG_WRITE_FLAGS, + _GIT_CONFIG_WRITE_SUBCOMMANDS, + _check_sandbox, +) + +# ── the corpus ───────────────────────────────────────────────────────────── + +WRITES = [ + # flag spelling + "git config --unset foo.bar", + "git config --unset-all foo.bar", + "git config --edit", + "git config -e", + "git config --remove-section foo", + "git config --rename-section foo bar", + "git config --add foo.bar baz", + "git config --replace-all foo.bar baz", + "git config foo.bar baz", + "git config user.name x", + # subcommand spelling (git 2.46+) + "git config set user.name x", + "git config unset foo.bar", + "git config unset-all foo.bar", + "git config edit", + "git config remove-section foo", + "git config rename-section foo bar", +] + +READS = [ + "git config foo.bar", + "git config -l", + "git config --list", + "git config --get foo.bar", + "git config --get-all foo.bar", + "git config --get-regexp foo", + "git config get foo.bar", + "git config get-all foo.bar", + "git config list", +] + + +@pytest.mark.parametrize("cmd", WRITES) +def test_a_writing_git_config_shape_is_blocked(cmd: str) -> None: + allowed, reason, _ = _check_sandbox(cmd, "read-only") + assert allowed is False, f"{cmd!r} writes but read-only allowed it" + # The block must come from the git rule, not from something incidental + # (a redirect scan, a path rule) that would leave the shape undecided. + assert "git" in (reason or "").lower(), f"{cmd!r} blocked for the wrong reason: {reason!r}" + + +@pytest.mark.parametrize("cmd", READS) +def test_a_reading_git_config_shape_is_allowed(cmd: str) -> None: + allowed, reason, _ = _check_sandbox(cmd, "read-only") + assert allowed is True, f"{cmd!r} only reads but was refused: {reason!r}" + + +def test_both_spellings_of_one_operation_agree() -> None: + """`--unset k` and `unset k` are the same operation, so they share a verdict. + + This is the property that makes the flag list and the subcommand list one + decision rather than two, and it fails if only one of them is maintained. + """ + pairs = [ + ("git config --unset foo.bar", "git config unset foo.bar"), + ("git config --unset-all foo.bar", "git config unset-all foo.bar"), + ("git config --edit", "git config edit"), + ("git config --remove-section foo", "git config remove-section foo"), + ("git config --rename-section foo bar", "git config rename-section foo bar"), + ("git config --get foo.bar", "git config get foo.bar"), + ("git config --list", "git config list"), + ] + for flagged, spelled in pairs: + a = _check_sandbox(flagged, "read-only")[0] + b = _check_sandbox(spelled, "read-only")[0] + assert a == b, f"{flagged!r} -> {a} but {spelled!r} -> {b}" + + +def test_a_key_named_like_a_subcommand_still_reads() -> None: + """The subcommand test must look at position, not at the word. + + `git config --get unset` reads a key that *happens* to be spelled like a + write subcommand; it must stay allowed. A bare `unset` in command position + is the subcommand (and would be a write), which is the fail-closed reading + and is asserted in the corpus above — the two are told apart by where the + word sits, which is the whole point. + """ + for cmd in ("git config get", "git config --get unset", "git config --get get", + "git config list"): + allowed, reason, _ = _check_sandbox(cmd, "read-only") + assert allowed is True, f"{cmd!r} only reads but was refused: {reason!r}" + + +def test_a_wrapped_write_is_still_blocked() -> None: + """The recursion through `sh -c` must reach the config rule too.""" + allowed, reason, _ = _check_sandbox( + "sh -c 'git config --unset foo.bar'", "read-only") + assert allowed is False, f"a nested write was allowed: {reason!r}" + + +def test_the_lists_are_not_vacuous() -> None: + """The two-directional corpus above is only meaningful if the tables are real.""" + assert _GIT_CONFIG_WRITE_FLAGS + assert _GIT_CONFIG_WRITE_SUBCOMMANDS + assert _GIT_CONFIG_READ_SUBCOMMANDS + # The spellings this work was reported for must be in the tables, so a later + # "cleanup" that drops one is red here rather than silently permissive. + for flag in ("--unset", "--unset-all", "--edit", "-e", "--remove-section"): + assert flag in _GIT_CONFIG_WRITE_FLAGS, flag + for sub in ("unset", "edit", "set"): + assert sub in _GIT_CONFIG_WRITE_SUBCOMMANDS, sub + assert "get" in _GIT_CONFIG_READ_SUBCOMMANDS