From 9adff12a381fe9b3f54fdad684bca29486c1c6c5 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 24 May 2022 17:00:54 -0400 Subject: [PATCH 1/3] osv: fix incorrect schema assumptions We shouldn't assume that any fields other than `id` or `modified` exist, and we currently don't check the latter. Signed-off-by: William Woodruff --- pip_audit/_service/osv.py | 27 ++++++++++++-- test/service/test_osv.py | 77 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/pip_audit/_service/osv.py b/pip_audit/_service/osv.py index cfbd56fa..1a2260f1 100644 --- a/pip_audit/_service/osv.py +++ b/pip_audit/_service/osv.py @@ -3,6 +3,7 @@ """ import json +import logging from pathlib import Path from typing import List, Optional, Tuple, cast @@ -18,6 +19,8 @@ VulnerabilityService, ) +logger = logging.getLogger(__name__) + class OsvService(VulnerabilityService): """ @@ -77,10 +80,28 @@ def query(self, spec: Dependency) -> Tuple[Dependency, List[VulnerabilityResult] for vuln in response_json["vulns"]: id = vuln["id"] - description = vuln["details"] - aliases = set(vuln["aliases"]) + + # The summary is intended to be shorter, so we prefer it over + # details, if present. However, neither is required. + description = vuln.get("summary") + if description is None: + description = vuln.get("details") + if description is None: + description = "N/A" + + aliases = set() + if "aliases" in vuln: + aliases.update(vuln["aliases"]) + + # OSV doesn't mandate this field either. There's very little we + # can do without it, so we skip any results that are missing it. + affecteds = vuln.get("affected") + if affecteds is None: + logger.warning(f"OSV vuln entry '{id}' is missing 'affected' list") + continue + fix_versions: List[Version] = [] - for affected in vuln["affected"]: + for affected in affecteds: pkg = affected["package"] # We only care about PyPI versions if pkg["name"] == spec.canonical_name and pkg["ecosystem"] == "PyPI": diff --git a/test/service/test_osv.py b/test/service/test_osv.py index c2ffc967..c2729092 100644 --- a/test/service/test_osv.py +++ b/test/service/test_osv.py @@ -125,3 +125,80 @@ def test_osv_skipped_dep(): vulns = results[dep] assert len(vulns) == 0 + + +@pytest.mark.parametrize( + ["summary", "details", "description"], + [ + ("fakesummary", "fakedetails", "fakesummary"), + (None, "fakedetails", "fakedetails"), + (None, None, "N/A"), + ], +) +def test_osv_vuln_description_fallbacks(monkeypatch, summary, details, description): + payload = { + "vulns": [ + { + "id": "fakeid", + "summary": summary, + "details": details, + "affected": [ + { + "package": {"name": "foo", "ecosystem": "PyPI"}, + "ranges": [{"type": "ECOSYSTEM", "events": [{"fixed": "1.0.1"}]}], + } + ], + } + ], + } + + response = pretend.stub(raise_for_status=lambda: None, json=lambda: payload) + post = pretend.call_recorder(lambda *a, **kw: response) + + osv = service.OsvService() + monkeypatch.setattr(osv.session, "post", post) + + dep = service.ResolvedDependency("foo", Version("1.0.0")) + results = dict(osv.query_all(iter([dep]))) + + assert len(results) == 1 + assert dep in results + + vulns = results[dep] + assert len(vulns) == 1 + + assert vulns[0].description == description + + +def test_osv_vuln_affected_missing(monkeypatch): + logger = pretend.stub(warning=pretend.call_recorder(lambda s: None)) + monkeypatch.setattr(service.osv, "logger", logger) + + payload = { + "vulns": [ + { + "id": "fakeid", + "summary": "fakesummary", + "details": "fakedetails", + } + ], + } + + response = pretend.stub(raise_for_status=lambda: None, json=lambda: payload) + post = pretend.call_recorder(lambda *a, **kw: response) + + osv = service.OsvService() + monkeypatch.setattr(osv.session, "post", post) + + dep = service.ResolvedDependency("foo", Version("1.0.0")) + results = dict(osv.query_all(iter([dep]))) + + assert len(results) == 1 + assert dep in results + + vulns = results[dep] + assert len(vulns) == 0 + + assert logger.warning.calls == [ + pretend.call("OSV vuln entry 'fakeid' is missing 'affected' list") + ] From 238ca3a762e04c32ec9cef2e9e17bb8a634592fa Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 24 May 2022 17:04:13 -0400 Subject: [PATCH 2/3] CHANGELOG: record changes Signed-off-by: William Woodruff --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d059dd7..3457c9f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ All versions prior to 0.0.9 are untracked. faster and more responsive ([#283](https://github.com/trailofbits/pip-audit/pull/283)) +### Fixed + +* Vulnerability sources: a bug stemming from an incorrect assumption + about OSV's schema guarantees was fixed + ([#284](https://github.com/trailofbits/pip-audit/pull/284)) + ## [2.3.1] - 2022-05-24 ### Fixed From d9639bbe12a7e1addc475a1b7df1453970475ac8 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 24 May 2022 17:18:46 -0400 Subject: [PATCH 3/3] Update pip_audit/_service/osv.py Co-authored-by: Dustin Ingram --- pip_audit/_service/osv.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pip_audit/_service/osv.py b/pip_audit/_service/osv.py index 1a2260f1..e10867dd 100644 --- a/pip_audit/_service/osv.py +++ b/pip_audit/_service/osv.py @@ -89,9 +89,7 @@ def query(self, spec: Dependency) -> Tuple[Dependency, List[VulnerabilityResult] if description is None: description = "N/A" - aliases = set() - if "aliases" in vuln: - aliases.update(vuln["aliases"]) + aliases = set(vuln.get("aliases", [])) # OSV doesn't mandate this field either. There's very little we # can do without it, so we skip any results that are missing it.