From d8cf40623eed50390c5d1d4d4bd790fbfbf15fab Mon Sep 17 00:00:00 2001 From: Frode Petterson Date: Mon, 15 Jun 2015 14:57:51 +0200 Subject: [PATCH 1/5] Improved error handling in case upload dir isn't writable, zip isn't enabled or folders are missing. --- h5p.classes.php | 71 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 18 deletions(-) diff --git a/h5p.classes.php b/h5p.classes.php index f0a5f2d..abebd87 100644 --- a/h5p.classes.php +++ b/h5p.classes.php @@ -640,6 +640,22 @@ class H5PValidator { * TRUE if the .h5p file is valid */ public function isValidPackage($skipContent = FALSE, $upgradeOnly = FALSE) { + // Check that directories are writable + if (!H5PCore::dirReady($this->h5pC->path . DIRECTORY_SEPARATOR . 'content')) { + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to write to the content directory.')); + return FALSE; + } + if (!H5PCore::dirReady($this->h5pC->path . DIRECTORY_SEPARATOR . 'libraries')) { + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to write to the libraries directory.')); + return FALSE; + } + + // Make sure Zip is present. + if (!class_exists('ZipArchive')) { + $this->h5pF->setErrorMessage($this->h5pF->t('Your PHP version does not support ZipArchive.')); + return FALSE; + } + // Create a temporary dir to extract package in. $tmpDir = $this->h5pF->getUploadedH5pFolderPath(); $tmpPath = $this->h5pF->getUploadedH5pPath(); @@ -1283,12 +1299,8 @@ class H5PStorage { $contentId = $this->h5pC->saveContent($content, $contentMainId); $this->contentId = $contentId; - $contents_path = $this->h5pC->path . DIRECTORY_SEPARATOR . 'content'; - if (!is_dir($contents_path)) { - mkdir($contents_path, 0777, true); - } - // Move the content folder + $contents_path = $this->h5pC->path . DIRECTORY_SEPARATOR . 'content'; $destination_path = $contents_path . DIRECTORY_SEPARATOR . $contentId; $this->h5pC->copyFileTree($current_path, $destination_path); @@ -1310,12 +1322,6 @@ class H5PStorage { $newOnes = 0; $oldOnes = 0; - // Find libraries directory and make sure it exists - $libraries_path = $this->h5pC->path . DIRECTORY_SEPARATOR . 'libraries'; - if (!is_dir($libraries_path)) { - mkdir($libraries_path, 0777, true); - } - // Go through libraries that came with this package foreach ($this->h5pC->librariesJsonData as $libString => &$library) { // Find local library identifier @@ -1345,6 +1351,7 @@ class H5PStorage { $this->h5pF->saveLibraryData($library, $new); // Make sure destination dir is free + $libraries_path = $this->h5pC->path . DIRECTORY_SEPARATOR . 'libraries'; $destination_path = $libraries_path . DIRECTORY_SEPARATOR . H5PCore::libraryToString($library, TRUE); H5PCore::deleteFileTree($destination_path); @@ -1485,12 +1492,15 @@ Class H5PExport { $tempPath = $h5pDir . 'temp' . DIRECTORY_SEPARATOR . $content['id']; $zipPath = $h5pDir . 'exports' . DIRECTORY_SEPARATOR . $content['slug'] . '-' . $content['id'] . '.h5p'; - // Temp dir to put the h5p files in - @mkdir($tempPath, 0777, TRUE); - @mkdir($h5pDir . 'exports', 0777, TRUE); + // Make sure the exports dir is ready + if (!H5PCore::dirReady($h5pDir . 'exports')) { + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to write to the exports directory.')); + return FALSE; + } // Create content folder if ($this->h5pC->copyFileTree($h5pDir . 'content' . DIRECTORY_SEPARATOR . $content['id'], $tempPath . DIRECTORY_SEPARATOR . 'content') === FALSE) { + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to write to the temporary directory.')); return FALSE; } file_put_contents($tempPath . DIRECTORY_SEPARATOR . 'content' . DIRECTORY_SEPARATOR . 'content.json', $content['params']); @@ -2084,14 +2094,12 @@ class H5PCore { * Indicates if the directory existed. */ public function copyFileTree($source, $destination) { - $dir = opendir($source); - - if ($dir === FALSE) { + if (!H5PCore::dirReady($destination)) { $this->h5pF->setErrorMessage($this->h5pF->t('Unable to copy tree, no such directory: @dir', array('@dir' => $source))); return FALSE; } - @mkdir($destination); + $dir = opendir($source); while (false !== ($file = readdir($dir))) { if (($file != '.') && ($file != '..') && $file != '.git' && $file != '.gitignore') { if (is_dir($source . DIRECTORY_SEPARATOR . $file)) { @@ -2475,6 +2483,33 @@ class H5PCore { return $input; } + + /** + * Recursive function that makes sure the specified directory exists and + * is writable. + * + * @param string $path + * @return bool + */ + public static function dirReady($path) { + if (!file_exists($path)) { + $parent = preg_replace("/\/[^\/]+\/?$/", '', $path); + if (!H5PCore::dirReady($parent)) { + return FALSE; + } + + if (!is_writable($parent)) { + return FALSE; + } + + mkdir($path, 0777, true); + } + if (!is_dir($path)) { + return FALSE; + } + + return is_writable($path); + } } /** From 2beff661953b2262becde58e82887b7ead98b9f9 Mon Sep 17 00:00:00 2001 From: Frode Petterson Date: Tue, 16 Jun 2015 14:52:22 +0200 Subject: [PATCH 2/5] Do not try to validate empty params. --- h5p.classes.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/h5p.classes.php b/h5p.classes.php index abebd87..5cb2f07 100644 --- a/h5p.classes.php +++ b/h5p.classes.php @@ -1766,6 +1766,9 @@ class H5PCore { 'library' => H5PCore::libraryToString($content['library']), 'params' => json_decode($content['params']) ); + if (!$params->params) { + return NULL; + } $validator->validateLibrary($params, (object) array('options' => array($params->library))); $params = json_encode($params->params); From 6ad53d66f3575e68d0ddb2a4fc6b5709883ad49c Mon Sep 17 00:00:00 2001 From: Frode Petterson Date: Mon, 22 Jun 2015 10:02:07 +0200 Subject: [PATCH 3/5] Improve error handling. --- h5p.classes.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/h5p.classes.php b/h5p.classes.php index 5cb2f07..89a3574 100644 --- a/h5p.classes.php +++ b/h5p.classes.php @@ -660,6 +660,11 @@ class H5PValidator { $tmpDir = $this->h5pF->getUploadedH5pFolderPath(); $tmpPath = $this->h5pF->getUploadedH5pPath(); + if (!H5PCore::dirReady($tmpDir)) { + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to write to the temporary directory.')); + return FALSE; + } + $valid = TRUE; // Extract and then remove the package file. From d4ea0a1255cd33f303d26d2744d29d8bb304f14e Mon Sep 17 00:00:00 2001 From: Frode Petterson Date: Mon, 22 Jun 2015 11:22:49 +0200 Subject: [PATCH 4/5] Use warnings to tell the admin what is wrong. Removed duplicate message --- h5p.classes.php | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/h5p.classes.php b/h5p.classes.php index 5cb2f07..bf4320f 100644 --- a/h5p.classes.php +++ b/h5p.classes.php @@ -1500,11 +1500,11 @@ Class H5PExport { // Create content folder if ($this->h5pC->copyFileTree($h5pDir . 'content' . DIRECTORY_SEPARATOR . $content['id'], $tempPath . DIRECTORY_SEPARATOR . 'content') === FALSE) { - $this->h5pF->setErrorMessage($this->h5pF->t('Unable to write to the temporary directory.')); return FALSE; } file_put_contents($tempPath . DIRECTORY_SEPARATOR . 'content' . DIRECTORY_SEPARATOR . 'content.json', $content['params']); + // Make embedTypes into an array $embedTypes = explode(', ', $content['embedType']); // Won't content always be embedded in one way? @@ -2098,11 +2098,16 @@ class H5PCore { */ public function copyFileTree($source, $destination) { if (!H5PCore::dirReady($destination)) { - $this->h5pF->setErrorMessage($this->h5pF->t('Unable to copy tree, no such directory: @dir', array('@dir' => $source))); + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to copy file tree.')); return FALSE; } $dir = opendir($source); + if ($dir === FALSE) { + $this->h5pF->setErrorMessage($this->h5pF->t('Unable to copy file tree.')); + return FALSE; + } + while (false !== ($file = readdir($dir))) { if (($file != '.') && ($file != '..') && $file != '.git' && $file != '.gitignore') { if (is_dir($source . DIRECTORY_SEPARATOR . $file)) { @@ -2501,17 +2506,20 @@ class H5PCore { return FALSE; } - if (!is_writable($parent)) { - return FALSE; - } - mkdir($path, 0777, true); } + if (!is_dir($path)) { + trigger_error('Path is not a directory ' . $path, E_USER_WARNING); return FALSE; } - return is_writable($path); + if (!is_writable($path)) { + trigger_error('Unable to write to ' . $path . ' – check directory permissions –', E_USER_WARNING); + return FALSE; + } + + return TRUE; } } From b9229ba75fb458355acfb0b2d12cec723b719900 Mon Sep 17 00:00:00 2001 From: Frode Petterson Date: Tue, 23 Jun 2015 11:12:29 +0200 Subject: [PATCH 5/5] Don't try to get data when there's no user. --- js/h5p.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/js/h5p.js b/js/h5p.js index 2137da4..04acfe1 100644 --- a/js/h5p.js +++ b/js/h5p.js @@ -1692,6 +1692,12 @@ H5P.createTitle = function (rawTitle, maxLength) { * @param {boolean} [async=true] */ function contentUserDataAjax(contentId, dataType, subContentId, done, data, preload, invalidate, async) { + if (H5PIntegration.user === undefined) { + // Not logged in, no use in saving. + done('Not signed in.'); + return; + } + var options = { url: H5PIntegration.ajax.contentUserData.replace(':contentId', contentId).replace(':dataType', dataType).replace(':subContentId', subContentId ? subContentId : 0), dataType: 'json',