Pre/Post Unzip Hooks

[dfavor]

Add Pre/Post hooks to unzip logic, providing groundwork for advanced Malware scanning + advanced role management by interacting with OS using chattr facility to enforce super admin capabilities, requiring OS level authentication prior to taking action on any zipfile contents.

Logic...

1) Refactor _unzip_file_pclzip() + _unzip_file_ziparchive() merge into single function + merge common code.

2) Hooks...

required_disk_space - notify how much additional disk space required
required_directories - notify list of missing directories, which will be created

pre_unzip - return zipfile path + archive component name if applicable
post_unzip - return zipfile path + unzip directory + archive component name if applicable

3) Additionally, zipfile source propagation could be highly useful.

Either local filesystem reference or download URL.

This introduces two new actions: pre_unzip_file and post_unzip_file.

Both actions pass the following:

• string $file - Full path and filename of ZIP archive.
• string $to - Full path on the filesystem to extract archive to.
• string[] $needed_dirs - A full list of required folders needed to be created.
• float|false $required_space - The space required to unzip the file and copy its contents, with a 10% buffer. False if disk_free_space() returned false.
• string $mode - The mode used to unzip. Accepts "ziparchive" or "pclzip". Default "ziparchive".

Additionally:

• Combines the _unzip_file_ziparchive() and _unzip_file_pclzip() functions into a new function: _unzip_file().
  • Conditionally runs distinct functionality, and merges common functionality.
  • For BC, both functions act as wrappers for _unzip_file( $file, $to, $needed_dirs, ziparchive||pclzip ).
  • Note: Despite the docblock clearly stating that these functions should not be used directly, ​plugins still use it directly.
• Implements _unzip_file() in unzip_file() to avoid calling the wrapper functions unnecessarily.
  • No other locations in Core use _unzip_file_ziparchive() or _unzip_file_pclzip() directly.

Trac ticket: https://core.trac.wordpress.org/ticket/37719

This introduces two new actions: pre_unzip_file and post_unzip_file.

Both actions pass the following:

• string $file - Full path and filename of ZIP archive.
• string $to - Full path on the filesystem to extract archive to.
• string[] $needed_dirs - A full list of required folders needed to be created.
• float|false $required_space - The space required to unzip the file and copy its contents, with a 10% buffer. False if disk_free_space() returned false.

Trac ticket: https://core.trac.wordpress.org/ticket/37719

[costdev]

PRs ​4219 and ​4220 implement the pre_unzip_file and post_unzip_file actions.

• ​4219 combines _unzip_file_ziparchive() and _unzip_file_pclzip() into a new function, _unzip_file(), makes both functions wrappers for _unzip_file(), and implements this in unzip_file() to avoid unnecessarily calling the wrapper functions.
• ​4220 is an alternative option that makes minimal changes by simply introducing the actions directly into _unzip_file_ziparchive() and _unzip_file_pclzip().

It should be noted that passing $needed_dirs in the actions means removing unset( $needed_dirs ). Unzipping archives is memory intensive, so we should decide whether this additional memory by retaining $needed_dirs, and by doing any hooked callbacks, is worth the benefits.

---

• Keeping 2nd-opinion to gather thoughts on the above and on both PRs
• Milestoning for 6.3 to raise awareness of this ticket.
• Both _unzip_file_ziparchive() and _unzip_file_pclzip() were introduced in 3.0. Updating the Version property to reflect this.

[costdev]

Pinging @azaozz, @SergeyBiryukov, @afragen and @pbiron for thoughts on the proposed enhancement and the different approaches of each PR.

[azaozz]

Replying to costdev:

Thinking ​4220 makes more sense. Merging of _unzip_file_ziparchive() and _unzip_file_pclzip() can be done in another ticket if needed.

It should be noted that passing $needed_dirs in the actions means removing unset( $needed_dirs ).

Probably missing something but why not unset $needed_dirs after do_action( 'post_unzip_file', ...?

[oglekler]

This ticket was discussed during the bug scrub with @mikeschroder, @mukeshpanchal27 and @audrasjb and was decided to move it into 6.4 milestone.

[oglekler]

@costdev can we proceed with this patch? I think that if we need just hooks, there is no need to make drastic changes. I suppose merging two function in one is unnecessary trouble, but there is a part which can be moved to another function for internal easily use and be called from both functions. But this can be done in another ticket.
I also wonder if post_unzip_file can be called after_unzip_file because otherwise it looks confusing.

[costdev]

@azaozz Probably missing something but why not unset $needed_dirs after do_action( 'post_unzip_file', ...?

Good point! I've updated ​PR 4220 to add this.

@oglekler I also wonder if post_unzip_file can be called after_unzip_file because otherwise it looks confusing.

Agreed! I've updated ​PR 4220 to change the hook names to before_unzip_file and after_unzip_file for clarity. Tests are also included now to ensure these actions are fired.

Can we proceed with this patch?

Yep I think this is ready for final review. 🙂

---

• Removing 2nd-opinion as we've had several opinions now.
• Adding to the commit queue.

[costdev]

In 56689:

Filesystem API: Introduce filters for before/after unzipping archives.

This introduces the following new filters which wrap the process of unzipping an archive:

• pre_unzip_file - Filters archive unzipping to allow an override with a custom process.
• unzip_file - Filters the result of unzipping an archive.

Both filters pass the following:

• string $file - Full path and filename of ZIP archive.
• string $to - Full path on the filesystem to extract archive to.
• string[] $needed_dirs - A full list of required folders that need to be created.
• float|false $required_space - The space required to unzip the file and copy its contents, with a 10% buffer.

Props dfavor, azaozz, oglekler, afragen, costdev.
Fixes #37719.

Committed in r56689.

[flixos90]

@costdev It looks like [56689] leads to test failures in trunk, see e.g. ​https://github.com/WordPress/wordpress-develop/actions/runs/6305454454/job/17119172683.

[costdev]

In 56691:

Filesystem API: Add missing ZIP file for unzip tests.

In [56689], a ZIP file is needed in tests/phpunit/data/filesystem/ but wasn't included in the changeset. This produced an error when attempting to create a subdirectory during the tests.

This adds the tests/phpunit/data/filesystem/archive.zip file.

Follow-up to [56689].

Props flixos90.
Fixes #37719.