search

h5p / h5p-wordpress-plugin

Adds support for H5P Content in WordPress.
https://wordpress.org/plugins/h5p/
71 stars 74 forks source link

Add filter that lets us hide the 'disable file check' option even for super admins #107

Closed SteelWagstaff closed 4 years ago

SteelWagstaff commented 4 years ago

The H5P plugin is written so that site admins (and super-admins on multisite installations) are automatically given permissions to opt-in to an option which allows them to 'Disable file extension check' when uploading h5p files/libraries: https://github.com/h5p/h5p-wordpress-plugin/blob/50744d332e331c4fb8b0d2aaefb0eec015bf9ad8/public/class-h5p-plugin.php#L651-L658

These admin users will see prompts allowing them to disable H5P security in two places: 1) the H5P libraries page: https://github.com/h5p/h5p-wordpress-plugin/blob/7059e2b1e43e377e03b2f2e5021da9ee744426f6/admin/views/libraries.php#L59-L64

Screenshot from 2019-12-07 16-42-34

and 2) the 'Add New' menu page (if the user choose not to use the recommended H5P hub for installing new activity types): https://github.com/h5p/h5p-wordpress-plugin/blob/e939e30534222edc68966793868a7fc2e4e7bbfe/admin/views/new-content.php#L31-L36

Screenshot from 2019-12-07 16-43-46

When active, the setting is used to exempt files from a filter_input function used when files are uploaded to the server: https://github.com/h5p/h5p-wordpress-plugin/blob/25134ec41cf0854a43d10c08de996ac8ef98dda7/admin/class-h5p-plugin-admin.php#L571-L577

It's unclear to us what types of situations a user would have a valid need to override this file extension check. For security reasons, we'd like to have a filter/setting which would allow us to remove this option (even for admins/super admins) without forking the plugin. Is that something that you'd be willing to add or consider a pull request for?

icc commented 4 years ago

If I remember correctly the idea is to open up for putting custom files in content types that are not typically supported. If I'm not wrong you should already be able to override this permission check or even just "remove" this permission from any existing roles. But there is some time since I've been playing around with this.

SteelWagstaff commented 4 years ago

Thank @icc -- our concern in our specific situation is that the option allows admins to upload just about anything they want as an H5P file/library. The 'warning' notice you provide is a good one, but we'd like to remove this ability entirely for users on our network. We could override the permission check or remove the permission from existing roles, but it would be easier to do so if there was a filter in the plugin or even a setting that toggled this on/off. Basically, the crucial piece we'd like to override/exclude is this part: https://github.com/h5p/h5p-wordpress-plugin/blob/50744d332e331c4fb8b0d2aaefb0eec015bf9ad8/public/class-h5p-plugin.php#L651-L658 (where admins on single WP installs and superadmins on multisites) are automatically given the 'disable_h5p_security' role permission by default. Does that make sense? If so, how would you recommend we go about doing this presently?

SteelWagstaff commented 4 years ago

@icc one of our devs took a closer look at this after your comment above. We now have solid idea how to do this ourselves. I'm closing the issue. Thanks!

dac514 commented 4 years ago

Related PR (in out stuff): https://github.com/pressbooks/pressbooks/pull/1874