feat: add OSV.dev dependency vulnerability analyzer (--use-osv) - #135
Conversation
…et/localtunnel.me ngrok migrated off ngrok.io to ngrok-free.dev / ngrok.app, so exfil to a current ngrok endpoint slipped past the suspicious-domain checks. Add the modern ngrok domains plus bore.pub, serveo.net and localtunnel.me to: - ContextExtractor.SUSPICIOUS_DOMAINS (Python string-literal URL classification) - tool_chaining_abuse_generic.yara / command_injection_generic.yara exfil dests - cross_skill_scanner exfil pattern list Add regression tests for the new domains (YARA true-positives + ContextExtractor). Co-authored-by: Cursor <cursoragent@cursor.com>
Skill packages are end-user applications, so unpinned dependencies (requests>=2 or a bare requests) let a later, potentially compromised release be pulled in at install time -- a supply-chain risk. Only the LLM prompt mentioned this previously; there was no deterministic check. Add StaticAnalyzer._check_dependency_pinning() which parses requirements*.txt files and a manifest metadata "dependencies" list, emitting SUPPLY_CHAIN_UNPINNED_DEPENDENCY (MEDIUM for open ranges/bare names, LOW for ==x.* wildcard pins). Skips when a lockfile (uv.lock/poetry.lock/...) is present, since versions are already resolved -- this is why the scanner's own library pinning policy (ranges in pyproject.toml) is not affected. Register the rule in the core pack.yaml, document the new pass in the static-analyzer docs and threat taxonomy, and add unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
The suspicious/legitimate domain lists and matching logic previously lived inside ContextExtractor and only ran over Python AST string literals, so a tunnel/proxy endpoint hidden in a config value (e.g. base_url in config.yaml) was never classified. Refactor (behavior-preserving): extract SUSPICIOUS_DOMAINS/LEGITIMATE_DOMAINS and the classification into skill_scanner/core/static_analysis/url_classifier.py (classify_url + extract_urls). ContextExtractor imports from it and keeps the lists as class attributes for backward compatibility; existing suspicious-URL behavior is unchanged (covered by refactor-safety tests). New pass: StaticAnalyzer._scan_config_files() parses config.yaml/.yml/.json, settings.*, and *.toml (regex fallback on parse failure), runs each URL through the shared classifier, and emits CONFIG_SUSPICIOUS_URL (HIGH). Registered in the core pack.yaml. Docs updated (static-analyzer, behavioral-analyzer). Adds unit tests plus a labeled config-routed exfil eval sample; benchmark stays at 100% P/R. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse the structured YAML/JSON/TOML parse-and-walk in config URL scanning down to a single raw-text extract_urls() pass. The raw scan is simpler, drops the json/yaml/tomllib imports and the _iter_string_values recursion, and additionally catches suspicious URLs hidden in config comments. classify_url() still only flags known tunnel/exfil domains, so false-positive risk stays low. Co-authored-by: Cursor <cursoragent@cursor.com>
Extend the unpinned-dependency check beyond requirements*.txt and manifest metadata to also read pyproject.toml ([project] dependencies and optional-dependencies), setup.cfg ([options] install_requires / extras), setup.py (install_requires literals via AST), and Pipfile ([packages]/[dev-packages]). All sources are normalized to requirement strings and run through the existing classifier, so a lockfile still suppresses findings and pinned specs stay clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Add an opt-in external analyzer that checks a skill's pinned Python dependencies against the free, open OSV.dev vulnerability database. Follows the established external-analyzer pattern (like VirusTotal): no API key, and it fails open so a network error never breaks a scan. - OSVAnalyzer parses requirements*.txt and manifest metadata dependencies, queries only exact (==) pins via OSV querybatch, and emits SUPPLY_CHAIN_KNOWN_VULNERABILITY (HIGH) with advisory IDs/links. - Wire use_osv through analyzer_factory, the CLI (--use-osv), the API router (use_osv on JSON + upload endpoints), and the setup wizard. - Uses httpx (already a dependency): zero new runtime dependencies. Tests mock all OSV HTTP calls (no live network). Adds a deep-dive doc and updates the analyzer selection guide, analyzer index, CLI/API references, and configuration/dependency references. Co-authored-by: Cursor <cursoragent@cursor.com>
Extend OSV pin collection beyond requirements*.txt and manifest metadata to also read pyproject.toml, setup.cfg, setup.py (install_requires via AST), and Pipfile. Only exact == pins are queried against OSV; ranges are left to the static unpinned-dependency check. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds an opt-in OSV analyzer for vulnerable pinned Python dependencies. Wires ChangesOSV Analyzer and Unpinned Dependency Detection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLIOrAPI
participant BuildAnalyzers as build_analyzers
participant OSVAnalyzer
participant OSVdev as OSV.dev API
CLIOrAPI->>BuildAnalyzers: build_analyzers(..., use_osv=True)
BuildAnalyzers->>OSVAnalyzer: instantiate analyzer
CLIOrAPI->>OSVAnalyzer: analyze(skill)
OSVAnalyzer->>OSVdev: POST querybatch(name, version pairs)
OSVdev-->>OSVAnalyzer: vulnerability results
OSVAnalyzer-->>CLIOrAPI: Finding list
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
skill_scanner/core/analyzers/osv_analyzer.py (2)
189-220: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicate pins across manifests produce duplicate findings with identical IDs.
_collect_pinned_dependenciesdoes not dedupe(name, version)pairs. If the same exact pin appears in more than one manifest (e.g.requirements.txtandpyproject.toml, a common redundancy), OSV is queried twice for the same package and twoFindingobjects are emitted with the exact sameid=f"OSV_{name}_{version}"(Line 286) but differentfile_path/line_number. Downstream consumers that key or dedupe findings byidmay silently drop one of the sources, or reports may show inflated/duplicate vulnerability counts.♻️ Suggested fix: dedupe by (name, version) while keeping all sources
def _collect_pinned_dependencies(self, skill: Skill) -> list[tuple[str, str, str, int | None]]: ... collected: list[tuple[str, str, str, int | None]] = [] + seen: set[tuple[str, str]] = set() for source, line_number, raw in self._iter_requirement_strings(skill): parsed = self._parse_pinned(raw) - if parsed is not None: + if parsed is not None and parsed not in seen: + seen.add(parsed) collected.append((parsed[0], parsed[1], source, line_number)) return collectedAlso applies to: 279-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/osv_analyzer.py` around lines 189 - 220, The duplicate OSV findings come from `_collect_pinned_dependencies` returning repeated `(name, version)` pins from different manifests, which then causes `_query_osv_batch` and `_create_finding` to emit multiple `Finding` objects with the same OSV id. Update `_collect_pinned_dependencies` (or the caller in `OSVAnalyzer.analyze`) to dedupe by `(name, version)` before querying, while preserving all source/file/line provenance for reporting so a single finding can represent multiple origins.
172-183: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
httpx.Clientis never closed.
OSVAnalyzer.__init__opens a persistenthttpx.Client(with its own connection pool) but the class exposes noclose()/__del__/context-manager support, and nothing in the analyzer callsself._client.close(). Since the factory constructs a brand-newOSVAnalyzer(and therefore a new client) on every scan request in the API server (router.py::_build_analyzersis invoked per/scan,/scan-upload, and batch-scan call), sustained traffic will churn many unclosed clients/connection pools, which is a documented source of memory/FD pressure under load for httpx-based services.♻️ Suggested fix: add explicit lifecycle management
def __init__( self, enabled: bool = True, ecosystem: str = "PyPI", timeout: float = 10.0, policy: ScanPolicy | None = None, ): super().__init__("osv_analyzer", policy=policy) self.enabled = enabled self.ecosystem = ecosystem self.timeout = timeout self._client = httpx.Client(timeout=timeout) + + def close(self) -> None: + """Release the underlying HTTP connection pool.""" + self._client.close() + + def __del__(self) -> None: + try: + self.close() + except Exception: + passIdeally
SkillScanner/build_analyzerscallers would also invokeclose()(or usewith) on teardown, but a defensive__del__prevents unbounded pool growth even if callers forget.Please confirm whether
BaseAnalyzerorSkillScanneralready has a teardown hook that closes analyzer resources elsewhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/osv_analyzer.py` around lines 172 - 183, `OSVAnalyzer` creates an `httpx.Client` in `__init__` but never closes it, so add explicit client lifecycle management to the analyzer. Introduce a close/cleanup path on `OSVAnalyzer` (and a context-manager or teardown hook if `BaseAnalyzer`/`SkillScanner` supports one), and make sure the client is closed when analyzers created by `_build_analyzers` are torn down after scans. If there is no existing teardown path in `BaseAnalyzer` or `SkillScanner`, add one and wire it into the API scan flow so `self._client.close()` is always called.tests/test_rule_registry.py (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace ambiguous en dash with a hyphen.
Ruff flags the
–(EN DASH) in the comment; using ASCII-avoids lint failures and encoding ambiguity.🔧 Proposed fix
- # OSV analyzer: SUPPLY_CHAIN_KNOWN_VULNERABILITY – from external OSV.dev API + # OSV analyzer: SUPPLY_CHAIN_KNOWN_VULNERABILITY - from external OSV.dev API🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_rule_registry.py` at line 92, The comment in tests/test_rule_registry.py uses an ambiguous EN DASH in the “OSV analyzer” note, which triggers Ruff/encoding issues. Update that comment to use a standard ASCII hyphen instead of the en dash, keeping the text otherwise unchanged so the rule registry test comment matches lint expectations.Source: Linters/SAST tools
skill_scanner/core/analyzers/static.py (2)
752-778: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDependency pinning check doesn't skip documentation/example manifests.
_collect_requirement_entriesmatchesrequirements*.txt,pyproject.toml,setup.cfg,setup.py, andPipfilepurely by basename, regardless of directory. Other checks in this class (e.g._scan_scriptsat line 524,_scan_asset_filesat line 1724) use_is_doc_file()to avoid flagging content under doc/example paths. An illustrativedocs/examples/requirements.txtsnippet in a skill's documentation would be flagged here as a real unpinned dependency, adding noise unrelated to the skill's actual install-time risk.Uses
doc_path_indicatorsanddoc_filename_patternsfrom the active scan policy to determine if a given relative path belongs to a documentation or example area (e.g.docs/,examples/).♻️ Proposed fix
for skill_file in skill.files: file_name = Path(skill_file.relative_path).name.lower() path = skill_file.relative_path + if self._is_doc_file(path): + continue if file_name.startswith("requirements") and file_name.endswith(".txt"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/static.py` around lines 752 - 778, The dependency collection in _collect_requirement_entries is treating documentation/example dependency files as real install inputs because it only checks basenames like requirements*.txt, pyproject.toml, setup.cfg, setup.py, and Pipfile. Update this method to skip paths identified as docs/examples using the same scan-policy-aware logic used by _scan_scripts and _scan_asset_files, leveraging doc_path_indicators and doc_filename_patterns before adding entries. Keep the filtering applied consistently for every dependency source path so only actual skill install-time manifests are scanned.
595-596: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix RUF012: mutable class attribute default.
_LOCKFILE_NAMESis asetliteral assigned directly in the class body, which Ruff flags because it creates shared mutable state across instances (even though it's never mutated here).🔧 Proposed fix
- # Lockfiles whose presence means dependency versions are already resolved/frozen. - _LOCKFILE_NAMES = {"uv.lock", "poetry.lock", "pipfile.lock", "requirements.lock"} + # Lockfiles whose presence means dependency versions are already resolved/frozen. + _LOCKFILE_NAMES = frozenset({"uv.lock", "poetry.lock", "pipfile.lock", "requirements.lock"})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/static.py` around lines 595 - 596, The class-level _LOCKFILE_NAMES set in the Static analyzer is a mutable attribute default that Ruff flags as shared state. Update the Static class definition to use an immutable container for the lockfile names, or otherwise move the value to a safe constant pattern so it is not a mutable class attribute. Keep the existing lockfile checks in the Static analyzer working by referencing the revised _LOCKFILE_NAMES symbol wherever it is used.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/analyzers/static-analyzer.md`:
- Around line 21-22: The TL;DR summary in static-analyzer.md is out of date
because the diagram now includes a new detection pass. Update the pass count in
the summary to reflect the added 15th pass, and make sure the wording stays
consistent with the flow shown by the analyzer pipeline entries like the
Dependency pinning checks and Referenced file scanning steps.
---
Nitpick comments:
In `@skill_scanner/core/analyzers/osv_analyzer.py`:
- Around line 189-220: The duplicate OSV findings come from
`_collect_pinned_dependencies` returning repeated `(name, version)` pins from
different manifests, which then causes `_query_osv_batch` and `_create_finding`
to emit multiple `Finding` objects with the same OSV id. Update
`_collect_pinned_dependencies` (or the caller in `OSVAnalyzer.analyze`) to
dedupe by `(name, version)` before querying, while preserving all
source/file/line provenance for reporting so a single finding can represent
multiple origins.
- Around line 172-183: `OSVAnalyzer` creates an `httpx.Client` in `__init__` but
never closes it, so add explicit client lifecycle management to the analyzer.
Introduce a close/cleanup path on `OSVAnalyzer` (and a context-manager or
teardown hook if `BaseAnalyzer`/`SkillScanner` supports one), and make sure the
client is closed when analyzers created by `_build_analyzers` are torn down
after scans. If there is no existing teardown path in `BaseAnalyzer` or
`SkillScanner`, add one and wire it into the API scan flow so
`self._client.close()` is always called.
In `@skill_scanner/core/analyzers/static.py`:
- Around line 752-778: The dependency collection in _collect_requirement_entries
is treating documentation/example dependency files as real install inputs
because it only checks basenames like requirements*.txt, pyproject.toml,
setup.cfg, setup.py, and Pipfile. Update this method to skip paths identified as
docs/examples using the same scan-policy-aware logic used by _scan_scripts and
_scan_asset_files, leveraging doc_path_indicators and doc_filename_patterns
before adding entries. Keep the filtering applied consistently for every
dependency source path so only actual skill install-time manifests are scanned.
- Around line 595-596: The class-level _LOCKFILE_NAMES set in the Static
analyzer is a mutable attribute default that Ruff flags as shared state. Update
the Static class definition to use an immutable container for the lockfile
names, or otherwise move the value to a safe constant pattern so it is not a
mutable class attribute. Keep the existing lockfile checks in the Static
analyzer working by referencing the revised _LOCKFILE_NAMES symbol wherever it
is used.
In `@tests/test_rule_registry.py`:
- Line 92: The comment in tests/test_rule_registry.py uses an ambiguous EN DASH
in the “OSV analyzer” note, which triggers Ruff/encoding issues. Update that
comment to use a standard ASCII hyphen instead of the en dash, keeping the text
otherwise unchanged so the rule registry test comment matches lint expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 732b38da-1029-4f25-ab9f-bab7b235c37c
📒 Files selected for processing (19)
docs/architecture/analyzers/index.mddocs/architecture/analyzers/meta-and-external-analyzers.mddocs/architecture/analyzers/osv-analyzer.mddocs/architecture/analyzers/static-analyzer.mddocs/architecture/threat-taxonomy.mddocs/reference/api-endpoint-reference.mddocs/reference/cli-command-reference.mddocs/reference/configuration-reference.mddocs/reference/dependencies-and-llm-providers.mdskill_scanner/api/router.pyskill_scanner/cli/cli.pyskill_scanner/cli/wizard.pyskill_scanner/core/analyzer_factory.pyskill_scanner/core/analyzers/osv_analyzer.pyskill_scanner/core/analyzers/static.pyskill_scanner/data/packs/core/pack.yamltests/static_analysis/test_dependency_pinning.pytests/test_osv_analyzer.pytests/test_rule_registry.py
ProviderConfig.validate() required a truthy credential for every provider except Bedrock and Ollama, and the only credential source it checked for Vertex was GOOGLE_APPLICATION_CREDENTIALS. This blocked ambient auth via a GCE/Cloud Run attached service account or Workload Identity, even though LiteLLM/google-auth already fall back to it automatically when no explicit credential is passed -- the same pattern already supported for Bedrock's IAM role. Excludes is_vertex from the check, mirroring the Bedrock/Ollama precedent, and documents the fallback.
- _resolve_api_key() now returns None for Vertex instead of the GOOGLE_APPLICATION_CREDENTIALS path, since vertex_ai/gemini-* models set both is_vertex and is_gemini, which was causing the file path to be written into GEMINI_API_KEY. - Regenerated configuration-reference.md via generate_reference_docs.py instead of hand-editing, and updated the underlying descriptions so the doc doesn't drift on next regeneration. Addresses CodeRabbit review feedbak on cisco-ai-defense#144.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
# Conflicts: # skill_scanner/core/static_analysis/context_extractor.py
# Conflicts: # docs/architecture/analyzers/static-analyzer.md # skill_scanner/core/analyzers/static.py # skill_scanner/data/packs/core/pack.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skill_scanner/core/analyzers/static.py (2)
688-707: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix:
_entries_from_setup_cfgcan crashanalyze()on a%ininstall_requires.The
try/except configparser.Errorblock only wrapsparser.read_string(content).parser.get("options", "install_requires")andparser.items("options.extras_require")run outside that block, andconfigparser's defaultBasicInterpolationraisesconfigparser.InterpolationSyntaxError(a subclass ofconfigparser.Error) when a value contains an unescaped%. Asetup.cfgwith a bare%ininstall_requires(malformed or adversarial input, which this analyzer must tolerate since it scans untrusted skill packages) crashes the wholeanalyze()call instead of being skipped gracefully like the TOML and setup.py paths.Disable interpolation, since these values are read as plain strings, not templated.
🛡️ Proposed fix
def _entries_from_setup_cfg(self, path: str, content: str) -> list[tuple[str, int | None, str]]: """``[options] install_requires`` and ``[options.extras_require]``.""" - parser = configparser.ConfigParser() + parser = configparser.ConfigParser(interpolation=None) try: parser.read_string(content) except configparser.Error: return []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/static.py` around lines 688 - 707, Update _entries_from_setup_cfg to construct ConfigParser with interpolation disabled, so install_requires and extras_require values are read as literal strings and bare '%' characters cannot trigger interpolation errors during analyze(). Preserve the existing parsing and entry extraction behavior.
799-801: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLockfile-presence check is bypassable and too broad.
any(... in self._LOCKFILE_NAMES for f in skill.files)skips ALL dependency-pinning findings for the entire skill package if a recognized lockfile filename appears anywhere in the file tree, regardless of directory or whether it actually corresponds to the flagged requirements source. A skill author can drop an unrelated or emptypoetry.lockanywhere in the package to silence everySUPPLY_CHAIN_UNPINNED_DEPENDENCYfinding for an unrelatedrequirements.txt,pyproject.toml, orPipfileelsewhere in the tree.Scope the lockfile check to the directory of the requirement source it's meant to cover, so an unrelated or planted lockfile cannot suppress findings for a different manifest.
🛡️ Proposed fix
findings: list[Finding] = [] - # A lockfile freezes the resolved versions, so ranges are intentional. - if any(Path(f.relative_path).name.lower() in self._LOCKFILE_NAMES for f in skill.files): - return findings + # A lockfile freezes the resolved versions, so ranges are intentional. + # Scope this to the lockfile's own directory so a lockfile placed + # elsewhere in the package cannot suppress findings for an + # unrelated, unpinned requirements source. + lockfile_dirs = { + Path(f.relative_path).parent + for f in skill.files + if Path(f.relative_path).name.lower() in self._LOCKFILE_NAMES + } for source_label, line_number, raw in self._collect_requirement_entries(skill): + if Path(source_label).parent in lockfile_dirs: + continue classified = self._classify_requirement(raw)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/static.py` around lines 799 - 801, Update the lockfile check in the surrounding analyzer method to evaluate only lockfiles located in the same directory as the dependency requirement source being analyzed, rather than anywhere under skill.files. Preserve the existing recognized-name filtering, and ensure unrelated or empty lockfiles elsewhere cannot suppress SUPPLY_CHAIN_UNPINNED_DEPENDENCY findings.
🧹 Nitpick comments (1)
skill_scanner/core/analyzers/static.py (1)
709-723: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider scoping the
ast.walkto list/tuple elements only.
ast.walk(node.value)descends into every nested node of theinstall_requiresexpression, not just its list elements. If the value is something likeinstall_requires=filter_deps(read_requirements("requirements.txt")), unrelated string arguments get collected and misclassified as package requirements, producing noisy false-positive findings.Restrict collection to elements of a literal list/tuple, and skip non-literal expressions.
♻️ Proposed refactor
for node in ast.walk(tree): if not (isinstance(node, ast.keyword) and node.arg == "install_requires"): continue - for literal in ast.walk(node.value): - if isinstance(literal, ast.Constant) and isinstance(literal.value, str): - line_number = getattr(literal, "lineno", None) - entries.append((path, line_number, literal.value)) + if not isinstance(node.value, (ast.List, ast.Tuple)): + continue + for elt in node.value.elts: + if isinstance(elt, ast.Constant) and isinstance(elt.value, str): + entries.append((path, getattr(elt, "lineno", None), elt.value))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skill_scanner/core/analyzers/static.py` around lines 709 - 723, Update _entries_from_setup_py to process install_requires only when node.value is an ast.List or ast.Tuple, collecting string constants from its direct elements rather than recursively walking the expression. Skip calls, names, and other non-literal expressions so unrelated nested strings are not treated as package requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@skill_scanner/core/analyzers/static.py`:
- Around line 688-707: Update _entries_from_setup_cfg to construct ConfigParser
with interpolation disabled, so install_requires and extras_require values are
read as literal strings and bare '%' characters cannot trigger interpolation
errors during analyze(). Preserve the existing parsing and entry extraction
behavior.
- Around line 799-801: Update the lockfile check in the surrounding analyzer
method to evaluate only lockfiles located in the same directory as the
dependency requirement source being analyzed, rather than anywhere under
skill.files. Preserve the existing recognized-name filtering, and ensure
unrelated or empty lockfiles elsewhere cannot suppress
SUPPLY_CHAIN_UNPINNED_DEPENDENCY findings.
---
Nitpick comments:
In `@skill_scanner/core/analyzers/static.py`:
- Around line 709-723: Update _entries_from_setup_py to process install_requires
only when node.value is an ast.List or ast.Tuple, collecting string constants
from its direct elements rather than recursively walking the expression. Skip
calls, names, and other non-literal expressions so unrelated nested strings are
not treated as package requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bfedc89-7beb-477f-bb62-71cc0d7bb923
📒 Files selected for processing (8)
docs/architecture/analyzers/static-analyzer.mddocs/reference/cli-command-reference.mddocs/reference/configuration-reference.mddocs/reference/dependencies-and-llm-providers.mdskill_scanner/api/router.pyskill_scanner/cli/cli.pyskill_scanner/core/analyzers/static.pyskill_scanner/data/packs/core/pack.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/reference/dependencies-and-llm-providers.md
- docs/architecture/analyzers/static-analyzer.md
- skill_scanner/data/packs/core/pack.yaml
- docs/reference/cli-command-reference.md
- skill_scanner/api/router.py
- docs/reference/configuration-reference.md
- skill_scanner/cli/cli.py
4 participants
Summary
Adds an optional external analyzer that checks a skill's pinned Python dependencies against the free, open OSV.dev vulnerability database. It follows the existing optional-analyzer pattern (like VirusTotal): opt-in via
--use-osv, no API key required, and it fails open on network errors so it never blocks a scan.core/analyzers/osv_analyzer.py(OSVAnalyzer) — batch-queriesapi.osv.dev/v1/querybatch, emitsSUPPLY_CHAIN_KNOWN_VULNERABILITY(HIGH) with advisory IDs/links.==pins are queried (an open range has no single version to look up; that risk is covered by the unpinned-dependency check). Pins are collected fromrequirements*.txt,pyproject.toml,setup.cfg,setup.py(AST),Pipfile, and manifest metadata.analyzer_factory, CLI (--use-osv), REST API (use_osv), and the interactive wizard.osv-analyzer.mddeep-dive + CLI/API/configuration reference updates.Test plan
tests/test_osv_analyzer.py— all HTTP mocked (no live network): pinned parsing, vulnerable/clean packages, unpinned not queried, network-error fail-open, multi-source pin collection (TOML tests skip on <3.11).uv run pytest tests/passes.uv run pre-commit run --all-filespasses.uv run python evals/runners/benchmark_runner.py— 100% precision/recall, no regression.Made with Cursor
Summary by CodeRabbit
New Features
--use-osvCLI support and a matching API option.Bug Fixes