mb_check_encoding use can raise a fatal error in some environments

[aaemnnosttv]

Bug Description

In #5868 we implemented a fix which resolves redirect issues on sites using internationalized domain names (IDN). This involves the use of a core PHP multibyte string function which may not exist on some environments. While the mbstring module is commonly enabled, it is not enabled by default so we cannot rely on it to be generally available.

WordPress polyfills a few mb_* functions, (e.g. mb_substr, and mb_strlen) but not all of them.

Steps to reproduce

• Reproducing this requires the mbstring PHP module to be disabled
• Various actions could raise this error as the hooked function runs during wp_validate_redirect which itself is called by various functions, notably wp_safe_redirect()
• E.g. disconnect your user in Site Kit and navigate to the splash page
• Change the googlesitekit-splash in the URL to googlesitekit-dashboard and then attempt to navigate to that URL to invoke the redirect
• See error

Support topics

• https://wordpress.org/support/topic/fatal-errors-in-the-wordpress-backend-after-latest-site-kit-update/

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Site Kit should not raise fatal errors in environments where the mbstring PHP module is not enabled
  • More specifically, Site Kit should not rely on any mb_* function to be provided by PHP
• The behavior introduced by Incorrect redirects to WP Dashboard for sites with Internationalised Domain Names #5868 should be preserved

Implementation Brief

• Remove the allowed_redirect_hosts filter callback from Plugin
• Reimplement the existing behavior from the Authentication::allowed_redirect_hosts method
  • Parse the site's hostname with URL::parse instead of (wp_)parse_url functions can have problems with multibyte hostnames (see Permuting site URLs can fail for some domains and environments #4776)
    • Note: we probably want to use admin_url as the URL to parse here rather than the home_url for accuracy. The hosts should be the same because SK does not support them being different but this is more accurate because SK only does redirects within WP admin
  • Replace the use of mb_check_encoding with the same is_ascii check we use in Module::permute_site_hosts

Test Coverage

• Add test coverage for the behavior in AuthenticationTest perhaps by testing a number of redirects using wp_safe_redirect with ascii + unicode domains and ensure the destination location is as expected (see RedirectException in our tests for examples)

QA Brief

• Follow the steps to reproduce above, to ensure the error no longer happens
• This issue modifies how the changes in Incorrect redirects to WP Dashboard for sites with Internationalised Domain Names #5868 were implemented but it should continue to work the same as before in a way that is safe for environments that don't have the mbstring module/extension enabled

Changelog entry

• Fix potential errors raised when the mbstring PHP extension is not loaded.

[aaemnnosttv]

One solution here would be to include the symfony/polyfill-mbstring package (v1.19 supports down to PHP 5.3) although if we can replace this with one of our other methods that don't rely on this, that may be better.

[felixarntz]

IB ✅

[jamesozzie]

Just an update on the impacted site, reported via the WordPress support forums:

• https://wordpress.org/support/topic/error-on-debug-pages/ - Open - Awaiting user response - SH info
• https://wordpress.org/support/topic/site-stopped-working-after-new-update-of-sitekit/ - User Resolved - User rolled back version - SH info
• https://wordpress.org/support/topic/fatal-errors-in-the-wordpress-backend-after-latest-site-kit-update - sh info
• https://wordpress.org/support/topic/_googlesitekituserdata-2/#new-topic-0 - Closed enabled mbstring - SH info
• https://wordpress.org/support/topic/fatal-errors-in-the-wordpress-backend-after-latest-site-kit-update/ - Open - Awaiting user response - SH info
• https://wordpress.org/support/topic/plugin-causing-error-7/ - Open - Awaiting user response - SH info
• https://wordpress.org/support/topic/_googlesitekituserdata-2/#post-16437254 - Open - Awaiting user response - SH info

@aaemnnosttv For those who have the ability to configure PHP modules, would enabling mbstring be something worth checking?

[aaemnnosttv]

@aaemnnosttv For those who have the ability to configure PHP modules, would enabling mbstring be something worth checking?

@jamesozzie sure, that would fix the issue too, although I'm guessing most users wouldn't have that option.

[jamesozzie]

Good to know, thanks @aaemnnosttv.

[jaschawilcox]

@aaemnnosttv For those who have the ability to configure PHP modules, would enabling mbstring be something worth checking?

@jamesozzie sure, that would fix the issue too, although I'm guessing most users wouldn't have that option.

Confirming that installing php-mbstring in my Ubuntu 20.04 environment instantly resolved this issue.

sudo apt install php-mbstring

[bamnet]

Got paged my blog was down, presumably after a plugin auto update, and found this error. If you can't install mbstring quickly, you can bring your site back online by hacking in wp-content/plugins/google-site-kit/includes/Plugin.php

Look for mb_check_encoding and short-circuit it. I did this:

if ( true || mb_check_encoding( $wpp['host'], 'ASCII' ) ) {

[wpdarren]

QA Update: ⚠️

@aaemnnosttv I am trying to recreate the issue with the site you gave me access to. When I change the URL to wp-admin/admin.php?page=googlesitekit-dashboard and then navigate to that URL, I am redirected to wp-admin/admin.php?page=googlesitekit-splash and no error appears in the console or on screen. This is on the latest release (1.93.0)

I would ideally like to recreate it so I can verify that the issue is resolved.

I am assuming that the mbstring PHP module is disabled on this site?

c.c. @tofumatt

[aaemnnosttv]

@wpdarren it is disabled but I didn't realize the polyfill plugin was still active which fills the gap there. I've since disabled it so this should be ready to test again 👍

[wpdarren]

QA Update: ✅

Verified:

• I was able to recreate the issue when changing the googlesitekit-splash in the URL to googlesitekit-dashboard and then attempting to navigate to that URL.
• When I switched to the main branch, and ran through the same steps, no error appeared. The URL redirected to googlesitekit-splash successfully. I also checked console and no new warnings or errors appeared.
• Used a site with IDNs.
  • When setting up Site Kit without Analytics, I was redirected to Site Kit dashboard.
  • When setting up Site Kit with an Analytics account for the domain, I was redirected to the Site Kit dashboard.
  • When setting up AdSense, Tag Manager and Optimize, I was redirected to the Site Kit dashboard.

Related to the IDN. There were a few bugs discovered during testing #5868 and these are highlighted in #6435.

Screenshots