diff --git a/docs/content/releases/os_upgrading/3.2.md b/docs/content/releases/os_upgrading/3.2.md index c75c1f63e6e..56be80ee858 100644 --- a/docs/content/releases/os_upgrading/3.2.md +++ b/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 diff --git a/dojo/finding/models.py b/dojo/finding/models.py index 4379590477c..b0f9364a216 100644 --- a/dojo/finding/models.py +++ b/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) diff --git a/dojo/importers/default_importer.py b/dojo/importers/default_importer.py index b687c5d5ef2..f93abdb6d78 100644 --- a/dojo/importers/default_importer.py +++ b/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) diff --git a/dojo/tools/blackduck/importer.py b/dojo/tools/blackduck/importer.py index ad93cce6d01..e0c3c4289de 100644 --- a/dojo/tools/blackduck/importer.py +++ b/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, diff --git a/dojo/tools/blackduck_binary_analysis/importer.py b/dojo/tools/blackduck_binary_analysis/importer.py index 3e737fb4dd0..28f156935e2 100644 --- a/dojo/tools/blackduck_binary_analysis/importer.py +++ b/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) diff --git a/dojo/tools/dependency_track/parser.py b/dojo/tools/dependency_track/parser.py index 3f0191ed1c0..3d3774eff32 100644 --- a/dojo/tools/dependency_track/parser.py +++ b/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: diff --git a/dojo/tools/legitify/parser.py b/dojo/tools/legitify/parser.py index 2a905b4ac40..bf28e1a85f1 100644 --- a/dojo/tools/legitify/parser.py +++ b/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,7 +65,7 @@ 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", "")}', @@ -70,9 +73,9 @@ def get_findings(self, file, test): 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 diff --git a/dojo/tools/sarif/parser.py b/dojo/tools/sarif/parser.py index 57e9a8ffc68..b9b1d712cf1 100644 --- a/dojo/tools/sarif/parser.py +++ b/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 diff --git a/unittests/test_hash_code_location_ordering.py b/unittests/test_hash_code_location_ordering.py new file mode 100644 index 00000000000..535908730a4 --- /dev/null +++ b/unittests/test_hash_code_location_ordering.py @@ -0,0 +1,174 @@ +""" +The locations/endpoints ingredient of hash_code must be order-independent. + +Two properties are pinned here, for both the locations (V3) and the endpoints (legacy) hash +ingredient: + +1. A finding's hash_code does not depend on the order in which the scanner reported its + locations. Scanners frequently report them from a set, so the order is not meaningful and can + differ between two imports of the same report. +2. The hash_code stored at import time is the hash_code a recomputation produces after the finding + was saved. The stored value comes from Finding.get_locations()'s unsaved branch and a + recomputation (`manage.py dedupe`, reimport, false positive history) uses its saved branch, so + the two branches have to agree - on ordering, and on the canonical form of each location. +""" + +from django.contrib.auth import get_user_model +from django.test import override_settings +from django.utils import timezone + +from dojo.importers.default_importer import DefaultImporter +from dojo.models import ( + Development_Environment, + Engagement, + Finding, + Product, + Product_Type, + Test, + Test_Type, +) +from dojo.tools.locations import LocationData + +from .dojo_test_case import DojoTestCase + +User = get_user_model() + +# "Qualys Scan" hashes ["title", "severity", "endpoints"] and allows a null cwe, so the reported +# locations really are part of the hash_code for this scan type. +SCAN_TYPE = "Qualys Scan" + +# deliberately not in sorted order +URLS = [ + "https://zulu.example.com/three", + "https://alpha.example.com/one", + "https://mike.example.com/two", +] + + +class HashCodeLocationOrderingMixin: + + """Assertions shared by the locations (V3) and endpoints (legacy) variants.""" + + def setUp(self): + super().setUp() + self.user, _ = User.objects.get_or_create(username="admin") + product_type, _ = Product_Type.objects.get_or_create(name="hash code ordering") + self.product, _ = Product.objects.get_or_create( + name="hash code ordering product", description="test", prod_type=product_type, + ) + self.engagement, _ = Engagement.objects.get_or_create( + name="hash code ordering engagement", + product=self.product, + target_start=timezone.now(), + target_end=timezone.now(), + ) + self.environment, _ = Development_Environment.objects.get_or_create(name="Development") + test_type, _ = Test_Type.objects.get_or_create(name=SCAN_TYPE) + # a test is needed even for the findings that are never saved: the hash_code fields are + # resolved from its test type + self.test = Test.objects.create( + engagement=self.engagement, + test_type=test_type, + scan_type=SCAN_TYPE, + target_start=timezone.now(), + target_end=timezone.now(), + ) + + def import_finding(self, finding): + """Import a single parsed finding the way a scan import does, and return it saved.""" + importer = DefaultImporter( + user=self.user, + lead=self.user, + scan_date=None, + environment=self.environment, + minimum_severity="Info", + active=True, + verified=True, + sync=True, + scan_type=SCAN_TYPE, + engagement=self.engagement, + ) + importer.create_test(SCAN_TYPE) + imported = importer.process_findings([finding]) + self.assertEqual(1, len(imported)) + return imported[0] + + def parsed_finding(self, urls, *, title="Insecure thing"): + finding = Finding(title=title, severity="High", description="whatever", dynamic_finding=True, test=self.test) + self.attach_locations(finding, urls) + return finding + + def test_hash_code_is_independent_of_the_reported_order(self): + """The same locations in a different order must produce the same hash_code.""" + first = self.parsed_finding(URLS).compute_hash_code() + second = self.parsed_finding(list(reversed(URLS))).compute_hash_code() + self.assertEqual(first, second) + + def test_hash_code_uses_every_reported_location(self): + """Guard against the assertion above passing because the locations are ignored entirely.""" + with_locations = self.parsed_finding(URLS).compute_hash_code() + without_one = self.parsed_finding(URLS[:-1]).compute_hash_code() + self.assertNotEqual(with_locations, without_one) + + def test_stored_hash_code_survives_recomputation(self): + """The hash stored on import must equal the hash recomputed from the saved finding.""" + finding = self.import_finding(self.parsed_finding(URLS)) + finding.refresh_from_db() + self.assertEqual(finding.hash_code, finding.compute_hash_code()) + + def test_stored_hash_code_survives_recomputation_for_non_canonical_locations(self): + """ + Same, for locations a parser did not hand over in canonical form. + + Cleaning rewrites the canonical string form of a location, so a hash taken before cleaning + does not match the one taken after the cleaned location was saved. + """ + finding = self.import_finding(self.parsed_finding_with_non_canonical_locations()) + finding.refresh_from_db() + self.assertEqual(finding.hash_code, finding.compute_hash_code()) + + def test_stored_hash_code_is_independent_of_the_reported_order(self): + """End to end: two imports of the same locations in different orders store the same hash.""" + first = self.import_finding(self.parsed_finding(URLS)) + second = self.import_finding(self.parsed_finding(list(reversed(URLS)))) + self.assertEqual(first.hash_code, second.hash_code) + + +@override_settings(V3_FEATURE_LOCATIONS=True) +class TestHashCodeLocationOrdering(HashCodeLocationOrderingMixin, DojoTestCase): + + """The locations hash ingredient (V3_FEATURE_LOCATIONS=True).""" + + def attach_locations(self, finding, urls): + finding.unsaved_locations = [LocationData.url(url=url) for url in urls] + + def parsed_finding_with_non_canonical_locations(self): + finding = Finding(title="Non canonical", severity="High", description="whatever", dynamic_finding=True) + # URL.clean() lowercases the protocol and the host + finding.unsaved_locations = [ + LocationData.url(url="HTTPS://ZULU.example.com/three"), + LocationData.url(url="HTTPS://ALPHA.example.com/one"), + ] + return finding + + +@override_settings(V3_FEATURE_LOCATIONS=False) +class TestHashCodeEndpointOrdering(HashCodeLocationOrderingMixin, DojoTestCase): + + """The endpoints hash ingredient (legacy, V3_FEATURE_LOCATIONS=False).""" + + def attach_locations(self, finding, urls): + from dojo.endpoint.models import Endpoint # noqa: PLC0415 -- import guarded by V3_FEATURE_LOCATIONS + + finding.unsaved_endpoints = [Endpoint.from_uri(url) for url in urls] + + def parsed_finding_with_non_canonical_locations(self): + from dojo.endpoint.models import Endpoint # noqa: PLC0415 -- import guarded by V3_FEATURE_LOCATIONS + + finding = Finding(title="Non canonical", severity="High", description="whatever", dynamic_finding=True) + # Endpoint.clean() strips the leading "/" from the path and the leading "?" from the query + finding.unsaved_endpoints = [ + Endpoint(protocol="https", host="zulu.example.com", path="/three", query="?b=2"), + Endpoint(protocol="https", host="alpha.example.com", path="/one", query="?a=1"), + ] + return finding diff --git a/unittests/test_hash_code_location_queries.py b/unittests/test_hash_code_location_queries.py new file mode 100644 index 00000000000..53a65345fe1 --- /dev/null +++ b/unittests/test_hash_code_location_queries.py @@ -0,0 +1,161 @@ +""" +The location read inside hash_code must honour a prefetched locations relation. + +``Finding.get_locations()`` feeds the ``endpoints`` hash ingredient for the 13 scan types whose +``HASHCODE_FIELDS_PER_SCANNER`` includes it. Every caller of the hash paths prefetches that +relation -- the batch dedupe loader, ``build_candidate_scope_queryset`` and ``manage.py dedupe`` +among them -- but the saved-locations read used ``finding.locations.filter(...)``, and +``.filter()`` on a related manager clones the queryset and drops ``_result_cache``. That bypassed +the prefetch entirely: every caller paid for a prefetch whose results were then discarded, then +took a query per location reference, plus a ``.count()`` that ran regardless of log level because +it sat inside string concatenation. + +Query counts, not timings, so these cannot flake on machine speed. + +The existing perf suites do not cover this. Their fixtures use scan types that do not hash +``endpoints``, so ``get_locations()`` is never reached there and their counts are unchanged by +this fix -- verified by running both before and after. +""" + +from django.contrib.auth import get_user_model +from django.test import override_settings +from django.utils import timezone + +from dojo.importers.default_importer import DefaultImporter +from dojo.models import ( + Development_Environment, + Engagement, + Finding, + Product, + Product_Type, + Test, + Test_Type, +) +from dojo.tools.locations import LocationData + +from .dojo_test_case import DojoTestCase + +User = get_user_model() + +# "Qualys Scan" hashes ["title", "severity", "endpoints"], so the reported locations really are +# part of hash_code for this scan type and get_locations() is reached. +SCAN_TYPE = "Qualys Scan" + +URLS = [ + "https://zulu.example.com/three", + "https://alpha.example.com/one", + "https://mike.example.com/two", +] + + +@override_settings(V3_FEATURE_LOCATIONS=True) +class TestHashCodeLocationQueryCount(DojoTestCase): + + """Computing hash_code must not re-query locations the caller already prefetched.""" + + def setUp(self): + super().setUp() + self.user, _ = User.objects.get_or_create(username="admin") + product_type, _ = Product_Type.objects.get_or_create(name="hash code location queries") + self.product, _ = Product.objects.get_or_create( + name="hash code location queries product", + description="test", + prod_type=product_type, + ) + self.engagement, _ = Engagement.objects.get_or_create( + name="hash code location queries engagement", + product=self.product, + target_start=timezone.now(), + target_end=timezone.now(), + ) + self.environment, _ = Development_Environment.objects.get_or_create(name="Development") + test_type, _ = Test_Type.objects.get_or_create(name=SCAN_TYPE) + self.test = Test.objects.create( + engagement=self.engagement, + test_type=test_type, + scan_type=SCAN_TYPE, + target_start=timezone.now(), + target_end=timezone.now(), + ) + + def _saved_finding_with_locations(self, urls): + """Import one finding carrying ``urls`` as locations, the way a scan import does.""" + parsed = Finding( + title="Insecure thing", + severity="High", + description="whatever", + dynamic_finding=True, + test=self.test, + ) + parsed.unsaved_locations = [LocationData.url(url=url) for url in urls] + + importer = DefaultImporter( + user=self.user, + lead=self.user, + scan_date=None, + environment=self.environment, + minimum_severity="Info", + active=True, + verified=True, + sync=True, + scan_type=SCAN_TYPE, + engagement=self.engagement, + ) + importer.create_test(SCAN_TYPE) + imported = importer.process_findings([parsed]) + self.assertEqual(1, len(imported)) + return imported[0] + + def test_prefetched_locations_are_not_requeried(self): + """ + The point of the fix. With the relation prefetched, the read comes from cache and costs + nothing. Before, ``.filter()`` discarded the cache and this took a query for the filtered + set, another for ``.count()``, and one per location reference. + """ + finding = self._saved_finding_with_locations(URLS) + + prefetched = Finding.objects.filter(pk=finding.pk).prefetch_related("locations__location")[0] + + with self.assertNumQueries(0): + prefetched.get_locations() + + def test_prefetching_only_the_references_still_queries_every_location(self): + """ + Why callers have to prefetch two hops, not one. + + The read needs ``ref.location.location_value``, so prefetching just ``locations`` leaves a + query per location reference. This is what makes ``locations__location`` (or a ``Prefetch`` + carrying ``select_related("location")``) the lookup callers actually want -- and what an + unprefetched read costs, one per location. + """ + finding = self._saved_finding_with_locations(URLS) + + shallow = Finding.objects.filter(pk=finding.pk).prefetch_related("locations")[0] + + # One query per location, because only the reference level was prefetched. + with self.assertNumQueries(len(URLS)): + shallow.get_locations() + + def test_the_hash_ingredient_is_unchanged_by_narrowing_in_python(self): + """ + Narrowing the relation in Python instead of SQL must produce the same string: URL + locations only, deduplicated and sorted. Prefetched and unprefetched must agree too. + """ + finding = self._saved_finding_with_locations(URLS) + + unprefetched = Finding.objects.get(pk=finding.pk) + prefetched = Finding.objects.filter(pk=finding.pk).prefetch_related("locations__location")[0] + + self.assertEqual(prefetched.get_locations(), unprefetched.get_locations()) + self.assertEqual( + unprefetched.get_locations(), + "".join(sorted(URLS)), + msg="the ingredient must stay the sorted, deduplicated canonical location values", + ) + + def test_the_hash_code_is_unchanged_by_narrowing_in_python(self): + """End to end: the stored hash still matches a recomputation from the saved finding.""" + finding = self._saved_finding_with_locations(URLS) + finding.refresh_from_db() + + self.assertEqual(finding.hash_code, finding.compute_hash_code()) diff --git a/unittests/test_parsers.py b/unittests/test_parsers.py index ae88f73b924..c944c0bd01f 100644 --- a/unittests/test_parsers.py +++ b/unittests/test_parsers.py @@ -1,3 +1,4 @@ +import ast import os from pathlib import Path @@ -7,6 +8,93 @@ basedir = get_unit_tests_path().parent +# Python randomises string hashing per process, so a set of strings iterates in a different order +# in every import worker. Whatever a parser builds by iterating a set therefore reshuffles on every +# import: text joined into description/file_path/references, the order of unsaved_locations, or +# which vulnerability id ends up being the primary one. Any of those that is a hash_code field +# gives the same unchanged report a different hash_code on every import, which breaks false +# positive history, risk acceptance copies and similar findings. +# +# Sort at the point the set is consumed. Where the order provably cannot escape, annotate the +# statement with this marker instead. +SET_ORDER_MARKER = "set-order-ok" + +# Names of calls that turn a set into a sequence, freezing an arbitrary order into it +SET_TO_SEQUENCE_CALLS = {"list", "tuple"} + + +def _is_set_expression(node, set_names): + """Whether this expression is (most likely) an unordered set.""" + if isinstance(node, ast.Set | ast.SetComp): + return True + if isinstance(node, ast.Name): + return node.id in set_names + if isinstance(node, ast.Call): + if isinstance(node.func, ast.Name): + return node.func.id in {"set", "frozenset"} + if isinstance(node.func, ast.Attribute): + # set.union(...), set.difference(...), ... + return node.func.attr in { + "difference", "intersection", "symmetric_difference", "union", + } and _is_set_expression(node.func.value, set_names) + return False + if isinstance(node, ast.BinOp): + # set algebra: a - b, a & b, a | b, a ^ b + return isinstance(node.op, ast.Sub | ast.BitAnd | ast.BitOr | ast.BitXor) and ( + _is_set_expression(node.left, set_names) or _is_set_expression(node.right, set_names) + ) + return False + + +def _set_variable_names(tree): + """ + Names assigned a set anywhere in the module. + + Deliberately flow-insensitive (and therefore approximate): a name assigned a set once is + treated as a set everywhere. Repeat until nothing new is found, so `b = set(); a = b` is + resolved as well. + """ + set_names = set() + while True: + before = len(set_names) + for node in ast.walk(tree): + if isinstance(node, ast.Assign): + targets, value = node.targets, node.value + elif isinstance(node, ast.AnnAssign) and node.value is not None: + targets, value = [node.target], node.value + else: + continue + if _is_set_expression(value, set_names): + set_names.update(t.id for t in targets if isinstance(t, ast.Name)) + if len(set_names) == before: + return set_names + + +def find_set_order_dependencies(source): + """Return [(lineno, source line)] for every place where the iteration order of a set is used.""" + tree = ast.parse(source) + set_names = _set_variable_names(tree) + lines = source.splitlines() + findings = [] + for node in ast.walk(tree): + if isinstance(node, ast.For) and _is_set_expression(node.iter, set_names): + # for x in : whatever the body accumulates inherits the set's order + offenders = [node.iter] + elif isinstance(node, ast.ListComp | ast.SetComp | ast.GeneratorExp | ast.DictComp): + offenders = [gen.iter for gen in node.generators if _is_set_expression(gen.iter, set_names)] + elif isinstance(node, ast.Call) and node.args and ( + (isinstance(node.func, ast.Attribute) and node.func.attr == "join") + or (isinstance(node.func, ast.Name) and node.func.id in SET_TO_SEQUENCE_CALLS) + ): + offenders = [node.args[0]] if _is_set_expression(node.args[0], set_names) else [] + else: + continue + for offender in offenders: + line = lines[offender.lineno - 1] + if SET_ORDER_MARKER not in line: + findings.append((offender.lineno, line.strip())) + return sorted(set(findings)) + @test_tag("parser-supplement-tests") class TestParsers(DojoTestCase): @@ -105,6 +193,54 @@ def test_file_existence(self): read_true = True i = 0 + def test_no_set_order_dependencies(self): + """ + No parser may let the iteration order of a set decide what it produces. + + See SET_ORDER_MARKER above for why, and for how to annotate the rare case where the order + provably cannot escape. + """ + for parser_file in sorted((Path(basedir) / "dojo" / "tools").rglob("*.py")): + source = parser_file.read_text(encoding="utf-8") + findings = find_set_order_dependencies(source) + if findings: + with self.subTest(parser=str(parser_file.relative_to(Path(basedir) / "dojo" / "tools"))): + reported = "\n".join(f" {parser_file}:{lineno}: {line}" for lineno, line in findings) + self.fail( + "the iteration order of a set is used here, which makes the parser output " + f"depend on PYTHONHASHSEED:\n{reported}\n" + "Sort where the set is consumed (sorted(...)), or annotate the line with " + f'"# {SET_ORDER_MARKER}: ".', + ) + + def test_set_order_checker(self): + """The checker behind test_no_set_order_dependencies: it must catch the shapes that broke real parsers.""" + order_leaks = { + # "".join(), inline and through a variable (both were real bugs) + "inline join": 'x = ", ".join(set(symbols))', + "join via variable": 'names = set()\nnames.add(a)\nx = "; ".join(names)', + # a set frozen into a sequence: the first element becomes the primary vulnerability id + "list of a set": "ids = {vuln_id}\nvulnerability_ids = list(ids)", + # locations built by iterating a set + "comprehension": "locations = set()\nf.unsaved_locations = [url(u) for u in locations]", + "for loop": "seen = set()\nfor s in seen:\n description += s", + "set algebra": "common = set(a) & set(b)\nfor key in common:\n pass", + } + for name, source in order_leaks.items(): + with self.subTest(source=name): + self.assertTrue(find_set_order_dependencies(source), f"{name} should be reported") + + ordered = { + "sorted set": 'seen = set()\nx = ", ".join(sorted(seen))', + "list": 'items = [1, 2]\nx = ", ".join(items)\nfor i in items:\n pass', + "dict (insertion ordered)": 'd = {}\nx = ", ".join(d)\nfor key in d:\n pass', + "membership test": 'keys = {"a", "b"}\nfound = "a" in keys', + "annotated": f"seen = set()\nfor s in seen: # {SET_ORDER_MARKER}: only counted\n total += 1", + } + for name, source in ordered.items(): + with self.subTest(source=name): + self.assertEqual([], find_set_order_dependencies(source), f"{name} should not be reported") + def test_parser_existence(self): for docs in os.scandir(Path(basedir) / "docs" / "content" / "supported_tools" / "parsers" / "file"): if docs.name not in {