Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#30123 closed defect (bug) (fixed)

Custom Mime Type filter problem in upload.php list view

Reported by: csschris's profile csschris Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch
Focuses: administration Cc:

Description

The issue is that the option values in the select box for attachment-filters are being passed through urlencode on page render and form submission.

I describe the problem with more detail here

This only happens in list view.

Attachments (4)

class-wp-media-list-table.diff.txt (1.6 KB) - added by birgire 9 years ago.
class-wp-media-list-table2.diff.txt (1.3 KB) - added by birgire 9 years ago.
class-wp-media-list-table.diff (1.3 KB) - added by birgire 9 years ago.
sorry, hopefully this is the correct .diff file, not the txt version as my previous ones ;-)
class-wp-media-list-table2.diff (1.3 KB) - added by birgire 9 years ago.
sorry, hopefully this is the correct .diff file, not the txt version as my previous ones ;-)

Download all attachments as: .zip

Change History (13)

#1 @birgire
9 years ago

As also reported independently here:

http://wordpress.stackexchange.com/questions/166530/post-mime-types-filter-not-working-in-list-mode

there's a potential problem when filtering custom post mime types, in the list mode.

When we register a custom post mime type, as suggested:

add_filter( 'post_mime_types', function( $post_mime_types ) {
    $post_mime_types['application/pdf'] = array( __( 'PDFs' ), __( 'Manage PDFs' ), _n_noop( 'PDF <span class="count">(%s)</span>', 'PDFs <span class="count">(%s)</span>' ) );
    return $post_mime_types;
});

then it will add the following option to the select box for attachments filters:

<option value="post_mime_type:application%2Fpdf" selected="selected">PDF (1)</option>

When we submit this option, the relevant GET request becomes:

&attachment-filter=post_mime_type%3Aapplication%252Fpdf

The reason for this is the urlencode part:

'<option value="post_mime_type:' . urlencode( $mime_type )

Check out this source line: https://core.trac.wordpress.org/browser/tags/4.0/src/wp-admin/includes/class-wp-media-list-table.php#L73

This wasn't a problem in WP 3.9 because it wasn't encoded:

$type_links[$mime_type] = "<a href='upload.php?post_mime_type=$mime_type'$class>"

See this code line in version 3.9: https://core.trac.wordpress.org/browser/tags/3.9/src/wp-admin/includes/class-wp-media-list-table.php#L67

I wonder if we could use esc_attr() instead of urlencode():

'<option value="post_mime_type:' . esc_attr( $mime_type )

so the HTML of the option becomes:

<option value="post_mime_type:application/pdf" selected="selected">PDF (1)</option>

and the GET request becomes:

&attachment-filter=post_mime_type%3Aapplication%2Fpdf

which works as expected.

Ps:

Another idea would be to use the sanitize_mime_type core function:

'<option value="post_mime_type:' . sanitize_mime_type( $mime_type )

where:

function sanitize_mime_type( $mime_type ) {
        $sani_mime_type = preg_replace( '/[^-+*.a-zA-Z0-9\/]/', '', $mime_type );
        /**
         * Filter a mime type following sanitization.
         *
         * @since 3.1.3
         *
         * @param string $sani_mime_type The sanitized mime type.
         * @param string $mime_type      The mime type prior to sanitization.
         */
        return apply_filters( 'sanitize_mime_type', $sani_mime_type, $mime_type );
}
Last edited 9 years ago by birgire (previous) (diff)

@birgire
9 years ago

sorry, hopefully this is the correct .diff file, not the txt version as my previous ones ;-)

@birgire
9 years ago

sorry, hopefully this is the correct .diff file, not the txt version as my previous ones ;-)

#2 @birgire
9 years ago

  • Keywords has-patch added

#3 @wonderboymusic
9 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Owner set to wonderboymusic
  • Status changed from new to assigned

#4 @wonderboymusic
9 years ago

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

In 30661:

Use sanitize_mime_type() instead of urlencode() when string-building <option> values in ->get_views() for media list tables.

urlencode() was introduced in [28553] via _duck and yours truly.

Got weirder after [29625].

Props birgire.
Fixes #30123.

#5 @SergeyBiryukov
9 years ago

#30520 was marked as a duplicate.

#6 @mdgl
9 years ago

Note that the chosen solution here using sanitize_mime_type() breaks when presented with a comma-separated list of MIME types which is a valid return from get_post_mime_types(). The originally suggested fix of using esc_attr() works properly and seems more correct to me. See #30788.

#7 @birgire
9 years ago

@mdgl that's interesting.

To handle a comma sepereated list of mime types, like

application/x-pdf, application/pdf 

I think it would be logical to extend sanitize_mime_type to support this.

For example by adding a single comma (,) into the preg_replace part of that function.

So instead of this line of the sanitize_mime_type() function:

$sani_mime_type = preg_replace( '/[^-+*.a-zA-Z0-9\/]/', '', $mime_type );

we could instead use this:

$sani_mime_type = preg_replace( '/[^-+*.,a-zA-Z0-9\/]/', '', $mime_type );

#8 @mdgl
9 years ago

Hello birgire, yes, changing sanitize_mime_type() as you suggest would fix the problem, but I don't think this is the right way to go. The comma is not valid in (simple) MIME type names [see https://www.ietf.org/rfc/rfc2045.txt and http://www.rfc-editor.org/rfc/rfc2231.txt] and by changing this function we might be introducing problems elsewhere.

The bug here is simply that the value needs escaping as it is being used inside a HTML attribute, so I think your original suggestion of using esc_attr() is the correct solution.

#9 @birgire
9 years ago

Allowing the extra comma in sanitize_mime_type() would be like working with sanitize_mime_types() instead.

But if not, then I guess we could use the esc_attr() function as suggested ;-)

Version 1, edited 9 years ago by birgire (previous) (next) (diff)
Note: See TracTickets for help on using tickets.