WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#52458 reviewing defect (bug)

WordPress version check is passing "false" value to "esc_url" causing errors (in the logs)

Reported by: jipmoors Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 5.3
Component: Upgrade/Install Keywords: has-patch reporter-feedback
Focuses: Cc:

Description

At least one code-path that leads to this situation is via the following line: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/update.php#L197

The response of WordPress.org for core does not return a value for all "package" entries. Thus passing false to the esc_url array_map.

Array
(
    [full] => https://downloads.wordpress.org/release/wordpress-5.6.1.zip
    [no_content] => https://downloads.wordpress.org/release/wordpress-5.6.1-no-content.zip
    [new_bundled] => https://downloads.wordpress.org/release/wordpress-5.6.1-new-bundled.zip
    [partial] =>
    [rollback] =>
)

In the esc_url function, the value is checked for an empty string, but no type safety is applied.

The subsequent logic calls ltrim which expects the input to be a string.
If this is not the case, there will be an error thrown about this.

This has been added on July 1st, 2019.
See: https://github.com/WordPress/wordpress-develop/commit/78e096fe98531d0799c42705d1329f808e9ee944

Attachments (2)

esc_url_type_safety.patch (425 bytes) - added by jipmoors 2 months ago.
Add a type check before using ltrim
52458.diff (622 bytes) - added by SergeyBiryukov 2 months ago.

Download all attachments as: .zip

Change History (10)

@jipmoors
2 months ago

Add a type check before using ltrim

#1 @SergeyBiryukov
2 months ago

  • Component changed from General to Upgrade/Install
  • Milestone changed from Awaiting Review to 5.7
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#2 @SergeyBiryukov
2 months ago

  • Keywords reporter-feedback added

Thanks for the patch! Linking the original changeset here for reference: [45578] / #36369.

Could you share the exact error message you're getting? I could not reproduce the issue on a clean install:

  • Passing a false or null value to ltrim() does in fact seem to work as expected on all PHP versions, including PHP 8: https://3v4l.org/XtVCn
  • The only issue I see is when passing an array() to ltrim(): https://3v4l.org/lXd4s. This causes a warning on PHP 7 and a fatal error on PHP 8.
  • Or, passing a non-array value to array_map(): https://3v4l.org/IOuSW. This also causes a warning on PHP 7 and a fatal error on PHP 8.

Passing a non-string value to esc_url() is a developer error, so the developer should be notified of that. Patching the function itself to hide the warning would just pass the issue further. The function is documented to receive and return a string, so returning something else instead might have unintended side effects elsewhere. I think the current behavior is appropriate.

Passing a non-array value to array_map() in wp_version_check() seems like something we could protect against, see 52458.diff, though I could not reproduce that scenario in my testing either, so it does not seem to occur often. For that to happen, the WP.org API would have to return a non-empty offers array with an empty packages key.

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


2 months ago

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


2 months ago

#5 @lukecarbis
2 months ago

@jipmoors Is this bug specific to the 5.7? Would it be possible to punt this ticket to 5.8, as the 5.7 beta period is almost lapsed.

#6 @jipmoors
2 months ago

@lukecarbis not specific to 5.7, as the source indicates, the code has been touched in the summer of last year.

@SergeyBiryukov I'm going to dive some deeper in the specific environment I've encountered this in. Quite possibly some plugin that is causing the specific problems.

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


2 months ago

#8 @hellofromTonya
2 months ago

  • Milestone changed from 5.7 to Future Release

With 5.7 RC1 landing tomorrow, punting this ticket to Future Release.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Note: See TracTickets for help on using tickets.