Skip to content

Commit 53c84d1

Browse files
valentijnscholtenvalentijnscholten
committed
vulnerability_id processing: remove duplicate delete, fix bug, add tests
1 parent 5a05a60 commit 53c84d1

8 files changed

Lines changed: 654 additions & 33 deletions

‎dojo/finding/helper.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -762,12 +762,15 @@ def add_endpoints(new_finding, form):
762762
endpoint=endpoint, defaults={"date": form.cleaned_data["date"] or timezone.now()})
763763

764764

765-
def save_vulnerability_ids(finding, vulnerability_ids):
765+
def save_vulnerability_ids(finding, vulnerability_ids, *, delete_existing: bool = True):
766766
# Remove duplicates
767767
vulnerability_ids = list(dict.fromkeys(vulnerability_ids))
768768

769-
# Remove old vulnerability ids
770-
Vulnerability_Id.objects.filter(finding=finding).delete()
769+
# Remove old vulnerability ids if requested
770+
# Callers can set delete_existing=False when they know there are no existing IDs
771+
# to avoid an unnecessary delete query (e.g., for new findings)
772+
if delete_existing:
773+
Vulnerability_Id.objects.filter(finding=finding).delete()
771774

772775
# Save new vulnerability ids
773776
# Using bulk create throws Django 50 warnings about unsaved models...

‎dojo/importers/base_importer.py‎

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
Test_Import,
3232
Test_Import_Finding_Action,
3333
Test_Type,
34-
Vulnerability_Id,
3534
)
3635
from dojo.notifications.helper import create_notification
3736
from dojo.tag_utils import bulk_add_tags_to_instances
@@ -796,36 +795,41 @@ def process_vulnerability_ids(
796795
to create `Vulnerability_Id` objects with the finding associated correctly.
797796
Only updates if vulnerability_ids have changed to avoid unnecessary database operations.
798797
798+
Note: The finding parameter can be:
799+
- A new finding (from import) with unsaved_vulnerability_ids set from the parsed report
800+
- An existing finding (from reimport) with unsaved_vulnerability_ids copied from the
801+
finding_from_report before calling this method
802+
799803
Args:
800-
finding: The finding to process vulnerability IDs for
804+
finding: The finding to process vulnerability IDs for.
805+
For reimports, this is the existing finding with unsaved_vulnerability_ids
806+
set from the import/reimport report.
801807
802808
Returns:
803809
The finding object
804810
805811
"""
806-
if finding.unsaved_vulnerability_ids:
807-
# Only check for changes if the finding has been saved and might have existing vulnerability_ids
808-
# For new findings, we always need to save vulnerability_ids
809-
if finding.pk and finding.vulnerability_id_set.exists():
810-
# Check if vulnerability_ids have changed before updating
811-
# Get existing vulnerability IDs from the database
812-
existing_vuln_ids = set(finding.vulnerability_ids) if finding.vulnerability_ids else set()
813-
# Normalize the new vulnerability IDs (remove duplicates for comparison)
814-
new_vuln_ids = set(finding.unsaved_vulnerability_ids)
815-
816-
# Only update if vulnerability IDs have changed
817-
if existing_vuln_ids == new_vuln_ids:
818-
logger.debug(
819-
f"Skipping vulnerability_ids update for finding {finding.id} - "
820-
f"vulnerability_ids unchanged: {sorted(existing_vuln_ids)}",
821-
)
822-
return finding
823-
824-
# Remove old vulnerability ids - keeping this call only because of flake8
825-
Vulnerability_Id.objects.filter(finding=finding).delete()
826-
827-
# user the helper function
828-
finding_helper.save_vulnerability_ids(finding, finding.unsaved_vulnerability_ids)
812+
# Normalize to empty list if None - always process vulnerability IDs
813+
vulnerability_ids_to_process = finding.unsaved_vulnerability_ids or []
814+
815+
# For existing findings, check if there are changes before updating
816+
if finding.pk and len(finding.vulnerability_id_set.all()) > 0:
817+
# Get existing vulnerability IDs from the database
818+
existing_vuln_ids = set(finding.vulnerability_ids) if finding.vulnerability_ids else set()
819+
# Normalize the new vulnerability IDs (remove duplicates for comparison)
820+
new_vuln_ids = set(vulnerability_ids_to_process)
821+
822+
# Only update if vulnerability IDs have changed
823+
if existing_vuln_ids == new_vuln_ids:
824+
logger.debug(
825+
f"Skipping vulnerability_ids update for finding {finding.id} - "
826+
f"vulnerability_ids unchanged: {sorted(existing_vuln_ids)}",
827+
)
828+
return finding
829+
830+
# Use the helper function which handles deletion and creation
831+
# Always use delete_existing=True - it's a no-op if there are no existing IDs
832+
finding_helper.save_vulnerability_ids(finding, vulnerability_ids_to_process, delete_existing=True)
829833

830834
return finding
831835

‎dojo/importers/default_reimporter.py‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,7 @@ def finding_post_processing(
695695
self,
696696
finding: Finding,
697697
finding_from_report: Finding,
698-
) -> None:
698+
) -> Finding:
699699
"""
700700
Save all associated objects to the finding after it has been saved
701701
for the purpose of foreign key restrictions
@@ -715,10 +715,19 @@ def finding_post_processing(
715715
finding.unsaved_files = finding_from_report.unsaved_files
716716
self.process_files(finding)
717717
# Process vulnerability IDs
718-
if finding_from_report.unsaved_vulnerability_ids:
719-
finding.unsaved_vulnerability_ids = finding_from_report.unsaved_vulnerability_ids
718+
# Copy unsaved_vulnerability_ids from the report finding to the existing finding
719+
# so process_vulnerability_ids can process them (see its docstring for details)
720+
# Always set it (even if empty list) so we can clear existing IDs when report has none
721+
finding.unsaved_vulnerability_ids = finding_from_report.unsaved_vulnerability_ids or []
722+
# Store the current cve value to check if it changes
723+
old_cve = finding.cve
720724
# legacy cve field has already been processed/set earlier
721-
return self.process_vulnerability_ids(finding)
725+
finding = self.process_vulnerability_ids(finding)
726+
# Save the finding only if the cve field was changed by save_vulnerability_ids
727+
# This is temporary as the cve field will be phased out
728+
if finding.cve != old_cve:
729+
finding.save()
730+
return finding
722731

723732
def process_groups_for_all_findings(
724733
self,

‎unittests/scans/anchore_grype/check_all_fields_different_ids_fabricated.json‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
{
2+
"matches": [
3+
{
4+
"vulnerability": {
5+
"id": "GHSA-v6rh-hp5x-86rv",
6+
"dataSource": "https://github.com/advisories/GHSA-v6rh-hp5x-86rv",
7+
"namespace": "github:python",
8+
"severity": "High",
9+
"urls": [
10+
"https://github.com/advisories/GHSA-v6rh-hp5x-86rv"
11+
],
12+
"description": "Potential bypass of an upstream access control based on URL paths in Django",
13+
"cvss": [],
14+
"fix": {
15+
"versions": [
16+
"3.2.10"
17+
],
18+
"state": "fixed"
19+
},
20+
"advisories": []
21+
},
22+
"relatedVulnerabilities": [
23+
{
24+
"id": "CVE-2021-1234",
25+
"dataSource": "https://nvd.nist.gov/vuln/detail/CVE-2021-1234",
26+
"namespace": "nvd",
27+
"severity": "High",
28+
"urls": [
29+
"https://example.com/cve-2021-1234"
30+
],
31+
"description": "A different CVE for testing vulnerability ID changes",
32+
"cvss": [
33+
{
34+
"version": "3.1",
35+
"vector": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H",
36+
"metrics": {
37+
"baseScore": 9.8,
38+
"exploitabilityScore": 3.9,
39+
"impactScore": 5.9
40+
},
41+
"vendorMetadata": {}
42+
}
43+
]
44+
},
45+
{
46+
"id": "CVE-2021-5678",
47+
"dataSource": "https://nvd.nist.gov/vuln/detail/CVE-2021-5678",
48+
"namespace": "nvd",
49+
"severity": "Medium",
50+
"urls": [
51+
"https://example.com/cve-2021-5678"
52+
],
53+
"description": "Another different CVE for testing vulnerability ID changes",
54+
"cvss": [
55+
{
56+
"version": "3.1",
57+
"vector": "CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:L/I:L/A:L",
58+
"metrics": {
59+
"baseScore": 6.3,
60+
"exploitabilityScore": 3.9,
61+
"impactScore": 2.4
62+
},
63+
"vendorMetadata": {}
64+
}
65+
]
66+
}
67+
],
68+
"matchDetails": [
69+
{
70+
"matcher": "python-matcher",
71+
"searchedBy": {
72+
"language": "python",
73+
"namespace": "github:python"
74+
},
75+
"found": {
76+
"versionConstraint": ">=3.2,<3.2.10 (python)"
77+
}
78+
}
79+
],
80+
"artifact": {
81+
"name": "Django",
82+
"version": "3.2.9",
83+
"type": "python",
84+
"locations": [
85+
{
86+
"path": "/usr/local/lib/python3.8/site-packages/Django-3.2.9.dist-info/METADATA",
87+
"layerID": "sha256:b1d4455cf82b15a50b006fe87bd29f694c8f9155456253eb67fdd155b5edcf4a"
88+
}
89+
],
90+
"language": "python",
91+
"licenses": [
92+
"BSD-3-Clause"
93+
],
94+
"cpes": [
95+
"cpe:2.3:a:django_software_foundation:Django:3.2.9:*:*:*:*:*:*:*"
96+
],
97+
"purl": "pkg:pypi/Django@3.2.9",
98+
"metadata": null
99+
}
100+
}
101+
],
102+
"source": {
103+
"type": "image",
104+
"target": {
105+
"userInput": "vulnerable-image:latest",
106+
"imageID": "sha256:ce9898fd214aef9c994a42624b09056bdce3ff4a8e3f68dc242d967b80fcbeee",
107+
"manifestDigest": "sha256:9d8825ab20ac86b40eb71495bece1608a302fb180384740697a28c2b0a5a0fc6",
108+
"mediaType": "application/vnd.docker.distribution.manifest.v2+json",
109+
"tags": [
110+
"vulnerable-image:latest"
111+
],
112+
"imageSize": 707381791,
113+
"layers": []
114+
}
115+
},
116+
"distro": {
117+
"name": "debian",
118+
"version": "10",
119+
"idLike": ""
120+
},
121+
"descriptor": {
122+
"name": "grype",
123+
"version": "0.28.0",
124+
"configuration": {
125+
"configPath": "",
126+
"output": "json",
127+
"file": "",
128+
"output-template-file": "",
129+
"quiet": false,
130+
"check-for-app-update": true,
131+
"only-fixed": false,
132+
"scope": "Squashed",
133+
"log": {
134+
"structured": false,
135+
"level": "",
136+
"file": ""
137+
},
138+
"db": {
139+
"cache-dir": "/home/user/.cache/grype/db",
140+
"update-url": "https://toolbox-data.anchore.io/grype/databases/listing.json",
141+
"ca-cert": "",
142+
"auto-update": true,
143+
"validate-by-hash-on-start": false
144+
},
145+
"dev": {
146+
"profile-cpu": false,
147+
"profile-mem": false
148+
},
149+
"fail-on-severity": "",
150+
"registry": {
151+
"insecure-skip-tls-verify": false,
152+
"insecure-use-http": false,
153+
"auth": []
154+
},
155+
"ignore": null,
156+
"exclude": []
157+
},
158+
"db": {
159+
"built": "2021-12-24T08:14:02Z",
160+
"schemaVersion": 3,
161+
"location": "/home/user/.cache/grype/db/3",
162+
"checksum": "sha256:6c4777e1acea787e5335ccee6b5e4562cd1767b9cca138c07e0802efb2a74162",
163+
"error": null
164+
}
165+
}
166+
}
167+

0 commit comments

Comments
 (0)