The wp_update_https_detection_errors function may fail to update option values.

[tmatsuur]

In a development environment that does not support HTTPS, the "Update your site to use HTTPS" button was displayed in Site Health, so I investigated the cause.

OS: Windows 10(Japanese)
PHP: 7.2.3
Web: Apache 2.4

The reason for this was that the return value of the wp_is_https_supported function was true.

The wp_is_https_supported function updates the value of "https_detection_errors", but in this case the add_option function fails to update the wp_options table.

update_option( 'https_detection_errors', $support_errors->errors );

The reason is that the content of "$support_errors->errors" obtained by the wp_update_https_detection_errors function was garbled.

array(1) {
  ["http_request_failed"]=>
  array(1) {
    [0]=>
    string(68) "�Ώۂ̃R���s���[�^�[�ɂ���ċ��ۂ��ꂽ���߁A�ڑ��ł��܂���ł����B
"
  }
}

The garbled text looks like this when encoded in UTF-8.

string(101) "対象のコンピューターによって拒否されたため、接続できませんでした。
"

It's a simple change, but you can update the table by making the following changes:

update_option( 'https_detection_errors', map_deep( $support_errors->errors, 'sanitize_text_field' ) );

I think the content of "$support_errors->errors" needs to be sanitized or the character encoding needs to be converted to UTF-8.

[SergeyBiryukov]

Thanks for the report! Moving to 5.7 for visibility, as this is new in [49904] / #47577.

[lukecarbis]

@SergeyBiryukov This seems likely to be caused the Site Health change slated for 5.7 [49904], so it would be good to sort it out at some point during the next week before we reach a release candidate. Could you please take a look, or nominate an owner who might be well positioned to figure this out?

[tmatsuur]

Regarding this problem, I created a patch to improve exception handling in the request method of the WP_Http class.

[SergeyBiryukov]

Thanks for the patch!

• I think the encoding conversion should be in the wp_update_https_detection_errors() function rather than in WP_Http::request(), as there is no way to know in what other contexts the error message might be used.
• This would need a check if the mb_detect_order() and mb_convert_encoding() functions exist, as the mbstring extension could be disabled.
• The message would need to be converted to the site encoding determined by get_option( 'blog_charset' ), which might not necessarily be UTF-8.
• It looks like the message is not displayed anywhere in the UI, so perhaps it should not be stored in the database at all.

Since this needs more investigation, I'm moving the ticket to 5.7.1 for now.

[tmatsuur]

Thanks @SergeyBiryukov

I understand those concerns.

It is also an environment-dependent issue, and it is understandable that it will take time.

[flixos90]

@SergeyBiryukov I agree there is some extra complexity here, but I think we should at least add a partial fix to the problem before releasing 5.7 since showing the direct HTTPS update CTA when it's not actually supported would be detrimental; users would falsely be assured that they can safely switch.

I suggest we come up with a simpler solution and prioritize this again for the 5.7 release, as IMO due to the above concern it's critical enough.

• Either go with the simpler solution outlined in the ticket description of using sanitize_text_field() on the error messages.
• Or check if the update_option call failed, and re-attempt with a static message like "HTTP request failed.".
• Or, since we don't use the real message anyway, simply always use "HTTP request failed." instead of the exception message.

This patch changes the logic in update_https_detection_errors() to never store error messages from the actual request since they could be UTF-8 encoded, which would make storing them in an option potentially fail, leading WordPress to then falsely assume that HTTPS is supported.

While this doesn't actually fix the encoding issue, it is not crucial to do so anyway, since these messages are not used anywhere. A simple differentiation between whether the overall HTTPS request or only the SSL verification failed should be sufficient.

Trac ticket: https://core.trac.wordpress.org/ticket/52484

[flixos90]

@SergeyBiryukov Let me know what you think about the PR; I think it's worth fixing this for 5.7.

[SergeyBiryukov]

Replying to flixos90:

Or, since we don't use the real message anyway, simply always use "HTTP request failed." instead of the exception message.

Makes sense to me, let's go with this for 5.7. Thanks for the PR!

We could add more information to this message later, though perhaps that would be something for Site Health and not for this particular function.

Marking as i18n-change due to two new strings.

[SergeyBiryukov]

In 50471:

Security, Site Health: Do not store HTTPS request error messages in an option.

This changes the logic in update_https_detection_errors() to never store error messages from the actual request since they could use a different encoding, which would make storing them in an option potentially fail, leading WordPress to then falsely assume that HTTPS is supported.

While this doesn't actually fix the encoding issue, it is not crucial to do so anyway, since these messages are not used anywhere. A simple differentiation between whether the overall HTTPS request or only the SSL verification failed should be sufficient for the purpose of this function.

Props flixos90, tmatsuur, lukecarbis.
Fixes #52484.

[SergeyBiryukov]

Reopening for backporting to the 5.7 branch.

[SergeyBiryukov]

In 50472:

Security, Site Health: Do not store HTTPS request error messages in an option.

This changes the logic in update_https_detection_errors() to never store error messages from the actual request since they could use a different encoding, which would make storing them in an option potentially fail, leading WordPress to then falsely assume that HTTPS is supported.

While this doesn't actually fix the encoding issue, it is not crucial to do so anyway, since these messages are not used anywhere. A simple differentiation between whether the overall HTTPS request or only the SSL verification failed should be sufficient for the purpose of this function.

Props flixos90, tmatsuur, lukecarbis.
Reviewed by flixos90, SergeyBiryukov.
Merges [50471] to the 5.7 branch.
Fixes #52484.

[tmatsuur]

Thanks @flixos90 @hellofromTonya @lukecarbis @SergeyBiryukov .

I updated to RC2 and confirmed that the "Site Health" display changed.

The value of the option "https_detection_errors" was as follows.

a:1:{s:20:"https_request_failed";a:1:{i:0;s:21:"HTTPS request failed.";}}

I'm glad that the problem of "site health" has been improved.