From 51163713c700eb8b03ce5a49615e98f8c7485619 Mon Sep 17 00:00:00 2001 From: oleibman Date: Sat, 24 Jul 2021 20:44:04 -0700 Subject: [PATCH] Tweaks to Input File Validation (#2217) * Tweaks to Input File Validation This started as a response to issue #1718, for which it is a partial (not complete) solution. The following changes are made: - canRead can currently throw an exception. This seems wrong. It should just return true/false. - Breaking change of sorts. When AssertFile encounters a non-existent or unreadable file, it throws InvalidArgumentException. This does not make sense. I have changed it to throw PhpSpreadsheet/Reader/Exception. - Since the previous bullet item required changing of most of the Reader files anyhow, this is a good time to add explicit typing for canRead in the function signature rather than the DocBlock. Since all the canRead functions inherit from an abstract version in IReader, they all have to be changed simulatneously. Except for Xlsx and Ods, most of the Reader files are otherwise unchanged. - AssertFile is changed to add an optional "zip member" parameter. It will check for the existence of an appropriate member in what is supposed to be a zip file. It is used by Xlsx and Ods. - Verifying that a given file is a valid zip ought to be a feature of ZipArchive. Thanks to a particularly nasty bug in php/libzip (see https://bugs.php.net/bug.php?id=81222), it is unsafe to attempt to open a zero-length file as a zip archive. There is a solution, but it does not apply to all the PHP releases which we support, and isn't even necessarily supported on all the point versions of the PHP versions which we do support. I have coded up a manual test for "valid zip", with a comment pointing to the spec. - In theory, tests now cover 100% of the code in Shared/File. In practice ... One of the tests require that chmod works properly, which is not quite true on Windows systems, so that test is skipped on Windows. Another test requires that php.ini uses a non-default value for upload_temp_dir (can't be overridden in application code), which is probably not the case when Github runs the unit tests, so that test is skipped when appropriate. I have run tests for both on systems where they are not skipped. * Update File.php * Scrutinizer Timeout It's not actually timing out, it's just waiting for something to finish that finished ages ago. Making a meaningless comment change in hopes that will clear the jam. Not particularly hopeful. --- phpstan-baseline.neon | 15 --- ...dling_loader_exceptions_using_TryCatch.php | 3 +- src/PhpSpreadsheet/Reader/Csv.php | 12 +- src/PhpSpreadsheet/Reader/Gnumeric.php | 12 +- src/PhpSpreadsheet/Reader/Html.php | 6 +- src/PhpSpreadsheet/Reader/IReader.php | 6 +- src/PhpSpreadsheet/Reader/Ods.php | 83 ++++++-------- src/PhpSpreadsheet/Reader/Slk.php | 9 +- src/PhpSpreadsheet/Reader/Xls.php | 10 +- src/PhpSpreadsheet/Reader/Xlsx.php | 26 ++--- src/PhpSpreadsheet/Reader/Xml.php | 6 +- src/PhpSpreadsheet/Shared/File.php | 103 ++++++++++++------ tests/PhpSpreadsheetTests/IOFactoryTest.php | 6 +- .../Reader/Ods/EmptyFileTest.php | 52 +++++++++ .../Reader/Ods/InvalidFileTest.php | 64 +++++++++++ .../Reader/Xls/NonExistentFileTest.php | 19 ++++ .../Reader/Xlsx/EmptyFileTest.php | 52 +++++++++ .../Reader/Xlsx/InvalidFileTest.php | 64 +++++++++++ tests/PhpSpreadsheetTests/Shared/FileTest.php | 80 ++++++++++++++ 19 files changed, 467 insertions(+), 161 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Reader/Ods/EmptyFileTest.php create mode 100644 tests/PhpSpreadsheetTests/Reader/Ods/InvalidFileTest.php create mode 100644 tests/PhpSpreadsheetTests/Reader/Xls/NonExistentFileTest.php create mode 100644 tests/PhpSpreadsheetTests/Reader/Xlsx/EmptyFileTest.php create mode 100644 tests/PhpSpreadsheetTests/Reader/Xlsx/InvalidFileTest.php diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index b970b1f34..ac4129916 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -3625,21 +3625,6 @@ parameters: count: 1 path: src/PhpSpreadsheet/Shared/Escher/DgContainer/SpgrContainer.php - - - message: "#^Strict comparison using \\=\\=\\= between string\\|false and null will always evaluate to false\\.$#" - count: 1 - path: src/PhpSpreadsheet/Shared/File.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\File\\:\\:realpath\\(\\) should return string but returns string\\|false\\.$#" - count: 1 - path: src/PhpSpreadsheet/Shared/File.php - - - - message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\File\\:\\:sysGetTempDir\\(\\) should return string but returns string\\|false\\.$#" - count: 2 - path: src/PhpSpreadsheet/Shared/File.php - - message: "#^Property PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\Font\\:\\:\\$autoSizeMethods has no typehint specified\\.$#" count: 1 diff --git a/samples/Reader/16_Handling_loader_exceptions_using_TryCatch.php b/samples/Reader/16_Handling_loader_exceptions_using_TryCatch.php index 80bb371d0..5b1029670 100644 --- a/samples/Reader/16_Handling_loader_exceptions_using_TryCatch.php +++ b/samples/Reader/16_Handling_loader_exceptions_using_TryCatch.php @@ -1,6 +1,7 @@ log('Loading file ' . pathinfo($inputFileName, PATHINFO_BASENAME) . ' u try { $spreadsheet = IOFactory::load($inputFileName); -} catch (InvalidArgumentException $e) { +} catch (ReaderException $e) { $helper->log('Error loading file "' . pathinfo($inputFileName, PATHINFO_BASENAME) . '": ' . $e->getMessage()); } diff --git a/src/PhpSpreadsheet/Reader/Csv.php b/src/PhpSpreadsheet/Reader/Csv.php index f06de635a..2d4a01e4b 100644 --- a/src/PhpSpreadsheet/Reader/Csv.php +++ b/src/PhpSpreadsheet/Reader/Csv.php @@ -2,9 +2,9 @@ namespace PhpOffice\PhpSpreadsheet\Reader; -use InvalidArgumentException; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Reader\Csv\Delimiter; +use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; use PhpOffice\PhpSpreadsheet\Shared\StringHelper; use PhpOffice\PhpSpreadsheet\Spreadsheet; @@ -35,7 +35,7 @@ class Csv extends BaseReader private $inputEncoding = 'UTF-8'; /** - * Fallback encoding if 'guess' strikes out. + * Fallback encoding if guess strikes out. * * @var string */ @@ -410,17 +410,13 @@ class Csv extends BaseReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { // Check if file exists try { $this->openFile($pFilename); - } catch (InvalidArgumentException $e) { + } catch (ReaderException $e) { return false; } diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php index 438faea69..98e3f56cc 100644 --- a/src/PhpSpreadsheet/Reader/Gnumeric.php +++ b/src/PhpSpreadsheet/Reader/Gnumeric.php @@ -77,18 +77,12 @@ class Gnumeric extends BaseReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { - File::assertFile($pFilename); - - // Check if gzlib functions are available $data = ''; - if (function_exists('gzread')) { + // Check if gzlib functions are available + if (File::testFileNoThrow($pFilename) && function_exists('gzread')) { // Read signature data (first 3 bytes) $fh = fopen($pFilename, 'rb'); if ($fh !== false) { diff --git a/src/PhpSpreadsheet/Reader/Html.php b/src/PhpSpreadsheet/Reader/Html.php index 342ce5600..60bd7757b 100644 --- a/src/PhpSpreadsheet/Reader/Html.php +++ b/src/PhpSpreadsheet/Reader/Html.php @@ -137,12 +137,8 @@ class Html extends BaseReader /** * Validate that the current file is an HTML file. - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { // Check if file exists try { diff --git a/src/PhpSpreadsheet/Reader/IReader.php b/src/PhpSpreadsheet/Reader/IReader.php index 6a57f4a05..447123cc9 100644 --- a/src/PhpSpreadsheet/Reader/IReader.php +++ b/src/PhpSpreadsheet/Reader/IReader.php @@ -13,12 +13,8 @@ interface IReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename); + public function canRead(string $pFilename): bool; /** * Read data only? diff --git a/src/PhpSpreadsheet/Reader/Ods.php b/src/PhpSpreadsheet/Reader/Ods.php index b39fffedf..a7120507c 100644 --- a/src/PhpSpreadsheet/Reader/Ods.php +++ b/src/PhpSpreadsheet/Reader/Ods.php @@ -10,7 +10,6 @@ use DOMNode; use PhpOffice\PhpSpreadsheet\Calculation\Calculation; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Cell\DataType; -use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; use PhpOffice\PhpSpreadsheet\Reader\Ods\AutoFilter; use PhpOffice\PhpSpreadsheet\Reader\Ods\DefinedNames; use PhpOffice\PhpSpreadsheet\Reader\Ods\PageSettings; @@ -28,6 +27,8 @@ use ZipArchive; class Ods extends BaseReader { + const INITIAL_FILE = 'content.xml'; + /** * Create a new Ods Reader instance. */ @@ -39,46 +40,42 @@ class Ods extends BaseReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { - File::assertFile($pFilename); - $mimeType = 'UNKNOWN'; // Load file - $zip = new ZipArchive(); - if ($zip->open($pFilename) === true) { - // check if it is an OOXML archive - $stat = $zip->statName('mimetype'); - if ($stat && ($stat['size'] <= 255)) { - $mimeType = $zip->getFromName($stat['name']); - } elseif ($zip->statName('META-INF/manifest.xml')) { - $xml = simplexml_load_string( - $this->securityScanner->scan($zip->getFromName('META-INF/manifest.xml')), - 'SimpleXMLElement', - Settings::getLibXmlLoaderOptions() - ); - $namespacesContent = $xml->getNamespaces(true); - if (isset($namespacesContent['manifest'])) { - $manifest = $xml->children($namespacesContent['manifest']); - foreach ($manifest as $manifestDataSet) { - $manifestAttributes = $manifestDataSet->attributes($namespacesContent['manifest']); - if ($manifestAttributes->{'full-path'} == '/') { - $mimeType = (string) $manifestAttributes->{'media-type'}; + if (File::testFileNoThrow($pFilename)) { + $zip = new ZipArchive(); + if ($zip->open($pFilename) === true) { + // check if it is an OOXML archive + $stat = $zip->statName('mimetype'); + if ($stat && ($stat['size'] <= 255)) { + $mimeType = $zip->getFromName($stat['name']); + } elseif ($zip->statName('META-INF/manifest.xml')) { + $xml = simplexml_load_string( + $this->securityScanner->scan($zip->getFromName('META-INF/manifest.xml')), + 'SimpleXMLElement', + Settings::getLibXmlLoaderOptions() + ); + $namespacesContent = $xml->getNamespaces(true); + if (isset($namespacesContent['manifest'])) { + $manifest = $xml->children($namespacesContent['manifest']); + foreach ($manifest as $manifestDataSet) { + $manifestAttributes = $manifestDataSet->attributes($namespacesContent['manifest']); + if ($manifestAttributes->{'full-path'} == '/') { + $mimeType = (string) $manifestAttributes->{'media-type'}; - break; + break; + } } } } - } - $zip->close(); + $zip->close(); + } } return $mimeType === 'application/vnd.oasis.opendocument.spreadsheet'; @@ -93,18 +90,13 @@ class Ods extends BaseReader */ public function listWorksheetNames($pFilename) { - File::assertFile($pFilename); - - $zip = new ZipArchive(); - if ($zip->open($pFilename) !== true) { - throw new ReaderException('Could not open ' . $pFilename . ' for reading! Error opening file.'); - } + File::assertFile($pFilename, self::INITIAL_FILE); $worksheetNames = []; $xml = new XMLReader(); $xml->xml( - $this->securityScanner->scanFile('zip://' . realpath($pFilename) . '#content.xml'), + $this->securityScanner->scanFile('zip://' . realpath($pFilename) . '#' . self::INITIAL_FILE), null, Settings::getLibXmlLoaderOptions() ); @@ -145,18 +137,13 @@ class Ods extends BaseReader */ public function listWorksheetInfo($pFilename) { - File::assertFile($pFilename); + File::assertFile($pFilename, self::INITIAL_FILE); $worksheetInfo = []; - $zip = new ZipArchive(); - if ($zip->open($pFilename) !== true) { - throw new ReaderException('Could not open ' . $pFilename . ' for reading! Error opening file.'); - } - $xml = new XMLReader(); $xml->xml( - $this->securityScanner->scanFile('zip://' . realpath($pFilename) . '#content.xml'), + $this->securityScanner->scanFile('zip://' . realpath($pFilename) . '#' . self::INITIAL_FILE), null, Settings::getLibXmlLoaderOptions() ); @@ -253,12 +240,10 @@ class Ods extends BaseReader */ public function loadIntoExisting($pFilename, Spreadsheet $spreadsheet) { - File::assertFile($pFilename); + File::assertFile($pFilename, self::INITIAL_FILE); $zip = new ZipArchive(); - if ($zip->open($pFilename) !== true) { - throw new Exception("Could not open {$pFilename} for reading! Error opening file."); - } + $zip->open($pFilename); // Meta @@ -289,7 +274,7 @@ class Ods extends BaseReader $dom = new DOMDocument('1.01', 'UTF-8'); $dom->loadXML( - $this->securityScanner->scan($zip->getFromName('content.xml')), + $this->securityScanner->scan($zip->getFromName(self::INITIAL_FILE)), Settings::getLibXmlLoaderOptions() ); diff --git a/src/PhpSpreadsheet/Reader/Slk.php b/src/PhpSpreadsheet/Reader/Slk.php index 4e76a1616..2eca85de4 100644 --- a/src/PhpSpreadsheet/Reader/Slk.php +++ b/src/PhpSpreadsheet/Reader/Slk.php @@ -2,7 +2,6 @@ namespace PhpOffice\PhpSpreadsheet\Reader; -use InvalidArgumentException; use PhpOffice\PhpSpreadsheet\Calculation\Calculation; use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; @@ -65,16 +64,12 @@ class Slk extends BaseReader /** * Validate that the current file is a SYLK file. - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { try { $this->openFile($pFilename); - } catch (InvalidArgumentException $e) { + } catch (ReaderException $e) { return false; } diff --git a/src/PhpSpreadsheet/Reader/Xls.php b/src/PhpSpreadsheet/Reader/Xls.php index 1077e9801..42f5fceed 100644 --- a/src/PhpSpreadsheet/Reader/Xls.php +++ b/src/PhpSpreadsheet/Reader/Xls.php @@ -419,14 +419,12 @@ class Xls extends BaseReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { - File::assertFile($pFilename); + if (!File::testFileNoThrow($pFilename)) { + return false; + } try { // Use ParseXL for the hard work. diff --git a/src/PhpSpreadsheet/Reader/Xlsx.php b/src/PhpSpreadsheet/Reader/Xlsx.php index d78b227eb..5d379dafa 100644 --- a/src/PhpSpreadsheet/Reader/Xlsx.php +++ b/src/PhpSpreadsheet/Reader/Xlsx.php @@ -41,6 +41,8 @@ use ZipArchive; class Xlsx extends BaseReader { + const INITIAL_FILE = '_rels/.rels'; + /** * ReferenceHelper instance. * @@ -72,14 +74,12 @@ class Xlsx extends BaseReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { - File::assertFile($pFilename); + if (!File::testFileNoThrow($pFilename, self::INITIAL_FILE)) { + return false; + } $result = false; $this->zip = $zip = new ZipArchive(); @@ -168,7 +168,7 @@ class Xlsx extends BaseReader */ public function listWorksheetNames($pFilename) { - File::assertFile($pFilename); + File::assertFile($pFilename, self::INITIAL_FILE); $worksheetNames = []; @@ -176,7 +176,7 @@ class Xlsx extends BaseReader $zip->open($pFilename); // The files we're looking at here are small enough that simpleXML is more efficient than XMLReader - $rels = $this->loadZip('_rels/.rels', Namespaces::RELATIONSHIPS); + $rels = $this->loadZip(self::INITIAL_FILE, Namespaces::RELATIONSHIPS); foreach ($rels->Relationship as $relx) { $rel = self::getAttributes($relx); $relType = (string) $rel['Type']; @@ -207,14 +207,14 @@ class Xlsx extends BaseReader */ public function listWorksheetInfo($pFilename) { - File::assertFile($pFilename); + File::assertFile($pFilename, self::INITIAL_FILE); $worksheetInfo = []; $this->zip = $zip = new ZipArchive(); $zip->open($pFilename); - $rels = $this->loadZip('_rels/.rels', Namespaces::RELATIONSHIPS); + $rels = $this->loadZip(self::INITIAL_FILE, Namespaces::RELATIONSHIPS); foreach ($rels->Relationship as $relx) { $rel = self::getAttributes($relx); $relType = (string) $rel['Type']; @@ -389,7 +389,7 @@ class Xlsx extends BaseReader */ public function load(string $pFilename, int $flags = 0): Spreadsheet { - File::assertFile($pFilename); + File::assertFile($pFilename, self::INITIAL_FILE); $this->processFlags($flags); // Initialisations @@ -444,7 +444,7 @@ class Xlsx extends BaseReader } } - $rels = $this->loadZip('_rels/.rels', Namespaces::RELATIONSHIPS); + $rels = $this->loadZip(self::INITIAL_FILE, Namespaces::RELATIONSHIPS); $propertyReader = new PropertyReader($this->securityScanner, $excel->getProperties()); foreach ($rels->Relationship as $relx) { @@ -1957,7 +1957,7 @@ class Xlsx extends BaseReader $xmlNamespaceBase = ''; // check if it is an OOXML archive - $rels = $this->loadZip('_rels/.rels'); + $rels = $this->loadZip(self::INITIAL_FILE); foreach ($rels->children(Namespaces::RELATIONSHIPS)->Relationship as $rel) { $rel = self::getAttributes($rel); switch ($rel['Type']) { diff --git a/src/PhpSpreadsheet/Reader/Xml.php b/src/PhpSpreadsheet/Reader/Xml.php index f63bc7987..ae698cefd 100644 --- a/src/PhpSpreadsheet/Reader/Xml.php +++ b/src/PhpSpreadsheet/Reader/Xml.php @@ -53,12 +53,8 @@ class Xml extends BaseReader /** * Can the current IReader read the file? - * - * @param string $pFilename - * - * @return bool */ - public function canRead($pFilename) + public function canRead(string $pFilename): bool { // Office xmlns:o="urn:schemas-microsoft-com:office:office" // Excel xmlns:x="urn:schemas-microsoft-com:office:excel" diff --git a/src/PhpSpreadsheet/Shared/File.php b/src/PhpSpreadsheet/Shared/File.php index 7991ed458..aa917e3f2 100644 --- a/src/PhpSpreadsheet/Shared/File.php +++ b/src/PhpSpreadsheet/Shared/File.php @@ -2,8 +2,8 @@ namespace PhpOffice\PhpSpreadsheet\Shared; -use InvalidArgumentException; use PhpOffice\PhpSpreadsheet\Exception; +use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; use ZipArchive; class File @@ -17,47 +17,57 @@ class File /** * Set the flag indicating whether the File Upload Temp directory should be used for temporary files. - * - * @param bool $useUploadTempDir Use File Upload Temporary directory (true or false) */ - public static function setUseUploadTempDirectory($useUploadTempDir): void + public static function setUseUploadTempDirectory(bool $useUploadTempDir): void { self::$useUploadTempDirectory = (bool) $useUploadTempDir; } /** * Get the flag indicating whether the File Upload Temp directory should be used for temporary files. - * - * @return bool Use File Upload Temporary directory (true or false) */ - public static function getUseUploadTempDirectory() + public static function getUseUploadTempDirectory(): bool { return self::$useUploadTempDirectory; } + // https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT + // Section 4.3.7 + // Looks like there might be endian-ness considerations + private const ZIP_FIRST_4 = [ + "\x50\x4b\x03\x04", // what it looks like on my system + "\x04\x03\x4b\x50", // what it says in documentation + ]; + + private static function validateZipFirst4(string $zipFile): bool + { + $contents = @file_get_contents($zipFile, false, null, 0, 4); + + return in_array($contents, self::ZIP_FIRST_4, true); + } + /** * Verify if a file exists. - * - * @param string $pFilename Filename - * - * @return bool */ - public static function fileExists($pFilename) + public static function fileExists(string $pFilename): bool { // Sick construction, but it seems that // file_exists returns strange values when // doing the original file_exists on ZIP archives... - if (strtolower(substr($pFilename, 0, 3)) == 'zip') { + if (strtolower(substr($pFilename, 0, 6)) == 'zip://') { // Open ZIP file and verify if the file exists $zipFile = substr($pFilename, 6, strpos($pFilename, '#') - 6); $archiveFile = substr($pFilename, strpos($pFilename, '#') + 1); - $zip = new ZipArchive(); - if ($zip->open($zipFile) === true) { - $returnValue = ($zip->getFromName($archiveFile) !== false); - $zip->close(); + if (self::validateZipFirst4($zipFile)) { + $zip = new ZipArchive(); + $res = $zip->open($zipFile, ZipArchive::CHECKCONS); + if ($res === true) { + $returnValue = ($zip->getFromName($archiveFile) !== false); + $zip->close(); - return $returnValue; + return $returnValue; + } } return false; @@ -68,23 +78,19 @@ class File /** * Returns canonicalized absolute pathname, also for ZIP archives. - * - * @param string $pFilename - * - * @return string */ - public static function realpath($pFilename) + public static function realpath(string $pFilename): string { // Returnvalue $returnValue = ''; // Try using realpath() if (file_exists($pFilename)) { - $returnValue = realpath($pFilename); + $returnValue = realpath($pFilename) ?: ''; } // Found something? - if ($returnValue == '' || ($returnValue === null)) { + if ($returnValue === '') { $pathArray = explode('/', $pFilename); while (in_array('..', $pathArray) && $pathArray[0] != '..') { $iMax = count($pathArray); @@ -105,24 +111,23 @@ class File /** * Get the systems temporary directory. - * - * @return string */ - public static function sysGetTempDir() + public static function sysGetTempDir(): string { + $path = sys_get_temp_dir(); if (self::$useUploadTempDirectory) { // use upload-directory when defined to allow running on environments having very restricted // open_basedir configs if (ini_get('upload_tmp_dir') !== false) { if ($temp = ini_get('upload_tmp_dir')) { if (file_exists($temp)) { - return realpath($temp); + $path = $temp; } } } } - return realpath(sys_get_temp_dir()); + return realpath($path) ?: ''; } public static function temporaryFilename(): string @@ -137,17 +142,45 @@ class File /** * Assert that given path is an existing file and is readable, otherwise throw exception. - * - * @param string $filename */ - public static function assertFile($filename): void + public static function assertFile(string $filename, string $zipMember = ''): void { if (!is_file($filename)) { - throw new InvalidArgumentException('File "' . $filename . '" does not exist.'); + throw new ReaderException('File "' . $filename . '" does not exist.'); } if (!is_readable($filename)) { - throw new InvalidArgumentException('Could not open "' . $filename . '" for reading.'); + throw new ReaderException('Could not open "' . $filename . '" for reading.'); + } + + if ($zipMember !== '') { + $zipfile = "zip://$filename#$zipMember"; + if (!self::fileExists($zipfile)) { + throw new ReaderException("Could not find zip member $zipfile"); + } } } + + /** + * Same as assertFile, except return true/false and don't throw Exception. + */ + public static function testFileNoThrow(string $filename, string $zipMember = ''): bool + { + if (!is_file($filename)) { + return false; + } + + if (!is_readable($filename)) { + return false; + } + + if ($zipMember !== '') { + $zipfile = "zip://$filename#$zipMember"; + if (!self::fileExists($zipfile)) { + return false; + } + } + + return true; + } } diff --git a/tests/PhpSpreadsheetTests/IOFactoryTest.php b/tests/PhpSpreadsheetTests/IOFactoryTest.php index 4837d6cf8..b8c6ff835 100644 --- a/tests/PhpSpreadsheetTests/IOFactoryTest.php +++ b/tests/PhpSpreadsheetTests/IOFactoryTest.php @@ -2,9 +2,9 @@ namespace PhpOffice\PhpSpreadsheetTests; -use InvalidArgumentException; use PhpOffice\PhpSpreadsheet\IOFactory; use PhpOffice\PhpSpreadsheet\Reader; +use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; use PhpOffice\PhpSpreadsheet\Spreadsheet; use PhpOffice\PhpSpreadsheet\Writer; use PHPUnit\Framework\TestCase; @@ -136,14 +136,14 @@ class IOFactoryTest extends TestCase public function testIdentifyNonExistingFileThrowException(): void { - $this->expectException(InvalidArgumentException::class); + $this->expectException(ReaderException::class); IOFactory::identify('/non/existing/file'); } public function testIdentifyExistingDirectoryThrowExceptions(): void { - $this->expectException(InvalidArgumentException::class); + $this->expectException(ReaderException::class); IOFactory::identify('.'); } diff --git a/tests/PhpSpreadsheetTests/Reader/Ods/EmptyFileTest.php b/tests/PhpSpreadsheetTests/Reader/Ods/EmptyFileTest.php new file mode 100644 index 000000000..5876eaab1 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Ods/EmptyFileTest.php @@ -0,0 +1,52 @@ +tempfile !== '') { + unlink($this->tempfile); + $this->tempfile = ''; + } + } + + public function testEmptyFileLoad(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + $reader = new Ods(); + $reader->load($temp); + } + + public function testEmptyFileNames(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + $reader = new Ods(); + $reader->listWorksheetNames($temp); + } + + public function testEmptyInfo(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + $reader = new Ods(); + $reader->listWorksheetInfo($temp); + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Ods/InvalidFileTest.php b/tests/PhpSpreadsheetTests/Reader/Ods/InvalidFileTest.php new file mode 100644 index 000000000..d56af4ded --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Ods/InvalidFileTest.php @@ -0,0 +1,64 @@ +expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = __FILE__; + $reader = new Ods(); + $reader->load($temp); + } + + public function testInvalidFileNames(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = __FILE__; + $reader = new Ods(); + $reader->listWorksheetNames($temp); + } + + public function testInvalidInfo(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = __FILE__; + $reader = new Ods(); + $reader->listWorksheetInfo($temp); + } + + public function testXlsxFileLoad(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = 'samples/templates/26template.xlsx'; + $reader = new Ods(); + $reader->load($temp); + } + + public function testXlsxFileNames(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = 'samples/templates/26template.xlsx'; + $reader = new Ods(); + $reader->listWorksheetNames($temp); + } + + public function testXlsxInfo(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = 'samples/templates/26template.xlsx'; + $reader = new Ods(); + $reader->listWorksheetInfo($temp); + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Xls/NonExistentFileTest.php b/tests/PhpSpreadsheetTests/Reader/Xls/NonExistentFileTest.php new file mode 100644 index 000000000..76db7b62a --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xls/NonExistentFileTest.php @@ -0,0 +1,19 @@ +canRead($temp)); + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/EmptyFileTest.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/EmptyFileTest.php new file mode 100644 index 000000000..a2e78da4a --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/EmptyFileTest.php @@ -0,0 +1,52 @@ +tempfile !== '') { + unlink($this->tempfile); + $this->tempfile = ''; + } + } + + public function testEmptyFileLoad(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + $reader = new Xlsx(); + $reader->load($temp); + } + + public function testEmptyFileNames(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + $reader = new Xlsx(); + $reader->listWorksheetNames($temp); + } + + public function testEmptyInfo(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + $reader = new Xlsx(); + $reader->listWorksheetInfo($temp); + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/InvalidFileTest.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/InvalidFileTest.php new file mode 100644 index 000000000..05acd8743 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/InvalidFileTest.php @@ -0,0 +1,64 @@ +expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = __FILE__; + $reader = new Xlsx(); + $reader->load($temp); + } + + public function testInvalidFileNames(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = __FILE__; + $reader = new Xlsx(); + $reader->listWorksheetNames($temp); + } + + public function testInvalidInfo(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = __FILE__; + $reader = new Xlsx(); + $reader->listWorksheetInfo($temp); + } + + public function testOdsFileLoad(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = 'samples/templates/OOCalcTest.ods'; + $reader = new Xlsx(); + $reader->load($temp); + } + + public function testOdsFileNames(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = 'samples/templates/OOCalcTest.ods'; + $reader = new Xlsx(); + $reader->listWorksheetNames($temp); + } + + public function testOdsInfo(): void + { + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + $temp = 'samples/templates/OOCalcTest.ods'; + $reader = new Xlsx(); + $reader->listWorksheetInfo($temp); + } +} diff --git a/tests/PhpSpreadsheetTests/Shared/FileTest.php b/tests/PhpSpreadsheetTests/Shared/FileTest.php index 6b9500f6f..ddc54b5ee 100644 --- a/tests/PhpSpreadsheetTests/Shared/FileTest.php +++ b/tests/PhpSpreadsheetTests/Shared/FileTest.php @@ -2,11 +2,32 @@ namespace PhpOffice\PhpSpreadsheetTests\Shared; +use PhpOffice\PhpSpreadsheet\Reader\Exception as ReaderException; use PhpOffice\PhpSpreadsheet\Shared\File; use PHPUnit\Framework\TestCase; class FileTest extends TestCase { + /** @var bool */ + private $uploadFlag = false; + + /** @var string */ + private $tempfile = ''; + + protected function setUp(): void + { + $this->uploadFlag = File::getUseUploadTempDirectory(); + } + + protected function tearDown(): void + { + File::setUseUploadTempDirectory($this->uploadFlag); + if ($this->tempfile !== '') { + unlink($this->tempfile); + $this->tempfile = ''; + } + } + public function testGetUseUploadTempDirectory(): void { $expectedResult = false; @@ -21,12 +42,71 @@ class FileTest extends TestCase true, false, ]; + $temp = ini_get('upload_tmp_dir') ?: ''; + $badArray = ['', sys_get_temp_dir()]; foreach ($useUploadTempDirectoryValues as $useUploadTempDirectoryValue) { File::setUseUploadTempDirectory($useUploadTempDirectoryValue); $result = File::getUseUploadTempDirectory(); self::assertEquals($useUploadTempDirectoryValue, $result); + $result = File::sysGetTempDir(); + if (!$useUploadTempDirectoryValue || in_array($temp, $badArray, true)) { + self::assertSame(realpath(sys_get_temp_dir()), $result); + } else { + self::assertSame(realpath($temp), $result); + } } } + + public function testUploadTmpDir(): void + { + $temp = ini_get('upload_tmp_dir') ?: ''; + $badArray = ['', sys_get_temp_dir()]; + if (in_array($temp, $badArray, true)) { + self::markTestSkipped('upload_tmp_dir setting unusable for this test'); + } else { + File::setUseUploadTempDirectory(true); + $result = File::sysGetTempDir(); + self::assertSame(realpath($temp), $result); + } + } + + public function testNotExists(): void + { + $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + File::assertFile($temp); + self::assertTrue(File::testFileNoThrow($temp)); + unlink($temp); + self::assertFalse(File::testFileNoThrow($temp)); + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('does not exist'); + File::assertFile($temp); + } + + public function testNotReadable(): void + { + if (PHP_OS_FAMILY === 'Windows') { + self::markTestSkipped('chmod does not work reliably on Windows'); + } + $this->tempfile = $temp = File::temporaryFileName(); + file_put_contents($temp, ''); + chmod($temp, 0070); + self::assertFalse(File::testFileNoThrow($temp)); + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('for reading'); + File::assertFile($temp); + } + + public function testZip(): void + { + $temp = 'samples/templates/26template.xlsx'; + File::assertFile($temp, 'xl/workbook.xml'); + self::assertTrue(File::testFileNoThrow($temp, 'xl/workbook.xml')); + self::assertFalse(File::testFileNoThrow($temp, 'xl/xworkbook.xml')); + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Could not find zip member'); + File::assertFile($temp, 'xl/xworkbook.xml'); + } }