Make WordPress Core

Opened 8 years ago

Closed 8 months ago

#37719 closed feature request (fixed)

Pre/Post Unzip Hooks

Reported by: dfavor's profile dfavor Owned by: costdev's profile costdev
Milestone: 6.4 Priority: normal
Severity: normal Version: 3.0
Component: Filesystem API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

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.

Change History (17)

#1 @swissspidy
7 years ago

  • Version 4.7 deleted

#2 @desrosj
5 years ago

  • Keywords needs-patch 2nd-opinion added

This ticket was mentioned in PR #4219 on WordPress/wordpress-develop by @costdev.


15 months ago
#3

  • Keywords has-patch added; needs-patch removed

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 ticket was mentioned in PR #4220 on WordPress/wordpress-develop by @costdev.


15 months ago
#4

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

#5 follow-up: @costdev
15 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Version set to 3.0

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.

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


14 months ago

#7 @costdev
12 months ago

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

#8 in reply to: ↑ 5 @azaozz
11 months ago

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', ...?

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


11 months ago

#10 @oglekler
11 months ago

  • Milestone changed from 6.3 to 6.4

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

#11 @oglekler
9 months ago

@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.

#12 @costdev
9 months ago

  • Keywords has-unit-tests commit added; 2nd-opinion removed

@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.

#13 @costdev
9 months ago

  • Owner set to costdev
  • Status changed from new to assigned

#14 @costdev
8 months ago

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

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.

@costdev commented on PR #4220:


8 months ago
#15

Committed in r56689.

#16 @flixos90
8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@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.

#17 @costdev
8 months ago

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

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.

Note: See TracTickets for help on using tickets.