Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#37608 closed defect (bug) (fixed)

Undefined index: extension in class-wp-image-editor.php

Reported by: jaworskimatt's profile JaworskiMatt Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.7 Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: good-first-bug has-patch
Focuses: Cc:

Description

Under some circumstances, 3rd party plugins will cause the WP_Image_Editor::generate_filename() to print a notice about a missing "extension" index in the $info variable.

It's rather hard to reproduce, but definitely happens inside PeepSo AJAX upload with some photos containing EXIF rotation information. It can also be found by simply googling it, like on this website:

http://www.pasadenamusclecompany.com/tag/featured

Obviously it only happens with WP_DEBUG set to TRUE.

The $info variable is created using PHP's very own pathinfo() like this:

$info = pathinfo( $this->file );
$dir  = $info['dirname'];
$ext  = $info['extension'];

According to PHP documentation, the extension index can be missing (not set) if the file does not have one.

http://php.net/manual/en/function.pathinfo.php

The code above will obviously generate a notice in such a scenario. An easy way around this is NOT to assume the "extension" index is present, for example like this:

$dir  = pathinfo( $this->file, PATHINFO_DIRNAME );
$ext  = pathinfo( $this->file, PATHINFO_EXTENSION );

As far as I can tell, there are no drawbacks to this approach.

Attachments (3)

37608.patch (614 bytes) - added by JaworskiMatt 8 years ago.
Proposed solution
37608.diff (6.4 KB) - added by wonderboymusic 8 years ago.
37608.2.diff (619 bytes) - added by jeremyfelt 8 years ago.

Download all attachments as: .zip

Change History (18)

#1 @JaworskiMatt
8 years ago

  • Severity changed from normal to trivial

Might be somehow connected to #30052

Specifically this comment

https://core.trac.wordpress.org/ticket/30052#comment:7

Last edited 8 years ago by JaworskiMatt (previous) (diff)

#2 @SergeyBiryukov
8 years ago

  • Component changed from General to Media
  • Severity changed from trivial to normal

#3 @ocean90
8 years ago

  • Version changed from trunk to 2.5

Introduced in [7041], moved to WP_Image_Editor::generate_filename() in [22094].

#4 @swissspidy
8 years ago

  • Keywords needs-patch good-first-bug added

@JaworskiMatt
8 years ago

Proposed solution

#5 @JaworskiMatt
8 years ago

I have added a .patch file containing a proposed solution

From what I can see in class-wp-image-editor.php there is at least one case of pathinfo() usage, but in that other case the index extension is being checked with isset()

#6 @JaworskiMatt
8 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

#7 @wonderboymusic
8 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.7
  • Owner set to wonderboymusic
  • Status changed from new to assigned

@wonderboymusic
8 years ago

#8 @wonderboymusic
8 years ago

this is a problem every time pathinfo() is called, see 37608.diff

#9 @wonderboymusic
8 years ago

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

In 38294:

Media: when calling pathinfo(), also pass a PATHINFO_* constant to avoid array notices for unset keys.

Props JaworskiMatt.
Fixes #37608.

@jeremyfelt
8 years ago

#10 follow-up: @jeremyfelt
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In [38294], PATHINFO_BASENAME was used in media_handle_upload() in a spot that had previously stripped the file extension from the basename to build an attachment title. This results in attachment titles of My Title-jpg instead of My Title for an uploaded image of My Title.jpg, which is a change in behavior.

37608.2.diff uses PATHINFO_FILENAME instead.

#11 in reply to: ↑ 10 @joemcgill
8 years ago

Replying to jeremyfelt:

In [38294], PATHINFO_BASENAME was used in media_handle_upload() in a spot that had previously stripped the file extension from the basename to build an attachment title. This results in attachment titles of My Title-jpg instead of My Title for an uploaded image of My Title.jpg, which is a change in behavior.

37608.2.diff uses PATHINFO_FILENAME instead.

Note that I've incorporated the same change into 37989.2.diff, so fixing #37989 will likely resolve this issue as well.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#13 @JanR
8 years ago

Today I noticed something relevant -I think- in the Media Library: strange behavior when a file name has a % sign in it. In a very extreme case, double extension it and name your file filename.html%00.png

On my dev site: after uploading, it has the following Attachment Details:

WordPress trunk

File name: filename.html00.png
File type: image/png
URL https://www.vanilla-wp.org/wp-content/uploads/2016/09/filename.html00.png
Title: filename-html%00-png

and the View attachment URL goes to about:blank in Chrome. The HTML code for the link has an additional /:

<a class="view-attachment" href="https://www.vanilla-wp.org/filename-html%00-png/">View attachment page</a>

Firefox does not have this behavior and the link is: https://www.vanilla-wp.org/filename-html%00-png/ resulting in a HTTP 400 Bad Request - Invalid URL error. (on IIS).

When I remove the double extension, the upload gives me an error message:

filename.html%00 This file type is not allowed. Please try another.

In WordPress 4.6.1 (had to remove this test URL):

File name: filename.html00.png
File type: image/png
URL http://www.vanilla-wp.org/test-4.6/wp-content/uploads/2016/09/filename.html00.png
Title: filename-html%00

After talking to @joemcgill on Slack, I tested WordPress trunk with https://core.trac.wordpress.org/attachment/ticket/37989/37989.3.diff applied:

File name: filename.html00.png
File type: image/png
URL https://www.vanilla-wp.org/wp-content/uploads/2016/09/filename.html00.png
Title: filename.html

The View attachment URL issue is fixed with the applied diff.

Now if you asked me what correct behavior is, I don't know. But I don't think it's a good idea to base the file type from it's last extension (in case of a double extension).

#14 @SergeyBiryukov
7 years ago

The issue with pathinfo() is that it depends on PHP locale and does not always work correctly with UTF-8 characters, leading to issues with truncated titles, see comment:40:ticket:37989.

We've encountered similar issues with basename() before, see #21217 and #23267.

[38294] needs a thorough test with multibyte file names. Some of the pathinfo() instances may need to be replaced with wp_basename(), which is the i18n-friendly version of basename().

#15 @joemcgill
7 years ago

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

In 38673:

Media: Use wp_basename() to create attachment titles from filenames.

In [38294], pathinfo() was used with the PATHINFO_BASENAME constant to
get the basename of the file to be used as an attachment title, which depends
on PHP locale and can cause issues with UTF-8 characters. This uses
wp_basename() instead, which is a more i18n-friendly version of basename().

Props SergeyBiryukov.
Fixes #37608, #37989.

Note: See TracTickets for help on using tickets.