Skip to content
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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions editor/utils/editor-media-upload/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { noop } from 'lodash';
*/
import { select } from '@wordpress/data';
import { mediaUpload } from '@wordpress/utils';
import { __, sprintf } from '@wordpress/i18n';

/**
* Upload a media file when the file upload button is activated.
Expand All @@ -31,22 +30,6 @@ export default function editorMediaUpload( {
} ) {
const postId = select( 'core/editor' ).getCurrentPostId();

const errorHandler = ( { file, sizeAboveLimit, generalError } ) => {
let errorMsg;
if ( sizeAboveLimit ) {
errorMsg = sprintf(
__( '%s exceeds the maximum upload size for this site.' ),
file.name
);
} else if ( generalError ) {
errorMsg = sprintf(
__( 'Error while uploading file %s to the media library.' ),
file.name
);
}
onError( errorMsg );
};

mediaUpload( {
allowedType,
filesList,
Expand All @@ -55,6 +38,6 @@ export default function editorMediaUpload( {
post: postId,
},
maxUploadFileSize,
onError: errorHandler,
onError: ( { message } ) => onError( message ),
} );
}
5 changes: 3 additions & 2 deletions lib/client-assets.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function gutenberg_register_scripts_and_styles() {
wp_register_script(
'wp-utils',
gutenberg_url( 'build/utils/index.js' ),
array( 'lodash', 'wp-blob', 'wp-deprecated', 'wp-dom', 'wp-api-request' ),
array( 'lodash', 'wp-blob', 'wp-deprecated', 'wp-dom', 'wp-api-request', 'wp-i18n' ),
filemtime( gutenberg_dir_path() . 'build/utils/index.js' ),
true
);
Expand Down Expand Up @@ -1048,7 +1048,8 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
}
// Initialize media settings.
wp_add_inline_script( 'wp-editor', 'window._wpMediaSettings = ' . wp_json_encode( array(
'maxUploadSize' => $max_upload_size,
'maxUploadSize' => $max_upload_size,
'allowedMimeTypes' => get_allowed_mime_types(),
) ), 'before' );

// Prepare Jed locale data.
Expand Down
44 changes: 41 additions & 3 deletions utils/mediaupload.js
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 '@wordpress/i18n';

/**
* WordPress dependencies
Expand Down Expand Up @@ -38,15 +43,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' ] );
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const isAllowedMimeTypeForUser = ( fileType ) => {
return includes( allowedMimeTypesForUser, fileType );
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}

Expand All @@ -69,7 +100,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,
} );
}
);
} );
Expand Down
26 changes: 24 additions & 2 deletions utils/test/mediaupload.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe( 'mediaUpload', () => {
expect( console.error ).not.toHaveBeenCalled();
} );

it( 'should call error handler with the correct message if file size is greater than the maximum', () => {
it( 'should call error handler with the correct error object if file size is greater than the maximum', () => {
const onError = jest.fn();
mediaUpload( {
allowedType: 'image',
Expand All @@ -54,6 +54,28 @@ describe( 'mediaUpload', () => {
maxUploadFileSize: 512,
onError,
} );
expect( onError.mock.calls ).toEqual( [ [ { sizeAboveLimit: true, file: validMediaObj } ] ] );
expect( onError ).toBeCalledWith( {
code: 'SIZE_ABOVE_LIMIT',
file: validMediaObj,
message: `${ validMediaObj.name } exceeds the maximum upload size for this site.`,
} );
} );

it( 'should call error handler with the correct error object if file type is not allowed for user', () => {
const onError = jest.fn();
global._wpMediaSettings = {
allowedMimeTypes: { aac: 'audio/aac' },
};
mediaUpload( {
allowedType: 'image',
filesList: [ validMediaObj ],
onFileChange,
onError,
} );
expect( onError ).toBeCalledWith( {
code: 'MIME_TYPE_NOT_ALLOWED_FOR_USER',
file: validMediaObj,
message: 'Sorry, this file type is not permitted for security reasons.',
} );
} );
} );