Closed lazardanlucian closed 3 years ago
related to #190
@mohammedeisa can you test if this fix doesn't have an impact on other items?
Dovy asked me to peek that this. So, here's the problem. This function is a fallback for get_contents in case wp_filesystem fails. Adding the regex to look for only .php files could, potentially, lock out anyone attempting to read file contents other than a PHP file. I would suggest adding 'is_file' to that if statement first and see if that solves the problem.
@lazardanlucian We've merged a fix into the repo here. Would you mind testing it quickly to see if it works for you on VIP?
file_exists will always return true for extension-free values (e.g directory names).
https://docs.wpvip.com/technical-references/vip-go-files-system/media-uploads/
Why would you 'require' any other file than php? require/include will parse php code. If you really really feel like you don't know what you're doing and need to allow any files, maybe use include/include_once which will not break;
require is identical to include except upon failure it will also produce a fatal E_COMPILE_ERROR level error. In other words, it will halt the script whereas include only emits a warning (E_WARNING) which allows the script to continue.
https://www.php.net/manual/en/function.require.php https://www.php.net/manual/en/function.include.php
After my coffe I see you used is_file, Something weird is happening, I tested it now with your fix, and without my fix, both seem to work now, This might have more implications, like if the directory exists already ? i'm not sure how to generate one since we don't have access to the f.s.
But my point still stands with the require: 1 Why would you require other files than php ? 2 If the files are optional and might be broken, shouldn't you use include instead (which doesn't error out, skips bad files)
@dovy I will not even test your update, https://docs.wpvip.com/technical-references/vip-go-files-system/media-uploads/ clearly states file_exists can return true for directories.
actually i see now that you want to use is_file, I will do a quick test, although reading on the subject, the norm is to use file_exists && is_file together,
I edited my email response, sorry for my foggy mind, still I feel like requiring all sorts of files is bad, maybe switch to include if you want
@lazardanlucian There are situations where people will want to require another type of file and read it into a buffer. Like with the raw field.
If you could test it, I'd appreciate it.
due to some filesystems nature ( proemintantly VIP ), require_once was trying to load a directory I'm not sure why $abs_path was a directory, albeit from seeing in the code, the redux autoload class puts $abs_path as /uploads/ + /redux/version , so it might start from there, while this is a temporary fix for wp-admin not to break in some instances, i'm not sure if other issues are present in the plugin.