Opened 5 years ago
Last modified 3 years ago
#36308 assigned defect (bug)
get_attached_file() destroys file paths on Windows
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (14)
This ticket was mentioned in Slack in #core-media by desrosj. View the logs.
3 years ago
#5
@
3 years 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()
.
#6
@
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.
#8
@
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.
#9
@
3 years 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).
As noted above, related #24824.