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

Inconsistency in ZipArchive::addGlob 'remove_path' Option Behavior #12661

Closed
MoonlitSyntax opened this issue Nov 13, 2023 · 4 comments
Closed

Comments

@MoonlitSyntax
Copy link

MoonlitSyntax commented Nov 13, 2023

Description

Steps to Reproduce

I've encountered an issue with the remove_path option in the ZipArchive::addGlob method where its behavior doesn't align with the documentation. The documentation states that remove_path is used to "remove prefix from matching file paths before adding to the archive." However, it appears that remove_path matches and removes parts of the file path that are not just prefixes.

Create a directory structure as shown below:

/tmp
├── Dire
│   └── This_Is_A_File
└── test.php

Run the following PHP script

<?php
$zip = new ZipArchive();
$filename = "./HereIsZip.zip";

if ($zip->open($filename, ZipArchive::CREATE)!==TRUE) {
    exit("Wrong!!!\n");
}
$dir = '/tmp/Dire';
$FirstDir = '/Dire/';
$SecondDir = 'This_Is';
$ThirdDir = '/D';
$zip->addGlob('/tmp/Dire/This_Is_A_File', 0, ['add_path' => '/', 'remove_path' => $FirstDir]);
$zip->addGlob('/tmp/Dire/This_Is_A_File', 0, ['add_path' => '/', 'remove_path' => $SecondDir]);
$zip->addGlob('/tmp/Dire/This_Is_A_File', 0, ['add_path' => '/', 'remove_path' => $ThirdDir]);
$zip->addGlob('/tmp/Dire/This_Is_A_File', 0, ['add_path' => '/', 'remove_path' => $dir]);
$zip->close();

if ($zip->open($filename) === true) {

    $extractPath = '/tmp'; 
    $zip->extractTo($extractPath);

    $zip->close();

} else {
    echo "Wrong\n";
}

Observe the output directory structure:

/tmp
├── Dire
│   └── This_Is_A_File
├── HereIsZip.zip
├── ire
│   └── This_Is_A_File
├── mp
│   └── Dire
│       └── This_Is_A_File
├── re
│   └── This_Is_A_File
├── test.php
└── This_Is_A_File

Expected Result:

Files are added to the archive with the specified remove_path prefix removed.

Actual Result:

The remove_path option removes parts of the file path that are not prefixes, leading to unexpected directory structures in the output.

$dir = '/tmp/Dire'     //   =>This_Is_A_File
$FirstDir = '/Dire/';     //   =>ire/This_Is_A_File
$SecondDir = 'This_Is';      //  =>re/This_Is_A_File
$ThirdDir = '/D';    //    =>mp/Dire/This_Is_A_File

Additional Information:

This behavior is inconsistent with the documented purpose of remove_path and affects the usability of the ZipArchive::addGlob method when handling file paths.

The core of the issue seems to be in the way remove_path is checked within the addGlob function. Specifically, the code zval_file = opts.remove_path && strstr(Z_STRVAL_P(zval_file), opts.remove_path appears to detect the presence of remove_path anywhere in the file path, not just at the beginning

if ((zval_file = zend_hash_index_find(Z_ARRVAL_P(return_value), i)) != NULL) {
				if (opts.remove_all_path) {
					basename = php_basename(Z_STRVAL_P(zval_file), Z_STRLEN_P(zval_file), NULL, 0);
					file_stripped = ZSTR_VAL(basename);
					file_stripped_len = ZSTR_LEN(basename);
				} else if (opts.remove_path && strstr(Z_STRVAL_P(zval_file), opts.remove_path) != NULL) {
					if (IS_SLASH(Z_STRVAL_P(zval_file)[opts.remove_path_len])) {
						file_stripped = Z_STRVAL_P(zval_file) + opts.remove_path_len + 1;
						file_stripped_len = Z_STRLEN_P(zval_file) - opts.remove_path_len - 1;
					} else {
						file_stripped = Z_STRVAL_P(zval_file) + opts.remove_path_len;
						file_stripped_len = Z_STRLEN_P(zval_file) - opts.remove_path_len;
					}
				} else {
					file_stripped = Z_STRVAL_P(zval_file);
					file_stripped_len = Z_STRLEN_P(zval_file);
				}

PHP Version

PHP 8.2.12

Operating System

Arch Linux

@Zeroc0077
Copy link

A similar issue in php7.4.
Test code:

<?php
$file = '/tmp/.a/.test';
touch($file);

$zip = new ZipArchive();
$zip->open('test.zip', ZipArchive::CREATE);
$zip->addGlob($file, 0, ['add_path' => '/tmp/', 'remove_path' => '/tmp/./']);

for($i = 0; $i < $zip->numFiles; $i++) {
	$sb = $zip->statIndex($i);
	echo $sb['name'];
}

$zip->close();
echo "\n";

the output:

root@b8e2ac23aa0c:/tmp# php test.php 
/tmp/.test

There should be a problem with the logic of processing ..

@remicollet
Copy link
Contributor

@Zeroc0077 this is a different issue., In such case you have to use realpath to cleanup the option

@remicollet
Copy link
Contributor

Fixed upstream
pierrejoye/php_zip@784e4bc

remicollet added a commit to remicollet/php-src that referenced this issue Nov 14, 2023
@remicollet
Copy link
Contributor

See PR #12666

remicollet added a commit to remicollet/php-src that referenced this issue Nov 14, 2023
remicollet added a commit that referenced this issue Nov 14, 2023
* PHP-8.1:
  NEWS
  fix GH-12661 (Inconsistency in ZipArchive::addGlob remove_path Option Behavior)
remicollet added a commit that referenced this issue Nov 14, 2023
* PHP-8.2:
  NEWS
  NEWS
  fix GH-12661 (Inconsistency in ZipArchive::addGlob remove_path Option Behavior)
remicollet added a commit that referenced this issue Nov 14, 2023
* PHP-8.3:
  NEWS
  NEWS
  fix GH-12661 (Inconsistency in ZipArchive::addGlob remove_path Option Behavior)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants