[TallblokeUK]

Safe mode's capability check could be re-entered while WordPress was still resolving the current user, on sites running a plugin that builds a URL from a determine_current_user callback.

Guards the check against re-entry, and leaves URLs untouched until the current user is known.

Verification

• Safe-mode PHPUnit group: passes with the change, and fails in both directions without it.
• Full PHPUnit suite: 348 tests, 795 assertions, no failures.
• npm run lint:php: clean.
• Checked against a WooCommerce install: safe mode engages for an administrator and stays out of anonymous requests.

[sheabunge]

Do we need to actually check the user here? I feel like we can just pass on the var whenever it's set, and then only check the current user when evaluating snippets.

[TallblokeUK]

You're right that it works, and it is simpler. I tried it: with the capability check taken out of this callback, an anonymous request carrying the query var comes back 200 and the recursion is gone without needing the guard at all. It also drops a current_user_can() call that currently runs on every home_url() in the page, which is worth having.

One consequence to weigh first. home_url() is a base that callers append to, so adding the query var to it corrupts anything built by concatenation. Passing the var on unconditionally does that for every visitor:

http://example.test?snippets-safe-mode=1%2Findex.php&rest_route=/

That is the REST URL on plain permalinks, and it is wrong.

Worth saying that this is not caused by either version — it already happens for a user who does hold the capability, on core-beta as it stands:

http://example.test?snippets-safe-mode=1%2Findex.php&rest_route=/code-snippets/v1/snippets

So safe mode breaks REST URLs for administrators today on plain permalinks, which matters given the manage screen needs the REST API and safe mode is the route in when something is broken. I'll raise that separately. Taking the capability check out here widens it from administrators to everyone, which is the only reason I'd hesitate.

A third option, if you like it: take the simplification and stop filtering home_url as well, keeping only admin_url. Safe mode navigation is an admin concern, and rest_url() derives from home_url(), so that removes the malformed URLs rather than extending them. No more code than your suggestion.

Happy to go whichever way you prefer — it's your integration.

[lightbulbman]

Going with the third option from the thread above. Pushed to this branch:

• home_url is no longer filtered; only admin_url carries snippets-safe-mode. Site URLs, REST URLs among them, are left intact.
• add_safe_mode_query_var() adds the var whenever the request has it, with no capability check, so building a URL never resolves the current user. The capability is still checked in is_safe_mode_requested(), where snippets are evaluated.
• The re-entry guard in is_safe_mode_requested() and the set_current_user check in the URL callback are removed, as neither is needed once the callback no longer checks the user.
• Tests: the stand-in plugin on determine_current_user now builds an admin URL as well as a home URL, and the user is still resolved once. Two tests replace the capability-based URL tests: site and REST URLs never carry the var, and admin URLs carry it whatever the user.
• Changelog entry extended to cover the REST URLs. Merged core-beta, resolving the CHANGELOG.md conflict.

Verification: full PHPUnit suite passes (473 tests, 36 skipped), the safe-mode group also passes with WP_MULTISITE=1, lint:php is clean, and code-snippets-quicknav-admin-bar.spec.ts passes in both projects.

Note for #596, which changes the same file: with home_url unfiltered, REST URLs are no longer malformed in safe mode, so its rest_url handling should not be needed. Its other part is still wanted: the admin scripts adding the flag to their own REST requests, so a deactivate or delete request also runs in safe mode.