From f75993325373cbd8a9d93e405664b19411a54346 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Mon, 30 Mar 2026 16:08:49 +0200 Subject: [PATCH] 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. --- .../finding-groups/finding-groups.test.ts | 231 +++++++++++++++++- ui/actions/finding-groups/finding-groups.ts | 18 ++ .../findings/findings-by-resource.test.ts | 79 ++++++ ui/actions/findings/findings-by-resource.ts | 4 + 4 files changed, 331 insertions(+), 1 deletion(-) diff --git a/ui/actions/finding-groups/finding-groups.test.ts b/ui/actions/finding-groups/finding-groups.test.ts index c73f9e8be5..0d8c2df53a 100644 --- a/ui/actions/finding-groups/finding-groups.test.ts +++ b/ui/actions/finding-groups/finding-groups.test.ts @@ -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) => { + 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); + }); +}); diff --git a/ui/actions/finding-groups/finding-groups.ts b/ui/actions/finding-groups/finding-groups.ts index 25c1ba26e5..b31d7a5bfc 100644 --- a/ui/actions/finding-groups/finding-groups.ts +++ b/ui/actions/finding-groups/finding-groups.ts @@ -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, diff --git a/ui/actions/findings/findings-by-resource.test.ts b/ui/actions/findings/findings-by-resource.test.ts index 1e3f65246f..c637e5f707 100644 --- a/ui/actions/findings/findings-by-resource.test.ts +++ b/ui/actions/findings/findings-by-resource.test.ts @@ -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 // diff --git a/ui/actions/findings/findings-by-resource.ts b/ui/actions/findings/findings-by-resource.ts index 9d7afccb40..12900ffdca 100644 --- a/ui/actions/findings/findings-by-resource.ts +++ b/ui/actions/findings/findings-by-resource.ts @@ -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; }