Skip to content

fix(security): scan .sh files for dependency confusion in pre-commit …#2241

Merged
imran-siddique merged 1 commit into
mainfrom
fix/2207-shell-script-dep-confusion
May 14, 2026
Merged

fix(security): scan .sh files for dependency confusion in pre-commit …#2241
imran-siddique merged 1 commit into
mainfrom
fix/2207-shell-script-dep-confusion

Conversation

@Ricky-G
Copy link
Copy Markdown
Contributor

@Ricky-G Ricky-G commented May 13, 2026

Closes #2207

Description

Pre-commit mode of scripts/check_dependency_confusion.py previously filtered staged files by extension and excluded .sh/.bash, so real pip install commands inside shell scripts (e.g. agent-governance-python/agent-os/modules/control-plane/build_and_publish.sh, .../scak/build_and_publish.sh) were never scanned for dependency-confusion risks.

This PR closes that gap:

  • Includes shell scripts in pre-commit scanning. Adds .sh and .bash to the staged-file extension filter in main().
  • Avoids false positives in shell scripts. In check_file(), pip install matches that fall inside a shell comment (#) or an echo/printf invocation are skipped. The check is scoped to the current shell command segment (split on ;, &&, ||, |), so a real install after a separator (e.g. echo done; pip install foo) is still flagged.
  • Preserves existing behavior for other file types. The new suppression is gated to .sh/.bash, so .md, .py, .txt, .ipynb scanning is unchanged (e.g. # Install: pip install <unregistered> in markdown is still flagged).
  • Adds twine to REGISTERED_PACKAGES since the now-scanned existing build_and_publish.sh scripts use it and it is a legitimate PyPI package.
  • Regression tests under tests/ci/test_check_dependency_confusion.py cover: real pip install <unregistered> in .sh is flagged; echo "pip install <unregistered>" is not; printf "... pip install <unregistered>" is not; full-line and inline # comments are not; known package (pydantic) in .sh is not; echo done; pip install <unregistered> (segment edge case) is flagged; markdown # lines still flagged; .txt behavior unchanged; pre-commit extension filter sanity checks for .sh/.bash and existing .py/.md/.txt/.ipynb.

Out of scope (intentional, per issue): Dockerfile support, heredoc bodies, and multi-line pip install \ continuations. These are tracked as follow-ups.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature
  • Breaking change
  • Documentation update
  • Maintenance
  • Security fix

Package(s) Affected

  • agent-os-kernel
  • agent-mesh
  • agent-runtime
  • agent-sre
  • agent-governance
  • docs / root (scripts/ + tests/)

Checklist

  • My code follows the project style guidelines (ruff check passes)
  • I have added tests that prove my fix works (15 regression tests)
  • All new and existing tests pass (pytest tests/ci/test_check_dependency_confusion.py — 15/15)
  • I have updated documentation as needed (no doc changes required; behavior of documented usage unchanged)
  • I have signed the Microsoft CLA

Attribution & Prior Art

  • This contribution does not contain code copied or derived from other projects without attribution
  • Any external projects that inspired this design are credited in code comments or documentation
  • N/A — this is a bug fix to an existing script

Prior art / related projects: None.

AI Assistance

  • I can explain every meaningful change in this PR
  • I have run tests and verification appropriate for this change
  • No part of this PR was autonomously submitted by an AI agent without my review
  • I have not used AI to generate review comments on others' PRs

GitHub Copilot was used to scaffold the regression tests and the segment-based suppression logic; all output was reviewed, tested, and validated against the issue requirements.

IP, Patents, and Licensing

  • This contribution does not implement patent-pending or patent-encumbered techniques
  • This contribution does not require an NDA or licensing agreement to understand or use
  • Any AI tools used have terms compatible with the MIT License

Related Issues

Closes #2207

Follow-ups

  • Dockerfile (FROM/RUN pip install) scanning in pre-commit mode
  • Shell heredoc body suppression (<<EOF ... EOF)
  • Multi-line pip install \ continuation parsing

@Ricky-G Ricky-G requested a review from imran-siddique May 13, 2026 05:55
@github-actions github-actions Bot added the tests label May 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

🤖 AI Agent: test-generator — `scripts/check_dependency_confusion.py`

scripts/check_dependency_confusion.py

  • test_sh_multiline_pip_install_continuation -- Validate handling of multi-line pip install \ continuations in .sh files.
  • test_dockerfile_pip_install -- Ensure pip install commands in Dockerfile RUN instructions are flagged.
  • test_sh_heredoc_pip_install -- Test suppression of pip install commands inside shell heredoc bodies (<<EOF ... EOF).

tests/ci/test_check_dependency_confusion.py

  • test_sh_multiline_pip_install_continuation -- Add regression test for multi-line pip install \ continuation parsing.
  • test_dockerfile_pip_install -- Add test for Dockerfile RUN pip install scanning.
  • test_sh_heredoc_pip_install -- Add test for heredoc body suppression in .sh files.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

🤖 AI Agent: security-scanner — View details

No security issues found.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

🤖 AI Agent: docs-sync-checker — Docs Sync

Docs Sync

  • scripts/check_dependency_confusion.py -- check_file() and main() have been updated, but no docstrings were added or updated to reflect the new behavior for .sh/.bash files.
  • README.md -- if it documents the usage or behavior of scripts/check_dependency_confusion.py, it may need updates to reflect the inclusion of .sh/.bash files in pre-commit scanning and the new suppression logic.
  • CHANGELOG.md -- missing entry for the behavioral change in scripts/check_dependency_confusion.py to include .sh/.bash files in pre-commit scanning and suppress false positives.

@github-actions github-actions Bot added the size/M Medium PR (< 200 lines) label May 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

🤖 AI Agent: breaking-change-detector — API Compatibility

API Compatibility

No breaking changes detected.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

🤖 AI Agent: code-reviewer — View details

TL;DR: 0 blockers, 0 warnings. Clean and robust security fix.

# Sev Issue Where

No action items.

@github-actions
Copy link
Copy Markdown

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential NONE
Overall MEDIUM

Automated check by AGT Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label May 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

PR Review Summary

Check Status Details
🔍 Code Review ✅ Passed No issues found
🛡️ Security Scan ✅ Passed No issues found
🔄 Breaking Changes ✅ Passed No issues found
📝 Docs Sync ✅ Completed Analysis complete
🧪 Test Coverage ✅ Completed Analysis complete

Verdict: ✅ Ready for human review

@Ricky-G Ricky-G removed the needs-review:MEDIUM Contributor check flagged MEDIUM risk label May 13, 2026
@Ricky-G Ricky-G force-pushed the fix/2207-shell-script-dep-confusion branch from 2450e31 to edfa460 Compare May 13, 2026 05:58
…2207)

Pre-commit mode now includes .sh and .bash files when filtering staged files for dependency-confusion scanning. To avoid false positives in shell scripts, check_file() skips pip install matches that fall inside a comment (#) or an echo/printf invocation, scoped to the current shell command segment (split on ;, &&, ||, |) so real installs after a separator (e.g. 'echo done; pip install foo') are still flagged. Suppression is gated to .sh/.bash so behavior for .md/.py/.txt/.ipynb is unchanged. Adds twine to REGISTERED_PACKAGES (used by existing build_and_publish.sh scripts). Regression tests cover shell-script scanning behavior. Closes #2207.

Signed-off-by: Ricky Gummadi <[email protected]>
@Ricky-G Ricky-G force-pushed the fix/2207-shell-script-dep-confusion branch from 153463e to 39bfd41 Compare May 14, 2026 03:00
@imran-siddique imran-siddique merged commit 0f0e47b into main May 14, 2026
98 of 100 checks passed
@imran-siddique imran-siddique deleted the fix/2207-shell-script-dep-confusion branch May 14, 2026 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR (< 200 lines) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: dependency-confusion pre-commit path ignores shell scripts with pip install commands

2 participants