Skip to content

fix: upgrade osv-scanner to v2 and handle v2 output format - #108

Merged
3asm merged 3 commits into
mainfrom
fix/upgrade-osv-scanner-v2
Sep 21, 2026
Merged

3asm merged 3 commits into
mainfrom
fix/upgrade-osv-scanner-v2

Conversation

@azizelbelaychy

Copy link
Copy Markdown
Contributor

Summary

Upgrades osv-scanner from v1 to v2 (v2.6.0+) to eliminate false-positive vulnerability reports on unpinned packages.

In OSV v1, unpinned dependencies (such as unversioned packages in requirements.txt) were assigned dummy "0.0.0" versions, resulting in dozens of phantom CVE reports. OSV v2 resolves
this by skipping unpinned dependencies.

Additionally, updates _is_valid_osv_result() to handle OSV v2's JSON output structure, which introduces top-level metadata fields (such as "experimental_config").
──────

How It Works

  1. Scanning: osv-scanner v2 scans the dependency file and skips unpinned packages without fabricating "0.0.0" versions.
  2. Output Validation: _is_valid_osv_result() checks not parsed.get("results") rather than strict dictionary equality (== {"results": []}), ensuring empty scans with extra top-level
    metadata keys (experimental_config) are correctly identified as having no findings.
  3. Reporting: Only legitimate, versioned vulnerabilities are parsed and emitted to the pipeline.
    ──────

Key Changes

• Dockerfile: Upgraded installation from github.com/google/osv-scanner/cmd/osv-scanner@v1 to github.com/google/osv-scanner/v2/cmd/osv-scanner@v2.
• agent/osv_agent.py: Updated _is_valid_osv_result() to check not parsed.get("results") to properly handle OSV v2 JSON output.
• tests/osv_agent_test.py: Added unit tests covering v1 and v2 empty result structures, malformed JSON, and populated findings.
──────

Verification

• All 103 unit tests pass (pytest -m "not docker").
• Local verification on unpinned curl dependencies: eliminated all 26 false positives (reported 0 findings).
• Local verification on pinned dependencies (cryptography==3.2.0): confirmed real CVEs are still detected and emitted properly.

@ostorlab-ai-pr-review

ostorlab-ai-pr-review Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

📊 PR Complexity Assessment: Moderate (Score: 3/5)

This PR upgrades osv-scanner from v1 to v2.6.0 in the Dockerfile and adjusts _is_valid_osv_result() in agent/osv_agent.py to handle OSV v2's JSON output, which adds top-level metadata (e.g., experimental_config) alongside results. It also adds extensive unit tests. The code change is small and well-focused, but it touches the security scanning pipeline and changes the external tool major version plus base build image, so it warrants moderate review diligence.

🔍 Complexity Drivers

  • Major version upgrade of an external security tool (osv-scanner v1 -> v2.6.0) with a changed JSON output contract
  • Validation logic governs whether vulnerabilities are emitted, so an incorrect result could silently suppress findings
  • Dockerfile base image bump (golang 1.22 -> 1.27) alongside the tool upgrade
  • Moderate blast radius across the OSV agent scanning/reporting path
  • Tests cover several edge cases, but downstream v2 field-schema compatibility is only partially verified

🎯 Suggested Attention Areas

  • agent/osv_agent.py (OSV result validation logic): Verify _is_valid_osv_result() correctly treats empty/malformed output as invalid and a non-empty results list as valid. Check all edge cases: JSON null/scalars/arrays, dicts missing results, results present but not a list, and results items with v2-specific schemas. Ensure downstream handlers never receive an empty or malformed result that could raise KeyError/IndexError, and confirm real vulnerabilities are not suppressed.
  • Dockerfile (osv-scanner v2 upgrade and Go base image bump): Confirm the module path github.com/google/osv-scanner/v2/cmd/osv-scanner@v2.6.0 is correct and that version exists. Validate the golang 1.27-alpine base image builds the scanner successfully and that the final image contains a working osv-scanner binary. Also verify the Go version bump does not introduce build-time or compatibility regressions.
  • tests/osv_agent_test.py (Test coverage for OSV v2 output parsing): Review the new tests to ensure they reflect genuine v2 output structure rather than only synthetic experimental_config additions. Verify the populated-vulnerability test actually asserts that v2 fields are parsed correctly downstream, and consider adding cases for a dict with no results key, malformed result items, and non-zero subprocess return codes.

⚠️ Sensitive Paths Touched

  • Dockerfile

⚡ Auto-Merge Assessment

❌ Not recommended for auto-merge — manual peer review required.

@ostorlab-ai-pr-review ostorlab-ai-pr-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is generally sound with no critical defects or security vulnerabilities identified. Three maintainability improvements are recommended: defensively type-checking the parsed OSV JSON before attribute access, pinning the Dockerfile toolchain/scanner versions for reproducible builds, and exercising the validation behavior through the public agent path rather than the private helper.

Comment thread agent/osv_agent.py Outdated
Comment thread Dockerfile Outdated
Comment thread tests/osv_agent_test.py Outdated
Comment thread tests/osv_agent_test.py Outdated
Comment thread agent/osv_agent.py Outdated
Comment thread agent/osv_agent.py Outdated
Comment thread agent/osv_agent.py Outdated
Comment thread tests/osv_agent_test.py Outdated

@ostorlab-ai-pr-review ostorlab-ai-pr-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Total Issues Found: 1
Critical Issues (Bugs): 0
Security Issues: 0
Suggestions: 1
Estimated Review Effort: 1/5

Key Findings:
The PR contains one minor style/convention issue in tests/osv_agent_test.py: a local import json inside a function should be moved to the top of the file alongside the other standard-library imports. No functional, security, or correctness problems were identified.

Comment thread tests/osv_agent_test.py Outdated

@ostorlab-ai-pr-review ostorlab-ai-pr-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found two minor maintainability suggestions and no critical or security issues. The validator should enforce that every result entry is a dict before returning True, and the v2 regression test should assert exactly one emitted message with the expected title and dna rather than only checking that some message was emitted.

Comment thread agent/osv_agent.py
Comment thread tests/osv_agent_test.py
@3asm
3asm merged commit c237580 into main Sep 21, 2026
5 checks passed
@3asm
3asm deleted the fix/upgrade-osv-scanner-v2 branch September 21, 2026 17:33
@ostorlab-ai-pr-review

Copy link
Copy Markdown

Reviewer Scores for Ostorlab/agent_osv #108

Each reviewer who left comments is scored between -10 and +10 based on the overall quality of their review feedback. -10 is harmless, not useful, or wastes time; +10 finds an actual, useful bug.

Total reviewers scored: 2
Useful (score > 0): 2
Neutral (score = 0): 0
Not useful (score < 0): 0
Average score: 6.5

Reviewer Score Reasoning
@m0hamed541 6 Provided useful feedback: recommending tests exercise the public OSCAgent.process() path and, more importantly, flagging that replacing an empty-list check with implicit falsiness broadens the invalid-result behavior in a way that could suppress findings. Both comments are reasonable and add real review value, though neither is a critical bug.
@yassminassaii 7 Caught a real crash for valid non-object JSON (null, []) that would raise AttributeError before .get(), which is a material correctness/usefulness issue. The remaining comments about explicit checks and top-level imports are minor but aligned with project conventions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants