#42619 closed defect (bug) (fixed)
WordPress tries to access.bzr or .git without checking open_basedir restrictions
Reported by: | meyegui | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 4.9 |
Component: | Upgrade/Install | Keywords: | has-patch needs-testing has-unit-tests commit |
Focuses: | Cc: |
Description
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
Attachments (2)
Change History (28)
#2
@
7 years ago
- Component changed from General to Upgrade/Install
- Keywords needs-patch added
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.
#3
@
3 years ago
Added is_allowed_dir()
that checks to see if one of of the allowed open_basedir directories starts off the directory to be checked.
#4
@
3 years ago
- Summary changed from WordPress tries to access /home/.bzr but to no avail to WordPress tries to access.bzr or .git without checking open_basedir restrictions
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
#6
@
2 years ago
- Milestone changed from Awaiting Review to 6.1
Some history here, via comment:3:ticket:56189:
- [25700] / #22704 introduced this code.
- [25764] / #25572 added the
@
error suppression tois_dir()
to address the same issue being reported here.
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
#11
@
2 years ago
- Keywords needs-unit-tests added
Thanks for the ticket @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 🙂
This ticket was mentioned in PR #3224 on WordPress/wordpress-develop by costdev.
2 years ago
#12
- Keywords has-unit-tests added; needs-unit-tests removed
Trac ticket: https://core.trac.wordpress.org/ticket/42619
#13
@
2 years ago
- Keywords dev-feedback added
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 🙂
#14
@
2 years ago
- Milestone changed from 6.1 to 6.2
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.
This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.
2 years ago
#17
@
2 years ago
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.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
21 months ago
#19
@
21 months ago
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.
This ticket was mentioned in Slack in #core-test by robinwpdeveloper. View the logs.
21 months ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
21 months ago
#22
@
21 months ago
- Keywords commit added; dev-feedback removed
This ticket was discussed in the bug scrub. Pinging @SergeyBiryukov and @audrasjb for a final test/review for commit
consideration.
21 months ago
#23
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.
#24
@
21 months ago
Additional props to @afragen and @pbiron for rubber-ducking with me on the open_basedir
tests.
#25
@
21 months ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 55425:
@SergeyBiryukov commented on PR #3224:
21 months ago
#26
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 thevendor
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.
Still happening in WordPress 4.9.2.
Any idea why WP would try to read that file?