Make WordPress Core

Opened 5 years ago

Last modified 2 weeks ago

#36308 assigned defect (bug)

get_attached_file() destroys file paths on Windows

Reported by: Whissi Owned by: stevenlinx
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.4.2
Component: Media Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:


While working on ticket #36273 I noticed that get_attached_file() from wp-includes/post.php will destroy paths normalized by wp_normalize_path() on Windows:

For example the function starts with

$file = get_post_meta( $attachment_id, '_wp_attached_file', true );
// $file = 'C:/WWW/Sites/demo/htdocs/wordpress/wp-content/uploads/2016/03/example.jpg'

However this will become

$file = 'C:\WWW\Sites\demo\htdocs\wordpress/wp-content/uploads/C:/WWW/Sites/demo/htdocs/wordpress/wp-content/uploads/2016/03/example.jpg'

due to

if ( $file && 0 !== strpos($file, '/') && !preg_match('|^.:\\\|', $file) && ( ($uploads = wp_upload_dir()) && false === $uploads['error'] ) )
        $file = $uploads['basedir'] . "/$file";

This is similar to ticket #24824 however we are dealing will full qualified paths here, not URLs (well, both are URIs...).

PS: Yes, $uploads['basedir'] contains mixed directory separators. That's another thing.

Attachments (4)

36308.diff (1.0 KB) - added by stevenlinx 4 years ago.
36308.2.diff (723 bytes) - added by stevenlinx 4 years ago.
36308.3.diff (2.7 KB) - added by stevenlinx 3 years ago.
36308.4.diff (4.0 KB) - added by birgire 3 years ago.

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
5 years ago

  • Component changed from General to Media

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.

4 years ago

#3 @desrosj
4 years ago

  • Keywords needs-patch needs-unit-tests good-first-bug added

As noted above, related #24824.

4 years ago

#4 @stevenlinx
4 years ago

  • Keywords has-patch added; needs-patch removed

#5 @birgire
4 years ago

There is actually a core function called path_is_absolute():


so I wonder why it isn't used in get_attached_file(), like:

if ( ! path_is_absolute( $file ) && ( ( $uploads = wp_get_upload_dir()) && false === $uploads['error'] ) )
        $file = $uploads['basedir'] . "/$file";

In that case path_is_absolute() could be further updated to handle url or normalized paths.

There's also the path_join():


that uses path_is_absolute() internally. This is e.g. used in _wp_upload_dir().

Last edited 4 years ago by birgire (previous) (diff)

4 years ago

#6 @DrewAPicture
3 years ago

  • Owner set to stevenlinx
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

@stevenlinx Thanks for the patches! It likes we're still in need of unit tests here.

Last edited 3 years ago by DrewAPicture (previous) (diff)

3 years ago

#7 @stevenlinx
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#8 @birgire
3 years ago

In 36308.4.diff I adjusted 36308.3.diff according to the Coding Standard and added more tests to the test_path_is_absolute() test method.

3 years ago

#9 @stevenlinx
3 years ago

Thanks @birgire


I assume we should also commit ticket #24824 at the same time? since the two are quite related (as noted above).

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.

3 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.

2 weeks ago

#12 @SergeyBiryukov
2 weeks ago

  • Milestone changed from Awaiting Review to 5.9
Note: See TracTickets for help on using tickets.