Stricter check whether file exists before reading

This should avoid issues when user submit a non-existing filename
that is wrongly identified as CSV.

Typically https://github.com/PHPOffice/PHPExcel/issues/1076 and
https://github.com/PHPOffice/PHPExcel/issues/1078 could have been
avoided.

src/PhpSpreadsheet/IOFactory.php

@@ -2,6 +2,8 @@
 
 namespace PhpOffice\PhpSpreadsheet;
 
+use PhpOffice\PhpSpreadsheet\Shared\File;
+
 /**
  * Copyright (c) 2006 - 2016 PhpSpreadsheet
  *
@@ -202,6 +204,8 @@ class IOFactory
      */
     public static function createReaderForFile($pFilename)
     {
+        File::assertFile($pFilename);
+
         // First, lucky guess by inspecting file extension
         $pathinfo = pathinfo($pFilename);
 

src/PhpSpreadsheet/Reader/BaseReader.php

@@ -2,6 +2,8 @@
 
 namespace PhpOffice\PhpSpreadsheet\Reader;
 
+use PhpOffice\PhpSpreadsheet\Shared\File;
+
 /**
  * Copyright (c) 2006 - 2016 PhpSpreadsheet
  *
@@ -231,10 +233,7 @@ abstract class BaseReader implements IReader
      */
     protected function openFile($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename) || !is_readable($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         // Open file
         $this->fileHandle = fopen($pFilename, 'r');

src/PhpSpreadsheet/Reader/Excel2003XML.php

@@ -2,6 +2,7 @@
 
 namespace PhpOffice\PhpSpreadsheet\Reader;
 
+use PhpOffice\PhpSpreadsheet\Shared\File;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 
 /**
@@ -108,10 +109,7 @@ class Excel2003XML extends BaseReader implements IReader
      */
     public function listWorksheetNames($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
         if (!$this->canRead($pFilename)) {
             throw new Exception($pFilename . ' is an Invalid Spreadsheet file.');
         }
@@ -142,10 +140,7 @@ class Excel2003XML extends BaseReader implements IReader
      */
     public function listWorksheetInfo($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $worksheetInfo = [];
 
@@ -311,11 +306,7 @@ class Excel2003XML extends BaseReader implements IReader
         $timezoneObj = new \DateTimeZone('Europe/London');
         $GMT = new \DateTimeZone('UTC');
 
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
-
         if (!$this->canRead($pFilename)) {
             throw new Exception($pFilename . ' is an Invalid Spreadsheet file.');
         }

src/PhpSpreadsheet/Reader/Gnumeric.php

@@ -3,6 +3,7 @@
 namespace PhpOffice\PhpSpreadsheet\Reader;
 
 use DateTimeZone;
+use PhpOffice\PhpSpreadsheet\Shared\File;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 
 /**
@@ -62,10 +63,7 @@ class Gnumeric extends BaseReader implements IReader
      */
     public function canRead($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         // Check if gzlib functions are available
         if (!function_exists('gzread')) {
@@ -92,10 +90,7 @@ class Gnumeric extends BaseReader implements IReader
      */
     public function listWorksheetNames($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $xml = new XMLReader();
         $xml->xml($this->securityScanFile('compress.zlib://' . realpath($pFilename)), null, \PhpOffice\PhpSpreadsheet\Settings::getLibXmlLoaderOptions());
@@ -123,10 +118,7 @@ class Gnumeric extends BaseReader implements IReader
      */
     public function listWorksheetInfo($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $xml = new XMLReader();
         $xml->xml($this->securityScanFile('compress.zlib://' . realpath($pFilename)), null, \PhpOffice\PhpSpreadsheet\Settings::getLibXmlLoaderOptions());
@@ -208,10 +200,7 @@ class Gnumeric extends BaseReader implements IReader
      */
     public function loadIntoExisting($pFilename, Spreadsheet $spreadsheet)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $timezoneObj = new DateTimeZone('Europe/London');
         $GMT = new DateTimeZone('UTC');

src/PhpSpreadsheet/Reader/Ods.php

@@ -4,6 +4,7 @@ namespace PhpOffice\PhpSpreadsheet\Reader;
 
 use DateTime;
 use DateTimeZone;
+use PhpOffice\PhpSpreadsheet\Shared\File;
 
 /**
  * Copyright (c) 2006 - 2016 PhpSpreadsheet
@@ -52,18 +53,10 @@ class Ods extends BaseReader implements IReader
      */
     public function canRead($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $zipClass = \PhpOffice\PhpSpreadsheet\Settings::getZipClass();
 
-        // Check if zip class exists
-//        if (!class_exists($zipClass, false)) {
-//            throw new Exception($zipClass . " library is not enabled");
-//        }
-
         $mimeType = 'UNKNOWN';
         // Load file
         $zip = new $zipClass();
@@ -107,10 +100,7 @@ class Ods extends BaseReader implements IReader
      */
     public function listWorksheetNames($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $zipClass = \PhpOffice\PhpSpreadsheet\Settings::getZipClass();
 
@@ -163,10 +153,7 @@ class Ods extends BaseReader implements IReader
      */
     public function listWorksheetInfo($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $worksheetInfo = [];
 
@@ -289,10 +276,7 @@ class Ods extends BaseReader implements IReader
      */
     public function loadIntoExisting($pFilename, \PhpOffice\PhpSpreadsheet\Spreadsheet $spreadsheet)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $timezoneObj = new DateTimeZone('Europe/London');
         $GMT = new \DateTimeZone('UTC');

src/PhpSpreadsheet/Reader/Xls.php

@@ -2,6 +2,8 @@
 
 namespace PhpOffice\PhpSpreadsheet\Reader;
 
+use PhpOffice\PhpSpreadsheet\Shared\File;
+
 /**
  * Copyright (c) 2006 - 2016 PhpSpreadsheet
  *
@@ -417,10 +419,7 @@ class Xls extends BaseReader implements IReader
      */
     public function canRead($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         try {
             // Use ParseXL for the hard work.
@@ -443,10 +442,7 @@ class Xls extends BaseReader implements IReader
      */
     public function listWorksheetNames($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $worksheetNames = [];
 
@@ -499,10 +495,7 @@ class Xls extends BaseReader implements IReader
      */
     public function listWorksheetInfo($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $worksheetInfo = [];
 

src/PhpSpreadsheet/Reader/Xlsx.php

@@ -2,6 +2,8 @@
 
 namespace PhpOffice\PhpSpreadsheet\Reader;
 
+use PhpOffice\PhpSpreadsheet\Shared\File;
+
 /**
  * Copyright (c) 2006 - 2016 PhpSpreadsheet
  *
@@ -57,10 +59,7 @@ class Xlsx extends BaseReader implements IReader
      */
     public function canRead($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $zipClass = \PhpOffice\PhpSpreadsheet\Settings::getZipClass();
 
@@ -106,10 +105,7 @@ class Xlsx extends BaseReader implements IReader
      */
     public function listWorksheetNames($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $worksheetNames = [];
 
@@ -159,10 +155,7 @@ class Xlsx extends BaseReader implements IReader
      */
     public function listWorksheetInfo($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         $worksheetInfo = [];
 
@@ -338,10 +331,7 @@ class Xlsx extends BaseReader implements IReader
      */
     public function load($pFilename)
     {
-        // Check if file exists
+        File::assertFile($pFilename);
-        if (!file_exists($pFilename)) {
-            throw new Exception('Could not open ' . $pFilename . ' for reading! File does not exist.');
-        }
 
         // Initialisations
         $excel = new \PhpOffice\PhpSpreadsheet\Spreadsheet();

src/PhpSpreadsheet/Shared/File.php

@@ -31,6 +31,7 @@ class File
      * @protected
      * @var    boolean
      */
+
     protected static $useUploadTempDirectory = false;
 
     /**
@@ -175,4 +176,20 @@ class File
         //        be called if we're running 5.2.1 or earlier
         return realpath(sys_get_temp_dir());
     }
+
+    /**
+     * Assert that given path is an existing file and is readable, otherwise throw exception
+     * @param string $filename
+     * @throws \InvalidArgumentException
+     */
+    public static function assertFile($filename)
+    {
+        if (!is_file($filename)) {
+            throw new \InvalidArgumentException('File "' . $filename . '" does not exist.');
+        }
+
+        if (!is_readable($filename)) {
+            throw new \InvalidArgumentException('Could not open "' . $filename . '" for reading.');
+        }
+    }
 }

src/PhpSpreadsheet/Shared/OLERead.php

@@ -69,27 +69,24 @@ class OLERead
     /**
      * Read the file
      *
-     * @param $sFileName string Filename
+     * @param $pFilename string Filename
      * @throws \PhpOffice\PhpSpreadsheet\Reader\Exception
      */
-    public function read($sFileName)
+    public function read($pFilename)
     {
-        // Check if file exists and is readable
+        File::assertFile($pFilename);
-        if (!is_readable($sFileName)) {
-            throw new \PhpOffice\PhpSpreadsheet\Reader\Exception('Could not open ' . $sFileName . ' for reading! File does not exist, or it is not readable.');
-        }
 
         // Get the file identifier
         // Don't bother reading the whole file until we know it's a valid OLE file
-        $this->data = file_get_contents($sFileName, false, null, 0, 8);
+        $this->data = file_get_contents($pFilename, false, null, 0, 8);
 
         // Check OLE identifier
         if ($this->data != self::IDENTIFIER_OLE) {
-            throw new \PhpOffice\PhpSpreadsheet\Reader\Exception('The filename ' . $sFileName . ' is not recognised as an OLE file');
+            throw new \PhpOffice\PhpSpreadsheet\Reader\Exception('The filename ' . $pFilename . ' is not recognised as an OLE file');
         }
 
         // Get the file data
-        $this->data = file_get_contents($sFileName);
+        $this->data = file_get_contents($pFilename);
 
         // Total number of sectors used for the SAT
         $this->numBigBlockDepotBlocks = self::getInt4d($this->data, self::NUM_BIG_BLOCK_DEPOT_BLOCKS_POS);

tests/PhpSpreadsheetTests/IOFactoryTest.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests;
+
+use PhpOffice\PhpSpreadsheet\IOFactory;
+
+class IOFactoryTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @dataProvider providerIdentify
+     */
+    public function testIdentify($file, $expected)
+    {
+        $actual = IOFactory::identify($file);
+        $this->assertSame($expected, $actual);
+    }
+
+    public function providerIdentify()
+    {
+        return [
+            ['../samples/templates/26template.xlsx', 'Xlsx'],
+            ['../samples/templates/GnumericTest.gnumeric', 'Gnumeric'],
+            ['../samples/templates/30template.xls', 'Xls'],
+            ['../samples/templates/OOCalcTest.ods', 'Ods'],
+            ['../samples/templates/SylkTest.slk', 'SYLK'],
+            ['../samples/templates/Excel2003XMLTest.xml', 'Excel2003XML'],
+        ];
+    }
+
+    /**
+     * @expectedException \InvalidArgumentException
+     */
+    public function testIdentifyNonExistingFileThrowException()
+    {
+        IOFactory::identify('/non/existing/file');
+    }
+
+    /**
+     * @expectedException \InvalidArgumentException
+     */
+    public function testIdentifyExistingDirectoryThrowExceptions()
+    {
+        IOFactory::identify('.');
+    }
+}