fix(rest): gate per-user REST routes on desktop mode enabled#285
Merged
Conversation
Several per-user REST routes (os-settings, session, default-window, seen-intros, pwa-state) gated only on is_user_logged_in() plus the 'read' capability. Because every authenticated role carries 'read', any logged-in user could reach these endpoints even without having entered Desktop Mode. The routes are already scoped to the current user via get_current_user_id(), so this is a missing-gate hardening (defense in depth) rather than cross-user data exposure. Introduce a shared desktop_mode_rest_require_enabled() permission gate (logged in plus desktop_mode_is_enabled(), returning 401 when logged out and 403 when desktop mode is off) and route all six per-user callbacks through it, converging the presence callback that already used this shape onto the shared helper. Add unit tests for the gate (including a regression test that a logged-in user without desktop mode is denied) and update the affected REST tests to opt their users into desktop mode. Update the REST route index and architecture docs to describe the new requirement.
There was a problem hiding this comment.
Pull request overview
This PR hardens Desktop Mode’s per-user REST endpoints so they can only be used by authenticated users who have actually enabled Desktop Mode (desktop_mode_is_enabled()), rather than any logged-in user with the ubiquitous read capability.
Changes:
- Introduces a shared REST permission helper
desktop_mode_rest_require_enabled()that returns401when logged out and403when Desktop Mode is not enabled. - Updates the permission callbacks for the affected per-user REST route families to delegate to the shared helper.
- Updates PHPUnit tests and documentation to reflect the new gate behavior and status codes.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
includes/helpers.php |
Adds shared REST permission gate desktop_mode_rest_require_enabled() returning `true |
includes/session.php |
Routes session REST permission callback through the shared gate and updates docblock. |
includes/seen-intros.php |
Routes seen-intros REST permission callback through the shared gate and updates docblock. |
includes/default-window.php |
Switches default-window route permission callback to the shared gate. |
includes/os-settings.php |
Routes OS settings permission callback through the shared gate and updates docblock. |
includes/pwa.php |
Routes PWA state REST permission callback through the shared gate and updates docblock. |
includes/presence.php |
Replaces the local presence permission implementation with the shared gate. |
tests/phpunit/tests/desktopMode.php |
Adds unit tests for the new shared REST gate (401/403/allow). |
tests/phpunit/tests/desktopModeSession.php |
Updates session REST permission tests for WP_Error and adds a regression test for 403 when not enabled; opts user into Desktop Mode in setup. |
tests/phpunit/tests/desktopModeDefaultWindow.php |
Opts the test user into Desktop Mode in setup so tests reach the route body. |
tests/phpunit/tests/seenIntros.php |
Opts the test user into Desktop Mode in setup so tests reach the route body. |
includes/rest/README.md |
Updates documented permissions for affected routes (but still needs route/verb accuracy fixes). |
docs/architecture.md |
Updates session route permission documentation to reflect the new shared gate and status codes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3fe8f51 to
5688ebf
Compare
Fix pre-existing inaccuracies flagged in PR review: - /default-window is POST only (not GET/POST) - seen-intros routes are POST /intros/seen and DELETE /intros (no /seen-intros) - pwa route is /pwa-state (GET/POST), not /pwa/* - /presence is GET and POST (not GET only)
…desktop-mode-gate # Conflicts: # .wp-env.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Hardens the permission gate on Desktop Mode's per-user REST routes so they require the caller to have actually entered Desktop Mode, not merely to be logged in.
Previously these routes gated only on
is_user_logged_in()pluscurrent_user_can( 'read' ):GET/POST /desktop-mode/v1/os-settingsGET/POST/DELETE /desktop-mode/v1/sessionPOST /desktop-mode/v1/default-windowPOST /desktop-mode/v1/intros/seen,DELETE /desktop-mode/v1/introsGET/POST /desktop-mode/v1/pwa-stateEvery authenticated role carries
read, so any logged-in user (including a Subscriber who never opened the desktop) could read and write through these endpoints.Why this is a hardening, not a data-exposure fix
Each handler is hard-scoped to
get_current_user_id()and accepts no target-user parameter, so a caller can only ever touch their own per-user state. There is no cross-user read or write, and no path to another account's data (verified, see below). This is a missing-gate / defense-in-depth issue (CWE-862, broken access control): accounts that never use Desktop Mode should not be able to write Desktop Mode state.Change
desktop_mode_rest_require_enabled()inincludes/helpers.php: logged in anddesktop_mode_is_enabled(), returning401when logged out and403when desktop mode is off.presencecallback already used this exact shape, so it now delegates to the shared helper (one implementation instead of seven copies).The gate intentionally keys on "desktop mode enabled" rather than a role/capability floor. That is the documented extension point: sites that want to deny Desktop Mode to specific roles use the
desktop_mode_mode_enabledfilter (docs/examples/gate-by-role.md), which now also denies these routes for those users.Tests
403).401.Manual verification (local wp-env)
Against a Subscriber account:
403(was200)401200, but only their own dataDocs
includes/rest/README.mdroute table + conventions updated.docs/architecture.mdsession-routes permission note updated.Out of scope (flagged for separate decision)
/my-wordpress/terms/*(term-stats.php) has the same loosereadgate, but its batch sibling/term-countsalready usesedit_posts, so the right fix there is likelyedit_postsrather than the desktop-mode gate./ai/searchis gated on login plus the AI feature flag and has a documented programmatic surface; left untouched here.No plugin version bump in this PR (release step). New code is tagged
@since 0.8.10.