Allow XmlScanner to correctly restore libxml entity_loader setting (#…

…1050) XmlScanner was not restoring libxml_disable_entity_loader since destruct was not being called until script shutdown. This is because the shutdown handler required an XmlScanner instance. Also fix an unrelated bug where the UTF-8 encoding test was case sensitive.
PHPOffice · Jul 3, 2019 · 6ab969e · 6ab969e
1 parent 352c700
commit 6ab969e
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 23 deletions.
diff --git a/src/PhpSpreadsheet/Reader/BaseReader.php b/src/PhpSpreadsheet/Reader/BaseReader.php
@@ -58,21 +58,6 @@ abstract class BaseReader implements IReader
     public function __construct()
     {
         $this->readFilter = new DefaultReadFilter();
-
-        // A fatal error will bypass the destructor, so we register a shutdown here
-        register_shutdown_function([$this, '__destruct']);
-    }
-
-    private function shutdown()
-    {
-        if ($this->securityScanner !== null) {
-            $this->securityScanner = null;
-        }
-    }
-
-    public function __destruct()
-    {
-        $this->shutdown();
     }
 
     public function getReadDataOnly()
@@ -146,7 +131,7 @@ public function setReadFilter(IReadFilter $pValue)
         return $this;
     }
 
-    public function getSecuritySCanner()
+    public function getSecurityScanner()
     {
         if (property_exists($this, 'securityScanner')) {
             return $this->securityScanner;

diff --git a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php
@@ -25,7 +25,7 @@ public function __construct($pattern = '<!DOCTYPE')
         $this->disableEntityLoaderCheck();
 
         // A fatal error will bypass the destructor, so we register a shutdown here
-        register_shutdown_function([$this, '__destruct']);
+        register_shutdown_function([__CLASS__, 'shutdown']);
     }
 
     public static function getInstance(Reader\IReader $reader)
@@ -72,16 +72,17 @@ private function disableEntityLoaderCheck()
         }
     }
 
-    private function shutdown()
+    public static function shutdown()
     {
         if (self::$libxmlDisableEntityLoaderValue !== null) {
             libxml_disable_entity_loader(self::$libxmlDisableEntityLoaderValue);
+            self::$libxmlDisableEntityLoaderValue = null;
         }
     }
 
     public function __destruct()
     {
-        $this->shutdown();
+        self::shutdown();
     }
 
     public function setAdditionalCallback(callable $callback)
@@ -93,7 +94,7 @@ private function toUtf8($xml)
     {
         $pattern = '/encoding="(.*?)"/';
         $result = preg_match($pattern, $xml, $matches);
-        $charset = $result ? $matches[1] : 'UTF-8';
+        $charset = strtoupper($result ? $matches[1] : 'UTF-8');
 
         if ($charset !== 'UTF-8') {
             $xml = mb_convert_encoding($xml, 'UTF-8', $charset);

diff --git a/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php b/tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php
@@ -9,6 +9,11 @@
 
 class XmlScannerTest extends TestCase
 {
+    protected function setUp()
+    {
+        libxml_disable_entity_loader(false);
+    }
+
     /**
      * @dataProvider providerValidXML
      *
@@ -74,7 +79,7 @@ public function providerInvalidXML()
     public function testGetSecurityScannerForXmlBasedReader()
     {
         $fileReader = new Xlsx();
-        $scanner = $fileReader->getSecuritySCanner();
+        $scanner = $fileReader->getSecurityScanner();
 
         //    Must return an object...
         $this->assertInternalType('object', $scanner);
@@ -85,7 +90,7 @@ public function testGetSecurityScannerForXmlBasedReader()
     public function testGetSecurityScannerForNonXmlBasedReader()
     {
         $fileReader = new Xls();
-        $scanner = $fileReader->getSecuritySCanner();
+        $scanner = $fileReader->getSecurityScanner();
         //    Must return a null...
         $this->assertNull($scanner);
     }
@@ -99,7 +104,7 @@ public function testGetSecurityScannerForNonXmlBasedReader()
     public function testSecurityScanWithCallback($filename, $expectedResult)
     {
         $fileReader = new Xlsx();
-        $scanner = $fileReader->getSecuritySCanner();
+        $scanner = $fileReader->getSecurityScanner();
         $scanner->setAdditionalCallback('strrev');
         $xml = $scanner->scanFile($filename);
 
@@ -115,4 +120,21 @@ public function providerValidXMLForCallback()
 
         return $tests;
     }
+
+    public function testLibxmlDisableEntityLoaderIsRestoredWithoutShutdown()
+    {
+        $reader = new Xlsx();
+        unset($reader);
+
+        $reader = new \XMLReader();
+        $opened = $reader->open(__DIR__ . '/../../../data/Reader/Xml/SecurityScannerWithCallbackExample.xml');
+        $this->assertTrue($opened);
+    }
+
+    public function testEncodingAllowsMixedCase()
+    {
+        $scanner = new XmlScanner();
+        $output = $scanner->scan($input = '<?xml version="1.0" encoding="utf-8"?><foo>bar</foo>');
+        $this->assertSame($input, $output);
+    }
 }