Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(Task/Ext/Archive/ZipTask): Preseves directories' permissions in zip #1820

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

leagris
Copy link
Contributor

@leagris leagris commented Apr 27, 2024

Fix #1817 When file permissions are supported, stores directories' original permissions as ZipArchive::OPSYS_UNIX ExternalAttribute.

Copy link

codecov bot commented Apr 27, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 53.57%. Comparing base (f74ea08) to head (a0f5c56).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/Phing/Task/Ext/Archive/ZipTask.php 81.25% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1820      +/-   ##
============================================
+ Coverage     53.55%   53.57%   +0.01%     
- Complexity     9806     9813       +7     
============================================
  Files           495      495              
  Lines         24194    24209      +15     
============================================
+ Hits          12958    12970      +12     
- Misses        11236    11239       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mrook
Copy link
Member

mrook commented Apr 28, 2024

Hi,

Thanks for your contribution! I'd like to make this behavior optional (i.e. controlled by an attribute). The attribute should default to false because we're changing behavior. The attribute should also be documented in the guide and the RNG grammar. When that's all done, it'll be added to the 3.1 release line (not 3.0).

@leagris
Copy link
Contributor Author

leagris commented Apr 28, 2024

Hi,

Thanks for your contribution! I'd like to make this behavior optional (i.e. controlled by an attribute). The attribute should default to false because we're changing behavior. The attribute should also be documented in the guide and the RNG grammar. When that's all done, it'll be added to the 3.1 release line (not 3.0).

I am ok with that, though, something annoys me with letting the current working as default.
With current behaviour, files permissions are preserved because it is handled natively by the ZipArchive's addFile method. But since directories are not copied but created, ZipArchive does not know the original directory permissions.

Having files inheriting permissions but not directories ; it is more a bug than a feature. When dealing with such zip archives in a unix/Linux system, unzipped with the unzip command, those 777 read/write/execute permissions for everyone in directories is a serious problem. Letting this as the default feels quite odd.

I'd propose some alternative instead with a savFileAttributes switch default false as you propose, to save the extra attributes (os-dependant or unix/POSIX only?)

The ZipTask's addFilesetsToArchive method would need to be updated to implement those two attributes with:

  • savFileAttributes="false" (default): Add files and directories to the zip without preserving their permission and timestamp. (Means not using ZipArchive's addFile but some other method like addFromString which does not have a source file attributes.
  • saveFileAttributes="true": The addFile methods would do that natively, but it is not handled for directories. The handling of directories' file attributes need to be implemented outside the ZipArchive's expansion which does not support it natively. It has OS-specific handling that can add complexity especially for OS/2 or VMs (does not look like a priority to deal with those). If we stick to Unix/Linux/BSD POSIX environment, permission bits and timestamps are the things to preserve in fileAttributes.

See: the zip command's man-page for the -X, --no-extra option:

-X
--no-extra
Do not save extra file attributes (Extended Attributes on OS/2, uid/gid and file times on Unix). The zip format uses extra fields to include additional information for each entry. Some extra fields are specific to particular systems while others are applicable to all systems. Normally when zip reads entries from an existing archive, it reads the extra fields it knows, strips the rest, and adds the extra fields applicable to that system. With -X, zip strips all old fields and only includes the Unicode and Zip64 extra fields (currently these two extra fields cannot be disabled).

Negating this option, -X-, includes all the default extra fields, but also copies over any unrecognized extra fields.

@leagris
Copy link
Contributor Author

leagris commented Apr 28, 2024

@mrook Implemented a savefileattributes flag.

@leagris leagris force-pushed the dev-1817 branch 3 times, most recently from 3d921b4 to b15083e Compare April 30, 2024 13:20
@leagris
Copy link
Contributor Author

leagris commented May 1, 2024

Even Copilot says my code is smart ;)

The addFile method in the ZipArchive class is typically used to add a file to the zip archive. However, in your script, it’s being used in a clever way to add a directory.

Here’s how it works:

When you call addFile with the mtimeDummy file and the entryName (which is the directory path), it adds an entry to the zip archive with the name of the directory. But because the mtimeDummy file is empty, this entry is effectively a directory, not a file.

The key here is the trailing slash in the entryName. In a zip archive, an entry with a trailing slash is treated as a directory.

So, even though addFile is normally used to add files, in this case, it’s being used to add a directory. This is possible because of the way entries are handled in zip archives and the use of the mtimeDummy file. This workaround allows you to preserve the mtime of the directory in the zip archive, despite the limitations of PHP 7.4’s ZipArchive class. It’s a smart solution! 👍

Fix phingofficial#1817 When file permissions are supported, stores directories's original
permissions as `ZipArchive::OPSYS_UNIX` `ExternalAttribute`.
By default false when omitted, file permissions attribute are not saved.
When setting the savefileattributes to true, files permissions
are savec as external attributes in the Zip archive.

Example:

```xml
<zip includeemptydirs="true" destfile="${project.basedir}/test.zip" basedir="./dir" savefileattributes="true">
    <fileset dir="dir">
        <include name="**/**"/>
    </fileset>
</zip>
```
ZipArchive::setMtimeIndex (PHP >= 8.0.0, PECL zip >= 1.16.0)
@mrook
Copy link
Member

mrook commented Sep 4, 2024

I'll take a look at this soon. Sorry for not having had the time earlier.

@mrook mrook added this to the 3.1.0 milestone Sep 4, 2024
@siad007
Copy link
Member

siad007 commented Sep 13, 2024

@leagris would you mind to add a test case please?
You can add one to this BuildTest and to the correspondent TestBuild XML file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

ZipTask stores directories with 777 permission
3 participants