Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 12 months ago

#55109 closed defect (bug) (fixed)

Plugins no longer download to tmp folder

Reported by: antonynz's profile antonynz Owned by: clorith's profile Clorith
Milestone: 5.9.1 Priority: normal
Severity: normal Version: 5.9
Component: Filesystem API Keywords: has-patch needs-testing fixed-major
Focuses: Cc:

Description

5.9 introduces a content-disposition check in the download_URL function to update the filename externally.

However when this header is set, the temporary path is removed from the $tmpfname variable which is added earlier, i.e:

$tmpfname = wp_tempnam( $url_filename );
is overridden with:
$tmpfname = $tmpfname_disposition;

This results in plugins that are downloaded from the WordPress plugin repository (as they include the content-disposition header) being downloaded to the folder the user is in eg, wp-admin if updating via the plugins page or the home directory if updated via a cron auto-update job for example. The overriding of the variable also skips the writable directory checks within wp_tempnam.

I've read over the comments for the addition of this code, but it's not clear what the use case is or if it was intended to apply to plugins?
[https://github.com/WordPress/WordPress/commit/7a0a07d69156aba316d566f37dcdd498f59d138d
]

Attachments (1)

55109.diff (544 bytes) - added by azouamauriac 3 years ago.

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
3 years ago

  • Component changed from General to Filesystem API
  • Milestone changed from Awaiting Review to 5.9.1

Hi there, welcome to WordPress Trac! Thanks for the report.

Introduced in [51939] / #38231. Moving to the 5.9.1 milestone for visibility.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

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


3 years ago

#3 @Clorith
3 years ago

  • Owner set to Clorith
  • Status changed from new to accepted

@azouamauriac
3 years ago

#4 @azouamauriac
3 years ago

  • Keywords has-patch needs-testing added

Hi thanks for the report, the issue is due to php rename fonction, see https://www.php.net/manual/fr/function.rename.php#97347.

I have submitted a patch to ensure that the rename occurs to the original file dir.

#5 @Clorith
3 years ago

This seems like the simplest solution, but pinging @johnjamesjacoby since he spearheaded the original implementation, in case there's a consideration that was taken there that might be overlooked here.

#6 follow-up: @Clorith
3 years ago

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

In 52734:

Filesystem API: Use a temp folder for Content-Disposition files.

#38231 added support for files fetched remotely to have their filename defined by the host using the Content-Disposition header. This would then take priority over the existing temporary file name created with wp_tempnam() earlier in the process.

The change unintentionally omitted the temporary directory path used during uploads, since the wp_tempnam() function would have added it previously, so that files with this header ended up being stored in the WordPress root folder, or wp-admin folder, when triggered by WP_Cron or user interactions respectively.

This change makes sure the file path includes the temporary directory location when the header is used.

Follow-up to [51939].

Props antonynz, azouamauriac.
Fixes #55109.

#7 @Clorith
3 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 5.9.1 backports

(note on comment:5, I walked through all the changes of the original ticket, and it seems the missing tmp path was an oversight as the initial tickets would add the path via helper functions)

#8 in reply to: ↑ 6 @azouamauriac
3 years ago

Replying to Clorith:

In 52734:

Filesystem API: Use a temp folder for Content-Disposition files.

#38231 added support for files fetched remotely to have their filename defined by the host using the Content-Disposition header. This would then take priority over the existing temporary file name created with wp_tempnam() earlier in the process.

The change unintentionally omitted the temporary directory path used during uploads, since the wp_tempnam() function would have added it previously, so that files with this header ended up being stored in the WordPress root folder, or wp-admin folder, when triggered by WP_Cron or user interactions respectively.

This change makes sure the file path includes the temporary directory location when the header is used.

Follow-up to [51939].

Props antonynz, azouamauriac.
Fixes #55109.

Hello @Clorith thanks for the commit. I notice that in [52734] you add some unit test, that's good, just one thing, in the test function docblock you reference the trac ticket like that:

* @ticket #55109

see: https://prnt.sc/26xt3yp;
I think it should be:

* @ticket 55109

the "#" should not be there I think. as per doc https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/#annotations https://prnt.sc/26xtihw.

Let me know your thought. thanks.

#9 @SergeyBiryukov
3 years ago

In 52760:

Tests: Correct the @ticket reference in a download_url() test with the Content-Disposition header.

Move the data provider next to the test, for consistency.

Follow-up to [51939], [52734].

Props azouamauriac.
See #55109.

#10 @SergeyBiryukov
3 years ago

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

In 52761:

Filesystem API: Use a temp folder for Content-Disposition files.

#38231 added support for files fetched remotely to have their filename defined by the host using the Content-Disposition header. This would then take priority over the existing temporary file name created with wp_tempnam() earlier in the process.

The change unintentionally omitted the temporary directory path used during uploads, since the wp_tempnam() function would have added it previously, so that files with this header ended up being stored in the WordPress root folder, or wp-admin folder, when triggered by WP_Cron or user interactions respectively.

This change makes sure the file path includes the temporary directory location when the header is used.

Follow-up to [51939].

Props antonynz, azouamauriac, Clorith.
Merges [52734] and [52760] to the 5.9 branch.
Fixes #55109.

This ticket was mentioned in Slack in #core-auto-updates by peterwilsoncc. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.