WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 weeks ago

#36308 assigned defect (bug)

get_attached_file() destroys file paths on Windows

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

Description

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 9 months ago.
36308.2.diff (723 bytes) - added by stevenlinx 7 months ago.
36308.3.diff (2.7 KB) - added by stevenlinx 2 months ago.
36308.4.diff (4.0 KB) - added by birgire 2 months ago.

Download all attachments as: .zip

Change History (14)

#1 @SergeyBiryukov
2 years ago

  • Component changed from General to Media

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


10 months ago

#3 @desrosj
10 months ago

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

As noted above, related #24824.

@stevenlinx
9 months ago

#4 @stevenlinx
9 months ago

  • Keywords has-patch added; needs-patch removed

#5 @birgire
7 months ago

There is actually a core function called path_is_absolute():

https://developer.wordpress.org/reference/functions/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():

https://developer.wordpress.org/reference/functions/path_join/

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

Last edited 7 months ago by birgire (previous) (diff)

@stevenlinx
7 months ago

#6 @DrewAPicture
4 months 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 4 months ago by DrewAPicture (previous) (diff)

@stevenlinx
2 months ago

#7 @stevenlinx
2 months ago

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

#8 @birgire
2 months 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.

@birgire
2 months ago

#9 @stevenlinx
2 months ago

1.) Thanks @birgire

2.) @DrewAPicture,

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.


4 weeks ago

Note: See TracTickets for help on using tickets.