Skip to content

Merge updates from upstream and integrate local security report optimizations - #163

Open
hbui290 wants to merge 8 commits into
NVIDIA:mainfrom
hbui290:main
Open

Merge updates from upstream and integrate local security report optimizations#163
hbui290 wants to merge 8 commits into
NVIDIA:mainfrom
hbui290:main

Conversation

@hbui290

@hbui290 hbui290 commented Jun 23, 2026

Copy link
Copy Markdown

This Pull Request merges the upstream changes from NVIDIA/SkillSpector into the codebase and integrates custom enhancements:

  1. Resolved Merge Conflicts: Handled conflicts across core files like llm_analyzer_base.py, report.py, meta_analyzer.py, build_context.py, and test suites.
  2. Custom Scoring Multiplier: Ensured the 1.3x risk scoring multiplier is correctly applied only to findings detected within executable components (rather than scaling the entire report score), combined with the upstream diminishing returns logic.
  3. Optimizations: Addressed edge-case issues in mock initializations during unit tests.

All 725+ tests pass successfully.

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

Verdict: Request changes (strong) — a 19-file upstream-merge grab-bag with some genuinely good ideas (SQLite cache, block-aware chunking, retry/backoff), but several blocking problems including a fail-open detection regression.

Blocking

  1. Detection-coverage regression (fail-open). _SKIP_DIRS in src/skillspector/nodes/build_context.py is expanded to include docs, doc, tests, test, spec, specs, build, dist, out, target, images, media, brand (~L775-808), now matched against the relative path (~L884). A malicious skill can hide a payload in e.g. tests/ or docs/ and evade scanning entirely. For a security scanner this is a serious regression — these must not be skipped by default. (The bundled BAO_CAO_TOI_UU_HOA.md even frames this as fixing a "file omission" bug, but the net effect is more directories silently skipped.)
  2. Committed personal/local artifacts. BAO_CAO_TOI_UU_HOA.md and HUONG_DAN_SU_DUNG.md are non-project docs containing dev-machine absolute paths (file:///Users/winston/.gemini/antigravity-ide/scratch/SkillSpector/...). These should not land in the repo.
  3. load_dotenv() at package import in src/skillspector/__init__.py (~L135-136): an import-time side effect that auto-loads .env from CWD. Risky for a tool run against untrusted skill directories — it could ingest a scanned skill's .env. Move it behind the CLI entrypoint, not package import.
  4. Production code detecting test mocks. LLMAnalyzerBase._original_run_batches = LLMAnalyzerBase.run_batches (llm_analyzer_base.py ~L673) plus the is_patched mock-introspection branch in semantic_security_discovery.py (~L730-756) couple prod behavior to the test harness — a code smell that can mask real regressions.
  5. Scope / conflicts. Bundles unrelated changes and overlaps/conflicts with #157 (binary skip), #159/#164/#116 (zip-slip/ingest, _download_file/_extract_zip), and #142/#153/#122 (_compute_risk_score / scoring + report.py). Please split into focused PRs and rebase.

The good ideas (persistent SQLite cache, smarter chunk_file_by_lines, deterministic finding ordering, retry/backoff) are worth landing — just on their own focused PRs and without the _SKIP_DIRS expansion, the committed local docs, and the import-time load_dotenv().

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

[Automated SkillSpector Review]

Changes requested. This 19-file merge bundle introduces multiple blocking security and maintainability regressions:

  1. The expanded file exclusions skip docs, tests, spec, build/output directories, gitignored files, and lockfiles. A malicious skill can place executable or instruction payloads under those ordinary names and evade analysis entirely. Restore security-oriented traversal that scans untrusted content by default; exclusions must be narrowly justified and tested against bypasses.
  2. Package import calls load_dotenv() against the process working directory. When SkillSpector runs on untrusted input, importing the package can ingest attacker-controlled environment values. Load configuration only at an explicit trusted CLI boundary.
  3. Remove the two personal/local documentation artifacts containing developer-machine file:///Users/... links.
  4. Remove production behavior that introspects whether run_batches was replaced by test mocks. Production execution must not branch on test-harness patching.
  5. Split and rebase the unrelated cache, archive handling, AST, scoring, traversal, and LLM changes. The current head is reported as conflicted and is too broad to validate as one security-sensitive change.

These blockers remain present at the current head; GitHub reports no completed check runs.

".github",
".idea",
".settings",
"docs",

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.

This exclusion creates a fail-open scanner: untrusted skills can place executable code or hidden model instructions under docs/, tests/, spec/, or the other newly skipped directories and evade every downstream analyzer. Keep traversal inclusive by default and add regression tests proving payloads in ordinary nested directories remain scanned.

"""Skillspector v2 LangGraph workflow package."""

from dotenv import load_dotenv
load_dotenv()

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.

Do not load .env during package import. Because the scanner can be launched from or against an untrusted skill directory, this can ingest attacker-controlled provider, endpoint, or credential-related values before input validation. Move dotenv loading to an explicit trusted CLI startup path.

@rng1995

rng1995 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

@hbui290 - Please address review comments and resolve merge conflicts.

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