Merge branch 'master' into fix_s_tag

Author: oleibman

.github/workflows/main.yml

@@ -210,7 +210,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
         with:
           fetch-depth: 0
 

CHANGELOG.md

@@ -5,13 +5,39 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com)
 and this project adheres to [Semantic Versioning](https://semver.org).
 
-## TBD - 3.4.0
+## TBD - 3.5.0
+
+### Added
+
+- Nothing yet.
+
+### Changed
+
+- Nothing yet.
+
+### Moved
+
+- Nothing yet.
+
+### Deprecated
+
+- Nothing yet.
+
+### Fixed
+
+- Nothing yet.
+
+## 2024-11-10 - 3.4.0
+
+### Security Fix
+
+- Several security patches.
 
 ### Added
 
 - Add Dynamic valueBinder Property to Spreadsheet and Readers. [Issue #1395](https://github.com/PHPOffice/PhpSpreadsheet/issues/1395) [PR #4185](https://github.com/PHPOffice/PhpSpreadsheet/pull/4185)
 - Allow Omitting Chart Border. [Issue #562](https://github.com/PHPOffice/PhpSpreadsheet/issues/562) [PR #4188](https://github.com/PHPOffice/PhpSpreadsheet/pull/4188)
-- Method to Test Whether Csv Will Be Affected by Php0. [PR #4189](https://github.com/PHPOffice/PhpSpreadsheet/pull/4189)
+- Method to Test Whether Csv Will Be Affected by Php9. [PR #4189](https://github.com/PHPOffice/PhpSpreadsheet/pull/4189)
 
 ### Changed
 

CONTRIBUTING.md

@@ -4,6 +4,7 @@ If you would like to contribute, here are some notes and guidelines:
 
  - All new development should be on feature/fix branches, which are then merged to the `master` branch once stable and approved; so the `master` branch is always the most up-to-date, working code
  - If you are going to submit a pull request, please fork from `master`, and submit your pull request back as a fix/feature branch referencing the GitHub issue number
+ - Install (development) dependencies by running `composer install` inside your PhpSpreadsheet clone.
  - The code must work with all PHP versions that we support.
    - You can call `composer versions` to test version compatibility. 
  - Code style should be maintained.
@@ -39,7 +40,10 @@ This makes it easier to see exactly what is being tested when reviewing the PR.
     2. Tag subject must be the version number, eg: `1.2.3`
     3. Tag body must be a copy-paste of the changelog entries.
 3. Push the tag with `git push --tags`, GitHub Actions will create a GitHub release automatically, and the release details will automatically be sent to packagist.
-4. Github seems to remove markdown headings in the Release Notes, so you should edit to restore these.
+4. By default, Github remove markdown headings in the Release Notes. You can either edit to restore these, or, probably preferably, change the default comment character on your system - `git config core.commentChar ';'`.
 
-> **Note:** Tagged releases are made from the `master` branch. Only in an emergency should a tagged release be made from the `release` branch. (i.e. cherry-picked hot-fixes.)
+> **Note:** Tagged releases are made from the `master` branch. Only in an emergency should a tagged release be made from the `release` branch. (i.e. cherry-picked hot-fixes.) However, there are 3 branches which have been updated to apply security patches, and those may be tagged if future security updates are needed.
+- release1291
+- release210
+- release222
 

composer.json

@@ -45,12 +45,12 @@
     ],
     "scripts": {
         "check": [
-            "./bin/check-phpdoc-types",
+            "php ./bin/check-phpdoc-types",
             "phpcs samples/ src/ tests/ --report=checkstyle",
-            "phpcs samples/ src/ tests/ --standard=PHPCompatibility --runtime-set testVersion 8.0- -n",
+            "phpcs samples/ src/ tests/ --standard=PHPCompatibility --runtime-set testVersion 8.0- --exclude=PHPCompatibility.Variables.ForbiddenThisUseContexts -n",
             "php-cs-fixer fix --ansi --dry-run --diff",
-            "phpunit --color=always",
-            "phpstan analyse --ansi --memory-limit=2048M"
+            "phpstan analyse --ansi --memory-limit=2048M",
+            "phpunit --color=always"
         ],
         "style": [
             "phpcs samples/ src/ tests/ --report=checkstyle",
@@ -61,7 +61,7 @@
             "php-cs-fixer fix"
         ],
         "versions": [
-            "phpcs samples/ src/ tests/ --standard=PHPCompatibility --runtime-set testVersion 8.0- -n"
+            "phpcs samples/ src/ tests/ --standard=PHPCompatibility --runtime-set testVersion 8.0- --exclude=PHPCompatibility.Variables.ForbiddenThisUseContexts -n"
         ]
     },
     "require": {

src/PhpSpreadsheet/Reader/Security/XmlScanner.php

@@ -6,6 +6,9 @@
 
 class XmlScanner
 {
+    private const ENCODING_PATTERN = '/encoding\\s*=\\s*(["\'])(.+?)\\1/s';
+    private const ENCODING_UTF7 = '/encoding\\s*=\\s*(["\'])UTF-7\\1/si';
+
     private string $pattern;
 
     /** @var ?callable */
@@ -36,29 +39,41 @@ private static function forceString(mixed $arg): string
     private function toUtf8(string $xml): string
     {
         $charset = $this->findCharSet($xml);
+        $foundUtf7 = $charset === 'UTF-7';
         if ($charset !== 'UTF-8') {
+            $testStart = '/^.{0,4}\\s*<?xml/s';
+            $startWithXml1 = preg_match($testStart, $xml);
             $xml = self::forceString(mb_convert_encoding($xml, 'UTF-8', $charset));
-
-            $charset = $this->findCharSet($xml);
-            if ($charset !== 'UTF-8') {
-                throw new Reader\Exception('Suspicious Double-encoded XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
+            if ($startWithXml1 === 1 && preg_match($testStart, $xml) !== 1) {
+                throw new Reader\Exception('Double encoding not permitted');
             }
+            $foundUtf7 = $foundUtf7 || (preg_match(self::ENCODING_UTF7, $xml) === 1);
+            $xml = preg_replace(self::ENCODING_PATTERN, '', $xml) ?? $xml;
+        } else {
+            $foundUtf7 = $foundUtf7 || (preg_match(self::ENCODING_UTF7, $xml) === 1);
+        }
+        if ($foundUtf7) {
+            throw new Reader\Exception('UTF-7 encoding not permitted');
+        }
+        if (substr($xml, 0, Reader\Csv::UTF8_BOM_LEN) === Reader\Csv::UTF8_BOM) {
+            $xml = substr($xml, Reader\Csv::UTF8_BOM_LEN);
         }
 
         return $xml;
     }
 
     private function findCharSet(string $xml): string
     {
-        $patterns = [
-            '/encoding\\s*=\\s*"([^"]*]?)"/',
-            "/encoding\\s*=\\s*'([^']*?)'/",
-        ];
-
-        foreach ($patterns as $pattern) {
-            if (preg_match($pattern, $xml, $matches)) {
-                return strtoupper($matches[1]);
-            }
+        if (substr($xml, 0, 4) === "\x4c\x6f\xa7\x94") {
+            throw new Reader\Exception('EBCDIC encoding not permitted');
+        }
+        $encoding = Reader\Csv::guessEncodingBom('', $xml);
+        if ($encoding !== '') {
+            return $encoding;
+        }
+        $xml = str_replace("\0", '', $xml);
+        if (preg_match(self::ENCODING_PATTERN, $xml, $matches)) {
+            return strtoupper($matches[2]);
         }
 
         return 'UTF-8';
@@ -71,13 +86,15 @@ private function findCharSet(string $xml): string
      */
     public function scan($xml): string
     {
+        // Don't rely purely on libxml_disable_entity_loader()
+        $pattern = '/\\0*' . implode('\\0*', str_split($this->pattern)) . '\\0*/';
+
         $xml = "$xml";
+        if (preg_match($pattern, $xml)) {
+            throw new Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
+        }
 
         $xml = $this->toUtf8($xml);
-
-        // 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 Reader\Exception('Detected use of ENTITY in XML, spreadsheet file load() aborted to prevent XXE/XEE attacks');
         }
@@ -90,7 +107,7 @@ public function scan($xml): string
     }
 
     /**
-     * Scan theXML for use of <!ENTITY to prevent XXE/XEE attacks.
+     * Scan the XML for use of <!ENTITY to prevent XXE/XEE attacks.
      */
     public function scanFile(string $filestream): string
     {

src/PhpSpreadsheet/Reader/Xml.php

@@ -18,7 +18,6 @@
 use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Shared\Date;
 use PhpOffice\PhpSpreadsheet\Shared\File;
-use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Worksheet\SheetView;
 use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;
@@ -91,7 +90,8 @@ public function canRead(string $filename): bool
         ];
 
         // Open file
-        $data = file_get_contents($filename) ?: '';
+        $data = (string) file_get_contents($filename);
+        $data = $this->getSecurityScannerOrThrow()->scan($data);
 
         // Why?
         //$data = str_replace("'", '"', $data); // fix headers with single quote
@@ -106,14 +106,6 @@ public function canRead(string $filename): bool
             }
         }
 
-        //    Retrieve charset encoding
-        if (preg_match('/<?xml.*encoding=[\'"](.*?)[\'"].*?>/m', $data, $matches)) {
-            $charSet = strtoupper($matches[1]);
-            if (preg_match('/^ISO-8859-\d[\dL]?$/i', $charSet) === 1) {
-                $data = StringHelper::convertEncoding($data, 'UTF-8', $charSet);
-                $data = (string) preg_replace('/(<?xml.*encoding=[\'"]).*?([\'"].*?>)/um', '$1' . 'UTF-8' . '$2', $data, 1);
-            }
-        }
         $this->fileContents = $data;
 
         return $valid;

tests/PhpSpreadsheetTests/Reader/Security/XmlScannerTest.php

@@ -30,7 +30,11 @@ public static function providerValidXML(): array
         self::assertNotFalse($glob);
         foreach ($glob as $file) {
             $filename = realpath($file);
-            $expectedResult = file_get_contents($file);
+            $expectedResult = (string) file_get_contents($file);
+            if (preg_match('/UTF-16(LE|BE)?/', $file, $matches) == 1) {
+                $expectedResult = (string) mb_convert_encoding($expectedResult, 'UTF-8', $matches[0]);
+                $expectedResult = preg_replace('/encoding\\s*=\\s*[\'"]UTF-\\d+(LE|BE)?[\'"]/', '', $expectedResult) ?? $expectedResult;
+            }
             $tests[basename($file)] = [$filename, $expectedResult];
         }
 
@@ -132,19 +136,47 @@ public function testEncodingAllowsMixedCase(): void
         self::assertSame($input, $output);
     }
 
-    public function testUtf7Whitespace(): void
+    /**
+     * @dataProvider providerInvalidXlsx
+     */
+    public function testInvalidXlsx(string $filename, string $message): void
     {
         $this->expectException(ReaderException::class);
-        $this->expectExceptionMessage('Double-encoded');
+        $this->expectExceptionMessage($message);
         $reader = new Xlsx();
-        $reader->load('tests/data/Reader/XLSX/utf7white.dontuse');
+        $reader->load("tests/data/Reader/XLSX/$filename");
     }
 
-    public function testUtf8Entity(): void
+    public static function providerInvalidXlsx(): array
+    {
+        return [
+            ['utf7white.dontuse', 'UTF-7 encoding not permitted'],
+            ['utf7quoteorder.dontuse', 'UTF-7 encoding not permitted'],
+            ['utf8and16.dontuse', 'Double encoding not permitted'],
+            ['utf8and16.entity.dontuse', 'Detected use of ENTITY'],
+            ['utf8entity.dontuse', 'Detected use of ENTITY'],
+            ['utf16entity.dontuse', 'Detected use of ENTITY'],
+            ['ebcdic.dontuse', 'EBCDIC encoding not permitted'],
+        ];
+    }
+
+    /**
+     * @dataProvider providerValidUtf16
+     */
+    public function testValidUtf16(string $filename): void
     {
-        $this->expectException(ReaderException::class);
-        $this->expectExceptionMessage('Detected use of ENTITY');
         $reader = new Xlsx();
-        $reader->load('tests/data/Reader/XLSX/utf8entity.dontuse');
+        $spreadsheet = $reader->load("tests/data/Reader/XLSX/$filename");
+        $sheet = $spreadsheet->getActiveSheet();
+        self::assertSame(1, $sheet->getCell('A1')->getValue());
+        $spreadsheet->disconnectWorksheets();
+    }
+
+    public static function providerValidUtf16(): array
+    {
+        return [
+            ['utf16be.xlsx'],
+            ['utf16be.bom.xlsx'],
+        ];
     }
 }