Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#52458 reviewing defect (bug)

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

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


At least one code-path that leads to this situation is via the following line:

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

    [full] =>
    [no_content] =>
    [new_bundled] =>
    [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.

Attachments (3)

esc_url_type_safety.patch (425 bytes) - added by jipmoors 3 years ago.
Add a type check before using ltrim
52458.diff (622 bytes) - added by SergeyBiryukov 3 years ago.
52458.test.diff (1.1 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (12)

3 years ago

Add a type check before using ltrim

#1 @SergeyBiryukov
3 years 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

3 years ago

#2 @SergeyBiryukov
3 years 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:
  • The only issue I see is when passing an array() to ltrim(): This causes a warning on PHP 7 and a fatal error on PHP 8.
  • Or, passing a non-array value to array_map(): 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 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.

3 years ago

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

3 years ago

#5 @lukecarbis
3 years 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
3 years 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.

3 years ago

#8 @hellofromTonya
3 years 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.

#9 @SergeyBiryukov
3 years ago

52458.test.diff is an initial test for the current behavior of esc_url() with non-string values.

Note: See TracTickets for help on using tickets.