WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32746 closed defect (bug) (fixed)

Media Upload mimeType validation bug

Reported by: vivekbhusal Owned by: wonderboymusic
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.2.2
Component: Media Keywords: close
Focuses: javascript Cc:
PR Number:

Description

I am working on plugin that lets user upload/select doc or docx file and followed by running task to parse those file and create post.

Problem: I am using media uploader (wp.media.frame) to select or upload files of only mimetype
["application/vnd.openxmlformats-officedocument.wordprocessingml.document", "application/msword"]

user can choose and upload the file, but after the file is uploaded, the mediaframe should be updated to display the newly uploaded file asynchronously, but it doesn't. However it does if I am only one mimeType.
Here is the Code I use for media upload

 // If the media frame already exists, reopen it.
        var alex_doc_uploader = $thisButton.data('file_frame');
        if (alex_doc_uploader) {
            alex_doc_uploader.open();
            return;
        }

        // Create the media frame.
        alex_doc_uploader = wp.media.frames.file_frame = wp.media({
            title: "Select Documents",
            button: {
                text: "Select Documents"
            },
            library : {
              type:   ["application/vnd.openxmlformats-officedocument.wordprocessingml.document", "application/msword"]
            },
            multiple: true  // Set to true to allow multiple files to be selected
        });



        $thisButton.data('file_frame', alex_doc_uploader);


        /** NOT very relevant after this but still adding it here**/
        alex_doc_uploader.on('select', function(){
            var selection = alex_doc_uploader.state().get('selection');

            selection.map( function(attachment) {
                if(attachment.attributes.filename)
                    attachments.push({'id':attachment.id, 'attachment':attachment.attributes.filename});
            });

            if(attachments.length > 0) {
                $("#alex-doc-import-selector").hide();
                $('#alex-doc-file-insert').show();

                $.each(attachments, function(index, value) {
                    $("#alex-import-selected-file-type").append("<li data-id='"+index+"'>"+value.attachment+"</li>");
                });
                $("#alex-doc-popup-container").dialog('open');
            }
        });

Issue explained here aswell: StackOverfflow

So on tracking the problem I found, after upload it verifies the validity of newly added content on media-models.js .
In the process calls filter.type function at line 897 media-models.js

/**
		 * @static
		 * @param {wp.media.model.Attachment} attachment
		 *
		 * @this wp.media.model.Attachments
		 *
		 * @returns {Boolean}
		 */
		type: function( attachment ) {
			var type = this.props.get('type');
			return ! type || -1 !== type.indexOf( attachment.get('type') );
		},

Here it get attachment.get(type) as "application" so it works when using one mimeType, because it uses index of to compare in string, but won't work for mimeType array.

Here is the my log,
var type = ["application/vnd.openxmlformats-officedocument.wordprocessingml.document", "application/msword"]
attachment.get("type") gives "application"

and attachment object is
Screenshot of attachment object on console

Solution:

                 /**
                 * @since 4.2.3
		 * @static
		 * @param {wp.media.model.Attachment} attachment
		 *
		 * @this wp.media.model.Attachments
		 *
		 * @returns {Boolean}
		 */
		type: function( attachment ) {
			var type = this.props.get('type');
			return ! type || -1 !== type.indexOf( attachment.get('mime') );
		},

Attachments (3)

attachmentobject.png (120.7 KB) - added by vivekbhusal 4 years ago.
Screenshot of attachment object on console
32746.patch (2.9 KB) - added by vivekbhusal 4 years ago.
When filtering media attachment by type in wp.media.model.Attachments.filters.type(), checks for type being array or string and validades accordingly
32746.diff (1.6 KB) - added by wonderboymusic 4 years ago.

Download all attachments as: .zip

Change History (14)

@vivekbhusal
4 years ago

Screenshot of attachment object on console

#1 follow-up: @wonderboymusic
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3

Shouldn't it be:

var type = this.props.get('type');
return ! type || -1 !== attachment.get('mime').indexOf( type );

Can you try that and see if it works?

#2 @wonderboymusic
4 years ago

The reason being: type can be abbreviated - image/jpeg or just image

#3 @wonderboymusic
4 years ago

Actually, indexOf is polymorphic, so the original issue is that an array of full mime-types was passed to type, it was looking for an abbreviated type in a list of full types.

#4 @wonderboymusic
4 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 32915:

When filtering media by type in wp.media.model.Attachments.filters.type(), account for the library's type being an array of full mime-types.

Fixes #32746.

#5 follow-up: @wonderboymusic
4 years ago

In 32916:

Because programming is fun. After [32915], _.find() will return undefined if an empty array is passed - in that case, the function should return true.

See #32746.

#6 in reply to: ↑ 1 @vivekbhusal
4 years ago

Replying to wonderboymusic:

Shouldn't it be:

var type = this.props.get('type');
return ! type || -1 !== attachment.get('mime').indexOf( type );

Can you try that and see if it works?

It should be type.indexOf(attachment.get("mime");
"type" being the array of all supported mime types and attachment.get("mime") being the mime type of current file.

#7 in reply to: ↑ 5 @vivekbhusal
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to wonderboymusic:

In 32916:

Because programming is fun. After [32915], _.find() will return undefined if an empty array is passed - in that case, the function should return true.

See #32746.

Agree with your patch but there are some fundamental issues.
First thing, we don't need to convert attachment to JSON everytime, its just added overhead,
Second if we are gonna compare our new file type to provided mime type and also it should support abbreviated file type, we don't require mimetype of file.
Third, if above everything the type doesn't exist you have nothing to worry about and can return at the beginning.

Its just added minor condition check, whether the type is array or not. The fundamental logic would be still same. This will work better

           type: function( attachment ) {

                if( ! this.props.get("type") ) {
                    return true;
                }

                var type = this.props.get('type'),
                    attachmentType = attachment.get("type"), found;

                if ( _.isArray( type ) ) {
                    found = _.find( type, function (t) {
                            return -1 !== t.indexOf( attachmentType );
                    } );
                } else {
                    found = -1 !== type.indexOf( attachmentType );
                }

                return found;

            },

@vivekbhusal
4 years ago

When filtering media attachment by type in wp.media.model.Attachments.filters.type(), checks for type being array or string and validades accordingly

#8 @obenland
4 years ago

  • Keywords has-patch added; needs-patch removed

@wonderboymusic could you see if the patch lets you finish this ticket?

@wonderboymusic
4 years ago

#9 @wonderboymusic
4 years ago

@vivekbhusal some thoughts:

  • 32746.diff are the changes I think we should make. If you're running the SVN repo, you can run grunt precommit before making your patch and it will alert you to JS coding style changes that need to made
  • I think calling .toJSON() is fine - internally, in Backbone, it is just a getter for model.attributes, which is more forward-compatible that directly accessing the property
  • The abbreviated mime is supported - type is just the first segment of the mime-type. If your library declares type as an abbreviated version, it will match. The difference we made in [32915] is that the exact mime-type will also match. The type for these MS docs is actually application, which could match several false-positives. I believe the problem before was this: we were trying to match full mime-types against type, which would never return true. Also, because indexOf() is polymorphic (works on arrays and strings), and we weren't checking the type of type, we were getting inconsistent results.

#10 @wonderboymusic
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 33091:

In wp.media.model.Attachments.filters.type(), return true earlier if type isn't set.

Props vivekbhusal.
Fixes #32746.

#11 @vivekbhusal
4 years ago

  • Keywords close added; has-patch removed

@wonderboymusic: I will check that next time, thanks for that. Supporting both mimetype and then type makes more sense as it reduces false positive.

Note: See TracTickets for help on using tickets.