View Issue Details

IDProjectCategoryView StatusLast Update
0016922phplist applicationpluginspublic18-12-14 14:06
Reporterduncanc 
PrioritynormalSeverityminorReproducibilitysometimes
Status resolvedResolutionfixed 
Product Version3.0.5 
Target Version3.0.XFixed in Version3.0.11 
Summary0016922: Install plugin does not handle invalid zip file
DescriptionTrying to track down why a plugin would not install from GitHub, for some reason instead of the zip file being downloaded there was a text file containing some html about being redirected. It looks as if redirects were not handled transparently in that configuration, but that is a different problem.

The failure to install showed that the code to open a zip file does not handle errors correctly.
It should check the return from $zip->open for being a resoure instead of true, as an integer error code can be returned.

See file admin/plugins.php line
change from
  if (!empty($filename) && $zip->open($GLOBALS['tmpdir'].'/phpListPlugin-'.$filename)) {

to
  if (!empty($filename) && is_resource($zip->open($GLOBALS['tmpdir'].'/phpListPlugin-'.$filename))) {

Then at the end of that if statement reinstate the else part that is currently commented out. It might also be useful to include the error code returned by $zip->open although I can't find any explanation of what the codes actually are.

  //} else {
    //Error(s('Invalid plugin package'));

The effect of this problem is that the install does fail later but for an undetermined reason.
TagsNo tags attached.

Activities

michiel

18-12-14 12:00

manager   ~0055775

I've put the error back (Invalid package) but when I tried using the is_resource it failed even on a valid zip file.

duncanc

18-12-14 12:35

developer   ~0055777

My mistake, I was looking at the documentation for the zip_open function instead of the method.
See http://php.net/manual/en/ziparchive.open.php

The line should probably be:

  if (!empty($filename) && $zip->open($GLOBALS['tmpdir'].'/phpListPlugin-'.$filename) === TRUE) {

michiel

18-12-14 13:13

manager   ~0055779

well, but that's the same as just testing on the open result.

duncanc

18-12-14 13:26

developer   ~0055780

I think that you are missing my point.

The return from zip_open() will evaluate true even when it is one of the error codes documented here http://php.net/manual/en/ziparchive.open.php

A strict comparison using === is necessary to test that it actually has returned true instead of an error code. There are some examples on that documentation page.

michiel

18-12-14 14:06

manager   ~0055781

ok, added the evaluation: https://github.com/michield/phplist/commit/d33bed42fcd16035a2b329a5c98ef2915fd76be8