Make WordPress Core

Opened 3 years ago

Closed 2 months ago

#54111 closed defect (bug) (worksforme)

Delete attachment with sizes not working on Windows file system

Reported by: kybernetikservices's profile kybernetikservices Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.8.1
Component: Media Keywords: reporter-feedback has-patch dev-feedback needs-testing
Focuses: Cc:

Description

I wrote a custom plugin for importing data, including media files, from a CRM system. The media files are stored in a dedicated folder in wp-content/uploads.
When I delete a custom post I do also delete the attached media files with
wp_delete_attachment().
I noted, on a Windows server, that just the primary image file will be deleted. All related image sizes are not deleted.

I debuged the function wp_delete_attachment_files() in wp-includes/post.php and figured out there is something wrong with the function path_join() (see code line 6240).
Before path_join() the variable $intermediate_file is

E:/folder1/folder2/folder3/wp-content/uploads/extra-folder/91010/65535_49693031347_cd702ea089_h_1280_700_nofilter-300x164.jpg.

It is the full windows path including file.
After $intermediate_file = path_join( $uploadpath['basedir'], $intermediate_file ); the variable $intermediate_file is equal

E:/folder1/folder2/folder3/wp-content/uploads/E:/folder1/folder2/folder3/wp-content/uploads/extra-folder/91010/65535_49693031347_cd702ea089_h_1280_700_nofilter-300x164.jpg

Please check the path. It has added the full $intermediate_file to the $uploadpath['basedir'].
Same happen with the $intermediate_dir. After usign path_join() (line 6234) the path is

E:/folder1/folder2/folder3/wp-content/uploads/E:/folder1/folder2/folder3/wp-content/uploads/extra-folder/91010

As you can see, it is the $uploadpath['basedir'] followed by the full folder path.

The same code is working on a linux server as expected.

Please let me know if I can give you more details to reproduce and fix this error.

Thanks for all your effort.

Attachments (2)

54111.patch (1.1 KB) - added by bgoewert 16 months ago.
54111_v1.php (2.9 KB) - added by bgoewert 3 months ago.
Plugin to delete all attachments after post deletion.

Download all attachments as: .zip

Change History (23)

#1 @SergeyBiryukov
3 years ago

  • Component changed from General to Media

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


3 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

@bgoewert
16 months ago

#4 @bgoewert
16 months ago

  • Keywords reporter-feedback has-patch dev-feedback added
  • Severity changed from critical to normal

Hello @kybernetikservices and welcome to WordPress Trac!

It's been 16 months since this was submitted. Are you still able to reproduce this issue? If so, can you provide a little more information about your environment, particularly the Windows Server and PHP version?

Looking into path_join() brings me to path_is_absolute() which implements some checks on the given path.

If E:/folder1/folder2/folder3/wp-content/uploads/extra-folder/91010/65535_49693031347_cd702ea089_h_1280_700_nofilter-300x164.jpg was your actual path format, it seems like it would have failed all those checks. This is an invalid Windows path format (/ vs \).

One of the checks that path_is_absolute() performs uses realpath() which returns the correct path format, if this was a valid path. However, path_is_absolute() checks the canonical path against the ill-formatted one that was given, meaning this would not match because the slashes are different.

/*
 * This is definitive if true but fails if $path does not exist or contains
 * a symbolic link.
 */
if ( realpath( $path ) === $path ) {
    return true;
}

There is a Windows specific check as well. Although, this check does not take into account the ill-formatted path.

// Windows allows absolute paths like this.
if ( preg_match( '#^[a-zA-Z]:\\\\#', $path ) ) {
    return true;
}

I would venture to guess that somewhere in your plugin, or in your environment, the slashes got reversed and caused this to fail.

What we could do is add realpath() to the Windows check to ensure it's using the correct path format.
I've attached a patch for that and just need someone to review.

54111.patch

Last edited 16 months ago by bgoewert (previous) (diff)

#5 @kybernetikservices
15 months ago

Thank you @bgoewert for your message and effort to fix this issue.
First of all sorry for my late reply.
I'm glad to tell you that your fix is working as expected. The patch will delete all attachment sizes on Windows.
Hope this fix will find its way into WordPress core with the next update.
Once again thank you for your time and effort!

Last edited 15 months ago by kybernetikservices (previous) (diff)

#6 @bgoewert
15 months ago

  • Keywords needs-testing needs-testing-info added

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


13 months ago

#8 @antpb
13 months ago

  • Milestone changed from Awaiting Review to 6.3

Moving this into 6.3 to get it in hopefully the next release. Thanks for all the work here!

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


10 months ago

#10 @antpb
10 months ago

  • Milestone changed from 6.3 to 6.4

We're a bit lacking in the Windows environment and experience to get this done in time for 6.3 so we'll need to move to 6.4 to give more time to properly get the right people to help investigate a fix.

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


9 months ago

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


8 months ago

#13 @nicolefurlan
6 months ago

@kybernetikservices or @bgoewert would you be able to add testing instructions to this ticket? We're looking to include this in the 6.4 release and we're ~2 weeks out from release candidate 1.

#14 @bgoewert
6 months ago

@nicolefurlan Unfortunately I don't have enough time to give sufficient instructions. It looks like path_is_absolute() was only used in the path_join() function, correct? I'd look at processes that use path_join() and feed incorrect path styles to it. Media imports like what this ticket was about is a good example. Sorry, if I had more time, I'd write proper instructions with an initial test.

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


6 months ago

#16 @antpb
6 months ago

  • Milestone changed from 6.4 to 6.5

With 6.4 RC1 fast approaching its best to move this to 6.5 for more investigation

#17 follow-up: @kybernetikservices
3 months ago

Hi @antpb. Is there a timeline for when this fix will be included in WordPress core?
The fix proided by @bgoewert 13 months ago is working. So, is there any other reason why this can not be published? Thanks!

Last edited 3 months ago by kybernetikservices (previous) (diff)

#18 in reply to: ↑ 17 @bgoewert
3 months ago

Replying to kybernetikservices:

Hi @antpb. Is there a timeline for when this fix will be included in WordPress core?
The fix proided by @bgoewert 13 months ago is working. So, is there any other reason why this can not be published? Thanks!

Hey @kybernetikservices, I think it's waiting on testing info and someone to provide a test report before the patch can be accepted. (Unless a core committer approves it?)

Would you have time to confirm these testing instructions and possibly provide some testing? I'll also try spin up a trial version of Windows Server 2022 and see if I can't reproduce this issue too.

Also, have you been able to confirm that this only happens to custom post types?
Or does this also happen for the default post and page types?

Testing Instructions

Requires running WordPress on Windows (Server).

From my understanding, these are the steps to take to reproduce the issue.

Once I am able, I can upload a basic plugin to test with that creates a new post type and a hook to delete attachments upon post deletion. Or if @kybernetikservices can, that would be helpful as well.

Steps to Reproduce

  1. (optional) Create a custom post type.
  2. Add a hook that calls wp_delete_attachment() to delete all the post attachments when a post, page, or custom post type is trashed.
  3. Create a new post.
  4. Insert an image as a post attachment. (this will create image variations)
  5. Delete the post.

Expected Results

When reproducing the bug:

  • Image variations are not removed when trashing a post.

When testing the patch:

  • All image variations should be removed along with the post attachment.
Last edited 3 months ago by bgoewert (previous) (diff)

@bgoewert
3 months ago

Plugin to delete all attachments after post deletion.

#19 @bgoewert
3 months ago

  • Keywords needs-testing-info removed

Reproduction Report

After hours of setting up IIS and testing, I was unable to reproduce the issue.

@kybernetikservices, if you could provide a report similar to this and test a bare wordpress installation with the plugin provided here, that would be most helpful. This will verify whether it is an issue with WordPress or your server setup.

But from what I can tell, at least with 6.4, it seems to be either an issue with a plugin or a configuration issue with the web server.

Environment

  • OS: Pop_OS! 22.04
  • Server: Windows Server 2022 / IIS 10
  • PHP: 8.2
  • WP: 6.4
  • Browser: Firefox
  • Theme: TT4
  • Active Plugins: 54111_v1.php

Actual Results

  • ❌ All attachements and size variations were deleted upon post deletion (not reproduced)

Additional Notes

  • IIS is a pain
  • The wp_delete_file_from_directory() function, called in wp_delete_attachment_files(), utlizes wp_normalize_path() and realpath() to validate the path before deleting, similar to what path_is_absolute() does.
Last edited 3 months ago by bgoewert (previous) (diff)

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


2 months ago

#21 @antpb
2 months ago

  • Milestone 6.5 deleted
  • Resolution set to worksforme
  • Status changed from new to closed

@bgoewert just wanted to say massive props to you on setting up an IIS instance and doing this investigation. I am very familiar with how big a task this can be especially in the context of finding bugs in Core.

"Additional Notes IIS is a pain" 🤣

We haven't heard back in some time now so I'll close this to keep the backlog of issues true.

Note: See TracTickets for help on using tickets.