If plugin zip file has very long name, windows systems can fail to upgrade

[rogerlos]

If a plugin upgrade zip file has an extremely long name and the plugin a deep directory structure, windows can fail to create directories and therefore the plugin upgrade will fail.

I suggest WordPress have an upper limit on the working directory name that is well-shy of that limit--and realistically, even 32 characters might be plenty, as the upgrader only needs to avoid other plugins being upgraded at the same time.

Windows has a 247-character limit to the total path name for directories, as said directory needs to be able to hold, at a minimum, an "8+3" file, and allow for a null byte or similar at the end of path + file (260 total characters).

As an example, SearchWP fails to upgrade on my local system (WAMP running on top of Windows 10) because the downloaded zip filename is 142 characters long sans extension (redacted in case this is a license key):

XXX0XXX0XxxxXXxxXXX0XxXxXxX0XXXxXxXxXXXxXxXxXXX0XXx
0X0XxXXx0XxX0XXxxXXX0XXx0X0XxXXX0Xxx0XxX0XXXxXXX0XX
XxXxx0xXXXXx0xXXXxxxxxXxxxX0XxXxX-0xxXXx.zip

Combine that with my wordpress location on disk:

C:/wamp/www/example/wp-content/upgrade/

And this directory cannot be created because the total path is 254 characters long (including the hidden null):

C:/wamp/www/example/wp-content/upgrade/XXX0XXX0Xxxx
XXxxXXX0XxXxXxX0XXXxXxXxXXXxXxXxXXX0XXx0X0XxXXx0XxX
0XXxxXXX0XXx0X0XxXXX0Xxx0XxX0XXXxXXX0XXXxXxx0xXXXXx
0xXXXxxxxxXxxxX0XxXxX-0xxXXx/searchwp/vendor/
pdfparser/vendor/smalot/pdfparser/src/Smalot/PdfParser

I think that while this may be an outlier, it's a hard bug to chase for a vendor who may not have windows systems available...and even they do have such an environment available, the results returned by Apache when running WAMP (or in similar environments) does not reflect the actual cause of the error.

Looking at the structure above as pretty much as edge-case as you are likely to get, even a truncate at 64 characters would have been more than adequate, while 32 characters would give quite a bit of room for flexibility.

I believe that this could be affected by altering wp_tempnam() in file.php:

/**
 * Returns a filename of a Temporary unique file.
 * Please note that the calling function must unlink() this itself.
 *
 * The filename is based off the passed parameter or defaults to the current unix timestamp,
 * while the directory can either be passed as well, or by leaving it blank, default to a writable temporary directory.
 *
 * @since 2.6.0
 *
 * @param string $filename Optional. Filename to base the Unique file off. Default empty.
 * @param string $dir      Optional. Directory to store the file in. Default empty.
 * @return string a writable filename
 *
 * @since x.x.x
 * @param int|string $maxlen  Optional. Maximum length (excluding extension) of returned filename
 */
function wp_tempnam( $filename = '', $dir = '', $maxlen = 32 ) {
    if ( empty( $dir ) ) {
        $dir = get_temp_dir();
    }

    if ( empty( $filename ) || '.' == $filename || '/' == $filename || '\\' == $filename ) {
        $filename = time();
    }

    // Use the basename of the given file without the extension as the name for the temporary directory
    $temp_filename = basename( $filename );
    $temp_filename = preg_replace( '|\.[^.]*$|', '', $temp_filename );

    // new: truncate the filename if longer than specified
    $max = intval( $maxlen ) > 0 ? intval( $maxlen ) - 6 : 32;
    $temp_filename = strlen( $temp_filename ) > $max ?
            substr( $temp_filename, 0, $max ) : $temp_filename;

    // ... remainder omitted for clarity

    return $temp_filename;
}

An alternative would be to require plugin upgrade files to have filenames shorter than [insert recommendation here], though actually checking that would mean a similar chunk of code would need to be added elsewhere in WP.

(Not sure exactly where the best place to document this for plugin authors, which could be done immediately.)

[rogerlos]

Sorry, just realized the hyphenated portion of the filename is the random string added by wordpress, so the original filename was 135 characters. Point still stands!

[JoshuaWold]

Appreciate the thoughtful breakdown of the problem, and the suggested solution!

Just a quick practical question. Is the change you're suggesting for this based on the WordPress download itself? I checked through and didn't find the location listed above for wp_tempnam().

If this change is implemented what would be some helpful things to validate to ensure it isn't breaking anything else?

[rogerlos]

I'm not entirely clear on your question, but:

• wp_tempnam() is in wp-admin/includes/file.php at (currently) line 622
• The long filename which caused the upgrade to fail was created as part of the automatic WP upgrade process, I assume based on the zip provided to the plugins repository

I am not an expert on this bit of WordPress, but as far as I can determine, wp_tempnam() is used to avoid naming conflicts in processes which might touch a number of files, or write to a common directory. It does this by adding a hyphen + six random characters to the filename. (Sorry for restating stuff you probably already know!)

So, in theory, it should not matter what WP returns from this function, as the "tempnam" is just that, a temporary name to use for the duration of a single process, or be passed as a parameter to other processes. WordPress itself seems to think six characters of noise is enough to avoid conflicts.

I would suggest that code which attempts to parse a returned value from wp_tempnam() is not adhering to best practices. The code should already know what was passed to that function, or if it's "downstream" code, the original name should have been passed as a parameter.

As far as I can tell, the only things which might break if a length limit was enforced are such situations. But I can't say that with any authority.

Rather than enforcing a set limit, perhaps wp_tempnam() can add a "this is really huge" test and only truncate those filenames that fail it. That test would have to be something like this pseudocode:

$fail = strlen( $path_on_disk ) + strlen( $filename ) >= $max_path[ $server_os ];

But that seems like a lot more work than just deciding on an upper limit, as you now have to detect operating systems, find out what their limits are, etc.

Another option would be to simply return false or null (or throw an exception) if the result of the calculation is longer than whatever the arbitrary limit is.

(An alternative would be to establish an arbitrary but healthy filename length limit as a max for developers adding files to the WP repository. 64 seems like plenty to me, but even 128 would have allowed SearchWP to update.)

[rogerlos]

Clarifying: One of the reasons SearchWP failed to upgrade was not only was the zip filename large, the plugin uses a deeply nested directory structure as well. So a reasonable-length filename can still keep windows from creating a new directory if the total character length of the path is over 247 characters:

C:/wamp/www/example/wp-content/upgrade/REASONABLE_LENGTH_ZIP_FILE_NAME-d8310f/
20-character-dirname/20-character-dirname/20-character-dirname/
20-character-dirname/20-character-dirname/20-character-dirname/
20-character-dirname/20-character-dirname/new

Would also fail.

In this case, WordPress would have no knowledge at the time of the wp_tempnam() call that the code using the returned result would be adding hundreds of additional characters to the path.

Welp.

Perhaps there is nothing which could really be done other than make it clear to authors that deeply-nested directory structures combined with long filenames means they're flirting with failure.

I do not know enough about windows (or mkdir) to know if this limit is always true, or only if using absolute paths. If the latter, it might be able to be skirting by calling mkdir using a relative path repeatedly, but I have no idea.

In sum: This is an actual problem, but I have no idea what an actual solution might be. My max-length solution would probably catch all but the edge-cases of edge-cases, and would have caught the actual error in this particular instance.

At the very least, documentation? And perhaps something in the repository review process which flags deeply-nested file structures and lets the developer know they're treading on dangerous ground?

[JoshuaWold]

Thank you for the thorough explanation. This was helpful!

In sum: This is an actual problem, but I have no idea what an actual solution might be. My max-length solution would probably catch all but the edge-cases of edge-cases, and would have caught the actual error in this particular instance.

Based on what you've shared here, updating the max length seems to make sense.

At the very least, documentation? And perhaps something in the repository review process which flags deeply-nested file structures and lets the developer know they're treading on dangerous ground?

If not, adding documentation would be helpful.

My background is more related to the UI/UX side of things, and some frontend development experience. Would love to see if someone else with a backend development background could also weigh in and help validate whether we can move this issue forward.