humanmade / S3-Uploads

The WordPress Plugin to Store Uploads on Amazon S3
1.92k stars 388 forks source link

Media Library vs. Uploads folder - Plugin conflicts #363

Open redstamp-development opened 4 years ago

redstamp-development commented 4 years ago

The common use-case for this plugin is to host the Media Library on S3, but as a number of open tickets suggest, there are unintended consequences of the implementation (the entire uploads folder vs just the media library). I think it would be helpful to add this to the documentation, so that developers can be aware of and address any potential conflicts this creates.

For instance, database plugins such as Migrate DB can store exported/backed up databases in the uploads folder (in their own subdirectory), but these will then be hosted in the S3 bucket, potentially unbeknownst to the dev / site owner. The S3 bucket may have different permissions then if the uploads folder was hosted locally, and this can present a potentially large security risk.

As a feature request, it would be very helpful to have both a known plugin issue list, as well as the ability to easily add subfolders inside 'uploads' to a blacklist via a setting (passing an array of folder names for instance)

codeagencybe commented 4 years ago

@redstamp-development I'm just trying this plugin on a test instance, and indeed the plugin is changing the path for many unwanted effects.

In my test instance, we use a plugin to handle EU VAT/TAX managing, and the plugin fails too with below error code:

Error [wc-aelia-foundation-classes - 1100] GeoIP database file not found. Please try to install the database again. If the error persists, please download the the database manually, from http://geoip.aelia.co/Geolite2-City.mmdb.gz. Extract file GeoLite2-City.mmdb from the archive and copy it to s3://testfabio/uploads. Geolocation will become available automatically, as soon as the GeoIP database is copied in the indicated folder.

This is just one of the several things I notice. Do you happen to know if there is somewhere an option to declare/exlude folders or paths from modifying the source path? This is pretty much very problematic for many websites.

hubertnguyen commented 4 years ago

@redstamp-development adding a whitelist of file types/extensions or a limit on the file sizes could be low-hanging fruits to avoiding having a DB uploaded to S3 as well. Blacklisting doesn't protect you from a new plug-in or a change in existing plugins, in case they decide to rename their folders.

dsgolfside commented 4 years ago

i'm really interested in using this plugin specifically because it supports the entire uploads folder...there is a plugin i use that stores media files in their own custom directly inside the uploads folder so the usual plugins for s3 don't work ....as long as i dont have anything strange in the uploads folder i should be ok then ? or is @hubertnguyen fix to whitelist extensions (kind of like how wordpress does it only allow jpg, gif, mp4, etc ) is that fix going to be implemented soon ?

heathera2016 commented 4 years ago

@hubertnguyen How can I enable whitelist for specific folders? Any ideas to help me? Thank you :)

hubertnguyen commented 4 years ago

@heathera2016 , I was just suggesting an improvement to the plugin. I don't think someone added that yet, sorry.

joehoyle commented 4 years ago

The common use-case for this plugin is to host the Media Library on S3, but as a number of open tickets suggest, there are unintended consequences of the implementation (the entire uploads folder vs just the media library). I think it would be helpful to add this to the documentation, so that developers can be aware of and address any potential conflicts this creates.

The use-case for this plugin is specifically to have the entire uploads directory on S3, it's not an implementation detail, and the use-case is not to just support the media library. I think it would probably be a good idea to state this quite clearly in the project readme though. S3 Uploads is designed to allow you to run your WordPress base site without any writable directories in the webroot. This means you can do things like mount the file-system as read-only (good security practice) and the like.

As a feature request, it would be very helpful to have both a known plugin issue list, as well as the ability to easily add subfolders inside 'uploads' to a blacklist via a setting (passing an array of folder names for instance)

This goes against the goal of this plugin, as I think I've mentioned in a few other issues asking for this feature. The reason we implement it at such a low level (switching wp_upload_dir() for a stream-wrapped URL scheme) is specifically to make it more like a file-system replacement).

For instance, database plugins such as Migrate DB can store exported/backed up databases in the uploads folder (in their own subdirectory), but these will then be hosted in the S3 bucket, potentially unbeknownst to the dev / site owner. The S3 bucket may have different permissions then if the uploads folder was hosted locally, and this can present a potentially large security risk.

This is potentially interesting. Can you suggest any bucket permission configurations that are more insecure than the world-readable nature of people's local uploads directories? Directory listing is the only thing that comes to mind, though I think it's actually probably more common to have directory listing accidentally enabled in apache/nginx than it is s3.

hubertnguyen commented 4 years ago

It seems like a bad practice to put non-uploads items in the /uploads/ directory. Why not have these in /wp-content/ unless the user opts for /uploads/ ?

From a security standpoint, I think that you should be able to tighten security on a per-folder basis by restricting the bucket, then giving IAM users a different level of access.

However, if you want the general public to access /uploads/ (maybe via Cloudfront), you have to have public access permission for the whole bucket. At least, it's my understanding. So, it would be very bad to have backups and other "private" things uploaded in there.

joehoyle commented 4 years ago

Yup agreed.