WordPress tries to access.bzr or .git without checking open_basedir restrictions

[meyegui]

Hi,

I'm getting the following error in my logs. I'm aware that I have open_basedir enabled, but I don't think WordPress should try to read files outside its installation directory. If I'm mistaken, I'm sorry and I'll be glad to receive any explanation as to why.

This bug doesn't generate any visible error or message other than this log, so I would definitely consider it "low severity". The log file was generated by a plugin of mine, but as you can see, the error doesn't occur in my own files. It's located in a core file.

Here's what I get in my log file:

[14:35:04]	
******************
PHP SHUTDOWN ERROR

Type: 2
Message: is_dir(): open_basedir restriction in effect. File(/home/.bzr) is not within the allowed path(s): (/home/httpd/vhosts/[hidden]/:/tmp/)
File: /home/httpd/vhosts/[hidden]/subdomains/[hidden]/wp-admin/includes/class-wp-automatic-updater.php:98
******************

Best regards

[meyegui]

Still happening in WordPress 4.9.2.

Any idea why WP would try to read that file?

[dd32]

Hi @meyegui and welcome to Trac.

I'd like to apologise for this ticket not getting a response until now.

This is caused by WordPress checking to see if it's running within a version-controlled environment, and avoiding autoupdating if that's the case.
The code responsible for this is located here: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-automatic-updater.php?marks=74-106#L58

WordPress doesn't take into consideration the PHP open_basedir setting, which causes it to process further up the path list than expected.

#35536 is a related ticket, where it processes up into completely invalid directories.

If you're curious as to why we care about a .git or .bzr file in a /home/username/ folder, it's because we decided to be ultra-conservative and check all the way up to / instead of just the parent directory of WordPress for if it's running within a VCS environment.
We could probably relax this restriction to checking the immediate parent of WordPress only, but that wouldn't take into account some edge-cases of deployment situations, where for example, the VCS files are in the grandparent instead - Like I said, this code was written extremely conservatively.

Fixing this, adding a check to ensure that it's not going to run into an open_basedir restriction would be good, although maybe we can look at relaxing this restriction in the first place at the same time.

[markjaquith]

Check open_basedir before checking a directory's existence

[markjaquith]

Added is_allowed_dir() that checks to see if one of of the allowed open_basedir directories starts off the directory to be checked.

[arnolp]

Still happening in 5.8.2

is_dir(): open_basedir restriction in effect. File(/.git) is not within the allowed path(s):

class-wp-automatic-updater.php:104

[SergeyBiryukov]

#56189 was marked as a duplicate.

[SergeyBiryukov]

Some history here, via comment:3:ticket:56189:

• [25700] / #22704 introduced this code.
• [25764] / #25572 added the @ error suppression to is_dir() to address the same issue being reported here.

[SergeyBiryukov]

Refreshed the patch to:

• Fix WPCS issues
• Add missing documentation
• Use the str_starts_with() polyfill introduced in [52040] / #54377

[costdev]

Thanks for the patch @meyegui!

Per the discussion in the bug scrub, I'm adding the needs-unit-tests keyword and I'll work on unit tests for the new function 🙂

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

[costdev]

​PR 3224 attempts to add PHPUnit tests for the new method. However, tests are failing due to issues resetting open_basedir. See ​this comment for things I've tried to get this working. Any other ideas are appreciated 🙂

[audrasjb]

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone as it still needs dev-feedback and testing.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

[costdev]

I've updated ​PR 3224 to remove the problematic tests that tested open_basedir, as this proved seemingly untestable.

This still needs some manual testing by setting the open_basedir PHP directive.

[mukesh27]

This ticket was discussed during the recent bug scrub. It looks like it's unlikely that work will be done on this during the 6.2 cycle.

Thanks @costdev for PR. ping @robinwpdeveloper for testing.

[costdev]

This ticket was discussed in the bug scrub. Pinging @SergeyBiryukov and @audrasjb for a final test/review for commit consideration.

Update: I've added two additional tests to the PR that use @runInSeparateProcess and @preserveGlobalState disabled to allow for testing the open_basedir PHP directive. The docblock for the test methods explains why these annotations are used for these.

[costdev]

Additional props to @afragen and @pbiron for rubber-ducking with me on the open_basedir tests.

[SergeyBiryukov]

In 55425:

Upgrade/Install: Introduce WP_Automatic_Updater::is_allowed_dir() method.

As part of determining whether to perform automatic updates, WordPress checks if it is running within a version-controlled environment, recursively looking up the filesystem to the top of the drive, looking for a Subversion, Git, Mercurial, or Bazaar directory, erring on the side of detecting a VCS checkout somewhere.

This commit avoids a PHP warning if the open_basedir directive is in use and any of the directories checked in the process are not allowed:

is_dir(): open_basedir restriction in effect. File(/.git) is not within the allowed path(s)

Follow-up to [25421], [25700], [25764], [25835], [25859].

Props costdev, markjaquith, meyegui, dd32, arnolp, robin-labadie, hellofromTonya, afragen, pbiron, SergeyBiryukov.
Fixes #42619.

Thanks for the PR! Merged in r55425.

Note:

• I have slightly modified the two tests for the open_basedir directive, as they failed on my setup with multiple checkouts and a symlink to the vendor directory in the “primary” checkout. There were some issues with PHPUnit along the lines of:

  Warning: include(): open_basedir restriction in effect. File(S:\home\wordpress.test\develop-trunk\vendor\phpunit\phpunit\src\Framework\TestFailure.php) is not within the allowed path(s): (S:\home\wordpress.test\develop-test/;S:\home\wordpress.test\develop-test/src/) in S:\home\wordpress.test\develop-trunk\vendor\composer\ClassLoader.php on line 444
  Warning: include(S:\home\wordpress.test\develop-trunk\vendor\phpunit\phpunit\src\Framework\TestFailure.php): Failed to open stream: Operation not permitted in S:\home\wordpress.test\develop-trunk\vendor\composer\ClassLoader.php on line 444
  Warning: include(): Failed opening 'S:\home\wordpress.test\develop-trunk\vendor\composer/../phpunit/phpunit/src/Framework/TestFailure.php' for inclusion (include_path='.;C:\php\pear') in S:\home\wordpress.test\develop-trunk\vendor\composer\ClassLoader.php on line 444
  Class "PHPUnit\Framework\TestFailure" not found
  

• I have also removed the error suppression operator from the is_dir() call, which was added in r25764 as a workaround for the same issue, and should no longer be necessary with this commit.