diff --git a/doc/code/setup/finding_fields.md b/doc/code/setup/finding_fields.md new file mode 100644 index 0000000000..e97f083b9d --- /dev/null +++ b/doc/code/setup/finding_fields.md @@ -0,0 +1,87 @@ +# Finding Fields + +A finding in CoPyRIT has a title, a severity and a set of fields. Each deployment chooses those fields. The finding dialog, the finding cards and the exports all follow them. + +## Default Fields + +Without any configuration, findings use these fields: + +- **Severity**: Critical, Important, Moderate, Low or Informational. People can also choose **Other** and type their own text. The dialog preselects Moderate. +- **Harm-type**: optional. Its list is searchable, and it also takes **Other** text. +- **Description**: optional multi-line text. + +## Defining Your Own Fields + +To use other fields, subclass `FindingsInitializer` and return a `FindingFields` from `get_finding_fields`. The `severity` slot is always a required choice field. The fields in `fields` follow it in the dialog. + +```python +from pyrit.models import FindingField, FindingFields, FindingFieldType, FindingSeverityColor +from pyrit.setup.initializers import FindingsInitializer + + +class ContosoFindingsInitializer(FindingsInitializer): + """Finding fields for the Contoso red team.""" + + def get_finding_fields(self) -> FindingFields: + return FindingFields( + severity=FindingField( + key="severity", + label="Severity", + type=FindingFieldType.CHOICE, + options=[("sev1", "Sev 1"), ("sev2", "Sev 2"), ("sev3", "Sev 3")], + default="sev2", + colors={"sev1": FindingSeverityColor.RED, "sev2": FindingSeverityColor.ORANGE}, + ), + fields=[ + FindingField( + key="area", + label="Area", + type=FindingFieldType.CHOICE, + required=True, + options={ + ("content", "Content safety"): [("hate", "Hate"), ("violence", "Violence")], + ("security", "Security"): [("prompt_injection", "Prompt injection")], + }, + levels=[("subarea", "Subarea")], + ), + FindingField(key="repro_steps", label="Repro steps", type=FindingFieldType.MULTILINE), + FindingField(key="contoso_ticket", label="Contoso ticket", type=FindingFieldType.TEXT, internal=True), + ], + ) +``` + +A few things to note in this example: + +- Options are `(key, label)` pairs. The finding stores the `key`, and people see the `label`. Field and level keys use lowercase letters, digits and underscores. Option keys can be any text that isn't blank and has no surrounding whitespace. +- `Area` is a cascade. Each of its options maps to the options of a second dropdown. `levels` names that dropdown, and its value is stored under `subarea`. +- `colors` sets the badge color of a severity option. +- `internal=True` keeps `Contoso ticket` out of the Word and Markdown exports. The dashboard still shows it. + +`FindingField` also takes `hint`, `section` and `allow_other`. Its docstring describes each setting. + +Save the script as `contoso_findings.py` in your custom initializers source. The file name is the initializer's name. Then turn it on in `.pyrit_conf`: + +```yaml +allow_custom_initializers: true +custom_initializers_source: /srv/contoso/pyrit_initializers +initializers: + - name: target + - name: technique + - name: contoso_findings +``` + +With `allow_custom_initializers: true`, the backend runs every script in that source. Only trusted administrators should be able to write to it. See [the configuration file guide](../../getting_started/pyrit_conf.md) for details. + +## Rules + +- Use one `FindingsInitializer` per deployment. If the configuration lists two, or the fields are invalid, setup fails when the backend starts. The backend still runs, but it blocks runtime operations, including findings, until a process restart. Configuration repair stays available. A live apply reports invalid configuration instead, and the running fields don't change. +- Don't make `get_finding_fields` depend on environment variables. A live apply checks the fields before it applies the new environment. +- Change a field's `label` to rename what people see. +- A changed `key` makes a new field. Values saved under the old key show under **Retired fields**, where they are read-only. Edits keep them. **Remove** deletes one when you save. +- Other text that matches an option's key or label is rejected, ignoring case and surrounding spaces. The message names the option to choose instead. +- On the Operations list, severities that match no option share one **Other** count. +- Exports put multi-line fields in the Finding column, and choice and text fields in the Assessment column. They leave out internal fields and retired values. The export dialog warns when selected findings have retired values. + +## Upgrading + +One database migration moves findings saved before finding fields existed. It stores Other severity text as the `severity` itself, and moves the harm type and description into field values. Finding IDs and evidence don't change. Downgrading that migration refuses while any finding exists. diff --git a/doc/gui/0_gui.md b/doc/gui/0_gui.md index 8d2887df26..3e6b084139 100644 --- a/doc/gui/0_gui.md +++ b/doc/gui/0_gui.md @@ -213,8 +213,10 @@ This is a cross-result report, not the progress dashboard for one scanner run. ## Operations Operations organize human findings for an engagement. Create an operation, -record findings with severity and description, and attach saved attacks as -evidence. A model score does not create a finding automatically. +record findings, and attach saved attacks as evidence. Findings use the +deployment's [finding fields](../code/setup/finding_fields.md), which by default +are severity, harm type, and description. A model score does not create a +finding automatically. Select an operation in the labels bar to associate new chats and scans with it. The operation page brings its findings, linked evidence, and execution history diff --git a/doc/myst.yml b/doc/myst.yml index 70466e7614..a572278438 100644 --- a/doc/myst.yml +++ b/doc/myst.yml @@ -192,6 +192,7 @@ project: - file: code/setup/1_configuration.ipynb - file: code/setup/2_resiliency.ipynb - file: code/setup/default_values.md + - file: code/setup/finding_fields.md - file: code/setup/pyrit_initializer.ipynb - file: code/scenarios/0_scenarios.ipynb children: diff --git a/frontend/e2e/operations.spec.ts b/frontend/e2e/operations.spec.ts index 1d365de0db..838970e294 100644 --- a/frontend/e2e/operations.spec.ts +++ b/frontend/e2e/operations.spec.ts @@ -3,7 +3,7 @@ import { readFileSync } from 'node:fs' import { test, expect } from '@playwright/test' import type { - CreateAttackResponse, Finding, FindingEvidenceAttachResponse, FindingExportFormat, Operation, + CreateAttackResponse, Finding, FindingEvidenceAttachResponse, FindingExportFormat, FindingFields, Operation, } from '../src/types' import { waitForAppReady } from './_runtime' @@ -21,7 +21,7 @@ for (const width of [1280, 390]) { const severity = `Custom severity ${'classification'.repeat(16)}` const harmType = `Custom harm ${'unbroken'.repeat(25)}` const findingResponse = await request.post(`/api/operations/${operation.id}/findings`, { - headers, data: { title, severity: 'other', severity_other: severity, harm_type: 'Other', harm_type_other: harmType }, + headers, data: { title, severity, field_values: { harm_type: harmType } }, }) expect(findingResponse.status()).toBe(201) @@ -65,10 +65,7 @@ for (const width of [1280, 390]) { )).toBeTruthy() const label = dialog.locator('label').filter({ hasText: title }) expect(await label.evaluate(element => element.scrollWidth <= element.clientWidth)).toBeTruthy() - const pickerBadgeBox = await pickerBadge.boundingBox() - const harmBox = await dialog.getByText(`Harm-type: ${harmType}`, { exact: true }).boundingBox() - if (!pickerBadgeBox || !harmBox) throw new Error('Expected visible picker classifications.') - expect(pickerBadgeBox.y + pickerBadgeBox.height).toBeLessThanOrEqual(harmBox.y) + await expect(dialog.getByText(`Harm-type: ${harmType}`, { exact: true })).toHaveCount(0) expect(await page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth)).toBeTruthy() await radio.focus() await page.keyboard.press('Space') @@ -116,7 +113,8 @@ test('creates a unique operation and records findings within it @seeded', async dialog = page.getByRole('dialog') await expect(dialog.getByRole('combobox', { name: 'Operation' })).toHaveCount(0) await dialog.getByRole('textbox', { name: 'Title' }).fill(title) - await dialog.getByRole('combobox', { name: 'Severity' }).selectOption('informational') + await dialog.getByRole('combobox', { name: 'Severity' }).click() + await page.getByRole('option', { name: 'Informational', exact: true }).click() await dialog.getByRole('textbox', { name: 'Description' }).fill(description) const savedResponse = page.waitForResponse(response => response.url().endsWith(`/api/operations/${operation.id}/findings`) && response.request().method() === 'POST') @@ -145,7 +143,7 @@ test('creates a unique operation and records findings within it @seeded', async const headers = { 'PyRIT-Compatibility-ID': version.compatibility_id } for (let index = 0; index < 21; index++) { const response = await request.post(`/api/operations/${operation.id}/findings`, { - headers, data: { title: `Assessment ${index}`, description: '', severity: index === 20 ? 'critical' : 'low' }, + headers, data: { title: `Assessment ${index}`, severity: index === 20 ? 'critical' : 'low' }, }) expect(response.status()).toBe(201) @@ -189,7 +187,7 @@ test('renames an operation and deletes it only once it is empty @seeded', async headers, data: { name: `Typo operation ${stamp}` }, })).json() const finding: Finding = await (await request.post(`/api/operations/${operation.id}/findings`, { - headers, data: { title: 'Blocks deletion', description: '', severity: 'low' }, + headers, data: { title: 'Blocks deletion', severity: 'low' }, })).json() await page.goto(`/operations/${operation.id}`) @@ -305,7 +303,7 @@ test('selects saved operations inline and revisits ID-scoped attack history @see expect(other.ok()).toBeTruthy() } const finding = await request.post(`/api/operations/${operation.id}/findings`, { - headers, data: { title: 'Independent human finding', description: '', severity: 'low' }, + headers, data: { title: 'Independent human finding', severity: 'low' }, }) expect(finding.status()).toBe(201) if (!sourceAttack) throw new Error('Expected a saved attack for the evidence test.') @@ -372,7 +370,7 @@ test('exports selected findings as Word and Markdown @seeded', async ({ page, re // Saved out of severity order, so the document's order can only come from severity. const seeds = [ { title: 'Export low', severity: 'low' }, - { title: 'Export critical', severity: 'critical', description }, + { title: 'Export critical', severity: 'critical', field_values: { description } }, { title: longTitle, severity: 'informational' }, ...Array.from({ length: 17 }, (_, index) => ({ title: `Export moderate ${String(index).padStart(2, '0')}`, severity: 'moderate', @@ -511,3 +509,63 @@ test('exports selected findings as Word and Markdown @seeded', async ({ page, re await page.goto(link) await expect(page.getByText('Saved offline evidence.', { exact: true })).toBeVisible() }) + +test('keeps the page usable after removing a retired value and cancelling the edit @seeded', async ({ + page, request, +}) => { + const version = await (await request.get('/api/version')).json() + const headers = { 'PyRIT-Compatibility-ID': version.compatibility_id } + const fieldsResponse = await request.get('/api/operations/finding-fields', { headers }) + expect(fieldsResponse.ok()).toBeTruthy() + const fields: FindingFields = await fieldsResponse.json() + const operationResponse = await request.post('/api/operations', { + headers, data: { name: `Retired values ${Date.now()}` }, + }) + expect(operationResponse.status()).toBe(201) + const operation: Operation = await operationResponse.json() + const title = 'Contoso retired notes' + const notes = 'Recorded before Contoso changed its finding fields.' + const findingResponse = await request.post(`/api/operations/${operation.id}/findings`, { + headers, data: { title, severity: 'low', field_values: { description: notes } }, + }) + expect(findingResponse.status()).toBe(201) + // The seeded backend accepts only known keys, so the browser gets the fields without Description instead, + // which makes the saved description a retired value. + await page.route('**/api/operations/finding-fields', async route => { + await route.fulfill({ json: { ...fields, fields: fields.fields.filter(field => field.key !== 'description') } }) + }) + + await page.goto(`/operations/${operation.id}`) + await waitForAppReady(page) + const edit = page.getByRole('button', { name: `Edit finding: ${title}`, exact: true }) + const dialog = page.getByRole('dialog', { name: 'Edit finding' }) + const retired = dialog.getByRole('group', { name: 'Retired fields · read-only' }) + const removeButton = retired.getByRole('button', { name: 'Remove retired value description', exact: true }) + await edit.click() + await expect(retired.getByText(notes, { exact: true })).toBeVisible() + const confirmation = page.getByRole('dialog', { name: 'Remove description?' }) + await removeButton.click() + await confirmation.getByRole('button', { name: 'Keep it', exact: true }).click() + await expect(confirmation).toHaveCount(0) + await expect(removeButton).toBeFocused() + await removeButton.click() + await expect(confirmation).toBeVisible() + await page.keyboard.press('Escape') + await expect(confirmation).toHaveCount(0) + await expect(dialog).toBeVisible() + await expect(removeButton).toBeFocused() + + await removeButton.click() + await confirmation.getByRole('button', { name: 'Remove', exact: true }).click() + await expect(confirmation).toHaveCount(0) + await expect(dialog.getByRole('button', { name: 'Undo', exact: true })).toBeFocused() + await expect(dialog.getByText('1 retired value is removed when you save.', { exact: true })).toBeVisible() + await expect(retired).toHaveCount(0) + await dialog.getByRole('button', { name: 'Cancel', exact: true }).click() + await expect(page.getByRole('dialog')).toHaveCount(0) + + await edit.click() + await expect(dialog).toBeVisible() + await expect(retired.getByText(notes, { exact: true })).toBeVisible() + await expect(removeButton).toBeVisible() +}) diff --git a/frontend/src/components/Chat/FindingEvidenceDialog.test.tsx b/frontend/src/components/Chat/FindingEvidenceDialog.test.tsx index b30efff3dd..0027b8f44c 100644 --- a/frontend/src/components/Chat/FindingEvidenceDialog.test.tsx +++ b/frontend/src/components/Chat/FindingEvidenceDialog.test.tsx @@ -5,6 +5,9 @@ import { FluentProvider, webLightTheme } from '@fluentui/react-components' import { MemoryRouter } from 'react-router' import { attacksApi, operationsApi } from '@/services/api' +import { useRuntime } from '@/hooks/useRuntime' +import { pick } from '@/test-utils/findingDialog' +import { CONTOSO_FINDING_FIELDS, DEFAULT_FINDING_FIELDS_FIXTURE } from '@/test-utils/findingFieldsFixtures' import type { FindingListItem } from '@/types' import FindingEvidenceDialog from './FindingEvidenceDialog' @@ -12,12 +15,13 @@ jest.mock('@/services/api', () => ({ attacksApi: { getAttack: jest.fn() }, operationsApi: { get: jest.fn(), list: jest.fn(), listFindings: jest.fn(), attachFindingEvidence: jest.fn(), createFinding: jest.fn(), - getFindingOptions: jest.fn(), + getFindingFields: jest.fn(), }, })) +jest.mock('@/hooks/useRuntime', () => ({ ...jest.requireActual('@/hooks/useRuntime'), useRuntime: jest.fn() })) const FINDING: FindingListItem = { - id: 'finding', operation_id: 'saved', title: 'Proof', severity: 'low', description: '', + id: 'finding', operation_id: 'saved', title: 'Proof', severity: 'low', field_values: {}, created_at: '2026-10-08T12:00:00Z', evidence_count: 0, } const EMPTY = { items: [], has_more: false, next_offset: null } @@ -36,15 +40,22 @@ function Harness({ disabled, ...props }: { } -function renderDialog(defaultOperationId: string | null = null): void { - render( - - ) +/** Renders the dialog and returns a function that renders it again, as a context change would. */ +function renderDialog(defaultOperationId: string | null = null): () => void { + // A new element each time, so React renders the dialog again instead of keeping the previous output. + const tree = () => ( + + + + ) + const { rerender } = render(tree()) + return () => { rerender(tree()) } } beforeEach(() => { jest.clearAllMocks() + jest.mocked(useRuntime).mockReturnValue({ ready: true, state: 'ready', generation: 'one' }) for (const method of Object.values(operationsApi)) jest.mocked(method).mockReset() jest.mocked(attacksApi.getAttack).mockReset() ;(attacksApi.getAttack as jest.Mock).mockResolvedValue({ operation_id: 'saved', operation_name: 'Exact' }) @@ -53,7 +64,7 @@ beforeEach(() => { ;(operationsApi.listFindings as jest.Mock).mockResolvedValue({ ...EMPTY, items: [FINDING] }) ;(operationsApi.attachFindingEvidence as jest.Mock).mockResolvedValue({ item: { id: 'stable' }, created: true }) jest.mocked(operationsApi.createFinding).mockResolvedValue(FINDING) - jest.mocked(operationsApi.getFindingOptions).mockResolvedValue({ harm_types: ['Malware', 'Other'] }) + jest.mocked(operationsApi.getFindingFields).mockResolvedValue(DEFAULT_FINDING_FIELDS_FIXTURE) }) it('defaults to the attack\'s own Operation and attaches the viewed attack', async () => { @@ -74,6 +85,22 @@ it('defaults to the attack\'s own Operation and attaches the viewed attack', asy }) }) +it('loads its findings again when a live apply changes the severity order while it is open', async () => { + const user = userEvent.setup() + const renderAgain = renderDialog() + const action = await screen.findByRole('button', { name: 'Link to finding' }) + await waitFor(() => { expect(action).toBeEnabled() }) + await user.click(action) + await screen.findByRole('radio', { name: /Proof/ }) + const loads = jest.mocked(operationsApi.listFindings).mock.calls.length + + jest.mocked(operationsApi.getFindingFields).mockResolvedValue(CONTOSO_FINDING_FIELDS) + jest.mocked(useRuntime).mockReturnValue({ ready: true, state: 'ready', generation: 'two' }) + await act(async () => { renderAgain() }) + await waitFor(() => expect(operationsApi.listFindings).toHaveBeenCalledTimes(loads + 1)) + expect(screen.getByRole('dialog')).toBeInTheDocument() +}) + it.each([null, 'Exact'])('links an attack without an Operation ID (legacy name %s) using the selected Operation', async ( operation: string | null, ) => { @@ -168,10 +195,27 @@ it('offers creation inside the empty picker and retains the Operation link', asy expect(within(screen.getByRole('dialog')).getByRole('button', { name: 'New finding' })).toBeEnabled() }) +it('keeps New finding unavailable until the finding fields load, and retries them', async () => { + const user = userEvent.setup() + jest.mocked(operationsApi.getFindingFields).mockRejectedValueOnce(new Error('Fields offline')) + renderDialog() + await waitFor(() => { expect(screen.getByRole('button', { name: 'Link to finding' })).toBeEnabled() }) + await user.click(screen.getByRole('button', { name: 'Link to finding' })) + expect(await screen.findByText(/Could not load finding fields: Fields offline/)).toBeInTheDocument() + expect(await screen.findByRole('radio', { name: /Proof/ })).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'New finding' })).toBeDisabled() + await user.click(screen.getByRole('button', { name: 'Retry finding fields' })) + await waitFor(() => { expect(screen.getByRole('button', { name: 'New finding' })).toBeEnabled() }) + expect(screen.getByRole('radio', { name: /Proof/ })).toHaveAccessibleName(/Low/) + expect(screen.queryByText(/Could not load finding fields/)).not.toBeInTheDocument() +}) + async function openCreation(user: ReturnType): Promise { await waitFor(() => { expect(screen.getByRole('button', { name: 'Link to finding' })).toBeEnabled() }) await user.click(screen.getByRole('button', { name: 'Link to finding' })) - await user.click(await screen.findByRole('button', { name: 'New finding' })) + const newFinding = await screen.findByRole('button', { name: 'New finding' }) + await waitFor(() => { expect(newFinding).toBeEnabled() }) + await user.click(newFinding) await screen.findByRole('heading', { name: 'New finding' }) } @@ -179,48 +223,49 @@ it('creates the finding with the viewed attack as evidence in one request and cl const user = userEvent.setup() renderDialog() await openCreation(user) - expect(screen.getByRole('combobox', { name: 'Severity' })).toHaveValue('moderate') + expect(screen.getByRole('combobox', { name: 'Severity' })).toHaveValue('Moderate') expect(screen.getByRole('button', { name: 'Save finding' })).toBeDisabled() await user.type(screen.getByRole('textbox', { name: 'Title' }), ' ') expect(screen.getByRole('button', { name: 'Save finding' })).toBeDisabled() await user.type(screen.getByRole('textbox', { name: 'Title' }), 'New assessment') - await user.selectOptions(screen.getByRole('combobox', { name: 'Severity' }), 'important') + await pick(user, 'Severity', 'Important') await user.type(screen.getByRole('textbox', { name: 'Description' }), 'Optional notes') expect(screen.queryByRole('textbox', { name: 'Evidence attack ID' })).not.toBeInTheDocument() await user.click(screen.getByRole('button', { name: 'Save finding' })) expect(await screen.findByText(/Finding created and attack attached/)).toBeInTheDocument() expect(operationsApi.createFinding).toHaveBeenCalledWith('saved', { - id: expect.any(String), title: ' New assessment', severity: 'important', description: 'Optional notes', - severity_other: null, harm_type: null, harm_type_other: null, - attack_result_id: 'viewed', + id: expect.any(String), title: ' New assessment', severity: 'important', + field_values: { description: 'Optional notes' }, attack_result_id: 'viewed', }) expect(operationsApi.attachFindingEvidence).not.toHaveBeenCalled() await waitFor(() => { expect(screen.queryByRole('dialog')).not.toBeInTheDocument() }) await waitFor(() => { expect(screen.getByRole('button', { name: 'Link to finding' })).toHaveFocus() }) }) -it('creates and attaches custom classifications from the same shared form and shows them in the picker', async () => { +it('shows a custom severity but not the harm type in the picker, and creates and attaches custom classifications', async () => { const user = userEvent.setup() jest.mocked(operationsApi.listFindings).mockResolvedValue({ ...EMPTY, items: [{ - ...FINDING, severity: 'other', severity_other: 'Team severity', harm_type: 'Other', harm_type_other: 'Team harm', + ...FINDING, severity: 'Team severity', field_values: { harm_type: 'Team harm' }, }] }) renderDialog() await waitFor(() => expect(screen.getByRole('button', { name: 'Link to finding' })).toBeEnabled()) await user.click(screen.getByRole('button', { name: 'Link to finding' })) - expect(await screen.findByText('Team severity')).toBeInTheDocument() - expect(screen.getByText('Harm-type: Team harm')).toBeInTheDocument() + const entry = await screen.findByRole('radio', { name: /Proof/ }) + expect(entry).toHaveAccessibleName(/Team severity/) + expect(screen.getByText('Team severity')).toBeInTheDocument() + expect(screen.queryByText(/Harm-type/)).not.toBeInTheDocument() + expect(screen.queryByText(/Team harm/)).not.toBeInTheDocument() await user.click(screen.getByRole('button', { name: 'New finding' })) await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Custom') - await user.selectOptions(screen.getByRole('combobox', { name: 'Severity' }), 'other') - await user.type(screen.getByRole('textbox', { name: 'Other severity' }), 'Team severity') - await user.click(screen.getByRole('combobox', { name: 'Harm-type' })) - await user.click(within(await screen.findByRole('listbox')).getByRole('option', { name: 'Other', exact: true })) - await user.type(screen.getByRole('textbox', { name: 'Other harm-type' }), 'Team harm') + await pick(user, 'Severity', 'Other') + await user.type(screen.getByRole('textbox', { name: 'Other Severity' }), 'Team severity') + await pick(user, 'Harm-type', 'Other') + await user.type(screen.getByRole('textbox', { name: 'Other Harm-type' }), 'Team harm') await user.click(screen.getByRole('button', { name: 'Save finding' })) expect(await screen.findByText(/Finding created and attack attached/)).toBeInTheDocument() expect(operationsApi.createFinding).toHaveBeenCalledWith('saved', { - id: expect.any(String), title: 'Custom', description: '', severity: 'other', severity_other: 'Team severity', - harm_type: 'Other', harm_type_other: 'Team harm', attack_result_id: 'viewed', + id: expect.any(String), title: 'Custom', severity: 'Team severity', field_values: { harm_type: 'Team harm' }, + attack_result_id: 'viewed', }) }) diff --git a/frontend/src/components/Chat/FindingEvidenceDialog.tsx b/frontend/src/components/Chat/FindingEvidenceDialog.tsx index a9ab5adcd3..824fde4161 100644 --- a/frontend/src/components/Chat/FindingEvidenceDialog.tsx +++ b/frontend/src/components/Chat/FindingEvidenceDialog.tsx @@ -10,6 +10,7 @@ import { Link } from 'react-router' import { attacksApi, operationsApi } from '@/services/api' import FindingDialog, { type FindingSaveAttempt } from '@/components/Operations/FindingDialog' import FindingSeverityBadge from '@/components/Operations/FindingSeverityBadge' +import { useFindingFields } from '@/hooks/useFindingFields' import { toApiError } from '@/services/errors' import type { FindingCreate, FindingListItem, FindingListResponse, Operation } from '@/types' @@ -37,6 +38,8 @@ export default function FindingEvidenceDialog({ attackResultId, disabled, defaultOperationId = null, open, onOpenChange, returnFocusRef, }: FindingEvidenceDialogProps) { const styles = useFindingEvidenceDialogStyles() + // `open` stays true while the embedded finding dialog replaces the picker, so this covers both. + const { fields, error: fieldsError, retry: retryFields, orderRevision } = useFindingFields(open) const viewKey = attackResultId const [defaultLookup, setDefaultLookup] = useState<{ key: string; operation: Operation | null } | null>(null) const [chosen, setChosen] = useState<{ key: string; operation: Operation } | null>(null) @@ -68,7 +71,7 @@ export default function FindingEvidenceDialog({ // Evidence can come from any attack; the user picks whose findings to link, defaulting to the attack's own Operation. const operation = chosen?.key === viewKey ? chosen.operation : defaultLookup?.key === viewKey ? defaultLookup.operation : null - const requestKey = JSON.stringify([attackResultId, operation?.id, title, offset, listRevision]) + const requestKey = JSON.stringify([attackResultId, operation?.id, title, offset, listRevision, orderRevision]) // A selection only applies to the attack and operation it was made for. const selectionKey = JSON.stringify([attackResultId, operation?.id]) const selection = picked?.key === selectionKey ? picked.finding : null @@ -165,7 +168,8 @@ export default function FindingEvidenceDialog({ return ( <> {notice?.key === viewKey && {notice.text}} - {creating && Operation: {creationSource?.operation.name}. Save and attach the viewed attack. {!draftEligible && @@ -196,7 +200,7 @@ export default function FindingEvidenceDialog({ + {fieldsError && + Could not load finding fields: {fieldsError}{' '} + + } { setTitle(data.value); setOffset(0); setSelection(null); setNotice(null); setAttachError('') @@ -228,10 +236,7 @@ export default function FindingEvidenceDialog({ children: ( {item.title} - - {item.harm_type && Harm-type: { - item.harm_type === 'Other' ? item.harm_type_other : item.harm_type - }} + {fields !== null && } {new Date(item.created_at).toLocaleString()} ), diff --git a/frontend/src/components/Operations/FindingCardValues.tsx b/frontend/src/components/Operations/FindingCardValues.tsx new file mode 100644 index 0000000000..357e443490 --- /dev/null +++ b/frontend/src/components/Operations/FindingCardValues.tsx @@ -0,0 +1,42 @@ +import { useId } from 'react' +import { mergeClasses, Text } from '@fluentui/react-components' + +import type { FindingFields, FindingListItem } from '@/types' +import { describeFindingValues, type FindingValueRow } from '@/utils/findingFields' + +import { useOperationsStyles } from './Operations.styles' + +interface FindingCardValuesProps { + fields: FindingFields + finding: Pick +} + +/** A finding card's filled values, then the values of fields that are no longer configured under their raw keys. */ +export default function FindingCardValues({ fields, finding }: FindingCardValuesProps) { + const styles = useOperationsStyles() + const retiredHeadingId = useId() + const { rows, retired } = describeFindingValues(fields, finding.severity, finding.field_values) + if (rows.length === 0 && retired.length === 0) return null + + return ( +
+ {rows.map((row: FindingValueRow) => row.multiline ? ( +
+ {row.label} +

{row.value}

+
+ ) : {`${row.label}: ${row.value}`})} + {retired.length > 0 &&
+ Retired fields +
    + {retired.map(({ key, value }: { key: string; value: string }) => ( +
  • + {key} +

    {value}

    +
    • + ))} +
+
} +
+ ) +} diff --git a/frontend/src/components/Operations/FindingDialog.styles.ts b/frontend/src/components/Operations/FindingDialog.styles.ts index 5dc303c2d8..8ca35ce167 100644 --- a/frontend/src/components/Operations/FindingDialog.styles.ts +++ b/frontend/src/components/Operations/FindingDialog.styles.ts @@ -4,8 +4,7 @@ import { mobileTouchTarget } from '@/styles/touchTargets' export const useFindingDialogStyles = makeStyles({ form: { display: 'flex', flexDirection: 'column', gap: tokens.spacingVerticalM }, + section: { margin: 0, marginTop: tokens.spacingVerticalS }, actions: { display: 'flex', justifyContent: 'flex-end', gap: tokens.spacingHorizontalS }, button: { ...mobileTouchTarget }, - harmPicker: { minWidth: 0, width: '100%' }, - harmListbox: { maxHeight: '240px', overflowY: 'auto' }, }) diff --git a/frontend/src/components/Operations/FindingDialog.test.tsx b/frontend/src/components/Operations/FindingDialog.test.tsx index 0915b4eaad..c5c8cc4046 100644 --- a/frontend/src/components/Operations/FindingDialog.test.tsx +++ b/frontend/src/components/Operations/FindingDialog.test.tsx @@ -1,197 +1,349 @@ -import { fireEvent, render, screen, waitFor, within } from '@testing-library/react' -import userEvent from '@testing-library/user-event' +import React, { useState } from 'react' +import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react' +import userEvent, { type UserEvent } from '@testing-library/user-event' import { FluentProvider, webLightTheme } from '@fluentui/react-components' -import FindingDialog from './FindingDialog' -import { operationsApi } from '@/services/api' +import { deferred } from '@/test-utils/deferred' +import { findRemovalConfirmation, pick } from '@/test-utils/findingDialog' +import { CONTOSO_FINDING_FIELDS, DEFAULT_FINDING_FIELDS_FIXTURE } from '@/test-utils/findingFieldsFixtures' +import type { FindingCreate, FindingFields } from '@/types' -jest.mock('@/services/api', () => ({ operationsApi: { getFindingOptions: jest.fn() } })) +import FindingDialog, { type FindingDraft } from './FindingDialog' + +const TestWrapper: React.FC<{ children: React.ReactNode }> = ({ children }) => ( + {children} +) const ATTEMPT = { id: expect.any(String), attackResultId: '' } -beforeEach(() => { - jest.clearAllMocks() - jest.mocked(operationsApi.getFindingOptions).mockResolvedValue({ harm_types: ['Hate Speech', 'Malware', 'Other'] }) -}) +const CONTOSO_STEPS = '1. Ask the Contoso assistant to repeat its instructions.' +const CONTOSO_EDIT: FindingCreate = { + title: 'Contoso assistant repeats its system prompt', + severity: 'sev1', + field_values: { + severity_rationale: 'customer_impact', + category: 'model_security', + subcategory: 'system_prompt_leak', + steps: CONTOSO_STEPS, + description: 'Recorded before Contoso configured its own fields.', + }, +} +const CONTOSO_CURRENT_VALUES = { + severity_rationale: 'customer_impact', + category: 'model_security', + subcategory: 'system_prompt_leak', + steps: CONTOSO_STEPS, +} + +interface DialogOptions { + fields?: FindingFields + initialValues?: FindingCreate + editing?: boolean + evidenceField?: boolean + onSave?: (draft: FindingDraft, attempt: { id: string; attackResultId: string }) => Promise + onClose?: () => void +} + +function renderDialog({ + fields = DEFAULT_FINDING_FIELDS_FIXTURE, onSave = jest.fn().mockResolvedValue(undefined), onClose = jest.fn(), + ...props +}: DialogOptions = {}): void { + render() +} + +// Holds the dialog the way the Operation page does, so Cancel can close it and Edit can reopen it. +function EditHarness({ onSave }: { onSave: (draft: FindingDraft) => Promise }) { + const [open, setOpen] = useState(false) + return <> + + {open && { setOpen(false) }} />} + +} + +function saveButton(): HTMLElement { + return screen.getByRole('button', { name: 'Save finding' }) +} + +async function confirmRemoval(user: UserEvent, key: string): Promise { + await user.click(await screen.findByRole('button', { name: `Remove retired value ${key}` })) + const confirmation = await findRemovalConfirmation() + await user.click(within(confirmation).getByRole('button', { name: 'Remove' })) + await waitFor(() => expect(screen.queryByRole('button', { name: 'Keep it' })).not.toBeInTheDocument()) +} describe('FindingDialog', () => { - it('uses the same defaults and severity choices and submits only finding fields', async () => { - const user = userEvent.setup() - const onSave = jest.fn().mockResolvedValue(undefined) - render() - expect(screen.getByRole('combobox', { name: 'Severity' })).toHaveValue('moderate') - expect(within(screen.getByRole('combobox', { name: 'Severity' })).getAllByRole('option').map(option => option.textContent)).toEqual([ - 'Critical', 'Important', 'Moderate', 'Low', 'Informational', 'Other', - ]) - await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Human assessment') - await user.selectOptions(screen.getByRole('combobox', { name: 'Severity' }), 'critical') - await user.type(screen.getByRole('textbox', { name: 'Description' }), 'Notes') - await user.click(screen.getByRole('button', { name: 'Save finding' })) - await waitFor(() => { expect(onSave).toHaveBeenCalledWith({ - title: 'Human assessment', severity: 'critical', description: 'Notes', - severity_other: null, harm_type: null, harm_type_other: null, - }, ATTEMPT) }) + beforeEach(() => { + jest.clearAllMocks() }) - it('arms the unsaved changes guard for a severity only edit', async () => { + it('renders the default fields and saves only the filled values', async () => { const user = userEvent.setup() - const events: Event[] = [] - const listener = (event: Event): void => { events.push(event) } - window.addEventListener('beforeunload', listener) - render() + const onSave = jest.fn().mockResolvedValue(undefined) + renderDialog({ onSave }) - // No field the guard watches has a value yet, so nothing is armed. The - // prevented flag is read after the dispatch, when every listener has run. - fireEvent(window, new Event('beforeunload', { cancelable: true })) - expect(events[0].defaultPrevented).toBe(false) + const severity = screen.getByRole('combobox', { name: 'Severity' }) + expect(severity).toHaveValue('Moderate') + expect(severity).toBeRequired() + await user.click(severity) + expect(within(await screen.findByRole('listbox')).getAllByRole('option').map(option => option.textContent)) + .toEqual(['Critical', 'Important', 'Moderate', 'Low', 'Informational', 'Other']) + await user.click(screen.getByRole('option', { name: 'Moderate' })) - await user.selectOptions(screen.getByRole('combobox', { name: 'Severity' }), 'critical') - fireEvent(window, new Event('beforeunload', { cancelable: true })) - expect(events[1].defaultPrevented).toBe(true) + const harmType = screen.getByRole('combobox', { name: 'Harm-type' }) + expect(harmType).not.toBeRequired() + await user.type(harmType, 'mal') + expect(within(await screen.findByRole('listbox')).getAllByRole('option').map(option => option.textContent)) + .toEqual(['Not set', 'Malware', 'Other']) + await user.click(screen.getByRole('option', { name: 'Not set' })) + expect(harmType).toHaveValue('') - window.removeEventListener('beforeunload', listener) + const description = screen.getByRole('textbox', { name: 'Description' }) + expect(description).not.toBeRequired() + expect(description).toHaveAccessibleDescription('Optional') + expect(screen.queryByRole('heading', { level: 3 })).not.toBeInTheDocument() + + expect(saveButton()).toBeDisabled() + await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Human assessment') + await user.type(description, 'Notes') + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledWith({ + title: 'Human assessment', severity: 'moderate', field_values: { description: 'Notes' }, + }, ATTEMPT)) }) - it('retains an edited draft on error and permits a corrected retry', async () => { - const user = userEvent.setup() - const onSave = jest.fn().mockRejectedValueOnce(new Error('Storage offline')).mockResolvedValue(undefined) - render() - expect(screen.getByRole('heading', { name: 'Edit finding' })).toBeInTheDocument() - await user.type(screen.getByRole('textbox', { name: 'Title' }), ' edited') - await user.click(screen.getByRole('button', { name: 'Save finding' })) - expect(await screen.findByText('Storage offline')).toBeInTheDocument() - expect(screen.getByRole('textbox', { name: 'Title' })).toHaveValue('Existing edited') - await user.click(screen.getByRole('button', { name: 'Save finding' })) - await waitFor(() => { expect(screen.queryByText('Storage offline')).not.toBeInTheDocument() }) - expect(onSave).toHaveBeenCalledTimes(2) + it('renders sections, hints and required markers from the fields', () => { + renderDialog({ fields: CONTOSO_FINDING_FIELDS }) + + expect(screen.getAllByRole('heading', { level: 3 }).map(heading => heading.textContent)) + .toEqual(['Classification', 'Reproduction', 'Tracking']) + const steps = screen.getByRole('textbox', { name: 'Steps to reproduce' }) + expect(steps).toHaveAccessibleDescription('List each prompt you sent to the Contoso assistant, in order.') + expect(steps).toBeRequired() + expect(screen.getByRole('combobox', { name: 'Severity' })).toBeRequired() + expect(screen.getByRole('combobox', { name: 'Severity' })).toHaveValue('') + expect(screen.getByRole('combobox', { name: 'Category' })).toBeRequired() + expect(screen.getByRole('combobox', { name: 'Technique' })).not.toBeRequired() + expect(screen.getByRole('textbox', { name: 'Owner' })).not.toBeRequired() + + const reproduction = screen.getByRole('heading', { level: 3, name: 'Reproduction' }) + const technique = screen.getByRole('combobox', { name: 'Technique' }) + const category = screen.getByRole('combobox', { name: 'Category' }) + expect(reproduction.compareDocumentPosition(technique) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy() + expect(reproduction.compareDocumentPosition(category) & Node.DOCUMENT_POSITION_PRECEDING).toBeTruthy() }) - it('requires custom Other text, preserves it on error, and clears it when returning to presets', async () => { + it('disables save until every required value is filled', async () => { const user = userEvent.setup() - const onSave = jest.fn().mockRejectedValueOnce(new Error('Offline')).mockResolvedValue(undefined) - render() - await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Custom') - await user.selectOptions(screen.getByRole('combobox', { name: 'Severity' }), 'other') - expect(screen.getByRole('button', { name: 'Save finding' })).toBeDisabled() - await user.type(screen.getByRole('textbox', { name: 'Other severity' }), ' Team severity ') - await user.click(screen.getByRole('combobox', { name: 'Harm-type' })) - await user.click(within(await screen.findByRole('listbox')).getByRole('option', { name: 'Other', exact: true })) - expect(screen.getByRole('button', { name: 'Save finding' })).toBeDisabled() - await user.type(screen.getByRole('textbox', { name: 'Other harm-type' }), ' Team harm ') - await user.click(screen.getByRole('button', { name: 'Save finding' })) - expect(await screen.findByText('Offline')).toBeInTheDocument() - expect(screen.getByRole('textbox', { name: 'Other severity' })).toHaveValue(' Team severity ') - expect(screen.getByRole('textbox', { name: 'Other harm-type' })).toHaveValue(' Team harm ') - expect(onSave).toHaveBeenCalledWith({ - title: 'Custom', description: '', severity: 'other', severity_other: ' Team severity ', - harm_type: 'Other', harm_type_other: ' Team harm ', - }, ATTEMPT) - await user.selectOptions(screen.getByRole('combobox', { name: 'Severity' }), 'low') - await user.click(screen.getByRole('combobox', { name: 'Harm-type' })) - await user.click(await screen.findByRole('option', { name: 'Not set', exact: true })) - expect(screen.queryByRole('textbox', { name: 'Other severity' })).not.toBeInTheDocument() - expect(screen.queryByRole('textbox', { name: 'Other harm-type' })).not.toBeInTheDocument() - await user.click(screen.getByRole('button', { name: 'Save finding' })) - expect(onSave).toHaveBeenLastCalledWith({ - title: 'Custom', description: '', severity: 'low', severity_other: null, harm_type: null, harm_type_other: null, - }, ATTEMPT) + const onSave = jest.fn().mockResolvedValue(undefined) + renderDialog({ fields: CONTOSO_FINDING_FIELDS, onSave }) + + await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Contoso prompt leak') + expect(saveButton()).toBeDisabled() + await pick(user, 'Severity', 'Sev 1') + expect(saveButton()).toBeDisabled() + await pick(user, 'Severity rationale', 'Customer impact') + expect(saveButton()).toBeDisabled() + await pick(user, 'Category', 'Model security') + expect(saveButton()).toBeDisabled() + await pick(user, 'Subcategory', 'System prompt leak') + expect(saveButton()).toBeDisabled() + await user.type(screen.getByRole('textbox', { name: 'Steps to reproduce' }), CONTOSO_STEPS) + expect(saveButton()).toBeEnabled() + await user.clear(screen.getByRole('textbox', { name: 'Title' })) + expect(saveButton()).toBeDisabled() + await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Contoso prompt leak') + + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledWith({ + title: 'Contoso prompt leak', severity: 'sev1', field_values: CONTOSO_CURRENT_VALUES, + }, ATTEMPT)) }) - it('searches canonical harm choices without treating typed search text as a custom category', async () => { + it('disables save while a chosen Other has blank text', async () => { const user = userEvent.setup() const onSave = jest.fn().mockResolvedValue(undefined) - render() - await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Search') - await user.type(screen.getByRole('combobox', { name: 'Harm-type' }), 'mal') - expect(await screen.findByRole('option', { name: 'Malware' })).toBeInTheDocument() - expect(screen.queryByRole('option', { name: 'Hate Speech' })).not.toBeInTheDocument() - await user.click(screen.getByRole('option', { name: 'Malware' })) - expect(screen.getByRole('combobox', { name: 'Harm-type' })).toHaveValue('Malware') - await user.click(screen.getByRole('button', { name: 'Save finding' })) - expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ harm_type: 'Malware', harm_type_other: null }), ATTEMPT) + renderDialog({ onSave }) + await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Custom classifications') + + await pick(user, 'Harm-type', 'Other') + expect(saveButton()).toBeDisabled() + fireEvent.submit(saveButton().closest('form') as HTMLFormElement) + expect(onSave).not.toHaveBeenCalled() + await user.type(screen.getByRole('textbox', { name: 'Other Harm-type' }), 'Team harm') + expect(saveButton()).toBeEnabled() + + await pick(user, 'Severity', 'Other') + expect(saveButton()).toBeDisabled() + await user.type(screen.getByRole('textbox', { name: 'Other Severity' }), 'Team severity') + expect(saveButton()).toBeEnabled() + await user.clear(screen.getByRole('textbox', { name: 'Other Harm-type' })) + expect(saveButton()).toBeDisabled() + await user.type(screen.getByRole('textbox', { name: 'Other Harm-type' }), 'Team harm') + + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledWith({ + title: 'Custom classifications', severity: 'Team severity', field_values: { harm_type: 'Team harm' }, + }, ATTEMPT)) }) - it('keeps Other selectable when a category search has no match', async () => { + it('edits resend kept retired values unchanged', async () => { const user = userEvent.setup() - render() - const picker = screen.getByRole('combobox', { name: 'Harm-type' }) - await waitFor(() => expect(picker).toBeEnabled()) - await user.type(picker, 'organization-specific-risk') - const listbox = await screen.findByRole('listbox') - expect(within(listbox).getByRole('option', { name: 'Other', exact: true })).toBeEnabled() - await user.click(within(listbox).getByRole('option', { name: 'Other', exact: true })) - expect(screen.getByRole('textbox', { name: 'Other harm-type' })).toBeInTheDocument() + const onSave = jest.fn().mockResolvedValue(undefined) + renderDialog({ fields: CONTOSO_FINDING_FIELDS, editing: true, initialValues: CONTOSO_EDIT, onSave }) + + expect(screen.getByRole('heading', { name: 'Edit finding' })).toBeInTheDocument() + expect(screen.getByRole('combobox', { name: 'Severity' })).toHaveValue('Sev 1') + expect(screen.getByRole('textbox', { name: 'Steps to reproduce' })).toHaveValue(CONTOSO_STEPS) + const retired = screen.getByRole('group', { name: 'Retired fields · read-only' }) + expect(within(retired).getByText('description')).toBeInTheDocument() + expect(within(retired).getByText('Recorded before Contoso configured its own fields.')).toBeInTheDocument() + + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledWith({ + title: CONTOSO_EDIT.title, severity: 'sev1', field_values: CONTOSO_EDIT.field_values, remove_retired_keys: [], + }, ATTEMPT)) }) - it('reports category loading failures, retains an edit draft, and retries without clearing fields', async () => { + it('keeps the fields it opened with when the page reloads them, and resends a returned value unchanged', async () => { const user = userEvent.setup() - jest.mocked(operationsApi.getFindingOptions).mockRejectedValueOnce(new Error('Categories offline')) const onSave = jest.fn().mockResolvedValue(undefined) - render() - expect(await screen.findByText(/Could not load harm categories: Categories offline/)).toBeInTheDocument() - expect(screen.getByRole('button', { name: 'Save finding' })).toBeDisabled() - expect(screen.getByRole('textbox', { name: 'Other harm-type' })).toHaveValue('Team harm') - await user.click(screen.getByRole('button', { name: 'Retry harm categories' })) - await waitFor(() => expect(screen.getByRole('button', { name: 'Save finding' })).toBeEnabled()) - await user.click(screen.getByRole('button', { name: 'Save finding' })) - expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ harm_type_other: 'Team harm', severity_other: 'Team severity' }), ATTEMPT) + const props = { editing: true, initialValues: CONTOSO_EDIT, onSave, onClose: jest.fn() } + // A live apply brought `description` back after the dialog opened, and the page reloaded its fields. + const reloaded: FindingFields = { + ...CONTOSO_FINDING_FIELDS, fields: [...CONTOSO_FINDING_FIELDS.fields, DEFAULT_FINDING_FIELDS_FIXTURE.fields[1]], + } + const { rerender } = render() + rerender() + + const retired = screen.getByRole('group', { name: 'Retired fields · read-only' }) + expect(within(retired).getByText('description')).toBeInTheDocument() + expect(within(retired).getByText('Recorded before Contoso configured its own fields.')).toBeInTheDocument() + expect(screen.queryByRole('textbox', { name: 'Description' })).not.toBeInTheDocument() + + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledWith({ + title: CONTOSO_EDIT.title, severity: 'sev1', field_values: CONTOSO_EDIT.field_values, remove_retired_keys: [], + }, ATTEMPT)) }) - it('preserves custom harm text when the current Other option is selected again', async () => { + it('a removed retired value is sent in remove_retired_keys, and Cancel restores it', async () => { const user = userEvent.setup() - render() - await waitFor(() => expect(screen.getByRole('combobox', { name: 'Harm-type' })).toBeEnabled()) - await user.click(screen.getByRole('combobox', { name: 'Harm-type' })) - await user.click(within(await screen.findByRole('listbox')).getByRole('option', { name: 'Other', exact: true })) - expect(screen.getByRole('textbox', { name: 'Other harm-type' })).toHaveValue('Keep custom harm') + const onSave = jest.fn().mockResolvedValue(undefined) + render() + // This bare page has no other Tabster user, so it can stay aria-hidden after the dialog closes; keep the opener. + const edit = screen.getByRole('button', { name: 'Edit Contoso finding' }) + await user.click(edit) + + await confirmRemoval(user, 'description') + expect(await screen.findByText('1 retired value is removed when you save.')).toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Remove retired value description' })).not.toBeInTheDocument() + await user.click(screen.getByRole('button', { name: 'Cancel' })) + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()) + expect(onSave).not.toHaveBeenCalled() + + await user.click(edit) + expect(await screen.findByRole('button', { name: 'Remove retired value description' })).toBeInTheDocument() + expect(screen.queryByText(/removed when you save/)).not.toBeInTheDocument() + await confirmRemoval(user, 'description') + await user.click(await screen.findByRole('button', { name: 'Save finding' })) + await waitFor(() => expect(onSave).toHaveBeenCalledWith({ + title: CONTOSO_EDIT.title, severity: 'sev1', field_values: CONTOSO_CURRENT_VALUES, + remove_retired_keys: ['description'], + }, ATTEMPT)) + // The backend rejects a key that is both sent and listed for removal. + expect((onSave.mock.calls[0] as [FindingDraft])[0].field_values).not.toHaveProperty('description') }) - it('blocks blank custom values even when the form is submitted programmatically', async () => { + it('Escape closes only the removal confirmation', async () => { const user = userEvent.setup() - const onSave = jest.fn() - render() - await waitFor(() => expect(screen.getByRole('combobox', { name: 'Harm-type' })).toBeEnabled()) - await user.click(screen.getByRole('textbox', { name: 'Other severity' })) - fireEvent.submit(screen.getByRole('button', { name: 'Save finding' }).closest('form')!) - expect(onSave).not.toHaveBeenCalled() + const onClose = jest.fn() + renderDialog({ fields: CONTOSO_FINDING_FIELDS, editing: true, initialValues: CONTOSO_EDIT, onClose }) + + await user.click(screen.getByRole('button', { name: 'Remove retired value description' })) + await findRemovalConfirmation() + await user.keyboard('{Escape}') + await waitFor(() => expect(screen.queryByRole('button', { name: 'Keep it' })).not.toBeInTheDocument()) + expect(onClose).not.toHaveBeenCalled() + expect(await screen.findByRole('heading', { name: 'Edit finding' })).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Remove retired value description' })).toBeInTheDocument() }) - it('keeps one finding ID while a retry sends the same input, and starts a new one after an edit', async () => { + it('prevents duplicate submissions and reuses the attempt ID after a failure', async () => { const user = userEvent.setup() - const onSave = jest.fn().mockRejectedValue(new Error('Response lost')) - render() + const pending = deferred() + const onSave = jest.fn() + .mockReturnValueOnce(pending.promise) + .mockRejectedValueOnce(new Error('Response lost')) + .mockResolvedValue(undefined) + renderDialog({ onSave }) await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Once') - await user.click(screen.getByRole('button', { name: 'Save finding' })) - await user.click(await screen.findByRole('button', { name: 'Save finding' })) - await user.type(screen.getByRole('textbox', { name: 'Title' }), ' edited') - await user.click(screen.getByRole('button', { name: 'Save finding' })) + await user.click(saveButton()) + + const saving = screen.getByRole('button', { name: 'Saving…' }) + expect(saving).toHaveAttribute('aria-disabled', 'true') + await user.click(saving) + fireEvent.submit(saving.closest('form') as HTMLFormElement) + expect(onSave).toHaveBeenCalledTimes(1) + + await act(async () => { pending.reject(new Error('Response lost')) }) + expect(await screen.findByText('Response lost')).toBeInTheDocument() + expect(screen.getByRole('textbox', { name: 'Title' })).toHaveValue('Once') + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledTimes(2)) + await user.type(await screen.findByRole('textbox', { name: 'Title' }), ' edited') + await user.click(saveButton()) await waitFor(() => expect(onSave).toHaveBeenCalledTimes(3)) + const [first, retry, edited] = onSave.mock.calls.map(([, attempt]) => attempt.id) expect(retry).toBe(first) expect(edited).not.toBe(first) }) + it('shows a rejected Other text in the error message bar and keeps the draft', async () => { + const user = userEvent.setup() + const detail = 'Severity \'Critical\' matches the option \'Critical\' (key \'critical\'). Choose that option instead.' + const onSave = jest.fn().mockRejectedValueOnce({ isAxiosError: true, response: { status: 422, data: { detail } } }) + renderDialog({ onSave }) + await user.type(screen.getByRole('textbox', { name: 'Title' }), 'Typed severity') + await pick(user, 'Severity', 'Other') + await user.type(screen.getByRole('textbox', { name: 'Other Severity' }), 'Critical') + await user.click(saveButton()) + + expect(await screen.findByText(detail)).toBeInTheDocument() + expect(screen.getByRole('textbox', { name: 'Other Severity' })).toHaveValue('Critical') + expect(saveButton()).toBeEnabled() + }) + + it('arms the unsaved changes guard once the draft differs from how it opened', async () => { + const user = userEvent.setup() + const events: Event[] = [] + const listener = (event: Event): void => { events.push(event) } + window.addEventListener('beforeunload', listener) + renderDialog() + + // The prevented flag is read after the dispatch, when every listener has run. + fireEvent(window, new Event('beforeunload', { cancelable: true })) + expect(events[0].defaultPrevented).toBe(false) + await pick(user, 'Severity', 'Critical') + fireEvent(window, new Event('beforeunload', { cancelable: true })) + expect(events[1].defaultPrevented).toBe(true) + await pick(user, 'Severity', 'Moderate') + fireEvent(window, new Event('beforeunload', { cancelable: true })) + expect(events[2].defaultPrevented).toBe(false) + + window.removeEventListener('beforeunload', listener) + }) + it('offers an optional evidence attack only when asked and sends it trimmed', async () => { const user = userEvent.setup() const onSave = jest.fn().mockResolvedValue(undefined) - render( - ) + renderDialog({ evidenceField: true, onSave }) await user.type(screen.getByRole('textbox', { name: 'Title' }), 'With evidence') await user.type(screen.getByRole('textbox', { name: 'Evidence attack ID' }), ' attack-1 ') - await waitFor(() => expect(screen.getByRole('button', { name: 'Save finding' })).toBeEnabled()) - await user.click(screen.getByRole('button', { name: 'Save finding' })) - expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ title: 'With evidence' }), { + await user.click(saveButton()) + await waitFor(() => expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ title: 'With evidence' }), { id: expect.any(String), attackResultId: 'attack-1', - }) + })) }) }) diff --git a/frontend/src/components/Operations/FindingDialog.tsx b/frontend/src/components/Operations/FindingDialog.tsx index 04e7e47bcb..8876aa28c9 100644 --- a/frontend/src/components/Operations/FindingDialog.tsx +++ b/frontend/src/components/Operations/FindingDialog.tsx @@ -1,21 +1,20 @@ -import { useEffect, useRef, useState } from 'react' +import { Fragment, useEffect, useRef, useState } from 'react' import { - Button, Combobox, Dialog, DialogBody, DialogContent, DialogSurface, DialogTitle, - Field, Input, MessageBar, MessageBarBody, Option, Select, Text, Textarea, + Button, Dialog, DialogBody, DialogContent, DialogSurface, DialogTitle, + Field, Input, MessageBar, MessageBarBody, Text, } from '@fluentui/react-components' -import { operationsApi } from '@/services/api' import { toApiError } from '@/services/errors' -import type { FindingCreate, FindingSeverity } from '@/types' +import type { FindingCreate, FindingField, FindingFields } from '@/types' import { generateClientId } from '@/utils/clientId' -import { FINDING_SEVERITY_LABELS } from '@/utils/findingSeverity' +import { firstMissingRequired, type RetiredValue, SEVERITY_KEY, splitRetiredValues } from '@/utils/findingFields' import { useFindingDialogStyles } from './FindingDialog.styles' +import FindingFieldInput from './FindingFieldInput' +import RetiredFieldValues from './RetiredFieldValues' -const EMPTY_DRAFT: FindingCreate = { - title: '', description: '', severity: 'moderate', severity_other: null, harm_type: null, harm_type_other: null, -} +const NO_KEYS: ReadonlySet = new Set() /** One save attempt. The id repeats while the input does, so a retry returns a finding a lost response saved. */ export interface FindingSaveAttempt { @@ -23,52 +22,100 @@ export interface FindingSaveAttempt { attackResultId: string } +/** The finding Save sends. An edit also resends its kept retired values unchanged, and lists the keys to delete. */ +export interface FindingDraft extends FindingCreate { + remove_retired_keys?: string[] +} + interface FindingDialogProps { + /** Read once, when the dialog opens. A reload while it's open doesn't change its inputs or retired values. */ + fields: FindingFields initialValues?: FindingCreate editing?: boolean context?: React.ReactNode /** Shows an optional attack ID to link as evidence. */ evidenceField?: boolean saveDisabled?: boolean - onSave: (draft: FindingCreate, attempt: FindingSaveAttempt) => Promise + onSave: (draft: FindingDraft, attempt: FindingSaveAttempt) => Promise onClose: () => void } +/** A new finding starts from the field defaults. An edit starts from its values, with retired ones set apart. */ +function openingState( + fields: FindingFields, initialValues: FindingCreate | undefined, +): { draft: FindingCreate; retired: RetiredValue[] } { + if (initialValues === undefined) { + const defaults: Record = {} + for (const field of fields.fields) { + if (field.default !== null) defaults[field.key] = field.default + } + return { draft: { title: '', severity: fields.severity.default ?? '', field_values: defaults }, retired: [] } + } + const { current, retired } = splitRetiredValues(fields, initialValues.field_values) + return { draft: { title: initialValues.title, severity: initialValues.severity, field_values: current }, retired } +} + +/** The draft as the backend stores it, which drops blank values. */ +function filledFinding(draft: FindingCreate): FindingCreate { + const filled = Object.entries(draft.field_values).filter(([, value]: [string, string]) => value.trim() !== '') + return { title: draft.title, severity: draft.severity, field_values: Object.fromEntries(filled) } +} + +/** The retired values an edit keeps and resends unchanged: all but the removed ones, whose keys it never sends. */ +function keptValues(retired: RetiredValue[], removed: ReadonlySet): Record { + const kept = retired.filter(({ key }: RetiredValue) => !removed.has(key)) + return Object.fromEntries(kept.map(({ key, value }: RetiredValue) => [key, value])) +} + +function sameFinding(left: FindingCreate, right: FindingCreate): boolean { + const leftValues = Object.entries(filledFinding(left).field_values) + const rightValues = filledFinding(right).field_values + return left.title === right.title && left.severity === right.severity + && leftValues.length === Object.keys(rightValues).length + && leftValues.every(([key, value]: [string, string]) => rightValues[key] === value) +} + +/** Each field's section heading, shown where the section differs from the field before it. */ +function sectionHeading(slots: FindingField[], index: number): string | null { + const section = slots[index].section + return section !== null && section !== slots[index - 1]?.section ? section : null +} + export default function FindingDialog({ - initialValues = EMPTY_DRAFT, editing = false, context, evidenceField = false, saveDisabled = false, onSave, onClose, + fields, initialValues, editing = false, context, evidenceField = false, saveDisabled = false, onSave, onClose, }: FindingDialogProps) { const styles = useFindingDialogStyles() - const [draft, setDraft] = useState({ ...EMPTY_DRAFT, ...initialValues }) + const [openFields] = useState(fields) + const [opening] = useState(() => openingState(openFields, initialValues)) + const [draft, setDraft] = useState(opening.draft) + const [removed, setRemoved] = useState>(NO_KEYS) + // Whether each field shows Other with blank text, which leaves its value '' just like Not set. + const [blankOther, setBlankOther] = useState>({}) const [attackResultId, setAttackResultId] = useState('') const [error, setError] = useState('') const [saving, setSaving] = useState(false) const submitting = useRef(false) const attempt = useRef<{ signature: string; id: string } | null>(null) - const [harmTypes, setHarmTypes] = useState(null) - const [optionsError, setOptionsError] = useState('') - const [optionsRevision, setOptionsRevision] = useState(0) - const [harmOpen, setHarmOpen] = useState(false) - const [harmSearch, setHarmSearch] = useState('') - const classificationsValid = (draft.severity !== 'other' || Boolean(draft.severity_other?.trim())) - && (draft.harm_type !== 'Other' || Boolean(draft.harm_type_other?.trim())) - const canSave = !saveDisabled && Boolean(draft.title.trim()) && classificationsValid && harmTypes !== null - && !optionsError + const slots = [openFields.severity, ...openFields.fields] + const values = { ...draft.field_values, [SEVERITY_KEY]: draft.severity } + const canSave = !saveDisabled && Boolean(draft.title.trim()) + && firstMissingRequired(openFields, draft.severity, draft.field_values) === null + && !Object.values(blankOther).some(Boolean) + const changed = !sameFinding(draft, opening.draft) || removed.size > 0 || attackResultId !== '' useEffect(() => { - let ignore = false - operationsApi.getFindingOptions() - .then(options => { if (!ignore) { setHarmTypes(options.harm_types); setOptionsError('') } }) - .catch((cause: unknown) => { if (!ignore) setOptionsError(toApiError(cause).detail) }) - return () => { ignore = true } - }, [optionsRevision]) - - useEffect(() => { - if (!draft.title && !draft.description && !draft.severity_other && !draft.harm_type && !draft.harm_type_other - && draft.severity === initialValues.severity && !attackResultId) return + if (!changed) return const protectDraft = (event: BeforeUnloadEvent): void => { event.preventDefault() } window.addEventListener('beforeunload', protectDraft) return () => { window.removeEventListener('beforeunload', protectDraft) } - }, [draft.title, draft.description, draft.severity_other, draft.harm_type, draft.harm_type_other, draft.severity, initialValues.severity, attackResultId]) + }, [changed]) + + const applyChanges = (changes: Record): void => { + setDraft((current: FindingCreate) => { + const { [SEVERITY_KEY]: severity = current.severity, ...fieldChanges } = changes + return { ...current, severity, field_values: { ...current.field_values, ...fieldChanges } } + }) + } const save = async (): Promise => { if (submitting.current || !canSave) return @@ -76,10 +123,19 @@ export default function FindingDialog({ setSaving(true) setError('') const trimmedAttackResultId = attackResultId.trim() - const signature = JSON.stringify([draft, trimmedAttackResultId]) + // The server clears a current field left out, and its fields may differ from those this dialog opened with. + const filled = filledFinding(draft) + const finding: FindingDraft = editing + ? { + ...filled, + field_values: { ...keptValues(opening.retired, removed), ...filled.field_values }, + remove_retired_keys: [...removed], + } + : filled + const signature = JSON.stringify([finding, trimmedAttackResultId]) if (attempt.current?.signature !== signature) attempt.current = { signature, id: generateClientId() } try { - await onSave(draft, { id: attempt.current.id, attackResultId: trimmedAttackResultId }) + await onSave(finding, { id: attempt.current.id, attackResultId: trimmedAttackResultId }) } catch (cause: unknown) { setError(toApiError(cause).detail) } finally { @@ -100,63 +156,23 @@ export default function FindingDialog({ { setDraft(value => ({ ...value, title: data.value })) }} /> - - - - {draft.severity === 'other' && - { setDraft(value => ({ ...value, severity_other: data.value })) }} /> - } - {optionsError && - Could not load harm categories: {optionsError}{' '} - - } - - { setHarmOpen(data.open); setHarmSearch('') }} - onChange={event => { setHarmSearch(event.target.value); setHarmOpen(true) }} - onOptionSelect={(_, data) => { - const harmType = data.optionValue - if (harmType !== '' && !harmTypes?.includes(harmType ?? '')) return - setDraft(value => ({ - ...value, harm_type: harmType || null, - harm_type_other: harmType === 'Other' ? (value.harm_type === 'Other' ? value.harm_type_other : '') : null, - })) - setHarmOpen(false) - setHarmSearch('') - }} - listbox={{ className: styles.harmListbox }}> - - {harmTypes?.filter(category => category === 'Other' - || category.toLowerCase().includes(harmSearch.toLowerCase())) - .map(category => )} - {harmTypes && !harmTypes.some(category => category.toLowerCase().includes(harmSearch.toLowerCase())) - && } - - - {draft.harm_type === 'Other' && - { setDraft(value => ({ ...value, harm_type_other: data.value })) }} /> - } - -