Custom fonts extension, get_wp_filesystem() return value not checked can result in an exception thrown
jptrosclair opened this issue · 15 comments
I realize that theme users should file bug reports to the theme developers, but this is an issue in redux-framework:
On line 456 of class-redux-filesystem.php it states that get_wp_filesystem() may return a boolean false value.
On line 239 of class-redux-extension-custom-fonts.php get_wp_filesystem() is called without checking the return value causing an exception to be thrown when false is returned.
PHP Fatal error: Uncaught Error: Call to a member function dirlist() on bool in ..../wp-content/plugins/redux-framework/redux-core/inc/extensions/custom_fonts/class-redux-extension-custom-fonts.php:239
This may very well be an issue caused by the theme, but the lack of a check brings the site down for users unfortunate enough to update redux-framework past version 4.2.31. There are similar, but not identical reports of this on the redux-framework forums with notes about changes to custom fonts.
A simple check will prevent the crash and at least some headache for end-users with themes using redux-framework triggering this scenario to happen:
$wpfs= $this->parent->filesystem->get_wp_filesystem();
if($wpfs!== false) {
$fonts = $wpfs->dirlist( $this->upload_dir, false, true );
}
It's a band-aid solution, but then fonts won't load if the call falses out, and I get frustrating messages that say nothing more than "it don't work" and nothing else. I'd be more interested in discovering why the error occurs, to begin with. That is, why does the call to the wp_filesystem proxy fail? Custom font has been an extension for almost a decade and this has never been reported.
This has never been a problem for us until the most recent update. Version 4.2.31 works without issue, anything newer does not. While it may be a band-aid fix, dropping a message to the error log or having something appear under wp-admin letting admins know something isn't right instead of an exception would be better than having the entire site go down. For security reasons alone it's better for end-users to not have plugins breaking the entire site, especially something like this where it's not immediately apparent how to proceed with fixing it thus prompting a potentially indefinite roll-back to whatever the last known working version was.
There is no version 4.2.31. There never was.
Since you're clearly learned with this, I'd be more interested in knowing why your server is failing to initialize the wp_filesystem properly. That is the true cause I'd prefer to fix. Otherwise, even if I drop a "file system failed to initialize, fonts will not load," I still have to figure out why, and I can't do that without crawling all over someone else's server, which I would prefer not to do.
So, if that's a determination you can make, that data would be more valuable to us. If not, I can throw in that catch, but the extension won't load fonts on your system.
Apologies, I meant 4.3.21.
I'm actively trying to determine why wp_filesystem is failing to initialize. The file system where wordpress is installed is read-only for the user the httpd/apache service runs as except for the wordpress uploads directory where it has read/write access and I wonder if that may be causing initialization failures, but there is nothing logged saying so if it is. Updates to plugins and wordpress are managed using wp-cli.
As best as I can tell, the custom fonts extension was merged in after 4.3.21 (I don't see it in the source tree for that version). That makes me wonder if this isn't a new problem, rather maybe wp_filesystem hasn't been initializing for a long time and the inclusion of custom_fonts has brought that to light since it's using wp_filesystem.
I did not develop the theme in question, but I have not found where it was using anything custom fonts before so I'm not certain it is the cause of this issue. My main concern with this report is seeing if we can get it fixed so that it doesn't bring the site down by using more graceful error handling for this error condition. While I understand something is still wrong, the trade-off of not doing it is that we're stuck not updating this plugin and that part isn't acceptable from a security point of view.
I'll continue to look into this and see if I can find an answer as to wp_filesystem is not initializing correctly so we can deal with that part as well.
I just reworked the directory enumeration code not to use the WP_Filesystem (which is badly implemented, IMO). This should avoid exceptions. Give the version of Redux I just pushed to the repo a try, v4.3.24.1, and let me know if the issue is resolved on your system.
Forcing the file system method to direct in wp-config.php allows wp_filesystem to initialize correctly:
define('FS_METHOD', 'direct');
In class-redux-filesystem.php in the maybe_init_wp_filesystem() method, the call to wp_filesystem($credentials) fails with the below error so the call to request_filesystem_credentials() is detecting that it doesn't have r/w access, or more specifically the wordpress core function get_filesystem_method() is detecting so. Setting the FS_METHOD constant overrides the check that get_filesystem_method() does allowing it to work.
Error info from note above:
[23-Jan-2023 22:27:02 UTC] WP_Filesystem_FTPext Object
(
[link] =>
[verbose] =>
[cache] => Array
(
)
[method] => ftpext
[errors] => WP_Error Object
(
[errors] => Array
(
[empty_hostname] => Array
(
[0] => FTP hostname is required
)
[empty_username] => Array
(
[0] => FTP username is required
)
[empty_password] => Array
(
[0] => FTP password is required
)
)
[error_data] => Array
(
)
[additional_data:protected] => Array
(
)
)
[options] => Array
(
[port] => 21
[ssl] =>
)
)
I've confirmed with the FS_METHOD constant set to direct that the custom fonts extension loads without triggering an exception on version 4.3.24.
/**
* Redux, a simple, truly extensible and fully responsive option framework
* for WordPress themes and plugins. Developed with WordPress coding
* standards and PHP best practices in mind.
*
* Plugin Name: Template Library and Redux Framework
* Plugin URI: http://wordpress.org/plugins/redux-framework
* GitHub URI: reduxframework/redux-framework
* Description: Build better sites in WordPress fast!
* Version: 4.3.24
* Requires at least: 4.0
* Requires PHP: 7.1
* Author: Extendify
* Author URI: https://extendify.com/?utm_source=redux&utm_medium=plugins-page&utm_campaign=by
* License: GPLv3 or later
* License URI: http://www.gnu.org/licenses/gpl-3.0.txt
* Text Domain: redux-framework
* Provides: ReduxFramework
Got it, but I'm working to take the wp_filesystem out of it completely. I just need to know if the version in the repo works. Thanks.
I have tested version 4.3.24.1 with the FS_METHOD option removed and confirmed that it does not throw an exception like the previous versions. There were no custom fonts in use to begin with, so I can't test that part to make sure it actually works- only that that no errors are triggered bringing the site down.
crash with define('FS_METHOD', 'ftpext'); and crash also with define('FS_METHOD', 'direct');, if remove FS_METHOD it working latest version, if use FS_METHOD ftpext o direct working only with version 4.3.22, my PHP 8.1.14
Please use the version available in this GitHub repo.
Please use the version available in this GitHub repo.
I have automatic updates in wordpress; I've already manually reloaded the latest available version from github but it doesn't work if I set FS_METHOD in the config
The 4.3.24.1 is no longer using the WP_Filesystem API call for Custom Fonts, so please stop doing what the other contributor suggested, It's not needed, and I never suggested it. Thanks,
Hello,
on github and wordpress I see version 4.3.24 and not 4.3.24.1 among the releases;
however the error I see is this:
Uncaught Error: Call to a member function dirlist() on bool in /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/inc/extensions/custom_fonts/class-redux-extension-custom-fonts.php:239
Stack trace:
#0 /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/inc/extensions/custom_fonts/class-redux-extension-custom-fonts.php(103): Redux_Extension_Custom_Fonts->get_fonts()
#1 /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/inc/classes/class-redux-api.php(250): Redux_Extension_Custom_Fonts->__construct()
#2 /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/inc/classes/class-redux-api.php(1611): Redux::load_extensions()
#3 /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/inc/classes/class-redux-extensions.php(118): Redux::set_extensions()
#4 /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/inc/classes/class-redux-extensions.php(28): Redux_Extensions->load()
#5 /web/htdocs/mmw/home/wp-content/plugins/redux-framework/redux-core/framework.php(500): Redux_Extensions->__construct()
#6 /web/htdocs/mmw/home/wp-content/themes/besa/inc/vendors/redux-framework/redux-config.php(62): ReduxFramework->__construct()
#7 /web/htdocs/mmw/home/wp-includes/class-wp-hook.php(308): Besa_Redux_Framework_Config->initSettings()
#8 /web/htdocs/mmw/home/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#9 /web/htdocs/mmw/home/wp-includes/plugin.php(517): WP_Hook->do_action()
#10 /web/htdocs/mmw/home/wp-settings.php(617): do_action()
#11 /web/htdocs/mmw/home/wp-config.php(104): require_once('...')
#12 /web/htdocs/mmw/home/wp-load.php(50): require_once('...')
#13 /web/htdocs/mmw/home/wp-admin/admin-ajax.php(22): require_once('...')
#14 {main}
thrown
then if it is a problem for you I will avoid reporting the problem in the future.
I will be using an older version that supports FS_METHOD for now, disabling updates to this plugin
thank you all the same
It's right here: https://github.com/reduxframework/redux-framework
Last commit is v4.3.14.2 beta.