mirror of
https://github.com/prowler-cloud/prowler.git
synced 2026-10-29 14:54:30 +00:00
fix(ui): FAIL-first sort, FAIL-only filter, and mute scope for findings drill-down
Address 3 product blockers from Pepe's review: - Add sort=-status to getFindingGroupResources and getLatestFindingGroupResources so FAIL resources appear before PASS in the drill-down list (sort kept as future-proofing even when filter[status]=FAIL makes it a no-op for now) - Add filter[status]=FAIL to both resource endpoints so drill-down only shows impacted resources, matching the count in the group header - Add filter[status]=FAIL to createResourceFindingResolutionUrl so muting a group only affects active FAIL findings, not all historical - Use .set() after appendSanitizedProviderFilters to guarantee hardcoded FAIL wins even if caller passes filter[status] in filters (prevents duplicate filter[status] params with undefined API behavior) - Apply symmetric .set() pattern in findings-by-resource.ts for consistency 26 tests (10 new). All passing. Judgment Day: 2 rounds, 0 discrepancies remaining.
This commit is contained in:
4 files changed
+331
-1
No files matched your search
@@ -18,7 +18,16 @@ vi.mock("@/lib", () => ({
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/provider-filters", () => ({
|
||||
appendSanitizedProviderFilters: vi.fn(),
|
||||
// Simulate real appendSanitizedProviderFilters: appends all non-undefined filters to the URL.
|
||||
appendSanitizedProviderFilters: vi.fn(
|
||||
(url: URL, filters: Record<string, string | string[] | undefined>) => {
|
||||
Object.entries(filters).forEach(([key, value]) => {
|
||||
if (value !== undefined) {
|
||||
url.searchParams.append(key, String(value));
|
||||
}
|
||||
});
|
||||
},
|
||||
),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/server-actions-helper", () => ({
|
||||
@@ -38,6 +47,10 @@ import {
|
||||
getLatestFindingGroupResources,
|
||||
} from "./finding-groups";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Blocker 1 + 2: FAIL-first sort and FAIL-only filter for drill-down resources
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Tests
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -154,3 +167,219 @@ describe("getLatestFindingGroupResources — SSRF path traversal protection", ()
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Blocker 1: Resources list must show FAIL first (sort=-status)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("getFindingGroupResources — Blocker 1: FAIL-first sort", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should include sort=-status in the API call so FAIL resources appear first", async () => {
|
||||
// Given
|
||||
const checkId = "s3_bucket_public_access";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId });
|
||||
|
||||
// Then — the URL must contain sort=-status
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.get("sort")).toBe("-status");
|
||||
});
|
||||
|
||||
it("should include filter[status]=FAIL in the API call so only impacted resources are shown", async () => {
|
||||
// Given
|
||||
const checkId = "s3_bucket_public_access";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId });
|
||||
|
||||
// Then — the URL must contain filter[status]=FAIL
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.get("filter[status]")).toBe("FAIL");
|
||||
});
|
||||
});
|
||||
|
||||
describe("getLatestFindingGroupResources — Blocker 1: FAIL-first sort", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should include sort=-status in the API call so FAIL resources appear first", async () => {
|
||||
// Given
|
||||
const checkId = "iam_user_mfa_enabled";
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.get("sort")).toBe("-status");
|
||||
});
|
||||
|
||||
it("should include filter[status]=FAIL in the API call so only impacted resources are shown", async () => {
|
||||
// Given
|
||||
const checkId = "iam_user_mfa_enabled";
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.get("filter[status]")).toBe("FAIL");
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Triangulation: sort + filter coexist with pagination and caller filters
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("getFindingGroupResources — triangulation: params coexist", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should send sort=-status AND filter[status]=FAIL alongside pagination params", async () => {
|
||||
// Given
|
||||
const checkId = "s3_bucket_versioning";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId, page: 2, pageSize: 50 });
|
||||
|
||||
// Then — all four params present together
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.get("page[number]")).toBe("2");
|
||||
expect(url.searchParams.get("page[size]")).toBe("50");
|
||||
expect(url.searchParams.get("sort")).toBe("-status");
|
||||
expect(url.searchParams.get("filter[status]")).toBe("FAIL");
|
||||
});
|
||||
});
|
||||
|
||||
describe("getLatestFindingGroupResources — triangulation: params coexist", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should send sort=-status AND filter[status]=FAIL alongside pagination params", async () => {
|
||||
// Given
|
||||
const checkId = "iam_root_mfa_enabled";
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId, page: 3, pageSize: 20 });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.get("page[number]")).toBe("3");
|
||||
expect(url.searchParams.get("page[size]")).toBe("20");
|
||||
expect(url.searchParams.get("sort")).toBe("-status");
|
||||
expect(url.searchParams.get("filter[status]")).toBe("FAIL");
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Blocker: Duplicate filter[status] — caller-supplied status must be stripped
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("getFindingGroupResources — Blocker: caller filter[status] is always overridden to FAIL", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should use filter[status]=FAIL even when caller passes filter[status]=PASS", async () => {
|
||||
// Given — caller explicitly passes PASS, which must be ignored
|
||||
const checkId = "s3_bucket_public_access";
|
||||
const filters = { "filter[status]": "PASS" };
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId, filters });
|
||||
|
||||
// Then — the final URL must have exactly one filter[status]=FAIL, not PASS
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
const allStatusValues = url.searchParams.getAll("filter[status]");
|
||||
expect(allStatusValues).toHaveLength(1);
|
||||
expect(allStatusValues[0]).toBe("FAIL");
|
||||
});
|
||||
|
||||
it("should not have duplicate filter[status] params when caller passes filter[status]", async () => {
|
||||
// Given
|
||||
const checkId = "s3_bucket_public_access";
|
||||
const filters = { "filter[status]": "PASS" };
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId, filters });
|
||||
|
||||
// Then — no duplicates
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.getAll("filter[status]")).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("getLatestFindingGroupResources — Blocker: caller filter[status] is always overridden to FAIL", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should use filter[status]=FAIL even when caller passes filter[status]=PASS", async () => {
|
||||
// Given — caller explicitly passes PASS, which must be ignored
|
||||
const checkId = "iam_user_mfa_enabled";
|
||||
const filters = { "filter[status]": "PASS" };
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId, filters });
|
||||
|
||||
// Then — the final URL must have exactly one filter[status]=FAIL, not PASS
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
const allStatusValues = url.searchParams.getAll("filter[status]");
|
||||
expect(allStatusValues).toHaveLength(1);
|
||||
expect(allStatusValues[0]).toBe("FAIL");
|
||||
});
|
||||
|
||||
it("should not have duplicate filter[status] params when caller passes filter[status]", async () => {
|
||||
// Given
|
||||
const checkId = "iam_user_mfa_enabled";
|
||||
const filters = { "filter[status]": "PASS" };
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId, filters });
|
||||
|
||||
// Then — no duplicates
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.searchParams.getAll("filter[status]")).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
@@ -96,9 +96,18 @@ export const getFindingGroupResources = async ({
|
||||
|
||||
if (page) url.searchParams.append("page[number]", page.toString());
|
||||
if (pageSize) url.searchParams.append("page[size]", pageSize.toString());
|
||||
// sort=-status is kept for future-proofing: if the filter[status]=FAIL
|
||||
// constraint is ever relaxed to allow multiple statuses, the sort ensures
|
||||
// FAIL resources still appear first in the result set.
|
||||
url.searchParams.append("sort", "-status");
|
||||
|
||||
appendSanitizedProviderFilters(url, filters);
|
||||
|
||||
// Use .set() AFTER appendSanitizedProviderFilters so our hardcoded FAIL
|
||||
// always wins, even if the caller passed a different filter[status] value.
|
||||
// Using .set() instead of .append() prevents duplicate filter[status] params.
|
||||
url.searchParams.set("filter[status]", "FAIL");
|
||||
|
||||
try {
|
||||
const response = await fetch(url.toString(), {
|
||||
headers,
|
||||
@@ -130,9 +139,18 @@ export const getLatestFindingGroupResources = async ({
|
||||
|
||||
if (page) url.searchParams.append("page[number]", page.toString());
|
||||
if (pageSize) url.searchParams.append("page[size]", pageSize.toString());
|
||||
// sort=-status is kept for future-proofing: if the filter[status]=FAIL
|
||||
// constraint is ever relaxed to allow multiple statuses, the sort ensures
|
||||
// FAIL resources still appear first in the result set.
|
||||
url.searchParams.append("sort", "-status");
|
||||
|
||||
appendSanitizedProviderFilters(url, filters);
|
||||
|
||||
// Use .set() AFTER appendSanitizedProviderFilters so our hardcoded FAIL
|
||||
// always wins, even if the caller passed a different filter[status] value.
|
||||
// Using .set() instead of .append() prevents duplicate filter[status] params.
|
||||
url.searchParams.set("filter[status]", "FAIL");
|
||||
|
||||
try {
|
||||
const response = await fetch(url.toString(), {
|
||||
headers,
|
||||
|
||||
@@ -268,6 +268,85 @@ describe("resolveFindingIdsByVisibleGroupResources", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Blocker 3: Muting a group mutes ALL historical findings, not just FAIL ones
|
||||
//
|
||||
// The fix: resolveFindingIds must include filter[status]=FAIL so only active
|
||||
// (failing) findings are resolved for mute, not historical/passing ones.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("resolveFindingIds — Blocker 3: only resolve FAIL findings for mute", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
});
|
||||
|
||||
it("should include filter[status]=FAIL in the findings resolution URL for mute", async () => {
|
||||
// Given
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
handleApiResponseMock.mockResolvedValue({
|
||||
data: [{ id: "finding-1" }, { id: "finding-2" }],
|
||||
});
|
||||
|
||||
// When
|
||||
await resolveFindingIds({
|
||||
checkId: "check-1",
|
||||
resourceUids: ["resource-1", "resource-2"],
|
||||
});
|
||||
|
||||
// Then — the URL must filter to only FAIL status findings
|
||||
const calledUrl = new URL(fetchMock.mock.calls[0][0]);
|
||||
expect(calledUrl.searchParams.get("filter[status]")).toBe("FAIL");
|
||||
});
|
||||
|
||||
it("should include filter[status]=FAIL even when date or scan filters are active", async () => {
|
||||
// Given
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
handleApiResponseMock.mockResolvedValue({
|
||||
data: [{ id: "finding-1" }],
|
||||
});
|
||||
|
||||
// When
|
||||
await resolveFindingIds({
|
||||
checkId: "check-1",
|
||||
resourceUids: ["resource-1"],
|
||||
hasDateOrScanFilter: true,
|
||||
filters: {
|
||||
"filter[inserted_at__gte]": "2026-01-01",
|
||||
},
|
||||
});
|
||||
|
||||
// Then
|
||||
const calledUrl = new URL(fetchMock.mock.calls[0][0]);
|
||||
expect(calledUrl.pathname).toBe("/api/v1/findings");
|
||||
expect(calledUrl.searchParams.get("filter[status]")).toBe("FAIL");
|
||||
});
|
||||
|
||||
it("should override caller filter[status] with FAIL — no duplicate params", async () => {
|
||||
// Given — caller passes filter[status]=PASS via filters dict
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
handleApiResponseMock.mockResolvedValue({
|
||||
data: [{ id: "finding-1" }],
|
||||
});
|
||||
|
||||
// When
|
||||
await resolveFindingIds({
|
||||
checkId: "check-1",
|
||||
resourceUids: ["resource-1"],
|
||||
filters: {
|
||||
"filter[status]": "PASS",
|
||||
},
|
||||
});
|
||||
|
||||
// Then — hardcoded FAIL must win, exactly 1 value
|
||||
const calledUrl = new URL(fetchMock.mock.calls[0][0] as string);
|
||||
const statusValues = calledUrl.searchParams.getAll("filter[status]");
|
||||
expect(statusValues).toHaveLength(1);
|
||||
expect(statusValues[0]).toBe("FAIL");
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fix 4: Unbounded page[size] cap
|
||||
//
|
||||
|
||||
@@ -127,6 +127,10 @@ function createResourceFindingResolutionUrl({
|
||||
|
||||
appendSanitizedProviderTypeFilters(url, filters);
|
||||
|
||||
// Hardcoded FAIL filter AFTER appendSanitizedProviderTypeFilters — .set()
|
||||
// guarantees this wins even if the caller passes filter[status] in filters.
|
||||
url.searchParams.set("filter[status]", "FAIL");
|
||||
|
||||
return url;
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user