From 82cad3ae024dd6b197100f13e45e388417c5ddbf Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:07:39 +0900 Subject: [PATCH 01/11] fix: deny shell patterns that expand onto the protected directory The Bash credential guard looked for six consecutive literal characters, so any metacharacter placed inside the directory name broke the match while the shell still expanded the pattern onto the real files. Against a throwaway HOME with bash 5.2, every one of these printed the credentials file and was allowed by the guard: cat ~/.mure?/credentials.json cat ~/.[m]ureo/credentials.json cat ~/.mur*/credentials.json cat ~/.m?reo/credentials.json cat ~/.?????/credentials.json cat ~/.[!.]*/credentials.json cat ~/.mure{o,x}/credentials.json This is not a regression from the boundary rule: the earlier bare substring check missed them too. The wildcard closed back in #393 was only the one *after* the name (`cat ~/.mureo/cred*`), and the docstring's claim that the guard "catches wildcard forms" made a partial fix read as a complete one. No regex over the command text can decide this, because the string that reaches the filesystem does not exist yet. So the guard now asks the question from the other side: it takes the path components of the command, keeps those that begin at a component boundary with a literal `.` and contain a metacharacter, and denies when fnmatch says the pattern matches `.mureo`. Requiring the literal leading dot is what makes that safe. A shell will not let a wildcard match the leading period of a filename unless dotglob is set, so `ls *` and `rm -rf build/*` cannot reach the directory and are never candidates -- without that restriction the rule would have to deny every glob anyone types. Quoted spans are skipped, because quoting suppresses pathname expansion: `sed 's/.*//'` is a regex, not a glob. Patterns spliced in by a substitution (`$D.mure?`, `printf '%s.mure?/'`) go through the same `$`/`%` clause the literal rule uses. Measured with the generated hook command run through bash: 73 everyday git/gh/ls/rm/grep/find/pytest/ruff/python commands, none denied; the 51 forms the guard must deny (the existing corpus plus the patterns above), all denied; the 10 mureo identifiers of #567 (window.MUREO_*, pkgs.mureo.jp), all still allowed. The docstring now states what the rule covers and what it does not: patterns the command text does not contain, patterns for sibling names, extended globs, quoting split inside a component, and dotglob shells. The payload gained chr(33) for `!`, which the pattern character class needs; a shell with history expansion on would rewrite `!` sequences inside the double quotes, so the test helper now rejects it alongside `$`, backticks, backslashes and newlines. --- mureo/credential_guard.py | 91 +++++++++++++++++++++++++++++++--- tests/hook_guard_runner.py | 6 +-- tests/test_credential_guard.py | 89 +++++++++++++++++++++++++++++++++ 3 files changed, 175 insertions(+), 11 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index db65cb36..a3e4eba9 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -23,10 +23,14 @@ ``~/.mureo/credentials.json`` that is itself a symlink pointing OUT — its realpath escapes the dir, but the requested path is still under it). Both cover every file in the directory, not just ``credentials.json``. -* Bash guard: denies any command whose text references ``.mureo`` where a - path component could *start*. A substring check is all a command string - allows, but anchoring on the directory name (not ``credentials``) also - catches wildcard forms like ``cat ~/.mureo/cred*``. +* Bash guard: two rules over the command text; either one denies. + + Rule 1 (the name spelled out) denies any command whose text contains + ``.mureo`` where a path component could *start*. Anchoring on the + directory name rather than on ``credentials`` also covers a wildcard + that follows the name (``cat ~/.mureo/cred*``) — but only because the + six characters of the name are still there verbatim. Rule 2 below is + what covers a wildcard placed *inside* the name. A bare substring test over-blocks badly, because case-folded ``.mureo`` is also a prefix of things that are emphatically not the directory: @@ -62,6 +66,56 @@ this: sibling directories (``~/.mureoX``, ``~/.mureo_backup``) still deny, since only the text before the name is consulted. + Rule 2 (the name written as a pattern) closes the other half. A + metacharacter placed inside the name breaks rule 1's six-character + literal while the shell still expands the pattern onto the real + directory: ``cat ~/.mure?/credentials.json`` prints the credentials + file, and so do ``.[m]ureo``, ``.mur*``, ``.m?reo``, ``.?????``, + ``.[!.]*`` and the brace form ``.mure{o,x}`` (checked against bash 5.2 + with a throwaway ``HOME``). No regex over the command text can decide + this, because the string that reaches the filesystem does not exist + yet — so the guard asks the question the other way round. It takes the + path components of the command, keeps those that begin at a component + boundary with a literal ``.`` and contain a metacharacter, and denies + when ``fnmatch`` says the pattern matches ``.mureo``. + + Requiring the literal leading ``.`` is what makes that safe to do. A + shell will not let a wildcard match the leading period of a filename + unless ``dotglob`` is set, so ``ls *``, ``rm -rf build/*`` and + ``tests/*.py`` cannot reach ``.mureo`` and are never candidates. + Without that restriction the rule would have to deny every glob anyone + types, ``fnmatch('.mureo', '*')`` being true. + + Quoted spans are skipped for this rule, because quoting suppresses + pathname expansion: ``sed 's/.*//'`` and ``find . -name '.*'`` are a + regex and a literal, not globs, and ``cat "$HOME/.mure?/x"`` opens + nothing. Unquoted, those same characters do glob, and are denied. A + pattern spliced in by a substitution (``$D.mure?``, ``printf + '%s.mure?/'``) is caught by the same ``$``/``%`` clause rule 1 uses, + where quoting is not consulted: the pattern reaches the shell through a + later expansion. Brace groups are replaced by ``*`` before matching, + an over-approximation that also denies things like ``mv .{env,bak}`` + which cannot name the directory — the safe direction. + + What rule 2 does not cover, and no part of the guard claims to: + + - a pattern the command text does not contain, because a variable set + by an earlier command or by another program supplies it (``cat + ~/$P/x``). Written out in the same command, ``P=.mure?; cat ~/$P/x`` + is denied — the pattern is in the text; + - patterns for *sibling* names (``~/.mur*_backup``): rule 2 asks only + whether the pattern matches ``.mureo`` itself, whereas rule 1 does + deny literal siblings such as ``~/.mureo_backup``; + - extended globs (``.mure@(o|x)``), which bash has off by default; + - quoting is scanned as balanced pairs and each quoted span is dropped + whole, rather than parsed as a shell would. A pattern split across + the quoting (``~/'.'mure?/x`` — the quoted dot still counts as + explicit to the shell) and one hidden behind a deliberately + unbalanced quote earlier in the line both escape this half of the + rule; + - shells configured with ``dotglob`` or ``GLOBIGNORE``, where ``*`` + does reach dotfiles. + Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the real file. On case-sensitive filesystems this can only over-block (a @@ -74,8 +128,11 @@ NOTE: the python payloads run inside double quotes on a shell command line (``python3 -c "..."``), so they must not contain double quotes, ``$``, -backticks, backslashes, or newlines. ``tests/test_credential_guard.py`` -enforces this along with the blocking behavior. +backticks, backslashes, newlines, or ``!`` — the last because a shell with +history expansion enabled rewrites ``!`` sequences inside double quotes. +``chr(36)`` and ``chr(33)`` stand in for the two that the patterns need. +``tests/test_credential_guard.py`` enforces this along with the blocking +behavior. """ from __future__ import annotations @@ -128,12 +185,30 @@ def _deny_expr(reason: str) -> str: " or lp==bl or lp.startswith(bl+os.sep)) else None" ) +# Source of a python expression yielding the regex for one path component +# written as a shell pattern: a literal dot plus the run of characters a +# pattern may contain. Neither ``/`` nor whitespace is in the set, so a run +# stops where the component does. The class needs no backslash escapes: +# ``]`` comes first and ``-`` last, and ``!`` arrives via ``chr(33)`` (see +# the NOTE in the module docstring for both prohibitions). +_PATTERN_COMPONENT = "'[.][]a-z0-9_.*?[^{},' + chr(33) + '-]*'" + _BASH_GUARD_CODE = ( - "import sys,json,re; " + "import sys,json,re,fnmatch; " "d=json.loads(sys.stdin.read() or '{}'); " "c=str((d.get('tool_input') or {}).get('command') or '').lower(); " + # Quoted spans undergo no pathname expansion, so they hold no globs. + "u=re.sub(chr(39) + '[^' + chr(39) + ']*' + chr(39), ' ', c); " + "u=re.sub(chr(34) + '[^' + chr(34) + ']*' + chr(34), ' ', u); " + "p=re.findall('(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', u) + " + "re.findall('[' + chr(36) + '%][a-z0-9_]*(' + " + + _PATTERN_COMPONENT + + " + ')', c); " + # A brace group stands for any of its alternatives; ``*`` covers them all. + "g=[x for x in p if set('*?[{') & set(x) and " + "fnmatch.fnmatchcase('.mureo', re.sub('[{].*[}]', '*', x))]; " "b=re.search('(^|[^a-z0-9_])[.]mureo', c) or " - "re.search('[' + chr(36) + '%][a-z0-9_]*[.]mureo', c); " + "re.search('[' + chr(36) + '%][a-z0-9_]*[.]mureo', c) or g; " + _deny_expr("mureo credential guard: commands referencing .mureo are blocked") + " if b else None" ) diff --git a/tests/hook_guard_runner.py b/tests/hook_guard_runner.py index 2f2c60f0..df3cb0fb 100644 --- a/tests/hook_guard_runner.py +++ b/tests/hook_guard_runner.py @@ -9,8 +9,8 @@ portable (no bash dependency on the Windows CI job) while exercising the exact code a shell would hand to ``python3 -c``. ``extract_python_code`` also asserts the payload is shell-safe: because the code sits inside double -quotes on the command line, any ``"``, ``$``, backtick, or backslash would -change meaning under a POSIX shell. +quotes on the command line, any ``"``, ``$``, backtick, backslash, or ``!`` +(history expansion) would change meaning under a POSIX shell. """ from __future__ import annotations @@ -25,7 +25,7 @@ _COMMAND_RE = re.compile(r'^python3 -c "(?P[^"]*)" # \[mureo-credential-guard\]$') -_SHELL_HAZARDS = ("$", "`", "\\", "\n") +_SHELL_HAZARDS = ("$", "`", "\\", "\n", "!") def extract_python_code(command: str) -> str: diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index dec5b75e..c1f19ef4 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -261,6 +261,95 @@ def test_denies_every_spelling_of_the_mureo_dir( ) assert deny_decision(proc) == "deny", command + @pytest.mark.parametrize( + "command", + [ + # Every one of these was verified against a throwaway $HOME with a + # real bash 5.2: each prints the contents of the credentials file. + "cat ~/.mure?/credentials.json", + "cat ~/.[m]ureo/credentials.json", + "cat ~/.mur*/credentials.json", + "cat ~/.m?reo/credentials.json", + "cat ~/.?????/credentials.json", + "cat ~/.[!.]*/credentials.json", + "cat ~/.mure[o]/credentials.json", + # Brace expansion runs before pathname expansion, so it produces + # the real directory name without any wildcard at all. + "cat ~/.mure{o,x}/credentials.json", + "cat ~/.mur{eo,ex}/credentials.json", + # Same patterns, other spellings of the parent directory. + "ls -la ~/.mure?", + "cp -r ~/.m?reo /tmp/exfil", + "cat $HOME/.mure?/credentials.json", + "cat ${HOME}/.mur*/credentials.json", + 'cat "$HOME"/.mure?/credentials.json', + "cat /Users/x/.mur*/credentials.json", + # Case-folded, as everywhere else in the guard. + "cat ~/.MURE?/credentials.json", + "cat ~/.[M]UREO/credentials.json", + # A substitution supplies the parent, so the pattern does not + # start at a path boundary — the same shapes rule 1 covers for + # the literal name. + "D=~/; cat $D.mure?/credentials.json", + "cat $(printf '%s.mure?/credentials.json' ~/)", + "python3 -c \"print(open('%s.mure?/credentials.json' % h).read())\"", + ], + ) + def test_denies_glob_patterns_matching_the_mureo_dir( + self, fake_home: Path, command: str + ) -> None: + """A wildcard inside the directory name still reaches the real files. + + The literal-substring rule looks for six consecutive characters, so + any metacharacter placed *inside* ``.mureo`` breaks the match while + the shell still expands the pattern onto the protected directory. + """ + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # Wildcards that cannot reach a dotfile at all: the shell requires + # a leading period to be matched explicitly. + "ls *", + "rm -rf build/*", + "cp dist/* /tmp/", + "node --test tests/js/*.test.js", + "pytest tests/test_*.py", + "ls -d */", + "git add -- mureo/*.py", + # Dot-leading patterns that cannot spell the directory name. + "rm -f .coverage*", + "ls -d .git*", + "cat .env.*", + "rm -rf .pytest_cache .ruff_cache", + # Quoted metacharacters never reach pathname expansion — regexes + # and format strings must not be read as globs. + "sed 's/.*//' notes.txt", + "grep -rn '.*TODO' mureo/", + "find . -name '*.py' -newer setup.py", + "find . -name '.*' -maxdepth 1", + "git log --grep '.*fix'", + # ...including a fully quoted path: quoting suppresses globbing, + # so `?` here is a literal character and opens nothing. + 'cat "$HOME/.mure?/credentials.json"', + # Ordinary commands with no pattern at all. + "ruff check .", + "git diff -- .", + "black --check .", + ], + ) + def test_allows_everyday_glob_commands(self, fake_home: Path, command: str) -> None: + """The pattern rule must not fire on day-to-day shell usage.""" + proc = run_guard( + _bash_guard_command(), {"command": command}, fake_home, tool_name="Bash" + ) + assert proc.returncode == 0 + assert proc.stdout.strip() == "", command + @pytest.mark.parametrize("command", ["echo hello", "ls -la", "git status"]) def test_allows_unrelated_commands(self, fake_home: Path, command: str) -> None: proc = run_guard( From 27cb7d0630500e57bc450a64705f9328e8d03308 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 05:20:27 +0900 Subject: [PATCH 02/11] fix: read the command the way a shell does, and fail closed Review of the first commit reproduced five one-line commands that still read the credentials file, four of them through the quoting layer: cat ~/'.'mure?/credentials.json cat ~/".mure"?/credentials.json echo "it's" ; cat ~/.mure?/credentials.json 'x' cat ~/.mure$(printf '?')/credentials.json shopt -s dotglob; cat ~/*/credentials.json The defect was hand-rolled quote stripping, not a missing character in a class. Two regex passes cannot decide quoting: the single-quote pass read the apostrophe in `"it's"` as an opening delimiter, paired it with the unrelated `'x'` later in the line, and deleted the real pattern between them. Quoting is now a left fold with a five-state automaton (unquoted, single, double, and the two escaped states), which also gets `echo it\'s` right -- an escaped quote is not a delimiter, and neither shlex in non-posix mode nor the previous regex handled that. A differential fuzz against a real bash then found two more classes that neither the review nor I had listed: - quote removal reassembles the name with no glob involved at all (`cat "$HOME"/.mure"o"/credentials.json`), so both rules now also run over the normalized text, not only over the text as written; - the substitution's own text extended the component being matched, so `.mure`printf o`` and `.mure$X` escaped. An expansion now normalizes to `*/`: unknown text, and unknown extent. Brace groups are expanded before matching rather than blanketed with `*`: a group containing a dot becomes `.*`, any other becomes `*`. That closes `~/{.,z}mureo`, where the group supplies the leading dot, and it removes the over-block the review flagged -- `mv .{env,env.bak}` is allowed again. `mv .{foo,bar}` still denies, and the docstring says so. Fail closed. Exit 1 is a non-blocking hook error in both hosts, so an exception escaping the payload was a bypass rather than a crash -- and both payloads already had one: malformed stdin exited 1 and let the call through, as did a path with an embedded NUL, which makes realpath raise. Both now set sys.excepthook to print the deny JSON and exit 0. Tests go through a real shell. run_guard lifts the python payload out of the command, so `bash -c "python3 -c \"...\""` -- where all of these live -- had never been exercised: 103 tests passed and caught none of them. TestGuardThroughARealShell runs the generated command as a host runs it, and pins the known-open bypasses so the surface cannot widen unnoticed. Measured against the real generated command through a real bash, with a marker credentials file: 73 everyday commands, 0 denied; 51 forms the guard must deny, 0 through; 10 mureo identifiers from #567, 0 denied. The fuzz spelled the directory name one character at a time in every quoting, escaping, class, range, brace and substitution form: of 2000 commands, 1621 read the file -- 873 with the name written out in the text, of which 0 got through, and 748 assembled at runtime, of which 265 did. Those 265 all produce the leading dot from a substitution, and with dotglob they are the shape that cannot be decided by reading command text. The docstring now says that outright, and says what this guard is: a deterrent against accidental access, not a security boundary. It runs as the same user as the file it protects and cannot be made into one -- the previous claim that "real safety comes from filesystem permissions" was wrong in the same direction, since permissions do not stop the owner either. --- mureo/credential_guard.py | 253 ++++++++++++++++++++++++--------- tests/hook_guard_runner.py | 46 ++++++ tests/test_credential_guard.py | 185 +++++++++++++++++++++++- 3 files changed, 417 insertions(+), 67 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index a3e4eba9..1a4cb32c 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -23,14 +23,18 @@ ``~/.mureo/credentials.json`` that is itself a symlink pointing OUT — its realpath escapes the dir, but the requested path is still under it). Both cover every file in the directory, not just ``credentials.json``. -* Bash guard: two rules over the command text; either one denies. +* Bash guard: reads the command text twice — as written, and as a shell + will read it once quoting is resolved — and applies two rules to both. + Either one denies. Rule 1 (the name spelled out) denies any command whose text contains ``.mureo`` where a path component could *start*. Anchoring on the directory name rather than on ``credentials`` also covers a wildcard that follows the name (``cat ~/.mureo/cred*``) — but only because the six characters of the name are still there verbatim. Rule 2 below is - what covers a wildcard placed *inside* the name. + what covers a metacharacter placed *inside* the name, and running both + rules over the normalized text is what covers a name that only becomes + contiguous after quote removal (``cat ~/.mure"o"/x``, ``~/.mur'e'o``). A bare substring test over-blocks badly, because case-folded ``.mureo`` is also a prefix of things that are emphatically not the directory: @@ -66,18 +70,18 @@ this: sibling directories (``~/.mureoX``, ``~/.mureo_backup``) still deny, since only the text before the name is consulted. - Rule 2 (the name written as a pattern) closes the other half. A - metacharacter placed inside the name breaks rule 1's six-character - literal while the shell still expands the pattern onto the real - directory: ``cat ~/.mure?/credentials.json`` prints the credentials - file, and so do ``.[m]ureo``, ``.mur*``, ``.m?reo``, ``.?????``, - ``.[!.]*`` and the brace form ``.mure{o,x}`` (checked against bash 5.2 - with a throwaway ``HOME``). No regex over the command text can decide - this, because the string that reaches the filesystem does not exist - yet — so the guard asks the question the other way round. It takes the - path components of the command, keeps those that begin at a component - boundary with a literal ``.`` and contain a metacharacter, and denies - when ``fnmatch`` says the pattern matches ``.mureo``. + Rule 2 (the name written as a pattern). A metacharacter inside the name + breaks rule 1's six-character literal while the shell still expands the + pattern onto the real directory: ``cat ~/.mure?/credentials.json`` + prints the credentials file, and so do ``.[m]ureo``, ``.mur*``, + ``.m?reo``, ``.?????``, ``.[!.]*`` and the brace form ``.mure{o,x}`` + (each run against bash 5.2 with a throwaway ``HOME``). No pattern over + the command text can decide this, because the string that reaches the + filesystem does not exist yet — so the guard asks the question the other + way round. It takes the path components of the normalized command, + keeps those beginning at a component boundary with a literal ``.`` and + containing a metacharacter, and denies when ``fnmatch`` says the pattern + matches ``.mureo``. Requiring the literal leading ``.`` is what makes that safe to do. A shell will not let a wildcard match the leading period of a filename @@ -86,35 +90,81 @@ Without that restriction the rule would have to deny every glob anyone types, ``fnmatch('.mureo', '*')`` being true. - Quoted spans are skipped for this rule, because quoting suppresses - pathname expansion: ``sed 's/.*//'`` and ``find . -name '.*'`` are a - regex and a literal, not globs, and ``cat "$HOME/.mure?/x"`` opens - nothing. Unquoted, those same characters do glob, and are denied. A - pattern spliced in by a substitution (``$D.mure?``, ``printf - '%s.mure?/'``) is caught by the same ``$``/``%`` clause rule 1 uses, - where quoting is not consulted: the pattern reaches the shell through a - later expansion. Brace groups are replaced by ``*`` before matching, - an over-approximation that also denies things like ``mv .{env,bak}`` - which cannot name the directory — the safe direction. - - What rule 2 does not cover, and no part of the guard claims to: - - - a pattern the command text does not contain, because a variable set - by an earlier command or by another program supplies it (``cat - ~/$P/x``). Written out in the same command, ``P=.mure?; cat ~/$P/x`` - is denied — the pattern is in the text; + Normalization is where both rules get their second reading of the + command, and it is a left fold over the characters with a five-state + quoting automaton — unquoted, single-quoted, double-quoted, and the two + escaped states — because that is the only way to get quoting right. An + earlier version stripped quoted spans with two regex passes and had the + defect that shape invites: in ``echo "it's" ; cat ~/.mure?/x 'x'`` the + single-quote pass read the apostrophe of ``it's`` as an opening + delimiter, paired it with the unrelated ``'x'`` at the end of the line, + and deleted the real pattern sitting between them. The fold cannot make + that mistake, and it also gets ``echo it\\'s`` right, where an escaped + quote is not a delimiter at all. + + The fold rewrites three things: + + - quote delimiters are dropped, so the text reads as the shell will read + it (this is what catches ``~/.mure"o"``); + - a *quoted* metacharacter becomes ``_``, because quoting makes it an + ordinary character and no ordinary character in ``.mureo`` is a + metacharacter. That is why ``sed 's/.*//'`` and ``find . -name '.*'`` + are a regex and a literal rather than globs, and why ``cat + "$HOME/.mure?/x"`` — which opens nothing — is allowed while the + unquoted spelling is denied; + - the start of an expansion (``$``, backtick) becomes ``*/``: ``*`` + because its text is unknown, ``/`` because its extent is unknown too, + so whatever follows in the command cannot be assumed to continue the + same path component. + + A brace group is replaced by ``.*`` when any alternative contains a dot + and by ``*`` otherwise, before the components are cut. That is what + catches both ``~/.mure{o,x}`` and ``~/{.,z}mureo``, where the group + supplies the leading dot itself. + + Deliberate over-blocks, all in the safe direction: + + - a brace group with no dot in it becomes ``*``, so ``mv .{foo,bar}`` + and ``rm .{a,b,c}`` deny although neither can name the directory. + (``mv .{env,env.bak}`` does not: the dot in an alternative makes the + replacement ``.*``, which cannot match a six-character name starting + with a single dot.) Expanding the alternatives exactly would fix + this, and is the change to make if it ever gets in the way; + - anything unquoted that really does glob dotfiles: ``ls .*``, ``ls -d + .??*``, ``rm -rf .[!.]*`` all reach ``~/.mureo`` from ``$HOME`` and + all deny; + - a component holding an expansion is unknown text, so ``ls .$X`` and + ``cat .$(cmd)`` deny. + + What the guard does not cover — measured, not assumed, and pinned by + ``test_known_open_bypasses``: + + - the shell's own options. ``shopt -s dotglob; cat ~/*/x`` reads the + file; the command text says nothing about whether ``dotglob`` is set, + and it can have been set in an earlier call on the same persistent + shell or in the user's rc file. Denying every ``*`` instead is not an + option; + - anything whose text the command does not contain: a name or pattern + produced by another program (``cat ~/$(printf '.')mureo/x``), taken + from a variable set elsewhere (``cat ~/$P/x``), or written in a + notation that has to be decoded first (``cat ~/$'\\x2emureo'/x``). + Both rules can only read what is written down. Where the text *is* + written down the guard does see it, which is why ``P=.mure?; cat + ~/$P/x`` denies; + - extended globs (``.mure@(o|x)``), which bash parses only with + ``extglob`` set, and which ``fnmatch`` does not implement; - patterns for *sibling* names (``~/.mur*_backup``): rule 2 asks only - whether the pattern matches ``.mureo`` itself, whereas rule 1 does - deny literal siblings such as ``~/.mureo_backup``; - - extended globs (``.mure@(o|x)``), which bash has off by default; - - quoting is scanned as balanced pairs and each quoted span is dropped - whole, rather than parsed as a shell would. A pattern split across - the quoting (``~/'.'mure?/x`` — the quoted dot still counts as - explicit to the shell) and one hidden behind a deliberately - unbalanced quote earlier in the line both escape this half of the - rule; - - shells configured with ``dotglob`` or ``GLOBIGNORE``, where ``*`` - does reach dotfiles. + whether a pattern matches ``.mureo`` itself, whereas rule 1 does deny + literal siblings such as ``~/.mureo_backup``. + + The first two are not closable by inspecting command text, and no + further rule should be added pretending otherwise. A random + differential fuzz against a real bash (2000 commands that spell the + directory name one character at a time, using every quoting, escaping, + class, range, brace and substitution form) found 1621 that really read + the file: of the 873 whose name is written out in the text, 0 got + through; of the 748 assembled at runtime, 265 did — all of them + producing the leading dot from a substitution. Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the @@ -122,17 +172,38 @@ genuinely distinct ``~/.MUREO`` directory), never under-block — the right direction for a guard. -The guard remains defense-in-depth, not the primary control: shell -indirection and encoded forms can still evade the Bash guard. Real safety -comes from filesystem permissions on ``~/.mureo`` itself. +Both payloads fail closed. A hook that exits non-zero for any reason +other than the documented block is a *non-blocking* error and the tool +call proceeds, so an exception escaping the payload is a bypass, not a +crash: ``sys.excepthook`` is set to print the deny JSON and exit 0. This +was not academic — malformed stdin made both payloads exit 1 and let the +call through, as did a path with an embedded NUL, which makes +``os.path.realpath`` raise. + +WHAT THIS GUARD IS. It is a deterrent against an agent reading the +credentials by accident or on a careless instruction — the cases that +actually happen. It is not a security boundary and cannot be made into +one. The agent runs as the user who owns the file, so it can read it +through any construction the text does not reveal: a variable, a +substitution, an encoding, a helper script, a language runtime. The +earlier claim here that "real safety comes from filesystem permissions" +was wrong in the same direction: permissions do not stop a process running +as the owner either. What actually limits the damage is not keeping +long-lived credentials where an autonomous agent runs, scoping and +rotating them, and the audit trail — not this hook. Judge changes to it +by whether they make the common accident less likely without blocking real +work, and do not describe it as more than that. NOTE: the python payloads run inside double quotes on a shell command line (``python3 -c "..."``), so they must not contain double quotes, ``$``, backticks, backslashes, newlines, or ``!`` — the last because a shell with history expansion enabled rewrites ``!`` sequences inside double quotes. -``chr(36)`` and ``chr(33)`` stand in for the two that the patterns need. -``tests/test_credential_guard.py`` enforces this along with the blocking -behavior. +Every one of those characters is also *data* the Bash guard needs, since +they are exactly the characters a shell treats as special, so each arrives +by ``chr()``: 33 ``!``, 34 ``"``, 36 ``$``, 39 ``'``, 92 backslash, 96 +backtick. ``tests/test_credential_guard.py`` enforces the prohibition, and +``TestGuardThroughARealShell`` runs the generated command through a real +bash so the wrapper's own quoting is exercised rather than assumed. """ from __future__ import annotations @@ -170,8 +241,15 @@ def _deny_expr(reason: str) -> str: ) +_PATH_REASON = "mureo credential guard: files under ~/.mureo are protected" + _PATH_GUARD_CODE = ( "import sys,json,os; " + # Fail closed: exit 1 is a non-blocking hook error in both hosts, so an + # escaping exception would let the call through. A path that makes + # realpath raise (an embedded NUL, say) must deny, not proceed. + "sys.excepthook=lambda *a: (" + _deny_expr(_PATH_REASON) + ", " + "sys.stdout.flush(), os._exit(0)); " "d=json.loads(sys.stdin.read() or '{}'); " "i=d.get('tool_input') or {}; " "p=str(i.get('file_path') or i.get('path') or i.get('notebook_path') or ''); " @@ -180,36 +258,81 @@ def _deny_expr(reason: str) -> str: "bl=os.path.abspath(os.path.expanduser('~/.mureo')).lower(); " "r=os.path.realpath(e).lower() if p else ''; " "lp=os.path.abspath(e).lower() if p else ''; " - + _deny_expr("mureo credential guard: files under ~/.mureo are protected") + + _deny_expr(_PATH_REASON) + " if p and (r==b or r.startswith(b+os.sep)" " or lp==bl or lp.startswith(bl+os.sep)) else None" ) +# Shell metacharacters, named once. None of them may appear literally in +# the payload (see the NOTE in the module docstring), so each arrives as a +# chr() call: q1 ', q2 ", bs backslash, dl $, tk backtick. +_CHARS = "q1=chr(39); q2=chr(34); bs=chr(92); dl=chr(36); tk=chr(96); mt='*?[]{},'; " + +# The quoting automaton, as the step function of a left fold. States: +# 0 unquoted, 1 single-quoted, 2 double-quoted, 3 escaped (from unquoted), +# 4 escaped (inside double quotes). Inside single quotes nothing is +# special, not even a backslash — the rule bash applies. +_QUOTE_STEP = ( + "lambda k,x: (1 if x==q1 else 2 if x==q2 else 3 if x==bs else 0) if k==0" + " else (0 if x==q1 else 1) if k==1" + " else (0 if x==q2 else 4 if x==bs else 2) if k==2" + " else (0 if k==3 else 2)" +) + +# Rebuild the command with quoting resolved, one character at a time: drop +# the delimiters; turn a quoted metacharacter into `_`, because quoting +# makes it an ordinary character and no ordinary character in `.mureo` is a +# metacharacter; and turn the start of an expansion into `*/`. +# +# `*` because its text is unknown, and `/` because where it *ends* is +# unknown too: the characters after it in the command (`o` in `.mure$X`, +# `printf o` inside backticks) are not necessarily part of the same path +# component, so they must not extend the pattern being tested. +_NORMALIZE = ( + "''.join('' if (k==0 and x in q1+q2+bs) or (k==1 and x==q1)" + " or (k==2 and x in q2+bs)" + " else ('*/' if x in dl+tk and k in (0,2) else ('_' if k and x in mt else x))" + " for x,k in zip(c,st))" +) + +# A brace group stands for any of its alternatives. One that contains a dot +# can supply the leading dot of a dotfile, so it becomes `.*`; any other +# becomes `*`. Applied twice, which covers a group nested in a group. +_DEBRACE = "re.sub(gr, fb, re.sub(gr, fb, t))" + # Source of a python expression yielding the regex for one path component # written as a shell pattern: a literal dot plus the run of characters a # pattern may contain. Neither ``/`` nor whitespace is in the set, so a run # stops where the component does. The class needs no backslash escapes: -# ``]`` comes first and ``-`` last, and ``!`` arrives via ``chr(33)`` (see -# the NOTE in the module docstring for both prohibitions). +# ``]`` comes first and ``-`` last, and ``!`` arrives via ``chr(33)``. _PATTERN_COMPONENT = "'[.][]a-z0-9_.*?[^{},' + chr(33) + '-]*'" +_BASH_REASON = "mureo credential guard: commands that can reach ~/.mureo are blocked" + _BASH_GUARD_CODE = ( - "import sys,json,re,fnmatch; " + "import sys,json,re,os,fnmatch,itertools; " + # Fail closed: an escaping exception exits 1, which both hosts treat as a + # non-blocking hook error, so every exception must deny instead. + "sys.excepthook=lambda *a: (" + _deny_expr(_BASH_REASON) + ", " + "sys.stdout.flush(), os._exit(0)); " "d=json.loads(sys.stdin.read() or '{}'); " "c=str((d.get('tool_input') or {}).get('command') or '').lower(); " - # Quoted spans undergo no pathname expansion, so they hold no globs. - "u=re.sub(chr(39) + '[^' + chr(39) + ']*' + chr(39), ' ', c); " - "u=re.sub(chr(34) + '[^' + chr(34) + ']*' + chr(34), ' ', u); " - "p=re.findall('(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', u) + " - "re.findall('[' + chr(36) + '%][a-z0-9_]*(' + " - + _PATTERN_COMPONENT - + " + ')', c); " - # A brace group stands for any of its alternatives; ``*`` covers them all. - "g=[x for x in p if set('*?[{') & set(x) and " - "fnmatch.fnmatchcase('.mureo', re.sub('[{].*[}]', '*', x))]; " - "b=re.search('(^|[^a-z0-9_])[.]mureo', c) or " - "re.search('[' + chr(36) + '%][a-z0-9_]*[.]mureo', c) or g; " - + _deny_expr("mureo credential guard: commands referencing .mureo are blocked") + + _CHARS + + "st=list(itertools.accumulate(c, " + + _QUOTE_STEP + + ", initial=0)); " + "t=" + _NORMALIZE + "; " + "gr='[{][^{}]*[}]'; fb=lambda m: '.*' if '.' in m.group() else '*'; " + "t=" + _DEBRACE + "; " + # The literal rules read the command as written and as the shell will + # read it, so quoting cannot reassemble the name unseen. + "j=c + ' ' + t; " + "p=re.findall('(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', t) + " + "re.findall('[' + dl + '%][a-z0-9_]*(' + " + _PATTERN_COMPONENT + " + ')', c); " + "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " + "b=re.search('(^|[^a-z0-9_])[.]mureo', j) or " + "re.search('[' + dl + '%][a-z0-9_]*[.]mureo', j) or g; " + + _deny_expr(_BASH_REASON) + " if b else None" ) diff --git a/tests/hook_guard_runner.py b/tests/hook_guard_runner.py index df3cb0fb..89e50b87 100644 --- a/tests/hook_guard_runner.py +++ b/tests/hook_guard_runner.py @@ -11,6 +11,12 @@ also asserts the payload is shell-safe: because the code sits inside double quotes on the command line, any ``"``, ``$``, backtick, backslash, or ``!`` (history expansion) would change meaning under a POSIX shell. + +That is not the whole story, though: lifting the payload out skips the +wrapper, so a quoting mistake in the wrapper itself would go unnoticed. +``run_guard_in_shell`` closes that by handing the whole command to a real +bash. Anything whose answer depends on quoting — the guard's own, or the +command's — belongs there. """ from __future__ import annotations @@ -18,6 +24,7 @@ import json import os import re +import shutil import subprocess import sys from pathlib import Path @@ -27,6 +34,9 @@ _SHELL_HAZARDS = ("$", "`", "\\", "\n", "!") +BASH = shutil.which("bash") +PYTHON3 = shutil.which("python3") + def extract_python_code(command: str) -> str: """Return the python payload from a guard command, refusing unsafe shapes.""" @@ -66,6 +76,42 @@ def run_guard( ) +def run_guard_in_shell( + command: str, + tool_input: dict[str, Any] | None, + home: Path, + tool_name: str = "Bash", + raw_stdin: str | None = None, +) -> subprocess.CompletedProcess[str]: + """Run the guard command *as a shell runs it*: ``bash -c ``. + + ``run_guard`` above lifts the python payload out of the command and runs + it directly, which never exercises the ``python3 -c "..."`` wrapper. The + quoting of that wrapper is a layer of its own: it is where a stray ``"``, + ``$``, backtick or ``!`` in the payload would change the program the + shell actually runs. Tests that care about a quoting question must go + through here. + + ``raw_stdin`` sends bytes verbatim instead of a tool-call JSON, for the + malformed-input cases. + """ + assert BASH is not None and PYTHON3 is not None, "needs bash and python3" + payload = ( + raw_stdin + if raw_stdin is not None + else json.dumps({"tool_name": tool_name, "tool_input": tool_input or {}}) + ) + env = dict(os.environ, HOME=str(home), USERPROFILE=str(home)) + return subprocess.run( + [BASH, "-c", command], + input=payload, + capture_output=True, + text=True, + env=env, + timeout=30, + ) + + def deny_decision(proc: subprocess.CompletedProcess[str]) -> str | None: """Return the ``permissionDecision`` emitted by a guard run, if any.""" if not proc.stdout.strip(): diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index c1f19ef4..8b013c30 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -8,7 +8,9 @@ never be mistaken for an intentional block. These tests execute the generated hook payloads in a subprocess with a fake -``$HOME``, mirroring how the agent harness invokes them. +``$HOME``, mirroring how the agent harness invokes them. The cases whose +answer depends on quoting run the whole command through a real bash instead +— see ``TestGuardThroughARealShell``. """ from __future__ import annotations @@ -20,7 +22,18 @@ import pytest -from tests.hook_guard_runner import deny_decision, run_guard +from tests.hook_guard_runner import ( + BASH, + PYTHON3, + deny_decision, + run_guard, + run_guard_in_shell, +) + +needs_shell = pytest.mark.skipif( + BASH is None or PYTHON3 is None, + reason="the shell layer needs both bash and python3 on PATH", +) _PROTECTED_FILES = ( "credentials.json", @@ -391,6 +404,174 @@ def test_allows_mureo_own_identifiers(self, fake_home: Path, command: str) -> No assert proc.stdout.strip() == "", command +# --------------------------------------------------------------------------- +# The shell layer +# --------------------------------------------------------------------------- + + +@needs_shell +@pytest.mark.unit +class TestGuardThroughARealShell: + """Run the generated command the way a host runs it: ``bash -c``. + + Every case below was checked outside the suite against a throwaway + ``HOME`` holding a marker credentials file: the deny cases print the + marker when the guard is removed, and the allow cases print nothing. + """ + + @pytest.mark.parametrize( + "command", + [ + # Quoting splits the name, and quote removal puts it back + # together. None of these contains `.mureo` as six consecutive + # characters, and the first four hide the metacharacter too. + "cat ~/'.'mure?/credentials.json", + 'cat ~/".mure"?/credentials.json', + "cat ~/.mure''?/credentials.json", + 'cat ~/.mure"o"/credentials.json', + 'cat "$HOME"/.mure"o"/credentials.json', + "cat ~/.mur'e'o/credentials.json", + # An apostrophe inside a double-quoted word is an ordinary + # character. Reading it as a delimiter pairs it with the next + # quote and swallows the real pattern in between. + "echo \"it's\" ; cat ~/.mure?/credentials.json 'x'", + "echo \"don't\" && cat ~/.mure?/credentials.json 'y'", + # A backslash-escaped quote is not a delimiter either. + "echo it\\'s ; cat ~/.mure?/credentials.json", + # An escaped metacharacter is literal, but an escaped letter is + # still the letter. + "cat ~/\\.mureo/credentials.json", + "cat ~/.mur\\eo/credentials.json", + # A substitution inside the name makes the rest of it unknown. + "cat ~/.mure$(printf '?')/credentials.json", + "cat ~/.mure`printf o`/credentials.json", + # A brace group can supply any character, including the dot. + "cat ~/{.,z}mureo/credentials.json", + "cat ~/.mure{o,x}/credentials.json", + "cat ~/.mur{e{o,z},y}/credentials.json", + # ...and the plain forms still deny through the shell layer. + "cat ~/.mureo/credentials.json", + "cat ~/.mure?/credentials.json", + "D=~/; cat $D.mure?/credentials.json", + "cat $(printf '%s.mure?/credentials.json' ~/)", + ], + ) + def test_denies_through_the_shell(self, fake_home: Path, command: str) -> None: + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert deny_decision(proc) == "deny", command + + @pytest.mark.parametrize( + "command", + [ + # Quoted metacharacters are ordinary characters: a regex, a + # literal argument, a path that opens nothing. + "sed 's/.*//' notes.txt", + "sed -e 's|.*/||' paths.txt", + "find . -name '.*' -maxdepth 1", + "grep -rn '.*TODO' mureo/", + 'cat "$HOME/.mure?/credentials.json"', + "cat '~/.mure?/credentials.json'", + # Everyday globbing, which cannot reach a dotfile. + "ls *", + "rm -rf build/*", + "node --test tests/js/*.test.js", + "ls -d .git*", + # mureo's own identifiers, including inside quotes. + "gh release create v0.10.44 --notes 'adds window.MUREO_REPORTS_FORMAT'", + "pip install --index-url https://pkgs.mureo.jp/simple/ mureo-agency", + "echo user@pkgs.mureo.jp", + ], + ) + def test_allows_through_the_shell(self, fake_home: Path, command: str) -> None: + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "", command + + @pytest.mark.parametrize( + "raw_stdin", + [ + "{not json", + "[]", + '{"tool_input": "not a dict"}', + "\x00\x01\x02", + ], + ) + def test_malformed_input_denies_rather_than_escapes( + self, fake_home: Path, raw_stdin: str + ) -> None: + """An exception in the payload is a bypass, not a crash. + + Exit 1 is a *non-blocking* hook error in both hosts, so a payload + that raises lets the tool call proceed. Anything the guard cannot + parse must therefore deny. + """ + proc = run_guard_in_shell( + _bash_guard_command(), None, fake_home, raw_stdin=raw_stdin + ) + assert deny_decision(proc) == "deny", raw_stdin + assert proc.returncode == 0, proc.stderr + + def test_path_guard_malformed_input_denies(self, fake_home: Path) -> None: + proc = run_guard_in_shell( + _path_guard_command(), None, fake_home, raw_stdin="{not json" + ) + assert deny_decision(proc) == "deny" + assert proc.returncode == 0 + + def test_path_guard_unusable_path_denies(self, fake_home: Path) -> None: + """``realpath`` raises on an embedded NUL — that must not fail open.""" + proc = run_guard_in_shell( + _path_guard_command(), + {"file_path": "\x00/x/.mureo/credentials.json"}, + fake_home, + tool_name="Read", + ) + assert deny_decision(proc) == "deny" + assert proc.returncode == 0 + + @pytest.mark.parametrize( + "command", + [ + # The shell's own glob options are not in the command text. With + # dotglob set — here, or in an earlier call on the persistent + # shell, or in the user's rc file — `*` reaches dotfiles. + "shopt -s dotglob; cat ~/*/credentials.json", + # The leading dot is produced at runtime, so the text never + # contains a dot-anchored component to test. + "cat ~/$(printf '.')mureo/credentials.json", + "cat ~/$(printf '.')mure?/credentials.json", + # The name is assembled by a previous command. + "cat ~/$P/credentials.json", + # Another notation has to be decoded first. + "cat ~/$'\\x2emureo'/credentials.json", + ], + ) + def test_known_open_bypasses(self, fake_home: Path, command: str) -> None: + """The bypasses this guard does not close, pinned so they cannot grow. + + Each one reads the real file (verified against a throwaway HOME) and + each is allowed. They are here so the open surface is a list someone + has to edit, rather than something discovered by a reviewer: closing + one means deleting its row and saying so in the module docstring. + + What they have in common is that the text handed to the guard does + not contain the thing that reaches the filesystem — it is produced + later, by the shell's options, by another program, or by decoding + another notation. No inspection of the command text can decide them; + see the module docstring in ``mureo/credential_guard.py``. + """ + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "", f"now denied, update the docstring: {command}" + + # --------------------------------------------------------------------------- # Template structure # --------------------------------------------------------------------------- From abfc2ece50593259cd6e636e9abfb3e78f0538d1 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 05:22:07 +0900 Subject: [PATCH 03/11] docs: state precisely which known-open rows were leak-verified The $P row reads the file only once an earlier call has set the variable; the docstring claimed all of them print the credentials file. --- tests/test_credential_guard.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 8b013c30..a8961936 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -554,10 +554,13 @@ def test_path_guard_unusable_path_denies(self, fake_home: Path) -> None: def test_known_open_bypasses(self, fake_home: Path, command: str) -> None: """The bypasses this guard does not close, pinned so they cannot grow. - Each one reads the real file (verified against a throwaway HOME) and - each is allowed. They are here so the open surface is a list someone - has to edit, rather than something discovered by a reviewer: closing - one means deleting its row and saying so in the module docstring. + All but one were run against a throwaway HOME and printed the + credentials file; the exception is ``cat ~/$P/x``, which reads it + only once an earlier call has set ``P`` — the point of the row is + that the name is nowhere in the text. They are here so the open + surface is a list someone has to edit, rather than something a + reviewer discovers: closing one means deleting its row and saying + so in the module docstring. What they have in common is that the text handed to the guard does not contain the thing that reaches the filesystem — it is produced From 4121f797aeadeefd1d53ec679b632cd5c21326c0 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 06:03:59 +0900 Subject: [PATCH 04/11] fix: delete line continuations before reading the command A backslash followed by a newline is removed by the shell, both characters, before it tokenises anything. The normalizer dropped the backslash and kept the newline, so the name was never contiguous and neither rule saw it: cat ~/.mu\ reo/credentials.json Verified against a throwaway HOME with bash 5.2: that prints the credentials file, and so do `.\mureo`, `.m\u\r\e\o`, `.mure\?`, `.m\ure?` and the same spellings inside double quotes, where the continuation still applies. Five leaked; all five now deny. The fold already had the states needed, so the fix is one clause: in the two escaped states, a newline is dropped along with its backslash. Inside single quotes a backslash is an ordinary character and there is no continuation, so `cat '~/.mureo/x'` is a name with a newline in it and stays allowed -- it opens nothing. This is not a regression: main misses it for the same reason, and so does shlex.split(posix=True), which leaves the raw newline in the token. It is in scope anyway, because the docstring claims to read the command as a shell reads it once quoting is resolved, and line continuation is resolved before that. The reason it was not already an enumerated known-open row is that the differential fuzz never generated `\`: a whole lexer-level rewrite went untested while the numbers looked complete. The generator now emits continuations in and out of quotes, plus `$"..."`. Re-run: of 2500 commands, 1995 read the file -- 825 with the name written out in the text, 0 through; 1170 assembled at runtime, 172 through, all producing the leading dot from a substitution. The docstring records both the new numbers and the lesson about trusting a generator's coverage. Also documented, both found while checking this: - the Bash payload needs Python 3.8+ for accumulate(initial=). It runs under whatever python3 is on PATH, not necessarily the interpreter mureo was installed with; on anything older it raises, which fails closed and denies every Bash call rather than letting one through. - an arithmetic expansion is treated like any other expansion, so `echo .$((1+1))` and `cat ~/.mure$((0))?/x` deny although neither can reach the directory. --- mureo/credential_guard.py | 61 ++++++++++++++++++++++++++-------- tests/test_credential_guard.py | 21 ++++++++++++ 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index 1a4cb32c..ca0a96b8 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -102,10 +102,19 @@ that mistake, and it also gets ``echo it\\'s`` right, where an escaped quote is not a delimiter at all. - The fold rewrites three things: + The fold rewrites four things: - quote delimiters are dropped, so the text reads as the shell will read it (this is what catches ``~/.mure"o"``); + - a line continuation — a backslash with a newline after it — is dropped + whole, both characters, because that is what a shell does with the + pair before it tokenises anything. ``cat ~/.mu\\reo/…`` + prints the credentials file, and so do ``.\\mureo``, + ``.mure\\?`` and the same spellings inside double quotes. + Keeping the newline was enough to stop the name ever being contiguous, + so neither rule saw it. Inside *single* quotes a backslash is an + ordinary character, so there is no continuation there and none is + normalized away; - a *quoted* metacharacter becomes ``_``, because quoting makes it an ordinary character and no ordinary character in ``.mureo`` is a metacharacter. That is why ``sed 's/.*//'`` and ``find . -name '.*'`` @@ -134,7 +143,9 @@ .??*``, ``rm -rf .[!.]*`` all reach ``~/.mureo`` from ``$HOME`` and all deny; - a component holding an expansion is unknown text, so ``ls .$X`` and - ``cat .$(cmd)`` deny. + ``cat .$(cmd)`` deny. An arithmetic expansion is not treated any + differently, so ``echo .$((1+1))`` and ``cat ~/.mure$((0))?/x`` deny + too, though neither can reach the directory. What the guard does not cover — measured, not assumed, and pinned by ``test_known_open_bypasses``: @@ -159,12 +170,18 @@ The first two are not closable by inspecting command text, and no further rule should be added pretending otherwise. A random - differential fuzz against a real bash (2000 commands that spell the + differential fuzz against a real bash (2500 commands that spell the directory name one character at a time, using every quoting, escaping, - class, range, brace and substitution form) found 1621 that really read - the file: of the 873 whose name is written out in the text, 0 got - through; of the 748 assembled at runtime, 265 did — all of them - producing the leading dot from a substitution. + line-continuation, class, range, brace and substitution form) found 1995 + that really read the file: of the 825 whose name is written out in the + text, 0 got through; of the 1170 assembled at runtime, 172 did — all of + them producing the leading dot from a substitution. + + That fuzz is also how the line-continuation family should have been + found, and was not: the generator had no ``\\`` among its + escaping forms, so a whole lexer-level rewrite went untested while the + numbers above looked complete. A form the generator cannot produce is a + form nothing here has checked; extend it before trusting it. Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the @@ -180,6 +197,14 @@ call through, as did a path with an embedded NUL, which makes ``os.path.realpath`` raise. +The payloads run under whatever ``python3`` the host finds on PATH, which +need not be the interpreter mureo itself was installed with. The Bash +payload needs **Python 3.8 or newer** for ``itertools.accumulate(..., +initial=...)``; on anything older it raises, which fails closed — it +denies every Bash call rather than letting any through, so the symptom is +loud and safe rather than silent. Keep it that way: a rewrite of the fold +that avoids ``initial=`` is fine, one that swallows the error is not. + WHAT THIS GUARD IS. It is a deterrent against an agent reading the credentials by accident or on a careless instruction — the cases that actually happen. It is not a security boundary and cannot be made into @@ -265,8 +290,11 @@ def _deny_expr(reason: str) -> str: # Shell metacharacters, named once. None of them may appear literally in # the payload (see the NOTE in the module docstring), so each arrives as a -# chr() call: q1 ', q2 ", bs backslash, dl $, tk backtick. -_CHARS = "q1=chr(39); q2=chr(34); bs=chr(92); dl=chr(36); tk=chr(96); mt='*?[]{},'; " +# chr() call: q1 ', q2 ", bs backslash, dl $, tk backtick, nl newline. +_CHARS = ( + "q1=chr(39); q2=chr(34); bs=chr(92); dl=chr(36); tk=chr(96); nl=chr(10); " + "mt='*?[]{},'; " +) # The quoting automaton, as the step function of a left fold. States: # 0 unquoted, 1 single-quoted, 2 double-quoted, 3 escaped (from unquoted), @@ -280,17 +308,24 @@ def _deny_expr(reason: str) -> str: ) # Rebuild the command with quoting resolved, one character at a time: drop -# the delimiters; turn a quoted metacharacter into `_`, because quoting -# makes it an ordinary character and no ordinary character in `.mureo` is a -# metacharacter; and turn the start of an expansion into `*/`. +# the delimiters; drop the newline of a line continuation, since a shell +# removes the pair before it tokenises anything; turn a quoted +# metacharacter into `_`, because quoting makes it an ordinary character +# and no ordinary character in `.mureo` is a metacharacter; and turn the +# start of an expansion into `*/`. # # `*` because its text is unknown, and `/` because where it *ends* is # unknown too: the characters after it in the command (`o` in `.mure$X`, # `printf o` inside backticks) are not necessarily part of the same path # component, so they must not extend the pattern being tested. +# +# `k>2` is the two escaped states: only there does a newline belong to a +# continuation. Inside single quotes a backslash is an ordinary character, +# so `.mu\\reo` in single quotes really is a name with a newline +# in it, and normalizing it away would over-block rather than protect. _NORMALIZE = ( "''.join('' if (k==0 and x in q1+q2+bs) or (k==1 and x==q1)" - " or (k==2 and x in q2+bs)" + " or (k==2 and x in q2+bs) or (k>2 and x==nl)" " else ('*/' if x in dl+tk and k in (0,2) else ('_' if k and x in mt else x))" " for x,k in zip(c,st))" ) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index a8961936..fcf12d38 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -449,6 +449,21 @@ class TestGuardThroughARealShell: "cat ~/{.,z}mureo/credentials.json", "cat ~/.mure{o,x}/credentials.json", "cat ~/.mur{e{o,z},y}/credentials.json", + # A line continuation is deleted, backslash and newline both, + # before the shell tokenises anything — so the name is spelled + # across two lines and is contiguous by the time it is used. + "cat ~/.mu\\\nreo/credentials.json", + "cat ~/.\\\nmureo/credentials.json", + "cat ~/.m\\\nu\\\nr\\\ne\\\no/credentials.json", + "cat ~/.mure\\\n?/credentials.json", + "cat ~/.m\\\nure?/credentials.json", + # ...including inside double quotes, where it is still a + # continuation (and where nothing globs, so the name itself is + # what has to be seen). + 'cat "$HOME/.mu\\\nreo/credentials.json"', + # `$"..."` is a translated string: the `$` is not an expansion + # of anything the guard cannot see. + 'cat ~/$".mureo"/credentials.json', # ...and the plain forms still deny through the shell layer. "cat ~/.mureo/credentials.json", "cat ~/.mure?/credentials.json", @@ -479,6 +494,12 @@ def test_denies_through_the_shell(self, fake_home: Path, command: str) -> None: "rm -rf build/*", "node --test tests/js/*.test.js", "ls -d .git*", + # Inside single quotes a backslash is an ordinary character, so + # this is a name with a newline in it, not a continuation, and + # it opens nothing. + "cat '~/.mu\\\nreo/credentials.json'", + # A continuation that only wraps a long line. + "ls -la \\\n ~/project", # mureo's own identifiers, including inside quotes. "gh release create v0.10.44 --notes 'adds window.MUREO_REPORTS_FORMAT'", "pip install --index-url https://pkgs.mureo.jp/simple/ mureo-agency", From 1741d9ccfca0726fe0b1a882063d2e1c39ffe631 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 07:09:07 +0900 Subject: [PATCH 05/11] refactor: read the command once, and have both rules read that The recurring defect was structural, not a missing case: one rule scanned the raw command and the other scanned the folded text, so whatever one of them resolved was invisible to the other. Each round of folding therefore opened a hole on the axis the other rule owned. This round's report: D2=~/ cat $D2.mu\ reo/credentials.json reads the file. The continuation was folded away in the text the pattern rule read, while the rule that understood `$D2` was still looking at raw text. `${D2}` denied only by accident, because the debrace pass happened to rewrite `{D2}`. A sibling needed no continuation at all: `D2=~/; cat $D2.mure"o"/credentials.json`. There is now one normalized reading, and both rules consume it. The piece that makes that possible is preserving, through the fold, the boundary information the literal rule used to get from raw text: an expansion becomes `*/` and swallows the identifier run naming it, so `$D.mureo`, `${D}.mureo`, `$1.mureo` and `%s.mureo` all read as `*/.mureo`, whose dot sits at a boundary exactly like the one in `~/.mureo`. The separate `[$%][a-z0-9_]*` rule over raw text is gone, and with it the asymmetry. Two things the corpus proved wrong on the way, both real design bugs: - the placeholder for a quoted metacharacter was `_`, an *identifier* character, so `'{}.mureo'` folded to `__.mureo` and the boundary test read one long name. It is now `=`, which reads as a boundary; - quoted text is literal unless a program will build a path out of it. `printf '%s.mure?/x'` is a template whose `?` survives into a filename that the shell then globs. A `%` in a quoted span now keeps that span's metacharacters live, and the flag resets at the end of the span, so `echo "100%" ; sed 's/.*//'` stays allowed. Measured against the real generated command through a real bash, with a marker credentials file: - an exhaustive product of {parent supplied by: literal, $HOME, "$HOME", $VAR, "$VAR", ${VAR}, $VAR$EMPTY, $1, $(cmd), backtick} x {name broken by: nothing, continuation, two continuations, single-quote split, single-quoted char, double-quote split, double-quoted char, escaped char, class, wildcard, brace, star} x {position}: all 560 members read the file, all 560 deny; - 73 everyday commands, 0 denied; 51 must-deny forms, 0 through; the 10 mureo identifiers of #567, 0 denied; the random fuzz unchanged at 0 undetected among names written out in the text. Both previous rounds were products of two axes and the generator had only walked the margins -- it emitted continuations, and it emitted substitutions, but never a continuation inside a substituted parent. It is now combinatorial over the product, and the docstring says that a form the generator cannot produce is a form nothing has checked, combinations included. Disclosed over-block, since the same normalization drives it: a format string of the shape `.` cannot be told apart from `printf '%s.mureo/...' ~/`, so `printf '%s.%s' a b` denies. Of twenty %-heavy everyday commands it is the only one. --- mureo/credential_guard.py | 236 +++++++++++++++++++++------------ tests/test_credential_guard.py | 19 +++ 2 files changed, 169 insertions(+), 86 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index ca0a96b8..ae0049b5 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -23,18 +23,30 @@ ``~/.mureo/credentials.json`` that is itself a symlink pointing OUT — its realpath escapes the dir, but the requested path is still under it). Both cover every file in the directory, not just ``credentials.json``. -* Bash guard: reads the command text twice — as written, and as a shell - will read it once quoting is resolved — and applies two rules to both. - Either one denies. - - Rule 1 (the name spelled out) denies any command whose text contains +* Bash guard: normalizes the command *once* into the text a shell would + read after quoting, line continuations and expansions are resolved, and + applies two rules to that one string. Either one denies. + + The single reading is the load-bearing part, and it was learned the + expensive way. Earlier versions had one rule scanning the raw command + and another scanning the folded text; every obfuscation one of them + resolved was invisible to the other, so each new fold opened a new hole + on the axis the other rule owned. ``D=~/; cat $D.mu\\reo/…`` + reads the file: the continuation was folded away in the text the + pattern rule read, while the rule that knew about ``$D`` was still + looking at the raw command. Nothing here may reintroduce a second + reader. If a rule needs information the fold destroys, the fold has to + preserve it — which is what ``_COLLAPSE`` does for expansion + boundaries — rather than the rule reaching for a different string. + + Rule 1 (the name spelled out) denies when the normalized text contains ``.mureo`` where a path component could *start*. Anchoring on the directory name rather than on ``credentials`` also covers a wildcard - that follows the name (``cat ~/.mureo/cred*``) — but only because the - six characters of the name are still there verbatim. Rule 2 below is - what covers a metacharacter placed *inside* the name, and running both - rules over the normalized text is what covers a name that only becomes - contiguous after quote removal (``cat ~/.mure"o"/x``, ``~/.mur'e'o``). + that follows the name (``cat ~/.mureo/cred*``). Rule 2 below covers a + metacharacter placed *inside* the name. Because both read the + normalized text, a name that only becomes contiguous once the shell has + worked on it — ``.mure"o"``, ``.mur'e'o``, ``.mu\\reo``, + ``$D.mureo`` — is as visible to them as one written out. A bare substring test over-blocks badly, because case-folded ``.mureo`` is also a prefix of things that are emphatically not the directory: @@ -51,20 +63,22 @@ substring is at the start of the command or preceded by a non-identifier character. - That test alone would be too weak, because an identifier character can - also be the tail of a *substitution* that supplies the parent directory: - with ``D=~/``, the command ``cat $D.mureo/credentials.json`` resolves - into the protected directory while putting ``D`` immediately before the - name. Of all the ways a shell can splice text, only ``$NAME`` and - ``$1`` end in an identifier character — ``${...}``, ``$(...)``, - backticks and brace expansion all close with punctuation, which the - boundary test already catches. The same applies one level up, to - format specifiers consumed by a program (``printf '%s.mureo/...' ~/``). - So the guard additionally denies when the identifier run before the - substring is itself introduced by ``$`` or ``%``. - - Note ``$`` cannot appear literally in the payload (see the NOTE below), - hence ``chr(36)``. + That boundary test would be too weak on the raw command, because an + identifier character can also be the tail of a *substitution* that + supplies the parent directory: with ``D=~/``, the command ``cat + $D.mureo/credentials.json`` resolves into the protected directory while + putting ``D`` immediately before the name. The same applies one level + up, to a format specifier a program will fill in (``printf + '%s.mureo/...' ~/``). + + This is where an earlier design added a *second rule* over the raw text, + and where the split-brain bugs came from. Normalization handles it + instead: an expansion becomes ``*/`` and swallows the identifier run + that names it, so ``$D.mureo``, ``${D}.mureo``, ``$1.mureo`` and + ``%s.mureo`` all read as ``*/.mureo``. The dot then sits after a + non-identifier character, exactly as it does in ``~/.mureo``, and the + one boundary test sees every one of them — including when the name is + *also* broken up, which is what the two-rule version could not do. Nothing that names the directory in plain path syntax is admitted by this: sibling directories (``~/.mureoX``, ``~/.mureo_backup``) still @@ -90,19 +104,18 @@ Without that restriction the rule would have to deny every glob anyone types, ``fnmatch('.mureo', '*')`` being true. - Normalization is where both rules get their second reading of the - command, and it is a left fold over the characters with a five-state - quoting automaton — unquoted, single-quoted, double-quoted, and the two - escaped states — because that is the only way to get quoting right. An - earlier version stripped quoted spans with two regex passes and had the - defect that shape invites: in ``echo "it's" ; cat ~/.mure?/x 'x'`` the - single-quote pass read the apostrophe of ``it's`` as an opening - delimiter, paired it with the unrelated ``'x'`` at the end of the line, - and deleted the real pattern sitting between them. The fold cannot make - that mistake, and it also gets ``echo it\\'s`` right, where an escaped - quote is not a delimiter at all. + Normalization produces that one reading, and it is a left fold over the + characters with a five-state quoting automaton — unquoted, single-quoted, + double-quoted, and the two escaped states — because that is the only way + to get quoting right. An earlier version stripped quoted spans with two + regex passes and had the defect that shape invites: in ``echo "it's" ; + cat ~/.mure?/x 'x'`` the single-quote pass read the apostrophe of + ``it's`` as an opening delimiter, paired it with the unrelated ``'x'`` + at the end of the line, and deleted the real pattern sitting between + them. The fold cannot make that mistake, and it also gets ``echo + it\\'s`` right, where an escaped quote is not a delimiter at all. - The fold rewrites four things: + The fold rewrites five things: - quote delimiters are dropped, so the text reads as the shell will read it (this is what catches ``~/.mure"o"``); @@ -111,20 +124,31 @@ pair before it tokenises anything. ``cat ~/.mu\\reo/…`` prints the credentials file, and so do ``.\\mureo``, ``.mure\\?`` and the same spellings inside double quotes. - Keeping the newline was enough to stop the name ever being contiguous, - so neither rule saw it. Inside *single* quotes a backslash is an - ordinary character, so there is no continuation there and none is - normalized away; - - a *quoted* metacharacter becomes ``_``, because quoting makes it an + Keeping the newline was enough to stop the name ever being contiguous. + Inside *single* quotes a backslash is an ordinary character, so there + is no continuation there and none is normalized away; + - a *quoted* metacharacter becomes ``=``, because quoting makes it an ordinary character and no ordinary character in ``.mureo`` is a metacharacter. That is why ``sed 's/.*//'`` and ``find . -name '.*'`` are a regex and a literal rather than globs, and why ``cat "$HOME/.mure?/x"`` — which opens nothing — is allowed while the - unquoted spelling is denied; - - the start of an expansion (``$``, backtick) becomes ``*/``: ``*`` - because its text is unknown, ``/`` because its extent is unknown too, - so whatever follows in the command cannot be assumed to continue the - same path component. + unquoted spelling is denied. The placeholder has to be a character + that reads as a *boundary*: it was ``_`` once, and since ``_`` is an + identifier character, ``'{}.mureo'`` folded to ``__.mureo`` and the + boundary test saw one long name rather than the directory; + - the start of an expansion (``$``, backtick, ``%``) becomes ``*/`` and + swallows the identifier run naming it: ``*`` because its text is + unknown, ``/`` because its extent is unknown too, so what follows + cannot be assumed to continue the same path component, and swallowing + ``D`` in ``$D`` so the expansion reads as one unknown thing. ``%`` is + an expansion in every state, quoted or not, because the program that + fills it in is the next one along, not this shell; + - a quoted span containing ``%`` keeps its metacharacters live, because + such a span is a template rather than text: ``printf + '%s.mure?/x'`` builds a name whose ``?`` the shell then globs. The + flag resets at the end of the span, so a ``%`` in one argument cannot + animate the metacharacters of a later one — ``echo "100%" ; sed + 's/.*//'`` is still allowed. A brace group is replaced by ``.*`` when any alternative contains a dot and by ``*`` otherwise, before the components are cut. That is what @@ -145,7 +169,13 @@ - a component holding an expansion is unknown text, so ``ls .$X`` and ``cat .$(cmd)`` deny. An arithmetic expansion is not treated any differently, so ``echo .$((1+1))`` and ``cat ~/.mure$((0))?/x`` deny - too, though neither can reach the directory. + too, though neither can reach the directory; + - a format string that builds ``.`` is the shape + of ``printf '%s.mureo/…' ~/``, and nothing in the text distinguishes + them, so ``printf '%s.%s' a b`` denies. Of twenty ``%``-heavy + everyday commands (``date +%Y-%m-%d``, ``git log --format=%h``, + ``awk '{printf "%.2f", $1}'``, ``grep '100%'``, a commit message + reading ``30% faster``) that is the only one that does. What the guard does not cover — measured, not assumed, and pinned by ``test_known_open_bypasses``: @@ -169,19 +199,31 @@ literal siblings such as ``~/.mureo_backup``. The first two are not closable by inspecting command text, and no - further rule should be added pretending otherwise. A random - differential fuzz against a real bash (2500 commands that spell the - directory name one character at a time, using every quoting, escaping, - line-continuation, class, range, brace and substitution form) found 1995 - that really read the file: of the 825 whose name is written out in the - text, 0 got through; of the 1170 assembled at runtime, 172 did — all of - them producing the leading dot from a substitution. - - That fuzz is also how the line-continuation family should have been - found, and was not: the generator had no ``\\`` among its - escaping forms, so a whole lexer-level rewrite went untested while the - numbers above looked complete. A form the generator cannot produce is a - form nothing here has checked; extend it before trusting it. + further rule should be added pretending otherwise. + + Two differential tests against a real bash back that up, both checking + what the shell actually reads rather than what the rule thinks: + + - an exhaustive product of {how the parent directory is supplied: + literal, ``$HOME``, ``"$HOME"``, ``$VAR``, ``"$VAR"``, ``${VAR}``, + ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, backtick} x {how the name is + broken: not at all, continuation, two continuations, single-quote + split, single-quoted character, double-quote split, double-quoted + character, escaped character, class, wildcard, brace, star} x {where}. + All 560 members read the credentials file, and all 560 deny; + - a random fuzz that spells the name one character at a time in the same + forms: of 2500 commands, 1995 read the file — 825 with the name + written out in the text, 0 through; 1170 assembled at runtime, 172 + through, every one of them producing the leading dot from a + substitution. + + Both of the last two rounds of bugs were products of two axes, and both + times the generator had only walked the margins: it emitted continuations + and it emitted substitutions, but never a continuation *inside* a + substituted parent, which is exactly where the guard was blind. A form + the generator cannot produce is a form nothing here has checked — and + that applies to combinations, not just to features. Extend it before + trusting it. Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the @@ -288,31 +330,44 @@ def _deny_expr(reason: str) -> str: " or lp==bl or lp.startswith(bl+os.sep)) else None" ) -# Shell metacharacters, named once. None of them may appear literally in -# the payload (see the NOTE in the module docstring), so each arrives as a -# chr() call: q1 ', q2 ", bs backslash, dl $, tk backtick, nl newline. +# Shell metacharacters, named once. Most may not appear literally in the +# payload (see the NOTE in the module docstring), so each arrives as a +# chr() call: q1 ', q2 ", bs backslash, dl $, tk backtick, nl newline, +# pc %. `ho` is the placeholder a quoted metacharacter collapses to — it +# must be none of: an identifier character (it has to read as a component +# boundary), a metacharacter, or a dot. _CHARS = ( "q1=chr(39); q2=chr(34); bs=chr(92); dl=chr(36); tk=chr(96); nl=chr(10); " - "mt='*?[]{},'; " + "pc=chr(37); ho='='; mt='*?[]{},'; " ) -# The quoting automaton, as the step function of a left fold. States: -# 0 unquoted, 1 single-quoted, 2 double-quoted, 3 escaped (from unquoted), -# 4 escaped (inside double quotes). Inside single quotes nothing is -# special, not even a backslash — the rule bash applies. +# The quoting automaton, as the step function of a left fold. The state is +# a pair. First, where we are: 0 unquoted, 1 single-quoted, 2 +# double-quoted, 3 escaped (from unquoted), 4 escaped (inside double +# quotes). Inside single quotes nothing is special, not even a backslash — +# the rule bash applies. +# +# Second, whether a `%` has appeared in the quoted span we are inside. A +# quoted string is ordinary text, unless a program is going to build a path +# out of it: ``printf '%s.mure?/x'`` is a template whose metacharacters +# survive into a filename, and the shell then globs the result. The flag +# resets on leaving the span, so the `%` in one argument cannot make the +# metacharacters of a later one live. _QUOTE_STEP = ( - "lambda k,x: (1 if x==q1 else 2 if x==q2 else 3 if x==bs else 0) if k==0" - " else (0 if x==q1 else 1) if k==1" - " else (0 if x==q2 else 4 if x==bs else 2) if k==2" - " else (0 if k==3 else 2)" + "lambda kv,x: (" + "(1 if x==q1 else 2 if x==q2 else 3 if x==bs else 0) if kv[0]==0" + " else (0 if x==q1 else 1) if kv[0]==1" + " else (0 if x==q2 else 4 if x==bs else 2) if kv[0]==2" + " else (0 if kv[0]==3 else 2)," + " 1 if x==pc else (kv[1] if kv[0] else 0))" ) # Rebuild the command with quoting resolved, one character at a time: drop # the delimiters; drop the newline of a line continuation, since a shell # removes the pair before it tokenises anything; turn a quoted -# metacharacter into `_`, because quoting makes it an ordinary character -# and no ordinary character in `.mureo` is a metacharacter; and turn the -# start of an expansion into `*/`. +# metacharacter into the placeholder, because quoting makes it an ordinary +# character and no ordinary character in `.mureo` is a metacharacter; and +# turn the start of an expansion into `*/`. # # `*` because its text is unknown, and `/` because where it *ends* is # unknown too: the characters after it in the command (`o` in `.mure$X`, @@ -323,13 +378,25 @@ def _deny_expr(reason: str) -> str: # continuation. Inside single quotes a backslash is an ordinary character, # so `.mu\\reo` in single quotes really is a name with a newline # in it, and normalizing it away would over-block rather than protect. +# +# `%` becomes an expansion in *every* state, quoted or not, because it is +# the next program along that expands it, not this shell. _NORMALIZE = ( "''.join('' if (k==0 and x in q1+q2+bs) or (k==1 and x==q1)" " or (k==2 and x in q2+bs) or (k>2 and x==nl)" - " else ('*/' if x in dl+tk and k in (0,2) else ('_' if k and x in mt else x))" - " for x,k in zip(c,st))" + " else ('*/' if x in dl+tk+pc else (ho if k and not m and x in mt else x))" + " for x,(k,m) in zip(c,st))" ) +# An expansion swallows the identifier run that names it: `$D` and `%s` are +# one unknown thing, not an unknown thing followed by the letters `d`/`s`. +# Collapsing them is what lets a single reading serve both rules — after +# it, `$D.mureo` reads as `*/.mureo`, whose dot sits at a boundary exactly +# like the one in `~/.mureo`, so the literal rule needs no separate scan of +# the raw text to find it. This cannot hide a name: it removes only +# identifier characters, and every form the guard looks for contains a dot. +_COLLAPSE = "re.sub('[*]/[a-z0-9_]*', '*/', t)" + # A brace group stands for any of its alternatives. One that contains a dot # can supply the leading dot of a dotfile, so it becomes `.*`; any other # becomes `*`. Applied twice, which covers a group nested in a group. @@ -355,18 +422,15 @@ def _deny_expr(reason: str) -> str: + _CHARS + "st=list(itertools.accumulate(c, " + _QUOTE_STEP - + ", initial=0)); " + + ", initial=(0,0))); " + # One reading of the command, built once, consumed by both rules below. "t=" + _NORMALIZE + "; " - "gr='[{][^{}]*[}]'; fb=lambda m: '.*' if '.' in m.group() else '*'; " + "t=" + _COLLAPSE + "; " + "gr='[{][^{}]*[}]'; fb=lambda w: '.*' if '.' in w.group() else '*'; " "t=" + _DEBRACE + "; " - # The literal rules read the command as written and as the shell will - # read it, so quoting cannot reassemble the name unseen. - "j=c + ' ' + t; " - "p=re.findall('(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', t) + " - "re.findall('[' + dl + '%][a-z0-9_]*(' + " + _PATTERN_COMPONENT + " + ')', c); " + "p=re.findall('(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', t); " "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " - "b=re.search('(^|[^a-z0-9_])[.]mureo', j) or " - "re.search('[' + dl + '%][a-z0-9_]*[.]mureo', j) or g; " + "b=re.search('(^|[^a-z0-9_])[.]mureo', t) or g; " + _deny_expr(_BASH_REASON) + " if b else None" ) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index fcf12d38..2db33207 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -464,6 +464,25 @@ class TestGuardThroughARealShell: # `$"..."` is a translated string: the `$` is not an expansion # of anything the guard cannot see. 'cat ~/$".mureo"/credentials.json', + # The parent directory comes from a substitution *and* the name + # is broken by something only normalization resolves. This is + # the product of the two axes, and it is the category that + # shipped twice: while the two rules read different strings, + # whatever one of them folded away the other could not see. + "D2=~/; cat $D2.mu\\\nreo/credentials.json", + 'D2=~/; cat "$D2".mu\\\nreo/credentials.json', + "set -- ~/; cat $1.mu\\\nreo/credentials.json", + "D2=~/; E2=; cat $D2$E2.mu\\\nreo/credentials.json", + "D2=~/; cat ${D2}.mu\\\nreo/credentials.json", + "D2=~/; cat $D2.\\\nmureo/credentials.json", + "D2=~/; cat $D2.m\\\nur\\\neo/credentials.json", + 'D2=~/; cat $D2.mure"o"/credentials.json', + "D2=~/; cat $D2.mur'e'o/credentials.json", + "D2=~/; cat $D2.mure?/credentials.json", + 'D2=~/; cat "$D2".mure[o]/credentials.json', + "set -- ~/; cat $1.mure{o,x}/credentials.json", + "cat $(printf '%s' ~/).mu\\\nreo/credentials.json", + "cat `printf '%s' ~/`.mure?/credentials.json", # ...and the plain forms still deny through the shell layer. "cat ~/.mureo/credentials.json", "cat ~/.mure?/credentials.json", From 69eb84a072c18d14942362fa644bc452b0dafafc Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 08:43:33 +0900 Subject: [PATCH 06/11] fix: expand brace groups instead of guessing what they contain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cat ~/.{mureo,x.y}/credentials.json` reads the credentials file. The group was folded to a single placeholder chosen by asking whether the span contained a dot anywhere, never where: a dot before the group and a dot inside an unrelated alternative combined into `..*`, which wants two leading dots, while the literal `.mureo` rule 1 would have matched had already been replaced. Both rules passed. Brace groups are now expanded rather than approximated. The normalized command becomes the list of strings the shell would produce, and every rule runs against all of them, so nothing depends on a guess about any one of them. `~/.{mureo,x.y}`, `.mure{o,x.y}`, `.m{ureo,x.y}`, `.{MUREO,x.y}`, `$HOME/.{mureo,x.y}`, `.{mure?,x.y}`, `.{a.b,c.d,mureo}` and the nested `.{mureo,{a,b}.c}` all deny. Two forms found while writing the expansion, which the report did not list and which the old fold also missed: sequence expressions. `cat ~/.{l..n}ureo/credentials.json` and `cat ~/.mure{n..p}/…` read the file, and the letter `m` appears nowhere in either. A sequence is not a list of alternatives that can be enumerated by splitting on commas, so it falls back to *both* coarse readings -- `*` and `.*` -- which between them cover "supplies a leading dot" and "does not". That pair is precisely what the old single guess was missing; where a guess is unavoidable, take both branches rather than picking one. Expansion also removes an over-block, as predicted: `mv .{foo,bar}` and `rm .{a,b,c}` are allowed again, each alternative now being judged on its own. And a group with neither a comma nor a `..` is not brace expansion at all -- bash leaves `{eo}` literal -- so `~/.mur{eo}` is allowed, matching what the shell actually does rather than over-blocking it. The generator gained the dimension this round exposed. It could vary which breaking form was used and where, but every brace alternative was inert filler, so a group holding an unrelated dot could not be produced at all. It is now a product over {parent supplied by} x {name broken by} x {what the breaking form contains: plain, an alternative with its own dot, with two, a backup-looking name, a nested group, a metacharacter, a leading dot} x {where}. 1510 members, every one of which reads the file in real bash, and all 1510 deny. Unchanged elsewhere: 73 everyday commands 0 denied, 51 must-deny forms 0 through, the 10 mureo identifiers of #567 0 denied, the continuation and substitution families still deny, fail-closed still holds, and the five known-open rows are still exactly those five. The docstring now lists the coarse approximations that remain after this -- an expansion's text, an expansion's extent, a `%` template's result, a sequence group, an oversized group -- and says that computing what the shell would produce, rather than approximating it, is the shape of the fix for each. --- mureo/credential_guard.py | 134 +++++++++++++++++++++++++-------- tests/test_credential_guard.py | 18 +++++ 2 files changed, 122 insertions(+), 30 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index ae0049b5..f07a061f 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -150,19 +150,31 @@ animate the metacharacters of a later one — ``echo "100%" ; sed 's/.*//'`` is still allowed. - A brace group is replaced by ``.*`` when any alternative contains a dot - and by ``*`` otherwise, before the components are cut. That is what - catches both ``~/.mure{o,x}`` and ``~/{.,z}mureo``, where the group - supplies the leading dot itself. + Brace groups are then *expanded*, not approximated: the normalized text + becomes the list of strings the shell would produce, and every rule runs + against all of them. ``~/.mure{o,x}`` and ``~/{.,z}mureo`` are caught + because ``.mureo`` is literally among the results. + + An earlier version folded each group to one placeholder and guessed + which — ``.*`` if the group held a dot anywhere, ``*`` otherwise — and + the guess is what broke. ``~/.{mureo,x.y}`` has a dot before the group + and a dot inside an alternative that has nothing to do with the + directory; the fold read them as one, produced ``..*``, which requires + two leading dots, and meanwhile the literal ``.mureo`` that rule 1 would + have matched had already been replaced. Both rules passed and the file + was read. Expanding removes the guess instead of refining it. + + Two groups are not lists of alternatives and cannot be enumerated this + way: a sequence (``.{l..n}ureo`` covers ``m`` without the letter + appearing anywhere) and one with absurdly many alternatives. Those fall + back to *both* coarse readings, ``*`` and ``.*``, which between them + cover "supplies a leading dot" and "does not" — the pair the single + guess was missing. A group with neither a comma nor a ``..`` is not + brace expansion at all; bash leaves ``{eo}`` literal, so the guard does + too, and ``~/.mur{eo}`` is allowed because it opens nothing. Deliberate over-blocks, all in the safe direction: - - a brace group with no dot in it becomes ``*``, so ``mv .{foo,bar}`` - and ``rm .{a,b,c}`` deny although neither can name the directory. - (``mv .{env,env.bak}`` does not: the dot in an alternative makes the - replacement ``.*``, which cannot match a six-character name starting - with a single dot.) Expanding the alternatives exactly would fix - this, and is the change to make if it ever gets in the way; - anything unquoted that really does glob dotfiles: ``ls .*``, ``ls -d .??*``, ``rm -rf .[!.]*`` all reach ``~/.mureo`` from ``$HOME`` and all deny; @@ -177,6 +189,18 @@ ``awk '{printf "%.2f", $1}'``, ``grep '100%'``, a commit message reading ``30% faster``) that is the only one that does. + Brace expansion used to be on this list — ``mv .{foo,bar}`` and ``rm + .{a,b,c}`` denied although neither can name the directory. Expanding + the alternatives exactly, rather than folding them to a placeholder, + removed those: each alternative is now judged on its own, and both are + allowed. That is the shape of the right fix for the remaining entries + too — compute what the shell would produce instead of approximating it. + + The coarse approximations that are left, and would each have to be + replaced the same way: an expansion's *text* (unknowable, so ``*``), an + expansion's *extent* (unknowable, so ``/``), a ``%`` template's result, + a sequence group, and a group with more than 64 alternatives. + What the guard does not cover — measured, not assumed, and pinned by ``test_known_open_bypasses``: @@ -209,21 +233,30 @@ ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, backtick} x {how the name is broken: not at all, continuation, two continuations, single-quote split, single-quoted character, double-quote split, double-quoted - character, escaped character, class, wildcard, brace, star} x {where}. - All 560 members read the credentials file, and all 560 deny; + character, escaped character, class, wildcard, brace here, brace tail, + brace whole, sequence, star} x {what the breaking form contains: plain, + an alternative with its own dot, with two, a backup-looking name, a + nested group, a metacharacter, a leading dot} x {where}. All 1510 + members read the credentials file, and all 1510 deny; - a random fuzz that spells the name one character at a time in the same forms: of 2500 commands, 1995 read the file — 825 with the name written out in the text, 0 through; 1170 assembled at runtime, 172 through, every one of them producing the leading dot from a substitution. - Both of the last two rounds of bugs were products of two axes, and both - times the generator had only walked the margins: it emitted continuations - and it emitted substitutions, but never a continuation *inside* a - substituted parent, which is exactly where the guard was blind. A form - the generator cannot produce is a form nothing here has checked — and - that applies to combinations, not just to features. Extend it before - trusting it. + Each of the last three rounds of bugs was a product of axes the + generator only walked the margins of. It emitted continuations and it + emitted substitutions, but never a continuation *inside* a substituted + parent. Then it emitted brace groups, but every alternative was inert + filler, so a group holding an unrelated dot — the thing that broke the + fold — could not be produced. That is why there is now a dimension for + what a breaking form *contains*, not only for which form is used. + + Take the pattern seriously rather than the instances: a form the + generator cannot produce is a form nothing here has checked, and that + applies to the insides of forms and to combinations of them, not only to + the list of features. Before trusting a number in this docstring, look + at whether the generator can express the shape it is claiming to cover. Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the @@ -397,10 +430,46 @@ def _deny_expr(reason: str) -> str: # identifier characters, and every form the guard looks for contains a dot. _COLLAPSE = "re.sub('[*]/[a-z0-9_]*', '*/', t)" -# A brace group stands for any of its alternatives. One that contains a dot -# can supply the leading dot of a dotfile, so it becomes `.*`; any other -# becomes `*`. Applied twice, which covers a group nested in a group. -_DEBRACE = "re.sub(gr, fb, re.sub(gr, fb, t))" +# Brace expansion, done properly: the command is turned into the *list* of +# strings the shell would produce, and every rule runs against all of them. +# +# The previous version folded a group to one placeholder and guessed which: +# `.*` if the group contained a dot anywhere, `*` otherwise. It never asked +# *where* the dot was, so `~/.{mureo,x.y}` — a dot before the group and a +# dot inside an unrelated alternative — folded to `..*`, which wants two +# leading dots, while the literal `.mureo` that rule 1 would have caught had +# already been replaced. It read the credentials file. Expanding removes the +# guess rather than refining it, and it also stops over-blocking +# `mv .{foo,bar}`, since each alternative is now judged on its own. +# +# `fe` finds the first *expandable* innermost group, skipping `{a}`, which +# bash leaves alone — a group is expandable only with a comma or a `..`. +# `al` gives its alternatives; a sequence (`{l..n}`) is not a list of +# alternatives this can enumerate, and neither is a group with absurdly many +# of them, so those fall back to the two coarse readings — `*` and `.*` — +# which between them cover both "supplies a leading dot" and "does not". +# That pair is what the old single guess was missing. +_BRACE_HELPERS = ( + "ga='[{][^{}]*[}]'; " + "fe=lambda s: next((w for w in re.finditer(ga, s)" + " if ',' in w.group() or '..' in w.group()), None); " + "al=lambda w: (lambda v: v if ',' in w.group() and len(v)<=64 else ['*','.*'])" + "(w.group()[1:-1].split(',')); " + "ex=lambda s: (lambda w: [s[:w.start()] + a + s[w.end():] for a in al(w)]" + " if w else [s])(fe(s)); " +) + +# Eight passes expand eight groups, innermost first, so nesting resolves as +# the outer group becomes innermost. The cap keeps a pathological command +# from exploding the hook: exceeding it abandons that pass and leaves the +# groups for the coarse fallback below, which over-approximates rather than +# dropping candidates. +_EXPAND = ( + "ls=functools.reduce(lambda acc,_: (lambda n: n if len(n)<=400 else acc)" + "([y for x in acc for y in ex(x)]), range(8), [t]); " + "ls=[y for x in ls for y in ([x] if not fe(x) else" + " [re.sub(ga,'*',re.sub(ga,'*',x)), re.sub(ga,'.*',re.sub(ga,'.*',x))])]; " +) # Source of a python expression yielding the regex for one path component # written as a shell pattern: a literal dot plus the run of characters a @@ -412,7 +481,7 @@ def _deny_expr(reason: str) -> str: _BASH_REASON = "mureo credential guard: commands that can reach ~/.mureo are blocked" _BASH_GUARD_CODE = ( - "import sys,json,re,os,fnmatch,itertools; " + "import sys,json,re,os,fnmatch,functools,itertools; " # Fail closed: an escaping exception exits 1, which both hosts treat as a # non-blocking hook error, so every exception must deny instead. "sys.excepthook=lambda *a: (" + _deny_expr(_BASH_REASON) + ", " @@ -423,14 +492,19 @@ def _deny_expr(reason: str) -> str: + "st=list(itertools.accumulate(c, " + _QUOTE_STEP + ", initial=(0,0))); " - # One reading of the command, built once, consumed by both rules below. + # One reading of the command, built once. Brace expansion turns it into + # the list of readings the shell would produce; both rules see all of + # them, so neither depends on a guess about any single one. "t=" + _NORMALIZE + "; " - "t=" + _COLLAPSE + "; " - "gr='[{][^{}]*[}]'; fb=lambda w: '.*' if '.' in w.group() else '*'; " - "t=" + _DEBRACE + "; " - "p=re.findall('(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', t); " + "t=" + + _COLLAPSE + + "; " + + _BRACE_HELPERS + + _EXPAND + + "p=[x for s in ls for x in re.findall(" + "'(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', s)]; " "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " - "b=re.search('(^|[^a-z0-9_])[.]mureo', t) or g; " + "b=[s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + _deny_expr(_BASH_REASON) + " if b else None" ) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 2db33207..0046d44e 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -449,6 +449,24 @@ class TestGuardThroughARealShell: "cat ~/{.,z}mureo/credentials.json", "cat ~/.mure{o,x}/credentials.json", "cat ~/.mur{e{o,z},y}/credentials.json", + # An alternative carrying its own dot, which is unrelated to the + # dot of the dotfile. Folding the group to a single placeholder + # had to guess which of the two the dot belonged to, and chose + # wrong: these read the credentials file. + "cat ~/.{mureo,x.y}/credentials.json", + "cp -r ~/.{mureo,bashrc.bak} /tmp/dest/", + "ls -la ~/.{mureo,x.y}", + "cat ~/.mure{o,x.y}/credentials.json", + "cat ~/.m{ureo,x.y}/credentials.json", + "cat ~/.{MUREO,x.y}/credentials.json", + "cat $HOME/.{mureo,x.y}/credentials.json", + "cat ~/.{mure?,x.y}/credentials.json", + "cat ~/.{a.b,c.d,mureo}/credentials.json", + "cat ~/.{mureo,{a,b}.c}/credentials.json", + # A sequence expression is not a list of alternatives at all, + # and `{l..n}` covers `m` without the letter appearing anywhere. + "cat ~/.{l..n}ureo/credentials.json", + "cat ~/.mure{n..p}/credentials.json", # A line continuation is deleted, backslash and newline both, # before the shell tokenises anything — so the name is spelled # across two lines and is contiguous by the time it is used. From 8ae528bca6f3488b983faeb18ecb2b4522f34767 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 09:22:03 +0900 Subject: [PATCH 07/11] fix: refuse brace structure the expansion budget cannot resolve The budget was the bug. Expansion runs eight passes and then, until now, handed whatever was left to a coarse fallback of two `re.sub` collapses. Past ten levels of nesting that left literal `{` and `}` in the candidates -- ordinary characters to fnmatch, not syntax -- so neither rule fired at all: cat ~/.{z11,{z10,{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}}}}}}}}}/credentials.json Ninety characters, no exotic syntax, and bash reads the file. With three alternatives per level the cliff is at depth 8, with five at depth 6. Measured across depths 1-20 x {2,3,5} alternatives: 60 cells, every one of which reads the file, 38 of them allowed. The docstring asserted the opposite -- that the fallback "over-approximates rather than dropping candidates". It dropped them. That sentence is corrected in place rather than quietly replaced, because the pattern it belongs to matters more than the sentence. The fix is a rule, not a bigger budget: when the guard cannot compute what the shell would produce, it denies. Anything still holding an expandable group after the passes denies on that ground alone, and the coarse fallback is gone. The budget stays at eight passes and 400 strings, since it exists to stop a fork bomb; exceeding it is now a refusal instead of a shrug. All 60 cells deny. What that refuses is a command with more than eight brace groups or an expansion of more than 400 strings. Of twenty-one brace-using everyday commands -- awk '{print $1}', find . -exec rm {} ;, mkdir -p build/{lib,bin,share}, mv file{1..10}.txt, jq '{name: .name}', eight groups on one line -- exactly one is refused: nine groups on one line. Quoted braces never reach the step, and a group with no comma and no `..` is literal to bash and to the guard. Also fixed rather than documented: the group regex used `[^{}]*`, which matches newlines, so unrelated braces on separate lines of a multi-line command could pair up and swallow what lay between them. An unquoted newline is a token separator and bash will not expand a group across one, so the newline is now excluded and the two `echo '{'` / `echo '}'` lines stay independent. The generator gained nesting depth as a dimension. It had "a nested group" as one fixed-depth filler, so all 1510 members sat at depth two or less and the cliff at eleven could not appear. It is now a product over {parent supplied by} x {name broken by} x {what the form contains} x {how deeply it nests: 0,1,2,3,5,8,9,11,14,20} x {where}: 2698 members, every one of which reads the file in real bash, and all 2698 deny. Rounds four, five and six were one shape: a defence reading an approximation instead of the text, and failing open where it could not compute. Expansion removed the guess for braces and the budget put a silent one back. The docstring now carries the general rule -- prefer a rule that fails closed on unresolved structure over a measurement that says the gap is unreachable -- and notes that each round's missing generator dimension was one level inside the last one added. --- mureo/credential_guard.py | 129 ++++++++++++++++++++++++--------- tests/test_credential_guard.py | 26 +++++++ 2 files changed, 119 insertions(+), 36 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index f07a061f..8cc8756c 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -173,6 +173,30 @@ brace expansion at all; bash leaves ``{eo}`` literal, so the guard does too, and ``~/.mur{eo}`` is allowed because it opens nothing. + Expansion has a budget — eight passes, 400 strings — so a pathological + command cannot explode the hook. **Whatever the budget does not resolve + is refused.** Anything still holding an expandable group after the + passes denies on that ground alone, without being examined further. + + That rule replaced a fallback that collapsed leftovers coarsely, and it + is worth saying plainly why, because the docstring claimed the fallback + "over-approximates rather than dropping candidates" and that was false. + Past ten levels of nesting the collapse left literal ``{`` and ``}`` in + the candidates, which ``fnmatch`` reads as ordinary characters, so + *neither* rule fired: ``cat ~/.{z11,{z10,…{z1,mureo}}}/…`` — ninety + characters, no exotic syntax — was allowed while bash read the file. A + budget that shrugs is a bypass with a length requirement. The general + form of the rule is: when the guard cannot compute what the shell would + produce, it denies. + + What that refuses in practice is a command with more than eight brace + groups, or one whose expansion exceeds 400 strings. Of twenty-one + brace-using everyday commands — ``awk '{print $1}'``, ``find . -exec rm + {} ;``, ``mkdir -p build/{lib,bin,share}``, ``mv file{1..10}.txt``, + ``jq '{name: .name}'``, eight groups on one line — exactly one is + refused: nine groups on one line. Quoted braces never reach this step, + and a group with no comma and no ``..`` is literal to bash and to ``fe``. + Deliberate over-blocks, all in the safe direction: - anything unquoted that really does glob dotfiles: ``ls .*``, ``ls -d @@ -187,19 +211,24 @@ them, so ``printf '%s.%s' a b`` denies. Of twenty ``%``-heavy everyday commands (``date +%Y-%m-%d``, ``git log --format=%h``, ``awk '{printf "%.2f", $1}'``, ``grep '100%'``, a commit message - reading ``30% faster``) that is the only one that does. - - Brace expansion used to be on this list — ``mv .{foo,bar}`` and ``rm - .{a,b,c}`` denied although neither can name the directory. Expanding - the alternatives exactly, rather than folding them to a placeholder, - removed those: each alternative is now judged on its own, and both are - allowed. That is the shape of the right fix for the remaining entries - too — compute what the shell would produce instead of approximating it. - - The coarse approximations that are left, and would each have to be - replaced the same way: an expansion's *text* (unknowable, so ``*``), an - expansion's *extent* (unknowable, so ``/``), a ``%`` template's result, - a sequence group, and a group with more than 64 alternatives. + reading ``30% faster``) that is the only one that does; + - brace structure the expansion budget could not resolve: more than + eight groups in one command, or an expansion of more than 400 strings. + + Brace expansion itself used to be on this list — ``mv .{foo,bar}`` and + ``rm .{a,b,c}`` denied although neither can name the directory. + Expanding the alternatives exactly, rather than folding them to a + placeholder, removed those: each alternative is judged on its own, and + both are allowed. That is the shape of the right fix for the remaining + entries — compute what the shell would produce instead of approximating + it — and where that is impossible, refuse rather than approximate. + + The coarse approximations that are left: an expansion's *text* + (unknowable, so ``*``), an expansion's *extent* (unknowable, so ``/``), + and a ``%`` template's result. Two more — a sequence group and a group + with more than 64 alternatives — still take both coarse readings rather + than being enumerated; enumerating them is a contained change and the + place to start if this list is ever shortened again. What the guard does not cover — measured, not assumed, and pinned by ``test_known_open_bypasses``: @@ -236,27 +265,35 @@ character, escaped character, class, wildcard, brace here, brace tail, brace whole, sequence, star} x {what the breaking form contains: plain, an alternative with its own dot, with two, a backup-looking name, a - nested group, a metacharacter, a leading dot} x {where}. All 1510 - members read the credentials file, and all 1510 deny; + nested group, a metacharacter, a leading dot} x {how deeply it is + nested: 0, 1, 2, 3, 5, 8, 9, 11, 14, 20 levels} x {where}. All 2698 + members read the credentials file, and all 2698 deny; + - the nesting cliff specifically, at every depth from 1 to 20 with two, + three and five alternatives per level: 60 cells, every one of which + reads the file, and every one denied. Before the refusal rule 38 of + them were allowed — the cliff sat at depth 11, 8 and 6 respectively; - a random fuzz that spells the name one character at a time in the same forms: of 2500 commands, 1995 read the file — 825 with the name written out in the text, 0 through; 1170 assembled at runtime, 172 through, every one of them producing the leading dot from a substitution. - Each of the last three rounds of bugs was a product of axes the - generator only walked the margins of. It emitted continuations and it - emitted substitutions, but never a continuation *inside* a substituted - parent. Then it emitted brace groups, but every alternative was inert - filler, so a group holding an unrelated dot — the thing that broke the - fold — could not be produced. That is why there is now a dimension for - what a breaking form *contains*, not only for which form is used. - - Take the pattern seriously rather than the instances: a form the - generator cannot produce is a form nothing here has checked, and that - applies to the insides of forms and to combinations of them, not only to - the list of features. Before trusting a number in this docstring, look - at whether the generator can express the shape it is claiming to cover. + Each round of bugs here has been a product of axes the generator only + walked the margins of. It emitted continuations and it emitted + substitutions, but never a continuation *inside* a substituted parent. + Then it emitted brace groups, but every alternative was inert filler, so + a group holding an unrelated dot could not be produced. Then it had a + "nested group" filler at one fixed depth, so 1510 members all sat at + depth two or less and the cliff at eleven was invisible. Each time the + missing dimension was one level *inside* the last one added. + + Take the pattern rather than the instances: a form the generator cannot + produce is a form nothing here has checked, and that applies to the + insides of forms, to how deeply they nest, and to combinations of them, + not only to the list of features. Before trusting a number in this + docstring, look at whether the generator can express the shape it claims + to cover — and prefer a rule that fails closed on what it cannot resolve + over a measurement that says the gap is not reachable. Both comparisons are case-folded: macOS and Windows filesystems are case-insensitive by default, so ``~/.MUREO/credentials.json`` opens the @@ -449,8 +486,14 @@ def _deny_expr(reason: str) -> str: # of them, so those fall back to the two coarse readings — `*` and `.*` — # which between them cover both "supplies a leading dot" and "does not". # That pair is what the old single guess was missing. +# +# `ga` excludes the newline from a group's contents, because an unquoted +# newline is a token separator: bash will not expand a brace group across +# one, so neither should this. Matching across newlines would also let two +# unrelated braces on different lines of a multi-line command pair up and +# swallow everything between them. _BRACE_HELPERS = ( - "ga='[{][^{}]*[}]'; " + "ga='[{][^{}' + nl + ']*[}]'; " "fe=lambda s: next((w for w in re.finditer(ga, s)" " if ',' in w.group() or '..' in w.group()), None); " "al=lambda w: (lambda v: v if ',' in w.group() and len(v)<=64 else ['*','.*'])" @@ -460,15 +503,28 @@ def _deny_expr(reason: str) -> str: ) # Eight passes expand eight groups, innermost first, so nesting resolves as -# the outer group becomes innermost. The cap keeps a pathological command -# from exploding the hook: exceeding it abandons that pass and leaves the -# groups for the coarse fallback below, which over-approximates rather than -# dropping candidates. +# the outer group becomes innermost, and a cap stops a pathological command +# from exploding the hook. +# +# Whatever is left when the budget runs out is *refused*, not approximated: +# `un` collects the candidates that still hold an expandable group, and a +# non-empty `un` denies on that ground alone. The budget used to end in a +# coarse fallback of two `re.sub` collapses, which past ten levels of +# nesting left literal braces in the candidates — text `fnmatch` reads as +# ordinary characters, so neither rule fired and +# `~/.{z11,{z10,...{z1,mureo}}}` was allowed while bash read the file. A +# budget that shrugs is a bypass with a length requirement. +# +# The rule is general: when the guard cannot compute what the shell would +# produce, it denies. Nothing legitimate is refused by it — a quoted +# `awk '{print $1}'` never reaches this step, and `find . -exec {} \\;` has +# neither a comma nor a `..`, so bash leaves it literal and so does `fe`. +# What is left is a command with more than eight brace groups, or one whose +# expansion exceeds 400 strings, and neither is a thing anyone types. _EXPAND = ( "ls=functools.reduce(lambda acc,_: (lambda n: n if len(n)<=400 else acc)" "([y for x in acc for y in ex(x)]), range(8), [t]); " - "ls=[y for x in ls for y in ([x] if not fe(x) else" - " [re.sub(ga,'*',re.sub(ga,'*',x)), re.sub(ga,'.*',re.sub(ga,'.*',x))])]; " + "un=[x for x in ls if fe(x)]; " ) # Source of a python expression yielding the regex for one path component @@ -504,7 +560,8 @@ def _deny_expr(reason: str) -> str: + "p=[x for s in ls for x in re.findall(" "'(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', s)]; " "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " - "b=[s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + # `un` first: structure the guard could not resolve denies on its own. + "b=un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + _deny_expr(_BASH_REASON) + " if b else None" ) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 0046d44e..4f40a4ae 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -467,6 +467,18 @@ class TestGuardThroughARealShell: # and `{l..n}` covers `m` without the letter appearing anywhere. "cat ~/.{l..n}ureo/credentials.json", "cat ~/.mure{n..p}/credentials.json", + # Nested past the expansion budget. The budget used to end in a + # coarse fallback that left literal braces in the candidates — + # ordinary characters to fnmatch — so neither rule fired and the + # file was read. Unresolved structure now denies on its own. + "cat ~/.{z2,{z1,mureo}}/credentials.json", + "cat ~/.{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}}}}}}}/creds", + "cat ~/.{z11,{z10,{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}" + "}}}}}}}}/credentials.json", + "cp -r ~/.{z11,{z10,{z9,{z8,{z7,{z6,{z5,{z4,{z3,{z2,{z1,mureo}}}" + "}}}}}}}}/ /tmp/dest/", + # Three and five alternatives per level reach the budget sooner. + "cat ~/.{a,b,{c,d,{e,f,{g,h,{i,j,{k,l,{m,n,mureo}}}}}}}/creds", # A line continuation is deleted, backslash and newline both, # before the shell tokenises anything — so the name is spelled # across two lines and is contiguous by the time it is used. @@ -535,6 +547,20 @@ def test_denies_through_the_shell(self, fake_home: Path, command: str) -> None: # this is a name with a newline in it, not a continuation, and # it opens nothing. "cat '~/.mu\\\nreo/credentials.json'", + # Brace usage that is not an attempt at the directory. The + # refusal on unresolved structure must not reach these: the + # awk/jq bodies are quoted, `{}` has no comma, and eight groups + # sit inside the expansion budget. + "awk '{print $1}' data.txt", + "find . -name '*.pyc' -exec rm {} ;", + "mkdir -p build/{lib,bin,share}", + "mv report.{txt,md}", + "mv file{1..10}.txt archive/", + "mkdir -p a/{1,2}/b/{3,4}/c/{5,6}/d/{7,8}", + "kubectl get pods -o jsonpath='{.items[0].metadata.name}'", + # Braces on separate lines of a multi-line command must not pair + # up across the newline and swallow what lies between them. + "echo '{' > a.json\necho '}' >> a.json", # A continuation that only wraps a long line. "ls -la \\\n ~/project", # mureo's own identifiers, including inside quotes. From c61fb744ad3a8139e35e108fe4c57afe1cd824bd Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:13:28 +0900 Subject: [PATCH 08/11] fix: bound the guard's work, and commit the product its numbers come from Three things, one of which is about this file's claims rather than its behaviour. 1. A guard that is merely slow is a guard that is bypassed. sys.excepthook catches what Python raises; it cannot catch the host killing a hook that overruns, and that process exits non-zero WITHOUT printing the deny JSON -- the non-blocking case, where the tool call proceeds. A 4 MB command of nested brace groups took it there: no answer in 45 s, 1.95 GB resident, and one of the alternatives was `mureo`. Measured before: 496 KB 1.4 s, 2 MB 16.5 s, 4 MB timeout. Three bounds, all cheap. The command is refused unread above 64 KB, so the normalization never runs on it. Expansion is budgeted on total normalized bytes rather than on candidate count, which is what let per-candidate text grow without limit. And a pass that reverts stops the loop instead of letting the remaining seven recompute and discard the same expansion. Multi-megabyte bombs now answer in ~0.2 s; the worst case under the cap is 0.13 s. Found while fixing it: the early break compared list *length* to decide nothing had changed, but a sequence group expands to a single alternative, so `mkdir -p test{1..3}/{a..c}` was marked finished with a group still unexpanded and denied on the refusal rule. It compares content now. 2. A bare sequence group was over-blocked. `echo {1..100}`, `for i in {1..5}`, `printf '%s' {A..Z}` and `touch file{1..20}.log` all denied, because any sequence collapsed to `.*`. A sequence yields integers or single characters: integers hold no dot, and a character range holds one only if it spans ASCII 46. Consulting the range fixes all four while keeping `.{l..n}ureo` and `.mure{n..p}` denied, and it correctly denies `{-..0}`, whose range does contain the dot. 3. The docstring quoted "2698 members", "1510", "60 cells", "2500 fuzz" with no committed artifact anywhere. CI checked ~92 parametrised rows and nothing else, so those numbers were claims about the past, not properties of the code -- the same defect as every round, at a larger scale and with bigger numbers. tests/credential_guard_product.py now builds the product, and tests/test_credential_guard_product.py runs it: an evenly-strided sample of 118 on every commit, and all 2698 members under `pytest -m slow`, each executed in a throwaway HOME to confirm it really reaches the file before the guard is asked. The nesting-depth table and the resource bounds are tests too. The random single-character fuzz figure is deleted rather than restated, because nothing in the repository reproduces it. The slow run is opt-in through a fixture that checks the marker expression rather than through a global addopts filter: a marker that silently vanishes from the default run is how a suite ends up with checks nobody has executed in months. Unchanged: 73 everyday commands 0 denied, 51 must-deny forms 0 through, the 10 mureo identifiers of #567 0 denied, 21 brace-using commands 1 refused (nine groups on one line), depth table 60/60, all families still denied, fail-closed still holds, known-open still exactly five rows. --- mureo/credential_guard.py | 118 +++++++++++---- pyproject.toml | 1 + tests/credential_guard_product.py | 158 +++++++++++++++++++ tests/test_credential_guard.py | 13 ++ tests/test_credential_guard_product.py | 200 +++++++++++++++++++++++++ 5 files changed, 457 insertions(+), 33 deletions(-) create mode 100644 tests/credential_guard_product.py create mode 100644 tests/test_credential_guard_product.py diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index 8cc8756c..14ff27e8 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -213,7 +213,16 @@ ``awk '{printf "%.2f", $1}'``, ``grep '100%'``, a commit message reading ``30% faster``) that is the only one that does; - brace structure the expansion budget could not resolve: more than - eight groups in one command, or an expansion of more than 400 strings. + eight groups in one command, or an expansion whose normalized text + exceeds 200 KB; + - a command longer than 64 KB, which is refused unread (see below); + - a sequence whose character range spans ASCII 46, since one of the + things it produces is the dot itself: ``echo {-..0}`` denies. Ranges + that cannot produce a dot are read exactly, so ``echo {1..100}``, + ``for i in {1..5}``, ``printf '%s' {A..Z}`` and ``touch + file{1..20}.log`` are allowed — they were denied until the range was + consulted, which is the kind of over-block that teaches people to turn + a guard off. Brace expansion itself used to be on this list — ``mv .{foo,bar}`` and ``rm .{a,b,c}`` denied although neither can name the directory. @@ -254,29 +263,37 @@ The first two are not closable by inspecting command text, and no further rule should be added pretending otherwise. - Two differential tests against a real bash back that up, both checking - what the shell actually reads rather than what the rule thinks: - - - an exhaustive product of {how the parent directory is supplied: - literal, ``$HOME``, ``"$HOME"``, ``$VAR``, ``"$VAR"``, ``${VAR}``, - ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, backtick} x {how the name is - broken: not at all, continuation, two continuations, single-quote - split, single-quoted character, double-quote split, double-quoted - character, escaped character, class, wildcard, brace here, brace tail, - brace whole, sequence, star} x {what the breaking form contains: plain, - an alternative with its own dot, with two, a backup-looking name, a - nested group, a metacharacter, a leading dot} x {how deeply it is - nested: 0, 1, 2, 3, 5, 8, 9, 11, 14, 20 levels} x {where}. All 2698 - members read the credentials file, and all 2698 deny; - - the nesting cliff specifically, at every depth from 1 to 20 with two, - three and five alternatives per level: 60 cells, every one of which - reads the file, and every one denied. Before the refusal rule 38 of - them were allowed — the cliff sat at depth 11, 8 and 6 respectively; - - a random fuzz that spells the name one character at a time in the same - forms: of 2500 commands, 1995 read the file — 825 with the name - written out in the text, 0 through; 1170 assembled at runtime, 172 - through, every one of them producing the leading dot from a - substitution. + What is actually checked, and where — every number below is produced by + committed code, not by a measurement someone once took: + + - ``tests/credential_guard_product.py`` builds a product of {how the + parent directory is supplied: literal, ``$HOME``, ``"$HOME"``, + ``$VAR``, ``"$VAR"``, ``${VAR}``, ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, + backtick} x {how the name is broken: not at all, continuation, two + continuations, single-quote split, single-quoted character, + double-quote split, double-quoted character, escaped character, class, + wildcard, brace here, brace tail, brace whole, sequence, star} x {what + the breaking form contains: plain, an alternative with its own dot, + with two, a backup-looking name, a nested group, a metacharacter, a + leading dot} x {how deeply it nests: 0, 1, 2, 3, 5, 8, 9, 11, 14, 20} + x {where}. 2698 members. ``pytest -m slow`` runs all of them, + executing each in a throwaway ``HOME`` to confirm it really does read + the marker file and then asking the guard: all 2698 read it, all 2698 + deny. The default run checks an evenly-strided sample of 118, so + every commit defends the property even without the slow pass; + - the nesting cliff has its own table, at depths 1 to 20 with two, three + and five alternatives per level, run by default. Before the refusal + rule, 38 of those cells were allowed while bash read the file — the + cliff sat at depth 11, 8 and 6 respectively; + - the resource bounds have their own tests: expansion bombs up to + multi-megabyte commands must still answer, and the 64 KB boundary must + refuse on one side and not the other. + + Older figures that once appeared here — a random single-character fuzz — + are gone rather than restated, because nothing in the repository + reproduces them. A number in a docstring with no committed artifact is + a claim about the past, not a property of the code; if a measurement is + worth quoting it is worth committing the thing that produces it. Each round of bugs here has been a product of axes the generator only walked the margins of. It emitted continuations and it emitted @@ -309,6 +326,21 @@ call through, as did a path with an embedded NUL, which makes ``os.path.realpath`` raise. +Failing closed is about time as well as exceptions. ``sys.excepthook`` +catches what Python raises; it cannot catch the host killing a hook that +overruns, and that process exits non-zero *without* printing the deny +JSON — which is precisely the non-blocking case where the tool call +proceeds. A guard that is merely slow is a guard that is bypassed, and a +4 MB command of nested brace groups used to take it there: no answer in +45 seconds, 1.95 GB resident. Three bounds keep that shut, all of them +cheap: the command is refused unread above 64 KB, expansion is budgeted on +total normalized bytes rather than on how many candidates there are, and a +pass that has to revert stops the loop instead of letting the remaining +seven recompute and discard the same expansion. Multi-megabyte bombs now +answer in about a fifth of a second. Nothing legitimate comes near 64 KB; +if that ever stops being true, raise the bound deliberately rather than +letting the work grow to fit. + The payloads run under whatever ``python3`` the host finds on PATH, which need not be the interpreter mureo itself was installed with. The Bash payload needs **Python 3.8 or newer** for ``itertools.accumulate(..., @@ -455,7 +487,7 @@ def _deny_expr(reason: str) -> str: "''.join('' if (k==0 and x in q1+q2+bs) or (k==1 and x==q1)" " or (k==2 and x in q2+bs) or (k>2 and x==nl)" " else ('*/' if x in dl+tk+pc else (ho if k and not m and x in mt else x))" - " for x,(k,m) in zip(c,st))" + " for x,(k,m) in zip(cc,st))" ) # An expansion swallows the identifier run that names it: `$D` and `%s` are @@ -496,8 +528,19 @@ def _deny_expr(reason: str) -> str: "ga='[{][^{}' + nl + ']*[}]'; " "fe=lambda s: next((w for w in re.finditer(ga, s)" " if ',' in w.group() or '..' in w.group()), None); " - "al=lambda w: (lambda v: v if ',' in w.group() and len(v)<=64 else ['*','.*'])" - "(w.group()[1:-1].split(',')); " + # A sequence yields integers or single characters. Integers hold no dot, + # and a character range holds one only if it spans ASCII 46 — so only + # then can the group supply the leading dot of a dotfile, and only then + # is the `.*` reading needed. Without this, `echo {1..100}` folded to + # `.*` and denied, which is a common idiom and not an attempt at + # anything. + "sq=lambda v: (lambda e: ['*'] if len(e)==2 and" + " ((e[0].lstrip(chr(45)).isdigit() and e[1].lstrip(chr(45)).isdigit())" + " or (len(e[0])==1 and len(e[1])==1 and not" + " (min(ord(e[0]),ord(e[1]))<=46<=max(ord(e[0]),ord(e[1])))))" + " else ['*','.*'])(v.split('..')); " + "al=lambda w: (lambda v: v if ',' in w.group() and len(v)<=64" + " else sq(w.group()[1:-1]))(w.group()[1:-1].split(',')); " "ex=lambda s: (lambda w: [s[:w.start()] + a + s[w.end():] for a in al(w)]" " if w else [s])(fe(s)); " ) @@ -522,9 +565,11 @@ def _deny_expr(reason: str) -> str: # What is left is a command with more than eight brace groups, or one whose # expansion exceeds 400 strings, and neither is a thing anyone types. _EXPAND = ( - "ls=functools.reduce(lambda acc,_: (lambda n: n if len(n)<=400 else acc)" - "([y for x in acc for y in ex(x)]), range(8), [t]); " - "un=[x for x in ls if fe(x)]; " + "rs=functools.reduce(lambda q,_: q if q[1] else" + " (lambda n: (q[0],True) if sum(map(len,n))>200000" + " else (n, n==q[0]))" + "([y for x in q[0] for y in ex(x)]), range(8), ([t],False)); " + "ls=rs[0]; un=[x for x in ls if fe(x)]; " ) # Source of a python expression yielding the regex for one path component @@ -544,8 +589,14 @@ def _deny_expr(reason: str) -> str: "sys.stdout.flush(), os._exit(0)); " "d=json.loads(sys.stdin.read() or '{}'); " "c=str((d.get('tool_input') or {}).get('command') or '').lower(); " + # A guard that is merely slow is a guard that is bypassed: the host + # kills a hook that overruns and that process exits non-zero without + # printing the deny JSON, which is the non-blocking case. So an + # oversized command is refused before any of the work below, and every + # later step runs on the empty string instead. + "bg=len(c)>65536; cc='' if bg else c; " + _CHARS - + "st=list(itertools.accumulate(c, " + + "st=list(itertools.accumulate(cc, " + _QUOTE_STEP + ", initial=(0,0))); " # One reading of the command, built once. Brace expansion turns it into @@ -560,8 +611,9 @@ def _deny_expr(reason: str) -> str: + "p=[x for s in ls for x in re.findall(" "'(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', s)]; " "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " - # `un` first: structure the guard could not resolve denies on its own. - "b=un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + # `bg` and `un` first: what the guard could not read, and what it could + # not resolve, each deny on their own. + "b=bg or un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + _deny_expr(_BASH_REASON) + " if b else None" ) diff --git a/pyproject.toml b/pyproject.toml index 7092b364..ce7c229f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -161,6 +161,7 @@ markers = [ "unit: Unit tests", "integration: Integration tests", "real_save: Opt out of the credentials-write stub and save for real (tmp_path)", + "slow: Exhaustive runs kept out of the default suite (pytest -m slow)", ] [tool.mypy] diff --git a/tests/credential_guard_product.py b/tests/credential_guard_product.py new file mode 100644 index 00000000..7473b34e --- /dev/null +++ b/tests/credential_guard_product.py @@ -0,0 +1,158 @@ +"""The differential product the Bash credential guard's numbers come from. + +``mureo/credential_guard.py`` quotes counts — "all N members read the +credentials file, and all N deny". This module is where those come from, so +the claim is a property that can be re-run rather than a number someone +once measured. ``tests/test_credential_guard_product.py`` runs it. + +The shape is a cartesian product of the axes that have actually produced +bypasses, because every one of them was a *combination* rather than a +feature: + +* how the parent directory is supplied — literal, ``$HOME``, ``"$HOME"``, + ``$VAR``, ``"$VAR"``, ``${VAR}``, ``$VAR$EMPTY``, ``$1``, ``$(cmd)``, + backtick; +* how the name is broken up — continuation, quote splits, escapes, + classes, wildcards, brace forms, sequences; +* what the breaking form *contains* — an alternative carrying its own dot + is what defeated the fold that replaced a group with one placeholder; +* how deeply the form nests — the expansion budget's cliff was invisible + while every member sat at depth two or less; +* where in the name it happens. + +Each member is checked two ways: run in a real bash against a throwaway +HOME holding a marker credentials file, and put through the real generated +hook command. A member that reads the marker and is not denied is a +bypass. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess + +Q1, Q2, BS, TICK, NL = chr(39), chr(34), chr(92), chr(96), chr(10) +CONT = BS + NL +NAME = ".mureo" +MARKER = "MUREO-PRODUCT-MARKER-8F31A7" + +# (label, prelude, text placed in front of the name) +PARENTS: list[tuple[str, str, str]] = [ + ("literal", "", "~/"), + ("$HOME", "", "$HOME/"), + ('"$HOME"', "", Q2 + "$HOME" + Q2 + "/"), + ("$VAR", "D2=~/; ", "$D2"), + ('"$VAR"', "D2=~/; ", Q2 + "$D2" + Q2), + ("${VAR}", "D2=~/; ", "${D2}"), + ("$VAR$EMPTY", "D2=~/; E2=; ", "$D2$E2"), + ("$1", "set -- ~/; ", "$1"), + ("$(cmd)", "", "$(printf " + Q1 + "%s" + Q1 + " ~/)"), + ("backtick", "", TICK + "printf " + Q1 + "%s" + Q1 + " ~/" + TICK), +] + +# (label, the other alternative of a brace group, extra members of a class) +FILLERS: list[tuple[str, str, str]] = [ + ("plain", "z", "z"), + ("dotted", "x.y", "x"), + ("dotted twice", "a.b.c", "a"), + ("backup-looking", "bashrc.bak", "b"), + ("nested group", "{a,b}.c", "c"), + ("metachar", "x*", "x"), + ("leading dot", ".hidden", "h"), +] + +NESTINGS = [0, 1, 2, 3, 5, 8, 9, 11, 14, 20] + + +def breaks(alt: str, cls: str) -> list[tuple[str, object]]: + """Ways to spell ``NAME`` so the six characters are not consecutive.""" + return [ + ("none", lambda n, i: n), + ("continuation", lambda n, i: n[:i] + CONT + n[i:]), + ("two continuations", lambda n, i: n[:i] + CONT + n[i:] + CONT), + ("single-quote split", lambda n, i: n[:i] + Q1 + Q1 + n[i:]), + ("single-quoted char", lambda n, i: n[:i] + Q1 + n[i] + Q1 + n[i + 1 :]), + ("double-quote split", lambda n, i: n[:i] + Q2 + Q2 + n[i:]), + ("double-quoted char", lambda n, i: n[:i] + Q2 + n[i] + Q2 + n[i + 1 :]), + ("escaped char", lambda n, i: n[:i] + BS + n[i] + n[i + 1 :]), + ("class", lambda n, i: n[:i] + "[" + n[i] + cls + "]" + n[i + 1 :]), + ("wildcard", lambda n, i: n[:i] + "?" + n[i + 1 :]), + ("brace here", lambda n, i: n[:i] + "{" + n[i] + "," + alt + "}" + n[i + 1 :]), + ("brace tail", lambda n, i: n[:i] + "{" + n[i:] + "," + alt + "}"), + ("brace whole", lambda n, i: "{" + n + "," + alt + "}"), + ("sequence", lambda n, i: n[:i] + "{" + n[i] + ".." + n[i] + "}" + n[i + 1 :]), + ("star", lambda n, i: n[:i] + "*" + n[i + 1 :]), + ] + + +def nest(spelled: str, depth: int) -> str: + """Bury a spelling inside ``depth`` further brace levels.""" + for level in range(depth): + spelled = "{z" + str(level) + "," + spelled + "}" + return spelled + + +def members() -> list[tuple[str, str]]: + """Every member of the product, as ``(label, command)``.""" + out: list[tuple[str, str]] = [] + seen: set[str] = set() + for plabel, prelude, prefix in PARENTS: + for flabel, alt, cls in FILLERS: + for blabel, spell in breaks(alt, cls): + # A filling only varies the forms that have one. + if flabel != "plain" and not ( + blabel.startswith("brace") or blabel == "class" + ): + continue + positions = [0] if blabel in ("none", "brace whole") else range(1, 6) + for i in positions: + try: + spelled = spell(NAME, i) # type: ignore[operator] + except IndexError: + continue + depths = ( + NESTINGS + if (flabel == "plain" and plabel in ("literal", "$VAR")) + else [0] + ) + for depth in depths: + body = nest(spelled, depth) + cmd = prelude + "cat " + prefix + body + "/credentials.json" + if cmd in seen: + continue + seen.add(cmd) + out.append( + (f"{plabel} | {blabel} | {flabel} | @{i} | d{depth}", cmd) + ) + return out + + +def build_home(root: str) -> str: + """A throwaway HOME with a marker credentials file.""" + home = os.path.join(root, "home") + shutil.rmtree(home, ignore_errors=True) + os.makedirs(os.path.join(home, ".mureo")) + with open( + os.path.join(home, ".mureo", "credentials.json"), "w", encoding="utf-8" + ) as fh: + fh.write(json.dumps({"access_token": MARKER})) + os.makedirs(os.path.join(home, "project"), exist_ok=True) + return home + + +def reads_marker(command: str, home: str, bash: str) -> bool: + """Does a real shell actually print the credentials file for this?""" + try: + proc = subprocess.run( + [bash, "-c", command], + capture_output=True, + text=True, + env={"HOME": home, "PATH": os.environ.get("PATH", "")}, + cwd=home, + timeout=15, + ) + except subprocess.TimeoutExpired: + return False + return MARKER in proc.stdout diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 4f40a4ae..6569b439 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -479,6 +479,10 @@ class TestGuardThroughARealShell: "}}}}}}}}/ /tmp/dest/", # Three and five alternatives per level reach the budget sooner. "cat ~/.{a,b,{c,d,{e,f,{g,h,{i,j,{k,l,{m,n,mureo}}}}}}}/creds", + # A character range that spans ASCII 46 can produce the dot + # itself, so the group can supply the leading dot of a dotfile. + "cat ~/.{l..n}ureo/credentials.json", + "cat ~/{-..0}mureo/credentials.json", # A line continuation is deleted, backslash and newline both, # before the shell tokenises anything — so the name is spelled # across two lines and is contiguous by the time it is used. @@ -556,6 +560,15 @@ def test_denies_through_the_shell(self, fake_home: Path, command: str) -> None: "mkdir -p build/{lib,bin,share}", "mv report.{txt,md}", "mv file{1..10}.txt archive/", + # A bare sequence group, which is a common idiom and denied + # until the range was consulted: integers hold no dot, and a + # character range holds one only if it spans ASCII 46. + "echo {1..100}", + "for i in {1..5}; do touch file$i.txt; done", + "printf '%s\\n' {A..Z}", + "touch file{1..20}.log", + "mkdir -p test{1..3}/{a..c}", + "echo {a..z}{0..9}", "mkdir -p a/{1,2}/b/{3,4}/c/{5,6}/d/{7,8}", "kubectl get pods -o jsonpath='{.items[0].metadata.name}'", # Braces on separate lines of a multi-line command must not pair diff --git a/tests/test_credential_guard_product.py b/tests/test_credential_guard_product.py new file mode 100644 index 00000000..a4eb4c21 --- /dev/null +++ b/tests/test_credential_guard_product.py @@ -0,0 +1,200 @@ +"""Differential tests for the Bash guard: what the shell does vs what the +guard decides. + +The parametrised rows in ``test_credential_guard.py`` pin roughly ninety +spellings someone thought of. These check a whole product, and they check +it against a real bash rather than against a re-implementation of the rule +— which is the only way the earlier bypasses were ever found. + +Two speeds: + +* the default run takes an evenly-strided sample of the product and asks + the guard about each. It is deterministic, needs a couple of hundred + subprocesses, and is what CI defends on every commit; +* ``-m slow`` runs the whole product, and additionally executes every + member in a throwaway HOME to confirm it really does read the marker + file. This is where the counts quoted in + ``mureo/credential_guard.py`` come from:: + + pytest tests/test_credential_guard_product.py -m slow + +Keep the numbers in that docstring and the output of the slow run in step. +If you add an axis here, update them; if a claim there has no counterpart +here, delete the claim. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path + +import pytest + +from tests.credential_guard_product import ( + build_home, + members, + reads_marker, +) +from tests.hook_guard_runner import BASH, PYTHON3, deny_decision, run_guard_in_shell + +needs_shell = pytest.mark.skipif( + BASH is None or PYTHON3 is None, + reason="the differential product needs both bash and python3 on PATH", +) + +# The sample the default run checks. Strided rather than random so a +# failure names the same member on every machine. +_SAMPLE_STRIDE = 23 + + +def _bash_guard_command() -> str: + from mureo.credential_guard import bash_guard_entry + + return bash_guard_entry()["hooks"][0]["command"] + + +def _denies(command: str, home: Path) -> bool: + proc = run_guard_in_shell(_bash_guard_command(), {"command": command}, home) + assert proc.returncode == 0, proc.stderr + return deny_decision(proc) == "deny" + + +@needs_shell +@pytest.mark.unit +class TestProductSample: + def test_sample_of_the_product_is_denied(self, tmp_path: Path) -> None: + """Every strided member of the product must deny. + + Each is a spelling of ``~/.mureo/credentials.json`` assembled from + a parent form, a way of breaking the name, a filling for that form, + a nesting depth and a position. None of them contains the six + characters of the directory name consecutively unless the ``none`` + break was chosen. + """ + home = Path(build_home(str(tmp_path))) + sample = members()[::_SAMPLE_STRIDE] + assert len(sample) > 100, "the product shrank; check the axes" + missed = [label for label, cmd in sample if not _denies(cmd, home)] + assert not missed, f"{len(missed)} of {len(sample)} allowed: {missed[:5]}" + + def test_the_axes_are_all_represented(self) -> None: + """A guard against an axis quietly dropping out of the product.""" + labels = [label for label, _ in members()] + for axis in ("$VAR", "backtick", "continuation", "brace here", "sequence"): + assert any(axis in label for label in labels), axis + for depth in ("d0", "d3", "d11", "d20"): + assert any(label.endswith(depth) for label in labels), depth + + +@pytest.fixture +def only_when_asked_for(request: pytest.FixtureRequest) -> None: + """Run only when ``slow`` was selected, so a plain ``pytest`` skips it. + + Expressed here rather than as a global ``addopts`` filter: a marker that + silently disappears from the default run is how a suite ends up with + checks nobody has executed in months. + """ + if "slow" not in str(request.config.getoption("markexpr")): + pytest.skip("exhaustive; run with: pytest -m slow") + + +@needs_shell +@pytest.mark.slow +class TestWholeProduct: + @pytest.mark.usefixtures("only_when_asked_for") + def test_every_member_reads_the_file_and_is_denied(self, tmp_path: Path) -> None: + """The claim the module docstring makes, in full. + + Both halves matter. That every member is denied is the guarantee; + that every member really reads the marker is what stops the product + quietly filling up with commands that prove nothing. + """ + home = Path(build_home(str(tmp_path))) + all_members = members() + inert = [ + label + for label, cmd in all_members + if not reads_marker(cmd, home, BASH or "bash") + ] + missed = [label for label, cmd in all_members if not _denies(cmd, home)] + assert not inert, f"{len(inert)} members do not reach the file: {inert[:5]}" + assert not missed, f"{len(missed)} allowed: {missed[:5]}" + + +@needs_shell +@pytest.mark.unit +class TestNestingDepth: + """The expansion budget's cliff, which cost a round on its own. + + Past the budget the old code left literal braces in the candidates — + ordinary characters to fnmatch — so neither rule fired. Unresolved + structure now denies on its own, and this is the table that says so. + """ + + @staticmethod + def _nested(depth: int, alts: int) -> str: + inner = "mureo" + for level in range(1, depth + 1): + filler = ",".join(f"z{level}x{k}" for k in range(alts - 1)) + inner = "{" + filler + "," + inner + "}" + return "cat ~/." + inner + "/credentials.json" + + @pytest.mark.parametrize("alts", [2, 3, 5]) + @pytest.mark.parametrize("depth", [1, 2, 5, 8, 9, 10, 11, 12, 16, 20]) + def test_denies_at_every_depth(self, tmp_path: Path, depth: int, alts: int) -> None: + home = Path(build_home(str(tmp_path))) + command = self._nested(depth, alts) + assert reads_marker(command, home, BASH or "bash"), "case proves nothing" + assert _denies(command, home), command + + +@needs_shell +@pytest.mark.unit +class TestResourceBounds: + """A guard that is merely slow is a guard that is bypassed. + + ``sys.excepthook`` cannot catch the host killing an overrunning hook, + and that process exits non-zero without printing the deny JSON — the + non-blocking case, where the tool call proceeds. Oversized input is + therefore refused before any of the analysis runs. + """ + + @staticmethod + def _decide(command: str, seconds: float) -> str: + payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": command}}) + proc = subprocess.run( + [BASH or "bash", "-c", _bash_guard_command()], + input=payload, + capture_output=True, + text=True, + timeout=seconds, + ) + assert proc.returncode == 0, proc.stderr[:400] + return "deny" if proc.stdout.strip() else "allow" + + @pytest.mark.parametrize( + ("label", "alt_len", "alts", "groups"), + [ + ("modest", 100, 4, 2), + ("wide alternatives", 8000, 64, 4), + ("wide and repeated", 8000, 64, 8), + ("multi-megabyte", 16000, 64, 8), + ], + ) + def test_expansion_bombs_are_answered_quickly( + self, label: str, alt_len: int, alts: int, groups: int + ) -> None: + body = ",".join(["z" * alt_len] * (alts - 1) + ["mureo"]) + command = ( + "cat ~/." + "".join("{" + body + "}" for _ in range(groups)) + "/creds" + ) + # A generous ceiling: the point is that it answers at all, in time + # the host will not kill it. Locally these run in ~0.2s. + assert self._decide(command, seconds=15) == "deny", label + + def test_oversized_commands_are_refused_at_the_boundary(self) -> None: + """Nothing legitimate approaches 64 KB, and past it the guard has + not read the command, so it cannot say the command is safe.""" + assert self._decide("echo " + "z" * (65536 - 5), seconds=15) == "allow" + assert self._decide("echo " + "z" * (65537 - 5), seconds=15) == "deny" From 3d8e93319596a7da013115128267aed282c0d5fe Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:50:57 +0900 Subject: [PATCH 09/11] fix: give a refusal its own reason, and correct two claims Three small things, two of which are the same defect in a smaller place. 1. A refusal is not a match. A 70 KB command with nothing resembling the directory in it returned "commands that can reach ~/.mureo are blocked", so an agent reading the reason would hunt for a reference that is not there and retry. Oversize now has its own reason saying what actually happened: the command was refused unread and nothing was concluded about it. The reason for a real hit is unchanged. 2. The sequence comment was wrong, and so was a test row that rested on it. It claimed `{-..0}` denies because its character range spans ASCII 46 and can therefore produce the dot. Bash does not expand `{-..0}` at all -- verified: printf '[%s]' {-..0} prints [{-..0}] -- because a sequence endpoint must be an integer or a single letter. Following that through: a letter range lies within ASCII 65..122, so no recognised sequence can produce a dot, and the `.*` reading is not for dot-producing ranges but for syntax the guard does not recognise. The comment now says that, and the deny is described as what it is: a conservative refusal of syntax this does not parse. The test row is worse than the comment was. `cat ~/{-..0}mureo/...` sat in the shell-level deny list under a comment about spanning ASCII 46, and since bash leaves it literal the command reaches nothing -- an over-block pinned as though it were protection, which is the shape of mistake this whole review has been about. It has moved to its own test with `echo {-..0}` and `echo {a..z..2}`, named for what it checks and documented as an over-block. 3. The depth table was claimed as 60 cells and committed as 30, because the parametrisation sampled ten depths rather than walking all twenty. The claim is the one worth keeping, so the test now walks 1 to 20 with two, three and five alternatives per level: 60 rows, each asserting both that the command reads the marker file and that the guard denies it. The docstring figure that had no counterpart -- "38 of those cells were allowed" -- is gone, replaced by what can be reproduced by reverting. Two and three are the same failure as every round of this review: a sentence stronger than the measurement behind it. The docstring is defended by CI now; comments, commit messages and test-row rationales are not, and that is exactly where it came back. The only durable answer is to write the check first and the sentence after. Verified unchanged: 73 everyday commands 0 denied, 51 must-deny forms 0 through, the 10 mureo identifiers of #567 0 denied, 21 brace-using commands 1 refused, 20 %-heavy commands 1 refused, all reproduction families denied, 4 MB and 8 MB answered in 0.13 s and 0.19 s, the 65536/65537 boundary exact, fail-closed intact, known-open still five rows. Guard rows 240 -> 272; product 2698, sample 118. --- mureo/credential_guard.py | 72 +++++++++++++++++--------- tests/test_credential_guard.py | 33 ++++++++++-- tests/test_credential_guard_product.py | 2 +- 3 files changed, 79 insertions(+), 28 deletions(-) diff --git a/mureo/credential_guard.py b/mureo/credential_guard.py index 14ff27e8..a1640ba8 100644 --- a/mureo/credential_guard.py +++ b/mureo/credential_guard.py @@ -216,13 +216,16 @@ eight groups in one command, or an expansion whose normalized text exceeds 200 KB; - a command longer than 64 KB, which is refused unread (see below); - - a sequence whose character range spans ASCII 46, since one of the - things it produces is the dot itself: ``echo {-..0}`` denies. Ranges - that cannot produce a dot are read exactly, so ``echo {1..100}``, - ``for i in {1..5}``, ``printf '%s' {A..Z}`` and ``touch - file{1..20}.log`` are allowed — they were denied until the range was - consulted, which is the kind of over-block that teaches people to turn - a guard off. + - sequence syntax this does not recognise — a three-part ``{a..z..2}``, + an endpoint that is neither an integer nor a single letter — which is + refused rather than reasoned about. Bash expands a sequence only for + those two endpoint kinds, so ``{-..0}`` is not a sequence at all and + stays literal; the refusal costs nothing real. Sequences that *are* + recognised are read exactly, so ``echo {1..100}``, ``for i in + {1..5}``, ``printf '%s' {A..Z}`` and ``touch file{1..20}.log`` are + allowed — every one of them denied until the endpoints were consulted, + which is the kind of over-block that teaches people to turn a guard + off. Brace expansion itself used to be on this list — ``mv .{foo,bar}`` and ``rm .{a,b,c}`` denied although neither can name the directory. @@ -281,10 +284,12 @@ the marker file and then asking the guard: all 2698 read it, all 2698 deny. The default run checks an evenly-strided sample of 118, so every commit defends the property even without the slow pass; - - the nesting cliff has its own table, at depths 1 to 20 with two, three - and five alternatives per level, run by default. Before the refusal - rule, 38 of those cells were allowed while bash read the file — the - cliff sat at depth 11, 8 and 6 respectively; + - the nesting cliff has its own table: every depth from 1 to 20 with + two, three and five alternatives per level, 60 cells, run by default. + Each asserts that the command really reads the marker file *and* that + the guard denies it. Against the commit before the refusal rule the + deeper cells were allowed while bash read the file, the cliff falling + at depth 11 for two alternatives per level and earlier for more; - the resource bounds have their own tests: expansion bombs up to multi-megabyte commands must still answer, and the 64 KB boundary must refuse on one side and not the other. @@ -515,9 +520,8 @@ def _deny_expr(reason: str) -> str: # bash leaves alone — a group is expandable only with a comma or a `..`. # `al` gives its alternatives; a sequence (`{l..n}`) is not a list of # alternatives this can enumerate, and neither is a group with absurdly many -# of them, so those fall back to the two coarse readings — `*` and `.*` — -# which between them cover both "supplies a leading dot" and "does not". -# That pair is what the old single guess was missing. +# of them, so those are read coarsely — see `sq` below for which of the two +# coarse readings applies and why. # # `ga` excludes the newline from a group's contents, because an unquoted # newline is a token separator: bash will not expand a brace group across @@ -528,12 +532,20 @@ def _deny_expr(reason: str) -> str: "ga='[{][^{}' + nl + ']*[}]'; " "fe=lambda s: next((w for w in re.finditer(ga, s)" " if ',' in w.group() or '..' in w.group()), None); " - # A sequence yields integers or single characters. Integers hold no dot, - # and a character range holds one only if it spans ASCII 46 — so only - # then can the group supply the leading dot of a dotfile, and only then - # is the `.*` reading needed. Without this, `echo {1..100}` folded to - # `.*` and denied, which is a common idiom and not an attempt at - # anything. + # A sequence bash recognises has endpoints that are integers or single + # *letters*, and neither can be a dot: an integer never contains one, + # and a letter range lies within ASCII 65..122, well clear of 46. So a + # recognised sequence cannot supply the leading dot of a dotfile and + # `*` alone reads it. Without that, `echo {1..100}` folded to `.*` and + # denied — a common idiom, and not an attempt at anything. + # + # The `.*` reading is kept for everything this does not recognise as a + # sequence: a three-part `{a..z..2}`, a range with an endpoint that is + # neither, anything malformed. Those are refused conservatively rather + # than reasoned about — bash leaves most of them literal (`{-..0}` is + # not a sequence at all and stays as written), so the cost is an + # over-block on text nobody types and the benefit is not having to be + # right about a syntax this does not parse. "sq=lambda v: (lambda e: ['*'] if len(e)==2 and" " ((e[0].lstrip(chr(45)).isdigit() and e[1].lstrip(chr(45)).isdigit())" " or (len(e[0])==1 and len(e[1])==1 and not" @@ -581,6 +593,16 @@ def _deny_expr(reason: str) -> str: _BASH_REASON = "mureo credential guard: commands that can reach ~/.mureo are blocked" +# A refusal is not a match, and the agent reading the reason acts on the +# difference: told the command references ~/.mureo, it goes looking for a +# reference that is not there and retries. This one says what actually +# happened — the command was too long to read, so nothing was concluded +# about it. +_OVERSIZE_REASON = ( + "mureo credential guard: command over 65536 bytes was refused unread, " + "not analysed; shorten it or run it in pieces" +) + _BASH_GUARD_CODE = ( "import sys,json,re,os,fnmatch,functools,itertools; " # Fail closed: an escaping exception exits 1, which both hosts treat as a @@ -611,11 +633,13 @@ def _deny_expr(reason: str) -> str: + "p=[x for s in ls for x in re.findall(" "'(?:^|[^a-z0-9_])(' + " + _PATTERN_COMPONENT + " + ')', s)]; " "g=[x for x in p if set('*?[') & set(x) and fnmatch.fnmatchcase('.mureo', x)]; " - # `bg` and `un` first: what the guard could not read, and what it could - # not resolve, each deny on their own. - "b=bg or un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + # `un` first: structure the guard could not resolve denies on its own. + # `bg` is answered separately below, because it needs its own reason. + "b=un or [s for s in ls if re.search('(^|[^a-z0-9_])[.]mureo', s)] or g; " + + _deny_expr(_OVERSIZE_REASON) + + " if bg else (" + _deny_expr(_BASH_REASON) - + " if b else None" + + " if b else None)" ) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 6569b439..3cac6eba 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -479,10 +479,10 @@ class TestGuardThroughARealShell: "}}}}}}}}/ /tmp/dest/", # Three and five alternatives per level reach the budget sooner. "cat ~/.{a,b,{c,d,{e,f,{g,h,{i,j,{k,l,{m,n,mureo}}}}}}}/creds", - # A character range that spans ASCII 46 can produce the dot - # itself, so the group can supply the leading dot of a dotfile. + # A letter range covering `m`. This one really does expand onto + # the directory; `{-..0}` below does not, and is a row in the + # conservative-refusal test instead. "cat ~/.{l..n}ureo/credentials.json", - "cat ~/{-..0}mureo/credentials.json", # A line continuation is deleted, backslash and newline both, # before the shell tokenises anything — so the name is spelled # across two lines and is contiguous by the time it is used. @@ -631,6 +631,33 @@ def test_path_guard_unusable_path_denies(self, fake_home: Path) -> None: assert deny_decision(proc) == "deny" assert proc.returncode == 0 + @pytest.mark.parametrize( + "command", + [ + "cat ~/{-..0}mureo/credentials.json", + "echo {-..0}", + "echo {a..z..2}", + ], + ) + def test_denies_sequence_syntax_it_does_not_recognise( + self, fake_home: Path, command: str + ) -> None: + """Unrecognised sequence syntax is refused rather than reasoned about. + + These are over-blocks, recorded as such. Bash expands a sequence + only when both endpoints are integers or single letters, so + ``{-..0}`` is not a sequence at all and stays literal — none of + these reaches the directory, and none of them is something a person + types. The guard does not parse the syntax and does not try to: it + reads what it cannot resolve conservatively, which is the same rule + that closed the nesting cliff. + """ + proc = run_guard_in_shell( + _bash_guard_command(), {"command": command}, fake_home + ) + assert proc.returncode == 0, proc.stderr + assert deny_decision(proc) == "deny", command + @pytest.mark.parametrize( "command", [ diff --git a/tests/test_credential_guard_product.py b/tests/test_credential_guard_product.py index a4eb4c21..c073bb76 100644 --- a/tests/test_credential_guard_product.py +++ b/tests/test_credential_guard_product.py @@ -141,7 +141,7 @@ def _nested(depth: int, alts: int) -> str: return "cat ~/." + inner + "/credentials.json" @pytest.mark.parametrize("alts", [2, 3, 5]) - @pytest.mark.parametrize("depth", [1, 2, 5, 8, 9, 10, 11, 12, 16, 20]) + @pytest.mark.parametrize("depth", list(range(1, 21))) def test_denies_at_every_depth(self, tmp_path: Path, depth: int, alts: int) -> None: home = Path(build_home(str(tmp_path))) command = self._nested(depth, alts) From 860abbae56139fd856ac3adb48bff9b5cd37531c Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 12:24:45 +0900 Subject: [PATCH 10/11] fix: coerce the test environment at the boundary, and split the POSIX claim test-windows failed 61 times with "TypeError: environment can only contain strings". `reads_marker` is annotated `home: str` and every caller passes a `Path`; subprocess on POSIX accepts one in `env` and Windows does not. The annotation disagreeing with the argument was not cosmetic -- it was a latent failure waiting for the first runtime that enforces the contract, and the review saw it, reasoned from one platform, and downgraded it. Coerced inside `reads_marker` rather than at the call sites, so the signature and the reality agree without every caller having to remember. `build_home` returns a Path now, which removes the Path/str juggling that produced the mismatch in the first place. Same function built a minimal `{"HOME", "PATH"}` environment. On a platform where a shell needs more than that to start, the failure mode is "the command did not read the file" -- indistinguishable from the guard working, which is the worst way for a check to break. It extends os.environ now, like hook_guard_runner already did. Audited the rest of the branch for the shape rather than the instance: hook_guard_runner passes str() and inherits os.environ in both helpers, and TestResourceBounds builds no environment at all. reads_marker was the only offender on either count. Two contract tests pin it, and they run on every platform precisely because the lenient one is where the mistake gets made: the environment handed to each subprocess must be all strings, and must extend os.environ rather than replace it. Separately, the depth table bundled two different claims into one test: that a real shell reaches the file, and that the guard denies. The first is a statement about POSIX path and glob semantics; under an emulated shell a "no" would mean "not reachable through this translation layer", which is not the property under test and reads exactly like the guard working. They are two tests now -- the guarantee runs everywhere, the premise is POSIX-only with a skip reason that names the requirement. No assertion was weakened to pass on both. Coverage lost on Windows: the 60 "really reaches the file" premises and the slow whole-product run, both of which are shell-semantics claims. Everything the guard decides is still checked there -- 60 depth denials, the 118-member product sample, the resource bounds, and all 272 rows in test_credential_guard.py. mypy --strict over the four guard test files is clean now (it is not in CI's scope, which stays mureo/ by an earlier documented decision -- I ran it by hand because this class of bug is exactly what it catches). Local: 8041 passed, the same 12 pre-existing failures; slow product 2698/2698. --- tests/credential_guard_product.py | 46 ++++++++--- tests/test_credential_guard.py | 4 +- tests/test_credential_guard_product.py | 106 +++++++++++++++++++++++-- 3 files changed, 136 insertions(+), 20 deletions(-) diff --git a/tests/credential_guard_product.py b/tests/credential_guard_product.py index 7473b34e..bab07ab0 100644 --- a/tests/credential_guard_product.py +++ b/tests/credential_guard_product.py @@ -32,6 +32,14 @@ import os import shutil import subprocess +from pathlib import Path + +# These tests execute the attack command in a real shell and check whether it +# reaches the file. That is a statement about POSIX path, glob and quoting +# semantics; under an emulated shell on Windows a "no" would mean "not +# reachable through this translation layer", which is not the property under +# test and is indistinguishable from the guard working. +POSIX_SHELL = os.name == "posix" Q1, Q2, BS, TICK, NL = chr(39), chr(34), chr(92), chr(96), chr(10) CONT = BS + NL @@ -129,28 +137,42 @@ def members() -> list[tuple[str, str]]: return out -def build_home(root: str) -> str: +def build_home(root: str | os.PathLike[str]) -> Path: """A throwaway HOME with a marker credentials file.""" - home = os.path.join(root, "home") + home = Path(root) / "home" shutil.rmtree(home, ignore_errors=True) - os.makedirs(os.path.join(home, ".mureo")) - with open( - os.path.join(home, ".mureo", "credentials.json"), "w", encoding="utf-8" - ) as fh: - fh.write(json.dumps({"access_token": MARKER})) - os.makedirs(os.path.join(home, "project"), exist_ok=True) + (home / ".mureo").mkdir(parents=True) + (home / ".mureo" / "credentials.json").write_text( + json.dumps({"access_token": MARKER}), encoding="utf-8" + ) + (home / "project").mkdir(exist_ok=True) return home -def reads_marker(command: str, home: str, bash: str) -> bool: - """Does a real shell actually print the credentials file for this?""" +def reads_marker(command: str, home: str | os.PathLike[str], bash: str) -> bool: + """Does a real shell actually print the credentials file for this? + + ``home`` is coerced here rather than at the call sites. It arrives as a + ``Path`` from every caller, and ``subprocess`` on POSIX accepts one in + ``env`` while Windows raises ``TypeError: environment can only contain + strings`` — so an annotation that disagreed with the argument was not + cosmetic, it was a failure waiting for the first platform that enforces + the contract. Coercing at the boundary means the signature and the + reality agree without every caller having to remember. + + The environment is ``os.environ`` with ``HOME`` overridden, not a + hand-built pair: a minimal env drops variables a shell needs to start + at all on some platforms, which fails as "the command did not read the + file" — indistinguishable from the guard working. + """ + env = dict(os.environ, HOME=str(home), USERPROFILE=str(home)) try: proc = subprocess.run( [bash, "-c", command], capture_output=True, text=True, - env={"HOME": home, "PATH": os.environ.get("PATH", "")}, - cwd=home, + env=env, + cwd=str(home), timeout=15, ) except subprocess.TimeoutExpired: diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 3cac6eba..7640e92b 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -57,13 +57,13 @@ def fake_home(tmp_path: Path) -> Path: def _path_guard_command() -> str: from mureo.credential_guard import path_guard_entry - return path_guard_entry()["hooks"][0]["command"] + return str(path_guard_entry()["hooks"][0]["command"]) def _bash_guard_command() -> str: from mureo.credential_guard import bash_guard_entry - return bash_guard_entry()["hooks"][0]["command"] + return str(bash_guard_entry()["hooks"][0]["command"]) # --------------------------------------------------------------------------- diff --git a/tests/test_credential_guard_product.py b/tests/test_credential_guard_product.py index c073bb76..c2758c77 100644 --- a/tests/test_credential_guard_product.py +++ b/tests/test_credential_guard_product.py @@ -32,6 +32,7 @@ import pytest from tests.credential_guard_product import ( + POSIX_SHELL, build_home, members, reads_marker, @@ -43,6 +44,20 @@ reason="the differential product needs both bash and python3 on PATH", ) +# Asking the guard about a command is platform-independent — it is string +# analysis in Python. *Executing* the command and seeing whether it reaches +# the file is not, so the two claims are separate tests rather than one test +# with a conditional assertion. Everything the guard decides is checked +# everywhere; only the premise about what a shell does is POSIX-only. +needs_posix_shell = pytest.mark.skipif( + not POSIX_SHELL, + reason=( + "executes the attack command and asserts a real shell reaches the " + "file: a claim about POSIX path and glob semantics, not about the " + "guard, which is checked on every platform by the deny tests" + ), +) + # The sample the default run checks. Strided rather than random so a # failure names the same member on every machine. _SAMPLE_STRIDE = 23 @@ -51,7 +66,7 @@ def _bash_guard_command() -> str: from mureo.credential_guard import bash_guard_entry - return bash_guard_entry()["hooks"][0]["command"] + return str(bash_guard_entry()["hooks"][0]["command"]) def _denies(command: str, home: Path) -> bool: @@ -72,7 +87,7 @@ def test_sample_of_the_product_is_denied(self, tmp_path: Path) -> None: characters of the directory name consecutively unless the ``none`` break was chosen. """ - home = Path(build_home(str(tmp_path))) + home = build_home(tmp_path) sample = members()[::_SAMPLE_STRIDE] assert len(sample) > 100, "the product shrank; check the axes" missed = [label for label, cmd in sample if not _denies(cmd, home)] @@ -87,6 +102,70 @@ def test_the_axes_are_all_represented(self) -> None: assert any(label.endswith(depth) for label in labels), depth +@pytest.mark.unit +class TestSubprocessContracts: + """The environment handed to a subprocess must be strings, everywhere. + + POSIX accepts a ``Path`` in ``env`` and Windows raises ``TypeError: + environment can only contain strings``. That difference turned an + annotation nobody enforced into 61 failures on the first Windows run, + so the contract is pinned here rather than left to the platform to + discover. These run on every platform precisely because the lenient + one is where the mistake gets made. + """ + + def test_reads_marker_passes_only_strings_to_the_environment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from tests import credential_guard_product as product + + seen: dict[str, object] = {} + + def fake_run( + *args: object, **kwargs: object + ) -> subprocess.CompletedProcess[str]: + seen.update(kwargs) + return subprocess.CompletedProcess( + args=[], returncode=0, stdout="", stderr="" + ) + + # Patch the shared module object both helpers import. + monkeypatch.setattr(subprocess, "run", fake_run) + product.reads_marker("echo hi", build_home(tmp_path), "bash") + + env = seen["env"] + assert isinstance(env, dict) + bad = {k: v for k, v in env.items() if not isinstance(v, str)} + assert not bad, f"non-string environment values: {bad}" + assert isinstance(seen["cwd"], str) + # Inherited rather than hand-built: a minimal env drops variables a + # shell needs to start, which looks like "the command read nothing". + assert len(env) > 2, "the environment should extend os.environ" + + def test_the_runner_passes_only_strings_to_the_environment( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from tests import hook_guard_runner as runner + + seen: dict[str, object] = {} + + def fake_run( + *args: object, **kwargs: object + ) -> subprocess.CompletedProcess[str]: + seen.update(kwargs) + return subprocess.CompletedProcess( + args=[], returncode=0, stdout="", stderr="" + ) + + monkeypatch.setattr(subprocess, "run", fake_run) + runner.run_guard(_bash_guard_command(), {"command": "echo hi"}, tmp_path) + + env = seen["env"] + assert isinstance(env, dict) + bad = {k: v for k, v in env.items() if not isinstance(v, str)} + assert not bad, f"non-string environment values: {bad}" + + @pytest.fixture def only_when_asked_for(request: pytest.FixtureRequest) -> None: """Run only when ``slow`` was selected, so a plain ``pytest`` skips it. @@ -100,6 +179,7 @@ def only_when_asked_for(request: pytest.FixtureRequest) -> None: @needs_shell +@needs_posix_shell @pytest.mark.slow class TestWholeProduct: @pytest.mark.usefixtures("only_when_asked_for") @@ -110,7 +190,7 @@ def test_every_member_reads_the_file_and_is_denied(self, tmp_path: Path) -> None that every member really reads the marker is what stops the product quietly filling up with commands that prove nothing. """ - home = Path(build_home(str(tmp_path))) + home = build_home(tmp_path) all_members = members() inert = [ label @@ -143,10 +223,24 @@ def _nested(depth: int, alts: int) -> str: @pytest.mark.parametrize("alts", [2, 3, 5]) @pytest.mark.parametrize("depth", list(range(1, 21))) def test_denies_at_every_depth(self, tmp_path: Path, depth: int, alts: int) -> None: - home = Path(build_home(str(tmp_path))) + """The guarantee: every cell denies. Runs everywhere.""" + home = build_home(tmp_path) + assert _denies(self._nested(depth, alts), home) + + @needs_posix_shell + @pytest.mark.parametrize("alts", [2, 3, 5]) + @pytest.mark.parametrize("depth", list(range(1, 21))) + def test_every_depth_really_reaches_the_file( + self, tmp_path: Path, depth: int, alts: int + ) -> None: + """The premise: every cell is a command that reads the file. + + Without this the table above could fill up with commands that deny + because they are nonsense rather than because the guard works. + """ + home = build_home(tmp_path) command = self._nested(depth, alts) - assert reads_marker(command, home, BASH or "bash"), "case proves nothing" - assert _denies(command, home), command + assert reads_marker(command, home, BASH or "bash"), command @needs_shell From cc7f4d156a37a66619a234a802396a1c8df82228 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 12:39:31 +0900 Subject: [PATCH 11/11] test: skip the NUL-path fail-closed row on Windows, with the reason test-windows was down to one failure after the environment fix, and it is a different defect: os.path.realpath raises on an embedded NUL on POSIX and does not on Windows, where the path simply fails to resolve under the protected directory. Nothing fails open there -- there is no exception to escape -- so the guard allowing it is correct, and the assertion was pinning a platform's behaviour rather than the guard's. This was one of the original 61 failures and was never an environment problem; it hid inside that count. Coverage lost on Windows: this single trigger. The property it demonstrates -- an exception denies rather than exiting 1, which is the non-blocking case both hosts treat as "proceed" -- is still asserted there by the malformed-stdin tests for both guards, which pass on every platform. --- tests/test_credential_guard.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/test_credential_guard.py b/tests/test_credential_guard.py index 7640e92b..fae68d3b 100644 --- a/tests/test_credential_guard.py +++ b/tests/test_credential_guard.py @@ -620,6 +620,17 @@ def test_path_guard_malformed_input_denies(self, fake_home: Path) -> None: assert deny_decision(proc) == "deny" assert proc.returncode == 0 + @pytest.mark.skipif( + sys.platform == "win32", + reason=( + "the trigger is POSIX-specific: os.path.realpath raises on an " + "embedded NUL there, while on Windows it returns and the path " + "simply does not resolve under ~/.mureo, so there is no " + "exception to escape. The property this demonstrates — an " + "exception denies rather than exits 1 — is asserted on every " + "platform by the malformed-stdin tests above, for both guards" + ), + ) def test_path_guard_unusable_path_denies(self, fake_home: Path) -> None: """``realpath`` raises on an embedded NUL — that must not fail open.""" proc = run_guard_in_shell(