fix(security): keep docstrings out of a source body's traversal subjects (#8643)

[bolichen97]

Problem / Motivation

Since #8550, is_sensitive_source_body hands every string constant of a Python source body — docstrings included — to the two traversal passes as per-string command subjects. A prose docstring opening with a verb the traversal grammar models (an English sentence starting "Find …") reads as a find invocation whose word count exhausts the pass's 64-traversal-root budget, and that budget refuses fail-closed. Real cron scripts are therefore permanently refused for their documentation alone: measured on one real install, pr_security_patrol.py is refused on current main solely for its "Find commits on main that belong to no pull request…" function docstring, with the misleading verdict Blocked: command traverses a sensitive credential path and delivers the match to a command.

Why it matters

Any user cron whose docstrings begin with common English verbs is blocked from scheduling on every fire, permanently, until someone edits the documentation. The refusal text names a credential-path traversal that does not exist, so the user cannot even tell what to fix. The sibling literal-feedback scan measured the same class (#8563/#8564): 3 of 23 real scripts' docstrings drew traversal verdicts while ~3,700 non-docstring literals drew zero.

What changed (motivation → approach → change)

Symptom → root cause: the collection rule in _source_command_subjects shipped on the premise that over-collecting "can only ADD denials" because a non-command matches no rule. The premise is false for prose: the find pass parses wide English sentences as find command lines, so over-collecting adds false denials.

Change, in three parts:

• Exclude docstrings from the subject collection. New helper _docstring_constant_ids(tree, types=(str,)) identifies docstring-position constants exactly as ast.get_docstring does (first statement of a module/class/function body that is a bare str literal; bytes in that position is not a docstring and stays collected). _source_command_subjects skips those ids. Non-docstring literals — including bare statement-position strings that are not docstrings, and f-string fragments — flow through unchanged.
• Withdraw the exclusion for any body that can read a docstring back. subprocess.run(f.__doc__, shell=True) executes the docstring verbatim — the complete command sits in the tree as one Constant, no runtime assembly involved — so the prose argument does not apply there. _reads_dunder_doc matches any __doc__ name or attribute, getdoc, or the constant "__doc__" anywhere in the tree; such a body keeps every docstring as a subject, the exact pre-exclusion treatment. The guard is name-based and over-broad on purpose: a false positive only restores the stricter behaviour (fail-closed direction). None of the measured real scripts reads __doc__, so the availability fix survives the guard.
• One spelling of the position walk. The fence scan's inline retained walk in _sensitive_run_in_source_literals now calls the same helper with types=(str, bytes) — its deliberate over-retention (deny direction) now lives in one argument at one call site instead of two hand-maintained walks. The fence scan's behaviour is byte-for-byte unchanged: a docstring naming a fenced store is still denied.

Accepted residual (recorded in docs/system-specs/modules/security.md alongside the subject-collection description): a verbatim command in docstring position read back through reflection the guard cannot name (a computed getattr string) is no longer seen by passes 4/5 — the same runtime-assembly limit the module already records for the re-authenticity guards.

The _env_subject join is built from the same collection, so docstrings drop out of it too; a docstring cannot be a fragment of a +-assembled command (it is a standalone statement, not an operand), and the newly-adjacent neighbours in the join can only add a match — both the deny direction.

Tests

All in test/test_security_source_command_subject.py, each mutation-verified (10 mutations, each caught by a distinct test):

• TestDocstringsAreNotSubjects — the distilled real docstring that Docstrings drawn into #8550's traversal subjects re-block real cron scripts #8643 measured is allowed in all four docstring positions (module/class/function/async); the same text as an assigned literal still denies (the discriminating pair pinning position over content); a bare non-docstring statement string and an if-block first string still deny; a docstring naming a fenced store still denies (fence-scan retention unchanged); bytes in docstring position is still a subject; the fixture-guard asserts the mechanism (traversal-root budget) so the allowed-verdicts cannot go vacuously green.
• TestDocstringExclusionIsWithdrawnForDocReaders — the f.__doc__ / bare __doc__ / getattr(f, "__doc__") / inspect.getdoc spellings each keep the docstring a subject and deny; the withdrawal is wholesale (a doc-reading body's prose docstring refuses again, pre-Docstrings drawn into #8550's traversal subjects re-block real cron scripts #8643 treatment); a body without doc reads keeps the exclusion; unit coverage of every _reads_dunder_doc spelling.

Existing deny-vector and fallback tests (307 in the three touched suites) all pass unchanged.

Manual verification

Ran mcp_cron._vet_script_contents over the real cron scripts of one install: pr_security_patrol.py (refused on pristine main solely for its docstring) now vets clean; no other script's verdict changed. Verified the fix fixture is red on pristine main and green on this branch.

Related Issues

Closes #8643

Pattern harvest

Rule candidate: review-prompt
Pattern: a scan-subject collection that includes prose positions (docstrings, comments-as-data) feeds command-shaped grammars that model English sentences as invocations — check any new per-string security pass for prose false-positives, and pair any prose exclusion with a reflection-read withdrawal guard.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of eec4632ba354978beaac5f8b4c70a00f8e9284e6 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound, measured carve-out, but the prose-misread root cause stays in the find pass and the reflection guard's breadth quietly re-arms the false refusal.

Watch

• The withdrawal guard fires on ubiquitous idioms — any .format() call, any getattr, any dunder read except __name__, an importlib import — and each one restores the whole body's docstring refusal with the same misleading credential-traversal verdict the PR names as the harm ("the user cannot even tell what to fix"). A script that later adds one e.__traceback__ log line silently regresses to permanently refused, so the availability fix decays as scripts grow; the "zero collateral" evidence is 23 scripts on one install.
• The pass-level defect — _check_find_traversal_reaches_fence reading English prose as a find invocation and refusing on word count — is untouched, and positional exclusion cannot extend to the next occurrence: the same prose in an assigned constant (a prompt string, argparse help text, exactly what agent cron scripts carry) still draws the permanent false refusal, as the PR's own discriminating-pair test pins. Expect this class to resurface; the budget-proxy fix the spec already defers (backtick openers) is the real closure.
• _reads_dunder_doc must be simultaneously near-complete against the stdlib (a missed accessor lets a docstring-parked command evade passes 4/5) and narrow enough not to kill the fix — the four frozensets whose comments record four rounds of falsification are a standing maintenance surface with failure modes in both directions.

Suggestions

• When a traversal budget refuses, name the offending subject and its position in the verdict; that resolves the "cannot tell what to fix" harm on every path this carve-out leaves open, cheaper than the next carve-out.

[DESIGN-REVIEWED] eec4632

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of eec4632ba354978beaac5f8b4c70a00f8e9284e6 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in. Composing the review.

First-Principles-Verdict: CONCERNS

The fix earns its place, but the new reflection audit respells two existing frozensets, and its dunder-class rule quietly re-refuses ordinary class-based scripts.

What this change ships

Intent: stop cron scripts being permanently refused for their prose docstrings (#8643) — a FIX.

1. A cron script with a prose docstring is no longer refused for its documentation — justified (measured defect, discriminating tests)
2. A script touching any introspection surface keeps the old refusal for every docstring — justified, declared fail-closed companion
3. That audit respells 7 builtin names — partial duplicate of security.py:10575/10591
4. Docstrings also stop feeding the env-credential pipeline matcher — declared, deny-direction argued
5. Fence scan's docstring walk collapses into the shared helper — rides along (net deletion, behavior pinned)
6. Spec paragraph and cap comment updated in the same commit — justified (AGENTS.md mandate)

Watch

• The withdrawal's "any dunder attribute except __name__" class rule fires on super().__init__() and self.__class__, so a class-based cron with a prose docstring keeps exactly the Docstrings drawn into #8550's traversal subjects re-block real cron scripts #8643 refusal this PR removes. The "zero real-corpus collateral, measured" claim rests on 23 scripts from one install.
• The fix is position-scoped while the cause is the find grammar reading English as a find invocation: assigned prose (an argparse description="Find ...") still permanently refuses. Measured 0 of ~3,700 non-docstring literals today, and the grammar residual is recorded only for backtick spans — the assigned-prose sibling is not.

Subtractions

• Build _DOC_REFLECTION_NAMES as _DYNAMIC_EXECUTION_BUILTINS | _DYNAMIC_NAMESPACE_BUILTINS | frozenset({...}) — it respells all 7 names of those two sets (security.py:10591, security.py:10575), which exist for the same withdraw-on-opaque-capability reason and will otherwise diverge from this copy.

[FIRST-PRINCIPLES-REVIEWED] eec4632

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed eec4632ba354978beaac5f8b4c70a00f8e9284e6 — this comment is updated in place on each push.

Review details

I've validated the single candidate and reviewed the actual code. The candidate describes the docstring-exclusion residual: a docstring holding a traversal command that is read back at runtime through a reflection route _reads_dunder_doc does not enumerate.

Working (a)/(b)/(c):

• The only way an excluded docstring ever executes is being read back via __doc__ and handed to a shell. _reads_dunder_doc withdraws the exclusion on an extremely broad surface: any dunder attribute read, getattr/eval/exec/compile/__import__, globals/locals/vars, format/format_map, the frame/code-bearing attributes, and imports of inspect/pydoc/importlib/doctest/builtins/gc/ctypes under any alias.
• To reach the residual, the body must read the docstring back through none of those — requiring a genuinely novel accessor or runtime string assembly. The candidate's own examples (__builtins__["getattr"](...), runtime-assembled accessor names) are exactly the "if a caller were to assemble at runtime" shape Step 1 forbids, and the module deliberately declines to follow runtime assembly (the same documented limit as the re-authenticity guards).
• The fence scan (_sensitive_run_in_source_literals) still retains and scans docstrings, so a docstring naming a fenced store is still denied.

(a) cannot be pinned to a concrete non-exotic input; (b) requires assuming a runtime-assembled route the static walk cannot see. The candidate itself scores "low." It dies under falsification.

I also verified the refactor is behavior-preserving: _docstring_constant_ids(tree, types=(str, bytes)) reproduces the former inline retained walk exactly, and the traversal/env-join exclusion drops only non-executing prose. No crash path (node.value in frozenset is safe for all Constant value types).

No findings.

[OPUS-REVIEWED] eec4632

Verdict parsed from the review's SHA-scoped output markers for commit eec4632ba354978beaac5f8b4c70a00f8e9284e6.

False positive or not applicable? A repository writer can comment:
/ai-review override fable eec4632ba354978beaac5f8b4c70a00f8e9284e6: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of eec4632ba354978beaac5f8b4c70a00f8e9284e6 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] eec4632

False positive or not applicable? A repository writer can comment:
/ai-review override gpt eec4632ba354978beaac5f8b4c70a00f8e9284e6: <one-sentence reason>

[bolichen97]

(span=732b722c70fd)
F1 (aliased getdoc bypass) -- FIXED at root on head b9fcf3c. The finding was correct: from inspect import getdoc as gd binds a name the call-site walk cannot recognize, and this is statically visible, not the accepted runtime-assembly residual. Fix shape: _reads_dunder_doc now matches the ast.ImportFrom itself when any alias's name == "getdoc" (the import is the tell; the source module is deliberately not checked since a re-export forwards the same capability), and the constant branch widened to ("__doc__", "getdoc") so getattr(inspect, "getdoc") is caught too. A wildcard from inspect import * cannot rename, so its call site still hits the bare-Name branch. Pinned: test_aliased_getdoc_import_keeps_the_docstring_a_subject (end-to-end deny through _vet_script_contents) plus the aliased-import and getattr spellings in test_reads_dunder_doc_spellings; mutation-verified (deleting the ImportFrom branch fails exactly those 2 tests).

[bolichen97]

(span=732b722c70fd)
F1 round 2 (eval("command.__doc__") bypass) -- FIXED at root on head 5c8d211, as a whole-class audit rather than a per-spelling patch. Second finding of the same class was the signal to stop reacting per-spelling: _reads_dunder_doc now withdraws on the closed stdlib reflection surface (_DOC_REFLECTION_NAMES), grouped by route: (b) opaque code strings -- eval/exec/compile/__import__ -- the exact rule the re-authenticity guard already applies to these names (a code string can carry ANY read the tree cannot see); (c) computed-attribute accessors -- getattr/attrgetter/__getattribute__/__dict__ -- closes getattr(f, "__do" + "c__"); (d) namespace mappings -- vars/globals/locals -- closes globals()[key] with a computed key; (e) documentation modules under any alias -- inspect/pydoc/importlib + help -- the IMPORT is the tell, same as the aliased getdoc fix last round. Availability preserved, measured: exactly one real cron script touches the surface (one getattr) and its verdict is unchanged; all 12 still vet clean. Pinned: end-to-end deny tests for the eval-string, namespace-mapping, and aliased-module-import routes plus 18 unit spellings (3 negative, including re.compile staying outside the surface); mutation-verified (dropping eval/exec, the mappings, or the module-import branch each fails distinct tests). Residual restated in the module spec: a read-back through a route no stdlib name in this audit spells -- a genuinely novel accessor or reflection assembled at runtime -- is the same boundary the re-authenticity guards record.

[bolichen97]

(span=732b722c70fd)
F1 round 3 (doctest absent from the audit) -- the audit omission is FIXED on head 476f3ab; the execution gap itself is PRE-EXISTING and now tracked as #8824 with measurements. Fixed here: doctest added to _DOC_REFLECTION_NAMES and _DOC_REFLECTION_MODULES (it does not merely READ a docstring, it EXECUTES its >>> examples), so a doctest-invoking body now keeps every docstring as a subject -- byte-identical to main's treatment. Pinned by test_doctest_import_keeps_the_docstring_a_subject (observable flip: plain-command docstring + import doctest now refused) and two unit spellings; mutation-verified (removing doctest from the audit fails exactly those tests). Measured, the named attack is NOT a regression of this PR: the exact adjudicated body is allowed on this head AND under a main-equivalent run handing ALL string constants, docstrings included, to the traversal passes -- the >>> spelling is Python-call syntax, not shell grammar, so passes 4/5 never matched it on any historical treatment. The remainder ("parse and inspect its examples") is a new scanning capability, not a restoration: it requires doctest.DocTestParser extraction and recursive source-scanning that no version of this gate has ever had. Filed as #8824 with the probe data, sibling of #8812 in the embedded-language class -- out of this availability-fix PR's scope.

Probe evidence (both treatments agree):

A doctest body containing a shell-invoking call in >>> syntax -> alt: False, find: False (both old and new treatment)
The same shell command as plain text, no >>> -> alt: True (the shape the passes do convict -- now restored for doctest bodies by this round's fix)

[bolichen97]

Opus BLOCKING ("{0.__doc__}".format(f) field-attribute doc-read) -- FIXED at root on head 6ba11db. The finding was correct: str.format's field syntax resolves attributes at runtime while the reflection lives only inside the format-string constant, and the module's own re-guard precedent already forfeits on exactly this stringify route. Fix shape: format / format_map / vformat added to _DOC_REFLECTION_ATTRS, so any .format(...) method call withdraws the docstring exclusion (the call is the tell; scanning the constant's field syntax would be an enumerable-spelling chase, which is the wrong shape per the module's own deny-first arguments). Availability, measured: no real cron script calls .format( as a method (the only greps are isoformat/fromisoformat, which the exact-match set does not touch); all 12 real scripts still vet clean. Pinned: test_format_field_doc_read_keeps_the_docstring_a_subject (end-to-end deny, the exact finding trigger) plus format/format_map unit spellings; mutation-verified (removing the format family from the set fails exactly those tests). Also in this push: the Backend Lint failure was a black-format drift in the test file from the previous round's edit -- reformatted, baseline gate green locally.

[bolichen97]

(span=732b722c70fd -- current-head lane pointer only; this disposition's own subject is the round-3 F2 finding below, which the judged head e64218c no longer carries live)

Round 3 (head e64218c) F2: case x in x) pattern paren truncates the substitution body.

REBUTTAL: real property, wrong attribution; filed as #8830. The truncation lives in the SHARED span finder _matching_close_paren, which this PR reuses but did not change. Measured on pristine main (9b46746): the same probe input returns the same truncated span and the same False verdict on that head -- byte-identical to this head, and the git-publish boundary-walk consumer inherits the same cut. Modeling case grammar belongs in the one shared machine so all consumers heal together (this module's own two-scanners rule); patching it per-consumer inside this PR would fork the span logic. Tracked with evidence and fix direction in #8830. Scope note: this PR's remainder ledger now spans #8822 (sibling quote-blind delta windows + pre-existing quadratic walk) and #8830 (case-pattern span truncation in the shared finder).

[bolichen97]

/ai-review override gpt 7e4c48b: The blocking verdict is stale from bd12cb9 whose co_consts finding is fixed and mutation-verified on this head (frame/code group completed), and the provider content-refused this head so no fresh verdict is possible per the workflow's own stale notice.

• Why override is the right disposition here: the workflow's stale notice states the provider REFUSED to review this head because of the diff's own content (security-test payloads) and that a re-run is very unlikely to produce a fresh verdict, instructing a repository writer to adjudicate.
• The stale finding is not outstanding: f_code/co_consts and the full frame/code object-bearing group are in _DOC_REFLECTION_ATTRS on this head, pinned by the sys._getframe().f_code.co_consts[0], __code__, and gi_code spellings in test_reads_dunder_doc_spellings, and mutation-verified at both depths (carriers alone are caught at the _getframe mint; removing mint+carriers together fails the test).
• Independent verdicts on this content: Opus, Design, First Principles and UX lanes all completed and passed on prior heads of this same test-fixture pattern; the fixtures are assembled from fragments precisely to avoid carrying live sensitive spellings.

[github-actions]

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 7e4c48be49d6d430c87944c33462940525427ae4.

The blocking verdict is stale from bd12cb9 whose co_consts finding is fixed and mutation-verified on this head (frame/code group completed), and the provider content-refused this head so no fresh verdict is possible per the workflow's own stale notice.

This decision applies only to this commit. A new push requires a new judgment.