From abad49d426757245f69b9ffbd3ae00367eddc742 Mon Sep 17 00:00:00 2001
From: MarkBaker <mark@lange.demon.co.uk>
Date: Fri, 23 Nov 2018 23:05:17 +0100
Subject: [PATCH] Use factory for XMLcanner

---
 src/PhpSpreadsheet/Reader/Gnumeric.php        |  2 +-
 src/PhpSpreadsheet/Reader/Html.php            |  2 +-
 src/PhpSpreadsheet/Reader/Ods.php             |  2 +-
 .../Reader/Security/XmlScanner.php            | 35 +++++++++++++++----
 src/PhpSpreadsheet/Reader/Xlsx.php            |  2 +-
 src/PhpSpreadsheet/Reader/Xml.php             |  2 +-
 .../Reader/Security/XmlScannerTest.php        |  4 +--
 7 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/src/PhpSpreadsheet/Reader/Gnumeric.php b/src/PhpSpreadsheet/Reader/Gnumeric.php
index a726599e..8837e7da 100644
--- a/src/PhpSpreadsheet/Reader/Gnumeric.php
+++ b/src/PhpSpreadsheet/Reader/Gnumeric.php
@@ -43,7 +43,7 @@ class Gnumeric extends BaseReader
     {
         $this->readFilter = new DefaultReadFilter();
         $this->referenceHelper = ReferenceHelper::getInstance();
-        $this->securityScanner = new XmlScanner();
+        $this->securityScanner = XmlScanner::getInstance($this);
     }
 
     /**
diff --git a/src/PhpSpreadsheet/Reader/Html.php b/src/PhpSpreadsheet/Reader/Html.php
index fc09f1be..e6543592 100644
--- a/src/PhpSpreadsheet/Reader/Html.php
+++ b/src/PhpSpreadsheet/Reader/Html.php
@@ -111,7 +111,7 @@ class Html extends BaseReader
     public function __construct()
     {
         $this->readFilter = new DefaultReadFilter();
-        $this->securityScanner = new XmlScanner('<!ENTITY');
+        $this->securityScanner = XmlScanner::getInstance($this);
     }
 
     /**
diff --git a/src/PhpSpreadsheet/Reader/Ods.php b/src/PhpSpreadsheet/Reader/Ods.php
index 7b25901e..80fab980 100644
--- a/src/PhpSpreadsheet/Reader/Ods.php
+++ b/src/PhpSpreadsheet/Reader/Ods.php
@@ -31,7 +31,7 @@ class Ods extends BaseReader
     public function __construct()
     {
         $this->readFilter = new DefaultReadFilter();
-        $this->securityScanner = new XmlScanner();
+        $this->securityScanner = XmlScanner::getInstance($this);
     }
 
     /**
diff --git a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php
index eefd7c7d..36710209 100644
--- a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php
+++ b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php
@@ -2,7 +2,7 @@
 
 namespace PhpOffice\PhpSpreadsheet\Reader\Security;
 
-use PhpOffice\PhpSpreadsheet\Reader\Exception;
+use PhpOffice\PhpSpreadsheet\Reader;
 
 class XmlScanner
 {
@@ -13,15 +13,38 @@ class XmlScanner
      */
     private $libxmlDisableEntityLoader = false;
 
+    private $previousLibxmlDisableEntityLoaderValue;
     private $pattern;
 
-    public function __construct($pattern = '<!DOCTYPE')
+    private function __construct($pattern = '<!DOCTYPE')
     {
         $this->pattern = $pattern;
         $this->libxmlDisableEntityLoader = $this->identifyLibxmlDisableEntityLoaderAvailability();
 
         if ($this->libxmlDisableEntityLoader) {
-            libxml_disable_entity_loader(true);
+            $this->previousLibxmlDisableEntityLoaderValue = libxml_disable_entity_loader(true);
+        }
+    }
+
+    public function __destruct()
+    {
+        if ($this->libxmlDisableEntityLoader) {
+            libxml_disable_entity_loader($this->previousLibxmlDisableEntityLoaderValue);
+        }
+    }
+
+    public static function getInstance(Reader\IReader $reader)
+    {
+        switch(true) {
+            case $reader instanceof Reader\Html:
+                return new self('<!ENTITY');
+            case $reader instanceof Reader\Xlsx:
+            case $reader instanceof Reader\Xml:
+            case $reader instanceof Reader\Ods:
+            case $reader instanceof Reader\Gnumeric:
+                return new self('<!DOCTYPE');
+            default:
+                return new self('<!DOCTYPE');
         }
     }
 
@@ -48,7 +71,7 @@ class XmlScanner
      *
      * @param mixed $xml
      *
-     * @throws Exception
+     * @throws Reader\Exception
      *
      * @return string
      */
@@ -65,7 +88,7 @@ class XmlScanner
         // Don't rely purely on libxml_disable_entity_loader()
         $pattern = '/\\0?' . implode('\\0?', str_split($this->pattern)) . '\\0?/';
         if (preg_match($pattern, $xml)) {
-            throw new Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
+            throw new Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
         }
 
         return $xml;
@@ -76,7 +99,7 @@ class XmlScanner
      *
      * @param string $filestream
      *
-     * @throws Exception
+     * @throws Reader\Exception
      *
      * @return string
      */
diff --git a/src/PhpSpreadsheet/Reader/Xlsx.php b/src/PhpSpreadsheet/Reader/Xlsx.php
index b1e076a8..806a6d4f 100644
--- a/src/PhpSpreadsheet/Reader/Xlsx.php
+++ b/src/PhpSpreadsheet/Reader/Xlsx.php
@@ -59,7 +59,7 @@ class Xlsx extends BaseReader
     {
         $this->readFilter = new DefaultReadFilter();
         $this->referenceHelper = ReferenceHelper::getInstance();
-        $this->securityScanner = new XmlScanner();
+        $this->securityScanner = XmlScanner::getInstance($this);
     }
 
     /**
diff --git a/src/PhpSpreadsheet/Reader/Xml.php b/src/PhpSpreadsheet/Reader/Xml.php
index 9e982f2a..3781c427 100644
--- a/src/PhpSpreadsheet/Reader/Xml.php
+++ b/src/PhpSpreadsheet/Reader/Xml.php
@@ -47,7 +47,7 @@ class Xml extends BaseReader
     public function __construct()
     {
         $this->readFilter = new DefaultReadFilter();
-        $this->securityScanner = new XmlScanner();
+        $this->securityScanner = XmlScanner::getInstance($this);
     }
 
     /**
diff --git a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php b/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php
index b914de08..8eaa56f8 100644
--- a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php
+++ b/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php
@@ -15,7 +15,7 @@ class XmlScannerTest extends TestCase
      */
     public function testValidXML($filename, $expectedResult)
     {
-        $reader = new XmlScanner();
+        $reader = XmlScanner::getInstance(new \PhpOffice\PhpSpreadsheet\Reader\Xml());
         $result = $reader->scanFile($filename);
         self::assertEquals($expectedResult, $result);
     }
@@ -39,7 +39,7 @@ class XmlScannerTest extends TestCase
     {
         $this->expectException(\PhpOffice\PhpSpreadsheet\Reader\Exception::class);
 
-        $reader = new XmlScanner();
+        $reader = XmlScanner::getInstance(new \PhpOffice\PhpSpreadsheet\Reader\Xml());
         $expectedResult = 'FAILURE: Should throw an Exception rather than return a value';
         $result = $reader->scanFile($filename);
         self::assertEquals($expectedResult, $result);