diff --git a/docs/content/releases/os_upgrading/3.0.md b/docs/content/releases/os_upgrading/3.0.md index 95e0ce984ea..cf0df405a99 100644 --- a/docs/content/releases/os_upgrading/3.0.md +++ b/docs/content/releases/os_upgrading/3.0.md @@ -2,7 +2,7 @@ title: 'Upgrading to DefectDojo Version 3.0.x' toc_hide: true weight: -20260615 -description: Locations and Asset/Organization labels are now enabled by default; Authorized Users panel replaces Members/Groups under legacy authorization; SSO providers move to DefectDojo Pro; removal of Questionnaire API Endpoints, Credential Manager, and Stub Findings +description: Locations and Asset/Organization labels are now enabled by default; Authorized Users panel replaces Members/Groups under legacy authorization; SSO providers move to DefectDojo Pro; removal of Questionnaire API Endpoints, Credential Manager, and Stub Findings; Dependency Check parser no longer emits separate findings for related dependencies; related file paths are now listed in the main finding's description. --- ## Locations enabled by default @@ -146,5 +146,28 @@ Any requests to this endpoint will now return a 404 Not Found error. The Stub Fi In [PR 14881](https://github.com/DefectDojo/django-DefectDojo/pull/14881)We optimized the way the Django Watson search index is updated during imports and reimports. There is not a single configuration setting to manage the threshold: `DD_WATSON_ASYNC_INDEX_UPDATE_BATCH_SIZE`. The default value should work fine for most instances. +## Dependency Check parser: related dependencies folded into the main finding -For more information, check the [Release Notes](https://github.com/DefectDojo/django-DefectDojo/releases/tag/3.0.0). +The Dependency Check parser previously created one finding per `` in the report, in addition to the finding for the main vulnerable dependency. Because OWASP Dependency-Check attaches the vulnerability only to the main dependency in the XML and the related entries are metadata pointing to other files in the same logical component, this produced N findings sharing the same title, CVE, component name and component version — only the file path differed. For projects with Spring Boot, ActiveMQ, or other libraries whose CPE matches many sibling artifacts this produced significant noise. + +Starting in 2.59.1, the parser emits exactly one finding per vulnerability per main dependency. The file paths of any related dependencies are surfaced in the finding description under a `**Related Filepaths:**` block. + +### Background: what `` actually contains + +OWASP Dependency-Check's `DependencyBundlingAnalyzer` merges co-grouped artifacts into a single main dependency and lists the others under ``. It does this under five scenarios: + +1. **Identical content (`hashesMatch`)** — the same jar (matching sha1) found at multiple paths, e.g. the same library packaged into multiple ear/war archives. +2. **Shaded jar (`isShadedJar`)** — a `.jar` and a `pom.xml` extracted from inside it share the same CPE; the pom.xml is recorded as related. +3. **WebJar (`isWebJar`)** — a `.js` file extracted from a WebJar matches the jar's CPE (mapped via `pkg:maven/org.webjars/@`); the js file is recorded as related to the jar. +4. **Same CPE + base path + vulnerabilities + filename match** — sibling artifacts in the same project that share a CPE. Example: `spring-boot`, `spring-boot-actuator`, `spring-boot-starter-jdbc`, etc. all map to the `spring_boot` CPE and are grouped under the main `spring-boot` jar. +5. **NPM same name + version** — the same npm package discovered via different resolution paths (for example `package-lock.json` plus `node_modules`). + +Only scenario 1 represents the same vulnerable artifact at multiple deploy locations. Scenarios 2-5 are different files representing one logical component. Both cases were previously inflated into separate findings; both now collapse to one finding with the related paths listed in the description. + +### Required actions + +- **Users filtering or grouping by the `related` tag**: that tag is no longer applied because related findings are no longer created. Update any saved filters, dashboards, or rules that depend on it. Equivalent information is now available in the finding description (look for `**Related Filepaths:**`). +- **Reimport behavior**: on the next reimport of an existing Dependency Check report, the previously-created `related` findings will be closed as no longer present in the report. This is expected and matches the new parsing behavior. + + +For more information, check the [Release Notes](https://github.com/DefectDojo/django-DefectDojo/releases/tag/3.0.0). \ No newline at end of file diff --git a/docs/content/supported_tools/parsers/file/dependency_check.md b/docs/content/supported_tools/parsers/file/dependency_check.md index f4f56ce8325..76d51277801 100644 --- a/docs/content/supported_tools/parsers/file/dependency_check.md +++ b/docs/content/supported_tools/parsers/file/dependency_check.md @@ -7,7 +7,21 @@ OWASP Dependency Check output can be imported in Xml format. This parser ingests * Suppressed vulnerabilities are tagged with the tag: `suppressed`. * Suppressed vulnerabilities are marked as mitigated. * If the suppression is missing any `` tag, it tags them as `no_suppression_document`. -* Related vulnerable dependencies are tagged with `related` tag. + +### Related dependencies + +OWASP Dependency-Check's `DependencyBundlingAnalyzer` merges co-grouped artifacts into a single main dependency and lists the others under `` in the report. The vulnerability is attached only to the main dependency; the related entries are metadata pointing to other files in the same logical component. + +The parser emits one finding per vulnerability per main dependency and surfaces the related file paths in the finding's description under a `**Related Filepaths:**` block (rather than creating a separate finding per related entry, which produced N findings sharing the same title and CVE differing only in file path). + +DC bundles dependencies under five scenarios: + +1. **Identical content (`hashesMatch`)** — the same jar (matching sha1) found at multiple paths (e.g. packaged into multiple ear/war archives). +2. **Shaded jar (`isShadedJar`)** — a `.jar` and a `pom.xml` extracted from inside it share the same CPE. +3. **WebJar (`isWebJar`)** — a `.js` file extracted from a WebJar maps to the jar's CPE via `pkg:maven/org.webjars/@`. +4. **Same CPE + base path + vulnerabilities + filename match** — sibling artifacts sharing a CPE (e.g. `spring-boot`, `spring-boot-actuator`, `spring-boot-starter-jdbc` all map to the `spring_boot` CPE). +5. **NPM same name + version** — the same npm package discovered via different resolution paths (e.g. `package-lock.json` + `node_modules`). + ### Sample Scan Data Sample Dependency Check scans can be found [here](https://github.com/DefectDojo/django-DefectDojo/tree/master/unittests/scans/dependency_check). diff --git a/dojo/admin.py b/dojo/admin.py index c7a21b91019..94d149af035 100644 --- a/dojo/admin.py +++ b/dojo/admin.py @@ -14,6 +14,20 @@ TextQuestion, ) + +# Django's default admin gate is `is_active and is_staff`. Under the legacy +# OS auth model is_staff is already a near-superuser bypass for queryset +# filters and most permission checks, so the standard UserAdmin change form +# would let any is_staff user with auth.change_user tick is_superuser on +# themselves. Rather than narrowing each ModelAdmin individually, restrict +# the entire admin site to superusers — there is no DefectDojo OS role that +# legitimately needs /admin/ access without being a superuser. +def _admin_site_has_permission(request): + return request.user.is_active and request.user.is_superuser + + +admin.site.has_permission = _admin_site_has_permission + # Conditionally unregister LogEntry from auditlog if it's registered try: from auditlog.models import LogEntry diff --git a/dojo/api_v2/serializers.py b/dojo/api_v2/serializers.py index dc15ac6c2dc..b3b83a239b5 100644 --- a/dojo/api_v2/serializers.py +++ b/dojo/api_v2/serializers.py @@ -613,6 +613,12 @@ def validate(self, data): msg = "Only superusers are allowed to add or edit superusers." raise ValidationError(msg) + instance_is_staff = self.instance.is_staff if self.instance is not None else False + data_is_staff = data.get("is_staff", instance_is_staff) + if not self.context["request"].user.is_superuser and data_is_staff != instance_is_staff: + msg = "Only superusers are allowed to add or edit staff users." + raise ValidationError(msg) + if self.context["request"].method in {"PATCH", "PUT"} and "password" in data: msg = "Update of password though API is not allowed" raise ValidationError(msg) diff --git a/dojo/finding/helper.py b/dojo/finding/helper.py index 7808fdd7cf5..140a6bd426d 100644 --- a/dojo/finding/helper.py +++ b/dojo/finding/helper.py @@ -603,6 +603,7 @@ def reconfigure_duplicate_cluster(original, cluster_outside): duplicate=False, duplicate_finding=None, active=original.active, + verified=original.verified, is_mitigated=original.is_mitigated, ) new_original.found_by.set(original.found_by.all()) diff --git a/dojo/forms.py b/dojo/forms.py index e33d7ef51c4..cb90d0f57de 100644 --- a/dojo/forms.py +++ b/dojo/forms.py @@ -2176,16 +2176,13 @@ class ReviewFindingForm(forms.Form): def __init__(self, *args, **kwargs): finding = kwargs.pop("finding", None) - user = kwargs.pop("user", None) + kwargs.pop("user", None) super().__init__(*args, **kwargs) # Get the list of users if finding is not None: users = get_authorized_users_for_product_and_product_type(None, finding.test.engagement.product, "edit") else: users = get_authorized_users("edit").filter(is_active=True) - # Remove the current user - if user is not None: - users = users.exclude(id=user.id) # Save a copy of the original query to be used in the validator self.reviewer_queryset = users # Set the users in the form diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index aa33c6153b0..e9c6567107a 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -801,9 +801,16 @@ def process_matched_mitigated_finding( existing_finding.mitigated = None existing_finding.is_mitigated = False existing_finding.mitigated_by = None - existing_finding.active = True - if self.verified is not None: - existing_finding.verified = self.verified + # A duplicate finding must stay inactive/unverified (see set_duplicate and the + # "Duplicate findings cannot be verified or active" form validation). Un-mitigate it + # but do not reactivate it, otherwise we create an invalid active/verified duplicate state. + if existing_finding.duplicate: + existing_finding.active = False + existing_finding.verified = False + else: + existing_finding.active = True + if self.verified is not None: + existing_finding.verified = self.verified component_name = getattr(unsaved_finding, "component_name", None) component_version = getattr(unsaved_finding, "component_version", None) @@ -819,7 +826,11 @@ def process_matched_mitigated_finding( # don't dedupe before endpoints/locations are added, postprocessing will be done on next save (in calling method) existing_finding.save_no_options() - note = Notes(entry=f"Re-activated by {self.scan_type} re-upload.", author=self.user) + if existing_finding.duplicate: + note_entry = f"Un-mitigated by {self.scan_type} re-upload but kept inactive because the finding is a duplicate." + else: + note_entry = f"Re-activated by {self.scan_type} re-upload." + note = Notes(entry=note_entry, author=self.user) note.save() self.location_handler.record_reactivations_for_finding(existing_finding) existing_finding.notes.add(note) diff --git a/dojo/settings/settings.dist.py b/dojo/settings/settings.dist.py index ce0966da78e..e0edbf0d192 100644 --- a/dojo/settings/settings.dist.py +++ b/dojo/settings/settings.dist.py @@ -51,7 +51,7 @@ DD_DJANGO_METRICS_ENABLED=(bool, False), DD_LOGIN_REDIRECT_URL=(str, "/"), DD_LOGIN_URL=(str, "/login"), - DD_DJANGO_ADMIN_ENABLED=(bool, True), + DD_DJANGO_ADMIN_ENABLED=(bool, False), DD_SESSION_COOKIE_HTTPONLY=(bool, True), DD_CSRF_COOKIE_HTTPONLY=(bool, True), DD_SECURE_SSL_REDIRECT=(bool, False), diff --git a/dojo/settings/template-env b/dojo/settings/template-env index bbfc9e90ad9..0263dc941a7 100644 --- a/dojo/settings/template-env +++ b/dojo/settings/template-env @@ -1,8 +1,9 @@ # Django Debug, don't enable on production! DD_DEBUG=False -# Enables Django Admin -DD_DJANGO_ADMIN_ENABLED=True +# Enables the Django admin interface at /admin/. Off by default; uncomment +# this line to expose it. Access is restricted to superusers regardless. +# DD_DJANGO_ADMIN_ENABLED=True # A secret key for a particular Django installation. DD_SECRET_KEY=#DD_SECRET_KEY# diff --git a/dojo/templates/dojo/metrics.html b/dojo/templates/dojo/metrics.html index 6e56099463f..2c7ab40fbf6 100644 --- a/dojo/templates/dojo/metrics.html +++ b/dojo/templates/dojo/metrics.html @@ -147,9 +147,11 @@

+ {% if form %}
{% include "dojo/filter_snippet.html" with form=form clear_link="/metrics/product/type" %}
+ {% endif %} diff --git a/dojo/templates_classic/dojo/metrics.html b/dojo/templates_classic/dojo/metrics.html index e011fc21055..777467e0e59 100644 --- a/dojo/templates_classic/dojo/metrics.html +++ b/dojo/templates_classic/dojo/metrics.html @@ -155,9 +155,11 @@

+ {% if form %}
{% include "dojo/filter_snippet.html" with form=form clear_link="/metrics/product/type" %}
+ {% endif %} diff --git a/dojo/tools/dependency_check/parser.py b/dojo/tools/dependency_check/parser.py index 4c472e2f4c4..03590bdd02d 100644 --- a/dojo/tools/dependency_check/parser.py +++ b/dojo/tools/dependency_check/parser.py @@ -86,6 +86,57 @@ def add_finding(self, finding, dupes): if key not in dupes: dupes[key] = finding + def build_related_dependencies_block(self, dependency, namespace): + """ + Return a markdown block listing related dependencies, or ''. + + Dependency-Check's DependencyBundlingAnalyzer merges co-grouped artifacts + into one main dependency and lists the others under . + The vulnerability is attached only to the main dependency in the XML; the + related entries are metadata pointing to other files in the same logical + component. Previously we emitted one finding per related entry, which + multiplied a single CVE into N findings sharing the same title and CVE + with only the file_path differing — pure noise for the user. Instead we + keep one finding for the main dependency and surface the related file + paths in its description. + + DC bundles dependencies under five scenarios (see + DependencyBundlingAnalyzer.evaluateDependencies): + + 1. hashesMatch — identical content (same sha1) found at multiple paths, + e.g. the same jar packaged into multiple ear/war archives. + 2. isShadedJar — a .jar and a pom.xml extracted from inside it share the + same CPE; the pom.xml is recorded as related to the jar. + 3. isWebJar — a .js file extracted from a WebJar matches the jar's CPE + (mapped via pkg:maven/org.webjars/@); the js is + related to the jar. + 4. CPE + base path + vulnerabilities + filename match — sibling artifacts + in the same project that share a CPE (e.g. spring-boot, + spring-boot-actuator, spring-boot-starter all map to the + spring_boot CPE). + 5. NPM same name + version — the same npm package discovered via + different resolution paths (e.g. package-lock.json + node_modules). + + Scenario 1 is the only case where the related entries are genuinely + separate vulnerable locations; scenarios 2-5 are different files + representing one logical component. Listing all related paths in the + description preserves the per-location information for scenario 1 while + removing the noise from scenarios 2-5. + """ + related = dependency.find(namespace + "relatedDependencies") + if related is None: + return "" + entries = [] + for rd in related.findall(namespace + "relatedDependency"): + file_name = rd.findtext(f"{namespace}fileName") + if not file_name: + continue + file_path = (rd.findtext(f"{namespace}filePath") or "").strip() + entries.append(f"- {file_name} ({file_path})" if file_path else f"- {file_name}") + if not entries: + return "" + return "\n**Related Filepaths:**\n" + "\n".join(entries) + def get_filename_and_path_from_dependency( self, dependency, @@ -448,6 +499,7 @@ def get_findings(self, filename, test): namespace + "vulnerabilities", ) if vulnerabilities is not None: + related_block = self.build_related_dependencies_block(dependency, namespace) for vulnerability in vulnerabilities.findall( namespace + "vulnerability", ): @@ -459,29 +511,12 @@ def get_findings(self, filename, test): test, namespace, ) + if related_block: + finding.description += related_block if scan_date: finding.date = scan_date self.add_finding(finding, dupes) - relatedDependencies = dependency.find( - namespace + "relatedDependencies", - ) - if relatedDependencies is not None: - for relatedDependency in relatedDependencies.findall( - namespace + "relatedDependency", - ): - finding = self.get_finding_from_vulnerability( - dependency, - relatedDependency, - vulnerability, - test, - namespace, - ) - if finding: # could be None - if scan_date: - finding.date = scan_date - self.add_finding(finding, dupes) - for suppressedVulnerability in vulnerabilities.findall( namespace + "suppressedVulnerability", ): @@ -493,6 +528,8 @@ def get_findings(self, filename, test): test, namespace, ) + if related_block: + finding.description += related_block if scan_date: finding.date = scan_date self.add_finding(finding, dupes) diff --git a/unittests/test_adminsite.py b/unittests/test_adminsite.py index 015c65ee467..70f08da8df2 100644 --- a/unittests/test_adminsite.py +++ b/unittests/test_adminsite.py @@ -1,5 +1,8 @@ import django.apps from django.contrib import admin +from django.contrib.auth import get_user_model +from django.contrib.auth.models import AnonymousUser +from django.http import HttpRequest from .dojo_test_case import DojoTestCase @@ -20,3 +23,56 @@ def test_is_model_defined(self): else: with self.subTest(type="tag", subclass=subclass): self.assertIn(subclass, admin.site._registry.keys(), f"{subclass} is not registered in 'tagulous.admin' in models.py") + + +class AdminAccessGate(DojoTestCase): + + """ + is_staff is a near-superuser bypass under the legacy OS auth model, so + /admin/ must require is_superuser. Django's default UserAdmin change + form would otherwise let any is_staff user with auth.change_user tick + is_superuser on themselves. Tested at the gate-function level so the + assertions hold regardless of whether DD_DJANGO_ADMIN_ENABLED mounts + the admin URLConf in the current environment. + """ + + @staticmethod + def _request_for(user): + req = HttpRequest() + req.user = user + return req + + def test_staff_non_superuser_denied(self): + User = get_user_model() + password = "testTEST1234!@#$" + staff = User.objects.create_user( + username="staff-no-root", password=password, is_staff=True, + ) + self.assertFalse(admin.site.has_permission(self._request_for(staff))) + + def test_non_staff_non_superuser_denied(self): + User = get_user_model() + password = "testTEST1234!@#$" + plain = User.objects.create_user(username="plain-user", password=password) + self.assertFalse(admin.site.has_permission(self._request_for(plain))) + + def test_anonymous_denied(self): + self.assertFalse(admin.site.has_permission(self._request_for(AnonymousUser()))) + + def test_inactive_superuser_denied(self): + User = get_user_model() + password = "testTEST1234!@#$" + root = User.objects.create_superuser( + username="inactive-root", email="i@example.com", password=password, + ) + root.is_active = False + root.save(update_fields=["is_active"]) + self.assertFalse(admin.site.has_permission(self._request_for(root))) + + def test_active_superuser_allowed(self): + User = get_user_model() + password = "testTEST1234!@#$" + root = User.objects.create_superuser( + username="root-test", email="r@example.com", password=password, + ) + self.assertTrue(admin.site.has_permission(self._request_for(root))) diff --git a/unittests/test_apiv2_user.py b/unittests/test_apiv2_user.py index afce21948f2..f6273fe9b47 100644 --- a/unittests/test_apiv2_user.py +++ b/unittests/test_apiv2_user.py @@ -1,3 +1,4 @@ +from django.contrib.auth.models import Permission from django.urls import reverse from django.utils import timezone from rest_framework.authtoken.models import Token @@ -201,6 +202,118 @@ def test_user_reset_api_token_denies_non_privileged(self): r = nonpriv_client.post(url) self.assertEqual(r.status_code, 403, r.content[:1000]) + def test_non_superuser_cannot_set_is_staff_via_api(self): + """ + A delegated user-manager (auth.change_user) must not be able to + flip is_staff on themselves or anyone else — is_staff is a + superuser-only flag under the legacy OS auth model, and granting + it via API would let a non-superuser pivot into Django admin / + full RBAC bypass. + """ + password = "testTEST1234!@#$" + r = self.client.post(reverse("user-list"), { + "username": "api-user-mgr", + "email": "admin@dojo.com", + "password": password, + }, format="json") + self.assertEqual(r.status_code, 201, r.content[:1000]) + mgr = User.objects.get(username="api-user-mgr") + mgr.user_permissions.add( + Permission.objects.get(codename="change_user"), + Permission.objects.get(codename="add_user"), + ) + + token_resp = self.client.post(reverse("api-token-auth"), { + "username": "api-user-mgr", + "password": password, + }, format="json") + self.assertEqual(token_resp.status_code, 200, token_resp.content[:1000]) + mgr_client = APIClient() + mgr_client.credentials(HTTP_AUTHORIZATION="Token " + token_resp.json()["token"]) + + # Self-escalation: setting is_staff on own account must be rejected. + r = mgr_client.patch("{}{}/".format(reverse("user-list"), mgr.id), { + "is_staff": True, + }, format="json") + self.assertEqual(r.status_code, 400, r.content[:1000]) + self.assertIn( + "Only superusers are allowed to add or edit staff users.", + r.content.decode("utf-8"), + ) + mgr.refresh_from_db() + self.assertFalse(mgr.is_staff) + + # Target-escalation: setting is_staff on another user must be rejected. + r = self.client.post(reverse("user-list"), { + "username": "api-user-target", + "email": "admin@dojo.com", + "password": password, + }, format="json") + self.assertEqual(r.status_code, 201, r.content[:1000]) + target_id = r.json()["id"] + + r = mgr_client.patch("{}{}/".format(reverse("user-list"), target_id), { + "is_staff": True, + }, format="json") + self.assertEqual(r.status_code, 400, r.content[:1000]) + target = User.objects.get(id=target_id) + self.assertFalse(target.is_staff) + + # Create-time escalation must also be rejected. + r = mgr_client.post(reverse("user-list"), { + "username": "api-user-staff-on-create", + "email": "admin@dojo.com", + "password": password, + "is_staff": True, + }, format="json") + self.assertEqual(r.status_code, 400, r.content[:1000]) + self.assertFalse(User.objects.filter(username="api-user-staff-on-create").exists()) + + def test_non_superuser_can_patch_self_without_touching_is_staff(self): + """ + Negative control for the is_staff guard: a delegated user-manager + can still PATCH non-privileged fields on their own account; the + new check only fires when is_staff actually changes. + """ + password = "testTEST1234!@#$" + r = self.client.post(reverse("user-list"), { + "username": "api-user-mgr2", + "email": "admin@dojo.com", + "password": password, + }, format="json") + self.assertEqual(r.status_code, 201, r.content[:1000]) + mgr = User.objects.get(username="api-user-mgr2") + mgr.user_permissions.add(Permission.objects.get(codename="change_user")) + + token_resp = self.client.post(reverse("api-token-auth"), { + "username": "api-user-mgr2", + "password": password, + }, format="json") + self.assertEqual(token_resp.status_code, 200, token_resp.content[:1000]) + mgr_client = APIClient() + mgr_client.credentials(HTTP_AUTHORIZATION="Token " + token_resp.json()["token"]) + + r = mgr_client.patch("{}{}/".format(reverse("user-list"), mgr.id), { + "first_name": "Renamed", + }, format="json") + self.assertEqual(r.status_code, 200, r.content[:1000]) + + def test_superuser_can_set_is_staff_via_api(self): + """Positive control: a superuser is still allowed to toggle is_staff.""" + r = self.client.post(reverse("user-list"), { + "username": "api-user-promotable", + "email": "admin@dojo.com", + "password": "testTEST1234!@#$", + }, format="json") + self.assertEqual(r.status_code, 201, r.content[:1000]) + user_id = r.json()["id"] + + r = self.client.patch("{}{}/".format(reverse("user-list"), user_id), { + "is_staff": True, + }, format="json") + self.assertEqual(r.status_code, 200, r.content[:1000]) + self.assertTrue(User.objects.get(id=user_id).is_staff) + def test_user_reset_api_token_denies_global_owner_legacy(self): """ Legacy: Global_Role(role=Owner) is inert. Resetting another diff --git a/unittests/test_importers_importer.py b/unittests/test_importers_importer.py index aa14ace8beb..ad9dd8f66c3 100644 --- a/unittests/test_importers_importer.py +++ b/unittests/test_importers_importer.py @@ -932,4 +932,115 @@ def test_change_vulnerability_ids_on_reimport(self): # Verify only new Vulnerability_Id objects exist vuln_ids = list(Vulnerability_Id.objects.filter(finding=finding).values_list("vulnerability_id", flat=True)) self.assertEqual(set(new_vulnerability_ids), set(vuln_ids)) - finding.delete() + + +class ReimportDuplicateReactivationTest(DojoTestCase): + + """ + Regression test for https://github.com/DefectDojo/django-DefectDojo/issues/14910 + + Reimport reactivation of a mitigated finding must not produce an invalid + active/verified duplicate finding state. + """ + + def setUp(self): + self.user, _ = User.objects.get_or_create(username="admin", is_superuser=True) + Development_Environment.objects.get_or_create(name="Development") + self.product_type, _ = Product_Type.objects.get_or_create(name="dup_reactivation_pt") + self.product, _ = Product.objects.get_or_create( + name="DupReactivationProduct", + description="test product", + prod_type=self.product_type, + ) + self.engagement = Engagement.objects.create( + name="Dup Reactivation Engagement", + product=self.product, + target_start=timezone.now(), + target_end=timezone.now(), + ) + self.test = self.create_test(engagement=self.engagement, scan_type=NPM_AUDIT_SCAN_TYPE, title="dup reactivation test") + + def _make_finding(self, title, **kwargs): + return Finding.objects.create( + title=title, + test=self.test, + severity="High", + reporter=self.user, + **kwargs, + ) + + def test_reactivation_keeps_duplicate_inactive_and_unverified(self): + # Original active finding + original = self._make_finding("original finding", active=True, verified=True) + # Mitigated finding that is marked as a duplicate of the original + existing_duplicate = self._make_finding( + "duplicate finding", + active=False, + verified=False, + duplicate=True, + duplicate_finding=original, + is_mitigated=True, + mitigated=timezone.now(), + mitigated_by=self.user, + ) + # The reimported (unsaved) finding that re-matches the duplicate, and is active/not mitigated + unsaved_finding = self._make_finding("duplicate finding incoming", active=True, verified=True) + + reimporter = DefaultReImporter( + test=self.test, + user=self.user, + scan_type=NPM_AUDIT_SCAN_TYPE, + active=True, + verified=True, + do_not_reactivate=False, + ) + # These accumulators are normally initialised inside process_findings(); set them + # here because the test drives process_matched_mitigated_finding() directly. + reimporter.new_items = [] + reimporter.reactivated_items = [] + reimporter.unchanged_items = [] + + result_finding, _ = reimporter.process_matched_mitigated_finding(unsaved_finding, existing_duplicate) + + result_finding.refresh_from_db() + # The mitigation is cleared (the finding reappeared in the scan)... + self.assertFalse(result_finding.is_mitigated) + self.assertIsNone(result_finding.mitigated) + # ...but a duplicate must never become active or verified (issue #14910) + self.assertTrue(result_finding.duplicate) + self.assertFalse(result_finding.active) + self.assertFalse(result_finding.verified) + + def test_reactivation_of_non_duplicate_still_activates(self): + # A regular mitigated finding (not a duplicate) must still reactivate as before + existing = self._make_finding( + "regular finding", + active=False, + verified=False, + is_mitigated=True, + mitigated=timezone.now(), + mitigated_by=self.user, + ) + unsaved_finding = self._make_finding("regular finding incoming", active=True, verified=True) + + reimporter = DefaultReImporter( + test=self.test, + user=self.user, + scan_type=NPM_AUDIT_SCAN_TYPE, + active=True, + verified=True, + do_not_reactivate=False, + ) + # These accumulators are normally initialised inside process_findings(); set them + # here because the test drives process_matched_mitigated_finding() directly. + reimporter.new_items = [] + reimporter.reactivated_items = [] + reimporter.unchanged_items = [] + + result_finding, _ = reimporter.process_matched_mitigated_finding(unsaved_finding, existing) + + result_finding.refresh_from_db() + self.assertFalse(result_finding.is_mitigated) + self.assertIsNone(result_finding.mitigated) + self.assertTrue(result_finding.active) + self.assertTrue(result_finding.verified) diff --git a/unittests/test_prepare_duplicates_for_delete.py b/unittests/test_prepare_duplicates_for_delete.py index 78d612dfdec..89ed84069db 100644 --- a/unittests/test_prepare_duplicates_for_delete.py +++ b/unittests/test_prepare_duplicates_for_delete.py @@ -294,15 +294,56 @@ def test_multiple_originals(self): self.assertFalse(dupe_of_b.duplicate) self.assertIsNone(dupe_of_b.duplicate_finding) - def test_original_status_copied_to_new_original(self): - """New original inherits active/is_mitigated status from deleted original.""" + def test_original_status_copied_to_new_original_active_verified(self): + """ + New original inherits active/verified/is_mitigated from deleted original. + + Positive case: original is an open, verified, not-mitigated finding. + Duplicate starts with the opposite of each field so every copy is observable. + + Regression test for issue #14911: promoted duplicates kept verified=False + even when the original was verified, blocking Jira "Push All Issues". + """ + original = self._create_finding(self.test1, "Original") + original.active = True + original.verified = True + original.is_mitigated = False + super(Finding, original).save(skip_validation=True) + + outside_dupe = self._create_finding(self.test2, "Outside Dupe") + outside_dupe.is_mitigated = True + super(Finding, outside_dupe).save(skip_validation=True) + self._make_duplicate(outside_dupe, original) # forces active=False, verified default False + + with impersonate(self.testuser): + prepare_duplicates_for_delete(self.test1) + + outside_dupe.refresh_from_db() + self.assertFalse(outside_dupe.duplicate) + self.assertTrue(outside_dupe.active) + self.assertTrue(outside_dupe.verified) + self.assertFalse(outside_dupe.is_mitigated) + + def test_original_status_copied_to_new_original_inactive_mitigated(self): + """ + New original inherits active/verified/is_mitigated from deleted original. + + Negative case: original is closed, unverified, mitigated. + Duplicate starts with the opposite of each field so every copy is observable. + """ original = self._create_finding(self.test1, "Original") original.active = False + original.verified = False original.is_mitigated = True super(Finding, original).save(skip_validation=True) outside_dupe = self._create_finding(self.test2, "Outside Dupe") + outside_dupe.verified = True + super(Finding, outside_dupe).save(skip_validation=True) self._make_duplicate(outside_dupe, original) + # _make_duplicate forces active=False; flip to True so the copy is observable + outside_dupe.active = True + super(Finding, outside_dupe).save(skip_validation=True) with impersonate(self.testuser): prepare_duplicates_for_delete(self.test1) @@ -310,6 +351,7 @@ def test_original_status_copied_to_new_original(self): outside_dupe.refresh_from_db() self.assertFalse(outside_dupe.duplicate) self.assertFalse(outside_dupe.active) + self.assertFalse(outside_dupe.verified) self.assertTrue(outside_dupe.is_mitigated) def test_found_by_copied_to_new_original(self): diff --git a/unittests/tools/test_dependency_check_parser.py b/unittests/tools/test_dependency_check_parser.py index 31e1394ec51..67349f25871 100644 --- a/unittests/tools/test_dependency_check_parser.py +++ b/unittests/tools/test_dependency_check_parser.py @@ -58,11 +58,11 @@ def test_parse_file_with_multiple_vulnerabilities_has_multiple_findings(self): parser = DependencyCheckParser() findings = parser.get_findings(testfile, Test()) items = findings - self.assertEqual(11, len(items)) + self.assertEqual(9, len(items)) # test also different component_name formats with self.subTest(i=0): - # identifier -> package url java + 2 relateddependencies + # identifier -> package url java + 2 relateddependencies (now folded into description) self.assertEqual(items[0].title, "org.dom4j:dom4j:2.1.1.redhat-00001 | CVE-0000-0001") self.assertEqual(items[0].component_name, "org.dom4j:dom4j") self.assertEqual(items[0].component_version, "2.1.1.redhat-00001") @@ -74,6 +74,15 @@ def test_parse_file_with_multiple_vulnerabilities_has_multiple_findings(self): "/var/lib/adapter-ear1.ear/dom4j-2.1.1.jar", items[0].description, ) + self.assertIn("**Related Filepaths:**", items[0].description) + self.assertIn( + "adapter-ear8.ear: dom4j-2.1.1.jar (/var/lib/adapter-ear8.ear/dom4j-2.1.1.jar)", + items[0].description, + ) + self.assertIn( + "adapter-ear1.ear: dom4j-extensions-2.1.1.jar (/var/lib/adapter-ear1.ear/dom4j-extensions-2.1.1.jar)", + items[0].description, + ) self.assertEqual(items[0].severity, "High") self.assertEqual(items[0].cvssv3, None) self.assertEqual(items[0].cvssv3_score, None) @@ -89,192 +98,143 @@ def test_parse_file_with_multiple_vulnerabilities_has_multiple_findings(self): self.assertEqual("CVE-0000-0001", items[0].unsaved_vulnerability_ids[0]) with self.subTest(i=1): - self.assertEqual(items[1].title, "org.dom4j:dom4j:2.1.1.redhat-00001 | CVE-0000-0001") - self.assertEqual(items[1].component_name, "org.dom4j:dom4j") - self.assertEqual(items[1].component_version, "2.1.1.redhat-00001") - self.assertIn( - "Description of a bad vulnerability.", - items[1].description, - ) - self.assertIn( - "/var/lib/adapter-ear8.ear/dom4j-2.1.1.jar", - items[1].description, + # identifier -> package url javascript, no vulnerabilitids, 3 vulnerabilities, relateddependencies without filename (pre v6.0.0) + self.assertEqual( + items[1].title, "yargs-parser:5.0.0 | 1500", ) - self.assertEqual(items[1].severity, "High") + self.assertEqual(items[1].component_name, "yargs-parser") + self.assertEqual(items[1].component_version, "5.0.0") + self.assertEqual(items[1].severity, "Low") self.assertEqual(items[1].cvssv3, None) self.assertEqual(items[1].cvssv3_score, None) - self.assertEqual(items[1].file_path, "adapter-ear8.ear: dom4j-2.1.1.jar") + self.assertEqual(items[1].file_path, "yargs-parser:5.0.0") self.assertEqual( - items[1].mitigation, - "Update org.dom4j:dom4j:2.1.1.redhat-00001 to at least the version recommended in the description", - ) - self.assertEqual(items[1].unsaved_tags, ["related"]) - self.assertEqual(1, len(items[1].unsaved_vulnerability_ids)) - self.assertEqual("CVE-0000-0001", items[1].unsaved_vulnerability_ids[0]) - - with self.subTest(i=2): - self.assertEqual(items[2].title, "org.dom4j:dom4j:2.1.1.redhat-00001 | CVE-0000-0001") - self.assertEqual(items[2].component_name, "org.dom4j:dom4j") - self.assertEqual(items[2].component_version, "2.1.1.redhat-00001") - self.assertIn( - "Description of a bad vulnerability.", - items[2].description, - ) - self.assertIn( - "/var/lib/adapter-ear1.ear/dom4j-extensions-2.1.1.jar", - items[2].description, - ) - self.assertEqual(items[2].severity, "High") - self.assertEqual(items[2].cvssv3, None) - self.assertEqual(items[2].cvssv3_score, None) - self.assertEqual(items[2].file_path, "adapter-ear1.ear: dom4j-extensions-2.1.1.jar") - self.assertEqual( - items[2].mitigation, - "Update org.dom4j:dom4j:2.1.1.redhat-00001 to at least the version recommended in the description", - ) - self.assertEqual(1, len(items[2].unsaved_vulnerability_ids)) - self.assertEqual("CVE-0000-0001", items[2].unsaved_vulnerability_ids[0]) - - with self.subTest(i=3): - # identifier -> package url javascript, no vulnerabilitids, 3 vulnerabilities, relateddependencies without filename (pre v6.0.0) - self.assertEqual( - items[3].title, "yargs-parser:5.0.0 | 1500", - ) - self.assertEqual(items[3].component_name, "yargs-parser") - self.assertEqual(items[3].component_version, "5.0.0") - # assert fails due to special characters, not too important - # self.assertEqual(items[1].description, "Affected versions of `yargs-parser` are vulnerable to prototype pollution. Arguments are not properly sanitized, allowing an attacker to modify the prototype of `Object`, causing the addition or modification of an existing property that will exist on all objects.Parsing the argument `--foo.__proto__.bar baz'` adds a `bar` property with value `baz` to all objects. This is only exploitable if attackers have control over the arguments being passed to `yargs-parser`.") - self.assertEqual(items[3].severity, "Low") - self.assertEqual(items[3].cvssv3, None) - self.assertEqual(items[3].cvssv3_score, None) - self.assertEqual(items[3].file_path, "yargs-parser:5.0.0") - self.assertEqual( - items[3].mitigation, "Update yargs-parser:5.0.0 to at least the version recommended in the description", + items[1].mitigation, "Update yargs-parser:5.0.0 to at least the version recommended in the description", ) self.assertIn( "**Source:** NPM", - items[3].description, + items[1].description, ) - self.assertIsNone(items[3].unsaved_vulnerability_ids) + self.assertIsNone(items[1].unsaved_vulnerability_ids) - with self.subTest(i=4): + with self.subTest(i=2): self.assertEqual( - items[4].title, + items[2].title, "yargs-parser:5.0.0 | CVE-2020-7608", ) - self.assertEqual(items[4].component_name, "yargs-parser") - self.assertEqual(items[4].component_version, "5.0.0") + self.assertEqual(items[2].component_name, "yargs-parser") + self.assertEqual(items[2].component_version, "5.0.0") self.assertIn( 'yargs-parser could be tricked into adding or modifying properties\n of Object.prototype using a "__proto__" payload.\n**Source:** OSSINDEX\n**Filepath:** \n /var/lib/jenkins/workspace/nl-selfservice_-_metrics_develop/package-lock.json?yargs-parser', - items[4].description, + items[2].description, ) self.assertIn( "/var/lib/jenkins/workspace/nl-selfservice_-_metrics_develop/package-lock.json?yargs-parser", - items[4].description, + items[2].description, ) - self.assertEqual(items[4].severity, "High") - self.assertEqual(items[4].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:H/A:N") - self.assertEqual(items[4].cvssv3_score, 7.5) - self.assertEqual(items[4].file_path, "yargs-parser:5.0.0") + self.assertEqual(items[2].severity, "High") + self.assertEqual(items[2].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:H/A:N") + self.assertEqual(items[2].cvssv3_score, 7.5) + self.assertEqual(items[2].file_path, "yargs-parser:5.0.0") self.assertEqual( - items[4].mitigation, "Update yargs-parser:5.0.0 to at least the version recommended in the description", + items[2].mitigation, "Update yargs-parser:5.0.0 to at least the version recommended in the description", ) - self.assertEqual(1, len(items[4].unsaved_vulnerability_ids)) - self.assertEqual("CVE-2020-7608", items[4].unsaved_vulnerability_ids[0]) + self.assertEqual(1, len(items[2].unsaved_vulnerability_ids)) + self.assertEqual("CVE-2020-7608", items[2].unsaved_vulnerability_ids[0]) - with self.subTest(i=5): + with self.subTest(i=3): self.assertEqual( - items[5].title, + items[3].title, "yargs-parser:5.0.0 | CWE-400: Uncontrolled Resource Consumption ('Resource Exhaustion')", ) - self.assertEqual(items[5].component_name, "yargs-parser") - self.assertEqual(items[5].component_version, "5.0.0") + self.assertEqual(items[3].component_name, "yargs-parser") + self.assertEqual(items[3].component_version, "5.0.0") self.assertIn( "The software does not properly restrict the size or amount of resources that are requested or influenced by an actor, which can be used to consume more resources than intended.", - items[5].description, + items[3].description, ) # check that the filepath is in the description self.assertIn( "/var/lib/jenkins/workspace/nl-selfservice_-_metrics_develop/package-lock.json?yargs-parser", - items[5].description, + items[3].description, ) - self.assertEqual(items[5].severity, "High") - self.assertEqual(items[5].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H") - self.assertEqual(items[5].cvssv3_score, 7.5) - self.assertEqual(items[5].file_path, "yargs-parser:5.0.0") + self.assertEqual(items[3].severity, "High") + self.assertEqual(items[3].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H") + self.assertEqual(items[3].cvssv3_score, 7.5) + self.assertEqual(items[3].file_path, "yargs-parser:5.0.0") self.assertEqual( - items[5].mitigation, "Update yargs-parser:5.0.0 to at least the version recommended in the description", + items[3].mitigation, "Update yargs-parser:5.0.0 to at least the version recommended in the description", ) - self.assertIsNone(items[5].unsaved_vulnerability_ids) + self.assertIsNone(items[3].unsaved_vulnerability_ids) - with self.subTest(i=6): + with self.subTest(i=4): # identifier -> cpe java - self.assertEqual(items[6].title, "org.dom4j:dom4j:2.1.1.redhat-00001 | CVE-0000-0001") - self.assertEqual(items[6].component_name, "org.dom4j:dom4j") - self.assertEqual(items[6].component_version, "2.1.1.redhat-00001") - self.assertEqual(items[6].severity, "High") - self.assertEqual(items[6].cvssv3, None) - self.assertEqual(items[6].cvssv3_score, None) - self.assertEqual(items[6].file_path, "adapter-ear2.ear: dom4j-2.1.1.jar") + self.assertEqual(items[4].title, "org.dom4j:dom4j:2.1.1.redhat-00001 | CVE-0000-0001") + self.assertEqual(items[4].component_name, "org.dom4j:dom4j") + self.assertEqual(items[4].component_version, "2.1.1.redhat-00001") + self.assertEqual(items[4].severity, "High") + self.assertEqual(items[4].cvssv3, None) + self.assertEqual(items[4].cvssv3_score, None) + self.assertEqual(items[4].file_path, "adapter-ear2.ear: dom4j-2.1.1.jar") self.assertEqual( - items[6].mitigation, + items[4].mitigation, "Update org.dom4j:dom4j:2.1.1.redhat-00001 to at least the version recommended in the description", ) - self.assertEqual(1, len(items[6].unsaved_vulnerability_ids)) - self.assertEqual("CVE-0000-0001", items[6].unsaved_vulnerability_ids[0]) + self.assertEqual(1, len(items[4].unsaved_vulnerability_ids)) + self.assertEqual("CVE-0000-0001", items[4].unsaved_vulnerability_ids[0]) - with self.subTest(i=7): + with self.subTest(i=5): # identifier -> maven java - self.assertEqual(items[7].title, "dom4j:2.1.1 | CVE-0000-0001") - self.assertEqual(items[7].component_name, "dom4j") - self.assertEqual(items[7].component_version, "2.1.1") - self.assertEqual(items[7].severity, "High") - self.assertEqual(items[7].cvssv3, None) - self.assertEqual(items[7].cvssv3_score, None) + self.assertEqual(items[5].title, "dom4j:2.1.1 | CVE-0000-0001") + self.assertEqual(items[5].component_name, "dom4j") + self.assertEqual(items[5].component_version, "2.1.1") + self.assertEqual(items[5].severity, "High") + self.assertEqual(items[5].cvssv3, None) + self.assertEqual(items[5].cvssv3_score, None) self.assertEqual( - items[7].mitigation, "Update dom4j:2.1.1 to at least the version recommended in the description", + items[5].mitigation, "Update dom4j:2.1.1 to at least the version recommended in the description", ) - with self.subTest(i=8): + with self.subTest(i=6): # evidencecollected -> single product + single verison javascript self.assertEqual( - items[8].title, + items[6].title, "jquery:3.1.1 | CVE-0000-0001", ) - self.assertEqual(items[8].component_name, "jquery") - self.assertEqual(items[8].component_version, "3.1.1") - self.assertEqual(items[8].severity, "High") - self.assertEqual(items[8].cvssv3, None) - self.assertEqual(items[8].cvssv3_score, None) + self.assertEqual(items[6].component_name, "jquery") + self.assertEqual(items[6].component_version, "3.1.1") + self.assertEqual(items[6].severity, "High") + self.assertEqual(items[6].cvssv3, None) + self.assertEqual(items[6].cvssv3_score, None) self.assertEqual( - items[8].mitigation, "Update jquery:3.1.1 to at least the version recommended in the description", + items[6].mitigation, "Update jquery:3.1.1 to at least the version recommended in the description", ) - with self.subTest(i=9): + with self.subTest(i=7): # Tests for two suppressed vulnerabilities, # One for Suppressed with notes, the other is without. - self.assertEqual(items[9].active, False) + self.assertEqual(items[7].active, False) self.assertEqual( - items[9].mitigation, + items[7].mitigation, "**This vulnerability is mitigated and/or suppressed:** Document on why we are suppressing this vulnerability is missing!\nUpdate jquery:3.1.1 to at least the version recommended in the description", ) - self.assertEqual(items[9].unsaved_tags, ["no_suppression_document", "suppressed"]) - self.assertEqual(items[9].severity, "Critical") - self.assertEqual(items[9].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H") - self.assertEqual(items[9].cvssv3_score, 9.8) - self.assertEqual(items[9].is_mitigated, True) + self.assertEqual(items[7].unsaved_tags, ["no_suppression_document", "suppressed"]) + self.assertEqual(items[7].severity, "Critical") + self.assertEqual(items[7].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H") + self.assertEqual(items[7].cvssv3_score, 9.8) + self.assertEqual(items[7].is_mitigated, True) - with self.subTest(i=10): - self.assertEqual(items[10].active, False) + with self.subTest(i=8): + self.assertEqual(items[8].active, False) self.assertEqual( - items[10].mitigation, + items[8].mitigation, "**This vulnerability is mitigated and/or suppressed:** This is our reason for not to upgrade it.\nUpdate jquery:3.1.1 to at least the version recommended in the description", ) - self.assertEqual(items[10].unsaved_tags, ["suppressed"]) - self.assertEqual(items[10].severity, "Critical") - self.assertEqual(items[10].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H") - self.assertEqual(items[10].cvssv3_score, 9.8) - self.assertEqual(items[10].is_mitigated, True) + self.assertEqual(items[8].unsaved_tags, ["suppressed"]) + self.assertEqual(items[8].severity, "Critical") + self.assertEqual(items[8].cvssv3, "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H") + self.assertEqual(items[8].cvssv3_score, 9.8) + self.assertEqual(items[8].is_mitigated, True) def test_parse_java_6_5_3(self): """Test with version 6.5.3""" @@ -303,7 +263,7 @@ def test_parse_file_pr6439(self): parser = DependencyCheckParser() findings = parser.get_findings(testfile, Test()) items = findings - self.assertEqual(37, len(items)) + self.assertEqual(34, len(items)) # test also different component_name formats with self.subTest(i=0):