Fix #80833: ZipArchive::getStream doesn't use setPassword#6752
Fix #80833: ZipArchive::getStream doesn't use setPassword#6752cmb69 wants to merge 1 commit intophp:PHP-7.4from
Conversation
As already elaborated in my comment on the ticket[1] this issue cannot be fixed in any stable version due to the ABI break. Thus, this patch adds an optional $password parameter to ::getStream(), so that users can retrieve streams of encrypted files in the archive. This also works for files with individual passwords (i.e. not using the general password of the archive), as can be seen in the accompanying test case. [1] <https://bugs.php.net/80833#1614950012>
|
Note that even if we're postponing the fix to "master" and going to store the default password in the ZipArchive object, adding the optional password parameter to @remicollet, thoughts? |
|
Any thoughts how to proceed here? |
| } | ||
|
|
||
| if (password != NULL) { | ||
| if (zip_set_default_password(stream_za, password) != 0) { |
There was a problem hiding this comment.
Wouldn't it make more sense to use zip_fopen_encrypted, rather than using zip_set_default_password for a single file?
There was a problem hiding this comment.
zip_fopen_encrypted() is only available as of libzip 1.0.0, but even "master" still supports libzip ≥ 0.11. :(
There was a problem hiding this comment.
I see, then this implementation makes sense...
|
Not really sure about this, would be good if @remicollet takes a look. I think the important thing to address is to make use of the password set by Having this as additional functionality still makes sense for file passwords, but I think that would need some more general support. In particular, this allows passing a file password to getStream(), but wouldn't it also make sense for getFromName() and getFromIndex(), which return a string rather than stream? |
|
I need to think more about this... So this is only about the getStream method.... seems a buggy one... |
That's great for single files, but not so when you need to read multiple files from the same archive, which have different passwords. On the other hand, such archives might be very rare. |
Yes, just searching why this doesn't work for getStream, so why we have 2 really different implementation to open a stream with different behavior |
|
Found explanation. On See PR #7359 which use the same zip object, which seems better for performance, and also fix this issue. |
|
Closing in favor of PR #7359. |
4 participants
As already elaborated in my comment on the ticket[1] this issue cannot
be fixed in any stable version due to the ABI break.
Thus, this patch adds an optional $password parameter to ::getStream(),
so that users can retrieve streams of encrypted files in the archive.
This also works for files with individual passwords (i.e. not using the
general password of the archive), as can be seen in the accompanying
test case.
[1] https://bugs.php.net/80833#1614950012