New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check allowed mime types before uploading media #6968

Merged
merged 6 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@mirka
Contributor

mirka commented May 27, 2018

Description

This adds mime type checking to the pre-upload error messaging system added in #4203. It checks against the mime types allowed for the current WP user, using get_allowed_mime_types(). This check will be especially important for the generic File block in #6805.

How has this been tested?

Try to upload a restricted file (like an SVG) from the Image block placeholder.

Caveats

  • Error messaging functionality is not added to Audio/Video blocks yet. Should be fixed in #6957
  • Eventually there should also be a system that handles errors for files dropped directly onto the editor (i.e. not onto a placeholder)

Screenshots

mime-type

Types of changes

Enhancement

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@mirka mirka referenced this pull request May 27, 2018

Closed

Implement File block #6805

11 of 11 tasks complete
@nb

This comment has been minimized.

Member

nb commented May 29, 2018

That looks useful, @mirka, thanks. What do you think are the trade-offs between the error message living in the UI component or in the upload component (utils/mediaupload.js)?

// Allowed types for the current WP_User
const allowedMimeTypesForUser = get( window, [ '_wpMediaSettings', 'allowedMimeTypes' ] );
const isAllowedMimeTypeForUser = ( fileType ) => {
return Object.values( allowedMimeTypesForUser ).includes( fileType );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Hi @mirka, I think IE11 does not supports includes and we are not poly filling it. The alternatives here are .lodash->includes https://lodash.com/docs/4.17.10#includes that can receive the object directly or indefOf.

This comment has been minimized.

@mirka

mirka Jun 1, 2018

Contributor

Thanks, I'll fix that!

const isAllowedType = ( fileType ) => startsWith( fileType, `${ allowedType }/` );
// Allowed types for the current WP_User
const allowedMimeTypesForUser = get( window, [ '_wpMediaSettings', 'allowedMimeTypes' ] );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Have you considered adding an optional property allowedMimeTypes that defaults to get( window, [ '_wpMediaSettings', 'allowedMimeTypes' ] ); ? (similar to what we do with maxUploadFileSize) This would allow us to avoid the usage of global in the test case, and would make this function more generic without errors whose triggering can only be parameterized using a global variable.

This comment has been minimized.

@mirka

mirka Jun 1, 2018

Contributor

I decided against it because although it would make testing simpler, I didn't think it made sense to expose allowedMimeTypes as an overridable property. It is explicitly for checking what WP says is allowed, and the allowedTypes property should be used instead for consumer-side restrictions.

But I do understand your point about testing, and maybe that is more important than exposing too many properties. What do you think? I probably don't have a full grasp of all the ramifications yet.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 7, 2018

Member

allowedMimeTypes as an overridable property. It is explicitly for checking what WP says is allowed, and the allowedTypes property should be used instead for consumer-side restrictions.

A use case may appear where we want to upload something but we want to verify if the upload is of a given mimeType. But I understand your point this is to verify if the mimeType is allowed by WordPress. If later we want to parameterize this it should verify the mimetype of the parameter and this one and only allow the upload if both mimetypes allow it.
I think we can keep this version.

file.name
);
break;
case 'MIME_TYPE_NOT_ALLOWED_FOR_USER':

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Is using MIME_TYPE_NOT_ALLOWED instead of MIME_TYPE_NOT_ALLOWED_FOR_USER and being more generic in the error message 'Sorry, this file type is not allowed.' an option? Or there is a reason to be more specific?
The get_allowed_mime_types function says "Retrieve list of allowed mime types and file extensions." does not specifies that this restriction is user specific or related with security reasons. I think some restrictions may be user specific and others may not be user specific.

This comment has been minimized.

@mirka

mirka Jun 1, 2018

Contributor

The reason I'm saying _FOR_USER is to differentiate it from the other prop we currently have, allowedTypes. That one is also doing mime type checking. However, it is being used for arbitrary restrictions that have nothing to do with WP user privileges, such as only allowing image files for the Image block.

Right now, the check for allowedTypes is not throwing errors (which it should). When we eventually make that enhancement, we should probably rename it so it isn't as confusing. Something that doesn't use the word "allowed", like maybe accept or suggestedTypes.

I'm using the error message "Sorry, this file type is not permitted for security reasons" for consistency, since that is the message used by Core that is returned when you upload a restricted file.

I think some restrictions may be user specific and others may not be user specific.

This is true, but the impression I got from the code was that it is ultimately a function of the user (e.g. get_allowed_mime_types( int|WP_User $user = null ) and apply_filters( 'upload_mimes', $t, $user )). Anyway, the verbose naming (_FOR_USER) is just intended for differentiation, so we could possibly get rid of it when allowedTypes is renamed 🙂

@jorgefilipecosta

Thank you for you PR @mirka, I left some comments. In general I feel this is a very useful 👍

@mirka

This comment has been minimized.

Contributor

mirka commented Jun 1, 2018

@nb

What do you think are the trade-offs between the error message living in the UI component or in the upload component (utils/mediaupload.js)?

I imagine the reasoning in having them live in the UI component was that error message strings are something you might want to override as a matter of presentation.

For maximum flexibility and ease of use, a good way might be for mediaupload.js to:

  • instead of returning on the first error encountered, make one onError() call at the end with an argument object containing an array of all the matched error types
  • include a suggested error message string for each error type

This way the consumer would be able to prioritize certain errors, or override the strings as necessary.

@mirka

This comment has been minimized.

Contributor

mirka commented Jun 1, 2018

Thank you for the review, @jorgefilipecosta !

@nb

This comment has been minimized.

Member

nb commented Jun 4, 2018

include a suggested error message string for each error type

This makes sense. To be honest, I don't imagine a lot of consumers overriding the error messages, but they always can if we give them both the code and the message.

For me, the bigger problem with keeping the error messages outside of the media upload code is that we make the coupling between it and its consumers awkward – every time we add a new error code we must update all consumers if we want them to get the new error message.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 5, 2018

Related #7137

/**
* WordPress dependencies
*/
import { __, sprintf } from '../node_modules/@wordpress/i18n';

This comment has been minimized.

@mirka

mirka Jun 7, 2018

Contributor

Couldn't figure out why it's unable to resolve the module normally with just from '@wordpress/i18n'

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 7, 2018

Member

I'm not certain of the reason why '@wordpress/i18n' can not be used here I don't think we have a circular dependency. It may be something intentional to keep utils generic but this util here is something specific to WordPress so it makes sense to import @wordpress/i18n here.
We already have '@wordpress/is-shallow-equal' so another option for us is to have '@wordpress/mediaupload'.
@gziolo would it be possible to share your thoughts on this? Are you aware of the reason why '@wordpress/i18n' cannot be imported in utils/mediaupload.js?

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

@wordpress/i18n lives inside WordPress/packages so definitely there is no circular dependency. I have no idea. It should work without workarounds because otherwise, you duplicate this package in the bundles shipped to all users.

This comment has been minimized.

@mirka

mirka Jun 7, 2018

Contributor

Just to make sure, does this problem reproduce in your environments? @nb tells me a normal import { __, sprintf } from '@wordpress/i18n'; here is working for him.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 7, 2018

Member

Yes, I'm able to reproduce the problem. Everything seems to work until an error message is triggered.
To easily trigger error messages we can limit file uploads on the site to 3MB by adding the following line to .htaccess:

php_value upload_max_filesize 3M
php_value post_max_size 3M

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 7, 2018

Member

Maybe we are in the presence of an intermitent error or something that just happens in some setups.

This comment has been minimized.

@mirka

mirka Jun 12, 2018

Contributor

I did a quick check of all the things we’re setting as @wordpress-prefixed externals in the webpack config, to see if they can be imported correctly from utils/index.js. The majority of them cannot. Here are the results in case someone has a clue or encounters the same issue:

Can be imported:
- editor
- blob
- deprecated
- dom
- api-request
- a11y

Can’t be imported:
- blocks
- components
- viewport
- plugins
- edit-post
- core-blocks
- nux
- data
- date
- element
- core-data
- dom-ready
- hooks
- i18n
- is-shallow-equal

Does this need a bug report? This won't be a problem for this PR since we're moving mediaupload.js out of utils, but it could potentially create some hard-to-catch bugs down the line, since they don't trigger an error immediately on build.

This comment has been minimized.

@gziolo

gziolo Jun 18, 2018

Member

I don't there is anything to report. It all boils down to the fact that each module need to specify its dependencies when enqueuing the scripts. See:
https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L188

array( 'lodash', 'wp-blob', 'wp-deprecated', 'wp-dom', 'wp-api-request' ),

The list you shared more or less aligns with the dependencies set for this module.

In general, we are trying to split utils into smaller modules, so it will no longer existing a few weeks from now.

// Allowed types for the current WP_User
const allowedMimeTypesForUser = get( window, [ '_wpMediaSettings', 'allowedMimeTypes' ] );
const isAllowedMimeTypeForUser = ( fileType ) => {
return includes( allowedMimeTypesForUser, fileType );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 7, 2018

Member

If because of some reason allowedMimeTypesForUser is undefined I think we should allow the upload and not reject the upload with a mimeType error. So maybe we should add here a check to see if allowedMimeTypesForUser is undefined.

This comment has been minimized.

@mirka

mirka Jun 12, 2018

Contributor

Yes, it's doing the check right here, should be good to go 👍

/**
* WordPress dependencies
*/
import { __, sprintf } from '../node_modules/@wordpress/i18n';

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

With all the changes introduced in here (translations, WordPress specific settings), I don't think this file makes sense in utils module anymore. Should we move it to the editor module? and keep it as an internal helper for editorMediaUpload? By the way, is it used somewhere else?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 7, 2018

Member

I agree with moving mediaUpload to the editor module, and given that mediaUpload is not used anywhere besides editorMediaUpload probably the two functions can be consolidated together.
If doing this we should leave mediaUpload as it was in utils for 2 versions with a deprecation message.

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

That would be great. I was planning to move it out of utils anyway. We are getting rid of this module as it doesn't make sense as an npm package. See #3955 if you want to see more details about it.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 18, 2018

@mirka @gziolo @jorgefilipecosta What's remaining for this to land?

@danielbachhuber danielbachhuber self-requested a review Jun 18, 2018

@danielbachhuber

I've verified this works as expected locally.

@gziolo

There is one outstanding blocker described in https://github.com/WordPress/gutenberg/pull/6968/files#r193871778 - utils can't import i18n functions using the exsisting statement:

import { __, sprintf } from '../node_modules/@wordpress/i18n';

This will bundle i18n directly into utils module causing code duplication.

There are also a few things which will have to be fixed before we can move forward with publishing packages to npm (related PR #3955):

  • we are deprecating utils module, so we shouldn't be adding new logic in there, instead we should move it to editor module
  • code shouldn't introduce new globals like get( window, [ '_wpMediaSettings', 'allowedMimeTypes' ] ), we should a better way to set those settings on the editor instance.

At least the first comment should be fixed before we land this PR.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 18, 2018

There is one outstanding blocker described in https://github.com/WordPress/gutenberg/pull/6968/files#r193871778 - utils can't import i18n functions using the exsisting statement:

@gziolo How can this be fixed?

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 18, 2018

I guess a quick fix would be to set i18n as dependency of utils in PHP when registering.

danielbachhuber added some commits Jun 18, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 18, 2018

I guess a quick fix would be to set i18n as dependency of utils in PHP when registering.

Updated in fcff0f0

@gziolo

gziolo approved these changes Jun 19, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 19, 2018

@danielbachhuber, yep, this is exactly what we need to proceed 👍

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 19, 2018

@mirka Thanks for all of your work on this!

@danielbachhuber danielbachhuber merged commit 8557d17 into WordPress:master Jun 19, 2018

2 checks passed

codecov/project 46.74% (+0.05%) compared to a02707d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment