Opened 3 years ago
Last modified 5 months ago
#53298 new defect (bug)
Checking if wp-config-sample.php file exists before checking if wp-config.php exists
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | trivial | Version: | 5.7.2 |
Component: | Upgrade/Install | Keywords: | needs-patch dev-feedback |
Focuses: | administration, privacy, coding-standards | Cc: |
Description
Currently in WordPress core, wp-admin/setup-config.php checks if wp-config-sample.php file exists before checking if wp-config.php exists. If the sample file exists, it then checks if the wp-config.php file exists, and if so, suggests deletion if necessary. For security, some WordPress users may delete the sample file, and restrict open_basedir for directory above that of the web root directory. Because of these two cases, the current order produces the follow error:
PHP message: PHP Warning: file_exists(): open_basedir restriction in effect. File(/var/www/example/wp-config-sample.php) is not within the allowed path(s): (/var/www/example/web:/var/www/example/private:/var/www/example/tmp:/tmp:...) in /var/www/example/web/wp-admin/setup-config.php on line 46
If the check for existence of sample file could be moved after checking if wp-config.php exists, we could avoid this error and avoid checking if sample file exists if wp-config.php does and not checking both if they both do.
i.e. Moving the section commented Support wp-config-sample.php one level up, for the develop repo.
to after the section commented Check if wp-config.php exists above the root directory but is not part of another installation.
in wp-admin/setup-config.php
Change History (6)
#4
follow-up:
↓ 5
@
14 months ago
- Keywords dev-feedback added
Hi @machineitsvcs, welcome to Trac and thanks for opening this ticket, and providing some potential solutions!
As you said, if we reverse the conditions, then if wp-config.php
and wp-config-sample.php
don't exist in ABSPATH
, the second wp-config-sample.php
check will try to go up one level, and the warning is still shown. For that reason, I don't think swapping the order is the solution here.
@SergeyBiryukov The @
operator seems the most appropriate solution here as it's already there for the wp-config.php and wp-settings.php checks.
However, I know that we're trying to reduce the number of @
operators in Core, and adding a new one seems counter-productive to that effort. That said, file_exists() emits an E_WARNING
, not a fatal error, and when considering consistency with other checks in this file, it may be acceptable to add a new @
operator here.
I'd appreciate your thoughts on how this might proceed.
#5
in reply to:
↑ 4
@
6 months ago
Replying to costdev:
I'd appreciate your thoughts on how this might proceed.
WP_Automatic_Updater::is_allowed_dir()
was recently introduced in [55425] / #42619 to check for an open_basedir
restriction in the context of automatic updates.
It looks like this may be useful in other contexts too, so perhaps we could move that check into a separate function and make that method a wrapper?
That said, it might not be worth it if this is the only other place where that check is needed, so using @
seems fine.
Replying to machineitsvcs:
For security, some WordPress users may delete the sample file, and restrict open_basedir for directory above that of the web root directory.
I'm curious though, what kind of security enhancement does removing the wp-config-sample.php
file provide? Unless I'm missing something, that file cannot be used in any way if wp-config.php
exists already.
#6
@
5 months ago
I believe the main consideration to note here, whether or not the @ is used, is that the order of checking wp-config-sample.php and wp-config.php should be swapped, as most requests for a WordPress site would be for completed installations where wp-config.php exists, and therefore checking if wp-config-sample.php exists when wp-config.php already exists would produce the message that the wp-config-sample.php doesn't exists instead of the message that wp-config.php already exists.
Unless your concern is that the message stating wp-config.php exists instructs that the wp-config.php be deleted first, and that this would result in a broken installation if wp-config-sample.php also did not exist, requiring the wp-config-sample.php file be re-uploaded, which is already presented in a message if the file is missing anyway, so the messaging wouldn't even have to change.
Regardless, in my opinion, it just seems that logically it should consider if wp-config.php exists first, and only check if wp-config-sample.php exists if not. Although the improved performance from removing the two additional file_exists calls in the current version would be negligable, it should still be seen as best practice in most use-cases.
As for the security situation I mentioned, I was referring specifically just to restricting open_basedir to not include the directory above the web directory (which shouldn't need to be explained), and that such a restriction, in combination with the deletion of wp-config-sample.php file, would result in the PHP warning message in the log. That is all. Deleting the wp-config-sample.php file in and of itself is not necessarily a "security measure" - although deleting the file on a completed installation does reduce the unnecessary overhead from requests to this file, which subsequently loads the wp-settings.php file, which loads numerous other functions for no reason, to inevitably return a "Database Error - Error establishing a database connection" page, whereas on a completed installation the wp-config.php returns nothing (except a 200 response) if it is even publicly accessible (I personally block public access to wp-config.php using .htaccess). However, most do not take any similar steps to restrict public access to the wp-config-sample.php file if it still exists, as they assume like you did that it's a non-issue. Which isn't wrong exactly, but I believe it could definitely be improved upon, but that's for another discussion I guess. Hopefully this at least helps to explain why someone, like me, would delete the wp-config-sample.php file, and be plagued by the persistent error log messages/warnings due to bad actors trying to probe the setup-config.php file on my sites with the current file check order used.
It may also be worth prefacing the elseif condition
file_exists( dirname( ABSPATH ) . '/wp-config-sample.php' )
with a@
to avoid any error should both the wp-config.php and wp-config-sample.php not exist and the parent directory be inaccessible, similar to what is already done for the wp-config.php when looking in parent directory.i.e. using
@file_exists( dirname( ABSPATH ) . '/wp-config-sample.php' )
instead