Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#19399 closed defect (bug) (fixed)

New uploader doesn't show if the options for large image sizes are not numbers

Reported by: westi Owned by: azaozz
Milestone: 3.3 Priority: high
Severity: major Version: 3.3
Component: Upload Keywords: has-patch
Focuses: Cc:


Error: syntax error
Source File: ...wp-admin/media-upload.php?post_id=xxx&
Line: 132, Column: 20
Source Code:
var resize_height = , resize_width = , 

We blindly output the data returned from get_option:

We should:

  • Validate it is an integer
  • Sanitize what we output

Attachments (5)

19399.patch (700 bytes) - added by mhauan 6 years ago.
19399-2.patch (832 bytes) - added by mhauan 6 years ago.
19399-3.patch (892 bytes) - added by mhauan 6 years ago.
19399-4.patch (858 bytes) - added by mhauan 6 years ago.
19399-5.patch (840 bytes) - added by mhauan 6 years ago.

Download all attachments as: .zip

Change History (17)

#2 @mhauan
6 years ago

  • Keywords has-patch added; needs-patch removed

Check that the get_option is a whole number before echoing, and if not echo 1024 that is the standard WordPress size.

6 years ago

#3 @mhauan
6 years ago

Replaced preg_replace() with absint()

6 years ago

#4 @mhauan
6 years ago

New patch without inline code.

#5 @nacin
6 years ago

Patch won't work quite right. That doesn't sanitize get_option( 'large_size_h' ), it only confirms it can be sanitized to a non-zero value.

You want:

$large_size_h = absint( get_option('large_size_h') );
if ( ! $large_size_h )
    $large_size_h = 1024;

6 years ago

#6 @mhauan
6 years ago

Added new patch. absint( '3asdf' ) will return int(3), and I added a preg_match that checks that the option only contains numbers.

#7 @ryan
6 years ago

Would a simple is_numeric() suffice?

#8 @mhauan
6 years ago

Probably, but "1e4" and "9.6" is also numeric. I not sure if that would break the upload script. The size should be a whole number. It might be better to use the ctype_digit() function then?

6 years ago

#9 @mhauan
6 years ago

New patch using ctype_digit() instead of preg_match().

#10 @nacin
6 years ago

We can't use ctype_digit(), as it's not available everywhere.

I think absint() is sufficient. We do it all over the place to fine effect. A bit of sanitization followed by validating that it is not 0 (if such a value is invalid in that context).

sanitize_option() sanitizes all of these options with absint() anyway, which leads me to A) wonder how westi did this :-) and B) not at all care about this beyond the JS error and empty/0 values.

6 years ago

#11 @mhauan
6 years ago

New patch based on nacin's idea. You're right, those values shouldn't be in to the database anyways. What if someone edit that option with update_option() in a plugin or something and caused an error?

#12 @azaozz
6 years ago

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

In [19494]:

Sanitize options for resizing in the uploader, props mhauan, nacin, fixes #19399

Note: See TracTickets for help on using tickets.