fix(api): resolve check_title filter to check_id for consistent finding-group counts (#10486)

This commit is contained in:
Adrián Peña
2026-03-27 09:05:02 +01:00
committed by GitHub
parent 73907db856
commit c953fa7e67
6 changed files with 269 additions and 35 deletions
+65 -21
View File
@@ -44,6 +44,7 @@ from api.models import (
ProviderGroup,
ProviderSecret,
Resource,
ResourceFindingMapping,
ResourceTag,
Role,
Scan,
@@ -197,17 +198,13 @@ class CommonFindingFilters(FilterSet):
field_name="resource_services", lookup_expr="icontains"
)
resource_uid = CharFilter(field_name="resources__uid")
resource_uid__in = CharInFilter(field_name="resources__uid", lookup_expr="in")
resource_uid__icontains = CharFilter(
field_name="resources__uid", lookup_expr="icontains"
)
resource_uid = CharFilter(method="filter_resource_uid")
resource_uid__in = CharInFilter(method="filter_resource_uid_in")
resource_uid__icontains = CharFilter(method="filter_resource_uid_icontains")
resource_name = CharFilter(field_name="resources__name")
resource_name__in = CharInFilter(field_name="resources__name", lookup_expr="in")
resource_name__icontains = CharFilter(
field_name="resources__name", lookup_expr="icontains"
)
resource_name = CharFilter(method="filter_resource_name")
resource_name__in = CharInFilter(method="filter_resource_name_in")
resource_name__icontains = CharFilter(method="filter_resource_name_icontains")
resource_type = CharFilter(method="filter_resource_type")
resource_type__in = CharInFilter(field_name="resource_types", lookup_expr="overlap")
@@ -266,10 +263,49 @@ class CommonFindingFilters(FilterSet):
return queryset.filter(overall_query).distinct()
def filter_check_title_icontains(self, queryset, name, value):
# Resolve from the summary table (has check_title column + trigram
# GIN index) instead of scanning JSON in the findings table.
matching_check_ids = (
FindingGroupDailySummary.objects.filter(
check_title__icontains=value,
)
.values_list("check_id", flat=True)
.distinct()
)
return queryset.filter(check_id__in=matching_check_ids)
# --- Resource subquery filters ---
# Resolve resource → RFM → finding_ids first, then filter findings
# by id__in. This avoids a 3-way JOIN driven from the (huge)
# findings side and lets PostgreSQL start from the resources
# unique-constraint index instead.
@staticmethod
def _finding_ids_for_resources(**lookup):
return ResourceFindingMapping.objects.filter(
resource__in=Resource.objects.filter(**lookup).values("id")
).values("finding_id")
def filter_resource_uid(self, queryset, name, value):
return queryset.filter(id__in=self._finding_ids_for_resources(uid=value))
def filter_resource_uid_in(self, queryset, name, value):
return queryset.filter(id__in=self._finding_ids_for_resources(uid__in=value))
def filter_resource_uid_icontains(self, queryset, name, value):
return queryset.filter(
Q(check_metadata__CheckTitle__icontains=value)
| Q(check_metadata__checktitle__icontains=value)
| Q(check_metadata__Checktitle__icontains=value)
id__in=self._finding_ids_for_resources(uid__icontains=value)
)
def filter_resource_name(self, queryset, name, value):
return queryset.filter(id__in=self._finding_ids_for_resources(name=value))
def filter_resource_name_in(self, queryset, name, value):
return queryset.filter(id__in=self._finding_ids_for_resources(name__in=value))
def filter_resource_name_icontains(self, queryset, name, value):
return queryset.filter(
id__in=self._finding_ids_for_resources(name__icontains=value)
)
@@ -919,7 +955,19 @@ class LatestFindingGroupFilter(CommonFindingFilters):
}
class FindingGroupSummaryFilter(FilterSet):
class _CheckTitleToCheckIdMixin:
"""Resolve check_title search to check_ids so all provider rows are kept."""
def filter_check_title_to_check_ids(self, queryset, name, value):
matching_check_ids = (
queryset.filter(check_title__icontains=value)
.values_list("check_id", flat=True)
.distinct()
)
return queryset.filter(check_id__in=matching_check_ids)
class FindingGroupSummaryFilter(_CheckTitleToCheckIdMixin, FilterSet):
"""
Filter for FindingGroupDailySummary queries.
@@ -942,9 +990,7 @@ class FindingGroupSummaryFilter(FilterSet):
check_id = CharFilter(field_name="check_id", lookup_expr="exact")
check_id__in = CharInFilter(field_name="check_id", lookup_expr="in")
check_id__icontains = CharFilter(field_name="check_id", lookup_expr="icontains")
check_title__icontains = CharFilter(
field_name="check_title", lookup_expr="icontains"
)
check_title__icontains = CharFilter(method="filter_check_title_to_check_ids")
# Provider filters
provider_id = UUIDFilter(field_name="provider_id", lookup_expr="exact")
@@ -1032,7 +1078,7 @@ class FindingGroupSummaryFilter(FilterSet):
return dt
class LatestFindingGroupSummaryFilter(FilterSet):
class LatestFindingGroupSummaryFilter(_CheckTitleToCheckIdMixin, FilterSet):
"""
Filter for FindingGroupDailySummary /latest endpoint.
@@ -1044,9 +1090,7 @@ class LatestFindingGroupSummaryFilter(FilterSet):
check_id = CharFilter(field_name="check_id", lookup_expr="exact")
check_id__in = CharInFilter(field_name="check_id", lookup_expr="in")
check_id__icontains = CharFilter(field_name="check_id", lookup_expr="icontains")
check_title__icontains = CharFilter(
field_name="check_title", lookup_expr="icontains"
)
check_title__icontains = CharFilter(method="filter_check_title_to_check_ids")
# Provider filters
provider_id = UUIDFilter(field_name="provider_id", lookup_expr="exact")
+82
View File
@@ -15810,6 +15810,50 @@ class TestFindingGroupViewSet:
assert len(data) == 1
assert data[0]["id"] == "s3_bucket_public_access"
@pytest.mark.parametrize(
"extra_filters",
[
{},
{"filter[muted]": "include"},
],
ids=["summary_path", "finding_level_path"],
)
def test_check_title_icontains_includes_all_title_variants(
self,
authenticated_client,
finding_groups_title_variants_fixture,
extra_filters,
):
"""
Regression: two providers report the same check_id with different
checktitle values (e.g. after a Prowler version upgrade). Filtering
by check_title__icontains with a term that matches only ONE variant
must still return the finding group with counts from BOTH providers.
Parametrized to cover both aggregation paths:
- summary_path: default, uses _CheckTitleToCheckIdMixin on summaries
- finding_level_path: filter[muted]=include forces CommonFindingFilters
"""
params = {
"filter[inserted_at]": TODAY,
"filter[check_title.icontains]": "Ensure repository",
**extra_filters,
}
response = authenticated_client.get(
reverse("finding-group-list"),
params,
)
assert response.status_code == status.HTTP_200_OK
data = response.json()["data"]
assert len(data) == 1
assert data[0]["id"] == "github_secret_scanning_enabled"
attrs = data[0]["attributes"]
# Both providers' findings must be counted
assert attrs["fail_count"] == 2, (
"fail_count must include findings from both providers, "
"regardless of which title variant matches the search"
)
def test_resources_not_found(self, authenticated_client):
"""Test 404 returned for nonexistent check_id."""
response = authenticated_client.get(
@@ -15851,6 +15895,44 @@ class TestFindingGroupViewSet:
assert resource.get("region"), "resource.region must not be empty"
assert resource.get("type"), "resource.type must not be empty"
def test_resources_resource_group(
self, authenticated_client, finding_groups_fixture
):
"""Test resource_group is extracted from check_metadata.resourcegroup."""
response = authenticated_client.get(
reverse(
"finding-group-resources", kwargs={"pk": "s3_bucket_public_access"}
),
{"filter[inserted_at]": TODAY},
)
assert response.status_code == status.HTTP_200_OK
data = response.json()["data"]
assert len(data) == 2
for item in data:
resource = item["attributes"]["resource"]
assert (
resource["resource_group"] == "storage"
), "resource_group must be 'storage'"
def test_resources_name_icontains(
self, authenticated_client, finding_groups_fixture
):
"""Test resource_name__icontains filters resources by name substring."""
# s3_bucket_public_access has "My Instance 1" and "My Instance 2"
response = authenticated_client.get(
reverse(
"finding-group-resources", kwargs={"pk": "s3_bucket_public_access"}
),
{
"filter[inserted_at]": TODAY,
"filter[resource_name.icontains]": "Instance 1",
},
)
assert response.status_code == status.HTTP_200_OK
data = response.json()["data"]
assert len(data) == 1
assert "Instance 1" in data[0]["attributes"]["resource"]["name"]
def test_resources_provider_info(
self, authenticated_client, finding_groups_fixture
):
+2
View File
@@ -4194,6 +4194,7 @@ class FindingGroupResourceSerializer(BaseSerializerV1):
"service": {"type": "string"},
"region": {"type": "string"},
"type": {"type": "string"},
"resource_group": {"type": "string"},
},
}
)
@@ -4205,6 +4206,7 @@ class FindingGroupResourceSerializer(BaseSerializerV1):
"service": obj.get("resource_service", ""),
"region": obj.get("resource_region", ""),
"type": obj.get("resource_type", ""),
"resource_group": obj.get("resource_group", ""),
}
@extend_schema_field(
+34 -14
View File
@@ -3483,7 +3483,7 @@ class FindingViewSet(PaginateByPkMixin, BaseRLSViewSet):
request,
filtered_queryset,
manager=Finding.all_objects,
select_related=["scan"],
select_related=["scan__provider"],
prefetch_related=["resources"],
)
@@ -3653,7 +3653,7 @@ class FindingViewSet(PaginateByPkMixin, BaseRLSViewSet):
tenant_id = request.tenant_id
filtered_queryset = self.filter_queryset(self.get_queryset())
latest_scan_ids = (
latest_scan_ids = list(
Scan.all_objects.filter(tenant_id=tenant_id, state=StateChoices.COMPLETED)
.order_by("provider_id", "-inserted_at")
.distinct("provider_id")
@@ -3667,7 +3667,7 @@ class FindingViewSet(PaginateByPkMixin, BaseRLSViewSet):
request,
filtered_queryset,
manager=Finding.all_objects,
select_related=["scan"],
select_related=["scan__provider"],
prefetch_related=["resources"],
)
@@ -6878,8 +6878,15 @@ class FindingGroupViewSet(BaseRLSViewSet):
"resource_type__icontains": "type__icontains",
}
# Fields accepted directly by LatestResourceFilter (no translation needed)
_RESOURCE_FILTER_FIELDS = {
f"{field}__{lookup}"
for field, lookups in LatestResourceFilter.Meta.fields.items()
for lookup in lookups
} | set(LatestResourceFilter.Meta.fields.keys())
def _split_resource_filters(self, params: QueryDict) -> tuple[QueryDict, QueryDict]:
resource_keys = set(self.RESOURCE_FILTER_MAP)
resource_keys = set(self.RESOURCE_FILTER_MAP) | self._RESOURCE_FILTER_FIELDS
finding_params = QueryDict(mutable=True)
resource_params = QueryDict(mutable=True)
for key, values in params.lists():
@@ -6900,11 +6907,16 @@ class FindingGroupViewSet(BaseRLSViewSet):
queryset = queryset.filter(tenant_id=tenant_id)
filter_params = QueryDict(mutable=True)
for key, mapped_key in self.RESOURCE_FILTER_MAP.items():
if key not in params:
for key, values in params.lists():
# Translate resource_* prefixed keys via the map
if key in self.RESOURCE_FILTER_MAP:
mapped_key = self.RESOURCE_FILTER_MAP[key]
elif key in self._RESOURCE_FILTER_FIELDS:
mapped_key = key
else:
continue
if key == "resources" or key.endswith("__in"):
values = params.getlist(key)
items: list[str] = []
for value in values:
if value is None:
@@ -7232,11 +7244,13 @@ class FindingGroupViewSet(BaseRLSViewSet):
),
first_seen_at=Min("finding__first_seen_at"),
last_seen_at=Max("finding__inserted_at"),
# Max() picks an arbitrary reason when a resource has multiple
# muted findings; this is acceptable because mute rules are
# applied per-check so all findings for the same resource
# share the same muted_reason in practice.
# Max() on muted_reason / check_metadata is safe because
# all findings for the same resource+check share identical
# values (mute rules and metadata are applied per-check).
muted_reason=Max("finding__muted_reason"),
resource_group=Max(
KeyTextTransform("resourcegroup", "finding__check_metadata")
),
)
.filter(resource_id__isnull=False)
.order_by("resource_id")
@@ -7273,6 +7287,7 @@ class FindingGroupViewSet(BaseRLSViewSet):
"first_seen_at": row["first_seen_at"],
"last_seen_at": row["last_seen_at"],
"muted_reason": row.get("muted_reason"),
"resource_group": row.get("resource_group", ""),
}
)
@@ -7307,14 +7322,19 @@ class FindingGroupViewSet(BaseRLSViewSet):
raise ValidationError(filterset.errors)
filtered_queryset = filterset.qs
# Only include summaries from each provider's most recent date
# (within the filtered range)
filtered_queryset = filtered_queryset.annotate(
# (within the filtered range).
# We use a subquery to strip the Window annotation so it does not
# leak into the GROUP BY of _aggregate_daily_summaries.
latest_per_provider = filtered_queryset.annotate(
_max_provider_date=Window(
expression=Max("inserted_at"),
partition_by=[F("provider_id")],
),
).filter(inserted_at=F("_max_provider_date"))
return self._aggregate_daily_summaries(filtered_queryset)
clean_queryset = FindingGroupDailySummary.objects.filter(
pk__in=latest_per_provider.values("pk")
)
return self._aggregate_daily_summaries(clean_queryset)
def _sorted_paginated_response(self, request, aggregated_queryset):
"""Apply ordering, pagination, post-processing, and return the Response."""
+85
View File
@@ -2012,6 +2012,7 @@ def finding_groups_fixture(
"CheckId": "s3_bucket_public_access",
"checktitle": "Ensure S3 buckets do not allow public access",
"Description": "S3 buckets should be configured to restrict public access.",
"resourcegroup": "storage",
},
first_seen_at="2024-01-02T00:00:00Z",
muted=False,
@@ -2036,6 +2037,7 @@ def finding_groups_fixture(
"CheckId": "s3_bucket_public_access",
"checktitle": "Ensure S3 buckets do not allow public access",
"Description": "S3 buckets should be configured to restrict public access.",
"resourcegroup": "storage",
},
first_seen_at="2024-01-03T00:00:00Z",
muted=False,
@@ -2234,6 +2236,89 @@ def finding_groups_fixture(
return findings
@pytest.fixture
def finding_groups_title_variants_fixture(
tenants_fixture, providers_fixture, scans_fixture, resources_fixture
):
"""
Two providers report the same check_id with different checktitle values.
Simulates a Prowler version upgrade where the check title changed but the
check_id stayed the same. Used to verify that check_title__icontains
resolves to check_id first, so results include all providers regardless
of which title variant matches the search term.
"""
tenant = tenants_fixture[0]
provider1, provider2, *_ = providers_fixture
scan1, scan2, *_ = scans_fixture
resource1, resource2, *_ = resources_fixture
findings = []
# Provider 1 — OLD title variant
finding_old = Finding.objects.create(
tenant_id=tenant.id,
uid="fg_title_variant_old",
scan=scan1,
delta="new",
status=Status.FAIL,
status_extended="Secret scanning not enabled",
impact=Severity.high,
impact_extended="High risk",
severity=Severity.high,
raw_result={"status": Status.FAIL, "severity": Severity.high},
tags={},
check_id="github_secret_scanning_enabled",
check_metadata={
"CheckId": "github_secret_scanning_enabled",
"checktitle": "Ensure repository has secret scanning enabled",
"Description": "Checks if secret scanning is enabled.",
},
first_seen_at="2024-01-01T00:00:00Z",
muted=False,
)
finding_old.add_resources([resource1])
findings.append(finding_old)
# Provider 2 — NEW title variant (same check_id, different checktitle)
finding_new = Finding.objects.create(
tenant_id=tenant.id,
uid="fg_title_variant_new",
scan=scan2,
delta="new",
status=Status.FAIL,
status_extended="Secret scanning not enabled on repo",
impact=Severity.high,
impact_extended="High risk",
severity=Severity.high,
raw_result={"status": Status.FAIL, "severity": Severity.high},
tags={},
check_id="github_secret_scanning_enabled",
check_metadata={
"CheckId": "github_secret_scanning_enabled",
"checktitle": "Check if secret scanning is enabled in GitHub",
"Description": "Checks if secret scanning is enabled.",
},
first_seen_at="2024-01-02T00:00:00Z",
muted=False,
)
finding_new.add_resources([resource2])
findings.append(finding_new)
from tasks.jobs.scan import aggregate_finding_group_summaries
aggregate_finding_group_summaries(
tenant_id=str(tenant.id),
scan_id=str(scan1.id),
)
aggregate_finding_group_summaries(
tenant_id=str(tenant.id),
scan_id=str(scan2.id),
)
return findings
def pytest_collection_modifyitems(items):
"""Ensure test_rbac.py is executed first."""
items.sort(key=lambda item: 0 if "test_rbac.py" in item.nodeid else 1)