Repository navigation
FEAT: Export Operation findings to Word and Markdown - #3493
Adrian Gavrila (adrian-gavrila) wants to merge 3 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| async def _load_selected_findings_async( | ||
| *, service: OperationService, operation_id: UUID, finding_ids: list[UUID] | ||
| ) -> list[Finding]: | ||
| """ | ||
| Read the selected findings in the order ``OperationService.list_findings_async`` lists them for the dashboard. | ||
|
|
||
| Returns: | ||
| list[Finding]: Every selected finding. | ||
|
|
||
| Raises: | ||
| OperationNotFoundError: If the Operation does not exist. | ||
| FindingNotFoundError: If a selected finding is not in the Operation. | ||
| """ | ||
| wanted = set(finding_ids) | ||
| selected: dict[UUID, Finding] = {} | ||
| offset: int | None = 0 | ||
| while offset is not None and len(selected) < len(wanted): | ||
| page = await service.list_findings_async(operation_id=operation_id, limit=_PAGE_SIZE, offset=offset) | ||
| for finding in page.items: | ||
| # A concurrent change can shift a row onto the next page, so keep its first read. | ||
| if finding.id in wanted: | ||
| selected.setdefault(finding.id, finding) | ||
| offset = page.next_offset | ||
| missing = next((finding_id for finding_id in finding_ids if finding_id not in selected), None) | ||
| if missing is not None: | ||
| raise FindingNotFoundError(f"Finding '{missing}' not found in operation '{operation_id}'.") | ||
| return list(selected.values()) | ||
|
|
||
|
|
||
| async def _load_evidence_async(*, service: OperationService, finding: Finding) -> list[FindingEvidenceItem]: | ||
| items: dict[UUID, FindingEvidenceItem] = {} | ||
| offset: int | None = 0 | ||
| while offset is not None: | ||
| page = await service.list_finding_evidence_async( | ||
| operation_id=finding.operation_id, finding_id=finding.id, limit=_PAGE_SIZE, offset=offset | ||
| ) | ||
| for item in page.items: | ||
| items.setdefault(item.item.id, item) | ||
| offset = page.next_offset | ||
| return list(items.values()) |
There was a problem hiding this comment.
[P2] Load the selected findings and their evidence in batches
The PR description lists finding-by-finding reads as a known limit. These loaders cause two problems:
- Speed: The loaders reuse the paged API methods. Each
list_finding_evidence_asynccall checks the operation and the finding again. Then it reads the evidence, attacks, operation names, score pieces, and conversation statistics. A finding with evidence needs about seven sequential database calls. The dialog selects all findings by default, so an operation with 300 findings can need about 2,000 calls. The findings scan also calculates evidence counts and then discards them. - Skipped rows: Offset pages move when rows change.
setdefaultremoves duplicate rows, but it cannot find skipped rows. For example, a finding has 101 evidence links. If someone removes one of the first 100 links between the two page reads, the 101st link moves to offset 99. The second page does not read it, and the export succeeds without it. In an operation with more than 100 findings, a delete or a severity change between page reads can cause a false "not found" error.
Please read the selected findings by ID with one query, and use the ORDER BY from get_findings_async to keep the dashboard order. Then read their evidence and attack context in batches. This fixes both problems, and the export stops scanning findings that the user did not select.
Review authored by GitHub Copilot.
| def _markdown_field(field: _ReportField) -> str: | ||
| if _is_short(field): | ||
| return f"**{_markdown_inline(field.label)}:** {_markdown_inline(field.value)}" | ||
| return f"**{_markdown_inline(field.label)}**\n\n{_markdown_literal(field.value)}" |
There was a problem hiding this comment.
[P2] Keep multi-line text as prose in the Markdown export
Each value with a line break goes into a fenced code block. Descriptions come from a multi-line text box, and model-generated score rationales often contain line breaks. As a result, many descriptions and rationales show as monospace code. The dashboard and the Word export show this text as prose. In Markdown, the same field is prose when it has one line and code when it has two lines.
The PR description says that the fences stop pasted text from forming headings, links, images, or HTML. Escaping can give the same protection and keep the text as prose:
- Escape each line with the same rules as
_markdown_inline. - Also escape block syntax at the start of each line: list markers, a number followed by a period,
=underlines, and leading spaces. - Join the lines with hard line breaks.
Users copy this export into reports, so prose is easier to use.
Review authored by GitHub Copilot.
| return quote(value, safe="!*'()") | ||
|
|
||
|
|
||
| def _render_docx(report: _FindingReport, *, now: datetime) -> bytes: |
There was a problem hiding this comment.
[P2] Revisit keeping the renderers in the backend
The PR description says that the renderers stay here because output printers return str and a .docx file is a ZIP file. That is true for DOCX. But Markdown fits the current printer contract.
doc/code/framework.md gives rendering to the Output module and tells the backend to reuse framework components. With both renderers in a backend service, notebook and SDK users have no public way to make the same report. The move is small now, because the renderers depend only on _FindingReport.
Please keep data loading and HTTP handling in the backend. Move the Markdown renderer, and the small report model that it takes, to pyrit.output. DOCX needs a decision about binary output first, for example a renderer that returns bytes beside the printers.
Minor: _render_docx(report, *, now) has two parameters, so the style guide requires report to be keyword-only too.
Review authored by GitHub Copilot.
| _use_current_word_spacing(document) | ||
| document.add_heading(title, level=1) | ||
| document.add_paragraph(_EVIDENCE_NOTE) | ||
| table = document.add_table(rows=1, cols=len(_WORD_COLUMNS)) |
There was a problem hiding this comment.
[P3] Set the table column widths
python-docx divides the table width equally between the columns. The Finding column holds the title, description, and all evidence. The Assessment column holds at most two short fields. As a result, the long text wraps in half the page width, and most of the Assessment column stays empty. Please set explicit widths on the grid columns and the cells, for example 75% and 25%.
Review authored by GitHub Copilot.
| def _replace_theme_formatting(document: DocxDocument) -> None: | ||
| """Name Latin fonts and text colors explicitly rather than through the template's 2007-era theme.""" | ||
| styles = document.styles.element | ||
| for fonts in styles.iter(qn("w:rFonts")): | ||
| theme = fonts.get(qn("w:asciiTheme")) | ||
| if theme is None: | ||
| continue | ||
| family = _WORD_HEADING_FONT if theme.startswith("major") else _WORD_BODY_FONT | ||
| for attribute in ("w:asciiTheme", "w:hAnsiTheme"): | ||
| fonts.attrib.pop(qn(attribute), None) | ||
| fonts.set(qn("w:ascii"), family) | ||
| fonts.set(qn("w:hAnsi"), family) | ||
| for color in styles.iter(qn("w:color")): | ||
| for attribute in _THEME_COLOR_ATTRIBUTES: | ||
| color.attrib.pop(qn(attribute), None) | ||
|
|
||
|
|
||
| def _use_current_word_spacing(document: DocxDocument) -> None: | ||
| """Give body text and the title the spacing and look of a new Word document.""" | ||
| normal = document.styles["Normal"].paragraph_format | ||
| normal.space_after = Pt(8) | ||
| normal.line_spacing = 1.08 | ||
| title = document.styles["Heading 1"] | ||
| title.font.color.rgb = _WORD_TITLE_COLOR | ||
| title.font.size = _WORD_TITLE_SIZE | ||
| title.font.bold = False | ||
| title.paragraph_format.space_before = Pt(0) | ||
| title.paragraph_format.space_after = Pt(4) |
There was a problem hiding this comment.
[P3] Consider removing the theme and spacing rewrites
The docs say that users copy rows from this file into their report. By default, Word applies the destination document's styles when style names conflict, for example Normal and List Bullet. As a result, these rewrites mostly change the downloaded file, not the pasted rows. They also depend on the XML of the python-docx default template, and test_docx_opens_as_a_current_word_document locks in their fonts, colors, and sizes.
Please keep the compatibility-mode and metadata fixes, because they are small and useful. Remove these two rewrites, or use a small template .docx file if the downloaded file must look like a new Word document.
Review authored by GitHub Copilot.
| def _conversation_link(*, dashboard_base_url: AnyHttpUrl, item: FindingEvidenceItem, conversation_id: str) -> str: | ||
| """ | ||
| Build the dashboard route that opens evidence's conversation and returns to the evidence. | ||
|
|
||
| Returns: | ||
| str: The absolute link, matching the dashboard's ``attackConversationRoutePath``. | ||
| """ | ||
| path = ( | ||
| f"/attacks/{_encode_segment(str(item.item.attack_result_id))}/conversations/{_encode_segment(conversation_id)}" | ||
| ) | ||
| query = {"scenarioResultId": str(item.scenario_result_id)} if item.scenario_result_id else {} | ||
| query["findingEvidenceId"] = str(item.item.id) | ||
| return f"{str(dashboard_base_url).rstrip('/')}{path}?{urlencode(query)}" | ||
|
|
||
|
|
||
| def _encode_segment(value: str) -> str: | ||
| # The same characters JavaScript's encodeURIComponent leaves unescaped. | ||
| return quote(value, safe="!*'()") |
There was a problem hiding this comment.
[P3] Simplify how the evidence links are built
The path-prefix support has no effect today. Vite sets no base, the router has no basename, and the backend serves the app at /. So import.meta.env.BASE_URL is always /, and the prefix tests in test_finding_export.py and FindingExportDialog.test.tsx test a deployment that the app does not support. Please send only the origin and remove those tests, or add base-path support to the app first.
Also, _conversation_link and _encode_segment copy attackConversationRoutePath and encodeURIComponent. The PR description accepts this dependency. But no unit test compares the two builders, and the seeded E2E test opens only one generated link. If the frontend route changes, new exports can contain broken links. Please add a test that keeps the two builders in step, or link to a stable route that the frontend owns, for example one that opens evidence by its ID.
Review authored by GitHub Copilot.
| def _export_filename(*, operation_id: UUID, operation_name: str, extension: str, now: datetime) -> str: | ||
| """ | ||
| Name a download after its Operation, with the time in labelled UTC. | ||
|
|
||
| Browsers save under the dashboard's own name, which uses the user's local time instead. | ||
|
|
||
| Returns: | ||
| str: For example ``copyrit-findings-synthetic-evaluation-2026-10-22-1658-utc.docx``. | ||
| """ | ||
| name = _FILENAME_SEPARATORS.sub("-", unicodedata.normalize("NFC", operation_name).lower()).strip("-") | ||
| if len(name) > _FILENAME_NAME_LIMIT: | ||
| end = _FILENAME_NAME_LIMIT | ||
| if name[end] != "-" and "-" in name[:end]: | ||
| end = name.rindex("-", 0, end) | ||
| # A word longer than the limit is cut between letters, never between a letter and its marks. | ||
| while end > 0 and _COMBINING_MARK.match(name[end]): | ||
| end -= 1 | ||
| name = name[:end].rstrip("-") | ||
| return f"copyrit-findings-{name or operation_id}-{now.astimezone(UTC):%Y-%m-%d-%H%M}-utc.{extension}" |
There was a problem hiding this comment.
[P3] Keep the filename rules in one place
The PR description says that the server name is for API clients. No client in this repository reads it: operationsApi.exportFindings returns only the Blob, and the dialog builds its own name with findingExportFilename. So the same Unicode slug and truncation rules exist in Python and in TypeScript, each with its own tests, and the two copies can drift.
Please keep one copy. The dialog can use the server name: a same-origin deployment can read Content-Disposition, and a cross-origin deployment can expose it. Or the server can send a simple name for API clients, for example with the operation ID and the UTC time.
Review authored by GitHub Copilot.
| if entry.summary and all(_is_short(field) for field in entry.summary): | ||
| items.append(_DETAIL_SEPARATOR.join(_markdown_field(field) for field in entry.summary)) | ||
| else: | ||
| items.extend(_markdown_field(field) for field in entry.summary) |
There was a problem hiding this comment.
[P3] Remove the unused summary branch and the Word layout assertions
As the PR description says, the generic report fields prepare for configurable finding fields. This branch is different. Outcome and Score are single-line values, so with real data only the first branch runs. Only test_markdown_fences_literal_body_and_escapes_inline_values reaches the else branch, with a synthetic multi-line Outcome. Please remove the branch until a real summary field needs it.
Also, test_docx_lists_every_finding_in_one_finding_and_assessment_table asserts paragraph styles and indents. These assertions fail after harmless layout changes. Please keep the Word tests on content, links, literal text, and document validity.
Review authored by GitHub Copilot.
| <li key={finding.id} className={styles.row}> | ||
| <Checkbox className={styles.checkbox} label={{ children: finding.title, className: styles.label }} | ||
| checked={selected.has(finding.id)} disabled={exporting} | ||
| onChange={(_, data) => { toggle(finding.id, data.checked === true) }} /> | ||
| <FindingSeverityBadge finding={finding} /> |
There was a problem hiding this comment.
[P3] Help users tell similar findings apart
Titles do not have to be unique, and each row shows only the title and severity. So two findings with the same title and severity look the same in this list. A user cannot be sure which one they remove from a report that can contain sensitive content. Please show more context in each row, for example the first line of the description or the creation date.
Review authored by GitHub Copilot.
| } catch (cause: unknown) { | ||
| if (!controller.signal.aborted) setExportError(toApiError(cause).detail) |
There was a problem hiding this comment.
[P3] Make the error for a deleted finding actionable
If someone deletes a selected finding while this dialog is open, the export fails with Finding '<id>' not found in operation '<id>'. The message shows only IDs, so the user cannot tell which row to clear. The dialog keeps its list, so each new attempt fails the same way until the user closes and opens the dialog again. Please show a clear message, and reload the list or remove the missing finding from the selection.
Review authored by GitHub Copilot.
Description
Follow-up to #3066 and #3275. An Operation can hold findings with linked attack evidence, but putting them into a report meant copying each one by hand. Export findings on the Operation page downloads the findings you select as one Word or Markdown document, ready to paste into a larger report.
What changed
copyrit-findings-synthetic-evaluation-2026-10-22-1258.docx.POST /api/operations/{id}/findings/exporttakesfinding_ids,format(docxby default, ormarkdown) anddashboard_base_url, and returns the document as an attachment. Empty or duplicate IDs, unknown fields, and a base URL that isn't plain HTTP(S) or has credentials, a query or a fragment return 422. A missing Operation, or a finding that is missing or belongs to another Operation, returns 404. If rendering fails, the response is a 500 with no partial file. The export only reads; there's no schema change or migration.�in both formats, and every line-break convention becomes a line break.ManualScorer), the rationale, and From operation when the attack belongs to a different Operation. A deleted attack keeps its entry under its attack ID and reads Evidence unavailable. Messages, attachments and media are never included. Links are built from the dashboard's own address, so they point at the deployment the export came from.OperationService.list_findings_async. The order of IDs in the request doesn't change it.downloadFilehelper. The Axios response interceptor reads error bodies from blob responses before its compatibility and 401 checks, so a failed download shows the server's message. The Operation header buttons (Edit, Delete, Export findings, New finding) have icons.Markdown export of the same seven synthetic findings
How it fits together
flowchart LR A["Operation page:<br/>Export findings"] --> B["Dialog pages every finding;<br/>user picks findings and format"] B -->|"POST finding_ids, format,<br/>dashboard_base_url"| C["Export route"] C --> D["finding_export: selected findings and<br/>evidence through OperationService"] D --> E["Small report model"] E --> F["Word renderer"] E --> G["Markdown renderer"] F --> H["Attachment"] G --> H H --> I["Browser saves the file"]Design choices and limits
pyrit/output. The output printers returnstr, and a.docxis a ZIP file. A GUI download didn't need that contract widened.attackConversationRoutePath. That function's JSDoc now says exported documents link there, and the seeded test opens one.Content-Dispositionuses the same rule in UTC with a-utcsuffix, for API clients and for cross-origin deployments that don't expose the header.\&. Evidence is read finding by finding; tests cover 101 findings and 101 evidence items on one finding.Follow-ups
_project_finding_fields. The renderers already take any list of labelled fields; that work also needs to decide how a field picks its Word column.Tests and Documentation
Backend tests cover the HTTP contract (attachment headers, Unicode filenames, 422, 404 and 500), the report built from SQLite (selection across the 100-row page, dashboard order, 101 evidence items, human and automated scores, undetermined and missing scores, deleted attacks, Other severity and harm type, cross-Operation evidence), and both renderers on whole documents (table layout, bullets, escaping and fencing, hostile text, links under a path prefix, control characters, Word document properties). Frontend tests cover the dialog (paging, selection, stale responses, focus, errors), blob error decoding, the download helper and the filename rule. A seeded Playwright test downloads both formats, checks the dialog at 390px (including that the warning fits its bar), and opens an exported link. Results on
a0beb5543, based on0f58aed71:pytest -n 4 --dist=loadfile --cov=pyrit --cov-fail-under=78 tests/unit: 31,218 passed, 57 skipped, 96.23% coverage. This ran on0c62fda60;a0beb5543changes only frontend files.diff-cover coverage.xml --compare-branch=0f58aed71 --fail-under=90: 99% of 329 changed lines. The one uncovered line is the_percent_encodefallback infinding_export.py, which no generated link reaches.pre-commit run --all-files: passed.npm run test:coverage: 3,654 passed.npm run lint,npx tsc --noEmit,npm run buildandnpx playwright test --list: passed.npx playwright test --project=seeded: 104 passed on0c62fda60. Ona0beb5543,operations.spec.tsandoperations-acceptance.spec.tspassed (15 tests).npx playwright test --project=mock: 180 passed, 22 failed, all in target registry, accessibility, routing and onboarding specs that this PR doesn't touch. Withmain's frontend,config.spec.tsfails 15 tests on this Windows machine, the same count as with this branch.OperationService.Docs:
doc/gui/0_gui.mdadds an "Exporting findings" section under Operations. No notebooks changed, so JupyText wasn't needed.