Skip to content

fix(analyzer): end an expansion word at its enclosing double quote - #780

Open
elliottwaves-20 wants to merge 6 commits into
NVIDIA:mainfrom
elliottwaves-20:fix/694-quoted-expansion-word
Open

elliottwaves-20 wants to merge 6 commits into
NVIDIA:mainfrom
elliottwaves-20:fix/694-quoted-expansion-word

Conversation

@elliottwaves-20

@elliottwaves-20 elliottwaves-20 commented Oct 6, 2026 •

Copy link
Copy Markdown

Summary

These inputs make has_bounded_parse_exhaustion (static_patterns_tool_misuse) report static_parse_limit once more than 4 KB of text follows:

  • a plain POSIX line such as echo "Started: $(date)";
  • the PowerShell form Write-Warning "Stale path: $($cfg.command)".

The file is then recorded as partially inspected, and every SKILL.md reference to it becomes an AE1. Refs #694 (the PowerShell interpolation case reported there; the POSIX form shows it is a general shell-parser issue).

Root cause

_has_shell_command_word_exhaustion checks every command-word candidate, including the $( inside the string. The word parser only knows where such a word ends when the quote is directly before it ("$(date)", via _command_wrapper_quote). With text in between, the word continues past ). The string's closing " then opens a new quote that runs to the end of the file, and the unresolved span exceeds _SHELL_COMMAND_WORD_CHARS. "x: $(date) y" was already complete only because the space ends the word first.

Fix

  • _parse_shell_command_word accepts an optional quoted_expansion_starts set. It records the start of each $(...), ${...}/$NAME and backtick expansion that sits directly inside a double-quoted span. Positions are committed only when that span's closing quote is reached, and are dropped when a parameter expansion consumes the quote (inherited_quote_closed).
  • _has_shell_command_word_exhaustion shares one set across its candidates. It parses a candidate at a recorded position with enclosing_delimiter='"', the mechanism already used for assignment values, so the word ends at the string's own closing quote. The runtime-command operand scan (_bounded_shell_tokens) receives the same boundary through a new optional enclosing_delimiter argument. Without it, a short script such as echo "x: $(date)" followed by rm -rf /tmp/build would newly become partial, because the operand scan would reopen the closing quote.
  • Expansions nested inside a substitution are never recorded, because the outer parser skips the substitution as a unit. The inner command words therefore remain independent candidates exactly as before.

Unchanged (stay partial):

  • Runtime-selected commands such as "x: $($TOOL -rf /)", "x: `$TOOL -rf /`" and "x: $(echo "a b"; $TOOL -rf /)".
  • printf reconstruction ("x: $(printf %s r m) -rf /").
  • A quoted runtime command word (x; "$CMD $(date)" -rf /) and sh -c strings.
  • Unclosed strings.
  • Runtime output joined to an escape, "Path: $($HOME)\bin". That word is already limited on main through the command-reconstruction check, and this PR does not touch that check.
  • Every rule finding from analyze(). For example, echo "x: $(rm -rf /)" keeps TM1.

Tests

tests/nodes/analyzers/test_double_quoted_expansion_word.py has 28 tests. 11 of them fail on main, and all controls pass on both.

  • Benign ⇒ complete: a substitution, two substitutions, a ${A:-$(date)} default, a nested quoted argument "x: $(echo "a b")", the PowerShell member and $env: forms, and a /bin suffix.
  • Already complete and still complete: backtick, ${HOME}, the adjacent quote, and a trailing word.
  • Short scripts with a later cleanup stay complete: echo "x: $(date)" followed by rm -rf /tmp/build, ; rm -rf ./build or rm -rf "$HOME"/cache.
  • Controls ⇒ partial: the eight unchanged cases listed above, plus the escape suffix, plus a runtime command after a mispaired or closed quote: a quote inside a comment, a quote inside single quotes, and echo "a $(x)"; $(printf r)"$X" -rf /.
  • Findings kept, scan level: TM1 is kept for the destructive substitution, and a run.sh helper is COMPLETED.
  • Full unit suite: the set of failures is identical with and without this change, which I ran together with fix(analyzer): treat a command wrapper with no command as complete #779 (wrappers without a command) (10108 passed). The remaining Windows-only failures also occur on unmodified main.
  • ruff check and ruff format --check are clean.
  • Real files: in three PowerShell maintenance scripts, the first parse-limit trigger was exactly this pattern: "...: $($manifest.servers.$n.command)", "...: $($cfg.activeProfiles -join ', ')" and "...: $($_.Exception.Message)". With this change the trigger disappears. Over 4000 files from about 200 public skills, no file goes from complete to partial, and the TM1 count is unchanged.
  • Differential fuzzing against main: I ran about 3000 targeted fragments (quoted expansions, mispaired quotes, comments, heredocs, assignments, followed by runtime commands and rm) and 3000 random ones.
    • Every input that went from partial to complete was checked. Each is either already complete on main in its equivalent plain form, such as $(printf rm) -rf / (where the TM1 finding is kept), or its "command" is inert: it lives in a comment, an assignment value or an argument, or it contains a space or t: and therefore cannot name rm.
    • A few inputs go from complete to partial. There, the string closes and real code such as ; $CMD -rf / follows, for example a="…$CMD"; $CMD -rf /". main reported these as complete.
  • The change merges cleanly with fix(static): stop code comparisons from reading as tag marker directives #777 and fix(analyzer): treat a command wrapper with no command as complete #779.
  • It has one small textual conflict with fix(analyzer): keep Python and Perl syntax from exhausting the shell parser #778, which moves the operand-scan call into _has_runtime_command_operand_exhaustion. The resolution passes enclosing_delimiter through that helper. I tested it applied on top of fix(analyzer): keep Python and Perl syntax from exhausting the shell parser #778: 1004 tests across this PR, fix(analyzer): treat a command wrapper with no command as complete #779, fix(analyzer): keep Python and Perl syntax from exhausting the shell parser #778 and test_security_reconstruction.py pass.

Residual

Other host-syntax families from #694 remain. In PowerShell, the backtick is an escape character ("`"$(...)`""). They also include JavaScript template literals, and Rust character literals and comment apostrophes: itoa 1.0.18 src/lib.rs is still partial on main. They need host-language ownership in the style of #778 and are not addressed here.

🤖 Generated with Claude Code

@yashrajp22 yashrajp22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One completeness regression is confirmed in both source and freshly installed wheels. The 111 selected HEAD tests pass in each mode, but the focused comment-ownership checks reproduce the issue below. Verification covered 80 CLI scans across BASE/HEAD and source/wheel modes; Greptile returned no findings on the narrower authored diff.

if owned_word_positions is not None:
owned_word_positions.add(cursor)
if quoted_expansion_starts is not None:
quoted_expansion_starts.update(pending_quoted_expansions)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep these positions local to the candidate and commit them only after the full word parses successfully and passes the comment/assignment checks? A referenced run.sh containing:

# "prefix $(date)
$CMD"" -rf /

leaves $CMD in the shared set when the parse starting in the comment later returns None. Its independent scan then treats the first quote as an enclosing delimiter and skips the destructive operands, even though the empty quotes belong to the real command (CMD could be rm). In both source and fresh-wheel scans, BASE retains static_parse_limit and AE1 with safe_to_install=false; HEAD reports complete inspection, drops AE1, and returns safe_to_install=true. Please discard quote ownership from failed/comment parses and cover this concatenated-quote case in a regression test.

elliottwaves-20 and others added 6 commits October 7, 2026 13:27
`echo "x: $(date)"` followed by more than 4 KB of text, and the PowerShell
form `"Stale path: $($cfg.command)"`, were recorded as
`static_parse_limit`. The `$(` inside the string is its own command-word
candidate, but only a quote directly before it (`"$(date)"`) told the word
parser where the word ends. Otherwise the word ran past `)` and the
string's closing quote opened a new quote that reached the end of the file.

The word parser now reports the expansions (`$(...)`, `${...}`/`$NAME`,
backticks) that sit directly inside a double-quoted span, and only once
that span's closing quote is reached. A candidate at such a position is
parsed with that quote as its enclosing delimiter, the mechanism already
used for assignment values, and its operand scan stops at the same quote.
Commands nested inside a substitution, unclosed strings and runtime output
joined to an escape keep today's results.

Refs NVIDIA#694

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: elliottwaves-20 <pail1217@web.de>
…arse

A parse that starts inside a comment can close a double quote on the next
line and record the real command there as quote-enclosed. When that parse
was later discarded or confined to the comment, the record stayed, and the
command's own operand scan stopped at its first quote, so `$CMD"" -rf /`
was reported as completely inspected. Collect the positions per candidate
and commit them only under the gate that already protects owned word
positions.

Reported-by: yashrajp22
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: elliottwaves-20 <pail1217@web.de>
A double-quoted span that crosses a line may pair a heredoc, comment or
prose quote with a later command line. Pin that such a command keeps its
operands under inspection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: elliottwaves-20 <pail1217@web.de>
The leak was not limited to comments: any candidate parse that closed a
double quote and was then discarded left its quoted-expansion starts
behind. Pin a plain code case that fails without the previous fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: elliottwaves-20 <pail1217@web.de>
A successful parse can still pair a quote from a heredoc body or Markdown
prose with the empty quotes of a later command line when another quote
follows, for example a trailing `# "` comment. It then recorded that
command as quote-enclosed, and the command's `-rf /` operands were never
inspected. Only a double-quoted span that stays on one line now ends the
word of an expansion inside it; multiline spans keep the conservative
partial result.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: elliottwaves-20 <pail1217@web.de>
…spans

CommonMark ends a line at a lone CR as well, so a Markdown file with CR
line endings could still pair a prose quote with a later command line.
A single-line span now contains neither LF nor CR; the CR of a CRLF line
ending lies outside such a span, so single-line strings stay complete.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: elliottwaves-20 <pail1217@web.de>
@elliottwaves-20

Copy link
Copy Markdown
Author

Thanks @yashrajp22. I confirmed the regression, and while fixing it I found a second path to the same bypass.

Root cause. _parse_shell_command_word wrote quoted-expansion starts straight into the shared quoted_expansion_starts set as soon as a double-quoted span closed. Two things could go wrong with that:

  1. Discarded parses. A candidate parse that started in your comment closed its quote on the next line and recorded $CMD. The parse was then discarded, but the record stayed. This also happened outside comments, for example "'$(x)'"$CMD followed by $CMD"" -rf / on the next line.
  2. Successful but mis-paired parses. A parse can pair a quote in a heredoc body or in Markdown prose with the empty quotes of a later command. If another quote follows, the parse succeeds, e.g. cat <<E / "a $(date) / E / $CMD"" -rf / # ".

In both cases the real $CMD was treated as quote-enclosed, and its operand scan stopped at its own first quote.

Fix.

  • 63ae555: each candidate collects its starts in a local set. That set is merged only when the parse succeeded and is not confined to a comment or assignment, the same gate that already protects owned_word_positions.
  • bbeda8e and 9fca6bb: a start is recorded only when the enclosing double-quoted span stays on one line, with no LF and no lone CR (CommonMark treats a lone CR as a line break). Multiline spans keep the conservative partial result. The CR of a CRLF ending lies outside a single-line span, so echo "x: $(date)" stays complete with CRLF too.

Tests in test_double_quoted_expansion_word.py:

  • test_quote_from_a_discarded_or_comment_parse_grants_no_ownership covers your case, a completed comment parse, a comment after a shebang, and the plain-code case. Each runs short and padded.
  • test_reviewer_script_is_not_fully_inspected runs your run.sh through run_static_patterns_with_ledger and asserts the outcome is not COMPLETED.
  • test_cross_line_quote_pairing_stays_partial covers heredoc bodies ($(…), backticks, quoted delimiter), Markdown prose before a fence, quotes closed by a trailing # " or by a later echo ", and lone-CR line endings.
  • test_heredoc_quote_cannot_hide_a_runtime_rm_at_scan_level is a scan-level run.sh with CMD="${TOOL:-rm}".

All of these pass on main. All except the completed-comment-parse pin fail on the previous head.

Rebase. The branch is rebased onto current main, including #778. The only conflict was the call into _has_runtime_command_operand_exhaustion, which now takes enclosing_delimiter and forwards it to _bounded_shell_tokens.

Verification against main 3c8e4b9:

  • Full suite: the set of failing tests is identical on main and on this branch. The failures that do occur are Windows/environment-specific on my machine (release, git and CLI-help tests).
  • Corpus of about 4,000 skill files: no file gets worse, and 2 files move from partial to complete.
  • Differential fuzzing, 100k generated shell snippets. The generator includes trailing # " comments, later echo " lines and heredoc terminators.
    • Every case where main is partial and this branch is complete went through a non-executing shell oracle. In that oracle PATH is empty and command_not_found_handle only logs.
    • 153 of these cases would make a shell invoke rm with /. 151 still carry TM1. The other 2 belong to an existing class on main, described in the next section.

Disclosure: an existing bypass on main, not caused by this PR. main already reports these inputs as completely inspected, with no TM1. A quote in a comment or heredoc body pairs with the empty quotes of a later real command, and no expansion is involved:

# x "
$CMD"" -rf / # "
cat <<E
he said "hi
E
$CMD"" -rf / # "

The same applies to cat <<E / "${X} / E / $CMD"" -rf / # ".

My fuzzing found one input of this class that main reports as partial only incidentally. An unrelated empty $() makes main end the earlier word elsewhere. As a Python literal, including the final newline:

'cat <<E\n"$(date)$()""\nE\n$CMD"" -rf / # "\n'

This branch reports it as complete, as main does for the simpler variants above. A second fuzz hit was built the same way, with a comment ending in a backslash before the heredoc terminator. Closing the class needs heredoc- and comment-aware quote pairing in the generic word scan, which is outside the scope of this PR. I am happy to open a separate issue with these reproducers.

@elliottwaves-20
elliottwaves-20 force-pushed the fix/694-quoted-expansion-word branch from a243e9e to 9fca6bb Compare October 7, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants