WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#52484 closed defect (bug) (fixed)

The wp_update_https_detection_errors function may fail to update option values.

Reported by: tmatsuur Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.7
Component: Security Keywords: has-patch has-unit-tests commit dev-reviewed i18n-change
Focuses: Cc:

Description

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.

Attachments (1)

52484.diff (719 bytes) - added by tmatsuur 2 months ago.

Download all attachments as: .zip

Change History (20)

#1 @SergeyBiryukov
2 months ago

  • Component changed from HTTP API to Security
  • Milestone changed from Awaiting Review to 5.7
  • Version set to trunk

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

This ticket was mentioned in Slack in #core by metalandcoffee. View the logs.


2 months ago

#3 @lukecarbis
2 months ago

@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?

#4 @tmatsuur
2 months ago

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

@tmatsuur
2 months ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 months ago

#6 @hellofromTonya
2 months ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 months ago

#8 follow-up: @SergeyBiryukov
2 months ago

  • Milestone changed from 5.7 to 5.7.1

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.

#9 in reply to: ↑ 8 @tmatsuur
2 months ago

Thanks @SergeyBiryukov

I understand those concerns.

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

#10 follow-up: @flixos90
7 weeks ago

@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 ticket was mentioned in PR #1062 on WordPress/wordpress-develop by felixarntz.


7 weeks ago

  • Keywords has-unit-tests added

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

#12 @flixos90
7 weeks ago

  • Keywords dev-feedback added
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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

#13 in reply to: ↑ 10 @SergeyBiryukov
7 weeks ago

  • Keywords dev-reviewed i18n-change added; dev-feedback removed
  • Milestone changed from 5.7.1 to 5.7

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 weeks ago

#16 @SergeyBiryukov
7 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#17 @SergeyBiryukov
7 weeks ago

  • Keywords commit added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.7 branch.

#18 @SergeyBiryukov
7 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#19 @tmatsuur
7 weeks ago

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.

Note: See TracTickets for help on using tickets.