Skip to content

Commit 80f8110

Browse files
valentijnscholtenvalentijnscholten
authored
Fix risk-accepted findings not being closed when vulnerability is fixed (#14125)
Fixes #10769 When a finding is risk-accepted and the underlying vulnerability is subsequently fixed (no longer appears in scan reports), the import/reimport process was failing to properly close these findings. This occurred because: 1. DefaultImporter.close_old_findings() only queried for active=True findings, missing risk-accepted findings which have active=False 2. BaseImporter.mitigate_finding() did not remove the risk_accepted status when closing findings Changes: - Modified DefaultImporter.close_old_findings() to include risk-accepted findings in the query (Q(active=True) | Q(risk_accepted=True)) - Added risk_unaccept() call in BaseImporter.mitigate_finding() to remove risk acceptance when findings are closed - Added comprehensive unit tests covering both scenarios: * Risk-accepted findings that are no longer in scan reports (should be closed and risk acceptance removed) * Risk-accepted findings that are still in scan reports (should remain risk-accepted) The fix ensures that when a previously risk-accepted vulnerability is genuinely fixed, the finding status accurately reflects this by being mitigated and having the risk acceptance removed.
1 parent bc79807 commit 80f8110

5 files changed

Lines changed: 346 additions & 1 deletion

File tree

‎dojo/importers/base_importer.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
from django.utils.timezone import make_aware
1414

1515
import dojo.finding.helper as finding_helper
16+
import dojo.risk_acceptance.helper as ra_helper
1617
from dojo import utils
1718
from dojo.importers.endpoint_manager import EndpointManager
1819
from dojo.importers.options import ImporterOptions
@@ -925,6 +926,9 @@ def mitigate_finding(
925926
author=self.user,
926927
entry=note_message,
927928
)
929+
# Remove risk acceptance if present (vulnerability is now fixed)
930+
# risk_unaccept will check if finding.risk_accepted is True before proceeding
931+
ra_helper.risk_unaccept(self.user, finding, perform_save=False, post_comments=False)
928932
# Mitigate the endpoint statuses
929933
self.endpoint_manager.mitigate_endpoint_status(finding.status_finding.all(), self.user, kwuser=self.user, sync=True)
930934
# to avoid pushing a finding group multiple times, we push those outside of the loop

‎dojo/importers/default_importer.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,11 @@ def close_old_findings(
328328
new_unique_ids_from_tool.append(unique_id_from_tool)
329329
# Get the initial filtered list of old findings to be closed without
330330
# considering the scope of the product or engagement
331+
# Include both active findings and risk-accepted findings (which have active=False)
332+
# Risk-accepted findings should be closed if the vulnerability is actually fixed
331333
old_findings = Finding.objects.filter(
334+
Q(active=True) | Q(risk_accepted=True),
332335
test__test_type=self.test.test_type,
333-
active=True,
334336
).exclude(test=self.test)
335337
# Filter further based on the deduplication algorithm set on the test
336338
self.deduplication_algorithm = self.determine_deduplication_algorithm()

‎unittests/scans/zap/0_zap_sample_without_zap3.xml‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
<?xml version="1.0"?><OWASPZAPReport version="2.9.0" generated="Tue, 12 May 2020 20:57:30">
2+
<site name="https://mainsite.com" host="mainsite.com" port="443" ssl="true"><alerts><alertitem>
3+
<pluginid>10011</pluginid>
4+
<alert>zap1: Cookie Without Secure Flag</alert>
5+
<name>Cookie Without Secure Flag</name>
6+
<riskcode>1</riskcode>
7+
<confidence>2</confidence>
8+
<riskdesc>Low (Medium)</riskdesc>
9+
<desc>A cookie has been set without the secure flag, which means that the cookie can be accessed via unencrypted connections.</desc>
10+
<instances>
11+
<instance>
12+
<uri>https://mainsite.com/dashboard</uri>
13+
<method>GET</method>
14+
<param>opvc</param>
15+
<evidence>Set-Cookie: opvc</evidence>
16+
</instance>
17+
<instance>
18+
<uri>https://mainsite.com/dashboard</uri>
19+
<method>GET</method>
20+
<param>dmid</param>
21+
<evidence>Set-Cookie: dmid</evidence>
22+
</instance>
23+
<instance>
24+
<uri>https://mainsite.com</uri>
25+
<method>GET</method>
26+
<param>sitevisitscookie</param>
27+
<evidence>Set-Cookie: sitevisitscookie</evidence>
28+
</instance>
29+
</instances>
30+
<count>3</count>
31+
<solution>Whenever a cookie contains sensitive information or is a session token, then it should always be passed using an encrypted channel. Ensure that the secure flag is set for cookies containing such sensitive information.</solution>
32+
<reference>http://www.owasp.org/index.php/Testing_for_cookies_attributes_(OWASP-SM-002)</reference>
33+
<cweid>614</cweid>
34+
<wascid>13</wascid>
35+
<sourceid>3</sourceid>
36+
</alertitem>
37+
<alertitem>
38+
<pluginid>10054</pluginid>
39+
<alert>zap2: Cookie Without SameSite Attribute</alert>
40+
<name>Cookie Without SameSite Attribute</name>
41+
<riskcode>1</riskcode>
42+
<confidence>2</confidence>
43+
<riskdesc>Low (Medium)</riskdesc>
44+
<desc>A cookie has been set without the SameSite attribute, which means that the cookie can be sent as a result of a &apos;cross-site&apos; request. The SameSite attribute is an effective counter measure to cross-site request forgery, cross-site script inclusion, and timing attacks.</desc>
45+
<instances>
46+
<instance>
47+
<uri>https://mainsite.com</uri>
48+
<method>GET</method>
49+
<param>sitevisitscookie</param>
50+
<evidence>Set-Cookie: sitevisitscookie</evidence>
51+
</instance>
52+
<instance>
53+
<uri>https://mainsite.com/dashboard</uri>
54+
<method>GET</method>
55+
<param>dmid</param>
56+
<evidence>Set-Cookie: dmid</evidence>
57+
</instance>
58+
<instance>
59+
<uri>https://mainsite.com</uri>
60+
<method>GET</method>
61+
<param>JSESSIONID</param>
62+
<evidence>Set-Cookie: JSESSIONID</evidence>
63+
</instance>
64+
<instance>
65+
<uri>https://mainsite.com/dashboard</uri>
66+
<method>GET</method>
67+
<param>opvc</param>
68+
<evidence>Set-Cookie: opvc</evidence>
69+
</instance>
70+
</instances>
71+
<count>4</count>
72+
<solution>Ensure that the SameSite attribute is set to either &apos;lax&apos; or ideally &apos;strict&apos; for all cookies.</solution>
73+
<reference>https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site</reference>
74+
<cweid>16</cweid>
75+
<wascid>13</wascid>
76+
<sourceid>3</sourceid>
77+
</alertitem>
78+
<!-- <alertitem>
79+
<pluginid>10055</pluginid>
80+
<alert>zap3: CSP Scanner: Wildcard Directive</alert>
81+
<name>CSP Scanner: Wildcard Directive</name>
82+
<riskcode>2</riskcode>
83+
<confidence>2</confidence>
84+
<riskdesc>Medium (Medium)</riskdesc>
85+
<desc>The following directives either allow wildcard sources (or ancestors), are not defined, or are overly broadly defined: script-src, script-src-elem, script-src-attr, style-src, style-src-elem, style-src-attr, img-src, connect-src, frame-src, font-src, media-src, object-src, manifest-src, worker-src, prefetch-src</desc>
86+
<instances>
87+
<instance>
88+
<uri>https://mainsite.com</uri>
89+
<method>GET</method>
90+
<param>Content-Security-Policy</param>
91+
<evidence>frame-ancestors &apos;self&apos;;</evidence>
92+
</instance>
93+
</instances>
94+
<count>1</count>
95+
<solution>Ensure that your web server, application server, load balancer, etc. is properly configured to set the Content-Security-Policy header.</solution>
96+
<reference>http://www.w3.org/TR/CSP2/http://www.w3.org/TR/CSP/http://caniuse.com/#search=content+security+policyhttp://content-security-policy.com/https://github.com/shapesecurity/salvation</reference>
97+
<cweid>16</cweid>
98+
<wascid>15</wascid>
99+
<sourceid>3</sourceid>
100+
</alertitem> -->
101+
<!-- <alertitem>
102+
<pluginid>10010</pluginid>
103+
<alert>zap4: Cookie No HttpOnly Flag</alert>
104+
<name>Cookie No HttpOnly Flag</name>
105+
<riskcode>1</riskcode>
106+
<confidence>2</confidence>
107+
<riskdesc>Low (Medium)</riskdesc>
108+
<desc>A cookie has been set without the HttpOnly flag, which means that the cookie can be accessed by JavaScript. If a malicious script can be run on this page then the cookie will be accessible and can be transmitted to another site. If this is a session cookie then session hijacking may be possible.</desc>
109+
<instances>
110+
<instance>
111+
<uri>https://mainsite.com</uri>
112+
<method>GET</method>
113+
<param>opvc</param>
114+
<evidence>Set-Cookie: opvc</evidence>
115+
</instance>
116+
<instance>
117+
<uri>https://mainsite.com</uri>
118+
<method>GET</method>
119+
<param>dmid</param>
120+
<evidence>Set-Cookie: dmid</evidence>
121+
</instance>
122+
<instance>
123+
<uri>https://mainsite.com</uri>
124+
<method>GET</method>
125+
<param>sitevisitscookie</param>
126+
<evidence>Set-Cookie: sitevisitscookie</evidence>
127+
</instance>
128+
</instances>
129+
<count>3</count>
130+
<solution>Ensure that the HttpOnly flag is set for all cookies.</solution>
131+
<reference>http://www.owasp.org/index.php/HttpOnly</reference>
132+
<cweid>16</cweid>
133+
<wascid>13</wascid>
134+
<sourceid>3</sourceid>
135+
</alertitem> -->
136+
<alertitem>
137+
<pluginid>10096</pluginid>
138+
<alert>zap5: Timestamp Disclosure - Unix</alert>
139+
<name>Timestamp Disclosure - Unix</name>
140+
<riskcode>1</riskcode>
141+
<confidence>1</confidence>
142+
<riskdesc>Low (Medium)</riskdesc>
143+
<desc>A timestamp was disclosed by the application/web server - Unix</desc>
144+
<instances>
145+
<instance>
146+
<uri>https://mainsite.com</uri>
147+
<method>GET</method>
148+
<evidence>265151019</evidence>
149+
</instance>
150+
<instance>
151+
<uri>https://mainsite.com</uri>
152+
<method>GET</method>
153+
<evidence>398525181</evidence>
154+
</instance>
155+
<instance>
156+
<uri>https://mainsite.com</uri>
157+
<method>GET</method>
158+
<evidence>153792000</evidence>
159+
</instance>
160+
<instance>
161+
<uri>https://mainsite.com/dashboard</uri>
162+
<method>GET</method>
163+
<evidence>1028274645</evidence>
164+
</instance>
165+
</instances>
166+
<count>4</count>
167+
<solution>Manually confirm that the timestamp data is not sensitive, and that the data cannot be aggregated to disclose exploitable patterns.</solution>
168+
<otherinfo>265151019, which evaluates to: 1978-05-27 22:03:39</otherinfo>
169+
<reference>https://www.owasp.org/index.php/Top_10_2013-A6-Sensitive_Data_Exposurehttp://projects.webappsec.org/w/page/13246936/Information%20Leakage</reference>
170+
<cweid>200</cweid>
171+
<wascid>13</wascid>
172+
<sourceid>3</sourceid>
173+
</alertitem>
174+
</alerts></site></OWASPZAPReport>

‎unittests/test_import_reimport.py‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ def __init__(self, *args, **kwargs):
6060
self.zap_sample1_filename = get_unit_tests_scans_path("zap") / "1_zap_sample_0_and_new_absent.xml"
6161
self.zap_sample2_filename = get_unit_tests_scans_path("zap") / "2_zap_sample_0_and_new_endpoint.xml"
6262
self.zap_sample3_filename = get_unit_tests_scans_path("zap") / "3_zap_sampl_0_and_different_severities.xml"
63+
self.zap_sample_without_zap3_filename = get_unit_tests_scans_path("zap") / "0_zap_sample_without_zap3.xml"
6364

6465
self.anchore_file_name = get_unit_tests_scans_path("anchore_engine") / "one_vuln_many_files.json"
6566
self.scan_type_anchore = "Anchore Engine Scan"
@@ -1573,6 +1574,110 @@ def test_import_reimport_keep_false_positive_and_out_of_scope(self):
15731574
self.assertFalse(finding["risk_accepted"])
15741575
self.assertFalse(finding["is_mitigated"])
15751576

1577+
def test_reimport_closes_risk_accepted_when_vulnerability_fixed(self):
1578+
"""Test that risk-accepted findings are closed when vulnerability no longer in scan"""
1579+
logger.debug("importing zap0, risk accepting zap3, reimporting without zap3, expecting zap3 to be closed")
1580+
1581+
# Import Zap0 with 4 findings (Zap1, Zap2, Zap3, Zap5)
1582+
import0 = self.import_scan_with_params(self.zap_sample0_filename)
1583+
test_id = import0["test"]
1584+
1585+
# Enable simple risk acceptance
1586+
test_api_response = self.get_test_api(test_id)
1587+
product_api_response = self.get_engagement_api(test_api_response["engagement"])
1588+
product_id = product_api_response["product"]
1589+
self.patch_product_api(product_id, {"enable_simple_risk_acceptance": True})
1590+
1591+
# Get all findings and risk accept Zap3
1592+
findings = self.get_test_findings_api(test_id, active=True)
1593+
self.assert_finding_count_json(4, findings)
1594+
1595+
zap3_id = None
1596+
for finding in findings["results"]:
1597+
if "Zap3" in finding["title"]:
1598+
# Risk accept via API (sets active=False, risk_accepted=True)
1599+
self.patch_finding_api(finding["id"], {
1600+
"risk_accepted": True,
1601+
"active": False,
1602+
})
1603+
zap3_id = finding["id"]
1604+
break
1605+
1606+
self.assertIsNotNone(zap3_id, "Zap3 finding should exist")
1607+
1608+
# Verify risk acceptance was applied
1609+
finding = self.get_finding_api(zap3_id)
1610+
self.assertTrue(finding["risk_accepted"])
1611+
self.assertFalse(finding["active"])
1612+
self.assertFalse(finding["is_mitigated"])
1613+
1614+
# Reimport scan WITHOUT Zap3 (vulnerability is fixed)
1615+
# This should close the risk-accepted finding and remove risk acceptance
1616+
# Zap1, Zap2, and Zap5 are still in the scan, so they are untouched
1617+
with assertTestImportModelsCreated(self, reimports=1, affected_findings=1, closed=1, untouched=3):
1618+
reimport0 = self.reimport_scan_with_params(test_id, self.zap_sample_without_zap3_filename)
1619+
1620+
self.assertEqual(reimport0["test"], test_id)
1621+
1622+
# Verify Zap3 is now mitigated and risk acceptance removed
1623+
finding = self.get_finding_api(zap3_id)
1624+
self.assertTrue(finding["is_mitigated"], "Finding should be mitigated when vulnerability is fixed")
1625+
self.assertFalse(finding["active"], "Finding should remain inactive")
1626+
self.assertFalse(finding["risk_accepted"], "Risk acceptance should be removed when vulnerability is fixed")
1627+
1628+
# Verify note was added explaining the closure
1629+
self.assertIsNotNone(finding.get("notes"))
1630+
1631+
def test_reimport_keeps_risk_accepted_when_still_in_scan(self):
1632+
"""Test that risk-accepted findings remain risk-accepted when still in scan (Scenario A)"""
1633+
logger.debug("importing zap0, risk accepting zap3, reimporting same scan, expecting zap3 to remain risk-accepted")
1634+
1635+
# Import Zap0 with 4 findings (Zap1, Zap2, Zap3, Zap5)
1636+
import0 = self.import_scan_with_params(self.zap_sample0_filename)
1637+
test_id = import0["test"]
1638+
1639+
# Enable simple risk acceptance
1640+
test_api_response = self.get_test_api(test_id)
1641+
product_api_response = self.get_engagement_api(test_api_response["engagement"])
1642+
product_id = product_api_response["product"]
1643+
self.patch_product_api(product_id, {"enable_simple_risk_acceptance": True})
1644+
1645+
# Get all findings and risk accept Zap3
1646+
findings = self.get_test_findings_api(test_id, active=True)
1647+
self.assert_finding_count_json(4, findings)
1648+
1649+
zap3_id = None
1650+
for finding in findings["results"]:
1651+
if "Zap3" in finding["title"]:
1652+
# Risk accept via API (sets active=False, risk_accepted=True)
1653+
self.patch_finding_api(finding["id"], {
1654+
"risk_accepted": True,
1655+
"active": False,
1656+
})
1657+
zap3_id = finding["id"]
1658+
break
1659+
1660+
self.assertIsNotNone(zap3_id, "Zap3 finding should exist")
1661+
1662+
# Verify risk acceptance was applied
1663+
finding = self.get_finding_api(zap3_id)
1664+
self.assertTrue(finding["risk_accepted"])
1665+
self.assertFalse(finding["active"])
1666+
self.assertFalse(finding["is_mitigated"])
1667+
1668+
# Reimport SAME scan (Zap3 is still present in scan)
1669+
# The risk-accepted finding should remain risk-accepted (NOT closed)
1670+
with assertTestImportModelsCreated(self, reimports=1, untouched=4):
1671+
reimport0 = self.reimport_scan_with_params(test_id, self.zap_sample0_filename)
1672+
1673+
self.assertEqual(reimport0["test"], test_id)
1674+
1675+
# Verify Zap3 is still risk-accepted (NOT closed)
1676+
finding = self.get_finding_api(zap3_id)
1677+
self.assertTrue(finding["risk_accepted"], "Finding should remain risk-accepted when still in scan")
1678+
self.assertFalse(finding["active"], "Finding should remain inactive")
1679+
self.assertFalse(finding["is_mitigated"], "Finding should NOT be mitigated when still risk-accepted and present in scan")
1680+
15761681
# import gitlab_dep_scan_components_filename with 6 findings
15771682
# findings 1, 2 and 3 have the same component_name (golang.org/x/crypto) and the same CVE (CVE-2020-29652), but different component_version
15781683
# findings 4 and 5 have the same component_name (golang.org/x/text) and the same CVE (CVE-2020-14040), but different component_version

‎unittests/test_importers_closeold.py‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from django.utils import timezone
44

5+
import dojo.risk_acceptance.helper as ra_helper
56
from dojo.importers.default_importer import DefaultImporter
67
from dojo.models import Development_Environment, Engagement, Product, Product_Type, User
78

@@ -171,3 +172,62 @@ def test_close_old_same_product_scan_matching_with_unique_id_from_tool(self):
171172
_, _, len_new_findings, len_closed_findings, _, _, _ = importer.process_scan(many_findings_scan)
172173
self.assertEqual(1, len_new_findings)
173174
self.assertEqual(1, len_closed_findings)
175+
176+
def test_close_old_closes_risk_accepted_findings(self):
177+
"""Test that close_old_findings closes risk-accepted findings when not in new scan"""
178+
scan_type = "Acunetix Scan"
179+
user, _ = User.objects.get_or_create(username="admin")
180+
product_type, _ = Product_Type.objects.get_or_create(name="closeold_risk")
181+
product, _ = Product.objects.get_or_create(
182+
name="TestCloseOldRiskAccepted",
183+
prod_type=product_type,
184+
)
185+
product.enable_simple_risk_acceptance = True
186+
product.save()
187+
188+
engagement, _ = Engagement.objects.get_or_create(
189+
name="Close Old Risk Accepted",
190+
product=product,
191+
target_start=timezone.now(),
192+
target_end=timezone.now(),
193+
)
194+
environment, _ = Development_Environment.objects.get_or_create(name="Development")
195+
import_options = {
196+
"user": user,
197+
"lead": user,
198+
"scan_date": None,
199+
"environment": environment,
200+
"active": True,
201+
"verified": False,
202+
"engagement": engagement,
203+
"scan_type": scan_type,
204+
}
205+
206+
# Import many findings
207+
with (get_unit_tests_scans_path("acunetix") / "many_findings.xml").open(encoding="utf-8") as scan:
208+
importer = DefaultImporter(close_old_findings=False, **import_options)
209+
test, _, len_new, len_closed, _, _, _ = importer.process_scan(scan)
210+
self.assertEqual(4, len_new)
211+
self.assertEqual(0, len_closed)
212+
213+
# Risk accept one finding
214+
finding_to_accept = test.finding_set.first()
215+
ra_helper.simple_risk_accept(user, finding_to_accept)
216+
finding_to_accept.refresh_from_db()
217+
self.assertTrue(finding_to_accept.risk_accepted)
218+
self.assertFalse(finding_to_accept.active)
219+
220+
# Import scan with only one finding (different from risk-accepted one)
221+
# close_old_findings should close the risk-accepted finding
222+
with (get_unit_tests_scans_path("acunetix") / "one_finding.xml").open(encoding="utf-8") as scan:
223+
importer = DefaultImporter(close_old_findings=True, **import_options)
224+
_, _, len_new, len_closed, _, _, _ = importer.process_scan(scan)
225+
self.assertEqual(1, len_new)
226+
# At least 3 findings should be closed (including the risk-accepted one)
227+
# The exact number depends on deduplication, but we verify below
228+
self.assertGreaterEqual(len_closed, 3)
229+
230+
# Verify risk-accepted finding was closed
231+
finding_to_accept.refresh_from_db()
232+
self.assertTrue(finding_to_accept.is_mitigated, "Risk-accepted finding should be mitigated when vulnerability is fixed")
233+
self.assertFalse(finding_to_accept.risk_accepted, "Risk acceptance should be removed when vulnerability is fixed")

0 commit comments

Comments
 (0)