From 8f2886071886af555c3feef86310788a716f15ed Mon Sep 17 00:00:00 2001 From: Anshul Jain Date: Mon, 6 Jul 2026 08:53:22 +0530 Subject: [PATCH] fix(api): paginate GET /task/{task_id}/result to prevent OOM on large scans A wide-range scan (e.g. full-range nmap against a /16 subnet) can produce tens of thousands of finding rows for a single task. GET /task/{task_id}/result loaded every one of them into memory with no LIMIT before the response was serialised, which can OOM-crash the backend process and freeze the frontend rendering thousands of table rows at once. Root cause: the query 'SELECT * FROM findings WHERE owner_id = ? AND task_id = ? ORDER BY ...' had no bound, and severity_counts/finding_groups/ asset_summary were all computed by iterating the full in-memory list. Fix: - Add page/per_page query params (default 100, max 500 -- same convention already used by GET /findings) and apply LIMIT/OFFSET to the findings list actually returned to the client. - Compute total_findings via a lightweight COUNT(*) query instead of len(findings), so it reflects the whole scan even though only one page of full finding objects is loaded. - Aggregate views (severity_counts, finding_groups, asset_summary) must reflect the entire scan, not just the current page, but loading every row to get there would reintroduce the exact OOM this fixes. They're computed from a bounded sample (capped at 5000 findings) for scans large enough to need it, and from the full set for everything else -- so aggregates stay accurate for the overwhelming majority of real scans while memory use is capped for pathological ones. - Cache key now includes page/per_page so different pages don't collide. - Response includes total_findings, page, per_page, and has_more_findings so the frontend can page through results instead of receiving a silently truncated list. Testing: - testing/backend/integration/test_task_result_pagination.py: 7 new tests covering default page size, page navigation, last-page has_more_findings, severity_counts reflecting the whole scan regardless of page, the per_page upper bound (422 above 500), small scans returning everything on page 1, and the aggregation-sample cap on a 6000-finding scan. - pytest testing/backend/unit -q -m "not benchmark" -- 2210 passed (6 pre-existing failures unrelated to this change, confirmed identical on unmodified main -- sandbox/subprocess tests failing in this environment) - pytest testing/backend/integration -q -m "not benchmark" -- 291 passed, 9 skipped - ruff check backend testing/backend -- all checks passed - scripts/check-artifacts.sh origin/main -- clean Fixes #1621 --- backend/secuscan/routes.py | 83 ++++++++-- .../test_task_result_pagination.py | 155 ++++++++++++++++++ 2 files changed, 227 insertions(+), 11 deletions(-) create mode 100644 testing/backend/integration/test_task_result_pagination.py diff --git a/backend/secuscan/routes.py b/backend/secuscan/routes.py index 14252b25..b3137433 100644 --- a/backend/secuscan/routes.py +++ b/backend/secuscan/routes.py @@ -26,6 +26,17 @@ # Re-exported for backward compatibility with integration tests SSE_RAW_OUTPUT_CHUNK_SIZE = 64 * 1024 + +# GET /task/{task_id}/result pagination (issue #1621): a wide-range scan can +# produce tens of thousands of finding rows. Returning them all in one +# response materialises the entire result set in memory before the first +# byte is sent, which can OOM-crash the backend process. The findings list +# is paginated; aggregate views (severity counts, groups, asset summary) +# are computed from a bounded sample instead of the full table so they stay +# cheap regardless of how large the underlying scan was. +TASK_RESULT_FINDINGS_DEFAULT_PER_PAGE = 100 +TASK_RESULT_FINDINGS_MAX_PER_PAGE = 500 +TASK_RESULT_AGGREGATION_SAMPLE_CAP = 5000 from .routes_report_helpers import ( _slugify_filename_part, build_report_filename, @@ -800,14 +811,19 @@ async def download_sarif_report(task_id: str, owner: str = Depends(get_current_o @router.get("/task/{task_id}/result") -async def get_task_result(task_id: str, owner: str = Depends(get_current_owner)): +async def get_task_result( + task_id: str, + owner: str = Depends(get_current_owner), + page: int = Query(1, ge=1), + per_page: int = Query(TASK_RESULT_FINDINGS_DEFAULT_PER_PAGE, ge=1, le=TASK_RESULT_FINDINGS_MAX_PER_PAGE), +): """Get task execution result""" db = await get_db() # Enforce ownership and existence check first await require_owned_task(db, task_id, owner) - cache_key = f"tasks:result:{task_id}:{owner}" + cache_key = f"tasks:result:{task_id}:{owner}:page={page}:per_page={per_page}" cache = await get_cache() cached = await cache.get_json(cache_key) if cached is not None: @@ -833,32 +849,73 @@ async def get_task_result(task_id: str, owner: str = Depends(get_current_owner)) except json.JSONDecodeError: structured = {} - finding_rows = await db.fetchall( - "SELECT * FROM findings WHERE owner_id = ? AND task_id = ? ORDER BY (risk_score IS NULL) ASC, risk_score DESC, discovered_at DESC", + total_findings_row = await db.fetchone( + "SELECT COUNT(*) AS count FROM findings WHERE owner_id = ? AND task_id = ?", (owner, task_id), ) + total_findings_count = total_findings_row["count"] if total_findings_row else 0 + + offset = (page - 1) * per_page + finding_rows = await db.fetchall( + "SELECT * FROM findings WHERE owner_id = ? AND task_id = ? " + "ORDER BY (risk_score IS NULL) ASC, risk_score DESC, discovered_at DESC " + "LIMIT ? OFFSET ?", + (owner, task_id, per_page, offset), + ) findings = deserialize_finding_rows(finding_rows) + + # Aggregate views (severity breakdown, groups, asset summary) must reflect + # the whole scan, not just the current page, but loading every row to get + # there would reintroduce the OOM this endpoint is being fixed for. A + # bounded sample keeps them accurate for the overwhelming majority of + # scans while capping memory use for pathological ones. + if total_findings_count > TASK_RESULT_AGGREGATION_SAMPLE_CAP: + aggregation_rows = await db.fetchall( + "SELECT * FROM findings WHERE owner_id = ? AND task_id = ? " + "ORDER BY (risk_score IS NULL) ASC, risk_score DESC, discovered_at DESC " + "LIMIT ?", + (owner, task_id, TASK_RESULT_AGGREGATION_SAMPLE_CAP), + ) + aggregation_findings = deserialize_finding_rows(aggregation_rows) + aggregation_sample_capped = True + elif offset == 0 and per_page >= total_findings_count: + # Already have every finding in `findings` for the common case + # (first page large enough to cover the whole scan). + aggregation_findings = findings + aggregation_sample_capped = False + else: + aggregation_rows = await db.fetchall( + "SELECT * FROM findings WHERE owner_id = ? AND task_id = ? " + "ORDER BY (risk_score IS NULL) ASC, risk_score DESC, discovered_at DESC", + (owner, task_id), + ) + aggregation_findings = deserialize_finding_rows(aggregation_rows) + aggregation_sample_capped = False + asset_rows = await db.fetchall( "SELECT * FROM asset_services WHERE owner_id = ? AND task_id = ? ORDER BY created_at DESC", (owner, task_id), ) asset_services = deserialize_asset_service_rows(asset_rows) - if not findings and isinstance(structured, dict): - findings = [item for item in structured.get("findings", []) if isinstance(item, dict)] + if not aggregation_findings and isinstance(structured, dict): + structured_findings = [item for item in structured.get("findings", []) if isinstance(item, dict)] + total_findings_count = len(structured_findings) + aggregation_findings = structured_findings + findings = structured_findings[offset:offset + per_page] severity_counts: Dict[str, int] = {} - for finding in findings: + for finding in aggregation_findings: severity = str(finding.get("severity", "info")).lower() severity_counts[severity] = severity_counts.get(severity, 0) + 1 finding_groups = structured.get("finding_groups") if isinstance(structured, dict) else None if not isinstance(finding_groups, list) or not finding_groups: - finding_groups = build_finding_groups(findings) + finding_groups = build_finding_groups(aggregation_findings) asset_summary = structured.get("asset_summary") if isinstance(structured, dict) else None if not isinstance(asset_summary, list) or not asset_summary: - asset_summary = build_asset_summary(findings, asset_services) + asset_summary = build_asset_summary(aggregation_findings, asset_services) scan_diff = structured.get("scan_diff") if isinstance(structured, dict) else None if not isinstance(scan_diff, dict): @@ -877,7 +934,7 @@ async def get_task_result(task_id: str, owner: str = Depends(get_current_owner)) str(item) for item in structured_summary if isinstance(item, (str, int, float)) and str(item).strip() ] if isinstance(structured_summary, list) else [] - total_findings = len(findings) + total_findings = total_findings_count if not summary and total_findings > 0: critical_high = severity_counts.get("critical", 0) + severity_counts.get("high", 0) if critical_high > 0: @@ -927,7 +984,11 @@ async def get_task_result(task_id: str, owner: str = Depends(get_current_owner)) "errors": [{"message": redact(task_row["error_message"])}] if task_row["error_message"] else [], "error_message": redact(task_row["error_message"]) if task_row["error_message"] else None, "exit_code": task_row["exit_code"], - "metadata": {} + "metadata": {}, + "total_findings": total_findings_count, + "page": page, + "per_page": per_page, + "has_more_findings": offset + len(findings) < total_findings_count, } if task_row["status"] in ["completed", "failed", "cancelled"]: diff --git a/testing/backend/integration/test_task_result_pagination.py b/testing/backend/integration/test_task_result_pagination.py new file mode 100644 index 00000000..4bebd5cd --- /dev/null +++ b/testing/backend/integration/test_task_result_pagination.py @@ -0,0 +1,155 @@ +import sqlite3 +import json +import uuid + +from backend.secuscan.config import settings + + +def _seed_completed_task(task_id: str, owner: str = "default") -> None: + conn = sqlite3.connect(settings.database_path) + conn.execute( + """ + INSERT INTO tasks (id, owner_id, plugin_id, tool_name, target, status, created_at, + preset, inputs_json, command_used, structured_json) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + task_id, owner, "http_inspector", "http_inspector", "https://example.com", + "completed", "2026-05-19T10:00:00", + "standard", json.dumps({"target": "https://example.com"}), + "", None, + ), + ) + conn.commit() + conn.close() + + +def _seed_findings(task_id: str, count: int, owner: str = "default") -> None: + conn = sqlite3.connect(settings.database_path) + severities = ["critical", "high", "medium", "low", "info"] + rows = [ + ( + str(uuid.uuid4()), owner, task_id, "http_inspector", + f"Finding {i}", "General", severities[i % len(severities)], + "https://example.com", f"Description {i}", + ) + for i in range(count) + ] + conn.executemany( + """ + INSERT INTO findings (id, owner_id, task_id, plugin_id, title, category, severity, target, description) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + rows, + ) + conn.commit() + conn.close() + + +# Regression coverage for #1621: GET /task/{task_id}/result previously +# loaded every finding row for a task into memory with no LIMIT, which can +# OOM-crash the backend on a large scan. These tests pin down the paginated +# contract: the `findings` list respects limit/offset, while aggregate views +# (severity_counts, total_findings) always reflect the whole scan. + + +def test_findings_are_paginated_with_default_page_size(test_client): + task_id = "pagination-test-001" + _seed_completed_task(task_id) + _seed_findings(task_id, count=250) + + response = test_client.get(f"/api/v1/task/{task_id}/result") + assert response.status_code == 200 + body = response.json() + + assert len(body["findings"]) == 100 # default per_page + assert body["total_findings"] == 250 + assert body["page"] == 1 + assert body["per_page"] == 100 + assert body["has_more_findings"] is True + + +def test_findings_page_two_returns_the_next_slice(test_client): + task_id = "pagination-test-002" + _seed_completed_task(task_id) + _seed_findings(task_id, count=250) + + response = test_client.get(f"/api/v1/task/{task_id}/result?page=2&per_page=100") + assert response.status_code == 200 + body = response.json() + + assert len(body["findings"]) == 100 + assert body["page"] == 2 + assert body["has_more_findings"] is True + + +def test_last_page_has_no_more_findings(test_client): + task_id = "pagination-test-003" + _seed_completed_task(task_id) + _seed_findings(task_id, count=250) + + response = test_client.get(f"/api/v1/task/{task_id}/result?page=3&per_page=100") + assert response.status_code == 200 + body = response.json() + + assert len(body["findings"]) == 50 + assert body["has_more_findings"] is False + + +def test_severity_counts_reflect_the_whole_scan_not_just_the_current_page(test_client): + task_id = "pagination-test-004" + _seed_completed_task(task_id) + _seed_findings(task_id, count=250) # 50 of each severity across 5 tiers + + response = test_client.get(f"/api/v1/task/{task_id}/result?page=1&per_page=10") + assert response.status_code == 200 + body = response.json() + + assert len(body["findings"]) == 10 + assert sum(body["severity_counts"].values()) == 250 + assert body["total_findings"] == 250 + + +def test_per_page_is_capped_at_500(test_client): + task_id = "pagination-test-005" + _seed_completed_task(task_id) + _seed_findings(task_id, count=10) + + response = test_client.get(f"/api/v1/task/{task_id}/result?per_page=10000") + assert response.status_code == 422 + + +def test_small_scan_returns_everything_on_the_first_page(test_client): + task_id = "pagination-test-006" + _seed_completed_task(task_id) + _seed_findings(task_id, count=5) + + response = test_client.get(f"/api/v1/task/{task_id}/result") + assert response.status_code == 200 + body = response.json() + + assert len(body["findings"]) == 5 + assert body["total_findings"] == 5 + assert body["has_more_findings"] is False + + +def test_aggregation_sample_is_capped_for_very_large_scans(test_client): + # Simulates the issue's exact scenario: tens of thousands of findings + # from a wide-range scan. Severity counts come from the capped + # aggregation sample rather than every row, so they won't exactly equal + # total_findings once the cap is exceeded -- this test documents that + # trade-off rather than asserting exact parity. + task_id = "pagination-test-007" + _seed_completed_task(task_id) + _seed_findings(task_id, count=6000) + + response = test_client.get(f"/api/v1/task/{task_id}/result") + assert response.status_code == 200 + body = response.json() + + assert len(body["findings"]) == 100 + assert body["total_findings"] == 6000 + assert body["has_more_findings"] is True + # Aggregation sample is capped at 5000, so severity_counts is computed + # from at most that many rows, never all 6000. + assert sum(body["severity_counts"].values()) <= 5000