Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#58563 closed defect (bug) (fixed)

Undefined variable $checkout in class-wp-automatic-updater.php line 175

Reported by: jqz's profile jqz Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: Upgrade/Install Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Fresh install on Windows with open_basedir in effect to limit filesystem access beyond the site itself.

See https://wordpress.org/support/topic/undefined-variable-checkout

If is_allowed_dir() doesn't get any true responses, $checkout never gets initialized, and this error occurs.

Change History (10)

This ticket was mentioned in PR #4630 on WordPress/wordpress-develop by @costdev.


10 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 follow-up: @costdev
10 months ago

  • Focuses coding-standards removed
  • Keywords reporter-feedback needs-testing-info needs-testing added
  • Milestone changed from Awaiting Review to 6.3
  • Version changed from 6.2.2 to 6.2

Hi @jqz, thanks for opening this ticket!

Looks like there's two issues to resolve here:

  1. That the $checkout variable isn't initialized and, as the loop's early exit can prevent assignment to $checkout, this can be undefined by the time it reaches the return statement.
  2. That WP_Automatic_Updater::is_allowed_dir() can return false for all directories on Windows, including ABSPATH.
    • This needs testing and some testing info.

@jqz Can you provide your open_basedir directive (anonymized if necessary), and the value of the ABSPATH constant? Thanks!


  • This was introduced in [55425], setting Version to 6.2.
  • Milestoning for 6.3 for visibility.

@audrasjb @SergeyBiryukov Should this be milestoned for 6.2.3, or are we unlikely to land that before 6.3 is released?

#3 @audrasjb
10 months ago

Let's keep it in milestone 6.3 until it's committed. Then we can milestone it to 6.2.3 with fixed-major workflow keyword and eventually – if 6.2.3 is confirmed – we can plan to backport it :)

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


10 months ago

#5 in reply to: ↑ 2 ; follow-up: @jqz
10 months ago

  • Keywords reporter-feedback removed

Replying to costdev:

@jqz Can you provide your open_basedir directive (anonymized if necessary), and the value of the ABSPATH constant? Thanks!

This are the relevant settings:

  • ABSPATH: D:\webs\localhost\example.co.uk\sites\ofs3\wordpress/
  • WP_CONTENT_DIR: D:\webs\localhost\example.co.uk\sites\ofs3/content
  • DOCUMENT_ROOT: D:/webs/localhost/example.co.uk
  • open_basedir: D:/webs/localhost/example.co.uk/sites/ofs3/;C:\WINDOWS\TEMP/;d:/webs/localhost/
  1. That WP_Automatic_Updater::is_allowed_dir() can return false for all directories on Windows, including ABSPATH.

Looks like WP_Automatic_Updater::is_allowed_dir() is not accounting for case insensitivity of Windows filenames, nor that the path separator can be either \ or /.

#6 in reply to: ↑ 5 @jqz
10 months ago

Replying to jqz:

This are the relevant settings:

  • ABSPATH: D:\webs\localhost\example.co.uk\sites\ofs3\wordpress/
  • WP_CONTENT_DIR: D:\webs\localhost\example.co.uk\sites\ofs3/content
  • open_basedir: D:/webs/localhost/example.co.uk/sites/ofs3/;C:\WINDOWS\TEMP/;d:/webs/localhost/

$check_dirs contains the following (which have different directory separators and casedness of drive letter):

  • D:\webs\localhost\example.co.uk\sites\ofs3/content/plugins
  • D:\webs\localhost\example.co.uk\sites\ofs3/content
  • D:\webs\localhost\example.co.uk\sites\ofs3
  • D:\webs\localhost\example.co.uk\sites
  • D:\webs\localhost\example.co.uk
  • D:\webs\localhost
  • D:\webs
  • D:\
  • D:\webs\localhost\example.co.uk\sites\ofs3\wordpress

#7 follow-up: @costdev
10 months ago

  • Keywords dev-feedback added; needs-testing-info needs-testing removed

Thanks a lot @jqz!

@audrasjb I'm thinking that PR 4630 could be committed in 6.3. Since this ticket is specifically about the $checkout variable's definition being dependent on a foreach loop, the PR targets this issue and includes PHPUnit tests, and so this ticket could be closed after the PR is committed.

We could then take the information from @jqz into a separate ticket to focus on investigating and resolving the Windows-specific issue that exposed the undefined $checkout. What do you think?

For now, I'm removing needs-testing and needs-testing-info, as this pertains to the Windows-specific issue, which may be separated into a different ticket.

#8 @SergeyBiryukov
10 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 56124:

Upgrade/Install: Initialize the local $checkout variable in WP_Automatic_Updater::is_vcs_checkout().

This avoids an Undefined variable $checkout PHP warning if all of the directories checked for access are disallowed due to the PHP open_basedir restrictions.

Follow-up to [55425].

Props jqz, costdev, audrasjb.
Fixes #58563.

@SergeyBiryukov commented on PR #4630:


10 months ago
#9

Thanks for the PR! Merged in r56124.

#10 in reply to: ↑ 7 @SergeyBiryukov
10 months ago

  • Keywords dev-feedback removed

Replying to costdev:

We could then take the information from @jqz into a separate ticket to focus on investigating and resolving the Windows-specific issue that exposed the undefined $checkout. What do you think?

Yes, please, let's create a new ticket for that :) Thanks!

Note: See TracTickets for help on using tickets.