fix: deny shell patterns that expand onto the protected directory - #568
Conversation
Review response — 16738e5All five reproduced against the shipped commit first (real generated command, real bash, marker credentials file). All five leaked, exactly as reported. Four now deny; one does not and cannot.
The quoting layerAgreed on the diagnosis: hand-rolled quote stripping was the defect. Quoting is now a left fold with a five-state automaton (unquoted, single, double, and the two escaped states), so 1–3 close by construction. I did not use
If you still want Two more classes the fuzz foundRandom differential fuzzing against a real bash (spell
Fail-closed — you were right, and it was already brokenExit 1 is a non-blocking hook error, so an escaping exception is a bypass. Both payloads had one before this PR: malformed stdin exited 1 and let the call through, and so did a path with an embedded NUL ( Tests through a real shell
Over-block disclosure, widened and narrowedBrace groups are no longer blanketed with
What is not closableOf 2000 fuzzed commands, 1621 read the file: 873 with the name written out in the text — 0 got through; 748 assembled at runtime — 265 did. Every one of those 265 produces the leading dot from a substitution. With On your questionIt is a deterrent, not a security boundary, and it cannot be made into one — the agent runs as the owner of the file and can read it through any construction the text does not reveal. Your instinct about the permissions line was right too: it was wrong in the same direction, since permissions do not stop a process running as the owner either. The module now says this in a Gates
|
Line continuation — fixed in 9da9829Reproduced first against the previous commit, real generated command through a real bash, marker credentials file. Five in the family leaked, not one.
Your diagnosis was exactly right, including the fix location. The fold already had the states; the change is one clause — in the two escaped states a newline is dropped along with its backslash. Single quotes are untouched, because there a backslash is an ordinary character and The generator, not the caseYou were right that the fuzz never produced 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 extended run also confirmed My own standard, appliedYou are right that the gap between measurement and claim was the real defect: a whole lexer-level rewrite went untested while the numbers read as complete. Since the family is now closed it is not a known-open row, but the docstring carries the lesson where the fuzz numbers are quoted:
Eight rows added to Smaller items
Re-verified after the changeNo movement anywhere else: everyday 73 → 0 denied; must-deny 51 → 0 through; #567 identifiers 10 → 0 denied; the four earlier review cases still deny; Gates: |
The split brain is gone — 6728616Your diagnosis was exact, including that One normalized reading, both rules consume it. The separate Reproduced first, then re-checked (real command, real bash, marker file):
Two design bugs the corpus caught during the passBoth would have been silent regressions; both were found by running the existing must-deny corpus through the new reading, not by inspection:
The generator now walks the productYou were right that it only walked the margins. It is now exhaustive over the product, not sampled: {parent: literal, 560 members. All 560 read the credentials file in real bash. All 560 deny. (The prototype and the shipped payload were run separately over the same product; identical.) The random fuzz is unchanged: 2500 commands, 1995 read the file, 0 undetected among the 825 whose name is written out, 172 of the 1170 runtime-assembled — all still producing the leading dot from a substitution. Regression checksEveryday 73 → 0 denied. Must-deny 51 → 0 through. #567 identifiers 10 → 0 denied. Continuation family (last round) → still all deny. Round-2 review cases → still deny. Fail-closed unchanged. Known-open rows unchanged and still exactly the enumerated five. New over-block, disclosed
On the
|
Brace groups are expanded now, not approximated — 338e4e2Reproduced first (real generated command, real bash, marker file), then fixed. Expansion, not per-position dot tracking — it was tractable in the payload.
The normalized command becomes the list of strings the shell would produce, and every rule runs against all of them. No rule depends on a guess about any single reading. Two more the report didn't listFound while writing the expansion — and the naive Sequence expressions. The letter Over-blocks released, and one corrected
The generator dimensionYou were right, and it landed one axis deeper than my own sentence anticipated. It could vary which breaking form and where, but every brace alternative was inert filler ( It is now a product over {parent supplied by} × {name broken by, incl. brace-here / brace-tail / brace-whole / sequence} × {what the breaking form contains: plain, alternative with its own dot, with two, backup-looking name, nested group, metacharacter, leading dot} × {where}. 1510 members. All 1510 read the credentials file in real bash. All 1510 deny. I also corrected the docstring claim you flagged as false — the round-4 text said "of the 825 whose name is written out, 0 got through" while this family was written out and got through. The numbers now come from the widened product. What coarse approximations remainStated in the docstring, as the short honest list you asked for:
1–3 are the "text the command does not contain" class and are not closable by reading command text. 4 and 5 are closable in principle — enumerate the sequence, lift the cap — and the docstring says the shape of the fix for each is the same one applied here: compute what the shell would produce instead of approximating it. Regression checksEveryday 73 → 0 denied. Must-deny 51 → 0 through. #567 identifiers 10 → 0 denied. Gates: |
Unresolved structure now denies — ec75d9aTaken as the rule, not as 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; the coarse fallback is deleted. The 8-pass / 400-string budget stays — it stops the fork bomb — but exceeding it is a refusal instead of a shrug. Depths 1–20 × {2, 3, 5} alternatives per levelEvery cell reads the credentials file in real bash.
38 of 60 cells leaked before; 0 leak now. Cliffs at depth 11 / 8 / 6, exactly as you reported. The false sentenceCorrected in place, and the reason kept next to it rather than tidied away — the docstring said the fallback "over-approximates rather than dropping candidates", and past the budget it dropped them. The text now reads: "A budget that shrugs is a bypass with a length requirement." What the refusal costsNine brace groups on one line. That is it. Of 21 brace-using everyday commands —
|
Round seven — b32fe2cAll three addressed. The third one is the one I sat with, and you were right that it is the recurring defect rather than a side item. 1. The computational escape
Worst case under the 64 KB cap, where the byte budget has to do the work alone: 8 groups × 64 alternatives at 64,077 bytes → deny in 0.13s; a 1000-alternative single group → 0.12s; 2000-deep nesting → 0.09s. Boundary is exact: 65,536 bytes processed, 65,537 refused. A bug I found while fixing this: the early break compared list length to detect "nothing changed", but a sequence group expands to a single alternative, so 2. Bare sequence over-blockA sequence yields integers or single characters. Integers hold no dot; a character range holds one only if it spans ASCII 46. Consulting the range fixes all four of your examples without giving up coverage:
All six allow cases are now pinned rows, and 3. The numbers now have a committed artifactThis is the honest fix, and I took your framing: the numbers growing was not evidence of rigour while none of them were re-runnable.
The slow test is opt-in via a fixture checking the marker expression, not a global I deleted the random-fuzz figure ("2500 commands, 1995 read the file…") rather than restating it, because nothing in the repo reproduces it. The docstring now says: "A number in a docstring with no committed artifact is a claim about the past, not a property of the code." Guard test count: 92 → 240 rows, plus the 2698-member slow run. RegressionsEveryday 73 → 0 denied. Must-deny 51 → 0 through. #567 identifiers 10 → 0 denied. Brace-using 21 → 1 refused (nine groups on one line, unchanged). Gates: On where this standsUnderstood on the frame, and I agree with it. The remaining disclosed gaps are unchanged and enumerated: dotglob, runtime-produced text, encoded notations, extglob, globbed siblings, plus the bounded refusals (>8 brace groups, >200 KB expansion, >64 KB command) and the over-blocks ( |
All three fixed — 0fae0ac1. A refusal now says it is a refusal
One branch on Unresolved structure still returns the match reason. I left that deliberately: unlike an unread command, the guard did read it and found brace structure it could not resolve, so "can reach" is a fair description of what it is refusing on. Say the word if you would rather it were a third string. 2. The
|
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.
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.
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.
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 `.\<NL>mureo`, `.m\<NL>u\<NL>r\<NL>e\<NL>o`,
`.mure\<NL>?`, `.m\<NL>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 '~/.mu<NL>reo/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 `\<newline>`: 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.
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 `<something>.<something>` 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.
`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.
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.
…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.
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.
0fae0ac to
3d8e933
Compare
… 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.
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.
1 participant
The hole
The Bash guard looks for
.mureoas six consecutive literal characters. A metacharacter placed inside the directory name breaks that match, while the shell still expands the pattern onto the real directory. Verified against a throwawayHOMEwith bash 5.2 — each of these printed the credentials file, and each was allowed by the guard onmain:This is not a regression from #567. The bare substring rule that preceded it missed the same forms. #393 noted "wildcards pass through" and closed the wildcard that comes after the name (
cat ~/.mureo/cred*) — the inside of the name was never covered, and the docstring's "also catches wildcard forms" made the partial fix read as a complete one. That sentence is the reason this survived five months, so it is gone.The path guard is unaffected: it compares
os.path.realpath/os.path.abspathof a concrete path field, and no shell expands anything on that route.file_path: "~/.mure?/credentials.json"is allowed by the guard and opens nothing, because no such literal path exists.Why not a regex
The string that reaches the filesystem does not exist yet at guard time, so no pattern over the command text can decide the question. The guard now asks it from the other side: cut the command into path components, keep the ones that begin at a component boundary with a literal
.and contain a metacharacter, and deny whenfnmatchsays the pattern matches.mureo.Three things keep that from over-blocking:
dotglobis set, sols *,rm -rf build/*andtests/*.pycannot reach.mureoand are never candidates. Without this the rule would have to deny every glob anyone types,fnmatch('.mureo', '*')being true.sed 's/.*//'andfind . -name '.*'are a regex and a literal, not globs — andcat "$HOME/.mure?/x"genuinely opens nothing. Unquoted, the same characters do glob and are denied.$D.mure?andprintf '%s.mure?/'are denied by the same$/%test the literal rule uses, where quoting is not consulted because the pattern reaches the shell through a later expansion.Brace groups are replaced by
*before matching. That is an over-approximation:.mure{o,x}denies (correct) and so doesmv .{env,env.bak}(which cannot name the directory). Over-blocking is the safe direction, the same argument the guard already makes for case folding.Measurements
All figures come from running the generated hook command (
bash_guard_command()) throughbash -cwith the tool JSON on stdin — the payload as shipped, not a re-implementation of the rule.git,gh,ls,rm,cp,grep,sed,find,pytest,ruff,black,mypy,python -c,node --test, glob-heavy forms)window.MUREO_*,pkgs.mureo.jp,docs.mureo.jp)Not one of #567's defenses was dropped: rule 1 is untouched, the new rule can only add denials, and every existing test still passes unchanged.
Commands that do get denied and did not before, all of which genuinely expand onto the directory when run from
$HOME:ls .*,ls -d .??*,rm -rf .[!.]*,cp .* /tmp/. The two that are honest over-blocks are the brace-rename idiomsmv .{env,env.bak}andcp .{zshrc,zshrc.bak}.What is still not covered
Written into the docstring, because a guard that overstates its reach is how this bug survived:
cat ~/$P/x; spelled out in one command,P=.mure?; cat ~/$P/xis denied);~/.mur*_backup) — rule 2 asks only whether the pattern matches.mureoitself, while rule 1 does deny literal siblings;.mure@(o|x)), off by default in bash;~/'.'mure?/x) or hidden behind a deliberately unbalanced quote escapes the unquoted-text half;dotglob/GLOBIGNORE, where*does reach dotfiles.The guard stays defense-in-depth; filesystem permissions on
~/.mureoremain the control that protects the files.Payload note
The character class the rule needs contains
!, which cannot appear literally in the payload — a shell with history expansion on rewrites!sequences inside double quotes. It arrives viachr(33), next to the existingchr(36)for$, andtests/hook_guard_runner.pynow rejects a literal!alongside$, backticks, backslashes and newlines.Tests
tests/test_credential_guard.pygains 20 deny cases (each verified to reach the file under a real bash) and 21 allow cases covering everyday globbing, quoted regexes and quoted paths. Written first, all 20 red before the change.Gates:
ruff check .,black --check .,mypy mureo --ignore-missing-importsclean;python -m pytest= 7810 passed, with only the 12 failures that already fail onmainlocally (9 ×test_live_clients.py, 3 × tool-registry).