WIP Xlsx Reader Vml File with Unclosed br Tags

Fix PHPOffice#3125 (rejected previous PR PHPOffice#1181 as too risky, issue was also reported as PHPOffice#170). Vml file should be valid Xml, but Excel (possibly just Excel 2013) can generate unclosed `<br>` tags, preventing Xlsx reader from reading file correctly. I believe a very narrowly targeted fix, changing `<br>` to `<br/>`, and only when reading vml files, probably mitigates the risk. I am going to leave this PR as a draft for a while to see if others disagree.

Another reason to leave it as a draft is the absence of a decent test file. I am able to copy an existing test spreadsheet which contains a vml file, and add some text and the problematic tag, and I can confirm that it fails to load correctly without the fix but loads correctly with the fix. However, it would be better if I had a real file. I cannot figure out how to generate a file like this "naturally"; when I add a textbox to a spreadsheet, it is stored as a regular xml file, not as vml, in both Excel 365 and 2007. Also, the text that I added in the vml file doesn't show anywhere when I open the file in Excel, so I don't know whether my file is a normal case for this condition. And, incidentally, when I load the file in PhpSpreadsheet and save it, the textbox data is not in the resulting output file; this is clearly not a problem in the particular case I constructed, but I don't know if that's true in general. If not, that would be a different problem than the one I'm fixing, but I would prefer to resolve them both at the same time.

Author: oleibman

src/PhpSpreadsheet/Reader/Xlsx.php

@@ -121,9 +121,12 @@ public static function falseToArray($value): array
         return is_array($value) ? $value : [];
     }
 
-    private function loadZip(string $filename, string $ns = ''): SimpleXMLElement
+    private function loadZip(string $filename, string $ns = '', bool $replaceUnclosedBr = false): SimpleXMLElement
     {
         $contents = $this->getFromZipArchive($this->zip, $filename);
+        if ($replaceUnclosedBr) {
+            $contents = str_replace('<br>', '<br/>', $contents);
+        }
         $rels = simplexml_load_string(
             $this->securityScanner->scan($contents),
             'SimpleXMLElement',
@@ -1030,7 +1033,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
 
                                     try {
                                         // no namespace okay - processed with Xpath
-                                        $vmlCommentsFile = $this->loadZip($relPath, '');
+                                        $vmlCommentsFile = $this->loadZip($relPath, '', true);
                                         $vmlCommentsFile->registerXPathNamespace('v', Namespaces::URN_VML);
                                     } catch (Throwable $ex) {
                                         //Ignore unparsable vmlDrawings. Later they will be moved from $unparsedVmlDrawings to $unparsedLoadedData

tests/PhpSpreadsheetTests/Reader/Xlsx/ColorTabTest.php

@@ -12,6 +12,30 @@ public function testIssue2316(): void
         $filename = 'tests/data/Reader/XLSX/colortabs.xlsx';
         $reader = new Xlsx();
         $spreadsheet = $reader->load($filename);
+        $unparsed = $spreadsheet->getUnparsedLoadedData();
+        self::assertArrayHasKey('sheets', $unparsed);
+        self::assertArrayHasKey('Worksheet', $unparsed['sheets']);
+        self::assertArrayNotHasKey('vmlDrawings', $unparsed['sheets']['Worksheet']);
+
+        // theme color
+        self::assertSame('FF548135', $spreadsheet->getSheet(0)->getTabColor()->getArgb());
+        // rgb color
+        self::assertSame('FFFFC000', $spreadsheet->getSheet(1)->getTabColor()->getArgb());
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    public function testIssue3125(): void
+    {
+        // Very artificial - no real sample file to go with issue.
+        $filename = 'tests/data/Reader/XLSX/colortabs.badbr.xlsx';
+        $reader = new Xlsx();
+        $spreadsheet = $reader->load($filename);
+        $unparsed = $spreadsheet->getUnparsedLoadedData();
+        self::assertArrayHasKey('sheets', $unparsed);
+        self::assertArrayHasKey('Worksheet', $unparsed['sheets']);
+        // Before fix, vml in sample file was unparseable as xml (unclosed br tag),
+        // so condition below would fail.
+        self::assertArrayNotHasKey('vmlDrawings', $unparsed['sheets']['Worksheet']);
 
         // theme color
         self::assertSame('FF548135', $spreadsheet->getSheet(0)->getTabColor()->getArgb());