mirror of
https://github.com/prowler-cloud/prowler.git
synced 2026-07-23 20:42:02 +00:00
fix(ui): security and crash fixes for findings grouped view (#10514)
This commit is contained in:
Alan Buscaglia
@@ -0,0 +1,156 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Hoisted mocks (must be declared before any imports that need them)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const { fetchMock, getAuthHeadersMock, handleApiResponseMock } = vi.hoisted(
|
||||
() => ({
|
||||
fetchMock: vi.fn(),
|
||||
getAuthHeadersMock: vi.fn(),
|
||||
handleApiResponseMock: vi.fn(),
|
||||
}),
|
||||
);
|
||||
|
||||
vi.mock("@/lib", () => ({
|
||||
apiBaseUrl: "https://api.example.com/api/v1",
|
||||
getAuthHeaders: getAuthHeadersMock,
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/provider-filters", () => ({
|
||||
appendSanitizedProviderFilters: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/server-actions-helper", () => ({
|
||||
handleApiResponse: handleApiResponseMock,
|
||||
}));
|
||||
|
||||
vi.mock("next/navigation", () => ({
|
||||
redirect: vi.fn(),
|
||||
}));
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Imports (after vi.mock declarations)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
import {
|
||||
getFindingGroupResources,
|
||||
getLatestFindingGroupResources,
|
||||
} from "./finding-groups";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Tests
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("getFindingGroupResources — SSRF path traversal protection", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should encode a normal checkId without alteration", async () => {
|
||||
// Given
|
||||
const checkId = "s3_bucket_public_access";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId });
|
||||
|
||||
// Then — URL path must contain encoded checkId, not raw
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
expect(calledUrl).toContain(
|
||||
`/api/v1/finding-groups/${encodeURIComponent(checkId)}/resources`,
|
||||
);
|
||||
});
|
||||
|
||||
it("should encode a checkId containing a forward slash (path traversal attempt)", async () => {
|
||||
// Given — checkId with embedded slash: attacker attempts path traversal
|
||||
const maliciousCheckId = "../../admin/secret";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId: maliciousCheckId });
|
||||
|
||||
// Then — the URL must NOT contain a raw slash from the checkId
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
// The path should NOT end in /resources with traversal segments between
|
||||
expect(url.pathname).not.toContain("/admin/secret/resources");
|
||||
// The encoded checkId must appear in the path
|
||||
expect(url.pathname).toContain(
|
||||
`/finding-groups/${encodeURIComponent(maliciousCheckId)}/resources`,
|
||||
);
|
||||
});
|
||||
|
||||
it("should encode a checkId containing %2F (URL-encoded slash traversal attempt)", async () => {
|
||||
// Given — checkId with %2F: double-encoding traversal attempt
|
||||
const maliciousCheckId = "foo%2Fbar";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId: maliciousCheckId });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.pathname).toContain(
|
||||
`/finding-groups/${encodeURIComponent(maliciousCheckId)}/resources`,
|
||||
);
|
||||
expect(url.pathname).not.toContain("/foo/bar/resources");
|
||||
});
|
||||
|
||||
it("should encode a checkId containing special chars like ? and #", async () => {
|
||||
// Given
|
||||
const maliciousCheckId = "check?admin=true#fragment";
|
||||
|
||||
// When
|
||||
await getFindingGroupResources({ checkId: maliciousCheckId });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
expect(calledUrl).not.toContain("?admin=true");
|
||||
expect(calledUrl.split("?")[0]).toContain(
|
||||
`/finding-groups/${encodeURIComponent(maliciousCheckId)}/resources`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("getLatestFindingGroupResources — SSRF path traversal protection", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
});
|
||||
|
||||
it("should encode a normal checkId without alteration", async () => {
|
||||
// Given
|
||||
const checkId = "iam_user_mfa_enabled";
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
expect(calledUrl).toContain(
|
||||
`/api/v1/finding-groups/latest/${encodeURIComponent(checkId)}/resources`,
|
||||
);
|
||||
});
|
||||
|
||||
it("should encode a checkId containing a forward slash in the latest endpoint", async () => {
|
||||
// Given
|
||||
const maliciousCheckId = "../other-endpoint";
|
||||
|
||||
// When
|
||||
await getLatestFindingGroupResources({ checkId: maliciousCheckId });
|
||||
|
||||
// Then
|
||||
const calledUrl = fetchMock.mock.calls[0][0] as string;
|
||||
const url = new URL(calledUrl);
|
||||
expect(url.pathname).not.toContain("/other-endpoint/resources");
|
||||
expect(url.pathname).toContain(
|
||||
`/finding-groups/latest/${encodeURIComponent(maliciousCheckId)}/resources`,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -90,7 +90,9 @@ export const getFindingGroupResources = async ({
|
||||
}) => {
|
||||
const headers = await getAuthHeaders({ contentType: false });
|
||||
|
||||
const url = new URL(`${apiBaseUrl}/finding-groups/${checkId}/resources`);
|
||||
const url = new URL(
|
||||
`${apiBaseUrl}/finding-groups/${encodeURIComponent(checkId)}/resources`,
|
||||
);
|
||||
|
||||
if (page) url.searchParams.append("page[number]", page.toString());
|
||||
if (pageSize) url.searchParams.append("page[size]", pageSize.toString());
|
||||
@@ -123,7 +125,7 @@ export const getLatestFindingGroupResources = async ({
|
||||
const headers = await getAuthHeaders({ contentType: false });
|
||||
|
||||
const url = new URL(
|
||||
`${apiBaseUrl}/finding-groups/latest/${checkId}/resources`,
|
||||
`${apiBaseUrl}/finding-groups/latest/${encodeURIComponent(checkId)}/resources`,
|
||||
);
|
||||
|
||||
if (page) url.searchParams.append("page[number]", page.toString());
|
||||
|
||||
@@ -267,3 +267,77 @@ describe("resolveFindingIdsByVisibleGroupResources", () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fix 4: Unbounded page[size] cap
|
||||
//
|
||||
// The bug: createResourceFindingResolutionUrl sets page[size]=resourceUids.length
|
||||
// with no upper bound guard. The production fix adds Math.min(resourceUids.length, MAX_PAGE_SIZE)
|
||||
// with MAX_PAGE_SIZE=500 as an explicit defensive cap.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("resolveFindingIds — Fix 4: page[size] explicit cap at MAX_PAGE_SIZE=500", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.stubGlobal("fetch", fetchMock);
|
||||
getAuthHeadersMock.mockResolvedValue({ Authorization: "Bearer token" });
|
||||
});
|
||||
|
||||
it("should use resourceUids.length as page[size] for a small batch (under 500)", async () => {
|
||||
// Given — 3 resources, well under the cap
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
handleApiResponseMock.mockResolvedValue({
|
||||
data: [{ id: "finding-1" }, { id: "finding-2" }, { id: "finding-3" }],
|
||||
});
|
||||
|
||||
// When
|
||||
await resolveFindingIds({
|
||||
checkId: "check-1",
|
||||
resourceUids: ["resource-1", "resource-2", "resource-3"],
|
||||
});
|
||||
|
||||
// Then — page[size] should equal the number of resourceUids (3)
|
||||
const calledUrl = new URL(fetchMock.mock.calls[0][0]);
|
||||
expect(calledUrl.searchParams.get("page[size]")).toBe("3");
|
||||
});
|
||||
|
||||
it("should cap page[size] at 500 when the chunk has exactly 500 UIDs (boundary value)", async () => {
|
||||
// Given — exactly 500 unique UIDs (at the cap boundary)
|
||||
const resourceUids = Array.from({ length: 500 }, (_, i) => `resource-${i}`);
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
|
||||
// When
|
||||
await resolveFindingIds({
|
||||
checkId: "check-1",
|
||||
resourceUids,
|
||||
});
|
||||
|
||||
// Then — page[size] must be exactly 500 (not capped lower)
|
||||
const firstUrl = new URL(fetchMock.mock.calls[0][0] as string);
|
||||
expect(firstUrl.searchParams.get("page[size]")).toBe("500");
|
||||
});
|
||||
|
||||
it("should cap page[size] at 500 even when a chunk would exceed 500 — Math.min guard in URL builder", async () => {
|
||||
// Given — 501 UIDs. The chunker splits into [500, 1].
|
||||
// The FIRST chunk has 500 UIDs → page[size] should be 500 (Math.min(500, 500)).
|
||||
// The SECOND chunk has 1 UID → page[size] should be 1 (Math.min(1, 500)).
|
||||
// This proves the Math.min cap fires correctly on every chunk.
|
||||
const resourceUids = Array.from({ length: 501 }, (_, i) => `resource-${i}`);
|
||||
fetchMock.mockResolvedValue(new Response("", { status: 200 }));
|
||||
handleApiResponseMock.mockResolvedValue({ data: [] });
|
||||
|
||||
// When
|
||||
await resolveFindingIds({
|
||||
checkId: "check-1",
|
||||
resourceUids,
|
||||
});
|
||||
|
||||
// Then — two fetch calls: one for 500 UIDs, one for 1 UID
|
||||
expect(fetchMock).toHaveBeenCalledTimes(2);
|
||||
const firstUrl = new URL(fetchMock.mock.calls[0][0] as string);
|
||||
const secondUrl = new URL(fetchMock.mock.calls[1][0] as string);
|
||||
expect(firstUrl.searchParams.get("page[size]")).toBe("500");
|
||||
expect(secondUrl.searchParams.get("page[size]")).toBe("1");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -14,6 +14,9 @@ const FINDING_IDS_RESOLUTION_CONCURRENCY = 4;
|
||||
const FINDING_GROUP_RESOURCES_RESOLUTION_PAGE_SIZE = 500;
|
||||
const FINDING_FIELDS = "uid";
|
||||
|
||||
/** Explicit upper bound for page[size] in resource-finding resolution requests. */
|
||||
const MAX_RESOURCE_FINDING_PAGE_SIZE = 500;
|
||||
|
||||
interface ResolveFindingIdsByCheckIdsParams {
|
||||
checkIds: string[];
|
||||
filters?: Record<string, string>;
|
||||
@@ -117,7 +120,10 @@ function createResourceFindingResolutionUrl({
|
||||
url.searchParams.append("filter[check_id]", checkId);
|
||||
url.searchParams.append("filter[resource_uid__in]", resourceUids.join(","));
|
||||
url.searchParams.append("filter[muted]", "false");
|
||||
url.searchParams.append("page[size]", resourceUids.length.toString());
|
||||
url.searchParams.append(
|
||||
"page[size]",
|
||||
Math.min(resourceUids.length, MAX_RESOURCE_FINDING_PAGE_SIZE).toString(),
|
||||
);
|
||||
|
||||
appendSanitizedProviderTypeFilters(url, filters);
|
||||
|
||||
|
||||
@@ -0,0 +1,313 @@
|
||||
/**
|
||||
* Tests for inline-resource-container.tsx
|
||||
*
|
||||
* Fix 2: SSR crash — createPortal must be guarded by isMounted state
|
||||
* Fix 3: Invalid Tailwind class — mt-[-10] → -mt-2.5
|
||||
*/
|
||||
|
||||
import { act, render, screen } from "@testing-library/react";
|
||||
import type {
|
||||
ComponentType,
|
||||
HTMLAttributes,
|
||||
ReactNode,
|
||||
TdHTMLAttributes,
|
||||
} from "react";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Mocks
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
// Hoist createPortal spy so it is available when the vi.mock factory runs.
|
||||
// vi.hoisted() runs before all imports, making the spy available in the factory.
|
||||
const { createPortalSpy } = vi.hoisted(() => ({
|
||||
createPortalSpy: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("react-dom", async (importOriginal) => {
|
||||
const original = await importOriginal<typeof import("react-dom")>();
|
||||
// Delegate to the real createPortal so other tests keep working,
|
||||
// but allow spy assertions on call count and timing.
|
||||
createPortalSpy.mockImplementation(original.createPortal);
|
||||
return {
|
||||
...original,
|
||||
createPortal: createPortalSpy,
|
||||
};
|
||||
});
|
||||
|
||||
// Mock next/navigation before component import
|
||||
vi.mock("next/navigation", () => ({
|
||||
useSearchParams: () => new URLSearchParams(),
|
||||
}));
|
||||
|
||||
// Mock heavy deps to avoid cascading import errors
|
||||
vi.mock("@/actions/findings/findings-by-resource", () => ({
|
||||
resolveFindingIds: vi.fn().mockResolvedValue([]),
|
||||
}));
|
||||
|
||||
vi.mock("@/hooks/use-infinite-resources", () => ({
|
||||
useInfiniteResources: () => ({
|
||||
sentinelRef: vi.fn(),
|
||||
refresh: vi.fn(),
|
||||
loadMore: vi.fn(),
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@/hooks/use-scroll-hint", () => ({
|
||||
useScrollHint: () => ({
|
||||
containerRef: vi.fn(),
|
||||
sentinelRef: vi.fn(),
|
||||
showScrollHint: false,
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib", () => ({
|
||||
hasDateOrScanFilter: vi.fn().mockReturnValue(false),
|
||||
}));
|
||||
|
||||
vi.mock("./column-finding-resources", () => ({
|
||||
getColumnFindingResources: vi.fn().mockReturnValue([]),
|
||||
}));
|
||||
|
||||
vi.mock("./findings-selection-context", () => ({
|
||||
FindingsSelectionContext: {
|
||||
Provider: ({ children }: { children: ReactNode; value: unknown }) => (
|
||||
<>{children}</>
|
||||
),
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("./resource-detail-drawer", () => ({
|
||||
ResourceDetailDrawer: () => (
|
||||
<div data-testid="resource-detail-drawer">Drawer</div>
|
||||
),
|
||||
useResourceDetailDrawer: () => ({
|
||||
isOpen: false,
|
||||
isLoading: false,
|
||||
isNavigating: false,
|
||||
checkMeta: null,
|
||||
currentIndex: 0,
|
||||
totalResources: 0,
|
||||
currentFinding: null,
|
||||
otherFindings: [],
|
||||
openDrawer: vi.fn(),
|
||||
closeDrawer: vi.fn(),
|
||||
navigatePrev: vi.fn(),
|
||||
navigateNext: vi.fn(),
|
||||
refetchCurrent: vi.fn(),
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@/components/shadcn/skeleton/skeleton", () => ({
|
||||
Skeleton: () => <div data-testid="skeleton" />,
|
||||
}));
|
||||
|
||||
vi.mock("@/components/shadcn/spinner/spinner", () => ({
|
||||
Spinner: () => <div data-testid="spinner" />,
|
||||
}));
|
||||
|
||||
vi.mock("@/components/ui/table", () => ({
|
||||
TableCell: ({
|
||||
children,
|
||||
...props
|
||||
}: { children?: ReactNode } & TdHTMLAttributes<HTMLTableCellElement>) => (
|
||||
<td {...props}>{children}</td>
|
||||
),
|
||||
TableRow: ({
|
||||
children,
|
||||
...props
|
||||
}: { children?: ReactNode } & HTMLAttributes<HTMLTableRowElement>) => (
|
||||
<tr {...props}>{children}</tr>
|
||||
),
|
||||
}));
|
||||
|
||||
// framer-motion: render children immediately
|
||||
vi.mock("framer-motion", () => ({
|
||||
AnimatePresence: ({ children }: { children: ReactNode }) => <>{children}</>,
|
||||
motion: {
|
||||
div: ({
|
||||
children,
|
||||
...props
|
||||
}: { children?: ReactNode } & HTMLAttributes<HTMLDivElement>) => (
|
||||
<div {...props}>{children}</div>
|
||||
),
|
||||
},
|
||||
}));
|
||||
|
||||
// lucide-react
|
||||
vi.mock("lucide-react", () => ({
|
||||
ChevronsDown: () => <svg data-testid="chevrons-down" />,
|
||||
}));
|
||||
|
||||
// @tanstack/react-table
|
||||
vi.mock("@tanstack/react-table", () => ({
|
||||
flexRender: (Component: ComponentType | string, ctx: unknown) => {
|
||||
if (typeof Component === "function")
|
||||
return <Component {...(ctx as object)} />;
|
||||
return Component;
|
||||
},
|
||||
getCoreRowModel: () => vi.fn(),
|
||||
useReactTable: () => ({
|
||||
getRowModel: () => ({ rows: [] }),
|
||||
getVisibleLeafColumns: () => [],
|
||||
}),
|
||||
}));
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Imports (after mocks)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
import type { FindingGroupRow } from "@/types";
|
||||
|
||||
import { InlineResourceContainer } from "./inline-resource-container";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fixtures
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const mockGroup: FindingGroupRow = {
|
||||
id: "group-1",
|
||||
rowType: "group",
|
||||
checkId: "s3_bucket_public_access",
|
||||
checkTitle: "S3 Bucket Public Access Check",
|
||||
resourcesTotal: 3,
|
||||
resourcesFail: 3,
|
||||
newCount: 0,
|
||||
changedCount: 0,
|
||||
mutedCount: 0,
|
||||
severity: "high",
|
||||
status: "FAIL",
|
||||
providers: ["aws"],
|
||||
updatedAt: "2024-01-01T00:00:00Z",
|
||||
};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fix 2: SSR crash — portal only mounted after client-side mount
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("InlineResourceContainer — Fix 2: SSR portal guard", () => {
|
||||
beforeEach(() => {
|
||||
createPortalSpy.mockClear();
|
||||
});
|
||||
|
||||
it("should render without crash when document.body exists (JSDOM)", async () => {
|
||||
// Given — JSDOM has document.body; this verifies the happy path
|
||||
let renderError: Error | null = null;
|
||||
|
||||
// When
|
||||
try {
|
||||
await act(async () => {
|
||||
render(
|
||||
<table>
|
||||
<tbody>
|
||||
<InlineResourceContainer
|
||||
group={mockGroup}
|
||||
resourceSearch=""
|
||||
columnCount={10}
|
||||
onResourceSelectionChange={vi.fn()}
|
||||
/>
|
||||
</tbody>
|
||||
</table>,
|
||||
);
|
||||
});
|
||||
} catch (e) {
|
||||
renderError = e as Error;
|
||||
}
|
||||
|
||||
// Then — component must not throw
|
||||
expect(renderError).toBeNull();
|
||||
});
|
||||
|
||||
it("should render the portal content (ResourceDetailDrawer) after mount", async () => {
|
||||
// Given
|
||||
await act(async () => {
|
||||
render(
|
||||
<table>
|
||||
<tbody>
|
||||
<InlineResourceContainer
|
||||
group={mockGroup}
|
||||
resourceSearch=""
|
||||
columnCount={10}
|
||||
onResourceSelectionChange={vi.fn()}
|
||||
/>
|
||||
</tbody>
|
||||
</table>,
|
||||
);
|
||||
});
|
||||
|
||||
// Then — drawer appears in the document via portal after mount
|
||||
expect(screen.getByTestId("resource-detail-drawer")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should NOT call createPortal synchronously on initial render — only after useEffect fires (isMounted guard)", () => {
|
||||
// Given — createPortalSpy is reset by beforeEach, so call count starts at 0.
|
||||
// Before the fix: createPortal runs on the initial synchronous render → crash in SSR.
|
||||
// After the fix: createPortal is guarded by isMounted (set via useEffect).
|
||||
// useEffect fires AFTER commit, so createPortal must NOT be called
|
||||
// during the synchronous render phase.
|
||||
|
||||
// When — render inside synchronous act() and capture spy count INSIDE the callback.
|
||||
// In React 19, act() DOES flush effects — but the callback runs BEFORE effects drain.
|
||||
// So: the callback body executes first (synchronous render only), THEN act() flushes
|
||||
// pending effects after the callback returns.
|
||||
// Capturing spy state inside the callback captures the pre-effect state.
|
||||
let portalCallsAfterSyncRender = 0;
|
||||
act(() => {
|
||||
render(
|
||||
<table>
|
||||
<tbody>
|
||||
<InlineResourceContainer
|
||||
group={mockGroup}
|
||||
resourceSearch=""
|
||||
columnCount={10}
|
||||
onResourceSelectionChange={vi.fn()}
|
||||
/>
|
||||
</tbody>
|
||||
</table>,
|
||||
);
|
||||
// Capture call count here — inside the callback = after synchronous render,
|
||||
// but BEFORE act() drains pending effects (effects flush after this returns).
|
||||
portalCallsAfterSyncRender = createPortalSpy.mock.calls.length;
|
||||
});
|
||||
// After the callback returns, act() flushes pending effects (isMounted = true → re-render)
|
||||
|
||||
// Then — createPortal must NOT have been called during the synchronous render phase
|
||||
// (isMounted starts as false, createPortal is inside {isMounted && createPortal(...)})
|
||||
expect(portalCallsAfterSyncRender).toBe(0);
|
||||
|
||||
// After act() completes, effects have flushed → isMounted = true → re-render → createPortal called
|
||||
expect(createPortalSpy).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fix 3: Invalid Tailwind class — -mt-2.5 instead of mt-[-10]
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("InlineResourceContainer — Fix 3: Valid Tailwind class", () => {
|
||||
it("should use -mt-2.5 (valid Tailwind scale) on the inner resource table", async () => {
|
||||
// Given
|
||||
let container!: HTMLElement;
|
||||
await act(async () => {
|
||||
const result = render(
|
||||
<table>
|
||||
<tbody>
|
||||
<InlineResourceContainer
|
||||
group={mockGroup}
|
||||
resourceSearch=""
|
||||
columnCount={10}
|
||||
onResourceSelectionChange={vi.fn()}
|
||||
/>
|
||||
</tbody>
|
||||
</table>,
|
||||
);
|
||||
container = result.container;
|
||||
});
|
||||
|
||||
// Then — the inner table element must have class "-mt-2.5", NOT "mt-[-10]"
|
||||
const innerTable = container.querySelector("table table");
|
||||
expect(innerTable).not.toBeNull();
|
||||
expect(innerTable!.className).toContain("-mt-2.5");
|
||||
expect(innerTable!.className).not.toContain("mt-[-10]");
|
||||
});
|
||||
});
|
||||
@@ -10,7 +10,7 @@ import {
|
||||
import { AnimatePresence, motion } from "framer-motion";
|
||||
import { ChevronsDown } from "lucide-react";
|
||||
import { useSearchParams } from "next/navigation";
|
||||
import { useImperativeHandle, useRef, useState } from "react";
|
||||
import { useEffect, useImperativeHandle, useRef, useState } from "react";
|
||||
import { createPortal } from "react-dom";
|
||||
|
||||
import { resolveFindingIds } from "@/actions/findings/findings-by-resource";
|
||||
@@ -133,6 +133,11 @@ export function InlineResourceContainer({
|
||||
const [rowSelection, setRowSelection] = useState<RowSelectionState>({});
|
||||
const [resources, setResources] = useState<FindingResourceRow[]>([]);
|
||||
const [isLoading, setIsLoading] = useState(true);
|
||||
const [isMounted, setIsMounted] = useState(false);
|
||||
|
||||
useEffect(() => {
|
||||
setIsMounted(true);
|
||||
}, []);
|
||||
|
||||
// Scroll hint: shows "scroll for more" when content overflows
|
||||
const {
|
||||
@@ -308,7 +313,7 @@ export function InlineResourceContainer({
|
||||
className="max-h-[440px] overflow-y-auto pl-6"
|
||||
>
|
||||
{/* Resource rows or skeleton placeholder */}
|
||||
<table className="mt-[-10] w-full border-separate border-spacing-y-4">
|
||||
<table className="-mt-2.5 w-full border-separate border-spacing-y-4">
|
||||
<tbody>
|
||||
{isLoading && rows.length === 0 ? (
|
||||
Array.from({
|
||||
@@ -390,25 +395,26 @@ export function InlineResourceContainer({
|
||||
</td>
|
||||
</tr>
|
||||
|
||||
{createPortal(
|
||||
<ResourceDetailDrawer
|
||||
open={drawer.isOpen}
|
||||
onOpenChange={(open) => {
|
||||
if (!open) drawer.closeDrawer();
|
||||
}}
|
||||
isLoading={drawer.isLoading}
|
||||
isNavigating={drawer.isNavigating}
|
||||
checkMeta={drawer.checkMeta}
|
||||
currentIndex={drawer.currentIndex}
|
||||
totalResources={drawer.totalResources}
|
||||
currentFinding={drawer.currentFinding}
|
||||
otherFindings={drawer.otherFindings}
|
||||
onNavigatePrev={drawer.navigatePrev}
|
||||
onNavigateNext={drawer.navigateNext}
|
||||
onMuteComplete={handleDrawerMuteComplete}
|
||||
/>,
|
||||
document.body,
|
||||
)}
|
||||
{isMounted &&
|
||||
createPortal(
|
||||
<ResourceDetailDrawer
|
||||
open={drawer.isOpen}
|
||||
onOpenChange={(open) => {
|
||||
if (!open) drawer.closeDrawer();
|
||||
}}
|
||||
isLoading={drawer.isLoading}
|
||||
isNavigating={drawer.isNavigating}
|
||||
checkMeta={drawer.checkMeta}
|
||||
currentIndex={drawer.currentIndex}
|
||||
totalResources={drawer.totalResources}
|
||||
currentFinding={drawer.currentFinding}
|
||||
otherFindings={drawer.otherFindings}
|
||||
onNavigatePrev={drawer.navigatePrev}
|
||||
onNavigateNext={drawer.navigateNext}
|
||||
onMuteComplete={handleDrawerMuteComplete}
|
||||
/>,
|
||||
document.body,
|
||||
)}
|
||||
</FindingsSelectionContext.Provider>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -206,7 +206,9 @@ describe("useInfiniteResources", () => {
|
||||
// Then — only page 1 was fetched, never page 2
|
||||
const calls =
|
||||
findingGroupActionsMock.getLatestFindingGroupResources.mock.calls;
|
||||
const pageNumbers = calls.map((c: { page: number }[]) => c[0].page);
|
||||
const pageNumbers = calls.map(
|
||||
(c: unknown[]) => (c[0] as { page: number }).page,
|
||||
);
|
||||
expect(pageNumbers.every((p: number) => p === 1)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -381,4 +383,157 @@ describe("useInfiniteResources", () => {
|
||||
).toHaveBeenCalledWith(expect.objectContaining({ filters }));
|
||||
});
|
||||
});
|
||||
|
||||
describe("when refresh() fires while loadNextPage is in-flight (race condition — Fix 5)", () => {
|
||||
it("should discard in-flight page 2 and fetch page 1 when refresh fires during loadNextPage", async () => {
|
||||
// Given — page 1 has 2 pages total, page 2 hangs indefinitely
|
||||
const page1Response = makeApiResponse(
|
||||
Array.from({ length: 10 }, (_, i) => ({ id: `r${i}` })),
|
||||
{ pages: 2 },
|
||||
);
|
||||
const page1Adapted = Array.from({ length: 10 }, (_, i) =>
|
||||
fakeResource(`r${i}`),
|
||||
);
|
||||
|
||||
const page2Response = makeApiResponse(
|
||||
Array.from({ length: 5 }, (_, i) => ({ id: `r${10 + i}` })),
|
||||
{ pages: 2 },
|
||||
);
|
||||
|
||||
const refreshPage1Response = makeApiResponse([{ id: "r-fresh-1" }], {
|
||||
pages: 1,
|
||||
});
|
||||
const refreshPage1Adapted = [fakeResource("r-fresh-1")];
|
||||
|
||||
// page 2 hangs until we explicitly resolve it
|
||||
let resolveNextPage: (v: unknown) => void = () => {};
|
||||
const hangingPage2 = new Promise((r) => {
|
||||
resolveNextPage = r;
|
||||
});
|
||||
|
||||
let callCount = 0;
|
||||
findingGroupActionsMock.getLatestFindingGroupResources.mockImplementation(
|
||||
(args: { page: number }) => {
|
||||
callCount++;
|
||||
if (callCount === 1) {
|
||||
return Promise.resolve(page1Response);
|
||||
}
|
||||
if (args.page === 2) {
|
||||
return hangingPage2;
|
||||
}
|
||||
return Promise.resolve(refreshPage1Response);
|
||||
},
|
||||
);
|
||||
|
||||
findingGroupActionsMock.adaptFindingGroupResourcesResponse
|
||||
.mockReturnValueOnce(page1Adapted)
|
||||
.mockReturnValue(refreshPage1Adapted);
|
||||
|
||||
const onSetResources = vi.fn();
|
||||
const onAppendResources = vi.fn();
|
||||
|
||||
// When — mount and wait for page 1
|
||||
const { result } = renderHook(() =>
|
||||
useInfiniteResources(
|
||||
defaultOptions({ onSetResources, onAppendResources }),
|
||||
),
|
||||
);
|
||||
await flushAsync();
|
||||
|
||||
expect(onSetResources).toHaveBeenCalledWith(page1Adapted, true);
|
||||
|
||||
// Trigger loadNextPage (increments pageRef to 2 in buggy code)
|
||||
const sentinel = document.createElement("div");
|
||||
act(() => {
|
||||
result.current.sentinelRef(sentinel);
|
||||
});
|
||||
act(() => {
|
||||
triggerIntersection();
|
||||
});
|
||||
// Do NOT flush — page 2 is hanging in-flight
|
||||
|
||||
// Refresh fires while page 2 is in-flight
|
||||
act(() => {
|
||||
result.current.refresh();
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
// Resolve hanging page 2 after refresh (simulates late stale response)
|
||||
await act(async () => {
|
||||
resolveNextPage(page2Response);
|
||||
await new Promise((r) => setTimeout(r, 0));
|
||||
});
|
||||
|
||||
// Then — the aborted page 2 must NOT deliver resources (signal.aborted check)
|
||||
expect(onAppendResources).not.toHaveBeenCalled();
|
||||
|
||||
// The refresh must have fetched page 1 and delivered fresh resources
|
||||
expect(onSetResources).toHaveBeenCalledWith(refreshPage1Adapted, false);
|
||||
|
||||
// The refresh call must request page=1 (not page=3 due to stale pageRef)
|
||||
// Exact call sequence: [0]=initial page 1, [1]=loadNextPage page 2, [2]=refresh page 1
|
||||
const calls =
|
||||
findingGroupActionsMock.getLatestFindingGroupResources.mock.calls;
|
||||
expect((calls[0][0] as { page: number }).page).toBe(1); // initial fetch
|
||||
expect((calls[1][0] as { page: number }).page).toBe(2); // loadNextPage
|
||||
expect((calls[2][0] as { page: number }).page).toBe(1); // refresh
|
||||
});
|
||||
|
||||
it("should fetch sequential pages without skipping when loadNextPage is used normally", async () => {
|
||||
// Given — page 1 has 3 pages; pages load sequentially
|
||||
const makePageResponse = (startIdx: number, total: number) =>
|
||||
makeApiResponse(
|
||||
Array.from({ length: 5 }, (_, i) => ({ id: `r${startIdx + i}` })),
|
||||
{ pages: total },
|
||||
);
|
||||
|
||||
findingGroupActionsMock.getLatestFindingGroupResources
|
||||
.mockResolvedValueOnce(makePageResponse(0, 3)) // page 1
|
||||
.mockResolvedValueOnce(makePageResponse(5, 3)) // page 2
|
||||
.mockResolvedValueOnce(makePageResponse(10, 3)); // page 3
|
||||
|
||||
findingGroupActionsMock.adaptFindingGroupResourcesResponse
|
||||
.mockReturnValueOnce(
|
||||
Array.from({ length: 5 }, (_, i) => fakeResource(`r${i}`)),
|
||||
)
|
||||
.mockReturnValueOnce(
|
||||
Array.from({ length: 5 }, (_, i) => fakeResource(`r${5 + i}`)),
|
||||
)
|
||||
.mockReturnValueOnce(
|
||||
Array.from({ length: 5 }, (_, i) => fakeResource(`r${10 + i}`)),
|
||||
);
|
||||
|
||||
const onAppendResources = vi.fn();
|
||||
|
||||
// When — mount and wait for page 1
|
||||
const { result } = renderHook(() =>
|
||||
useInfiniteResources(defaultOptions({ onAppendResources })),
|
||||
);
|
||||
await flushAsync();
|
||||
|
||||
// Attach sentinel
|
||||
const sentinel = document.createElement("div");
|
||||
act(() => {
|
||||
result.current.sentinelRef(sentinel);
|
||||
});
|
||||
|
||||
// Load page 2
|
||||
act(() => {
|
||||
triggerIntersection();
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
// Load page 3
|
||||
act(() => {
|
||||
triggerIntersection();
|
||||
});
|
||||
await flushAsync();
|
||||
|
||||
// Then — pages were fetched in order: 2, 3 (not 2, 4 due to double-increment)
|
||||
const calls =
|
||||
findingGroupActionsMock.getLatestFindingGroupResources.mock.calls;
|
||||
expect(calls[1][0].page).toBe(2);
|
||||
expect(calls[2][0].page).toBe(3);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -111,6 +111,11 @@ export function useInfiniteResources({
|
||||
const totalPages = response?.meta?.pagination?.pages ?? 1;
|
||||
const hasMore = page < totalPages;
|
||||
|
||||
// Commit the page number only after a successful (non-aborted) fetch.
|
||||
// This prevents a premature pageRef increment from loadNextPage being
|
||||
// permanently committed if a concurrent abort fires before fetchPage
|
||||
// starts executing.
|
||||
pageRef.current = page;
|
||||
hasMoreRef.current = hasMore;
|
||||
|
||||
if (append) {
|
||||
@@ -160,9 +165,11 @@ export function useInfiniteResources({
|
||||
)
|
||||
return;
|
||||
|
||||
const nextPage = pageRef.current + 1;
|
||||
pageRef.current = nextPage;
|
||||
fetchPage(nextPage, true, currentCheckIdRef.current, signal);
|
||||
// Pass the next page number as an argument without pre-committing
|
||||
// pageRef.current. The fetchPage function commits pageRef.current = page
|
||||
// only after a successful (non-aborted) response, eliminating the race
|
||||
// where a concurrent abort would leave pageRef permanently incremented.
|
||||
fetchPage(pageRef.current + 1, true, currentCheckIdRef.current, signal);
|
||||
}
|
||||
|
||||
// IntersectionObserver callback
|
||||
@@ -195,7 +202,6 @@ export function useInfiniteResources({
|
||||
const controller = new AbortController();
|
||||
controllerRef.current = controller;
|
||||
|
||||
pageRef.current = 1;
|
||||
hasMoreRef.current = true;
|
||||
isLoadingRef.current = false;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user