You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: cross-site user management and auth hardening - #195
Fixes four defects in cross-site user management: delete requests never reaching brand sites, a fatal error on every brand-site request, an incorrect way of saving passwords and names on user creation, and toast positioning.
Why
Each failed silently or broke a flow outright:
Delete user from sites did nothing — the brand-site lookup missed, the brand site rejected the request, and wp_safe_remote_request() refused to send it.
Brand sites fatalled on every request: Call to undefined function wp_get_current_user().
Create user stored a different password than the admin typed, so the user could never log in;
Toasts landed on the left side, out of view.
Related Issue(s):
AI Disclosure
Self + Opus 5.5 for fix testing & validation
Issues Fixed
Deleting a user from brand sites did nothing; partial failures were hidden.
Security: lookalike hosts passed auth, delete could remove the wrong user, and raw HTTP responses leaked to the browser.
Pairing lost sub-directory paths and failed on same-host/different-port sites.
Toasts were off-screen, overlapped, and cleared all at once.
Behaviour changes
Passwords: minimum 8 characters, no \ (as in core).
Sites now send X-OneAccess-Site-URL; update governing and brand sites together.
Testing Instructions
Setup: a governing site and a brand site on different URLs (or the wp-env pair: npm run wp-env start → 8888, npm run wp-env:child start → 8890).
Pairing & auth
Settings → Add Brand Site with the brand's URL and API key → health-check passes. On the brand, Settings shows the governing URL (for a sub-directory governing site, the full path is kept).
Load the brand site's /wp-admin/ → no fatal error.
From a terminal, send a health-check to the already-paired brand with its key but a different origin → 401, and the brand's governing URL is unchanged: curl -H "X-OneAccess-Token: <brand key>" -H "Origin: https://evil.example" https://<brand>/wp-json/oneaccess/v1/health-check
Create user
4. Manage Users → Create User with password it's a "test" <pw> and a single-word full name → created on the brand. Log in on the brand's wp-login.php with the password exactly as typed.
5. Try a password containing \ → the notice shows "Passwords cannot contain the "" character." (not a generic error). A password under 8 characters is rejected too.
Add to sites / role update (broken on main by #124)
6. Users → User actions → Add to Sites → the user is created on the selected brand site.
7. Users → User actions → Manage Roles → change the role → it changes on the brand site.
Delete
8. Users → User actions → Delete from the brand site → the user is removed there and the row disappears.
9. Partial failure: stop or disconnect one brand site (e.g. change its API key in Settings), then delete a user from two sites → the notice lists the failing site, the other site is deleted, and the list refreshes. In DevTools → Network, the delete-user-from-sites response has no raw HTTP response in error_log.
10. On the brand, create a different user with the same username but another email → deleting the shared user from the governing site leaves that user untouched.
Toasts
11. Trigger one notice, then several at once (e.g. create a user on two sites) → they stack bottom-right without overlapping, and dismissing one doesn't clear the others.
❌ Patch coverage is 2.82486% with 172 lines in your changes missing coverage. Please review.
✅ Project coverage is 6.60%. Comparing base (3ac4aa6) to head (6c87017).
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging into this, @Kallyan01. The direction is right, and the PR fixes more than its description says. I'm requesting changes for a handful of must-fix items; details are in the inline comments.
The rescaffold dropped the plugins_loaded wrapper in oneaccess.php, so Main::instance() runs while the plugin file loads, before pluggable.php. Profile_Request::register_hooks() calls wp_get_current_user().
Create user / add to sites / role update stop working on main
Settings::get_shared_sites() started keying sites by trailingslashit() URL, but every caller looks sites up with untrailingslashit() keys. Create fails with "API key not found" (hidden, because the UI ignored error_log), add-to-sites fails with "Site not found", and role update skips every site while still reporting success.
The dedup table stores site_urlwith a trailing slash (Actions_Controller::sanitize_user_data()), while sanitize_shared_sites() saves brand URLs without one since #12. The direct $oneaccess_sites_info[ $site['site_url'] ] lookup missed, an empty token was sent, and the brand site rejected the request.
Password not stored as typed
Initial commit
sanitize_text_field() on passwords. Intermittent: generate_strong_password() uses wp_generate_password( 32, true, true ), and roughly 30% of those passwords contain <.
wp_safe_remote_request() refusing to send
Not a regression
wp_safe_remote_* has been used since the initial commit; it only rejects private/loopback hosts (e.g. Local's *.local → 127.0.0.1).
So the get_shared_sites() key revert in Settings.php is what restores create, add-to-sites and role update. Please call that out in the description along with the #124 / #12 references.
Must fix before merge
Passwords containing ', " or \ still can't log in on brand sites: slashing mismatch with wp_signon() (inline on create_user()).
User-Agent host check accepts lookalike hosts (inline; agrees with Copilot's open thread).
Delete falls back to sending a request to an unconfigured URL with an empty token (inline).
Delete puts the raw brand HTTP response into error_log (file comment on Governing_Site_Controller.php).
Outside this diff: the delete UI in SharedUsers.tsx (L388–L414) still swallows per-site errors. On partial failure it shows the generic "Failed to delete user.", drops error_log, and skips fetchUsers() even though some sites were deleted. Please mirror what you did in CreateUser.tsx and refresh in finally. This is the headline flow of the PR.
Should fix
See the inline and file comments:
the safe-request filter scope
Origin sent only on delete
lookups that bypass get_shared_site_by_url()
role update reporting success while skipping sites
the duplicated min-length validation
the dedup check that never runs
delete matching users by login before email
toast overlap
Also outside this diff:
Bootstrap:oneaccess.php should defer Main::instance() to plugins_loaded again (see the Profile_Request.php comment).
Peer check:Actions_Controller::brand_site_to_governing_site_permission_check() still uses the full-URL strpos() User-Agent match. The two directions should share one helper.
Endpoint builder:Abstract_REST_Controller::build_api_endpoint() produces //wp-json and is unused. Fix it and use it for every outbound URL.
Tests
The PR template's tests box is unchecked and patch coverage is 0%. E2E only activates and deactivates the plugin, so the #124 fatal (brand sites only) couldn't be caught. Minimum I'd add:
PHPUnit: get_shared_sites() key format, plus get_shared_site_by_url() with and without trailing slash and with http vs https.
PHPUnit: brand /new-users password round-trip — wp_check_password( wp_slash( $typed ), $hash ) for <b>, %41, it's, "quoted" and a leading space.
E2E: set oneaccess_site_type to brand-site and load /wp-admin/.
Merge order
The release PR #132 (2.0.0) currently contains #124, so it shouldn't ship before this lands.
#145 touches SharedUsers.tsx, DB.php, Actions_Controller.php and Governing_Site_Controller.php, so expect conflicts.
Follow-up (non-blocking)
About ten copy-pasted "find site → build URL → set token → request → decode" blocks have drifted apart. That drift is why a single key change broke three flows.
A small Site_Client would kill this class of bug. It would resolve the site by URL, build the endpoint, set the token and headers, send the request, and decode the reply into { ok, code, message }. Pair it with one shared peer-verification helper.
The reason will be displayed to describe this comment to others. Learn more.
Agree with Copilot's open thread here. A host substring in the User-Agent also matches lookalike hosts: for example.com, WordPress/6.9; https://example.com.attacker.test passes.
WordPress sends WordPress/<version>; <home_url>, so we can parse that URL and reuse the exact host comparison:
This keeps the scheme-agnostic behaviour you were after, since is_same_domain() compares hosts only.
Two related notes, both outside the diff:
Case:is_same_domain() compares hosts case-sensitively (L148); strtolower() both sides.
Governing side: the reverse direction, Actions_Controller::brand_site_to_governing_site_permission_check() (L199), still matches the full URL with strpos(). The same check now follows two different rules, so a shared helper would keep them in sync.
The reason will be displayed to describe this comment to others. Learn more.
What do we do in Onesearch?
Seems like we should just align on an single Abstract_REST_Controller implementation, instead of solving the same issue in different ways in each codebase.
The reason will be displayed to describe this comment to others. Learn more.
This makes a global, production-wide change to work around a local-environment problem.
Context
Brand sites on public hosts already pass wp_safe_remote_*(). The filter only matters when a configured host resolves to a private or loopback IP, e.g. Local's *.local.
The repo already ships tests/_data/plugins/localhost-helper.php for the wp-env case.
As a global filter, it also relaxes the check for any other plugin's (or core's) safe requests to these hosts.
If private-network deployments need to be supported, I'd suggest one of these:
make it opt-in (a filter or constant), or
scope it to OneAccess's own requests: verify the URL belongs to a configured site, then pass reject_unsafe_urls => false from one shared request helper.
Smaller points:
Ports: it doesn't cover non-standard ports, which http_allowed_safe_ports rejects separately. So it won't help wp-env-style localhost:8889 setups.
Placement: the class docblock describes consumer-site hooks, so this would fit better next to the HTTP code.
The reason will be displayed to describe this comment to others. Learn more.
(FWIW I don't get the purpose of this entire class. Assuming the existing coupling is a workaround for send_users_for_deduplication() being built-in to that class instead of the endpoint. But either way yah, seems like this specific diff is solved by localhost-helper.php and doesn't need to exist at all. )
The reason will be displayed to describe this comment to others. Learn more.
The new minimum is fine, but:
Duplicated: the same block is pasted three times (L713, L1065, L1402). A single helper keeps the rules in sync and is a natural place for core's \ rule (see the create_user() comment):
privatestaticfunctionvalidate_password( string$password ): ?\WP_REST_Response {
if ( mb_strlen( $password ) < self::MIN_PASSWORD_LENGTH ) {
returnnew \WP_REST_Response(
[
'success' => false,
/* translators: %d is the minimum number of characters */'message' => sprintf( __( 'Password must be at least %d characters long.', 'oneaccess' ), self::MIN_PASSWORD_LENGTH ),
],
400
);
}
if ( str_contains( $password, '\\' ) ) {
returnnew \WP_REST_Response(
[
'success' => false,
'message' => __( 'Passwords cannot contain the "\\" character.', 'oneaccess' ),
],
400
);
}
returnnull;
}
Bytes vs characters:strlen() counts bytes, while the UI counts characters, so use mb_strlen().
Behaviour change: this is a new server-side policy, so please mention it in the PR description.
Message lost in the UI:CreateUser.tsx L188 never reads the body on ! response.ok, so this 400 shows up as "Failed to create user. Please try again later.". It should show data.message.
The reason will be displayed to describe this comment to others. Learn more.
Replaced Origin with X-OneAccess-Site-URL, which every outbound call to a token-protected route now sends. The check_api_permissions() still has the origin check to handel the in browser requests which by default adds origin in the request header.
The reason will be displayed to describe this comment to others. Learn more.
Good: per-site failures are visible now. Two follow-ups:
Lost server message: L188, the ! response.ok branch, never reads the body. Any 4xx/5xx shows "Failed to create user. Please try again later.", including the new 400 password message. Parse the JSON and prefer data.message.
Empty site name: when the site isn't found, site_name is '', so the notice reads "API key not found for site .". The server could fall back to the URL.
The same per-site treatment is still missing in the delete flow (SharedUsers.tsx), which is this PR's main fix. See the review summary.
The reason will be displayed to describe this comment to others. Learn more.
Dropping :has() simplifies this. But every standalone .components-snackbar is now position: fixed in the same corner as the list, so they overlap rather than stack when more than one is on screen:
CreateUser renders a standalone notice and a SnackbarList.
SharedUsers and ProfileRequests render their own snackbars.
The "several at once — toasts stack" test step only holds within a single SnackbarList.
A more robust approach:
Render one SnackbarList at the app root, fed by the @wordpress/notices store (createSuccessNotice( message, { type: 'snackbar' } )).
The reason will be displayed to describe this comment to others. Learn more.
Delete flow: issues outside the diff (the PR touches this flow, so worth fixing here):
Raw response leak (must fix). L479: $error_log[] = $response; pushes the raw wp_safe_remote_request() result (status, body, cookies) into the JSON returned to the browser. Remove it; the message pushed just above already covers the failure.
Missing decode guard. L484 ! $response_body['success'] runs on a possibly-null decode; reuse the is_array() guard from L1487.
Dead dedup check. L432: $processed_sites is only appended inside the skip branch, never after a site is processed, so the duplicate check never fires. Same at L758 in add_user_to_sites().
Wrong-user deletion risk.delete_user() (L363) looks the user up by login first, then email. A different person with the same username on a brand site would be deleted. Email is the identity everywhere else (dedup table, update_user()), so match by email and verify the login.
Orphaned posts.wp_delete_user( $user->ID, 0 ) (L382) reassigns the user's content to user ID 0. If keeping content is the intent, reassign to a real user, or make it a setting.
Inconsistent error labels. Delete errors show the URL, while create errors show the site name.
The reason will be displayed to describe this comment to others. Learn more.
Role update (update_user_roles_for_sites()): outside the diff, but it was broken on main by the same key change:
Silent skip. L951–L959: an unknown site is skipped with continue and never added to error_log, so the response says success while nothing changed. Report it like add-to-sites does.
Double slash. L953/L963 build https://brand.example/ + /wp-json/…, i.e. //wp-json. It works on most servers, but Abstract_REST_Controller::build_api_endpoint() has the same trailingslashit() . '/wp-json/' bug and isn't called anywhere. Fixing it and using it for every outbound URL would remove this class of issue.
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of the preg_replace() seems unnecessarily heavy for what we need to be doing here. Feel's like (un)trailingslashit( trim( $site_url ) ) should be more than enough, similar to Onesearch\Utils::normalize_url() without the Utils baggage.
The reason will be displayed to describe this comment to others. Learn more.
Review by Claude Opus 5.5
Opus got a lot of the flaws, but obvs doesn't have the context, so I unsurprisingly disagree with some of the conclusions (left my feedback inline on the existing comments).
Tl;dr where we can fix these problems by aligning our code with what rtcamp/onesearch is doing, it's better than shimming them in place and having things diverge even more.
PHPUnit Integration tests are probably the most important thing and the best way to catch issues with the rescaffold and prevent regressions, but doing it haphazardly on this PR doesn't feel like a must-have. The shared classes we can probably just copy/paste from OneSearch, but the others I'll leave it to your disgression as to whether it makes sense to throw them in here or the general backfilling that's required before release 🤷
The implicit port is always treated as 80. A stored https://host therefore fails comparison with the equivalent https://host:443, while an HTTP :80 source can compare equal to that HTTPS URL. Derive the default port from the stored URL's scheme.
This rejects a character that WordPress passwords support. The new wp_slash( $password ) call already ensures an original backslash survives wp_insert_user() unslashing, so this restriction unnecessarily prevents users from creating an account with an otherwise valid password.
wp_create_user() already passes the supplied password through unchanged; pre-slashing it makes quotes part of the stored password (for example, " becomes \"). This still prevents login with the exact password described in the PR. Pass the raw value here.
This issue also appears on line 1541 of the same file.
Defaulting a stored URL with no explicit port to 80 rejects an equivalent HTTPS origin that explicitly supplies :443. Derive the default from the stored URL's scheme so canonical and explicit HTTPS URLs authenticate consistently.
wp_create_user() accepts the raw password and hashes it before database escaping. Applying wp_slash() here changes quotes and backslashes, so the documented password test still creates credentials different from what the admin typed. Pass the unmodified password.
Backslashes are valid WordPress password characters, but this new rule rejects them despite the UI describing only an eight-character minimum and recommending special characters. Preserve the user's password verbatim rather than narrowing WordPress's accepted password set.
wp_create_user() expects the plaintext password and forwards it unchanged to wp_insert_user() for hashing. Pre-slashing here changes quotes (including the password characters called out in the test plan), so the stored hash still will not match what the admin typed. Pass $password directly.
This issue also appears on line 1541 of the same file.
🧠 Review effort: Balanced
Kallyan01
changed the title
fix: user deletion
fix: cross-site user management and auth hardening
Oct 8, 2026
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
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.
up1512001
left a comment
justlevine
left a comment
What
Fixes four defects in cross-site user management: delete requests never reaching brand sites, a fatal error on every brand-site request, an incorrect way of saving passwords and names on user creation, and toast positioning.
Why
Each failed silently or broke a flow outright:
wp_safe_remote_request()refused to send it.Call to undefined function wp_get_current_user().Related Issue(s):
AI Disclosure
Self + Opus 5.5 for fix testing & validation
Issues Fixed
',",<broke login); single-word names warned.Behaviour changes
\(as in core).X-OneAccess-Site-URL; update governing and brand sites together.Testing Instructions
Setup: a governing site and a brand site on different URLs (or the wp-env pair:
npm run wp-env start→ 8888,npm run wp-env:child start→ 8890).Pairing & auth
/wp-admin/→ no fatal error.401, and the brand's governing URL is unchanged:curl -H "X-OneAccess-Token: <brand key>" -H "Origin: https://evil.example" https://<brand>/wp-json/oneaccess/v1/health-checkCreate user
4. Manage Users → Create User with password
it's a "test" <pw>and a single-word full name → created on the brand. Log in on the brand'swp-login.phpwith the password exactly as typed.5. Try a password containing
\→ the notice shows "Passwords cannot contain the "" character." (not a generic error). A password under 8 characters is rejected too.Add to sites / role update (broken on
mainby #124)6. Users → User actions → Add to Sites → the user is created on the selected brand site.
7. Users → User actions → Manage Roles → change the role → it changes on the brand site.
Delete
8. Users → User actions → Delete from the brand site → the user is removed there and the row disappears.
9. Partial failure: stop or disconnect one brand site (e.g. change its API key in Settings), then delete a user from two sites → the notice lists the failing site, the other site is deleted, and the list refreshes. In DevTools → Network, the
delete-user-from-sitesresponse has no raw HTTP response inerror_log.10. On the brand, create a different user with the same username but another email → deleting the shared user from the governing site leaves that user untouched.
Toasts
11. Trigger one notice, then several at once (e.g. create a user on two sites) → they stack bottom-right without overlapping, and dismissing one doesn't clear the others.
Screenshots
Checklist