The configurations of plugin should be saved in database, not in config.json

lwxbr commented 5 years ago

The plugin have the configurations saved in a config.json. For security it is not good. All the persistent data of WordPress should be saved in database for security reasons. Anyone that have access to FTP can modify the options of the plugin and misuse it.

The second matter is that config.json can be imported to database, So, a nice feature of implement would be export/import configuration through that file. That way you don't will loose your work and keep the site safe! Thank you!

rosell-dk commented 5 years ago

If you have access to FTP, you can easily get access to database too, or put in malware for that sake.

lwxbr commented 5 years ago

Yes, unluckily, you can get the password of database in wp-config.php. But you can limit the ips that will have access to that server (DB server). I don't like the idea to get data from a file. If it would be in database, you could use get_option to get a particular data from database and use the option update_option to save it. But working with files, you have to get the content of whole file, modify it and save it again. All the plugins I got save data in database using that API. The exception is Webp Express.

rosell-dk commented 5 years ago

Actually, the .htaccess magic in WebP Express does not redirect to Wordpress, but directly to a php (no Wordpress bootstrap). That php does not have update_option or any other Wordpress functions at its disposal. It would require a Wordpress bootstrap.

I'm not worried about getting the content of a whole file, modifying it and saving it again. It is only when you save the options that happens, so it is not resource extensive. And the configuration file is small. And there has been no reports about corrupt configuration files.

lwxbr commented 5 years ago

Actually, the .htaccess magic in WebP Express does not redirect to Wordpress, but directly to a php (no Wordpress bootstrap). That php does not have update_option or any other Wordpress functions at its disposal. It would require a Wordpress bootstrap.

Ok. Webp Express didn't have update_option and get_option at disposal looking only as a php module of composer. The Webp Express work in another platforms other than Wordpress? Is a independent module, right?

Webp Express as a plugin should have a layer that works above the base module 'Webp Express' of composer. So it will have both APIs. The module itself didn't know what is WordPress.

I'm not worried about getting the content of a whole file, modifying it and saving it again. It is only when you save the options that happens, so it is not resource extensive. And the configuration file is small. And there has been no reports about corrupt configuration files.

I do worry because at the moment, I have a theme installer that preconfigurates the Webp Express Plugin to work as desired after theme installation and I have to modify that file.

rosell-dk commented 5 years ago

I'm back!

Actually, the name "WebP Express" is only used for the Wordpress plugin (the "press" part in "express" is meant to associate with Wordpress). The Wordpress plugin uses four composer libraries, which I created for the purpose of the plugin.

WebP Convert: For converting images to webp. And also for serving webp images. And the logic needed for "Serving converted" webp images (doing a conversion if needed, but otherwise simply sending the existing converted image)
WebP Convert Cloud Service: For the Web Service functionality
DOM Util for WebP: For the Alter HTML functionality
Image MimeType Guesser: For detecting mime types of images.

The reason storing settings in the database is a problem is because I cannot get redirection from a .htaccess to the converter work when the converter script does a Wordpress bootstrap.

When one activates "Enable redirection to converter?", rules will be generated for the .htaccess. These are generated by WebP Express (though I have thought about generalizing this code and create a new composer project for that - to make it easier to create plugins for other CMS`es).

The rules that does the redirect may look like this (depending on paths - in this install, I have moved both the wp-content folder and the plugin folder):

  # Redirect images to webp-on-demand.php (if browser supports webp)
  RewriteCond %{HTTP_ACCEPT} image/webp
  RewriteCond %{REQUEST_FILENAME} -f
  RewriteCond %{QUERY_STRING} (.*)
  RewriteRule ^/?(.+)\.(png)$ /plugins-moved/webp-express/wod/webp-on-demand.php?%1 [E=REQFN:%{REQUEST_FILENAME},E=WPCONTENT:wp-content-moved,NC,L]

So it redirects images to wod/webp-on-demand.php. This is an ordinary PHP file. There is no Wordpress bootstrap. The job of this file is to:

1) Find out where the configuration file is located. It does that by inspecting the environment variable "WPCONTENT" that is set in the RewriteRule when redirecting to the script. 2) Find out the filename of the image that is to be converted. It does that by examining the environment variable "REQFN" that is set in that same RewriteRule.

I have tried to let Wordpress handle the conversion requests by pointing to index.php from .htaccess - in order to be able to use Wordpress functions. When I did that, I however ran into a problem: I lost the information in the environment variables. That is: I lost the information about which file it is supposed to convert. I think the problem it was lost because of other .htaccess rules.

It may be that I could do a Wordpress bootstrap from webp-on-demand.php. I haven't tried that route.

rosell-dk commented 5 years ago

I just tried bootstapping Wordpress from webp-on-demand.php. The following actually seems to work fine.

require_once($pathToWpRoot . '/wp-load.php');

– and I have access to Wordpress functions. The path to wp root can be transferred to the script in a similar way as I transfer the path to wp-content.

rosell-dk commented 5 years ago

But the require_once above will not be secure.

Actually, setting an environment variable in a RewriteRule does not work on all systems. And it definitely doesn't work on Nginx. For that reason, as a fall back, webp-on-demand.php allows these variables to be passed in the querystring.

In the current implementation, this means that one can trick webp-on-demand.php to look for the configuration file in a wrong location. If that someone should have managed to place a "malicious" configuration file in another location, there is room for exploitation. To protect against this, webp-on-demand.php exits if it is called directly. But it cannot do that on Nginx because I have not found a way to detect a direct call on Nginx. So on Nginx, there is currently a security problem. But the malicious activities are limited, as all you can do with this hole is to convert image files into webp.

If however we use an insecure variable to load PHP, there would be real trouble.

So in order to do the bootstrap, I would need a secure method of obtaining the Wordpress root path.

rosell-dk commented 5 years ago

An idea for strengthening security in the current setup could be to compute a hash of the configuration file and store it in the .htaccess so it is passed to webp-on-demand.php in the query string. webp-on-demand.php could then compare the passed hash with the computed hash. This change would mean that .htaccess rules would have to be regenerated on every save.

For (the few) users that have hardened the security by not allowing .htaccess files, this would however be a bit cumbersome, as they would have to copy the rules to their virtual host file even when doing small configuration changes. And for Nginx users, it would be even more cumbersome, as they would have to compute the hash themselves. I could bypass this check for Nginx, but it is really on Nginx it is most needed.

rosell-dk commented 5 years ago

Perhaps a similar idea as the above could be used to make the require_once secure. The obvious first thought is to compute the hash of wp-load.php. However, this is not secure, as wp-load.php loads in wp-config.php (either in current or parent). An attacker could place an authentic wp-load.php together with a malicious wp-config.php. So the content of wp-config.php would also need to be hashed. Another problem is that this approach requires the rewrite rules to be updated upon Wordpress update and updates to wp-config.php. But perhaps something else could be used for hashing...

rosell-dk commented 5 years ago

I have asked for help on stackoverflow

rosell-dk commented 5 years ago

Of course, one way to go about it would be to entirely drop the feature of redirecting image requests to the converter script. With the Bulk convert functionality, the feature isn't required anymore. Also, in addition to Bulk convert, I could implement Background updates, as you suggested.

The redirect rules however has a couple of nice features:

They only convert the images that are actually referenced
The "webp realizer" functionality allows one to alter html to point to webp files that does not yet exist.

That said, the rules also has some drawbacks. They are complex and result in complexity and issues that are apparent in this issue. They also slow down the server a tiny bit. And they put a lot of pressure on the server when a page with many unconverted images are visited.

An in-between solution could be to have webp-on-demand.php and webp-realizer.php serve the original file and queue the convert request.

rosell-dk commented 5 years ago

I'm piling up solutions for making it secure to bootstrap Wordpress from within webp-on-demand.php (see the stackoverflow link above). And I also actually started implementing it. It is a relief to be able to use Wordpress functions and not have to divide the library into parts that depends on Wordpress and parts that does not. I'm however having second thoughts about doing this. The reason is performance. When no images have been converted yet and a page is visited, a lot of conversions is started. A lot of load, which can be a problem. If each of these requests has to boot Wordpress, it will add to the pressure.

lwxbr commented 5 years ago

Thank you, @rosell-dk, for giving attention to this thread. At the moment I'm very busy in another projects but as soon I got some time. I will continue the work in this plugin. Mostly with ideas than code. Keep going with the good work.

EusebiuOprinoiu commented 3 years ago

I know this issue is old, but if it's still relevant, don't bootstrap WordPress by loading /wp-load.php. It's a terrible solution in terms of performance. I remember a discussion on this topic from a few years ago when Otto Wood advised against this method even for a simple PHP file used as dynamic CSS. (and that's a case when the load happens once)

Implementing this in a script that generates images on the fly would be catastrophic in terms of performance, especially when there are multiple images on a page and /wp-load.php is called for every single one.

WillyReyno commented 10 months ago

Sorry to bump this topic, but the fact that configurations is set in json files makes it complicated to install the plugin from wp-packagist and have every plugins of a WordPress project managed by composer.

Hoping you will someday support storing config in database (maybe as an option)

rosell-dk / webp-express

The configurations of plugin should be saved in database, not in config.json #283