fix(cron): run command crons on hosts whose /bin/sh is bash - #7414
fix(cron): run command crons on hosts whose /bin/sh is bash#7414SebastianYuSun wants to merge 1 commit into
Conversation
github-actions
Bot
added
readiness: checking
16f3684 to
d94950d
Compare
github-actions
Bot
added
readiness: action required
|
On this head, The same boundary was crossed an hour earlier on an unrelated PR, on a different This diff cannot account for it. It adds 7 tests across The branch is up to date with base ( Could a maintainer re-run the cancelled |
Audit note — #7608 is being closed in favour of this PRYou are the surviving implementation; #7608 is being closed. What the two sharedBoth PRs add the same predicate to the same function at the same insertion point: a module-level bash-brace regex declared right after What #7608 had that this PR does notPlease pick these up (or say they are not wanted) so they do not disappear with that branch: src/kiro_crew/mcp_cron.py -- #7608's character class. Before closing #7608, widen #7414's This PR still needs work:
|
d94950d to
e4506d4
Compare
github-actions
Bot
added
readiness: checking
|
Rebased onto The Coverage Gate red is gone at the source. It was never this diff -- A nested-brace bypass in this PR's own regex is now closed. The inner character classes were The first word of each expansion is the credential directory, so This matters specifically because of the other half of this PR: the Local gates on the pushed head: black gate (repo wrapper, 4 files in scope), isort, flake8, mypy over |
|
The failure is a wall-clock ratio assertion, not a behaviour failure: Ratio 3.08 against a bound of 3.0, i.e. it missed by 2.7 percent of the threshold on a shared Windows runner. Shards 1, 3 and 4 all passed. It exercises The linearity property itself came from #7122, later touched by #7013 and #7356, none of them mine. I have not claimed a measured flake rate, because a Windows runner's wall clock is not reproducible here -- this is "not reproduced, platform mismatch" with the ownership paths above excluded, rather than a flake verdict. A re-run of that one shard is the cheapest way to settle it. |
|
Blocker closed, and thank you for finding it — the nested-comma class was a real hole in this PR's regex, and your framing of why it matters here is exactly right: this PR is what makes a brace-expanding bash the cron executor, so a class that is inert on Your prescribed change, applied verbatim. The pattern is now the widened form, character for character: Both inner classes dropped Your verified table, re-measured against the real One correction to the note, in your favour rather than against it. Tests. Both nested payloads are in Nothing else from #7608 picked up, agreed on your reasoning: the weaker error string, and black churn on lines baselined at Also confirmed on your |
|
Correcting my earlier triage of this red with measurements, because the honest verdict is weaker than what I wrote before. What I claimed last time: that a Windows runner's wall clock is not reproducible here. That was an assumption, not a measurement -- the test is pure Python timing and runs on Linux fine. So I measured it. Measured, 20 runs under CPU contention on Linux: Ownership, which is what I can settle. The failing assertion is
The part that may actually be worth your attention. Line 4496 is the one that failed, at ratio 3.08 against a bound of 3.0 -- 2.7% over, while shards 1, 3 and 4 passed. The test's docstring says the 3x bound is "generous ... so scheduler noise on a shared runner cannot red it"; on this runner it did. Widening the bound does not fix the shape, because it has to stay below the quadratic 4x signal while clearing worst-case noise, and those are not cleanly separated on a shared Windows runner. The assertion on the next line ( I have not touched any of it -- wrong file, not this diff's code, and finishing #7335's conversion belongs in its own change rather than riding along here. I have it written up and am happy to file it separately if you want, ideally checking with the #7335 author first since they chose the replacement shape. For this PR: a re-run of |
github-actions
Bot
added
readiness: action required
|
Two measurements from running this branch against 1. One over-refusal the storage-time predicate introduces
Mechanism: Mitigating context, which is why this is a note rather than a request: it is the fail-closed direction, and it matches how the vet's other rules already behave. 2. The failing Windows lane does not look like it is yours The That file is not in this diff. It reads as runner-timing flake, so a re-run should clear it, and |
|
Both measurements reproduce. Thank you for framing them as costs rather than asks -- that turned out to be the right call on the first one, and I want to record why with the measurement rather than just agreeing. 1. The over-refusal is real, and slightly broader than statedReproduced on
So it is not only single quotes -- double quotes suppress expansion too, and three of those four are genuine over-refusals this PR introduces. Your precedent also checks out as stated: 2. Why I am keeping it quote-blind rather than fixing the over-refusalThe obvious refinement -- skip braces that sit inside quotes -- is unsafe, because bash performs brace expansion before quote removal. Braces that straddle or contain a quote still expand, while a left-to-right quote tracker would read the region as inert: Four of six expand with a quote inside or adjacent to the braces. The concrete consequence, using a neutral stand-in directory name so nothing sensitive goes in this comment: The literal text carries no Doing it precisely needs a tokenizer that models expansion ORDER, not quote matching, which is a bigger change than this PR should carry and wants its own review. So: accepted cost, kept fail-closed, and written up separately rather than left implicit. If you would rather I add a narrower mitigation, say so and I will -- I did not want to ship a half-measure into a security predicate on my own judgement. 3. On the Windows lane -- agreed on ownership, one correction on "flake"Same conclusion as yours on whose code it is. I had traced it independently: The one place I would not go as far as "runner-timing flake": I measured it, 20 runs under CPU contention on Linux, and got STABLE 0/20. That is absence of evidence rather than a confirmed flake, so I am recording it as "not reproduced" and leaving the flake question open. One thing that may be more useful than a re-run: |
github-actions
Bot
added
the
readiness: action required
aa2120e to
6202d65
Compare
|
Both taken. The duplicate is deleted in favour of the export. You're right, and the sharpest part of the criticism is that I did it in the same PR whose Behaviour across the swap: all six continuation payloads still refused, the bash differential still 29 shapes / 0 holes / 1 over-refusal, 205 tests in that module. On the CRLF divergence: it's real, and it's the exported helper that departs from bash — I adopted it anyway, deliberately. Your clears-when offered "name a measured behavior the exported folder gets wrong" as the alternative route, and I have one: bash escapes the CR and lets the LF terminate, so I still deleted mine, because for a refusal scan over-folding is fail-closed: it can only join text the shell keeps apart, which makes this gate see more adjacency and refuse more, never less. A correct private copy plus a guaranteed drift risk is worse than one divergence in one place — and the divergence is now recorded in the call-site comment and the commit message rather than lost by adopting. If you'd rather the shared helper matched bash exactly on CRLF, that's a one-line change in Separately, and the reason I re-tested rather than pointing at the SHA: round 11 added
The 71 ms is the whole vet, not just this scan, and it's once per tick per job. The bound itself is intact: 8k / 200k / 2M cost 188 / 347 / 1880 ms. Noting for the record that Opus 4.8 has since cleared this head's predecessor (✅ no blocking findings on Two offers still open and unanswered, both structural rather than code: splitting the continuation fix into its own PR against |
github-actions
Bot
added
readiness: checking
6202d65 to
83f9b1c
Compare
|
Both of the items that are mine are done. The third is the one I keep declining to do unilaterally, and I'll say plainly what that now costs you. Owning specs, updated in this commit — you were right that AGENTS.md makes it mandatory (
The installed-base regression: you're right, and my description was wrong to frame it as an import-path cost. That was a framing error of mine, not a code defect. Added an "Installed-base impact, stated plainly" section saying what actually happens — a stored Worth being explicit about why it can't be had one way only: the retroactivity is the property that closes the legacy-store hole you found last round. A fire-time re-vet that skipped pre-existing jobs would be exactly the gap. So the honest description is "this reaches jobs that predate it, both when that helps and when it hurts" — which is what the section now says. Extglob: still not filed, and this is now the third clears-when it blocks. I've twice told you in this thread that I'd wait for your word before opening a public issue describing an unfixed credential-read route on Same standing offer on splitting the continuation normalization into its own PR against Verification for this round, since I changed |
83f9b1c to
27f1118
Compare
|
One of the two spec files you name was stale and is now fixed; the other claim doesn't hold, and I'd rather say so than quietly "fix" a file that was already correct.
That makes three owning docs updated in this commit: On your first Watch item — I agree it's yours to accept, and I've stopped framing it as anything else. The description now carries an "Installed-base impact, stated plainly" section saying that a stored Both Suggestions: I want to do both and have offered both twice. They're the same standing offer, unanswered:
I've held off on both for one reason, and it's worth stating so you can overrule it in a sentence: each means publishing a new public description of an unfixed credential-read path on |
github-actions
Bot
added
readiness: checking
|
The spec item cleared — thanks for re-checking it. Both remaining Watch items are yours, and I have a measured answer on the Suggestion rather than a change. Your governance observation is correct. Verified rather than assumed:
My round-11 addition is what made it execute twice per fire. I'm not folding it, and here's the measurement behind that:
The only fold that keeps the pre-existing One correction to the framing, in your favour: the redundancy is not free, but the cost isn't CPU. Tracing the paths:
So the real price is doubled degrade-audit volume on a host whose governance is already broken. If that noise matters more to you than the audit-scope distinction, the fold is the right call and I'll do it — that's a judgement about your SEL trail, so I'd rather you make it than have me quietly pick. On the two Watch items — both are explicitly yours and I've stopped arguing either way:
On #2 and on splitting the continuation fold, the position is unchanged and I want to be precise about why, because it is now the only thing I am declining to do: both actions publish a new public description of an unfixed credential-read route on Current head is |
Command crons were refused outright on any Linux host where /bin/sh is bash
(the default on AL2023, RHEL and Fedora): the POSIX-strict probe accepted only
a shell that preserves `x.{a,a}` when invoked `sh -c`, bash expands it, and
the resolver never looks past /bin/sh and /usr/bin/sh, so installing dash was
not a workaround either. Every command cron failed and auto-paused after five
consecutive failures.
The property the probe defends is real -- the storage-time vet gate tokenizes
the command once, so a runtime re-expansion widens what a deny-list can see --
but bash brace expansion was the one composition form the vet gate never
refused, which left the shell deciding whether the gate held.
- mcp_cron: refuse bash brace-expansion syntax (`{a,b}`, `{1..9}`) at storage
time, alongside the existing refusals for \$(...), \$'...', backticks,
non-plain \${...}, positional parameters and loops. The refusal reads QUOTE
STATE rather than matching a pattern, because bash requires the braces and
the separator unquoted but not the alternatives: `{w,"w w"}`, `{w,'w w'}`,
`{w,$'w w'}` and `{w,w\ w}` all expand, so a whitespace-free character class
would exempt exactly the live shapes. Three properties are load-bearing and
each was measured, not reasoned:
* whitespace disqualifies a group only when BARE, and only space, tab and
newline count -- bash still expands across form feed, vertical tab, CR
and NBSP, all of which `str.isspace()` calls whitespace. Since the
disqualifier is what makes the scan ALLOW, a generous whitespace class
fails OPEN. Bare space is what keeps `awk '{print x, y}'` allowed;
* nesting is counted, because `{{x}h,h}` expands and stopping at the first
`}` reads the outer group as separator-free;
* two levels are scanned -- the command as written, and the command with
quote delimiters removed, which is what a nested shell receives.
`bash -c "cat ~/.ss{h","h}/KEY"` puts the separator outside both quoted
runs while the braces sit inside, so no single-level rule can see it,
and the inner shell expands the reconstructed word onto the real path.
The cost is stated in the error text and the docstring: a quoted regex
interval (`grep -E '[0-9]{1,3}'`) is refused, with the escaped double-quoted
spelling `grep "[0-9]\{1,3\}"` offered as an accepted BRE equivalent.
- mcp_cron: scan what the shell PARSES, by folding backslash-newline before any
check runs, using the security module's own exported
`shell_normalizer._fold_line_continuations` rather than a second spelling of
it. A continuation is removed before parsing, so it splits whatever token a
static check matches on and the shell rejoins it afterwards. Every refusal in
`_vet_shell_command` was bypassable this way, and the un-split spelling of
each payload was refused as expected -- the credential-path pattern
(`cat ~/.ss\<newline>h/KEY`, which carries no composition form at all), the
command-substitution refusal (`$\<newline>(`), the non-plain `${...}`
refusal, the ANSI-C refusal, and the brace scan's sequence form
(`{s.\<newline>.s}`, since a comma is one character and cannot be split but
`..` is two). 7 of the added assertions fail without the folding. POSIX
requires the removal, so this is not bash-specific -- `sh` resolves the split
path too. One divergence measured while adopting the shared helper: it also
folds `\<CR><LF>`, which bash does NOT (bash escapes the CR and lets the LF
end the command, so `echo a\<CR><LF>b` runs as two commands). For a REFUSAL
scan that over-folding is fail-closed -- it can only join text the shell keeps
apart and make this gate see more adjacency -- so adopting it is safe here and
keeps the divergence in one place instead of two.
- mcp_cron: read quote state through `security.shell_normalizer._iter_shell_chars`
rather than a private copy of the rules. That generator is THE quote/escape
machine here and its docstring records the escape a copy gets wrong: inside
`$'...'` a backslash escapes, so `$'a\'b'` does not close at the escaped
quote. Measured on `x $'a\'b' {p,q} y`, a hand-rolled copy closed early,
reopened on the next quote and then disagreed for the rest of the string,
labelling an UNQUOTED `{p,q}` as single-quoted -- 10 of 17 positions. That
mislabelling did not open a hole in the brace rule, since the group is
mislabelled uniformly and the rule compares a group against its own opening
state, but the continuation normaliser above decides literalness from these
same states and runs BEFORE the `$'` refusal, so the desync was reachable.
`_quote_states` is now a thin adapter and the only remaining difference from
the generator is the quote characters themselves, where this convention
deliberately reports the state a quote is changing FROM.
- mcp_cron: bound the WORK the brace scan may do, and refuse when the bound is
hit. The walk is quadratic on a hostile shape -- a long run of `{` with no
closing brace at the same state makes the inner scan run to end-of-string for
every one of them. Measured: 145 ms at 1k, 572 ms at 2k, 2.3 s at 4k, 9.2 s at
8k, i.e. ~4x per doubling, so a few hundred KB hangs the process. A length cap
alone does not fix it: `cron_add` is capped at 5000 by
`validation.FieldSpec("command", max_len=5000)`, but `portability.py` re-vets an
IMPORTED job with the raw dict value where that cap does not apply, and 5000
still costs seconds once per job. Bounding steps bounds every shape, which is
the same reason `_CRON_MAX_GLOB_WORD` bounds the word handed to fnmatch rather
than the command. Exhaustion REFUSES with its own message: no verdict was
reached, so the command is not clean, and short-circuiting to clean would turn
a denial of service into a bypass. 2M characters now costs 1.8 s.
- mcp_cron: re-run the composition scan at FIRE time for `command` jobs, not
only at storage time. This is what makes the first half reach the installed
base: `vet_job_at_fire_time` exists because "a policy tightened AFTER
scheduling would never be re-evaluated", and it already re-scanned a `script`
BODY while a `command` body got only the governance ceiling. That asymmetry
is load-bearing once the resolver accepts a brace-expanding bash -- measured,
the ceiling ALLOWS `set -B; cat ~/.a{w,w}s/creds` while the storage-time scan
refuses it, so a command stored before this refusal existed would still run
after it, on the very executor this change newly admits. Deny semantics at
that seam are the right ones: the run fails, the job is KEPT, and the refusal
is audited under its own `cron_command_body` scope, mirroring
`cron_script_body` -- which also makes a newly-refused shape such as a quoted
regex interval surface as a legible audited failure instead of silence.
- docs: update the three owning documents in the same commit, which AGENTS.md
requires. `docs/system-specs/modules/governance.md` enumerated the fire-time
contract for a `command` job as the capability gate + the `commands` ceiling;
there is now a third check, and the entry also states why the ceiling and the
composition scan are distinct decisions that both have to be re-run.
`docs/guides/windows-install.md` gave brace expansion as the reason Windows
refuses command crons; that reason is retired by this change, so it now cites
what the code actually cites -- a shell whose language is wider than the one
the vet was written against.
`docs/system-specs/modules/learn-cron-dashboard.md` presented the fire-time
body re-scan as script-only, which is the asymmetry this change removes, so it
now states that a `command` body is re-scanned on the same terms and that the
"denied on every tick until it is edited" property applies to a stored command
a newly-added refusal rejects.
- cron_script: accept a trusted shell invoked with brace expansion off (`+B`).
The probe tries the plain form FIRST, so dash/ash/a real POSIX sh keep their
exact current argv, and the form that PASSED is recorded and re-used by the
executor through one shared argv builder -- previously the probe and the
executor spelled argv separately, so the probe could stop proving what runs.
The first half is what makes the second safe: with brace syntax refused at
storage time, a command cannot re-enable expansion and have anything left to
expand. Windows still refuses command crons by design, unchanged.
Every brace row in the tests was run against real bash, comparing `bash -c`
against `bash +B -c` on the same word, and for the nested level comparing
`bash -c 'bash -c "..."'` the same way. The assertion is one-directional: a
shape some shell expands must be refused, a shape every shell leaves literal
must not regress. The one deliberate over-refusal is a SINGLE-quoted group
such as `awk '{a,b}'`, since single quotes survive one level of double-quoted
nesting; the double-quoted spellings are refused for cause, because they
reconstruct into a live expansion.
Not fixed here, and reported on the PR rather than silently widened: brace
expansion is not the only unrefused composition form once bash is the
executor. An extglob pattern such as `.s@(|s)h`, enabled via
`shopt -s extglob; export BASHOPTS` and a nested shell, composes the private
key directory with no `eval` at all, and a pre-set `BASHOPTS` in the
executor's environment needs neither. That is a different composition form with its own
over-refusal cost (`awk '!(NR%2)'`, `awk '{print 2*(x+1)}'`), so it is the
maintainers' scope call, not a change to ride along here.
27f1118 to
509481e
Compare
|
Your Watch item is right, and I measured the half you flagged as unconfirmable. The argv floor does not catch it. The protected-branch push guard is bypassable today.
re.compile(r"\$\(|\$\{|`|\{[^{}]*(?:,|\.\.)[^{}]*\}")
Through the real gate (
So it is not caught downstream, and the reason is structural: You're also right that this is the same root cause. This PR's scan started as I have not changed it here, and I don't think I should. It's a different module, a different feature, and pre-existing — putting a git-publish security change inside a cron availability PR is exactly the coupling the design lane asked me to avoid for the line-continuation fix. Two candidate fixes, for whoever picks it up:
Flagging severity plainly because it's outside cron: this defeats a protected-branch push guard, That makes three pre-existing items now queued behind this PR rather than in it: the four line-continuation bypasses, the extglob composition route, and this. All three are exploitable on |
|
Rebased onto main Clean rebase: no conflicts. I verified the one moved call site still lands correctly, Gates run locally on the changed files only: Please review the rebase result. A maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is now needed. Reply here if anything looks wrong. |
github-actions
Bot
added
readiness: action required
|
Reviewed, and the rebase result is correct. I checked both of your claims against the tree rather than reading the diffstat, since the conflict region is the one place where a clean-looking rebase could still be wrong. The moved call site. Confirmed, and it is the claim worth checking, because the invariant it protects is silent when it breaks. That is the whole property
Gates. Re-measured on your head in a separate worktree rather than taking the numbers:
Both match your counts exactly. Since you scoped your lint run to the changed files, I also ran the four CI commands verbatim on the whole tree on your head, which is the only file set that is a superset of CI's: I have not pushed anything over your rebase. Nothing in the diff needed changing, and a force-push would have reset the workflow-approval gate your push just re-armed, for no content. Current state on One thing I got wrong, and it is a process error rather than a code one. The distinction I should have drawn and did not: the findings inside this PR's own scope are ones you asked me to make public, down to adding payloads to Two questions, both yours to answer:
|
|
Three lanes re-reviewed the rebase result and one is red, so taking them in order of what actually blocks. GPT 5.6 (🔴 blocking) — F1 re-tested against
And the mechanism is precisely the one F1's own adjudication predicted would be missing: Level 1 still returns No regression on the shapes that must stay allowed, checked in the same run: So F1 is closed on the current head by the change GPT prescribed. The red check is that lane not having re-run its body since First principles (🟡 CONCERNS) and design (🟡 CONCERNS) converge on one item, and it is now filed: #9488. Both lanes reduced to the same clears-when. First principles: "the release shipping this names the newly-refused shapes and the BRE-escape remedy in its notes, or a linked issue tracks that." Design: "the behaviour change is recorded where the release PR will pick it up ... not only in this PR's description." That is a documentation and release-process item with no bypass in it, so Design's second Watch — the extglob gap — is the one I am holding, and now for a concrete reason rather than caution. Its clears-when asks for a tracked issue referencing this PR's measurements. Design's Suggestion — split the four line-continuation fixes into their own PR. I agree with the reasoning and it is the third time it has come up, so I want to be plain about the one thing that makes me hesitate rather than keep deferring. Splitting them out means this PR's brace scan temporarily loses the folding, and every refusal in Verification state on |
|
Correcting my previous comment on one point, because I had the GPT lane's state wrong and the difference matters for what you do about it. I said the red was that lane "not having re-run its body". It did run on That is different vocabulary from what the same lane emitted on the heads it actually reviewed, and the contrast is clean across four heads:
So the blocking comment you see on this PR is a A lane-side outage is ruled out. Other fork PRs completed real GPT reviews inside my failing run's own window:
#4762 and #7791 both finished between my run's start and end. The lane was working while mine was not, so this is specific to this PR rather than to the pipeline. What changed at
The 22x jump in longest line is a much sharper discontinuity than the 2.4x jump in size. Both arrived with the same three files: the owning-spec doc updates AGENTS.md requires in the same commit. The 2,053-character line is a single prose paragraph I wrote unwrapped in I could not narrow it further from here. What I am not doing. I am not reshaping the diff to get a review bot to finish. The doc updates are required in this commit, and amending means force-pushing over the rebase you just landed, which resets the approval gate you re-armed. Both of those are costs you own, so the order should be yours to pick. Suggested order, cheapest first:
Worth noting for the pipeline itself, separate from this PR: a lane that cannot complete currently presents as |
github-actions
Bot
added
readiness: action required
|
CI is terminal on Root cause, from the job log rather than the rollup. That is
It exited at the dependency check and never evaluated The 403 is intermittent across the repo rather than a clean outage, which is why I did not stop at "infra, not mine":
#4762 succeeded six minutes before mine failed, so I cannot claim the service was simply down. That left a real unknown, and it is the part worth reading. Because Measured against
So once the artifact upload succeeds, the coverage lane should pass on this tree. A re-run of On rebasing, which my own tooling recommended and I am declining with a reason. A staleness check flagged So a rebase cannot fix this, and it would cost you the approval gate you re-armed plus the rebase you just landed. Not doing it. Nothing to fix in the diff. Asks, both requiring write access:
|
github-actions
Bot
added
the
merge conflict
3 participants
Problem / Motivation
Every
commandcron is refused on a Linux host whose/bin/shis bash — thedefault on AL2023, RHEL and Fedora — with
❌ No POSIX shell available to run this command cron, and the job auto-pauses after five consecutive failures.There is no host-side workaround:
_resolve_command_shell()consults only/bin/shand/usr/bin/sh, so installing dash elsewhere does not help.Why it matters
The
commandcron surface is unusable on the most common Linux configuration.The failure is silent-ish in the worst way: the job is accepted at storage time,
fails at every tick, and then disables itself, so a user sees a scheduled job
that simply stopped happening.
What changed (motivation → approach → change)
Symptom.
_resolve_command_shell()returnsNoneon this host even though/bin/shexists and is trusted.Root cause.
_shell_is_posix_strict()accepts a candidate only if itpreserves the literal
x.{a,a}, and it probes exactly one invocation form,sh -c. bash expands tox.a x.a, so both trusted candidates fail. Measuredagainst
mainon this host:Why not just accept bash. The property the probe defends is real: the
storage-time vet gate tokenizes the command once, so a runtime re-expansion
widens what a deny-list can see. And bash brace expansion is a composition form the
vet gate never refuses —
$(...),$'...', backticks, a non-plain${...},positional parameters, loops and unresolved variable references are all refused
there, but
_CRON_BRACE_EXPANSION_REmatches${...}parameter expansion only,and a bare
{a,a}carries no$. Measured:So merely switching expansion off at the shell would leave a stored command able
to switch it back on.
Approach — two halves, the first is what makes the second safe.
mcp_cron: refuse bash brace-expansion syntax at storage time, next to thecomposition forms already refused there, with the same "ship a
scriptjob"remedy. The refusal reads QUOTE STATE rather than matching a pattern, because
bash needs the braces and the separator unquoted but not the alternatives:
{w,"w w"},{w,'w w'},{w,$'w w'}and{w,w\ w}all expand, so awhitespace-free character class exempts exactly the live shapes. Three further
properties are load-bearing, each measured rather than reasoned:
newline count — bash still expands across form feed, vertical tab, CR and NBSP,
all of which
str.isspace()calls whitespace. Since the disqualifier is whatmakes the scan ALLOW, a generous whitespace class fails open. Bare space is
what keeps
find -exec {} ;,echo {print}andawk '{print x, y}'allowed;{{x}h,h}expands (echo p{{x}s,s}q→p{x}sq psq),and stopping at the first
}reads the outer group as separator-free;delimiters removed, which is what a nested shell receives.
sh -c "cat ~/.a{w,w}s/creds"is refused because the inner shell re-parses the braces bare,and
bash -c "cat ~/.ss{h","h}/KEY"is refused because concatenating two quotedruns puts the separator outside both while the braces sit inside — no
single-level rule can see that, and the inner shell expands the reconstructed
word. Stubbing this scan out shows it is the only refusal in
_vet_shell_commandcovering either.The price is one deliberate over-refusal: a whitespace-free single-quoted
group such as
awk '{a,b}', since single quotes survive one level ofdouble-quoted nesting. The double-quoted spellings are refused for cause.
Targeting the braces rather than the switch
is deliberate: a
set -B/shopt -s braceexpanddenylist leaks (eval "set -B"reaches the same state), while a command with no braces left hasnothing to expand.
cron_script: accept a trusted shell invoked with brace expansion off (+B).The plain form is probed FIRST, so dash / ash / a real POSIX
shresolveexactly as they do today and their argv is byte-for-byte unchanged. The form
that PASSED is recorded and re-used by the executor through one shared argv
builder — previously the probe and the executor each spelled
[shell, "-c", ...]separately, so accepting a form in one without the other would have leftthe probe reporting strict while the command ran expanded. The record is
written only after a form passes, so a concurrent cron never observes a form
that is still being tested, and a fully refused shell leaves no record behind.
mcp_cron: re-run the composition scan at fire time forcommandjobs,not only at storage time. This is what makes half 1 reach the installed base:
vet_job_at_fire_timeexists because "a policy tightened AFTER schedulingwould never be re-evaluated", and it already re-scanned a
scriptbodywhile a
commandbody got only the governance ceiling. Measured, the ceilingALLOWS
set -B; cat ~/.a{w,w}s/credentialswhile the storage-time scanrefuses it — so without this, a command stored before the refusal existed
still runs after it, on the very executor half 2 newly admits. Deny semantics
at that seam: the run fails, the job is kept, and the refusal is audited
under its own
cron_command_bodyscope, mirroringcron_script_body.Windows still refuses command crons by design; that path is untouched. Its
rationale in
docs/guides/windows-install.mdno longer cites brace expansion,since that is now refused at storage time and on every fire — the refusal rests
on "a shell whose language is wider than the one the vet was written against",
which is the reason the code gives.
docs/system-specs/modules/governance.mdis updated in this commit too: its fire-time contract enumerated the capability
gate + the
commandsceiling, and there is now a third check.Installed-base impact, stated plainly
The fire-time re-vet is what closes the legacy-store hole, and it also means the
newly-refused shapes apply retroactively, not only to newly-authored jobs.
Concretely: a stored
grep -E '[0-9]{1,3}'cron that has been running fineunder dash will now fail on every fire until it is rewritten — audited, with
the remedy in the message (
grep "[0-9]\{1,3\}"for a BRE tool), but permanentuntil someone edits the job. That is deliberate and I think it is the right
trade, since the alternative is a gate that never reaches the jobs most likely
to predate it, but it is a behaviour change for hosts that were working, not
merely an import-path cost, and it probably belongs in the release notes.
Tests
7 of the added assertions fail on unfixed
mainand pass with the change. The fourquoted / escaped-whitespace payloads are additionally measured as
ALLOWfrom_vet_shell_commandunder a whitespace-free rule, end to end, before the scanreplaced it — they were live, not hypothetical.
test/test_mcp_cron_security.py— brace-expansion payloads added toMALICIOUS_COMMANDS, each hiding a path whose literal text the credential scancannot see:
cat ~/.a{w,w}s/credentials,cp ~/.ss{h,h}/id_rsa /tmp/key, thesequence form
cat ~/.s{s..s}h/id_rsa, the nestedcp ~/.a{w,{w}}s/credentials,the
set -Bre-enable route, the four quoted / escaped-whitespace spellings, andthe nested-shell form
sh -c "cat ~/.a{w,w}s/credentials". Brace-bearing butnon-expanding commands are added alongside so the refusal cannot widen into
find -exec {} ;/awk '{print x, y}'._BRACE_SHAPES_MEASURED_AGAINST_BASHcarries the whole decision table, every rowrun rather than reasoned: the same word is echoed under
bash -candbash +B -cand the outputs compared — a difference is an expansion, identical output is quote
removal only — and the nested level is measured the same way through
bash -c 'bash -c "..."'. 29 shapes, 0 holes, 1 over-refusal. The assertion isone-directional by design: a shape some shell expands MUST be refused, since letting
one through means the composed word reaches the executor unseen, while refusing a
shape every shell leaves literal is only a false positive. A third column pins the
exact over-refusals on top, so a later change cannot trade one for a hole by
loosening a shape nobody was watching.
test_brace_scan_keeps_a_nested_shell_coveredpins the nested-shell coupling separately, including the quote-concatenation
spelling, so a change that "finishes" the quote-awareness fails on the test naming
the reason not to.
Line continuations, and four pre-existing bypasses they open. A backslash-newline
is deleted before the shell parses, so it splits whatever token a static check
matches on and the shell rejoins it afterwards. Every refusal in
_vet_shell_commandwas bypassable this way. Measured split-vs-control, all controls refused:
cat ~/.ss\<nl>h/KEY, no composition form at all$\<nl>(— command substitution$\<nl>{A}— non-plain${…}$\<nl>'\x73\x73'— ANSI-C quoting{s.\<nl>.s}— this PR's brace scan, sequence formThe first four are pre-existing on
mainand none is bash-specific: POSIX requiresthe removal, and
shresolves the split path exactly as bash does — so they arereachable on
mainon any host. The first needs no composition syntax at all.The fix is one normalization ahead of every scan, not five per-rule patches, so each
rule sees the string the executor will; it is scan-only, and quote-aware because inside
single quotes a backslash is literal and the shell never joins those halves. An escaped
backslash is likewise not a continuation —
echo a\\<nl>bis two commands, verified.7 of the added assertions fail without the normalization. Happy to split the four
pre-existing ones into a separate PR against
mainif you'd rather they moveindependently of this one.
Not fixed here
Brace expansion is not the only unrefused composition form once bash is the
executor, and the review lane was right to challenge the premise — the wording above
is corrected accordingly. Extglob composes a path out of text no static scan sees,
and
+Bdoes not disable it (+Bis brace expansion only). Measured against a realfixture, with
.q@(|x)rstanding in for the credential directory:So refusing
eval, orshopt, would each be insufficient: the switch can be setentirely outside the command text, which is the same reason the brace refusal targets
the braces rather than
set -B. The fail-closed fix targets the pattern syntax, andthat has a cost this PR should not spend on the maintainers' behalf — a quote-blind
refusal of
@( +( !( ?( *(also takesawk '!(NR%2)'andawk '{print 2*(x+1)}'.A precise alternative is to teach the existing
_glob_could_reach_credentialsaboutextglob so only words that can actually reach a credential path are refused, which is
more work and its own review.
Correction to my earlier framing here, since the design lane caught it and it
matters for who owns this. I first wrote that the route becomes reachable because
of this PR. That is wrong: it pre-exists on any host that merely has bash, because
the cron command can invoke bash explicitly and what
/bin/shis then does not matter— verified,
env -i /bin/sh -c 'bash -c "shopt -s extglob; eval …"'composes the path,and so does the same with
bash --posixoutside. So the extglob fix should not begated on this PR, and it is more urgent than "only if this merges" implied, not less.
What this PR does change is narrower: it makes bash the cron executor by default on the
most common distros while the vet gate models only POSIX-sh composition forms.
test/test_cron_script_more_coverage.pytest_a_brace_expanding_trusted_shell_is_accepted_with_expansion_offpins boththe acceptance and the probe ORDER (plain form first, then
+B), and that_command_argvthen returns the+Bform.test_a_posix_strict_shell_keeps_its_exact_argvpins the no-regression half:a strict shell still gets
[shell, "-c", command].test_the_spawn_uses_the_form_the_probe_provedasserts the executor's realspawn argv carries
+B— the gate on the two-literals defect.test_a_shell_that_expands_in_every_form_records_no_formpins that a refusedshell leaves no brace-off record for a later caller.
Measured:
354 passed, 1 skippedacrosstest_mcp_cron_security.py,test_cron_script.py,test_cron_script_more_coverage.py. With the two source files reverted tomainand the tests kept,
7 failed.Manual verification
Executed on an AL2023 host with
/bin/sh -> bashand no dash installed: beforethe change
_resolve_command_shell()returnsNoneandrun_command_sandboxed("echo hello")refuses; after it,/bin/shresolves withthe brace-off form recorded and the same call runs the command. The shell
measurements in the table above are unstubbed
subprocessruns; the probemeasurements stub
wrap_argvto the identity only because this host has nouser-namespace sandbox backend, which would otherwise fail-close for a reason
unrelated to the behaviour under test.
Screenshots / video
N/A — no user-visible UI change. The diff touches the cron command shell
resolver, one storage-time vet predicate and their tests; there is no panel,
component, layout or theme surface involved.
Related Issues
Fixes #7412
Pattern harvest
Rule candidate: review-prompt
Pattern: a probe and the executor it is meant to certify build their argv from
two separate literals, so the probe can keep passing while the executed form
diverges. Any "prove this invocation is safe, then run it" pair should share one
builder.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement