CUNY Academic Commons

The approach here seems good to me. Thanks for thinking it through, Ray.

I've got a new branch up to test: https://github.com/cuny-academic-commons/cac/compare/2.0.x...multisite-mime-types.

For point 1, WordPress already has a predetermined list of allowable mime types -- get_allowed_mime_types(). Let's use this to populate the 'pre_site_option_upload_filetypes' filter so we only have to take care of point 2

What I mention here is now easily handled by removing the multisite upload filter with remove_filter( 'upload_mimes', 'check_upload_mimes' ); . We no longer need to use the 'pre_site_option_upload_filetypes' filter anymore. With that said, I'm also hiding the "Upload File Types" network option to avoid potential confusion. Let me know what you think here, Boone. The CSS trick I'm using to hide this option only works in Chrome by default. View the commit for all details.

There are only a few file types in get_allowed_mime_types() that we would want to omit. Namely any archive-related ones in the Misc section

Now that we are relying on get_allowed_mime_types() for uploads, I've excluded all archive-related filetypes as well as .class files, which can be executed by Java. The .class exclusion could be reverted as we allow Excel spreadsheets to be uploaded and Excel spreadsheets can be a vector for malware via macros. Would also welcome your feedback here, Boone.

You've removed pptx and ics from the mime_types callback. Is that because they're already defined by WP?

What I mention here is now easily handled by removing the multisite upload filter with remove_filter( 'upload_mimes', 'check_upload_mimes' ); . We no longer need to use the 'pre_site_option_upload_filetypes' filter anymore. With that said, I'm also hiding the "Upload File Types" network option to avoid potential confusion. Let me know what you think here, Boone. The CSS trick I'm using to hide this option only works in Chrome by default. View the commit for all details.

This logic looks good to me. I don't think it matters if the upload_filetypes input is visible - no one ever looks at that page, and in any case, even now it it's non-operable (in that you can't really save changes by changing the value of the input).

Now that we are relying on get_allowed_mime_types() for uploads, I've excluded all archive-related filetypes as well as .class files, which can be executed by Java. The .class exclusion could be reverted as we allow Excel spreadsheets to be uploaded and Excel spreadsheets can be a vector for malware via macros. Would also welcome your feedback here, Boone.

Regarding the Excel vector, I don't think we can protect against all potential vectors for malware to be spread via downloaded file. If this were the case, we wouldn't allow any uploads at all. Instead, we want to focus our caution on the following: (a) files that can be executed by the browser in the case of a normal page visit (like an image file, or like an Office file that might be embeddable via plugin), or (b) files that might be executed by the server in a potentially malicious way (such as certain types of archive files). This feels like as much as we can reasonably do.

You've removed pptx and ics from the mime_types callback. Is that because they're already defined by WP?

Yes. For .ppsx, that's defined here: https://github.com/WordPress/WordPress/blob/dca7b5204b5fea54e6d1774689777b359a9222ab/wp-includes/functions.php#L3415 . And for .ics, that's defined here: https://github.com/WordPress/WordPress/blob/dca7b5204b5fea54e6d1774689777b359a9222ab/wp-includes/functions.php#L3365 . I tested with uploading an .ics file locally and I was able to upload to the Media Library.

Instead, we want to focus our caution on the following: (a) files that can be executed by the browser in the case of a normal page visit (like an image file, or like an Office file that might be embeddable via plugin), or (b) files that might be executed by the server in a potentially malicious way (such as certain types of archive files). This feels like as much as we can reasonably do.

Gotcha. The .class file type could potentially be embedded into a HTML file in a Java applet, so I'm going to keep that excluded. If there are any other file types in wp_get_mime_types() that should be excluded, let me know. Otherwise, would you feel comfortable if we merged these changes into 2.0.x? Or should this go into 2.1.x?