WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#12637 closed defect (bug) (fixed)

OS X-generated theme zips won't upload properly

Reported by: chrisjean Owned by: sivel
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Filesystem API Keywords: has-patch
Focuses: Cc:

Description

The current trunk doesn't handle theme zip files generated on OS X very well. There are a couple of issues:

  • r13006 introduced the ZIPARCHIVE::CHECKCONS check for validating whether a zip file can be properly opened. All zip files generated using OS X by right-clicking a directory and selecting "Compress" fail this consistency check and result in an error message of "Incompatible Archive" being displayed. While checking for consistency is nice, this will cause more issues than it will prevent.
  • When the uploaded theme zip is being parsed, if more than one directory exists, a nested directory structure will be created for the theme. Since many methods of generating a zip file on OS X cause an additional __MACOSX directory, which is hidden in OS X, to be added to the root of the zip file, uploading one of these zip files causes unexpected behavior of nested directories and causes a number of errors to be displayed. These errors indicate some additional problems which would be best dealt with in a separate ticket.

    For example, if a directory called sample-theme which contains theme files are zipped on OS X and uploaded via Add New Theme > Upload, the resulting directory structure would look like:
    • sample-theme/
    • sample-theme/sample-theme/
    • sample-theme/__MACOSX/

Way to reproduce problem 1:

  • In OS X, right-click (or ctrl+click) a theme directory and select "Compress" to create a zip file. premade sample theme zip
  • On a site running WP r13006 or above, go to Appearance > Add New Themes, click the Upload tab, select the zip file created above, and click "Install Now".

Problem 1 result:
http://new.gaarai.com/os-x-theme-zip-failure-step-1-example.png

Way to reproduce problem 2:

  • In OS X, right-click (or ctrl+click) a theme directory and select "Compress" to create a zip file. premade sample theme zip
  • On a site running 2.9.2 or trunk with the ZIPARCHIVE::CHECKCONS (represented as a number 4 on some builds) argument removed, go to Appearance > Add New Themes, click the Upload tab, select the zip file created above, and click "Install Now".

Problem 2 result:
http://new.gaarai.com/os-x-theme-zip-failure-step-2-example.png

The supplied patch fixes both of these problems.

Problem 1 is fixed by simply removing the ZIPARCHIVE::CHECKCONS argument for ZipArchive->open() in wp-admin/includes/file.php.

Problem 2 is fixed by adding checks in both the _unzip_file_ziparchive and _unzip_file_pclzip functions that prevent a root-level __MACOSX directory from being extracted with the rest of the files/directories.

Attachments (2)

12637.diff (2.6 KB) - added by chrisbliss18 8 years ago.
Alters the behavior of unzip_file() to strip out MACOSX directories
12637.noticefix.diff (708 bytes) - added by sivel 8 years ago.
Check if ZIP_ER_OK is defined before testing it

Download all attachments as: .zip

Change History (18)

#1 @chrisbliss18
8 years ago

Note that this will also fix the same issues with zipped plugins created on OS X systems.

#2 @ptahdunbar
8 years ago

Tested the patch on snow leopard. Fixes the zip issue.

#3 @nacin
8 years ago

  • Component changed from Themes to Filesystem
  • Keywords tested commit removed
  • Owner changed from chrisbliss18 to dd32
  • Status changed from new to assigned

#4 @dd32
8 years ago

  • Status changed from assigned to accepted

I'm wary of removing the consistency checks, If it fails the ZipArchive uncompression with "Incompatible archive" it falls through to PclZip, which will create the same error if it cant decompress it either.

As for the zip's, Yes, its probably best to ignore metadata directories in the zip files, But at the same time, the search function to locate the theme should be smart enough to work out which files should be copied.

I'll have a closer look into this over the weekend.

#5 @chrisbliss18
8 years ago

  • Keywords tested commit added

Can I get an update on this ticket?

I want this to be in 3.0 for release before this becomes an issue in the wild. At the very least, the inclusion of ZIPARCHIVE::CHECKCONS into a release should be punted to a future release so that it can be debated properly as there is too much going on right now. Including it in a release will cause issues with OS X while offering little to no added value.

As for the rest of the patch, it solves the MACOSX directory issue, which creates numerous issues that need to be addressed.

As many plugin and theme devs that I know run OS X, this could be a major issue if it is released in 3.0.

#6 @nacin
8 years ago

  • Keywords tested commit removed

#7 @dd32
8 years ago

The MAC_OSX annoyances are not a regression from a previous version. I feel that would be better dealt with by a better filepath parser/locator, There'll be some changes there in 3.1 most likely.. as multiple themes in uploaded zip's should be supposed

As for ZIPARCHIVE::CHECKCONS, I'm still reluctant to remove that, as it'll mean other corrupt archives could slip through with it, or partially decompress.

I do not have access to any form of Mac-based testbed, So i cant test directly here.

chrisbliss18: What is the actual error that occurs when it attempts to open the zip file?

For example, i'm using this code to test:

$zip = new ZipArchive; 
$open = $zip->open('C:/www/wordpress-commit/wordpress-commit.zip', ZIPARCHIVE::CHECKCONS);
$open2 = $zip->open('C:/www/wordpress-commit/wordpress-commit.zip');

//Results:
$open = ZIPARCHIVE::ER_NOZIP - 19
$open2 = (bool) true;

That zip archive is a 500KB archive with the 2nd half removed to simulate a incomplete archive. Thats the reason i dont want to remove the consistancy checking.

At present, if ZipArchive fails to open the archive with ZIPARCHIVE::CHECKCONS set, it WILL fall through to using the previous methods, PclZip. This has been done in such a way, ZipArchive will only be used in the event that it can open the archive correctly - That has been the case since just after adding the ZipArchive support: [13221]

#8 @chrisbliss18
8 years ago

Well this is embarrassing. Somehow my wp-admin/includes/file.php file wasn't updating, so I didn't see the fallback scenario, just the if/else without a fallback.

Since there was no resistance to the idea of stripping out the MACOSX directory when I proposed it on the IRC chat a few weeks back, I've updated the patch to only have the portion that strips the MACOSX directory.

I'll now shift my focus and work on a patch that fixes the multiple themes in one directory upload issue. I'll create a new ticket for that patch.

@chrisbliss18
8 years ago

Alters the behavior of unzip_file() to strip out MACOSX directories

#9 @chrisbliss18
8 years ago

FYI: I did some digging on why the OS X-produced zip files fail the consistency check. The failure seems to be specific to the version and compile options of the PHP interpreter that is running the code.

I freshly-compiled PHP from version 5.2.5 through 5.2.10 with the following config:

./configure --with-zip --with-apxs2=/opt/lampp/bin/apxs --enable-inline-optimization --disable-debug --enable-bcmath --enable-calendar --enable-ctype --enable-dbase --enable-discard-path --enable-exif --enable-filepro --enable-force-cgi-redirect --enable-ftp --enable-gd-imgstrttf --enable-gd-native-ttf --with-ttf --enable-magic-quotes --enable-memory-limit --enable-shmop --disable-sigchild --enable-sysvsem --enable-sysvshm --enable-track-vars --enable-trans-sid --enable-wddx --enable-yp --with-ftp --without-xpm --with-zlib=yes --with-gd --with-imap-ssl --enable-sockets --enable-mbstring=all --enable-mbregex --enable-zend-multibyte --enable-exif --enable-soap --enable-pcntl --with-mime-magic --with-iconv --enable-dio

Versions 5.2.5 and 5.2.6 both return a int(0) value on the ZipArchive::open call (when called with a OS X-generated zip file). Versions 5.2.8-5.2.10 all return true.

However, I do have an example of a version 5.2.9 failing with the following compile options:

'./configure' '--prefix=/opt/lampp' '--with-apxs2=/opt/lampp/bin/apxs' '--with-config-file-path=/opt/lampp/etc' '--with-mysql=/opt/lampp' '--enable-inline-optimization' '--disable-debug' '--enable-bcmath' '--enable-calendar' '--enable-ctype' '--enable-dbase' '--enable-discard-path' '--enable-exif' '--enable-filepro' '--enable-force-cgi-redirect' '--enable-ftp' '--enable-gd-imgstrttf' '--enable-gd-native-ttf' '--with-ttf' '--enable-magic-quotes' '--enable-memory-limit' '--enable-shmop' '--disable-sigchild' '--enable-sysvsem' '--enable-sysvshm' '--enable-track-vars' '--enable-trans-sid' '--enable-wddx' '--enable-yp' '--with-ftp' '--with-gdbm=/opt/lampp' '--with-jpeg-dir=/opt/lampp' '--with-png-dir=/opt/lampp' '--with-freetype-dir=/opt/lampp' '--without-xpm' '--with-zlib=yes' '--with-zlib-dir=/opt/lampp' '--with-openssl=/opt/lampp' '--with-expat-dir=/opt/lampp' '--enable-xslt=/opt/lampp' '--with-xsl=/opt/lampp' '--with-dom=/opt/lampp' '--with-ldap=/opt/lampp' '--with-ncurses=/opt/lampp' '--with-gd' '--with-imap-dir=/opt/lampp' '--with-imap-ssl' '--with-imap=/opt/lampp' '--with-gettext=/opt/lampp' '--with-mssql=/opt/lampp' '--with-sybase=/opt/lampp' '--with-interbase=shared,/opt/interbase' '--with-mysql-sock=/opt/lampp/var/mysql/mysql.sock' '--with-oci8=shared,instantclient,/opt/lampp/lib/instantclient' '--with-mcrypt=/opt/lampp' '--with-mhash=/opt/lampp' '--enable-sockets' '--enable-mbstring=all' '--with-curl=/opt/lampp' '--enable-mbregex' '--enable-zend-multibyte' '--enable-exif' '--with-bz2=/opt/lampp' '--with-sqlite=shared,/opt/lampp' '--with-libxml-dir=/opt/lampp' '--enable-soap' '--enable-pcntl' '--with-mysqli=/opt/lampp/bin/mysql_config' '--with-mime-magic' '--with-pgsql=shared,/opt/lampp/postgresql' '--with-iconv' '--enable-dio' '--with-pdo-mysql=/opt/lampp' '--with-pdo-pgsql=/opt/lampp/postgresql' '--with-pdo-sqlite' '--with-ming=shared,/opt/lampp'

This PHP version was provided by XAMPP. I tried to compile PHP 5.2.9 with a variety of different options in order to determine the option that is the source of the issue in versions 5.2.8+, but I was unable to find it.

#10 @demetris
8 years ago

  • Cc dkikizas@… added

Maybe duplicate: #11979. (After r12794 install by uploading fails for one ZIP archive (works for others)) — Which is closed at present because we could not reproduce consistently across systems.

#11 @dd32
8 years ago

(In [14346]) Check for "ZIP_ER_OK" return value on ZipArchive. See #12637

#12 @dd32
8 years ago

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

(In [14349]) Skip over MACOSX folders during zip extraction. Props chrisbliss18. Fixes #12637

#13 @sivel
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#14 @sivel
8 years ago

  • Owner changed from dd32 to sivel
  • Status changed from reopened to assigned

r14373 shows a notice during plugin installs:

Notice: Use of undefined constant ZIP_ER_OK - assumed 'ZIP_ER_OK' in /var/www/trunk/wp-admin/includes/file.php on line 580

@sivel
8 years ago

Check if ZIP_ER_OK is defined before testing it

#15 @dd32
8 years ago

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

(In [14377]) Fix ZIP_ER_OK constant, Its a class constant, and only accessible as such under PHP5. Props sivel for noticing. Fixes #12637

#16 @automattor
7 years ago

(In [15052]) Always fallback to PclZip in the event that ZipArchive does not return true. The PHP Zip extension is hit-and-miss with OSX generated zip files, sometimes 0 will be emitted and extraction will succeed, others it will fail with. Reverts r14346, r14377, partially r14800. See #12637. See #13491

Note: See TracTickets for help on using tickets.