WordPress.org

Make WordPress Core

#37608 closed defect (bug) (fixed)

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

Reported by: JaworskiMatt Owned by: 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 13 months ago.
Proposed solution
37608.diff (6.4 KB) - added by wonderboymusic 12 months ago.
37608.2.diff (619 bytes) - added by jeremyfelt 12 months ago.

Download all attachments as: .zip

Change History (18)

#1 @JaworskiMatt
13 months 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 13 months ago by JaworskiMatt (previous) (diff)

#2 @SergeyBiryukov
13 months ago

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

#3 @ocean90
13 months ago

  • Version changed from trunk to 2.5

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

#4 @swissspidy
13 months ago

  • Keywords needs-patch good-first-bug added

@JaworskiMatt
13 months ago

Proposed solution

#5 @JaworskiMatt
13 months 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
13 months ago

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

#7 @wonderboymusic
12 months ago

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

#8 @wonderboymusic
12 months ago

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

#9 @wonderboymusic
12 months 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
12 months ago

#10 follow-up: @jeremyfelt
12 months 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
11 months 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.


11 months ago

#13 @JanR
11 months 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
11 months 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
11 months 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.