Repository navigation
FEAT: Findings dashboard with persistent Operations in the GUI - #3066
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| let ignore = false | ||
| operationsApi.list() | ||
| .then(result => { | ||
| if (!ignore && !result.items.some(operation => operation.name === currentValue)) onMissingRef.current() |
There was a problem hiding this comment.
🔴 Must Fix: Please don't silently remove an existing Operation when it isn't in the new table.
On upgrade, an existing preference or server default such as operation: "contoso_q3" has no OperationEntries row because the migration doesn't import existing labels. This check calls onMissing, which removes the label and persists that removal in the browser. Subsequent attacks and scans then lose their Operation attribution without the user choosing to clear it.
I reproduced this in the browser. The updated keeps a legacy operation removable without offering it as a saved choice test also fails at line 314: it expects op_chosen_elsewhere but receives an empty value.
Please preserve the existing attribution until the user explicitly changes it, or provide an explicit import/register flow before replacing the label picker. Add an upgrade test covering both an existing browser preference and a configured default when the new table is empty.
| ...DEFAULT_HISTORY_FILTERS, operation: [current.operation.name], | ||
| })}`}>View execution history</Link> | ||
| <Text>Human assessments, separate from attack outcomes and scores.</Text> | ||
| {notice && <MessageBar intent="success" aria-live="polite"><MessageBarBody>{notice}</MessageBarBody></MessageBar>} |
There was a problem hiding this comment.
🟡 Should Fix: The success message can overlap the finding it is supposed to confirm.
I reproduced this at 390px width by saving a finding with a roughly 195-character title and a long description. The MessageBar shrinks to 36px high while its text needs about 120px, so the confirmation is drawn over the next finding's heading and severity. It also spills outside the background on desktop.
This section is a height-constrained flex column, so the status bar needs to keep its natural height. Please prevent the MessageBar from shrinking, or use a short confirmation such as Finding saved with the finding title shown separately. Add a browser assertion using long content, since jsdom won't catch the overlap.
| listbox={{ className: styles.harmListbox }}> | ||
| <Option value="">Not set</Option> | ||
| {harmTypes?.filter(category => category.toLowerCase().includes(harmSearch.toLowerCase())) | ||
| .map(category => <Option key={category} value={category}>{category}</Option>)} |
There was a problem hiding this comment.
🟡 Should Fix: Keep the custom-category option available when the search has no match.
For example, typing organization-specific-risk filters out every category, including Other. The dropdown then says Choose Other to enter custom text, but the only enabled choice is Not set. I confirmed this in the GUI.
Please keep Other outside the filtered list, or add a selectable Use this as a custom category action that selects Other and fills the custom-text field. Add a test that searches for an unknown category and completes that flow without clearing the search.
| <DialogActions> | ||
| <Button className={styles.button} disabled={submitting} onClick={() => { setOpen(false) }}>Cancel</Button> | ||
| <Button className={styles.button} appearance="primary" | ||
| disabled={submitting || !selection || settledKey !== requestKey || Boolean(listError) || disabled} |
There was a problem hiding this comment.
🔴 Must Fix: Keep keyboard focus inside the attachment dialog while submitting and after a failure.
I reproduced this by selecting a finding and making the evidence POST return 503. Disabling the focused Attach button moves focus to document.body. When the request fails, the error is visible but focus stays outside the modal, and pressing Escape no longer closes it.
Please use disabledFocusable for the submitting action, as the finding form already does, or explicitly restore focus inside the dialog on failure. The submission guard already prevents duplicate requests. Also announce the attachment error with a live region. Add a browser test that fails an attachment, checks that focus remains inside the dialog, and dismisses it with Escape.
| }}> | ||
| {page?.items.map(item => <Radio key={item.id} value={item.id} disabled={submitting} label={ | ||
| <span title={`Finding ID: ${item.id}`}> | ||
| <span className={styles.findingHeading}>{item.title} |
There was a problem hiding this comment.
🟡 Should Fix: Constrain and wrap long finding titles and custom classifications.
The form accepts these values, but the attachment picker does not keep them within the label width. With a long title/custom severity, I measured a radio row about 1,498px wide inside a 326px mobile dialog. The user has to scroll horizontally to read the choice. The detail page has a related problem at OperationDetailPage.tsx:154: custom severity text needs about 80px of height inside a 20px Badge and overlaps the title and timestamp.
Please give the radio/label flex children a shrinkable width and wrap long text. Custom severity text also needs a wrapping, auto-height presentation instead of a fixed-height Badge, on both the detail page and picker. Add desktop and narrow-screen browser coverage with long title and Other-classification values.
Description
This adds a findings dashboard to the GUI. A red teamer can now write a finding while testing (title, severity, an optional description and harm type), attach the conversations that prove it, and see everything an Operation turned up on one page.
To make that work, Operations become real records. They used to be a free-text label on attacks and scanner runs, so "Contoso Q3" and "contoso q3 " counted as two different engagements and there was nothing to attach a finding to. Now there's an
OperationEntriestable with a stable id and unique names (case and surrounding whitespace ignored). Findings, evidence links and per-Operation counts hang off that id. It also sets up a later change that gives attacks and scanner runs a properoperation_idforeign key in place of the label match.What's in it:
OperationEntry,FindingEntryandFindingEvidenceEntrytables, plus one Alembic migration (c8d3e5f7a901). Its downgrade refuses to run once any Operation exists, so it can't silently drop data. Harm type uses the existingHarmCategoryenum. Findings are ordered by severity, then newest first, and paginated.doc/gui/0_gui.mdcovers Operations and findings.Severities are critical, important, moderate, low, informational, and "other" with required free text.
Not in this PR:
operation_idforeign keys on attacks and scanner runs. The link is still the Operation name label.Tests and Documentation
pytest tests/unit/memory tests/unit/backend: 3530 passed, 5 skipped.npm test -- --runInBand(frontend): 112 suites, 2847 passedpre-commit run --files <changed files>: passed.frontend/e2e/operations.spec.ts,labels-operation-picker.spec.ts) and anything against SQL Server.doc/gui/0_gui.mdupdated.