Skip to content

fix: fixed health-check endpoint issue on sub-directory multisite#244

Open
Kallyan01 wants to merge 13 commits into
mainfrom
fix/multisite-connection-helath-check
Open

fix: fixed health-check endpoint issue on sub-directory multisite#244
Kallyan01 wants to merge 13 commits into
mainfrom
fix/multisite-connection-helath-check

Conversation

@Kallyan01

@Kallyan01 Kallyan01 commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

What

Brand sub-sites cannot be added on the governing site in multisite setup,

Why

This issue was happening because of the same-origin setup in the subdirectory multisite.

Related Issue(s):

How

AI Disclosure

Testing Instructions

  1. Set up a subdirectory-based multisite installation.
  2. Network-activate the plugin.
  3. Navigate to the governing site.
  4. Attempt to add a new brand site.

Screenshots

Additional Info

Checklist

  • I have read the Contribution Guidelines.
  • I have read the Development Guidelines.
  • I have added necessary tests to cover my changes.
  • I have updated the project documentation as needed.
  • My code has detailed inline documentation.
  • My code is tested to the best of my abilities.
  • My code passes all lints, tests, and checks.
Open WordPress Playground Preview

@Kallyan01 Kallyan01 marked this pull request as ready for review June 15, 2026 15:46
@Kallyan01 Kallyan01 requested review from justlevine and removed request for justlevine June 15, 2026 15:46
@Kallyan01 Kallyan01 marked this pull request as draft June 16, 2026 05:40
@Kallyan01 Kallyan01 marked this pull request as ready for review June 16, 2026 06:37
@Kallyan01 Kallyan01 requested a review from justlevine June 16, 2026 06:37
@codecov-commenter

codecov-commenter commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 53.12500% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.04%. Comparing base (a669ef9) to head (5d4b3fb).

Files with missing lines Patch % Lines
inc/Modules/Rest/Abstract_REST_Controller.php 50.00% 15 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #244      +/-   ##
============================================
- Coverage     86.46%   86.04%   -0.43%     
- Complexity      543      552       +9     
============================================
  Files            22       22              
  Lines          1988     1999      +11     
============================================
+ Hits           1719     1720       +1     
- Misses          269      279      +10     
Flag Coverage Δ
unit 86.04% <53.12%> (-0.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
inc/Modules/Core/Rest.php 100.00% <100.00%> (ø)
inc/Modules/Rest/Abstract_REST_Controller.php 59.25% <50.00%> (-13.47%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justlevine justlevine left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing for me:

Image

Steps:

  1. create a .wp-env.override.json and add "multisite": true
  2. Run npm run wp-env destroy && npm run wp-env start (note this will wipe the env db)
  3. Go to network admin, create the child site
  4. Go to the child site admin, activate the plugins, and set as brand site
  5. Try to connect.

Comment thread assets/src/components/SiteModal.tsx Outdated
Comment on lines +120 to +122
// Explicit site URL so the brand site can identify the governing site
// when both share the same domain (sub-directory multisite), where the
// browser omits Origin for same-origin requests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary. Readers can assume that anything in headers is required for our auth, the implementation details exist elsewhere.

Suggested change
// Explicit site URL so the brand site can identify the governing site
// when both share the same domain (sub-directory multisite), where the
// browser omits Origin for same-origin requests.

Comment thread inc/Modules/Core/Rest.php
public function allowed_cors_headers( $headers ): array {
// Skip if the headers are already present.
if ( in_array( 'X-OneSearch-Token', $headers, true ) ) {
if ( in_array( 'X-OneSearch-Token', $headers, true ) && in_array( 'X-OneSearch-Site-URL', $headers, true ) ) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption here is that either both headers are present or neither.

IMO that's fine just commenting for future reference ✔️

@justlevine

Copy link
Copy Markdown
Collaborator

Closes https://github.com/rtCamp/OnePress/issues/81 - 1st issue

PS, fixes and closes are keywords to autoclose issues, so if it's not meant to fully close it, use Part of or whatever. I updated the PR description here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts OneSearch’s cross-site REST authentication to better support sub-directory multisite setups, particularly where browser Origin behavior can break same-host detection during health checks.

Changes:

  • Adds X-OneSearch-Site-URL as an allowed REST CORS header and sends it from the SiteModal health-check request.
  • Reorders REST permission checks to prioritize token-based auth, with a fallback to the new site URL header when Origin is absent.
  • Updates/rests PHP unit coverage around the expanded CORS header list.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
inc/Modules/Rest/Abstract_REST_Controller.php Adjusts permission logic to prefer token auth and adds X-OneSearch-Site-URL fallback when Origin is missing.
inc/Modules/Core/Rest.php Adds X-OneSearch-Site-URL to the allowed CORS headers list.
assets/src/components/SiteModal.tsx Sends X-OneSearch-Site-URL on the brand-site health-check request.
tests/php/Unit/Modules/Core/RestTest.php Updates unit expectations for the expanded CORS header list.
Comments suppressed due to low confidence (1)

inc/Modules/Rest/Abstract_REST_Controller.php:69

  • wp_parse_url() can return false for an empty/invalid Origin header; the current code then reads $parsed_origin['scheme'/'host'/'port'], which can raise PHP warnings (and $origin_port becomes unreliable). Coerce the parse result to an array before indexing, and only compare ports when one was actually provided.
		$request_origin = $request->get_header( 'origin' );
		$request_origin = ! empty( $request_origin ) ? esc_url_raw( wp_unslash( $request_origin ) ) : '';
		$parsed_origin  = wp_parse_url( $request_origin );
		$request_url    = ! empty( $parsed_origin['scheme'] ) && ! empty( $parsed_origin['host'] ) ? sprintf(
			'%s://%s%s',

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread inc/Modules/Core/Rest.php
Comment on lines +91 to +100
$request_origin = esc_url_raw( wp_unslash( $site_url_header ) );
$parsed_origin = wp_parse_url( $request_origin );
$request_url = ! empty( $parsed_origin['scheme'] ) && ! empty( $parsed_origin['host'] ) ? sprintf(
'%s://%s%s',
$parsed_origin['scheme'],
$parsed_origin['host'],
isset( $parsed_origin['port'] ) ? ':' . $parsed_origin['port'] : ''
) : '';
$origin_port = $parsed_origin['port'] ?? 80;
}
Comment thread tests/php/Unit/Modules/Core/RestTest.php
@Kallyan01

Copy link
Copy Markdown
Collaborator Author

@justlevine, I think the issue is related to the build. Could you please build the JS assets? I completed the setup and ran npm run build: prod, it worked successfully on my end.

Screen.Recording.2026-06-18.at.20.29.47.mov

@justlevine

Copy link
Copy Markdown
Collaborator

Could you please build the JS assets?

Apologies, I guess I was working too quickly. After rebuilding I still have an error, it's just on the next screen:
image

Throwing an error_log() at the relevant spot in Governing_Data_Controller::get_all_brand_post_types() shows it isn't routing to the child-rest endpoint.

image

dependabot Bot and others added 9 commits June 23, 2026 15:46
Bumps [form-data](https://github.com/form-data/form-data) from 4.0.5 to 4.0.6.
- [Changelog](https://github.com/form-data/form-data/blob/master/CHANGELOG.md)
- [Commits](form-data/form-data@v4.0.5...v4.0.6)

---
updated-dependencies:
- dependency-name: form-data
  dependency-version: 4.0.6
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* tests: rename PHP `Unit` to `Integration`

* chore: formatting

* test: fix PHPUnit suite name

* chore: dir rename
#252)

Bumps the github-actions-updates group with 1 update: [actions/checkout](https://github.com/actions/checkout).


Updates `actions/checkout` from 6.0.3 to 7.0.0
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@df4cb1c...9c091bb)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-version: 7.0.0
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions-updates
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [wp-phpunit/wp-phpunit](https://github.com/wp-phpunit/wp-phpunit) from 6.9.4 to 7.0.0.
- [Commits](wp-phpunit/wp-phpunit@6.9.4...7.0.0)

---
updated-dependencies:
- dependency-name: wp-phpunit/wp-phpunit
  dependency-version: 7.0.0
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…y with 3 updates (#255)

Bumps the npm-dev-minor-patch group with 3 updates in the / directory: [@playwright/test](https://github.com/microsoft/playwright), [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) and [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser).


Updates `@playwright/test` from 1.60.0 to 1.61.0
- [Release notes](https://github.com/microsoft/playwright/releases)
- [Commits](microsoft/playwright@v1.60.0...v1.61.0)

Updates `@typescript-eslint/eslint-plugin` from 8.61.0 to 8.61.1
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.61.1/packages/eslint-plugin)

Updates `@typescript-eslint/parser` from 8.61.0 to 8.61.1
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.61.1/packages/parser)

---
updated-dependencies:
- dependency-name: "@playwright/test"
  dependency-version: 1.61.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: npm-dev-minor-patch
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-version: 8.61.1
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-dev-minor-patch
- dependency-name: "@typescript-eslint/parser"
  dependency-version: 8.61.1
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-dev-minor-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Kallyan01 Kallyan01 requested a review from justlevine June 23, 2026 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants