Make WordPress Core

Opened 8 years ago

Last modified 11 months ago

#36308 assigned defect (bug)

get_attached_file() destroys file paths on Windows

Reported by: whissi's profile Whissi Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4.2
Component: Media Keywords: has-unit-tests has-testing-info
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 (5)

36308.diff (1.0 KB) - added by stevenlinx 7 years ago.
36308.2.diff (723 bytes) - added by stevenlinx 7 years ago.
36308.3.diff (2.7 KB) - added by stevenlinx 6 years ago.
36308.4.diff (4.0 KB) - added by birgire 6 years ago.
trac-36308-minor-cs-follow-up.patch (1.2 KB) - added by jrf 20 months ago.
Minor CS fixes for path_is_absolute()

Download all attachments as: .zip

Change History (72)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Media

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


7 years ago

#3 @desrosj
7 years ago

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

As noted above, related #24824.

@stevenlinx
7 years ago

#4 @stevenlinx
7 years ago

  • Keywords has-patch added; needs-patch removed

#5 @birgire
7 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().

Version 1, edited 7 years ago by birgire (previous) (next) (diff)

@stevenlinx
7 years ago

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

@stevenlinx
6 years ago

#7 @stevenlinx
6 years ago

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

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

@birgire
6 years ago

#9 @stevenlinx
6 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).

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


6 years ago

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


3 years ago

#12 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#13 @sabernhardt
2 years ago

  • Milestone changed from 5.9 to 6.0

#15 @davidbaumwald
2 years ago

  • Keywords dev-feedback added

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.

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


2 years ago

#17 @costdev
2 years ago

  • Keywords has-testing-info needs-testing added

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.

Last edited 2 years ago by costdev (previous) (diff)

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


2 years ago

#19 @costdev
2 years ago

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.

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


2 years ago

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


2 years ago

#22 @peterwilsoncc
2 years ago

  • Keywords commit added

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

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


2 years ago

#24 @peterwilsoncc
2 years ago

  • Keywords early added
  • Milestone changed from 6.0 to 6.1

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.

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


2 years ago

#26 @hellofromTonya
2 years ago

  • Owner changed from stevenlinx to antpb
  • Status changed from assigned to reviewing

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.

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


2 years ago

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


2 years ago

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


2 years ago

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


23 months ago

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


23 months ago

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


23 months ago

#33 @audrasjb
23 months ago

  • Keywords commit removed

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

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


22 months ago

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


22 months ago

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


22 months ago

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


22 months ago

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


21 months ago

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


21 months ago

This ticket was mentioned in PR #3049 on WordPress/wordpress-develop by costdev.


21 months ago
#40

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

Trac ticket:

#41 @costdev
21 months ago

  • Keywords commit added

PR 2442 is ready for committer review.

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


21 months ago

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


21 months ago

costdev commented on PR #2442:


21 months ago
#44

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

johnbillion commented on PR #2442:


21 months ago
#45

Closing in favour of #3049 👍

johnbillion commented on PR #2442:


21 months ago
#46

Closing in favour of #3049 👍

#47 @johnbillion
21 months ago

  • Keywords good-first-bug dev-feedback removed

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

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


21 months ago

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


21 months ago

#50 @antpb
20 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#51 @jrf
20 months ago

  • Keywords has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

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


20 months ago

#53 @jrf
20 months ago

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
20 months ago

Minor CS fixes for path_is_absolute()

#54 @jrf
20 months ago

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

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


20 months ago

#56 @davidbaumwald
20 months ago

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

#57 @audrasjb
20 months ago

  • Keywords needs-testing early commit removed

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

#58 @antpb
20 months ago

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

#59 @antpb
20 months ago

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.

#60 @antpb
20 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#61 follow-up: @desrosj
18 months ago

  • Milestone changed from 6.1 to 6.2
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#62 in reply to: ↑ 61 @azaozz
18 months ago

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.

Last edited 18 months ago by azaozz (previous) (diff)

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


16 months ago

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


15 months ago

#65 @antpb
15 months ago

  • Milestone changed from 6.2 to 6.3
  • Owner antpb deleted
  • Status changed from reopened to assigned

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

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


11 months ago

#67 @antpb
11 months ago

  • Milestone changed from 6.3 to Future Release

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

Note: See TracTickets for help on using tickets.