get_attached_file() destroys file paths on Windows

[Whissi]

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.

[desrosj]

As noted above, related #24824.

[birgire]

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().

[DrewAPicture]

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.

[birgire]

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.

[stevenlinx]

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).

Trac ticket: https://core.trac.wordpress.org/ticket/36308.

[davidbaumwald]

Refreshed the latest patch with ​PR #2442. The magic sauce here is the path_join which uses path_is_absolute to make sure the root path isn't duplicated in the resulting path.

I was able to verify this fixes the issue. To test on Windows:

1. Create a media item.
2. Set its _wp_attached_file meta to something like C:/WWW/Sites/demo/htdocs/wordpress/wp-content/uploads/2016/03/example.jpg.
3. Call get_attached_file for the attachment ID.

Pre-patch, you should see a duplicated root in the path, as reported originally. Post-patch, the path should be just what you set in as _wp_attached_file.

Since we're messing with file paths here, I'd appreciate a good looks at this by some more experienced contributors/committers, even with the added tests.

[costdev]

Per the discussion in the bug scrub, adding has-testing-info and needs-testing for extra confidence after David's tests were successful.

See this comment for testing instructions.

As noted in David's comment, further feedback on this is welcome.

[costdev]

This ticket was discussed in the recent bug scrub.

@peterwilsoncc will be asking folks from the Security Team to take a look at this one too.

[peterwilsoncc]

I shared this in the security channel and no one freaked out so I think it's good for commit.

[peterwilsoncc]

File path related cahnges are always risky so let's do this a few days after 6.1 forks rather than a few days before the 6.0 RC.

[hellofromTonya]

If this get committed early, then there should be plenty of soak time throughout 6.1 for additional testing and feedback. Overall the PR LGTM. IMO it's ready for early early early (like now) commit. In discussing during the Media weekly chat, @antpb will prep the commit this week.

[audrasjb]

There’s a requested change on the PR, so let’s remove commit workflow keyword.

This PR refreshes ​PR 2442 to use a data provider to stabilise the new test method.

Trac ticket:

[costdev]

​PR 2442 is ready for committer review.

This looks good and there's a good amount of test coverage, but I'd also like to see the tests in Tests_Post_GetAttachedFile use a data provider 👍

Hi @johnbillion, see ​PR 3049 which is a refresh of this PR that uses a data provider in Test_Post_GetAttachedFile

Closing in favour of #3049 👍

Closing in favour of #3049 👍

[johnbillion]

​PR 3049 looks good to me but I'd like another nod from @antpb just to be sure.

[antpb]

In 53934:

Media: Account for Windows when normalizing file paths.
Previously, Windows paths in the path_is_absolute function resulted in incorrect URIs. This patch adjusts for forward slashes and adds tests for the get_attached_file function.
Props Whissi, SergeyBiryukov, desrosj, stevenlinx, birgire, davidbaumwald, costdev, peterwilsoncc, audrasjb, hellofromTonya, johnbillion.
Fixes #36308.

[jrf]

Uh oh... commit [53934] introduces 10 new test failures on PHP 8.1.

Failures are along the lines of:

1) Tests_oEmbed_Response_Data::test_get_oembed_response_data_with_thumbnail
ValueError: realpath(): Argument #1 ($path) must not contain any null bytes

path/to/src/wp-includes/functions.php:2104
path/to/src/wp-includes/functions.php:2139
path/to/src/wp-includes/post.php:731
path/to/src/wp-includes/post.php:6833
path/to/src/wp-includes/post.php:6886
path/to/src/wp-includes/media.php:192
path/to/src/wp-includes/media.php:954
path/to/src/wp-includes/media.php:1032
path/to/src/wp-includes/post.php:7802
path/to/tests/phpunit/tests/oembed/getResponseData.php:244

(note: line numbers may be off by a few lines as I'm working in a branch with a lot of other patches)

I'm currently investigating the issue, but suspect that the change to the first if in the get_attached_file() function is the culprit.

[jrf]

Note: I'm running the tests on Windows and seeing the errors locally. The errors are not showing in the GH Actions logs as the tests on GHA are run on Ubuntu.

Then again: as this ticket is about Windows support....

[jrf]

Minor CS fixes for path_is_absolute()

[jrf]

@antpb and me just got together in a call to investigate.

Preliminary conclusion: it's not so much this patch which is the problem, this patch just exposed a (much larger) underlying problem with slash normalization and Windows slashes getting removed from paths and more.

The fact that I discovered this now has more to do with an edge case set-up on my Windows machine (a subdirectory in the path to WP starting with a numeric 0), though that doesn't negate the underlying issue (it just helped to discover it).

@antpb will write up a new issue for this.

While looking at the code, we saw two small coding standards issues which should be fixed. There weren't introduced in this patch, but were in the functions touched in the patch.

I've uploaded a patch file with the fixes for those.

[davidbaumwald]

@antpb Will you be handling the follow-up commit for the CS issues? (This ticket popped up in a scrub)

[audrasjb]

Removing early and needs-testing workflow keyword as this patch was previously committed.

[antpb]

@davidbaumwald hi yes, I am handling the commit on this tomorrow

[antpb]

In 53946:

Coding Standards: Use strict comparisons in path_is_absolute().

This patch adjusts conditions to use strict comparisons when comparing realpath() in path_is_absolute().

Props jrf.
See #36308.

[desrosj]

#56924 raised performance concerns on certain filesystem set ups. After some discussion, it was decided that [53934] should be reverted and removed from 6.1 so more testing could be done.

Reopening to try again in 6.2.

[azaozz]

Replying to desrosj:

May I also suggest that this problem (and perhaps a link to https://core.trac.wordpress.org/ticket/56924) is mentioned on the docblock of path_is_absolute(), and perhaps path_join(). Seems the latter is used quite often and this slowdown may be causing other problems too.

Also, probably worth it to improve path_is_absolute() when called for paths on NFS shares.

[antpb]

This ticket needs a lot of testing and an update before commit. Moving this to 6.3 as it is not feeling like it has enough time for 6.2

[antpb]

It was discussed in the Media meeting that we really need someone strong in Windows filesystem experience to help get this to a state where we feel confident. Given the performance impact we saw from this change, we feel a bit less confident than last time.

With this in mind we should move this to future release until someone with more experience can lend a hand. :)