From 89f455c133e1a7e2d4abfadc43ce29eb7450d781 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Tue, 15 Sep 2026 19:03:30 -0700 Subject: [PATCH] Merge commit from fork * Security Patch * Minor Improvement to `unentity` * More Unentity Tweaks --- .../Reader/Security/XmlScanner.php | 5 + src/PhpSpreadsheet/Reader/Xml.php | 12 +- .../Reader/Xlsx/Sec973cTest.php | 20 +++ .../Reader/Xml/HtmlEntitiesLoadTest.php | 10 +- .../Reader/Xml/Sec25mgTest.php | 132 ++++++++++++++++++ tests/data/Reader/XLSX/sec937c.xlsx | Bin 0 -> 1852 bytes 6 files changed, 175 insertions(+), 4 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Reader/Xlsx/Sec973cTest.php create mode 100644 tests/PhpSpreadsheetTests/Reader/Xml/Sec25mgTest.php create mode 100644 tests/data/Reader/XLSX/sec937c.xlsx diff --git a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php index 7b4e10431..5d238a80b 100644 --- a/src/PhpSpreadsheet/Reader/Security/XmlScanner.php +++ b/src/PhpSpreadsheet/Reader/Security/XmlScanner.php @@ -56,6 +56,11 @@ class XmlScanner throw new Reader\Exception('UTF-7 encoding not permitted'); } if (substr($xml, 0, Reader\Csv::UTF8_BOM_LEN) === Reader\Csv::UTF8_BOM) { + if (preg_match(self::ENCODING_PATTERN, $xml, $matches) === 1) { + if (strtolower($matches[2]) !== 'utf-8') { + throw new Reader\Exception("BOM says UTF-8 but encoding says {$matches[2]}"); + } + } $xml = substr($xml, Reader\Csv::UTF8_BOM_LEN); } diff --git a/src/PhpSpreadsheet/Reader/Xml.php b/src/PhpSpreadsheet/Reader/Xml.php index f5cdc8108..9e5bd9975 100644 --- a/src/PhpSpreadsheet/Reader/Xml.php +++ b/src/PhpSpreadsheet/Reader/Xml.php @@ -2,6 +2,7 @@ namespace PhpOffice\PhpSpreadsheet\Reader; +use Composer\Pcre\Preg; use DateTime; use DateTimeZone; use PhpOffice\PhpSpreadsheet\Cell\AddressHelper; @@ -53,9 +54,14 @@ class Xml extends BaseReader public static function unentity(string $contents): string { - $contents = preg_replace('/&(amp|lt|gt|quot|apos);/', "\u{fffe}\u{feff}\$1;", trim($contents)) ?? $contents; + // fffe is invalid, replace with replacement char + $contents = str_replace("\u{fffe}", "\u{fffd}", $contents); + // use positive lookahead to "protect" valid xml entities + $contents = Preg::replace('/&(?=(?:amp|lt|gt|quot|apos|#[0-9]+|#x[0-9a-fA-F]+);)/', "\u{fffe}", trim($contents)); + // now decode remaining html entities $contents = html_entity_decode($contents, ENT_NOQUOTES | ENT_SUBSTITUTE | ENT_HTML401, 'UTF-8'); - $contents = str_replace("\u{fffe}\u{feff}", '&', $contents); + // Escape remaining ampersands, restore those which were replaced with fffe + $contents = str_replace(['&', "\u{fffe}"], ['&', '&'], $contents); return $contents; } @@ -651,7 +657,7 @@ class Xml extends BaseReader } $rangeCalculated = false; if (isset($xmlX->WorksheetOptions->Panes->Pane->RangeSelection)) { - if (1 === preg_match('/^R(\d+)C(\d+):R(\d+)C(\d+)$/', (string) $xmlX->WorksheetOptions->Panes->Pane->RangeSelection, $selectionMatches)) { + if (Preg::isMatch('/^R(\d+)C(\d+):R(\d+)C(\d+)$/', (string) $xmlX->WorksheetOptions->Panes->Pane->RangeSelection, $selectionMatches)) { $selectedCell = Coordinate::stringFromColumnIndex((int) $selectionMatches[2]) . $selectionMatches[1] . ':' diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/Sec973cTest.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/Sec973cTest.php new file mode 100644 index 000000000..1b29cb968 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/Sec973cTest.php @@ -0,0 +1,20 @@ +expectException(ReaderException::class); + $this->expectExceptionMessage('BOM says UTF-8 but encoding says ISO-2022-JP'); + $reader = new XlsxReader(); + $reader->load('tests/data/Reader/XLSX/sec937c.xlsx'); + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Xml/HtmlEntitiesLoadTest.php b/tests/PhpSpreadsheetTests/Reader/Xml/HtmlEntitiesLoadTest.php index 197e00841..60348d86c 100644 --- a/tests/PhpSpreadsheetTests/Reader/Xml/HtmlEntitiesLoadTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Xml/HtmlEntitiesLoadTest.php @@ -9,7 +9,7 @@ use PHPUnit\Framework\TestCase; class HtmlEntitiesLoadTest extends TestCase { - public static function testIssue2157(): void + public function testIssue2157(): void { $infile = 'tests/data/Reader/Xml/issue.2157.small.xml'; $contents = (string) file_get_contents($infile); @@ -26,4 +26,12 @@ class HtmlEntitiesLoadTest extends TestCase self::assertStringContainsString('
', $g2); $spreadsheet->disconnectWorksheets(); } + + public function testUnknownEntities(): void + { + $string = '& &Amp; Τ  㼒 g12; & &*3 <'; + $expected = '& &Amp; Τ  㼒 &#x3g12; & &*3 <'; + $result = XmlReader::unentity($string); + self::assertSame($expected, $result); + } } diff --git a/tests/PhpSpreadsheetTests/Reader/Xml/Sec25mgTest.php b/tests/PhpSpreadsheetTests/Reader/Xml/Sec25mgTest.php new file mode 100644 index 000000000..345b41050 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Reader/Xml/Sec25mgTest.php @@ -0,0 +1,132 @@ +filename !== '') { + unlink($this->filename); + } + } + + public function testNumericEntities(): void + { + $this->filename = File::temporaryFilename(); + $entity_value = str_repeat('A', 100000); + $refs = str_repeat('&big;', 200); + + $xml = << + + &#60;!DOCTYPE Workbook [ + &#60;!ENTITY big "$entity_value"> + ]> + + + + $refs +
+
+
+ EOF; + self::assertNotFalse( + file_put_contents($this->filename, $xml) + ); + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Cannot load invalid XML file'); + $reader = new XmlReader(); + $reader->load($this->filename); + } + + public function testDetectDoctype(): void + { + $this->filename = File::temporaryFilename(); + $entity_value = str_repeat('A', 100000); + $refs = str_repeat('&big;', 200); + + $xml = << + + + ]> + + + + $refs +
+
+
+ EOF; + self::assertNotFalse( + file_put_contents($this->filename, $xml) + ); + $this->expectException(ReaderException::class); + $this->expectExceptionMessage('Detected use of ENTITY'); + $reader = new XmlReader(); + $reader->load($this->filename); + } + + public function testNoDoctype(): void + { + $this->filename = File::temporaryFilename(); + $refs = str_repeat('&πϏ0', 200); + $refsOut = str_repeat('&πϏ0', 200); + + $xml = << + + + + + $refs +
+
+
+ EOF; + self::assertNotFalse( + file_put_contents($this->filename, $xml) + ); + $reader = new XmlReader(); + $spreadsheet = $reader->load($this->filename); + $sheet = $spreadsheet->getActiveSheet(); + self::assertSame($refsOut, $sheet->getCell('A1')->getValue()); + $spreadsheet->disconnectWorksheets(); + } + + public function testIsolated(): void + { + $scanner = new XmlScanner('setAdditionalCallback([XmlReader::class, 'unentity']); + $input = '&#60;!DOCTYPE test'; + $pass1 = $scanner->scan($input); + self::assertStringNotContainsString('scan($pass1); + self::assertStringNotContainsString('>C3E!(=7~24Nr_?VO)ilA2c%A5vM6S{$oak(;x2 z(pkU51_CYds-L#5np^asVbYg?9ZW)d4m`>c(*8HOJo?b>$4gihLtZRZKRxf}&tub8 zzkQNZc)&&NM^sLV<~HB2zXT_Ct*Kab^nlF!DN42rm?tSNI`1VuiTT{^E2}xGTI8#)7x48 z0Vu)b-MrXp8ZdZR85tP3fpmOPYEH4f9*FEcZ_9VcfQR+LY8N|)yG8~Jmc>TwQqmI$ z`tF|l=mcj#p1)g{gv8%JUn1AtKD+jnqfq)csa1HyL>jW^dPy^BMEPk+zvTM$_3i--k81WwW-JT@CEDEJN&<4!?Ss*@$KEp z^XumP$V$0+!o&0MwnvjwcN*%ux-rg{D(5%LM|Hk*D0Fulf7C z)P;6SSY@n|s#+U2aawdww?^6nzMOL_r%rP3zE>bB-1g|q#62gMF5UgaB|~jn$IbPh zLawL3?B(7d9QCVR@cGT&iq6M}lTTQ;6qxSZbL^Tb+uCj1e^x6=^vlNV-^IA`Y}?8u z432BHo7z?~uAFLf?a4%+dInU_{7hY1uny>n$-uaff_MfJ9mrk+N6p#^)_g#JwXGL= z${-fZ7&!c#i=22(+H;k7wG8}xju{5SWlRAqMDzSxW-bzXkMKFOk!BES7S zZou8O6sQ4F_x<-23kM+B<3wWAryLOm(Nds$!)VX zSi4EhdDwqJsqW-~M|Zz6qxy1_OTe{Opzk&Tu>{1IApaF-q^6b>>w_snP~7?YodzYp z-&3{ZHWvoxb8- zmx}9+54P)!SL8La@-OG*OE)-iV*VR@<$qE;`B9zz&ZbjQ5*Rrm$AB2(^x}-fqSTb& zlA_GK^kR^+-(IrKlL{1JeK1dVN97(RuV(h(9Ug5P|Abq(Zb*^RE^7QJ;g*!f@m_G_ zc9F=SZ3#gY!XLkjx?5R2-CJM(UsIp&t=gYeC>ZOBDJv^?^QdIG~w$8ks=lof%Bi=xlkp%u}ZM1^ zvGStcEWNhW{qH}S%saZ_{jB-FfBo7Uo|?7FD183B*;m6qE?lHj6e{-rUA)ZxHC4b= zz{q6JjJtq_IeA+nh=rxNMmGSx=t3Bv4vZtDf(%_FdZB^P_zb8Ktq4KajGjpmnl}S$ z8lYwzSr*+K^lX4IXC^b;5g3^Q-4yf`i7+LE1#AjN`V8=9Wdo^V1HyKocjvQ$cmPS@ B&SwAs literal 0 HcmV?d00001