fix(ui): security and crash fixes for findings grouped view#10514
Conversation
- Fix SSRF path traversal: encode checkId with encodeURIComponent in getFindingGroupResources and getLatestFindingGroupResources URL paths - Fix SSR crash: add isMounted guard for createPortal(document.body) in InlineResourceContainer to prevent document access during server render - Fix invalid Tailwind: replace mt-[-10] (no unit) with -mt-2.5 scale token - Fix unbounded page[size]: cap at 500 via Math.min in URL builder as defense-in-depth alongside existing chunking - Fix pageRef race condition: move page counter increment inside fetchPage after successful non-aborted response, remove redundant pre-set from refresh() to prevent permanent page skip on concurrent abort 26 tests across 4 test suites (2 new, 2 extended). All passing. TypeScript: 0 errors in changed files. ESLint: 0 errors. Claude Code validation: PASSED. Pre-existing healthcheck failures (.next/types, @codemirror) unrelated.
alejandrobailo
left a comment
There was a problem hiding this comment.
Review: Approve
Solid security and crash fixes, well-tested with TDD. A couple of observations:
1. pageRef race condition fix is incomplete in isolation
The fix correctly moves pageRef.current = page into fetchPage (post-success) and removes the pre-commit from loadNextPage. However, it also removes pageRef.current = 1 from refresh().
This creates a window where if refresh() is called and the subsequent fetchPage(1, ...) is aborted before completing (e.g., another refresh fires), pageRef.current retains its previous value. Then loadNextPage would calculate the wrong next page.
PR #10515 silently re-adds pageRef.current = 1 in refresh(), which fixes it. But the remove-in-#10514/add-in-#10515 dance is confusing for reviewers. Ideally this should have been complete within this PR.
Not blocking since #10515 fixes it, but worth noting.
2. Duplicate constant
MAX_RESOURCE_FINDING_PAGE_SIZE = 500 is defined alongside the existing FINDING_GROUP_RESOURCES_RESOLUTION_PAGE_SIZE = 500 in findings-by-resource.ts. Same value, same semantic purpose. Consider unifying or documenting why they're separate.
5306bb1
into
fix/ui-findings-groups-improvements
🔗 Part of Chained PRs
fix/ui-findings-groups-improvementsChain Overview
Context
Post-merge security and crash fixes for the findings grouped view (PR #10425 by @alejandrobailo). Issues were identified through adversarial code review (Judgment Day protocol with 2 independent blind judges).
Description
5 fixes, 26 tests (17 new), implemented with Strict TDD (Red → Green → Triangulate → Refactor):
SSRF/path traversal —
checkIdwas interpolated directly into URL paths without encoding. AddedencodeURIComponent(checkId)in bothgetFindingGroupResourcesandgetLatestFindingGroupResources. Tests cover: normal IDs, path traversal (../../admin), URL-encoded slashes (%2F), and query/fragment injection (?,#).SSR crash —
createPortal(_, document.body)was called unconditionally, crashing during server-side rendering wheredocumentis undefined. AddedisMountedstate guard withuseEffect. Tests verify portal is NOT called synchronously (before effects fire) and IS called after mount.Invalid Tailwind —
mt-[-10]has no unit, so Tailwind 4 generates no CSS (broken layout). Replaced with-mt-2.5(10px in Tailwind's 4px scale). Test asserts correct class and absence of invalid class.Unbounded page[size] —
page[size]was set toresourceUids.lengthwith no cap. AddedMath.min(length, 500)as defense-in-depth alongside existing chunking. Tests verify: boundary (500), over-cap (501→split into [500,1]), and under-cap (3).pageRef race condition —
loadNextPageincrementedpageRef.currentbeforefetchPage, causing permanent page skip if abort fired concurrently. Moved increment insidefetchPageafter successful response. Removed redundantpageRef.current = 1pre-set fromrefresh(). Tests verify: race between refresh and loadNextPage, sequential page ordering.TDD Cycle Evidence
finding-groups.test.ts/,%2F,?#, normalinline-resource-container.test.tsxinline-resource-container.test.tsxfindings-by-resource.test.tsuse-infinite-resources.test.tsJudgment Day: 3 rounds of adversarial review (6 judge delegations). Round 1 caught false-positive tests. Round 2 caught TypeScript error + "use server" export issue. Round 3: both judges CLEAN.
Steps to review
finding-groups.test.ts— verify SSRF test cases cover the attack vectorsinline-resource-container.test.tsx— verifycreatePortalspy correctly tests the SSR guarduse-infinite-resources.test.ts— verify race condition test setupcd ui && pnpm vitest run actions/finding-groups/finding-groups.test.ts actions/findings/findings-by-resource.test.ts components/findings/table/inline-resource-container.test.tsx hooks/use-infinite-resources.test.ts— all 26 should passChecklist
Community Checklist
UI
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.