From ab1c585737ee9c636491baa2af4fd6c0a0e1d214 Mon Sep 17 00:00:00 2001 From: Narendran Raghavan Date: Tue, 6 Oct 2026 13:11:07 -0700 Subject: [PATCH 1/2] fix(analyzer): keep Python and Perl syntax from exhausting the shell parser Valid host-language source could make has_bounded_parse_exhaustion report static_parse_limit. The file was then recorded as partially inspected and every SKILL.md reference to it became a HIGH AE1 finding. These triggers are new in 2.12.0: - Perl `eval BLOCK` (`eval { require $f; 1 } or die $@;`) reached the shell `eval` command-string reconstruction, and the dynamic `$f` operand made the reparsed string unresolvable. - In Python, a string, docstring or comment containing a Markdown fence, an unpaired backtick or an apostrophe opened a shell quote or command substitution that ran to the end of the view. Any file with more than 4096 characters after it was partial. - Python code that uses a shell wrapper name (`signal.alarm(timeout)`, `if (timeout > 0 ...`) was reparsed as a `timeout` command string, and a `$0` in a comment was treated as a runtime-selected command whose operands ran on into later code. Recognize Perl `eval BLOCK` in .pl files: a clause-initial `eval` followed by `{` is not a string reparse. `{` already starts a new clause, so the statements in the block are still scanned. String eval (`eval $code`, `eval "..."`, `eval qq{...}`) and shell `eval` are unchanged. For a complete .py module that ast.parse accepts (no non-Python shebang, no lone CR, within the AST size bound), compute string, f-string and comment token spans with tokenize and verify them against the source. Valid Python code outside those tokens contains no shell quote or expansion opener, so: - an unresolved shell word is charged only up to the first code-level word break after it, which ends the string or comment token that opened the quote, instead of the end of the view; - a command-string clause (eval, sh -c, wrappers) that begins in Python code is not reparsed as shell; - a runtime-selected command inside a comment is rechecked against that comment alone, because no shell ever receives a comment. Spans are computed lazily, only when one of these checks would otherwise fail closed, and they honor the runtime deadline. Fragments, malformed source, shell-shebang polyglots, a string or comment that itself exceeds the command-word budget, the destructive rm, brace-expansion and printf checks, runtime commands built from strings, and all shell and Markdown behavior keep their existing conservative results. Rule findings are unchanged. Refs #694 Signed-off-by: Narendran Raghavan Co-Authored-By: Claude Opus 5.5 --- docs/ANALYSIS_RESOURCE_BOUNDS.md | 21 ++ .../analyzers/static_patterns_tool_misuse.py | 303 ++++++++++++++-- .../test_host_language_shell_ownership.py | 341 ++++++++++++++++++ 3 files changed, 642 insertions(+), 23 deletions(-) create mode 100644 tests/nodes/analyzers/test_host_language_shell_ownership.py diff --git a/docs/ANALYSIS_RESOURCE_BOUNDS.md b/docs/ANALYSIS_RESOURCE_BOUNDS.md index a7b776564..279c960d5 100644 --- a/docs/ANALYSIS_RESOURCE_BOUNDS.md +++ b/docs/ANALYSIS_RESOURCE_BOUNDS.md @@ -149,6 +149,27 @@ legacy package separators, incomplete fragments, and real parser limits remain on the conservative analysis path. A complex helper may therefore still need its particular ledger reason and expression reviewed. +Perl `eval BLOCK` (for example `eval { require $module; 1 } or die $@;`) traps +exceptions in already-compiled code and is not treated as a shell `eval` of a +string. Its statements are still scanned. String forms such as `eval $code` +and `eval "..."` remain on the conservative path. + +### Python strings and comments + +For a complete `.py` module that the Python parser accepts, each string literal +(including f-strings) and comment owns its bytes. A shell quote or backtick that +is left open inside one of them, such as a Markdown fence in a string or an +apostrophe in a comment, is charged only up to the end of that token, never to +the code that follows. Python code that happens to use a shell wrapper name, as +in `signal.alarm(timeout)`, is not reparsed as a shell command string. A +runtime-selected command named in a comment takes operands only from that +comment. + +Literal payloads remain visible to every security check. A single string or +comment that itself holds more unresolved shell text than the parser bound, +invalid or fragmentary Python, and source with a non-Python shebang keep the +conservative result. + ## Structured skill data AISOP/AISP structured extraction consumes the already-bounded cache and shares the enclosing diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 183288cd5..86adfaf9b 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -25,15 +25,17 @@ from __future__ import annotations import ast +import io import re import sys +import tokenize from bisect import bisect_right from collections.abc import Callable, Iterator from dataclasses import dataclass from skillspector.logging_config import get_logger from skillspector.models import AnalyzerFinding, Location, Severity -from skillspector.python_ast import parse_python_source +from skillspector.python_ast import MAX_PYTHON_AST_SOURCE_CHARS, parse_python_source from skillspector.security_reconstruction import validated_json_string_spans from skillspector.state import AnalyzerNodeResponse, SkillspectorState @@ -104,6 +106,28 @@ ) _PERL_QUOTE_OPERATOR_RE = re.compile(r"\b(?:q[qwxr]?|m|s|tr|y)(?:\s+\S|[^\w\s])") _PERL_AMBIGUOUS_SIGIL_RE = re.compile(r"[$@%&*]\s*+[{#'\"`]") +# Perl's ``eval BLOCK`` runs already-compiled statements and traps exceptions. +# Only ``eval EXPR`` (a string, variable, or quote operator) reparses source. +_PERL_EVAL_BLOCK_RE = re.compile(r"\s*+\{") +# Outside string and comment tokens, valid Python ends a shell word at +# whitespace or a shell control character. A backslash-newline is a line +# continuation in both languages, so it does not end the word. +_PYTHON_SHELL_WORD_BREAK_RE = re.compile(r"(?]") +# The tokenizer and the compiler disagree about a carriage return that does +# not start CRLF, so such source never acquires token ownership. +_LONE_CARRIAGE_RETURN_RE = re.compile(r"\r(?!\n)") +# A shebang that names another interpreter (for example a shell polyglot) +# makes the Python host ambiguous. Without a shebang, ``.py`` source is Python. +_PYTHON_SHEBANG_RE = re.compile( + r"#![ \t]*+(?:\S*/)?(?:env[ \t]++(?:-\S*[ \t]++)*(?:\S*/)?)?" + r"(?:python[0-9.]*|pypy[0-9.]*|uv)(?=[ \t\r\n]|$)" +) +_PYTHON_TEMPLATE_STRING_STARTS = frozenset( + {tokenize.FSTRING_START, getattr(tokenize, "TSTRING_START", tokenize.FSTRING_START)} +) +_PYTHON_TEMPLATE_STRING_ENDS = frozenset( + {tokenize.FSTRING_END, getattr(tokenize, "TSTRING_END", tokenize.FSTRING_END)} +) _PRINTF_FORMAT_CONVERSION_RE = re.compile(r"%[-+ #0-9.*']*[A-Za-z%]") _SHELL_ROOT_TARGET_ESCAPE_RE = re.compile( r"\\(?:[/~*?]|x(?:2[fF]|7[eE]|2[aA]|3[fF])|" @@ -1930,9 +1954,17 @@ def _has_shell_command_word_exhaustion( *, structural_quote_closers: set[int] | None = None, structural_quote_openers: set[int] | None = None, + python_source: _PythonSourceOwnership | None = None, + perl_eval_blocks: bool = False, _command_string_depth: int = 0, ) -> bool: - """Find candidate command words whose deterministic parse hit a safety bound.""" + """Find candidate command words whose deterministic parse hit a safety bound. + + ``python_source`` supplies proven Python string and comment ownership, so + host code is never charged to a shell word or command string. + ``perl_eval_blocks`` recognizes Perl's exception-trapping ``eval BLOCK``, + which reparses no string. + """ parsed_through = 0 # Completed words own closing quotes, never executable expansion starts. # Inner commands remain independent candidates; never suppress their bodies. @@ -1942,6 +1974,7 @@ def _has_shell_command_word_exhaustion( backtick_end_cache: dict[int, int | None] = {} may_have_destructive_outer_operands = _may_have_destructive_outer_operands(content) json_openers = sorted(structural_quote_openers or ()) + complete_comments: set[tuple[int, int]] = set() for candidate in _SHELL_COMMAND_WORD_START_RE.finditer(content): check_runtime() start = candidate.start() @@ -2007,6 +2040,10 @@ def _has_shell_command_word_exhaustion( unresolved_end = ( json_openers[next_json] if next_json < len(json_openers) else len(content) ) + if unresolved_end - start > _SHELL_COMMAND_WORD_CHARS and python_source is not None: + # Likewise, a host string or comment token owns its bytes: a + # shell quote it opens cannot continue into later host code. + unresolved_end = min(unresolved_end, python_source.word_end(start)) if unresolved_end - start > _SHELL_COMMAND_WORD_CHARS: return True continue @@ -2049,30 +2086,38 @@ def _has_shell_command_word_exhaustion( continue if parsed.limited: return True - if parsed.dynamic and may_have_destructive_outer_operands: - tokens, command_end, exhausted = _bounded_shell_tokens( + if ( + parsed.dynamic + and may_have_destructive_outer_operands + and _has_runtime_command_operand_exhaustion( content, start, parsed.end, - check_runtime=check_runtime, + check_runtime, ) - if exhausted and command_end - start < _ROOT_GLOB_COMMAND_CHARS: - # Earlier prose (for example "row-first") cannot supply this - # command's operands. Refine only this candidate's exhausted - # span, including nested commands inside the executable word. - # Hitting the span bound always remains partial, including when - # real operands occur beyond that bound. This rescan is bounded - # by the same constant as the tokenizer. - check_runtime() - exhausted = _may_have_destructive_outer_operands(content[start:command_end]) - if ( - exhausted - or _has_unsupported_brace_expansion(tokens) - or _has_destructive_root_glob(tokens) - or _has_destructive_root_path(tokens) - ): + ): + comment = python_source.comment(start) if python_source is not None else None + if comment is None: return True - for command_string in _shell_command_strings(content, check_runtime): + # A Python comment is prose that no shell receives, so a command + # named in it can take operands only from the same comment, never + # from later host code. Recheck that comment as standalone shell + # text. Strings are not confined: concatenation can supply their + # runtime operands. + if comment not in complete_comments: + if _has_shell_command_word_exhaustion( + content[comment[0] : comment[1]], + check_runtime, + _command_string_depth=_command_string_depth, + ): + return True + complete_comments.add(comment) + for command_string in _shell_command_strings( + content, + check_runtime, + perl_eval_blocks=perl_eval_blocks, + python_source=python_source, + ): if command_string is None or _command_string_depth >= 8: return True if _has_shell_command_word_exhaustion( @@ -2084,9 +2129,42 @@ def _has_shell_command_word_exhaustion( return False +def _has_runtime_command_operand_exhaustion( + content: str, + start: int, + word_end: int, + check_runtime: Callable[[], None], +) -> bool: + """Return whether a runtime-selected command's operands are undecidable.""" + tokens, command_end, exhausted = _bounded_shell_tokens( + content, + start, + word_end, + check_runtime=check_runtime, + ) + if exhausted and command_end - start < _ROOT_GLOB_COMMAND_CHARS: + # Earlier prose (for example "row-first") cannot supply this + # command's operands. Refine only this candidate's exhausted + # span, including nested commands inside the executable word. + # Hitting the span bound always remains partial, including when + # real operands occur beyond that bound. This rescan is bounded + # by the same constant as the tokenizer. + check_runtime() + exhausted = _may_have_destructive_outer_operands(content[start:command_end]) + return ( + exhausted + or _has_unsupported_brace_expansion(tokens) + or _has_destructive_root_glob(tokens) + or _has_destructive_root_path(tokens) + ) + + def _shell_command_strings( content: str, check_runtime: Callable[[], None], + *, + perl_eval_blocks: bool = False, + python_source: _PythonSourceOwnership | None = None, ) -> Iterator[str | None]: """Yield bounded strings reparsed by ``eval`` or a shell ``-c`` wrapper.""" for clause_start in _shell_clause_starts(content, check_runtime): @@ -2095,9 +2173,20 @@ def _shell_command_strings( content, clause_start, check_runtime, + perl_eval_blocks=perl_eval_blocks, ) - if recognized: - yield command_string + if not recognized: + continue + if python_source is not None: + word_start = clause_start + while word_start < len(content) and content[word_start].isspace(): + word_start += 1 + if python_source.is_code(word_start): + # A clause that begins in Python code is Python, not shell: + # ``signal.alarm(timeout)`` names no ``timeout`` command. + # Shell text can only be reparsed from a string literal. + continue + yield command_string def _shell_clause_starts( @@ -2203,6 +2292,8 @@ def _command_string_from_clause( content: str, start: int, check_runtime: Callable[[], None], + *, + perl_eval_blocks: bool = False, ) -> tuple[bool, str | None]: """Resolve a bounded wrapper chain to an ``eval`` or shell command string.""" cursor = start @@ -2249,6 +2340,15 @@ def resolved_command_string(word: str | None, limited: bool) -> str | None: if command in _SHELL_CLAUSE_PREFIX_WORDS: continue if command == "eval": + if ( + perl_eval_blocks + and word == "eval" + and _PERL_EVAL_BLOCK_RE.match(content, cursor) is not None + ): + # Perl ``eval BLOCK`` is exception handling, not a string + # reparse. ``{`` starts a new clause, so the statements in the + # block are still scanned as ordinary command candidates. + return False, None return True, _eval_command_string(content, cursor, check_runtime) if command in _SHELL_COMMAND_STRING_SHELLS: for _ in range(16): @@ -2443,6 +2543,154 @@ def _perl_literal_print_shell_text( return "".join(output) +def _python_literal_spans( + content: str, + check_runtime: Callable[[], None], +) -> tuple[list[int], list[int]] | None: + """Return outermost string and comment token spans of valid Python source. + + Return ``None`` unless the whole text is accepted by the Python parser, + so a fragment, malformed file, or shell-shebang polyglot cannot borrow + host ownership. Every span is checked against the exact source text. + """ + if ( + len(content) > MAX_PYTHON_AST_SOURCE_CHARS + or _LONE_CARRIAGE_RETURN_RE.search(content) is not None + or (content.startswith("#!") and _PYTHON_SHEBANG_RE.match(content) is None) + ): + return None + check_runtime() + try: + # The lenient tokenizer accepts bytes such as ``$`` and a backtick as + # operators. Requiring a module parse proves that every byte outside + # the spans below is Python syntax, never a shell quote or expansion. + ast.parse(content) + except (SyntaxError, ValueError, RecursionError): + return None + check_runtime() + line_starts = [0] + line_starts.extend(match.end() for match in re.finditer("\n", content)) + starts: list[int] = [] + ends: list[int] = [] + template_depth = 0 + template_start = 0 + + def offset(position: tuple[int, int]) -> int: + return line_starts[position[0] - 1] + position[1] + + try: + for index, token in enumerate(tokenize.generate_tokens(io.StringIO(content).readline)): + if index % 256 == 0: + check_runtime() + if token.type in _PYTHON_TEMPLATE_STRING_STARTS: + if template_depth == 0: + template_start = offset(token.start) + if not content.startswith(token.string, template_start): + return None + template_depth += 1 + elif token.type in _PYTHON_TEMPLATE_STRING_ENDS: + template_depth -= 1 + end = offset(token.end) + if template_depth < 0 or not content.endswith(token.string, 0, end): + return None + if template_depth == 0: + starts.append(template_start) + ends.append(end) + elif not template_depth and token.type in (tokenize.STRING, tokenize.COMMENT): + # Inside a replacement field, nested strings and comments + # belong to the enclosing f-string or t-string span. + start, end = offset(token.start), offset(token.end) + if content[start:end] != token.string: + return None + starts.append(start) + ends.append(end) + except (tokenize.TokenError, SyntaxError, ValueError, IndexError): + return None + if template_depth or any(ends[index] > starts[index + 1] for index in range(len(starts) - 1)): + return None + return starts, ends + + +class _PythonSourceOwnership: + """Lazily proven Python string and comment ownership for one source text. + + Python has no shell semantics. Shell text can occur only inside a Python + string, which owns its bytes, or as prose in a comment that no shell ever + receives. Python code between those tokens contains no shell quote or + expansion opener, so an in-phase shell word ends at the first code-level + whitespace or control character. A quote or substitution that is still + open there was opened inside an earlier string or comment token of the + same word, and that token owns the rest of it. + + The token spans are computed lazily, only for source that reaches one of + these decisions. Invalid or ambiguous source keeps every conservative bound. + """ + + def __init__(self, content: str, check_runtime: Callable[[], None]) -> None: + self._content = content + self._check_runtime = check_runtime + self._spans: tuple[list[int], list[int]] | None = None + self._spans_computed = False + self._cached_word_start = -1 + self._cached_word_end = -1 + + def _owned_spans(self) -> tuple[list[int], list[int]] | None: + if not self._spans_computed: + self._spans = _python_literal_spans(self._content, self._check_runtime) + self._spans_computed = True + return self._spans + + def _owner(self, position: int) -> tuple[int, int] | None: + spans = self._owned_spans() + if spans is None: + return None + starts, ends = spans + index = bisect_right(starts, position) - 1 + if index >= 0 and position < ends[index]: + return starts[index], ends[index] + return None + + def is_code(self, position: int) -> bool: + """Return whether a position is proven Python code, not a literal.""" + return self._owned_spans() is not None and self._owner(position) is None + + def comment(self, position: int) -> tuple[int, int] | None: + """Return the span of a proven Python comment containing a position.""" + owner = self._owner(position) + if owner is None or self._content[owner[0]] != "#": + return None + return owner + + def word_end(self, start: int) -> int: + """Return the end of the host region a shell word at ``start`` can own.""" + spans = self._owned_spans() + content = self._content + if spans is None: + return len(content) + # Candidates arrive in source order. No code-level break exists + # between a cached start and its result, so they share that result. + if self._cached_word_start <= start <= self._cached_word_end: + return self._cached_word_end + starts, ends = spans + cursor = start + while True: + self._check_runtime() + index = bisect_right(starts, cursor) - 1 + if index >= 0 and cursor < ends[index]: + cursor = ends[index] + segment_end = starts[index + 1] if index + 1 < len(starts) else len(content) + match = _PYTHON_SHELL_WORD_BREAK_RE.search(content, cursor, segment_end) + if match is not None: + result = match.start() + break + if segment_end == len(content): + result = segment_end + break + cursor = segment_end + self._cached_word_start, self._cached_word_end = start, result + return result + + def _skip_command_substitution( content: str, start: int, @@ -3697,19 +3945,28 @@ def has_bounded_parse_exhaustion( structural_quote_closers = None structural_quote_openers = None json_strings: list[tuple[int, int]] = [] + python_source = None if file_type == "perl" and complete_context: content = _perl_literal_print_shell_text(content, check_runtime) + if file_type == "python" and complete_context: + # Only a complete module can prove Python token ownership. A fragment + # may begin inside a string and would invert code and literal bytes. + python_source = _PythonSourceOwnership(content, check_runtime) if file_type == "markdown": if complete_context: json_strings = validated_json_string_spans(content, check_runtime) structural_quote_closers = {end - 1 for _, end in json_strings} structural_quote_openers = {start for start, _ in json_strings} content = _markdown_shell_text(content, check_runtime, complete_context=complete_context) + # Perl ``eval BLOCK`` is recognized from its clause-initial ``eval`` and the + # adjacent brace alone, so unlike quote ownership it needs no full context. if _has_shell_command_word_exhaustion( content, check_runtime, structural_quote_closers=structural_quote_closers, structural_quote_openers=structural_quote_openers, + python_source=python_source, + perl_eval_blocks=file_type == "perl", ): return True covered_until = 0 diff --git a/tests/nodes/analyzers/test_host_language_shell_ownership.py b/tests/nodes/analyzers/test_host_language_shell_ownership.py new file mode 100644 index 000000000..e86d63571 --- /dev/null +++ b/tests/nodes/analyzers/test_host_language_shell_ownership.py @@ -0,0 +1,341 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Valid Python and Perl syntax must not exhaust the bounded shell parser. + +Python string and comment tokens own their bytes, and Perl ``eval BLOCK`` is +exception handling rather than a string reparse. Neither may turn ordinary +host source into a ``static_parse_limit`` result, while genuinely unresolved +shell text keeps its conservative partial outcome. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from skillspector.graph import graph +from skillspector.inspection_ledger import LedgerOutcome, LedgerReason +from skillspector.nodes.analyzers import static_patterns_tool_misuse as tm +from skillspector.nodes.analyzers import static_runner + +# More than the 4096-character command-word budget of ordinary code after +# the construct under test, with no quotes, backticks or comments of its own. +_TAIL = "".join(f"value_{index} = {index}\n" for index in range(600)) +# A view-level prefilter only considers runtime-selected command operands +# when recursive/force option text and a path occur somewhere in the source. +_OPTION_TEXT = 'BUILD_ARGS = ["--recursive", "--force", "/tmp/build"]\n' + + +def _python(content: str, *, complete_context: bool = True) -> bool: + return tm.has_bounded_parse_exhaustion( + content, lambda: None, file_type="python", complete_context=complete_context + ) + + +def _perl(content: str, *, complete_context: bool = True) -> bool: + return tm.has_bounded_parse_exhaustion( + content, lambda: None, file_type="perl", complete_context=complete_context + ) + + +def _shell(content: str) -> bool: + return tm.has_bounded_parse_exhaustion(content, lambda: None, file_type="shell") + + +def _ledger(path: str, content: str) -> dict: + return static_runner.run_static_patterns_with_ledger( + {"components": [path], "file_cache": {path: content}}, [tm] + ) + + +# --- Perl eval BLOCK ------------------------------------------------------- + + +@pytest.mark.parametrize( + "statement", + [ + 'eval { require $module; 1 } or die "load failed: $@";', + "eval { require $module };", + "eval { $handler->(); 1 };", + "eval\n{ require $module; 1 };", + '{ local $^W = 0; eval { require $module; 1 } or die "load failed: $@"; }', + 'if (defined &Plugin::init) { eval { Plugin::init(); 1 } or warn "init: $@"; }', + ], +) +@pytest.mark.parametrize("complete_context", [True, False], ids=["complete", "fragment"]) +def test_perl_eval_block_is_not_a_shell_string_eval(statement: str, complete_context: bool) -> None: + content = "use strict;\nuse warnings;\n" + statement + "\n" + + assert _perl(content, complete_context=complete_context) is False + + +def test_perl_eval_block_clause_is_not_a_command_string() -> None: + content = "eval { require $module; 1 };\n" + + assert tm._command_string_from_clause(content, 0, lambda: None, perl_eval_blocks=True) == ( + False, + None, + ) + # Shell has no block form: the same bytes in a shell file stay partial. + assert tm._command_string_from_clause(content, 0, lambda: None) == (True, None) + assert _shell(content) is True + + +@pytest.mark.parametrize( + "statement", + [ + "eval $code;", + 'eval "$code";', + "eval qq{$code};", + 'eval "require $module; 1" or die;', + "EVAL { $code };", + "eval { require $module; 1 } or die; eval $code;", + ], +) +def test_perl_string_eval_keeps_conservative_parse_status(statement: str) -> None: + assert _perl(statement + "\n") is True + + +@pytest.mark.parametrize( + "statement", + [ + "eval { my $out = `$TOOL -rf /`; 1 };", + 'eval { qx(sh -c "$cmd") };', + "eval { rm -rf {/,{1..2}} };", + ], +) +def test_commands_inside_perl_eval_block_remain_scanned(statement: str) -> None: + assert _perl(statement + "\n") is True + + +def test_destructive_command_inside_perl_eval_block_keeps_tm1() -> None: + result = _ledger("scripts/helper.pl", 'eval { system("rm -rf /") } or warn "failed: $@";\n') + + assert any(finding.rule_id == "TM1" for finding in result["findings"]) + + +# --- Python string and comment ownership ----------------------------------- + + +_BENIGN_PYTHON = [ + pytest.param('FENCE = "\\n```\\n"\n', id="fence-in-double-quoted-string"), + pytest.param( + 'def render(lang):\n return [f"```{lang}", "body", "```"]\n', + id="fence-in-f-string", + ), + pytest.param( + 'def quote():\n """Wrap inline code in a ` character pair."""\n return ""\n', + id="docstring-unpaired-backtick", + ), + pytest.param( + "def token():\n '''Return the user's ``token``.'''\n return None\n", + id="docstring-apostrophe-and-double-backticks", + ), + pytest.param("# The prefix doesn't include the array name.\n", id="comment-apostrophe"), + pytest.param( + "# Patch the shared `client` module so helper()'s requests are intercepted.\n", + id="comment-backtick-and-apostrophe", + ), + pytest.param('assert "see `latest" not in "text"\n', id="string-single-backtick"), + pytest.param( + 'import re\nWORD = re.compile(r"(?:^|[\\s;&|`(/])(tool)(?=\\s|$)")\n', + id="raw-regex-character-class", + ), + pytest.param('print("done", end="`")\n', id="keyword-argument-backtick"), + pytest.param('PARTS = ("first `part"\n "second part")\n', id="implicit-concatenation"), + pytest.param( + "import signal\n\n\ndef arm(timeout):\n" + " if (\n timeout > 0\n ):\n signal.alarm(timeout)\n", + id="python-name-matching-shell-wrapper", + ), + pytest.param( + _OPTION_TEXT + '# Pass $0 = the launcher\'s path so the `dirname "$0"`\n' + "# branch runs (and `command -v` is skipped).\n", + id="comment-runtime-parameter", + ), +] + + +@pytest.mark.parametrize("source", _BENIGN_PYTHON) +def test_python_literals_and_comments_own_their_bytes(source: str) -> None: + content = source + _TAIL + + assert _python(content) is False + # The same bytes read as shell text remain conservative. + assert _shell(content) is True + + +@pytest.mark.parametrize("newline", ["\r\n"], ids=["crlf"]) +def test_python_ownership_preserves_crlf_offsets(newline: str) -> None: + content = ('"""Module `doc."""\nFENCE = "\\n```\\n" # it\'s fine\n' + _TAIL).replace( + "\n", newline + ) + + assert _python(content) is False + + +def test_python_ownership_without_trailing_newline_or_with_non_ascii_text() -> None: + assert _python('FENCE = "\\n```\\n"\n' + _TAIL.rstrip("\n")) is False + assert _python('LABEL = "caf\u00e9 \\n```\\n" # na\u00efve\n' + _TAIL) is False + + +def test_python_literal_spans_are_exact_source_offsets() -> None: + content = ( + "#!/usr/bin/env python3\r\n" + "x = f\"a{y!r:>{w}}b\" + rb'\\d' # c\u00e9\r\n" + "z = f\"{f'{q}'}\"\r\n" + '"""doc\r\nstring"""\r\n' + ) + + spans = tm._python_literal_spans(content, lambda: None) + + assert spans is not None + assert [content[start:end] for start, end in zip(*spans, strict=True)] == [ + "#!/usr/bin/env python3", + 'f"a{y!r:>{w}}b"', + "rb'\\d'", + "# c\u00e9", + "f\"{f'{q}'}\"", + '"""doc\r\nstring"""', + ] + + +@pytest.mark.parametrize( + "content", + [ + pytest.param('FENCE = "\\n```\\n"\nnot python $(\n', id="invalid-python"), + pytest.param('#!/bin/sh\nFENCE = "\\n```\\n"\n', id="shell-shebang"), + pytest.param('FENCE = "\\n```\\n"\rvalue = 1\n', id="lone-carriage-return"), + pytest.param('FENCE = "\\n```\\n"\n\x00\n', id="nul-byte"), + ], +) +def test_unproven_python_source_has_no_token_ownership(content: str) -> None: + assert tm._python_literal_spans(content, lambda: None) is None + assert _python(content + _TAIL) is True + + +def test_python_ownership_requires_complete_context() -> None: + # A fragment can start inside a string and invert code and literal bytes. + assert _python('FENCE = "\\n```\\n"\n' + _TAIL, complete_context=False) is True + + +@pytest.mark.parametrize( + "source", + [ + pytest.param('COMMAND = "echo `' + "word " * 900 + '"\n', id="single-literal"), + pytest.param('COMMAND = "echo " "`' + "word " * 900 + '"\n', id="adjacent-literal"), + pytest.param("# doesn't " + "word " * 900 + "\n", id="single-comment"), + ], +) +def test_long_unresolved_shell_inside_one_python_token_stays_partial(source: str) -> None: + assert _python(source + "value = 1\n") is True + + +def test_runtime_command_with_destructive_operands_in_python_comment_stays_partial() -> None: + assert _python(_OPTION_TEXT + "# $TOOL -rf /\nvalue = 1\n" + _TAIL) is True + + +def test_shell_file_with_unmatched_backtick_and_long_tail_stays_partial() -> None: + content = "#!/bin/sh\necho `date\n" + "echo ordinary line\n" * 300 + + assert _shell(content) is True + result = _ledger("scripts/run.sh", content) + assert result["inspection_ledger"][0]["outcome"] is LedgerOutcome.PARTIAL + assert result["inspection_ledger"][0]["reason_code"] is LedgerReason.STATIC_PARSE_LIMIT + + +def test_python_token_ownership_honors_runtime_checks() -> None: + content = 'FENCE = "\\n```\\n"\n' + "".join( + f"value_{index} = 'x' # note\n" for index in range(5000) + ) + checks = 0 + + def check_runtime() -> None: + nonlocal checks + checks += 1 + # The first checks bracket the module parse; later ones must come + # from the bounded token scan itself. + if checks >= 4: + raise TimeoutError("test runtime bound") + + with pytest.raises(TimeoutError, match="test runtime bound"): + tm._python_literal_spans(content, check_runtime) + + +def test_python_ownership_keeps_tool_misuse_detection() -> None: + content = ( + 'FENCE = "\\n```\\n"\nimport subprocess\nsubprocess.run("rm -rf /", shell=True)\n' + _TAIL + ) + + result = _ledger("scripts/report.py", content) + + assert any(finding.rule_id == "TM1" for finding in result["findings"]) + assert result["inspection_ledger"][0]["outcome"] is LedgerOutcome.COMPLETED + + +# --- Whole-scan accounting ------------------------------------------------- + + +def _write_bundle(root: Path, files: dict[str, str]) -> None: + for relative_path, content in files.items(): + path = root / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + + +_MANIFEST = ( + "---\nname: report-builder\ndescription: Build a Markdown report from local results.\n" + "---\n\nRun `scripts/report.py` to build the report.\n\n" + "Run `scripts/load.pl` to load the optional plugin.\n" +) +_REPORT_SCRIPT = ( + '"""Render results as Markdown (see the `results` directory)."""\n\n' + "# The renderer doesn't escape table cells.\n" + 'FENCE = "\\n```\\n"\n\n\n' + "def render(lang, body):\n" + ' return f"```{lang}\\n{body}" + FENCE\n' + _TAIL +) +_LOADER_SCRIPT = ( + "#!/usr/bin/env perl\nuse strict;\nuse warnings;\n" + 'my $module = shift @ARGV or die "usage: load.pl ";\n' + '{ local $^W = 0; eval { require $module; 1 } or die "load failed: $@"; }\n' +) + + +def test_referenced_python_and_perl_helpers_are_complete_without_ae1(tmp_path: Path) -> None: + _write_bundle( + tmp_path, + { + "SKILL.md": _MANIFEST, + "scripts/report.py": _REPORT_SCRIPT, + "scripts/load.pl": _LOADER_SCRIPT, + }, + ) + + result = graph.invoke({"input_path": str(tmp_path), "use_llm": False, "output_format": "json"}) + + assert result["analysis_completeness"]["status"] == "complete" + assert not any(finding.rule_id == "AE1" for finding in result["findings"]) + + +def test_referenced_python_helper_with_unresolved_shell_literal_keeps_ae1( + tmp_path: Path, +) -> None: + _write_bundle( + tmp_path, + { + "SKILL.md": _MANIFEST, + "scripts/report.py": 'COMMAND = "echo `' + "word " * 900 + '"\n' + _TAIL, + "scripts/load.pl": _LOADER_SCRIPT, + }, + ) + + result = graph.invoke({"input_path": str(tmp_path), "use_llm": False, "output_format": "json"}) + + assert result["analysis_completeness"]["status"] == "partial" + assert any( + finding.rule_id == "AE1" and finding.evidence.get("target_path") == "scripts/report.py" + for finding in result["findings"] + ) From 53e2619dafe55cca00adc1656ec09aa59ec6f8d9 Mon Sep 17 00:00:00 2001 From: Narendran Raghavan Date: Tue, 6 Oct 2026 13:33:05 -0700 Subject: [PATCH 2/2] fix(analyzer): keep the Python ownership parse from re-emitting warnings The module parse that proves Python token ownership repeated every compiler warning the runner's own parse had already reported, such as an invalid `"\d"` escape. Ordinary skill code printed duplicate warnings, and under `-W error::SyntaxWarning` the parse raised and silently dropped ownership, so the file fell back to static_parse_limit. Run that parse with warnings ignored. `warnings.catch_warnings` swaps the process-global filter list, and analyzer nodes run on concurrent graph worker threads, where two such blocks that exit out of order leave their `ignore` filter installed for the whole process. Serialize the block with a module lock. Pin the existing result for a module that starts with a UTF-8 BOM: the file cache decodes with utf-8, not utf-8-sig, so a leading U+FEFF fails the parse and ownership stays unproven. That gap belongs with a separate decoding fix. Refs #694 Signed-off-by: Narendran Raghavan Co-Authored-By: Claude Opus 5.5 --- .../analyzers/static_patterns_tool_misuse.py | 19 ++++++- .../test_host_language_shell_ownership.py | 56 +++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 86adfaf9b..fd2cbf162 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -29,9 +29,11 @@ import re import sys import tokenize +import warnings from bisect import bisect_right from collections.abc import Callable, Iterator from dataclasses import dataclass +from threading import Lock from skillspector.logging_config import get_logger from skillspector.models import AnalyzerFinding, Location, Severity @@ -122,6 +124,11 @@ r"#![ \t]*+(?:\S*/)?(?:env[ \t]++(?:-\S*[ \t]++)*(?:\S*/)?)?" r"(?:python[0-9.]*|pypy[0-9.]*|uv)(?=[ \t\r\n]|$)" ) +# ``warnings.catch_warnings`` swaps the process-global filter list, and analyzer +# nodes run on concurrent graph worker threads. Two such blocks that exit out of +# order can leave their ``ignore`` filter installed for the whole process, so +# the ownership parse serializes its block. +_PYTHON_OWNERSHIP_PARSE_LOCK = Lock() _PYTHON_TEMPLATE_STRING_STARTS = frozenset( {tokenize.FSTRING_START, getattr(tokenize, "TSTRING_START", tokenize.FSTRING_START)} ) @@ -2553,6 +2560,10 @@ def _python_literal_spans( so a fragment, malformed file, or shell-shebang polyglot cannot borrow host ownership. Every span is checked against the exact source text. """ + # A leading U+FEFF byte-order mark fails the module parse below and keeps + # every conservative bound. The file cache decodes with ``utf-8``, not + # ``utf-8-sig``, so the AST analyzers already report such a file as a + # syntax error; stripping the mark belongs with that decoding fix. if ( len(content) > MAX_PYTHON_AST_SOURCE_CHARS or _LONE_CARRIAGE_RETURN_RE.search(content) is not None @@ -2564,7 +2575,13 @@ def _python_literal_spans( # The lenient tokenizer accepts bytes such as ``$`` and a backtick as # operators. Requiring a module parse proves that every byte outside # the spans below is Python syntax, never a shell quote or expansion. - ast.parse(content) + # The scanned module's own parse already reports its compiler warnings + # (for example an invalid ``"\d"`` escape). Repeating them here would + # duplicate that output, and ``-W error`` would turn them into a + # ``SyntaxError`` that silently drops ownership. + with _PYTHON_OWNERSHIP_PARSE_LOCK, warnings.catch_warnings(): + warnings.simplefilter("ignore") + ast.parse(content) except (SyntaxError, ValueError, RecursionError): return None check_runtime() diff --git a/tests/nodes/analyzers/test_host_language_shell_ownership.py b/tests/nodes/analyzers/test_host_language_shell_ownership.py index e86d63571..2c770d8dd 100644 --- a/tests/nodes/analyzers/test_host_language_shell_ownership.py +++ b/tests/nodes/analyzers/test_host_language_shell_ownership.py @@ -11,6 +11,8 @@ from __future__ import annotations +import threading +import warnings from pathlib import Path import pytest @@ -26,6 +28,9 @@ # A view-level prefilter only considers runtime-selected command operands # when recursive/force option text and a path occur somewhere in the source. _OPTION_TEXT = 'BUILD_ARGS = ["--recursive", "--force", "/tmp/build"]\n' +# Read as shell text, the fence backticks open a command substitution that the +# tail exhausts. Only proven Python token ownership makes them literal. +_FENCE_SOURCE = 'FENCE = "\\n```\\n"\n' + _TAIL def _python(content: str, *, complete_context: bool = True) -> bool: @@ -221,6 +226,57 @@ def test_python_ownership_requires_complete_context() -> None: assert _python('FENCE = "\\n```\\n"\n' + _TAIL, complete_context=False) is True +def test_bom_prefixed_python_keeps_conservative_result() -> None: + # The file cache decodes with utf-8, not utf-8-sig, so a leading U+FEFF is + # not Python syntax. Ownership stays unproven until that decoding changes. + content = "\ufeff" + _FENCE_SOURCE + + assert tm._python_literal_spans(content, lambda: None) is None + assert _python(content) is True + + +# A valid module whose invalid escape makes the compiler emit a SyntaxWarning. +_WARNING_SOURCE = 'PATTERN = "\\d+"\n' + _FENCE_SOURCE + + +def test_python_ownership_parse_emits_no_warnings() -> None: + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + filters = list(warnings.filters) + + assert _python(_WARNING_SOURCE) is False + assert list(warnings.filters) == filters + + assert [str(warning.message) for warning in caught] == [] + + +@pytest.mark.filterwarnings("error::SyntaxWarning") +def test_python_ownership_survives_syntax_warnings_as_errors() -> None: + assert _python(_WARNING_SOURCE) is False + + +def test_concurrent_ownership_parses_restore_warning_filters() -> None: + # Analyzer nodes run on graph worker threads. Interleaved catch_warnings + # exits must not leave the ownership parse's ignore filter installed. + filters = list(warnings.filters) + barrier = threading.Barrier(8) + proven: list[bool] = [] + + def parse() -> None: + barrier.wait() + for _ in range(10): + proven.append(tm._python_literal_spans(_WARNING_SOURCE, lambda: None) is not None) + + threads = [threading.Thread(target=parse) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert proven == [True] * 80 + assert list(warnings.filters) == filters + + @pytest.mark.parametrize( "source", [