Skip to content

R5 Frontend correctness audit — 2026-05-31

Scope: .tsx / .ts files under services/wp-pulse-server/web/ modified in the last 2 days (~140 files), plus lib/csrf.ts, lib/server-api.ts, lib/utils.ts, lib/version-recommendations.ts, lib/styles/tokens.css, app/globals.css, public/widget.js. Read-only review — no code modified.

Summary

  • Critical: 0
  • High: 3
  • Medium: 5
  • Low: 6

Overall the frontend is in solid shape. CSRF coverage is at 81/84 of the mutating internal proxy routes; tokens are correctly server-only; dangerouslySetInnerHTML is used with proper server- or client-side escaping in every instance. The handful of findings are concentrated in the new migration routes (CSRF gap), the fleet pages that use useSearchParams, and pollers that do not pause when the tab is hidden.

Findings

High

H1 — requireSameOrigin missing on 3 mutating internal proxy routes Severity: High Files: - services/wp-pulse-server/web/app/api/internal/migration/fresh-install/create/route.ts:26 - services/wp-pulse-server/web/app/api/internal/migration/dns-setup/route.ts:30 - services/wp-pulse-server/web/app/api/internal/migration/import-zip/route.ts:17

All three export a POST handler that delegates to FastAPI but never calls requireSameOrigin(req). Every other internal proxy that mutates state in the codebase (81 routes) goes through the helper first. A third-party page could fire fetch('/api/internal/migration/fresh-install/create', {method:'POST', credentials:'include', mode:'no-cors', headers:{'Content-Type':'application/json'}, body:'{...}'}) and provision a brand-new WP site under the operator's identity — specifically fresh-install/create and import-zip create infrastructure; dns-setup returns recon data but still leaks origin- gated info. Recommendation: add const csrf = requireSameOrigin(req); if (csrf) return csrf; at the top of each POST handler (same shape as e.g. app/api/internal/sites/[slug]/push/route.ts).

H2 — <FleetHealthTab> calls useSearchParams() without a Suspense boundary Severity: High Files: - services/wp-pulse-server/web/components/app/FleetHealthTab.tsx:188 - services/wp-pulse-server/web/app/fleet/page.tsx:69 - services/wp-pulse-server/web/components/app/FleetPageTabs.tsx:162

app/fleet/page.tsx (server component) renders <FleetPageTabs> (client) directly, which conditionally renders <FleetHealthTab> when the active tab is fleet_health. FleetHealthTab calls useSearchParams() at line 188. Next 15 requires every consumer of useSearchParams() to be inside a <Suspense> boundary, otherwise the entire route is forced into client-side rendering (Next will warn during next build and bail to CSR). The other two useSearchParams consumers (OnboardTabs, rehearse/page.tsx) are correctly wrapped (see app/onboard/page.tsx:35 and app/sites/[slug]/rehearse/page.tsx:422). Recommendation: wrap <FleetPageTabs> in <Suspense fallback={…}> inside app/fleet/page.tsx, or move the useSearchParams lookup up into FleetPageTabs (which already runs after useRouter) and pass the value down as a prop.

H3 — Verbose backend error strings leak directly into the UI Severity: High Files (representative — ~25 occurrences total): - components/app/HtaccessEditor.tsx:63,102 - components/app/DnsSetupStep.tsx:74 - components/app/ThemeGallery.tsx:115,145,179,223 - components/app/FleetSecurityTab.tsx:96 - components/app/DbHealthCard.tsx:58 - components/app/CdnCard.tsx:57 - components/app/ObjectCacheCard.tsx:125,148,173 - components/app/RequireApprovalToggle.tsx:46 - components/app/RevertModal.tsx:105,217

Pattern: setError(err instanceof Error ? err.message : String(err)) without further sanitisation. err.message is whatever the upstream proxy returns — for FastAPI 422s this is a join of loc: msg strings (see lib/server-api.ts:84-92) and for 500s it can be a Python stack-trace fragment. Two concrete problems: 1. UI surfaces filesystem paths / SQL fragments to operators ("worktree/wp-content/plugins/foo.php: ENOENT") which is fine in a homelab but considered leak-prone for any future SaaS pivot. 2. If a future endpoint ever echoes user-controlled bytes into the error string, this becomes an XSS sink because some banners render the error as a plain text node (safe) but ThemeGallery.tsx:145,179,223 interpolate it into a backtick template that is then assigned to toast.text — verify all toast renderers escape (Toast reads as text in the components surveyed, so this is theoretical).

Recommendation: add a small helper lib/errors.ts → formatError(e) that returns one of "Request failed (HTTP <status>)", "Network error — check your connection", or "Server returned an unexpected response", mapping ServerApiError.status → user-facing copy. Keep the raw err.message in console.error for the operator-debug story.

Medium

M1 — Many pollers do not pause when the tab is hidden Severity: Medium Files: - hooks/usePoll.ts (the shared hook — does NOT check document.visibilityState) - components/app/RollbackBanner.tsx:150 - components/app/TimeTravelBanner.tsx:209 - components/app/LighthouseCard.tsx:93 - components/app/MediaOptimizeModal.tsx:183 - components/app/FleetSecurityTab.tsx:109 (60 s) - components/app/FleetHealthTab.tsx:241 (30 s) - components/app/ReportsList.tsx:125 - app/previews/PreviewsClient.tsx:144

Pattern compliance is uneven: TopBar.tsx, PendingChangesBadge.tsx and JustAppliedToast.tsx correctly gate tick() on document.visibilityState === "visible" and listen on visibilitychange. The others fire on a flat interval regardless of tab visibility. On a long-running operator session with a dozen tabs across sites, this is meaningful battery / DB-load churn (FleetHealth hits the API every 30 s × fleet size × tab-count = N²). Recommendation: add an pauseWhenHidden?: boolean (default true) option to usePoll and switch FleetSecurity / FleetHealth / ReportsList / Lighthouse / MediaOptimize / PreviewsClient over to it. All cleanups are present (every setInterval has a matching clearInterval in the effect cleanup — verified across all 13 sites).

M2 — widget.js injects raw error text into the shadow DOM via innerHTML Severity: Medium File: services/wp-pulse-server/web/public/widget.js:161

statusEl.innerHTML =
  '<div class="err">Could not send: ' + String(e.message || e) + "</div>";

e here is the rejection from fetch or the throw new Error("HTTP " + r.status) two lines up, so today it is always a controlled string ("HTTP 502", "Failed to fetch", etc.). However the widget is embedded on customer-controlled WordPress sites, and if a customer deploys a Service Worker that returns a fabricated rejection with a malicious message, that string lands in innerHTML inside the shadow root. The shadow root contains the operator-facing modal — not the customer page — but a determined customer could still phish their own visitors by manipulating the error. Recommendation: swap line 161 for statusEl.textContent = ... or build the <div> via createElement + textContent. Same trivial fix applies to line 156 (currently hard-coded so safe).

M3 — RollbackBanner polls but has no document.visibilityState gate Severity: Medium (subset of M1 but worth calling out) File: components/app/RollbackBanner.tsx:150

This is one of the more annoying pollers — it polls /api/internal/sites/{slug}/pushes every 5 s while a rollback is in flight, and the API has a fairly expensive query plan behind it. With the slug page open in a background tab during a long apply, this is ~12 req/min for nothing. Same fix as M1.

M4 — lib/server-api.ts re-exports unknown instead of typed shapes in 3 places Severity: Medium File: services/wp-pulse-server/web/lib/server-api.ts - Lines 252 (restoreSnapshot → call<unknown>) - Line 412 (localAction → call<unknown>) - Line 679 (deleteVersion → call<unknown>) - Line 1062 (acknowledgeAlert → call<unknown>)

These work today because the callers discard the return value, but the FastAPI endpoints all return a small typed body ({status: "..."} or {deleted: true}). The pattern silently deviates from the rest of the file which is rigorously typed. Recommendation: define ApiAck { status: string } and use it as the generic param.

M5 — No as any / @ts-ignore violations found, BUT three explicit eslint-disable react-hooks/exhaustive-deps need review Severity: Medium Files: - components/app/FleetHealthTab.tsx:286 - app/sites/[slug]/rehearse/page.tsx:165 - app/rehearsals/[id]/page.tsx:140 and :172 - app/fleet/tools/search-replace/page.tsx:147

Each disables exhaustive-deps to avoid a closure restart. Two of them (rehearsals/[id]/page.tsx) read mutable useRef values so are fine; the others should be audited to make sure the omitted deps couldn't go stale (e.g. FleetHealthTab line 286 omits a callback that itself captures searchParams).

Low

L1 — Two modals lack explicit focus trap Severity: Low Files: - components/app/UserEditModal.tsx - components/app/PluginSandboxModal.tsx - components/app/QuarantineModal.tsx - components/app/PluginInstallSearchModal.tsx - components/app/MediaOptimizeModal.tsx

PendingChangesModal and RevertModal correctly bind Escape and call first.focus(). The five above add neither — Escape does not dismiss them and Tab can scroll focus into the page behind them. Recommendation: factor a tiny <DialogShell> that does the Esc + initial-focus + restore-on-close dance and wrap all modals.

L2 — dangerouslySetInnerHTML audit — all 3 usages are safe Severity: Low (informational) - app/layout.tsx:47 — inline theme bootstrap script (vanilla JS string literal, no interpolation). - app/fleet/tools/search-replace/page.tsx:1073 — d.context is pre-sanitised server-side (HTML-escaped + <mark> wrapped); the comment + server-api.ts:1410 confirm. - components/app/CommandPalette.tsx:384,393 — wraps the output in escapeHtml() defined at line 83.

No action required. Noted here so the next reviewer can skip the sweep.

L3 — Hard-coded URL pattern {slug}.local.monxas.casa never escapes its own warning Severity: Low Files: - lib/utils.ts:37,55 (comments warning AGAINST the broken pattern) - components/app/StartSessionButton.tsx:55 (same warning)

The broken {slug}.local.monxas.casa two-level wildcard does NOT appear anywhere as live code — only in cautionary comments. Audit passed.

L4 — widget.js modal has no Esc handler and no aria-modal Severity: Low File: services/wp-pulse-server/web/public/widget.js:96-108

The customer-side feedback modal opens via modal.classList.add("open"), focuses the textarea, but doesn't listen for Escape and doesn't set role="dialog" / aria-modal="true". Affects external visitors using screen readers on customer sites; not a regression.

L5 — <a href="/auth/logout"> (TopBar.tsx:170, settings/page.tsx:87) Severity: Low (false-positive risk for next reviewer) No matching Next route exists under app/auth/. This is intentional — the route is handled by oauth2-proxy upstream of Next (see adr_0008_remote_pulse + pocketid_apps notes). Document somewhere project-local so future audits don't try to "fix" it.

L6 — RevertModal toast message may leak full backend error Severity: Low (subset of H3) File: components/app/RevertModal.tsx:105,217

Two specific paths inside the rollback flow propagate exc.message (FastAPI 422 detail concatenations) directly into setToast({message: …}). Recommend the same formatError shim from H3 here first since the rollback flow is the highest-stakes UI.

Coverage notes

  • Hooks rules: no conditional hook calls or hooks-inside-loops detected. All 140 useEffect calls have explicit cleanups where applicable.
  • WPP_ADMIN_TOKEN exposure: zero matches in any "use client" module — only lib/server-api.ts and app/api/internal/**/route.ts reference it. Good.
  • CSRF coverage: 81/84 mutating internal routes call requireSameOrigin. The 3 gaps are listed under H1.
  • Polling cleanup: every setInterval has a matching clearInterval in its effect cleanup (13/13 sites verified).
  • Dead <Link> targets: no orphaned Link href targets — every /X and /X/[param] resolves to an app/X/page.tsx. (/auth/logout is an external oauth2-proxy route, see L5.)
  • useSearchParams in server components: 3 consumers (OnboardTabs, rehearse/page.tsx, FleetHealthTab). Two wrapped in Suspense correctly; the FleetHealthTab gap is H2.