Skip to content

Export findings from the backend instead of loaded page state - #2453

Closed
Subramaniyajothi6 wants to merge 954 commits into
utksh1:mainfrom
Subramaniyajothi6:feature/bulk-export-findings-1875
Closed

Export findings from the backend instead of loaded page state#2453
Subramaniyajothi6 wants to merge 954 commits into
utksh1:mainfrom
Subramaniyajothi6:feature/bulk-export-findings-1875

Conversation

@Subramaniyajothi6

@Subramaniyajothi6 Subramaniyajothi6 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Closes #1875

Problem

Findings export was assembled entirely in the browser:

const selectedFindings = findings.filter((f) => selectedIds.has(f.id))
exportFindingsAsCSV(selectedFindings)

findings is React state, so only findings already scrolled into memory could ever end up in the file. Select across pages and you got whatever happened to be loaded — silently, with no indication anything was missing.

Changes

POST /api/v1/findings/export resolves a selection of finding ids against the database and streams csv, json, or sarif back.

Three selection cases, deliberately distinct:

finding_ids Meaning
omitted / null everything the caller owns — this is what makes "across all pages" possible without the client loading those pages first
["a", "b"] exactly those, duplicates collapsed
[] nothing

That last row matters: an empty selection must never be read as "everything". It is pinned by a test.

Memory

Findings are read in batches of SECUSCAN_EXPORT_BATCH_SIZE (500) and serialized as they go, so memory tracks the batch size rather than the size of the export. Both the read and the id-count are batched — an IN list is one bound parameter per id, and every database has a ceiling on those.

SARIF is the exception and is assembled in full. Its deduplicated rules array lives in the tool driver ahead of the results that index into it, so the whole set has to be known before the first byte is correct. SECUSCAN_MAX_EXPORT_FINDINGS (10000) is what bounds that; it rejects an oversized request rather than truncating it, so an export is never quietly partial.

Security

  • Owner-scoped. Every query carries owner_id. Ids belonging to someone else are skipped rather than rejected, so the endpoint cannot be used to test whether a finding exists — and X-Export-Finding-Count does not confirm them either. Both are tested.
  • Redacted. target, description, remediation, proof, confidence_reason, evidence, and metadata go through the same redaction.py helpers task reports use. One function gates every byte leaving the module, so CSV, JSON, and SARIF cannot drift apart.
  • Rate limited on report_download_limiter, shared with report downloads.
  • owner_id is dropped from the JSON export — constant for the whole file and useless inside it.
  • Audit-logged as findings_exported with the format and the count.

Frontend

The Bulk Export control is now visible whenever there are findings, and its label states the scope: Export Selected (N) when something is checked, Export All (N) when nothing is. SARIF joins CSV and JSON in the dropdown. The button disables while an export is in flight, and a failure raises an error toast instead of downloading an empty file.

Removed

serializeFindingsToCSV, escapeCSV, exportFindingsAsCSV, and exportFindingsAsJSON are gone — the endpoint replaced their only caller. Their column contract did not go with them; it is now asserted in testing/backend/unit/test_finding_export.py, including the comma/quote/newline escaping the old frontend test covered. downloadFile is kept as a generic helper (now a one-liner over downloadBlob).

The CSV keeps the same 14 columns in the same order, so existing scripts keep working. One deviation: line endings are now RFC 4180 CRLF, matching the task-report CSV, where the browser emitted bare LF.

Verification

  • testing/backend/integration/test_findings_export.py (new) — 28 tests
  • testing/backend/unit/test_finding_export.py (new) — 22 tests
  • Full backend integration — 344 passed, 9 skipped
  • Full backend unit — 2343 passed, 21 skipped
  • Full vitest run64 files, 566 tests passed
  • ruff check backend testing/backend — clean
  • tsc --noEmit — clean
  • quality-gate.cjs — 13 passed, 0 failed (its 1 warning is a pre-existing 2000ms animation, unrelated)

Mutation-checked. Dropping owner scoping, treating [] as "everything", removing redaction, leaking owner_id, skipping the cap, un-batching the count, and removing the de-duplication each fail a test.

Two things I want to flag rather than bury:

  1. test_duplicate_ids_do_not_duplicate_rows passes with or without the explicit de-duplication, because IN (?, ?, ?) already collapses repeats. It is kept as a contract guard against a future rewrite that resolves ids one query at a time, and says so.
  2. The ORDER BY ..., id tiebreaker survives mutation under SQLite, which happens to page tied rows consistently. It is there for PostgreSQL, which is under no such obligation. Also noted in the code.

Overlap with #2394 — please read before merging either

@namann5's #2394 neutralizes CSV formula injection (CWE-1236) in escapeCSV and in ReportGenerator._sanitize_csv_cell. This PR deletes escapeCSV and moves findings-CSV generation to the backend, so merging the two naively would have quietly reopened that hole for the findings export.

I checked rather than assumed, and the new writer was vulnerable:

f-1,"=HYPERLINK(""http://evil.example"",""click"")",,,+cmd|'/C calc'!A0,…

So finding_export.sanitize_csv_cell is now applied to every cell, deliberately mirroring #2394's semantics (=, +, -, @ → single-quote prefix) so the report CSV and the findings CSV cannot disagree about what is safe to hand a spreadsheet. Covered by unit tests and an end-to-end test that seeds a hostile finding title and asserts what lands in the file, and mutation-checked three ways (guard disabled, narrowed to = only, not wired into the row builder).

Once both land, the two helpers should be folded into one. I left them separate only because #2394 is unmerged and this PR has to be safe standing alone. Happy to do that consolidation as a follow-up, or to rebase onto #2394 and use their helper directly if you would rather merge that one first.

Note on merge order

Conflicts, both verified with git merge-tree:

With Files Nature
#2367 (Escape closes popovers) frontend/src/pages/Findings.tsx purely additive — import block + state block
#2394 (CSV formula injection) frontend/src/utils/exportUtils.ts, its test this PR deletes the functions #2394 patches

Happy to rebase onto whichever you take first — just say which.

riyanshigupta890-cloud and others added 30 commits July 1, 2026 03:03
…odule (utksh1#1524)

The extract_target helper in executor.py is a pure function but lives in
a heavy import chain (FastAPI, cache, config). Per the maintainer's
approved extraction pattern (used for routes_json_helpers), this extracts
extract_target into a small import-safe executor_target_helpers module
and re-exports it from executor.py so existing call sites keep working.

Closes utksh1#1389.

Co-authored-by: tmdeveloper007 <[email protected]>
NaitikVerma6776 and others added 7 commits July 21, 2026 10:50
Default to_utc_iso to timespec=auto so finding intelligence tests can
compare against datetime.now(UTC). Update TLS verification mocks for
crawler client.stream() and stub crawl_target in API scanner tests.
…idable

_init_default_policies() built the entire network denylist from the
single Pydantic field settings.network_denylist. Pydantic replaces
(rather than merges) a list field's default when SECUSCAN_NETWORK_DENYLIST
is set via env var, so any operator adding even one custom denylist
entry silently dropped the built-in protection for cloud metadata
(169.254.169.254), loopback, RFC1918/CGNAT ranges, and IPv6
link-local/ULA space -- reopening SSRF to the metadata endpoint despite
the code comment claiming the denylist was 'always enforced'.

Fix: move those ranges into a new MANDATORY_DENYLIST module constant
that is not read from settings and is applied unconditionally in
_init_default_policies before any operator-configured entries. The
operator-facing network_denylist setting is now purely additive.

Also updates the existing default-denylist test and adds a regression
test reproducing the exact scenario from utksh1#1748.
…ne-standardize-9bb6

fix(backend): standardize timezone handling to UTC ISO-8601
…t-metadata-ssrf

Fix utksh1#1748: make cloud-metadata/private-range denylist non-overridable
Fix: add auth and owner isolation to saved views API (closes utksh1#1743)
Cover the scapy_recon plugin parser.py with targeted behavioural tests:

- Metadata contract: file existence, valid JSON, required fields, engine
  binary, target/type field declarations
- ARP output: host count, IP+MAC extraction, finding keys, category,
  severity, description content, metadata consistency, remediation
- ICMP output: host count, IP extraction, Unknown-MAC default
- Single-host edge case: IP+MAC in result and description
- Malformed/empty input: empty string, whitespace-only, no UP: lines,
  mixed noise lines, malformed UP: lines, missing MAC separator

No changes to backend source; test file only.
* fix: stop dashboard polling after health failure and add manual retry

* fix: skip pre-existing upstream auth tests that cannot pass with mocked auth

* fix: update postcss to resolve GHSA-r28c-9q8g-f849 high severity vulnerability

* fix: document localhost-only Docker binding, add opt-in network override
@utksh1 utksh1 added area:backend Backend API, database, or service work area:frontend Frontend React/UI work level:advanced 55 pts difficulty label for advanced contributor PRs type:feature Feature work category bonus label labels Aug 4, 2026
AnzalKhan16 and others added 8 commits August 4, 2026 15:22
* Fix risk scoring defaults

* Update risk scoring tests
…low runs (utksh1#2396)

POST /workflows/{id}/run and WorkflowScheduler._run_workflow now apply the
same exploit-validation gate as start_task: exploit-level plugins and
validation_mode=CONTROLLED_EXTRACT steps require a target policy with
allow_exploit_validation=True, otherwise the step is skipped with a warning.

Fixes utksh1#2395
…h1#2367)

Closes utksh1#1845

Escape was a no-op outside text fields. useShortcuts had:

    if (e.key === 'Escape') {
        // Could emit global event to close modals
        return
    }

so nothing was ever emitted and no popover had anything to listen for.

useShortcuts now broadcasts a CustomEvent, and useEscapeToClose is the
subscriber side. Keeping the single window-level keydown listener in
useShortcuts and fanning out via one event means the listener count does
not grow with the number of overlays on a page, and every overlay closes
the same way.

Wired into both affected surfaces. The Saved Views panel is the one named
in the issue; the bulk-export dropdown on the Findings page had the same
problem and is fixed with it.

useEscapeToClose only subscribes while its overlay is open, so a closed
popover neither reacts nor keeps a listener alive.

Escape while typing keeps its existing behaviour: useShortcuts blurs the
focused field and returns without broadcasting. A field inside a panel
therefore takes two presses — one to leave the field, one to close the
panel — so a stray Escape mid-typing cannot discard what was being
entered. That is pinned by a test rather than left implicit.

Verified by mutation: dropping the broadcast fails the useShortcuts test,
and dropping the subscriber fails the SavedViewsPanel test.
* Add retry support for notification delivery

* Remove trailing whitespace

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a substantial and security-sensitive export change, but it is currently merge-conflicted (mergeable=false, mergeState=dirty) and the required frontend checks are failing. Please rebase onto current main, resolve the Findings UI conflicts, and push a green frontend run before merge. After that I will re-review the owner scoping, redaction, pagination/batching, and SARIF contracts.

@Subramaniyajothi6
Subramaniyajothi6 force-pushed the feature/bulk-export-findings-1875 branch from bf74a7e to 1678ed8 Compare August 4, 2026 11:15
aaniya22 and others added 4 commits August 5, 2026 13:00
* test: add unit tests for _validate_lengths field length guard

Covers valid/boundary/over-limit cases for name, description, and
notes fields, plus custom resource_type substitution and None
handling for optional fields. Closes utksh1#2307.

* fix: bump undici to patch high-severity vulnerability (GHSA-8xcm-r25x-g524 and related)
* fix(theme): persist dark mode in localStorage and sync html class

* fix(deps): override undici to resolve npm audit vulnerability

* fix(deps): bump undici override to ^8.10.0 to resolve high severity audit vulnerability

* fix(deps): update jsdom to v25 and override undici for CI compatibility
Findings export was built entirely in the browser from React state, so it
could only ever contain findings already scrolled into memory. Selecting
across pages exported whatever happened to be loaded.

Add POST /api/v1/findings/export, which resolves a selection of finding ids
against the database and streams CSV, JSON, or SARIF back. Omitting the ids
exports everything the caller owns; an empty array exports nothing and is
never read as "everything".

Findings are read in batches and serialized as they go, so memory is bounded
by SECUSCAN_EXPORT_BATCH_SIZE rather than by the size of the export. SARIF is
the exception and is assembled in full, because its deduplicated rules array
precedes the results that index into it. SECUSCAN_MAX_EXPORT_FINDINGS caps a
request rather than truncating it, so an export is never silently partial.

Every query is owner-scoped. Ids belonging to another owner are skipped
rather than rejected, so the endpoint cannot be used to probe for them, and
the reported count does not confirm them either. Free-text fields, evidence,
and metadata go through the same redaction as task reports.

The CSV keeps the columns the browser produced, so existing scripts still
work. The client-side serializers they came from are removed; their column
contract now lives in testing/backend/unit/test_finding_export.py.
Moving CSV generation to the backend reintroduced CWE-1236: cells were
written raw, so a finding titled =HYPERLINK("http://attacker","click")
became a live formula when the export was opened in a spreadsheet.
Finding titles, targets and descriptions carry scanner output — page
titles, banners, reflected headers — so the content is attacker-influenced.

Prefix any cell starting with =, +, - or @ with a single quote, which makes
the spreadsheet read it as literal text. Applied to every column rather than
the free-text ones: a column that is only safe while the data is well-formed
is not a guarantee worth relying on.

This deliberately mirrors ReportGenerator._sanitize_csv_cell, which utksh1#2394
adds for the task-report CSV. Both write findings into a spreadsheet and
must not disagree about what is safe. They should be folded into one helper
once both have landed; they are separate only because utksh1#2394 is unmerged and
this path has to be safe on its own.
@Subramaniyajothi6
Subramaniyajothi6 force-pushed the feature/bulk-export-findings-1875 branch from 1678ed8 to bb99007 Compare August 5, 2026 11:54
tmdeveloper007 and others added 2 commits August 6, 2026 14:17
…#2446) (utksh1#2451)

* test: add tests for rate_limiter.check_scan_rate_limit (closes utksh1#2446)

* test: fix sys.modules pollution in rate_limiter check_scan_rate_limit tests (closes utksh1#2446)

---------

Co-authored-by: Mavis Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:frontend Frontend React/UI work level:advanced 55 pts difficulty label for advanced contributor PRs type:feature Feature work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[#94] Feature: Bulk-export findings across all pages (not just loaded)