@claude Can you please review this PR with a focus on security implications?

Claude finished @epeicher's task in 3m 2s —— View job

Security Review of PR #253 — Stale Nonce Refresh

Tasks:

• Understand the request (security-focused review)
• Read PHP nonce-refresh implementation (includes/nonce-refresh.php)
• Read TypeScript nonce-refresh implementation (src/nonce-refresh.ts)
• Read desktop.ts integration (src/desktop.ts)
• Read PHP tests (tests/phpunit/tests/nonceRefresh.php)
• Read JS tests (tests/vitest/nonce-refresh.test.ts)
• Review doc changes (docs/hooks-reference.md, docs/javascript-reference.md)
• Review entry-point change (desktop-mode.php)
• Compile security findings and post review

Overall assessment

The fix is mechanically sound and follows the established Gutenberg/Core pattern for keeping cached nonces alive across a long-running SPA. The data flow is server-controlled, the standard WordPress nonce API is used throughout, and neither PHP nor JS introduces a new exploitable surface. The findings below are graded Medium, Low, and Positive — nothing blocks the merge.

🟡 Medium — registerNonceTarget is documented as a public plugin API but is not reachable by third-party plugins

Files: docs/javascript-reference.md, src/nonce-refresh.ts

The PR description says "New registerNonceTarget() API for plugin authors" and the docs ship this example:

import { registerNonceTarget } from 'desktop-mode/nonce-refresh';

However:

1. There is no 'desktop-mode/nonce-refresh' alias in vite.config.js.
2. registerNonceTarget is not wired onto wp.desktop in the public facade (src/api/facade.ts).
3. Third-party plugin bundles are separate compiled IIFEs — they cannot import from the main bundle's internals.

The function is currently only callable from code already compiled into desktop.min.js. Documenting it as a plugin-author surface without exposing it via wp.desktop.registerNonceTarget creates a dead-end API trail.

Security note for when this is eventually exposed on wp.desktop: any script running in the shell context (including an XSS payload or a compromised plugin bundle) would be able to register a 'wp_rest' target and receive a freshly minted nonce on every Heartbeat tick. Nonces are per-user and bound to the session cookie, so intercepting one only yields the same privilege level the attacker already has — but it is worth documenting this explicitly once the API goes public.

Suggested fix: either (a) add registerNonceTarget to WpDesktopPublicApi + the facade and update the doc example to use wp.desktop.registerNonceTarget(...), or (b) reclassify this as an internal hook and remove the plugin-author framing from javascript-reference.md. Fix this →

🟡 Low — Heartbeat filter fires for all logged-in users on all admin pages, not just Desktop Mode sessions

File: includes/nonce-refresh.php:118

add_filter( 'heartbeat_received', 'desktop_mode_nonce_refresh_heartbeat_received', 5, 2 );

The handler adds three extra wp_create_nonce() calls + a desktop_mode_nonces field to every Heartbeat response for every authenticated user — including an editor on a classic post-edit screen who has never touched Desktop Mode. The comment explains this is intentional ("we want EVERY shell tab to receive the refresh"), but "every shell tab" ≠ "every logged-in user on every page".

This is not exploitable — nonces are per-user, per-action, and bound to the session cookie, so leaking them into an unnecessary response payload has no meaningful security consequence. The concern is minimal data exposure and unnecessary per-tick overhead for non-desktop-mode users.

Suggested fix (optional): add an early return when Desktop Mode isn't active for the current user:

function desktop_mode_nonce_refresh_heartbeat_received( $response, $data ) {
    if ( ! desktop_mode_is_enabled() ) {
        return $response;
    }
    // ...existing logic...
}

Fix this →

🟢 Low / Info — console.error log reveals nonce action strings

File: src/nonce-refresh.ts:117

console.error(
    `[desktop-mode/nonce-refresh] updater for "${ action }" threw:`,
    err,
);

Standard nonce action names (wp_rest, updates) are not secrets. However, if a third-party plugin registers a custom action string that encodes internal details (e.g., a secret internal action name or a per-tenant identifier), that string surfaces in the browser console whenever their updater throws. This is an accepted trade-off given that the error logging is genuinely useful for debugging; it's worth noting in developer-facing docs that action names should be treated as non-sensitive.

✅ Security positives

These are all intentional and worth calling out explicitly:

1. No client-driven nonce enumeration (includes/nonce-refresh.php:108): $data is immediately discarded. The list of actions to refresh is entirely server-determined. A compromised browser script cannot request nonces for arbitrary actions.

2. Proper authentication gate (includes/nonce-refresh.php:112): is_user_logged_in() is checked before any nonces are generated. Unauthenticated heartbeat requests (which WordPress allows) get nothing.

3. Standard WordPress nonce API used throughout: wp_create_nonce() on the server, wp_verify_nonce() in tests. No hand-rolled token generation.

4. Filter output cast to array (includes/nonce-refresh.php:78): (array) apply_filters(...) means a misbehaving filter returning null or a scalar doesn't cause a foreach crash.

5. Non-string / empty action entries silently dropped (includes/nonce-refresh.php:82–84): Protects against a filter returning a mixed array with injected non-string entries.

6. JS payload validation (src/nonce-refresh.ts:103–105): Non-object payloads and non-string / empty values are rejected before any writes occur.

7. try/catch around every updater call (src/nonce-refresh.ts:111–121): A throwing third-party updater cannot strand built-in targets or prevent the shell's own nonce from being refreshed.

8. Tests verify nonce correctness with wp_verify_nonce() (tests/phpunit/tests/nonceRefresh.php:58–63): The PHP tests assert assertSame(1, wp_verify_nonce(...)) — value 1 is WordPress's "valid within first 12-hour window" result. This catches any future mis-keying of the payload.

9. _resetNonceRefreshForTests is correctly annotated @internal (src/nonce-refresh.ts:218) and is not exposed on wp.desktop. Consistent with the existing _resetHeartbeatBusForTests pattern.

✅ WordPress Plugin Check Report

✅ Status: Passed

📊 Report

All checks passed! No errors or warnings found.

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

Addressed the two flagged items in b976264:

🟡 Medium — registerNonceTarget dead-end API trail

Reclassified as @internal. The public extension surface for third-party plugins is already exposed on the facade — wp.desktop.heartbeat.subscribe('desktop_mode_nonces', cb) — so I rewrote the doc to use that recipe instead of inventing a second path. registerNonceTarget stays as a framework-only helper used by our own built-in updaters; the doc explicitly tells plugin authors not to import it. Avoids the cross-bundle import problem without growing the public API surface for a one-bug fix.

🟡 Low — heartbeat handler fires for non-shell users

Gated on desktop_mode_is_enabled() rather than is_user_logged_in(). Users who haven't opted into Desktop Mode (editors on classic post-edit, subscribers reading the front-end heartbeat) no longer carry the desktop_mode_nonces field on their ticks. Added a PHPUnit case covering the opted-out path.

🟢 Low/Info — console.error reveals action strings

Left as-is — agreed it's an acceptable trade-off and the action names in our default set (wp_rest, desktop-mode-plugins, updates) carry no sensitivity. Worth surfacing in a future plugin-author guide that custom action names should be treated as non-sensitive.

Test status: lint, tsc --noEmit, npm run build green; 1464/1464 JS tests pass; PHP tests still need CI (local wp-env port collision).