fix(security): count a backtick substitution once, not twice - #8557
Conversation
`_find_substitution_openers` counted every backtick as a substitution opener. A backtick substitution is delimited by TWO backticks, an open and a close, while `$(`, `<(` and `>(` are one opener per substitution -- so the count double-counted by construction and halved the effective ceiling for backtick spellings. The test that pinned it said as much and accepted it as "the conservative direction", which was free while the only subject was a shell command line: nobody hand-writes 33 substitutions. It stopped being free once a source body's literals became subjects of this pass. A markdown code span in a docstring IS a backtick pair, so an ordinary docstring with 33 code spans read as 66 nested substitutions and the whole cron script was refused -- at every fire, with a message about shell substitutions, for prose. Counting pairs is accurate and still conservative: ceil keeps an unbalanced trailing backtick, which opens an unterminated substitution, from rounding away, so the result never UNDER-states how many substitutions a shell could see. The direction is what the measurements support. The budget exists because the view walk is quadratic in `$(` NESTING depth, and that is what it measures as -- depth 32 costs 3.9 ms, 64 costs 13.6 ms, 128 costs 57.1 ms. Backticks are nowhere near it: genuinely nested (escaped) backticks stay at ~0.2 ms to depth 32, and 512 FLAT backticks cost 19 ms, linear. So the character whose ceiling this halved is the cheap one. The bound itself is unmoved and pinned in both directions: `$(` nesting past the ceiling still refuses, a backtick count that reaches the ceiling on PAIRS still refuses, and a real traversal sitting among the code spans is still denied rather than laundered by them. Co-authored-by: Kiro Crew <kiro-crew@amazon.com>
4381634 to
a802597
Compare
github-actions
Bot
added
fork
|
#8526 records This is now confirmed by construction rather than assumed: #8550 and this PR carry disjoint changes on the same base — that one edits This PR's diff is |
github-actions
Bot
added
readiness: action required
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of The change is well-grounded: the base code confirms docstrings/literals are scanned subjects (security.py:9809), the budget guards the quadratic Design-Verdict: PASS A genuine counting defect fixed at its root, with the DoS bound's expensive dimension measured and provably untouched. [DESIGN-REVIEWED] a802597 |
First Principles Review (Fable 5, fork) — ✅ PASSPremise-level review of All claims verified against the base. Final review: First-Principles-Verdict: PASS A counting bug — one backtick pair read as two openers — refused real cron scripts for prose; the fix corrects the count at its cause. What this change shipsIntent: stop the security gate refusing cron scripts whose docstrings carry markdown code spans — a FIX.
Depth check: this is a cause-level fix, not a symptom patch — the counting expression itself was wrong even for shell command lines, and it converges on the module's existing pair-count idiom ( Subtractions
[FIRST-PRINCIPLES-REVIEWED] a802597 |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
github-actions
Bot
removed
the
readiness: checking
2 participants
Problem / Motivation
_find_substitution_openerscounts every backtick as a substitution opener:A backtick substitution is delimited by two backticks — an open and a close —
while
$(,<(and>(are one opener per substitution. So the count double-countsby construction and halves the effective ceiling for backtick spellings. The test that
pinned it said exactly that and accepted it:
That was free while the only subject was a shell command line — nobody hand-writes 33
substitutions. It stopped being free once a source body's literals became subjects
of this pass: a markdown code span in a docstring is a backtick pair, so an ordinary
docstring with 33 code spans reads as 66 nested substitutions and the whole cron script
is refused, at every fire, with a message about shell substitutions, for prose. Measured
on one real install, that is what refuses a 1107-line script whose offending literal is
a 2315-character docstring carrying 66 backticks and zero
$(.Why it matters
The double-count was free while the only subject was a shell command line — nobody hand-writes 33
substitutions, so halving that ceiling cost nothing observable. It stopped being free the moment a
source body's string literals became subjects of this pass, and the thing it now refuses is the most
ordinary text there is: a docstring with markdown code spans.
The cost lands where it is hardest to read. The refusal happens at every fire of the cron job,
and it names nested shell substitutions for a body whose offending literal is English prose with
33 backticked words — so the message points the reader at shell syntax that is not there.
It is also the wrong direction on the measurement: the ceiling exists for
$(nesting, which isquadratic (57 ms at depth 128), while backticks are linear and cheap (19 ms for 512 flat, 0.2 ms
nested to depth 32). The spelling whose budget was halved is the one that costs the least.
What changed
Count backtick-derived substitutions as pairs,
ceil-rounded:ceilrather than floor because an unbalanced trailing backtick opens an unterminatedsubstitution, so it must still count — the result never under-states how many
substitutions a shell could see.
Why this direction is the measured one
The budget exists because the view walk is quadratic in
$(nesting depth, andthat is what it measures as. Backticks are not close:
$(nesting, depth 32$(nesting, depth 64$(nesting, depth 128So the character whose ceiling the double-count halved is the cheap one, and the
expensive dimension the ceiling exists for is untouched.
Nesting depth is still bounded by pairs — every level of an escaped-backtick nest
consumes an open and a close — so
ceil(n/2)remains a sound upper bound onbacktick-derived depth, not merely a cheaper one.
Tests
test/test_security.py::TestFindTraversalReachesFence:echo `date` $(pwd) <(sort a)is 3 substitutions, not 4;
echo `datestill counts 1 (ceil);test_flat_backtick_spans_are_not_refused_for_the_budget, pins thebehaviour end to end — 66 backticks are 33 openers and a prose subject is judged
rather than refused, while
$(nesting past the ceiling still refuses, abacktick count that reaches the ceiling on pairs (65 spans) still refuses, and a real
find … -exec catsitting among the code spans is still denied rather than launderedby them.
Mutation-verified: restoring the per-backtick count reddens 2 cases.
Regression: 2479 passed, 1 skipped across
test_security.py,test_security_alt_traversal.pyandtest_mcp_cron_security.py. The over-budgetwidecase in the existing budget test is built from$(, so it is unaffected (75openers, still over).
Gates:
isort,flake87.1.0,mypy,scripts/check_black_formatting.py,scripts/check_subprocess_encoding.py,scripts/docs_lint.py— all pass.Relationship to #8550
Independent and independently justified: the double-count is wrong for a shell command
line too, and this PR stands on that plus the measurements above. It does not depend on
#8550 and does not touch its hunks.
The two together are what recover the install measured in #8550: 11 of 13 scripts with
#8550 alone, and this removes the budget refusal on a twelfth. Ordering does not matter
for correctness of either.
Refs #7912.
Pattern harvest
Rule candidate: semgrep.
Pattern: counting a SAME-CHARACTER-paired delimiter's occurrences as if each occurrence were an opener. The defect is visible in one expression:
Three of those four terms count a delimiter whose opener differs from its closer, so each hit is one construct. The backtick term counts a delimiter whose open and close are the SAME character, so each construct contributes two — the term double-counts by construction, and mixing it into the same sum silently halves the budget for that one spelling. A budget is exactly where this hurts, because the error is invisible until some subject legitimately carries many of them (here, markdown code spans in a docstring).
Candidate check: flag a sum of
str.count(...)terms in which at least one argument is a single character that is its own closer (`,",') while another argument is a multi-character opener whose closer differs ($(,<(,${,[[). The shape is fully static — the literals are right there — and the fix is always the same: divide the self-closing term by two, rounding up so an unbalanced trailing delimiter still counts.Sibling worth checking under the same rule, not touched here:
_alt_pipeline_stages_boundedand the brace-expansion budget also derive their counts from delimiter occurrences; whether either mixes the two kinds has not been measured.Not generalizable: the specific ceiling value (64) is unchanged and was not the defect.