Skip to content

fix(tm1): keep unsafe options within their command - #944

Open
deepujain wants to merge 1 commit into
NVIDIA:mainfrom
deepujain:fix/927-command-boundaries
Open

deepujain wants to merge 1 commit into
NVIDIA:mainfrom
deepujain:fix/927-command-boundaries

Conversation

@deepujain

Copy link
Copy Markdown
Contributor

Summary

A harmless git push example can inherit --force from a later Markdown block, placing the warning on the wrong line and consuming the real occurrence. Bound the affected git, curl, wget and permission-option matches to their command and Markdown example. The force push now reports its own location, and Bash/PowerShell continuations, quoted options and balanced multiline arguments remain detected.

The existing Markdown delimiter projection identifies which example owns a command. Bounded argument scanning retains shell substitutions and quoted arguments without letting a closing inline span or fence extend the match into prose.

Validation

  • uv run pytest tests/nodes/analyzers/test_tm1_command_boundaries.py -q: 83 passed, covering LF/CRLF, inline spans, list-indented fences, quoted arguments and long-input runtime bounds.
  • uv run make lint and uv run make format-check pass.
  • Offline CLI scans keep the harmless example clear and report the actual force push at line 8.
  • uv run make test-ci: 16,168 passed, 14 skipped, and 4 expected failures on Python 3.12.14.

Risk

The change affects command-option matching only. Windows was not run directly; its line endings and PowerShell continuation forms were exercised on macOS. Hosted CI and Docker smoke results are pending.

Fixes #927.

Stop option matches at command and Markdown example boundaries. Preserve
quoted arguments, supported continuations and balanced substitutions so
unsafe commands retain their own source locations.

Signed-off-by: Deepak Jain <deepujain@gmail.com>

@rng1995 rng1995 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.

[SkillSpector Review]

Value and readiness

The command-local bounds address #927's real location/match-consumption problem, and the continuation, inline-span and balanced-argument coverage is useful. One introduced detection gap needs correction before approval at 08e29f0fa7f6309b794dfd10a1e104d265b2e36d.

Material findings

Required — src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py:4881-4895. The fence limit comes from raw _OPTION_COMMAND_FENCE matches, independently of the Markdown ownership projection. It therefore treats a literal tilde run inside an open backtick fence as a command boundary:

```sh
curl --data 'first
~~~
second' --insecure https://example.com
```

This is one valid shell command: the middle lines are the quoted --data value, and --insecure still disables TLS verification. _markdown_shell_text correctly leaves the tilde run literal because its marker kind does not close the active backtick fence. The separate raw fence list nevertheless clips the candidate at that line, before the closing quote and unsafe option; the subsequent bounded regex cannot see --insecure, and there is no later curl prefix to recover it. The previous curl rule retained this candidate. Only use fence boundaries proven by the existing Markdown ownership/context (including marker kind and opening length), and add regressions combining balanced multiline arguments with non-closing fence-shaped data. Keep the actual-closing-fence/unclosed-quote regression as well. The same issue applies to shorter same-kind runs inside longer fences.

PIC tradeoffs

Bounding options to their command is appropriate; uncertain or literal Markdown marker text must not discard an otherwise retained unsafe-option match.

Verification and gaps

Read the full two-file diff, pinned command scanner/Markdown projection, all 138 lines of new tests, original issue and empty review/thread history. Traced the example through the prefix, projection, fence limit and bounded match without executing contributor code. Also checked the static-runner/security-view path: ordinary ASCII normalization does not remove these markers or line breaks. All six hosted checks passed on this head; the refreshed mergeability was temporarily UNKNOWN. No contributor tests, imports, builds or scanner runs were executed. The finding is a source-level TM1 candidate-loss proof, not a claim of an executed scan or complete SAFE verdict.

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.

TM1 borrows unsafe flags from later Markdown command examples

2 participants