Make WordPress Core

Opened 9 years ago

Last modified 7 years ago

#33053 new defect (bug)

download_url() includes query string in temporary filenames

Reported by: hyperopic's profile Hyperopic Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.2.1
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

When installing a theme update, I encountered an error traced back to the update file exceeding the Windows path\filename length limit. It turned out the root cause of this was that the download URL contained a query string with access key information, which was also being included in the filename of the temporary file created by $tmpfname = wp_tempnam($url); in the download_url() function.

In my case, for example, download URL was:

https://s3.amazonaws.com/marketplace-downloads.envato.com/files/140862862/enfold.zip?AWSAccessKeyId=*******************&Expires=1437422162&Signature=*****************-***********%3D&response-content-disposition=attachment%3B+filename%3Dthemeforest-4519990-enfold-responsive-multipurpose-theme-wordpress_theme.zip

which resulted in a temporary file called:

enfold.zipAWSAccessKeyId*Expires1437422162Signature*-*-3Dresponse-content-dispositionattachment-3B-filename-3Dthemeforest-4519990-enfold-responsive-multipurpose-theme-wordpress_theme.tmp

rather than the expected enfold.zip

I would suggest that downloaded files should probably exclude any query string from the URL as the simplest method of resolving this issue, but will leave that to development team.

Attachments (4)

file.php.patch (503 bytes) - added by Hyperopic 9 years ago.
33053.diff (2.6 KB) - added by dd32 9 years ago.
file.php.2.patch (1.6 KB) - added by liegeandlief 8 years ago.
file.php altered to prevent unzip working direcotry path from being too long
class-wp-upgrader.php.patch (2.4 KB) - added by liegeandlief 8 years ago.
class-wp-upgrader.php altered to prevent unzip working direcotry path from being too long

Download all attachments as: .zip

Change History (16)

#1 follow-up: @dd32
9 years ago

  • Keywords reporter-feedback added

This should be fixed in 4.2.2 - #31811, can you verify if that does fix it?

In the case of the above example, the temporary file should be based on enfold.zip only.

#2 in reply to: ↑ 1 @Hyperopic
9 years ago

  • Keywords needs-patch added

Replying to dd32:

This should be fixed in 4.2.2 - #31811, can you verify if that does fix it?

In the case of the above example, the temporary file should be based on enfold.zip only.

No, it's definitely not resolved in 4.2.2, just tested on development machine and live site is now up to date too. I've got it working now with an edit to wp-admin/includes/file.php with line 457 in download_url() edited from
$tmpfname = wp_tempnam($url);
to
$tmpfname = wp_tempnam( parse_url( $url, PHP_URL_PATH ) );

Unfortunately I've not had the time to sort out SVN and get a patch submitted.

@Hyperopic
9 years ago

#3 @Hyperopic
9 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

@dd32
9 years ago

#4 @dd32
9 years ago

  • Keywords reporter-feedback dev-feedback removed

Turns out this indeed a duplicate of #31811, however, the fix applied wasn't good for this example input.

If the query includes a . character, then the existing regular expression breaks.

I can't quite remember why this needed to be fixed in a lower-layer than download_url(), but IIRC there was another few uses of it that needed the fix.

33053.diff combines your patch with a better splitting of the path. With my change alone, it works as expected, but I see no harm in stripping off the query parameters in download_url() as well.

#5 @ancientvalley
8 years ago

This happened for me with version 4.4.1. When I attempted to update a theme, the download file name included all the querystring parameters, making it too long for Windows.

By making the changes shown in 33053.diff after line 153, the update was able to work fine.

#6 follow-up: @liegeandlief
8 years ago

I have experienced a similar issue when trying to update the Beaver Builder plugin on a Windows installation of WordPress 4.4.2. The URL (with my key replaced) from which the ZIP file is downloaded is:

http://updates.wpbeaverbuilder.com/?fl-api-method=download_update&license=****************************************&domain=http%3A%2F%2Fwww.novamedia1999.co.uk%2Fwordpress&product=Beaver+Builder+Plugin+%28Standard+Version%29

This results in the temporary file:

fl-api-methoddownload_updatelicensedomainhttp3A2F2Fwww.novamedia1999.co-TZR1bx.tmp

When _unzip_file_ziparchive tries to unzip the package it fails when it tries to create a directory or file with a path name which is too long.

Obviously this could happen with any theme or plugin depending on the length of the directory/file names within the package and the amount of nested directories. But I have never had this problem before with any other themes or plugins because the length of the temporary file's name has never been so long.

@liegeandlief
8 years ago

file.php altered to prevent unzip working direcotry path from being too long

#7 @liegeandlief
8 years ago

The attached file file.php.2.patch should prevent the working directory path name from being longer than 255 characters. I think the patch is in the wrong format but I cannot delete it. I will regenerate it in the correct format and attach it again.

Also if I am posting these comments and patches in the incorrect place then please let me know and I will post them on a different/new ticket. This is my first time using the Trac system so I still learning the ropes!

@liegeandlief
8 years ago

class-wp-upgrader.php altered to prevent unzip working direcotry path from being too long

#8 @liegeandlief
8 years ago

Please completely disregard the attached file file.php.2.patch. Not only was the patch in the wrong format I also named it incorrectly as the changes are to class-wp-upgrader.php not file.php.

Please see the new attachment class-wp-upgrader.php.patch.

#9 @ocean90
8 years ago

#36776 was marked as a duplicate.

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


8 years ago

#11 @swissspidy
7 years ago

  • Milestone changed from Awaiting Review to Future Release

#12 in reply to: ↑ 6 @JanR
7 years ago

Replying to liegeandlief:
[...]

When _unzip_file_ziparchive tries to unzip the package it fails when it tries to create a directory or file with a path name which is too long.

Not a real solution for now, but FYI: this issue is fixed in Windows Server 2016 (IIS 10), when setting the "Enable Win32 long paths" Group Policy Object.

Note: See TracTickets for help on using tickets.