fix(dedupe): order locations/endpoints inside hash_code, and catch set-order leaks in parsers

docs/content/releases/os_upgrading/3.2.md

@@ -2,7 +2,7 @@
 title: 'Upgrading to DefectDojo Version 3.2.x'
 toc_hide: true
 weight: -20260701
-description: Vulnerability ids gain an autodetected type and a uniqueness constraint; findings can now carry multiple CWEs via a new Finding_CWE relationship. Migrations add the type column, de-duplicate vulnerability-id rows, add the uniqueness constraint, create the CWE table, and backfill it. The vulnerability id and CWE changes leave existing hash codes untouched; four split deduplication registrations are repaired, which changes the identity of Burp Suite DAST Scan findings, and the AWS Security Hub parser now sorts resource IDs, which is an identity change for findings that report more than one resource. This release also deprecates the API-based (pull) parsers, the Tool Type / Tool Configuration feature, and the django-dbbackup integration, all scheduled for removal in 3.5.0.
+description: Vulnerability ids gain an autodetected type and a uniqueness constraint; findings can now carry multiple CWEs via a new Finding_CWE relationship. Migrations add the type column, de-duplicate vulnerability-id rows, add the uniqueness constraint, create the CWE table, and backfill it. The vulnerability id and CWE changes leave existing hash codes untouched; four split deduplication registrations are repaired, which changes the identity of Burp Suite DAST Scan findings, and the AWS Security Hub parser now sorts resource IDs, which is an identity change for findings that report more than one resource. The locations ingredient of the hash code is now sorted on both code paths, an identity change for multi-endpoint findings on installs running with DD_V3_FEATURE_LOCATIONS=False, and recomputing the hash code of a saved finding that has locations no longer raises an error. This release also deprecates the API-based (pull) parsers, the Tool Type / Tool Configuration feature, and the django-dbbackup integration, all scheduled for removal in 3.5.0.
 ---
 
 ## Vulnerability id type
@@ -131,6 +131,43 @@ The Yarn Audit parser (Yarn 2 output) joined unordered `set`s into the finding d
 
 This is not an identity change: "Yarn Audit Scan" declares its own hash fields (title, severity, file path, vulnerability IDs, CWE), and neither the description nor `component_version` is among them, so no `hash_code` changes. The only difference is that the values stop flapping in the UI.
 
+## `hash_code`: reported locations are always ordered
+
+The two parser fixes above each sorted one parser. This release also sorts the ingredient itself, so a scanner's ordering can no longer reach `hash_code` through the locations of a finding.
+
+The locations (URLs) a finding was reported at are part of `hash_code` for every scan type that lists `endpoints` in `HASHCODE_FIELDS_PER_SCANNER` (Qualys Scan, ffuf Scan, Dirsearch Scan, httpx Scan, Nettacker Scan, Legitify Scan and others). That ingredient is computed twice in a finding's life, from two different code paths: from the parser's output before the finding is saved (this is the value that gets stored on import), and from the saved rows whenever the hash is recomputed later.
+
+The saved path sorted the locations; the legacy endpoints path did not. So the stored identity of an imported finding depended on the order its scanner happened to report its endpoints in, and disagreed with what any recomputation produced. Both paths now sort, and both use the same canonical string form: the importer normalizes locations *before* hashing them, as re-import already did.
+
+### What you need to do
+
+Nothing is required. Note that this is an identity change, but only for installs running with `DD_V3_FEATURE_LOCATIONS=False`, and only for findings that report **two or more** endpoints — a single endpoint has one possible ordering, so its `hash_code` is unchanged. Installs on the default `DD_V3_FEATURE_LOCATIONS=True` are not affected at all: the locations path already sorted.
+
+For the affected findings there is no stable prior identity to preserve. The stored value was whatever order the report happened to arrive in, and it did not match what a recomputation would have produced anyway.
+
+## `manage.py dedupe` with locations enabled
+
+Recomputing the `hash_code` of an already saved finding that has URL locations raised `AttributeError: 'Location' object has no attribute 'get_location_value'`. This affected `manage.py dedupe` (and anything else that recomputes hashes) for every finding of a scan type that hashes `endpoints`, with the default `DD_V3_FEATURE_LOCATIONS=True`.
+
+It is fixed by reading the stored canonical value of the location. This is not an identity change: the value now produced is the same one the import path computes, and previously no value was produced at all.
+
+## Dependency Track parser: stable primary vulnerability id
+
+The Dependency Track parser collected a finding's vulnerability ids (the report's `vulnId` plus any aliases) in a `set` and handed them over in set order. The first id in that list becomes the finding's **primary** vulnerability id — the one shown as its CVE — so which identifier a finding was filed under was decided by `PYTHONHASHSEED` and could differ between two imports of the same report.
+
+The report's own `vulnId` is now always the primary id, and the aliases follow it in sorted order. `hash_code` is unaffected (it sorts vulnerability ids already), but the CVE displayed for a finding with aliases may change from an alias to the `vulnId`, which is what the finding's title has always used.
+
+## Ordering fixes without an identity change
+
+The following parsers also let the iteration order of a `set` decide what they emitted. None of these values is part of a `hash_code`, so no identity changes — they simply stop reshuffling on every import:
+
+- **Blackduck Hub Scan** — the file paths joined into `file_path`, and the order the findings themselves are produced in.
+- **Blackduck Binary Analysis** — the order the findings are produced in.
+- **SARIF** — the finding's tags.
+- **Legitify Scan** — the URLs listed under references.
+
+A unit test now scans every parser for this class of defect (`unittests/test_parsers.py`), so it is caught before merge rather than one report at a time.
+
 ## Deprecation: API-based (pull) parsers
 
 The following **API-based (pull) parsers** — which fetch findings directly from a vendor API using

dojo/finding/models.py

@@ -937,8 +937,13 @@ def _get_unsaved_endpoints(finding) -> str:
                     deduplicationLogger.debug("get_endpoints before the finding was saved")
                     # convert list of unsaved endpoints to the list of their canonical representation
                     endpoint_str_list = [str(endpoint) for endpoint in finding.unsaved_endpoints]
-                    # deduplicate (usually done upon saving finding) and sort endpoints
-                    return "".join(dict.fromkeys(endpoint_str_list))
+                    # deduplicate (usually done upon saving finding) and sort endpoints.
+                    # Sorting is what makes this agree with _get_saved_endpoints below: the
+                    # stored hash_code of an imported finding comes from this branch, so
+                    # without it the scanner's emission order would become part of the
+                    # finding's identity and any later recomputation would produce a
+                    # different hash_code for the same finding.
+                    return "".join(sorted(dict.fromkeys(endpoint_str_list)))
                 # we can get here when the parser defines static_finding=True but leaves dynamic_finding defaulted
                 # In this case, before saving the finding, both static_finding and dynamic_finding are True
                 # After saving dynamic_finding may be set to False probably during the saving process (observed on Bandit scan before forcing dynamic_finding=False at parser level)
@@ -985,12 +990,24 @@ def _get_unsaved_locations(finding) -> str:
         def _get_saved_locations(finding) -> str:
             if finding.id is not None:
                 from dojo.url.models import URL  # noqa: PLC0415 -- lazy import, avoids circular dependency
-                url_locations = finding.locations.filter(location__location_type=URL.get_location_type())
-                deduplicationLogger.debug("get_locations: after the finding was saved. Locations count: " + str(url_locations.count()))
-                # convert list of locations to the list of their canonical representation
-                locations = sorted({location_ref.location.get_location_value() for location_ref in url_locations.all()})
-                # sort locations strings
-                return "".join(sorted(locations))
+                url_location_type = URL.get_location_type()
+                # Read the relation with .all() and narrow in Python rather than with .filter():
+                # .filter() on a related manager clones the queryset and drops _result_cache, so it
+                # bypasses the prefetch every caller of the hash paths sets up (the batch dedupe
+                # loader, build_candidate_scope_queryset and manage.py dedupe among them all
+                # prefetch this relation). Narrowing here keeps that prefetch effective;
+                # a finding has few locations, so filtering them in Python costs nothing.
+                # deduplicate and sort the canonical representation of every location. The stored
+                # Location.location_value *is* that canonical representation: it is written from
+                # AbstractLocation.get_location_value() when the Location row is created, so this
+                # matches what _get_unsaved_locations computes below.
+                locations = sorted({
+                    location_ref.location.location_value
+                    for location_ref in finding.locations.all()
+                    if location_ref.location.location_type == url_location_type
+                })
+                deduplicationLogger.debug("get_locations: after the finding was saved. Locations count: %d", len(locations))
+                return "".join(locations)
             return ""
 
         return _get_saved_locations(self) or _get_unsaved_locations(self)

dojo/importers/default_importer.py

@@ -246,6 +246,12 @@ def _process_findings_internal(
                 unsaved_finding.unsaved_tags = merged_tags
             unsaved_finding.tags = None
             finding = self.process_cve(unsaved_finding)
+            # Normalize the locations/endpoints before they are hashed. Cleaning rewrites the
+            # canonical string form (a leading "/" is stripped from the path, a leading "?" from
+            # the query, the port is coerced to an int), so hashing before cleaning would make
+            # import store a different hash_code than reimport computes for the same report -
+            # reimport cleans first (see DefaultReImporter._process_findings_internal).
+            self.location_handler.clean_unsaved(finding)
             # Calculate hash_code before saving based on unsaved_endpoints/unsaved_locations and unsaved_vulnerability_ids
             finding.set_hash_code(True)
 

dojo/tools/blackduck/importer.py

@@ -71,7 +71,9 @@ def _process_project_findings(
         self, project_ids, security_issues, files=None,
     ):
         """Process findings per projects and return a BlackduckFinding object per the model"""
-        for project_id in project_ids:
+        # sorted(): project_ids is a set, so iterating it directly makes the order in which
+        # findings are produced depend on PYTHONHASHSEED
+        for project_id in sorted(project_ids):
             locations = set()
             if files is not None:
                 for file_entry in files[project_id]:
@@ -82,7 +84,7 @@ def _process_project_findings(
 
                     # 4000 character limit on this field
                     total_len = len(full_path)
-                    for location in list(locations):
+                    for location in list(locations):  # set-order-ok: only sums lengths
                         # + 2 for the ", " that will be added.
                         total_len += len(location) + 2
                     if total_len < 4000:
@@ -95,7 +97,9 @@ def _process_project_findings(
                 cve = self.get_cve(
                     security_issue_dict.get("Vulnerability id"),
                 ).upper()
-                location = ", ".join(locations)
+                # sorted(): locations is a set, and this string is stored as the finding's
+                # file_path, which must not reshuffle from one import to the next
+                location = ", ".join(sorted(locations))
 
                 yield BlackduckFinding(
                     cve,

dojo/tools/blackduck_binary_analysis/importer.py

@@ -32,7 +32,9 @@ def _process_vuln_results(
         self, sha1_hash_keys, orig_report_name, vulnerabilities,
     ):
         """Process findings for each project."""
-        for sha1_hash_key in sha1_hash_keys:
+        # sorted(): sha1_hash_keys is a set, so iterating it directly makes the order in which
+        # findings are produced depend on PYTHONHASHSEED
+        for sha1_hash_key in sorted(sha1_hash_keys):
             for vuln in vulnerabilities[sha1_hash_key]:
                 vuln_dict = dict(vuln)
 

dojo/tools/dependency_track/parser.py

@@ -71,14 +71,18 @@ def _convert_dependency_track_finding_to_dojo_finding(self, dependency_track_fin
 
         title = f"{component_name}:{version_description} affected by: {vuln_id} ({source})"
 
-        # Collect all vulnerability IDs: vulnId itself plus any aliases
-        set_of_ids = {vuln_id}
-        set_of_alias_sources = {"cveId", "sonatypeId", "ghsaId", "osvId", "snykId", "gsdId", "vulnDbId"}
+        # Collect all vulnerability IDs: vulnId itself plus any aliases.
+        # vuln_id is kept first because the first entry becomes the finding's primary
+        # vulnerability id (its `cve`), and the aliases are sorted: they are collected in a set,
+        # so emitting them in set order would let PYTHONHASHSEED decide both the primary id and
+        # the order the ids are stored in.
+        alias_sources = ("cveId", "sonatypeId", "ghsaId", "osvId", "snykId", "gsdId", "vulnDbId")
+        aliases = set()
         for alias in dependency_track_finding["vulnerability"].get("aliases") or []:
-            for alias_source in set_of_alias_sources:
+            for alias_source in alias_sources:
                 if alias_source in alias:
-                    set_of_ids.add(alias[alias_source])
-        vulnerability_id = list(set_of_ids)
+                    aliases.add(alias[alias_source])
+        vulnerability_id = [vuln_id, *sorted(aliases - {vuln_id})]
 
         # Default CWE to CWE-1035 Using Components with Known Vulnerabilities if there is no CWE
         if "cweId" in dependency_track_finding["vulnerability"] and dependency_track_finding["vulnerability"]["cweId"] is not None:

dojo/tools/legitify/parser.py

@@ -53,6 +53,9 @@ def get_findings(self, file, test):
                     if url:
                         locations.add(url)
             if is_finding:
+                # sorted(): locations is a set, so without this both the references text and the
+                # order of the locations/endpoints would depend on PYTHONHASHSEED
+                sorted_locations = sorted(locations)
                 remediation_steps = policy_info.get("remediationSteps", [])
                 fix_available = False
                 if remediation_steps:
@@ -62,17 +65,17 @@ def get_findings(self, file, test):
                     dynamic_finding=False,
                     impact="\n".join(policy_info.get("threat", [])),
                     mitigation="\n".join(remediation_steps),
-                    references="\n".join(locations),
+                    references="\n".join(sorted_locations),
                     severity=self.severity_mapper(policy_info.get("severity", "LOW")),
                     static_finding=True,
                     title=f'{policy_info.get("namespace", "").capitalize()} | {policy_info.get("title", "")}',
                     vuln_id_from_tool=policy_info.get("policyName", None),
                     fix_available=fix_available,
                 )
                 if settings.V3_FEATURE_LOCATIONS:
-                    finding.unsaved_locations = [LocationData.url(url=url) for url in locations]
+                    finding.unsaved_locations = [LocationData.url(url=url) for url in sorted_locations]
                 else:
                     # TODO: Delete this after the move to Locations
-                    finding.unsaved_endpoints = [Endpoint.from_uri(url) for url in locations]
+                    finding.unsaved_endpoints = [Endpoint.from_uri(url) for url in sorted_locations]
                 findings.append(finding)
         return findings

dojo/tools/sarif/parser.py

@@ -294,8 +294,10 @@ def get_items_from_result(self, result, rules, artifacts, run_date):
             if run_date:
                 finding.date = run_date
 
-            # manage tags provided in the report and rule and remove duplicated
-            tags = list(set(get_properties_tags(rule) + get_properties_tags(result)))
+            # manage tags provided in the report and rule and remove duplicated.
+            # sorted(): a set has no order, so without it the stored tag order would depend on
+            # PYTHONHASHSEED and reshuffle on every import
+            tags = sorted(set(get_properties_tags(rule) + get_properties_tags(result)))
             tags = [s.removeprefix("external/cwe/") for s in tags]
             finding.unsaved_tags = tags