Skip to content

Commit

Permalink
Dev: Misc fixes in plugin zip uploader (TODO: permissions in mkdir)
Browse files Browse the repository at this point in the history
  • Loading branch information
olleharstedt committed Feb 23, 2021
1 parent 899bc0c commit 7f99a8e
Showing 1 changed file with 20 additions and 15 deletions.
35 changes: 20 additions & 15 deletions application/libraries/ExtensionInstaller/FileFetcherUploadZip.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace LimeSurvey\ExtensionInstaller;

use Exception;

/**
* Extension file fetcher for upload ZIP file.
* Must work for all extension types: plugins, theme, question theme, etc.
Expand Down Expand Up @@ -50,33 +52,36 @@ public function move($destdir)

$tempdir = $this->getTempdir();
if (empty($tempdir)) {
throw new \Exception(gT('Temporary folder cannot be determined.'));
throw new Exception(gT('Temporary folder cannot be determined.'));
}

if (!file_exists($tempdir)) {
throw new \Exception(gT('Temporary folder does not exist.'));
throw new Exception(gT('Temporary folder does not exist.'));
}

// TODO: Correct permission?
mkdir($destdir, 0760, true);

if (!is_writable(dirname($destdir))) {
throw new \Exception(gT('Cannot move files due to permission problem.'));
throw new Exception(gT('Cannot move files due to permission problem. ' . $destdir));
}

if (file_exists($destdir) && !rmdirr($destdir)) {
throw new \Exception('Could not remove old files.');
throw new Exception('Could not remove old files.');
}

return $this->recurseCopy($tempdir, $destdir);
}

/**
* @return SimpleXMLElement
* @return \ExtensionConfig
* @throws Exception
*/
public function getConfig()
{
$tempdir = $this->getTempdir();
if (empty($tempdir)) {
throw new \Exception(gT('No temporary folder, cannot read configuration file.'));
throw new Exception(gT('No temporary folder, cannot read configuration file.'));
}

$configFile = $tempdir . DIRECTORY_SEPARATOR . 'config.xml';
Expand All @@ -96,17 +101,17 @@ public function getConfig()
//set new config file
$configFile = $configXml;
} else {
throw new \Exception(gT('Configuration file config.xml does not exist.'));
throw new Exception(gT('Configuration file config.xml does not exist.'));
}
} else {
throw new \Exception(gT('Configuration file config.xml does not exist.'));
throw new Exception(gT('Configuration file config.xml does not exist.'));
}
}

$config = \ExtensionConfig::loadConfigFromFile($configFile);

if (empty($config)) {
throw new \Exception(gT('Could not parse config.xml file.'));
throw new Exception(gT('Could not parse config.xml file.'));
}

return $config;
Expand Down Expand Up @@ -171,11 +176,11 @@ protected function clearTmpdir()
protected function checkFileSizeError()
{
if (!isset($_FILES['the_file'])) {
throw new \Exception(gT('Found no file'));
throw new Exception(gT('Found no file'));
}

if ($_FILES['the_file']['error'] == 1 || $_FILES['the_file']['error'] == 2) {
throw new \Exception(
throw new Exception(
sprintf(
gT("Sorry, this file is too large. Only files up to %01.2f MB are allowed."),
getMaximumFileUploadSize() / 1024 / 1024
Expand All @@ -193,7 +198,7 @@ protected function checkZipBom()
// Check zip bomb.
\Yii::import('application.helpers.common_helper', true);
if (isZipBomb($_FILES['the_file']['name'])) {
throw new \Exception(gT('Unzipped file is too big.'));
throw new Exception(gT('Unzipped file is too big.'));
}
}

Expand All @@ -209,13 +214,13 @@ protected function extractZipFile($tempdir)
$this->checkZipBom();

if (!is_file($_FILES['the_file']['tmp_name'])) {
throw new \Exception(
throw new Exception(
gT("An error occurred uploading your file. This may be caused by incorrect permissions for the application /tmp folder.")
);
}

if (empty($this->filterName)) {
throw new \Exception("No filter name is set, can't unzip.");
throw new Exception("No filter name is set, can't unzip.");
}

$zip = new \PclZip($_FILES['the_file']['tmp_name']);
Expand All @@ -227,7 +232,7 @@ protected function extractZipFile($tempdir)
);

if ($aExtractResult === 0) {
throw new \Exception(
throw new Exception(
gT("This file is not a valid ZIP file archive. Import failed.")
. ' ' . $zip->error_string
);
Expand Down

5 comments on commit 7f99a8e

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: permissions in mkdir

What do you mean ? Set permission in mkdir ?

@olleharstedt
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: permissions in mkdir

What do you mean ? Set permission in mkdir ?

umask, but I think it'll be mkdir($dir, umask(), true);

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind : a lot of server force umask here or need specific umask : maybe leave default ?
Or use acl or … or … or …

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here there are PHP file : suPHP for example have strict rule to allow PHP file exec.

@olleharstedt
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umask() returns the server default, I think.

Please sign in to comment.