-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Check allowed mime types before uploading media #6968
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
Changes from 4 commits
d044de0
6386d73
8fed14f
5b8f17d
fcff0f0
889c592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,12 @@ | ||
| /** | ||
| * External Dependencies | ||
| */ | ||
| import { compact, forEach, get, noop, startsWith } from 'lodash'; | ||
| import { compact, forEach, get, includes, noop, startsWith } from 'lodash'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __, sprintf } from '../node_modules/@wordpress/i18n'; | ||
|
||
|
|
||
| /** | ||
| * Media Upload is used by audio, image, gallery and video blocks to handle uploading a media file | ||
|
|
@@ -33,15 +38,41 @@ export function mediaUpload( { | |
| filesSet[ idx ] = value; | ||
| onFileChange( compact( filesSet ) ); | ||
| }; | ||
|
|
||
| // Allowed type specified by consumer | ||
| const isAllowedType = ( fileType ) => startsWith( fileType, `${ allowedType }/` ); | ||
|
|
||
| // Allowed types for the current WP_User | ||
| const allowedMimeTypesForUser = get( window, [ '_wpMediaSettings', 'allowedMimeTypes' ] ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided against it because although it would make testing simpler, I didn't think it made sense to expose 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| const isAllowedMimeTypeForUser = ( fileType ) => { | ||
| return includes( allowedMimeTypesForUser, fileType ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's doing the check right here, should be good to go 👍 |
||
| }; | ||
|
|
||
| files.forEach( ( mediaFile, idx ) => { | ||
| if ( ! isAllowedType( mediaFile.type ) ) { | ||
| return; | ||
| } | ||
|
|
||
| // verify if user is allowed to upload this mime type | ||
| if ( allowedMimeTypesForUser && ! isAllowedMimeTypeForUser( mediaFile.type ) ) { | ||
| onError( { | ||
| code: 'MIME_TYPE_NOT_ALLOWED_FOR_USER', | ||
| message: __( 'Sorry, this file type is not permitted for security reasons.' ), | ||
| file: mediaFile, | ||
| } ); | ||
| return; | ||
| } | ||
|
|
||
| // verify if file is greater than the maximum file upload size allowed for the site. | ||
| if ( maxUploadFileSize && mediaFile.size > maxUploadFileSize ) { | ||
| onError( { sizeAboveLimit: true, file: mediaFile } ); | ||
| onError( { | ||
| code: 'SIZE_ABOVE_LIMIT', | ||
| message: sprintf( | ||
| __( '%s exceeds the maximum upload size for this site.' ), | ||
| mediaFile.name | ||
| ), | ||
| file: mediaFile, | ||
| } ); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -66,7 +97,14 @@ export function mediaUpload( { | |
| () => { | ||
| // Reset to empty on failure. | ||
| setAndUpdateFiles( idx, null ); | ||
| onError( { generalError: true, file: mediaFile } ); | ||
| onError( { | ||
| code: 'GENERAL', | ||
| message: sprintf( | ||
| __( 'Error while uploading file %s to the media library.' ), | ||
| mediaFile.name | ||
| ), | ||
| file: mediaFile, | ||
| } ); | ||
| } | ||
| ); | ||
| } ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't figure out why it's unable to resolve the module normally with just
from '@wordpress/i18n'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wordpress/i18nlives insideWordPress/packagesso 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we are in the presence of an intermitent error or something that just happens in some setups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromutils/index.js. The majority of them cannot. Here are the results in case someone has a clue or encounters the same issue: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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
The list you shared more or less aligns with the dependencies set for this module.
In general, we are trying to split
utilsinto smaller modules, so it will no longer existing a few weeks from now.