Skip to content

Admin security, crash and data-loss fixes - #39

Closed
theobong wants to merge 21 commits into
chore/admin-campaign-02-test-harnessfrom
fix/admin-campaign-03-security-crash
Closed

theobong wants to merge 21 commits into
chore/admin-campaign-02-test-harnessfrom
fix/admin-campaign-03-security-crash

Conversation

@theobong

Copy link
Copy Markdown
Member

What changed

Security, crash and data-loss fixes for the operator dashboard:

  • Pin tooltips on the home live map show report and event titles as plain text. Before, a title with HTML in it ran as markup in the operator's session.
  • A malformed link no longer blanks the app.
  • A section that fails to load, or crashes, shows an error with Reload instead of a white page.
  • Confirm dialogs no longer confirm on Enter from Close or Cancel.
  • The compose, template, "Link reports" and "New organization" panels no longer throw away a typed draft on Escape or a stray backdrop click.
  • Attachments only link to web URLs.
  • Statuses this build doesn't know no longer crash pages.
  • Saving an org profile no longer overwrites fields someone else changed meanwhile.

Visible in: admin.

Before you start

Verify

[Admin]

Map tooltips:

  1. On the "Dashboard", hover the pin of the report whose title contains <img src=x onerror=alert(1)>. Expect: the tooltip shows that text literally, followed by · and the place. No alert appears and no broken image renders.

Malformed links:
2. Open admin.civfix.dev/#/reports/%E0%A4%A in a new tab. Expect: the "Dashboard" loads. Before this fix the page stayed blank.

Error boundaries:
3. From the "Dashboard", open DevTools, switch the network to Offline, and click a tile you have not opened yet, for example "Analytics". Expect: the top bar stays, and the page shows "This page could not load" and "The dashboard may have been updated. Reload to get the latest version." with a single "Reload" button.
4. Click "Dashboard" in the top bar. Expect: the Dashboard renders normally. Switch back online and click "Reload" to recover the section.

Confirm dialogs:
5. Open "Users", pick an Active account and click "Ban account". Expect: the "Ban user" dialog opens with focus on "Cancel".
6. Press Enter. Expect: the dialog closes, nobody is banned, and focus returns to "Ban account".
7. Reopen it, click the dialog's body text, and press Enter. Expect: nothing happens and the dialog stays open.
8. Press Tab once to reach "Ban", then press Enter. Expect: the account is banned and a toast confirms it. Undo with "Un-ban" (Tab to "Un-ban", then Enter).
9. Open "Organizations", select an org and click "Suspend". Type a reason, then press inside the reason box, drag out past the dialog edge and release over the dark backdrop. Expect: the dialog stays open with the reason intact.

Draft protection:
10. Open "Mail" and click "Compose". Type a subject, then press Escape and click the backdrop. Expect: "New message" stays open with the subject, and the page does not jump to the Dashboard.
11. Click "Cancel". Expect: the modal closes and focus returns to "Compose".
12. In "Mail", click "Default template" and edit the body. Press Escape, then click the backdrop. Expect: the edit is kept. "Cancel" discards it.
13. In "Jurisdictions", pick a row and click "Edit template". Expect: it behaves the same as step 12.
14. In "Events", select a cleanup and click "Link reports". Select one report, then press Escape and click the backdrop. Expect: the modal stays open with the selection. With only search text typed and nothing selected, Escape closes it.
15. In "Organizations", click "New organization", type a name, then click the dimmed backdrop. Expect: the panel stays open with the name. "Cancel" discards it.

Attachments:
16. In "Mail", switch to "Inbox" and open an email with an attachment. Expect: the attachment appears as a chip with the file name and size, and opens in a new tab.

Concurrent profile edits:
17. Operator A: "Organizations" → select an org → "Edit profile" → change only the description. Operator B: edit the same org's website and save. Operator A: wait 30 seconds, switch the network offline and back online, then "Save changes" with a reason. Expect: the website still shows B's value and the description shows A's.

Regression

Other confirm and reason dialogs: [Admin]

  1. For each of these, open the dialog, press Enter once, then Tab to the action and press Enter: "Reports" "Approve", "Send to city", "Remove report" and "Reject"; "Events" "Cancel event"; the "Moderation" queue "Remove"; the gov-claim "Approve"; "Organizations" "Verify as ..."; "Signup pages" "Unpublish". Expect: the first Enter always cancels, and the action runs only when its own button is activated. Escape still closes every dialog.
  2. In a reason prompt such as "Organizations" → "Suspend", type a reason and press Ctrl+Enter (Cmd+Enter on Mac). Expect: the prompt submits; plain Enter adds a new line.

Photos: [Admin]

  1. In "Reports", open a report with photos and click the main photo. Expect: the viewer opens. A click on the backdrop closes it; a press that starts on the photo and is released on the backdrop does not. The arrow keys, "Close" and "Refresh photo" work as before.

Escape and error states: [Admin]

  1. With no dialog open, press Escape on any section page. Expect: it returns to the Dashboard as before.
  2. Stop the API, or go offline after the page has loaded, and open "Reports". Expect: "Could not load this" with "Try again", unchanged.

Status pills: [Admin]

  1. Browse "Users", "Events", "Organizations" (and its "Events" tab) and "Reports". Expect: every known status shows its usual label and color.

Not covered

  • Statuses unknown to this build, and a render error that is not a network failure, can only be produced with a DevTools response override. The unit tests cover them.
  • The create-organization panel does not trap focus yet; that is PR 4.
  • "Signup pages" and "Host messaging" status pills still crash on an unknown status; that is PR 4.

Findings addressed

  • ADM-SEC-001 (critical): Leaflet bindTooltip received citizen text as an HTML string. It now gets a text node. The divIcon glyph lookup uses own keys only. A full sink audit found no other HTML sinks: attribution strings are constants, and divIcon interpolates constants only.
  • ADM-BUG-008: decodeURIComponent on the hash threw at module load and on popstate. The route now falls back to home.
  • ADM-BUG-002: there was no error boundary.
    • A section boundary is keyed by page; a root boundary wraps sign-in and the app.
    • A failed lazy chunk offers Reload only, because React.lazy caches the failed import.
    • Developer exception text is never shown.
  • ADM-BUG-001: a window-level Enter confirmed dialogs even when Close or Cancel had focus (dangerous for ban, remove, send). Now every confirm opens on Cancel, and only the confirm button's own activation confirms. A held Enter can't answer the dialog it opened.
  • ADM-BUG-038: a text-selection drag released on the backdrop cancelled prompts and the lightbox. One shared backdrop hook now dismisses only when the press and the click are both on the backdrop.
  • ADM-BUG-003: mail compose, the forward template (Mail and Jurisdictions) and "Link reports" become real dialogs:
    • role="dialog", aria-modal and a label;
    • a focus trap, with focus returned to the opener;
    • a named close button;
    • Escape and backdrop close only an unchanged draft.
  • ADM-BUG-011: the create-organization backdrop discarded a filled-in draft that Escape already protected.
  • ADM-BUG-019, plus the same shape for orgs: enum view maps were indexed without a fallback. Unknown values now render raw in a neutral pill (users, events, orgs), and reports offer no transitions. There is one shared event-status fallback.
  • ADM-BUG-056: the profile editor diffed against the live org, so a refetch mid-edit made the save send stale fields. It now diffs against its starting snapshot.
  • Attachment hardening (A4-S1): attachment hrefs are now allowed only for http(s) URLs, with rel="noopener noreferrer". One chip component shows "link unavailable" otherwise. The backend already presigns these keys, so real links keep working.

Decisions for the reviewer

  • Enter no longer confirms: every confirm dialog opens focused on "Cancel", and only activating the confirm button confirms. Before, Enter anywhere confirmed, even with Close focused. Operators lose the Enter-to-confirm habit in exchange for no accidental approve, send or ban.
  • Malformed links: a malformed focus id in a link opens the Dashboard rather than the section with nothing selected.
  • Unknown statuses: a status this build doesn't know shows its raw value (for example "archived") in a grey pill. The report pill keeps its existing "Needs verification" bucket fallback.
  • Draft rule: a modal with an unchanged draft still closes on Escape or a backdrop click. A changed draft closes only via its own "Cancel" or Close button; there is no "discard?" prompt, which matches the create-organization panel's existing rule.

User-visible copy changes

Where Before After
Section load failure blank white app "This page could not load" / "The dashboard may have been updated. Reload to get the latest version." / "Reload"
Section render error blank white app "Something went wrong" / "This page hit an unexpected error. Try again, or reload the dashboard." / "Try again", "Reload"
Attachment with no web URL link to a broken URL chip reading " · · link unavailable"
Compose and "Link reports" close buttons unnamed icon accessible name "Close"
Unknown status pill (users, events, orgs) page crash, or "Upcoming" for events the raw status value

Tests changed

  • Flipped "(current behavior)" pins from Admin test harness and characterization safety net #38 that described the defects this PR fixes:
    • "parses a pin title in the tooltip as HTML markup" now asserts literal text;
    • two malformed-percent-encoding tests (throws) now assert the home fallback.
  • modal-accessibility.test.tsx: dialog focus and Enter tests now assert Cancel-first focus. The "confirms on Enter from a target with no activation of its own" test became "does not confirm".
  • Event status test: the unknown fallback changed from "Upcoming" to the raw value (lib test and page test).
  • providers.test.tsx and error-boundary.test.tsx: a non-API exception shows the fixed line, not its raw message.
  • Five characterization tests from Admin test harness and characterization safety net #38 were made load-robust: they now wait on the right pane and use fake timers for debounce. Those commits live on Admin test harness and characterization safety net #38.

Verification

  • Node v22.23.2. pnpm lint, pnpm typecheck, pnpm build and pnpm test pass: 72 files, 685 tests.
  • The suite also passed 6 times sequentially and 4 times with two suites running in parallel.
  • Every fix has a test that fails without it. A reviewer reverted each fix's source in a scratch copy and confirmed:
    • leaflet 9 tests fail;
    • ui-store 4;
    • dialog/lightbox 7;
    • mail 8;
    • forward-template 7;
    • events 8;
    • inbox 1;
    • create-org 2;
    • profile 1;
    • org status 1;
    • reports 1;
    • users 2;
    • boundaries 3.
  • Adversarial review ran over correctness, security, performance and conventions, with a re-review of the fixes. It found and this PR fixed:
    • non-danger confirms that Enter could commit;
    • a held Enter that could answer a new dialog;
    • three different backdrop mechanisms;
    • two event-status fallbacks;
    • the boundary showing raw exception text;
    • duplicated attachment markup.
  • The security pass confirmed:
    • the XSS is closed on every pin path;
    • the attachment gate rejects javascript:, data:, control-character and protocol-relative tricks.

🤖 Generated with Claude Code

@byteful byteful closed this Sep 24, 2026
@byteful
byteful deleted the fix/admin-campaign-03-security-crash branch September 24, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants