Skip to content

Commit e04ed22

Browse files
oleibmanoleibman
committed
Backport Security Patch
1 parent 392dd08 commit e04ed22

File tree

12 files changed

+213
-41
lines changed

12 files changed

+213
-41
lines changed

‎src/PhpSpreadsheet/Reader/Xlsx.php‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,7 +1294,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
12941294
$hfImages[$shapeId]->setName((string) $imageData['title']);
12951295
}
12961296

1297-
$hfImages[$shapeId]->setPath('zip://' . File::realpath($filename) . '#' . $drawings[(string) $imageData['relid']], false);
1297+
$hfImages[$shapeId]->setPath('zip://' . File::realpath($filename) . '#' . $drawings[(string) $imageData['relid']], false, $zip);
12981298
$hfImages[$shapeId]->setResizeProportional(false);
12991299
$hfImages[$shapeId]->setWidth($style['width']);
13001300
$hfImages[$shapeId]->setHeight($style['height']);
@@ -1406,7 +1406,8 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14061406
$objDrawing->setPath(
14071407
'zip://' . File::realpath($filename) . '#'
14081408
. $images[$embedImageKey],
1409-
false
1409+
false,
1410+
$zip
14101411
);
14111412
} else {
14121413
$linkImageKey = (string) self::getArrayItem(
@@ -1417,6 +1418,9 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14171418
$url = str_replace('xl/drawings/', '', $images[$linkImageKey]);
14181419
$objDrawing->setPath($url);
14191420
}
1421+
if ($objDrawing->getPath() === '') {
1422+
continue;
1423+
}
14201424
}
14211425
$objDrawing->setCoordinates(Coordinate::stringFromColumnIndex(((int) $oneCellAnchor->from->col) + 1) . ($oneCellAnchor->from->row + 1));
14221426

@@ -1495,7 +1499,8 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14951499
$objDrawing->setPath(
14961500
'zip://' . File::realpath($filename) . '#'
14971501
. $images[$embedImageKey],
1498-
false
1502+
false,
1503+
$zip
14991504
);
15001505
} else {
15011506
$linkImageKey = (string) self::getArrayItem(
@@ -1506,6 +1511,9 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
15061511
$url = str_replace('xl/drawings/', '', $images[$linkImageKey]);
15071512
$objDrawing->setPath($url);
15081513
}
1514+
if ($objDrawing->getPath() === '') {
1515+
continue;
1516+
}
15091517
}
15101518
$objDrawing->setCoordinates(Coordinate::stringFromColumnIndex(((int) $twoCellAnchor->from->col) + 1) . ($twoCellAnchor->from->row + 1));
15111519

‎src/PhpSpreadsheet/Worksheet/BaseDrawing.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = f
192192
{
193193
if ($this->worksheet === null) {
194194
// Add drawing to \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
195-
if ($worksheet !== null) {
195+
if ($worksheet !== null && !($this instanceof Drawing && $this->getPath() === '')) {
196196
$this->worksheet = $worksheet;
197197
$this->worksheet->getCell($this->coordinates);
198198
$this->worksheet->getDrawingCollection()->append($this);

‎src/PhpSpreadsheet/Worksheet/Drawing.php‎

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -94,40 +94,72 @@ public function getPath(): string
9494
*/
9595
public function setPath(string $path, bool $verifyFile = true, ?ZipArchive $zip = null): static
9696
{
97-
if ($verifyFile && preg_match('~^data:image/[a-z]+;base64,~', $path) !== 1) {
98-
// Check if a URL has been passed. https://stackoverflow.com/a/2058596/1252979
99-
if (filter_var($path, FILTER_VALIDATE_URL)) {
100-
$this->path = $path;
101-
// Implicit that it is a URL, rather store info than running check above on value in other places.
102-
$this->isUrl = true;
103-
$imageContents = file_get_contents($path);
97+
$this->isUrl = false;
98+
if (preg_match('~^data:image/[a-z]+;base64,~', $path) === 1) {
99+
$this->path = $path;
100+
101+
return $this;
102+
}
103+
104+
$this->path = '';
105+
// Check if a URL has been passed. https://stackoverflow.com/a/2058596/1252979
106+
if (filter_var($path, FILTER_VALIDATE_URL)) {
107+
if (!preg_match('/^(http|https|file|ftp|s3):/', $path)) {
108+
throw new PhpSpreadsheetException('Invalid protocol for linked drawing');
109+
}
110+
// Implicit that it is a URL, rather store info than running check above on value in other places.
111+
$this->isUrl = true;
112+
$imageContents = @file_get_contents($path);
113+
if ($imageContents !== false) {
104114
$filePath = tempnam(sys_get_temp_dir(), 'Drawing');
105115
if ($filePath) {
106-
file_put_contents($filePath, $imageContents);
107-
if (file_exists($filePath)) {
108-
$this->setSizesAndType($filePath);
116+
$put = @file_put_contents($filePath, $imageContents);
117+
if ($put !== false) {
118+
if ($this->isImage($filePath)) {
119+
$this->path = $path;
120+
$this->setSizesAndType($filePath);
121+
}
109122
unlink($filePath);
110123
}
111124
}
112-
} elseif (file_exists($path)) {
113-
$this->path = $path;
114-
$this->setSizesAndType($path);
115-
} elseif ($zip instanceof ZipArchive) {
116-
$zipPath = explode('#', $path)[1];
117-
if ($zip->locateName($zipPath) !== false) {
125+
}
126+
} elseif ($zip instanceof ZipArchive) {
127+
$zipPath = explode('#', $path)[1];
128+
$locate = @$zip->locateName($zipPath);
129+
if ($locate !== false) {
130+
if ($this->isImage($path)) {
118131
$this->path = $path;
119132
$this->setSizesAndType($path);
120133
}
121-
} else {
122-
throw new PhpSpreadsheetException("File $path not found!");
123134
}
124135
} else {
125-
$this->path = $path;
136+
$exists = @file_exists($path);
137+
if ($exists !== false && $this->isImage($path)) {
138+
$this->path = $path;
139+
$this->setSizesAndType($path);
140+
}
141+
}
142+
if ($this->path === '' && $verifyFile) {
143+
throw new PhpSpreadsheetException("File $path not found!");
126144
}
127145

128146
return $this;
129147
}
130148

149+
private function isImage(string $path): bool
150+
{
151+
$mime = (string) @mime_content_type($path);
152+
$retVal = false;
153+
if (str_starts_with($mime, 'image/')) {
154+
$retVal = true;
155+
} elseif ($mime === 'application/octet-stream') {
156+
$extension = pathinfo($path, PATHINFO_EXTENSION);
157+
$retVal = in_array($extension, ['bin', 'emf'], true);
158+
}
159+
160+
return $retVal;
161+
}
162+
131163
/**
132164
* Get isURL.
133165
*/

‎src/PhpSpreadsheet/Writer/Html.php‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,9 @@ private function extendRowsAndColumns(Worksheet $worksheet, int &$colMax, int &$
572572
}
573573
}
574574
foreach ($worksheet->getDrawingCollection() as $drawing) {
575+
if ($drawing instanceof Drawing && $drawing->getPath() === '') {
576+
continue;
577+
}
575578
$imageTL = Coordinate::indexesFromString($drawing->getCoordinates());
576579
$this->sheetDrawings[$drawing->getCoordinates()] = $drawing;
577580
if ($imageTL[1] > $rowMax) {
@@ -614,7 +617,7 @@ private function writeImageInCell(string $coordinates): string
614617
if ($drawing !== null) {
615618
$filedesc = $drawing->getDescription();
616619
$filedesc = $filedesc ? htmlspecialchars($filedesc, ENT_QUOTES) : 'Embedded image';
617-
if ($drawing instanceof Drawing) {
620+
if ($drawing instanceof Drawing && $drawing->getPath() !== '') {
618621
$filename = $drawing->getPath();
619622

620623
// Strip off eventual '.'
@@ -633,12 +636,15 @@ private function writeImageInCell(string $coordinates): string
633636
$imageData = self::winFileToUrl($filename, $this instanceof Pdf\Mpdf);
634637

635638
if ($this->embedImages || str_starts_with($imageData, 'zip://')) {
639+
$imageData = 'data:,';
636640
$picture = @file_get_contents($filename);
637641
if ($picture !== false) {
638-
$imageDetails = getimagesize($filename) ?: ['mime' => ''];
639-
// base64 encode the binary data
640-
$base64 = base64_encode($picture);
641-
$imageData = 'data:' . $imageDetails['mime'] . ';base64,' . $base64;
642+
$mimeContentType = (string) @mime_content_type($filename);
643+
if (str_starts_with($mimeContentType, 'image/')) {
644+
// base64 encode the binary data
645+
$base64 = base64_encode($picture);
646+
$imageData = 'data:' . $mimeContentType . ';base64,' . $base64;
647+
}
642648
}
643649
}
644650

‎src/PhpSpreadsheet/Writer/Xls.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ private function processDrawing(BstoreContainer &$bstoreContainer, Drawing $draw
467467

468468
private function processBaseDrawing(BstoreContainer &$bstoreContainer, BaseDrawing $drawing): void
469469
{
470-
if ($drawing instanceof Drawing) {
470+
if ($drawing instanceof Drawing && $drawing->getPath() !== '') {
471471
$this->processDrawing($bstoreContainer, $drawing);
472472
} elseif ($drawing instanceof MemoryDrawing) {
473473
$this->processMemoryDrawing($bstoreContainer, $drawing, $drawing->getRenderingFunction());

‎src/PhpSpreadsheet/Writer/Xlsx.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,9 @@ public function save($filename, int $flags = 0): void
445445

446446
// Media
447447
foreach ($this->spreadSheet->getSheet($i)->getHeaderFooter()->getImages() as $image) {
448-
$zipContent['xl/media/' . $image->getIndexedFilename()] = file_get_contents($image->getPath());
448+
if ($image->getPath() !== '') {
449+
$zipContent['xl/media/' . $image->getIndexedFilename()] = file_get_contents($image->getPath());
450+
}
449451
}
450452
}
451453

@@ -461,6 +463,9 @@ public function save($filename, int $flags = 0): void
461463
if ($this->getDrawingHashTable()->getByIndex($i) instanceof WorksheetDrawing) {
462464
$imageContents = null;
463465
$imagePath = $this->getDrawingHashTable()->getByIndex($i)->getPath();
466+
if ($imagePath === '') {
467+
continue;
468+
}
464469
if (str_contains($imagePath, 'zip://')) {
465470
$imagePath = substr($imagePath, 6);
466471
$imagePathSplitted = explode('#', $imagePath);
@@ -654,6 +659,9 @@ private function processDrawing(WorksheetDrawing $drawing): string|null|false
654659
{
655660
$data = null;
656661
$filename = $drawing->getPath();
662+
if ($filename === '') {
663+
return null;
664+
}
657665
$imageData = getimagesize($filename);
658666

659667
if (!empty($imageData)) {

‎src/PhpSpreadsheet/Writer/Xlsx/ContentTypes.php‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use PhpOffice\PhpSpreadsheet\Shared\File;
77
use PhpOffice\PhpSpreadsheet\Shared\XMLWriter;
88
use PhpOffice\PhpSpreadsheet\Spreadsheet;
9+
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing as WorksheetDrawing;
910
use PhpOffice\PhpSpreadsheet\Worksheet\MemoryDrawing;
1011
use PhpOffice\PhpSpreadsheet\Writer\Exception as WriterException;
1112

@@ -132,18 +133,23 @@ public function writeContentTypes(Spreadsheet $spreadsheet, bool $includeCharts
132133
$extension = '';
133134
$mimeType = '';
134135

135-
if ($this->getParentWriter()->getDrawingHashTable()->getByIndex($i) instanceof \PhpOffice\PhpSpreadsheet\Worksheet\Drawing) {
136-
$extension = strtolower($this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getExtension());
137-
$mimeType = $this->getImageMimeType($this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getPath());
138-
} elseif ($this->getParentWriter()->getDrawingHashTable()->getByIndex($i) instanceof MemoryDrawing) {
139-
$extension = strtolower($this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getMimeType());
136+
$drawing = $this->getParentWriter()->getDrawingHashTable()->getByIndex($i);
137+
if ($drawing instanceof WorksheetDrawing) {
138+
$extension = strtolower($drawing->getExtension());
139+
if ($drawing->getIsUrl()) {
140+
$mimeType = image_type_to_mime_type($drawing->getType());
141+
} else {
142+
$mimeType = $this->getImageMimeType($drawing->getPath());
143+
}
144+
} elseif ($drawing instanceof MemoryDrawing) {
145+
$extension = strtolower($drawing->getMimeType());
140146
$extension = explode('/', $extension);
141147
$extension = $extension[1];
142148

143-
$mimeType = $this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getMimeType();
149+
$mimeType = $drawing->getMimeType();
144150
}
145151

146-
if (!isset($aMediaContentTypes[$extension])) {
152+
if ($mimeType !== '' && !isset($aMediaContentTypes[$extension])) {
147153
$aMediaContentTypes[$extension] = $mimeType;
148154

149155
$this->writeDefaultContentType($objWriter, $extension, $mimeType);
@@ -162,7 +168,7 @@ public function writeContentTypes(Spreadsheet $spreadsheet, bool $includeCharts
162168
for ($i = 0; $i < $sheetCount; ++$i) {
163169
if (count($spreadsheet->getSheet($i)->getHeaderFooter()->getImages()) > 0) {
164170
foreach ($spreadsheet->getSheet($i)->getHeaderFooter()->getImages() as $image) {
165-
if (!isset($aMediaContentTypes[strtolower($image->getExtension())])) {
171+
if ($image->getPath() !== '' && !isset($aMediaContentTypes[strtolower($image->getExtension())])) {
166172
$aMediaContentTypes[strtolower($image->getExtension())] = $this->getImageMimeType($image->getPath());
167173

168174
$this->writeDefaultContentType($objWriter, strtolower($image->getExtension()), $aMediaContentTypes[strtolower($image->getExtension())]);

‎tests/PhpSpreadsheetTests/Reader/Xlsx/URLImageTest.php‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,16 @@
44

55
namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
66

7+
use PhpOffice\PhpSpreadsheet\Exception as SpreadsheetException;
78
use PhpOffice\PhpSpreadsheet\IOFactory;
89
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
910
use PhpOffice\PhpSpreadsheetTests\Reader\Utility\File;
1011
use PHPUnit\Framework\TestCase;
1112

1213
class URLImageTest extends TestCase
1314
{
14-
public function testURLImageSource(): void
15+
// https://github.com/readthedocs/readthedocs.org/issues/11615
16+
public function xtestURLImageSource(): void
1517
{
1618
if (getenv('SKIP_URL_IMAGE_TEST') === '1') {
1719
self::markTestSkipped('Skipped due to setting of environment variable');
@@ -41,4 +43,28 @@ public function testURLImageSource(): void
4143
self::assertSame('png', $extension);
4244
}
4345
}
46+
47+
public function xtestURLImageSourceNotFound(): void
48+
{
49+
if (getenv('SKIP_URL_IMAGE_TEST') === '1') {
50+
self::markTestSkipped('Skipped due to setting of environment variable');
51+
}
52+
$filename = realpath(__DIR__ . '/../../../data/Reader/XLSX/urlImage.notfound.xlsx');
53+
self::assertNotFalse($filename);
54+
$reader = IOFactory::createReader('Xlsx');
55+
$spreadsheet = $reader->load($filename);
56+
$worksheet = $spreadsheet->getActiveSheet();
57+
$collection = $worksheet->getDrawingCollection();
58+
self::assertCount(0, $collection);
59+
}
60+
61+
public function testURLImageSourceBadProtocol(): void
62+
{
63+
$filename = realpath(__DIR__ . '/../../../data/Reader/XLSX/urlImage.bad.dontuse');
64+
self::assertNotFalse($filename);
65+
$this->expectException(SpreadsheetException::class);
66+
$this->expectExceptionMessage('Invalid protocol for linked drawing');
67+
$reader = IOFactory::createReader('Xlsx');
68+
$reader->load($filename);
69+
}
4470
}

‎tests/PhpSpreadsheetTests/Writer/Html/ExtendForChartsAndImagesTest.php‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
namespace PhpOffice\PhpSpreadsheetTests\Writer\Html;
66

7+
use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException;
78
use PhpOffice\PhpSpreadsheet\Spreadsheet;
89
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
910
use PhpOffice\PhpSpreadsheet\Writer\Html;
@@ -59,7 +60,9 @@ public function testSheetWithImageBelowData(): void
5960

6061
// Add a drawing to the worksheet
6162
$drawing = new Drawing();
62-
$drawing->setPath('foo.png', false);
63+
$path = 'tests/data/Writer/XLSX/blue_square.png';
64+
$drawing->setPath($path);
65+
self::assertSame($path, $drawing->getPath());
6366
$drawing->setCoordinates('A5');
6467
$drawing->setWorksheet($sheet);
6568

@@ -74,13 +77,51 @@ public function testSheetWithImageRightOfData(): void
7477

7578
// Add a drawing to the worksheet
7679
$drawing = new Drawing();
77-
$drawing->setPath('foo.png', false);
80+
$path = 'tests/data/Writer/XLSX/blue_square.png';
81+
$drawing->setPath($path);
82+
self::assertSame($path, $drawing->getPath());
7883
$drawing->setCoordinates('E1');
7984
$drawing->setWorksheet($sheet);
8085

8186
$this->assertMaxColumnAndMaxRow($spreadsheet, 5, 3);
8287
}
8388

89+
public function testSheetWithBadImageRightOfData(): void
90+
{
91+
$spreadsheet = new Spreadsheet();
92+
$sheet = $spreadsheet->getActiveSheet();
93+
$sheet->setCellValue('B3', 'foo');
94+
95+
// Add a drawing to the worksheet
96+
$drawing = new Drawing();
97+
$path = 'tests/data/Writer/XLSX/xblue_square.png';
98+
$drawing->setPath($path, false);
99+
self::assertSame('', $drawing->getPath());
100+
$drawing->setCoordinates('E1');
101+
$drawing->setWorksheet($sheet);
102+
103+
$this->assertMaxColumnAndMaxRow($spreadsheet, 2, 3);
104+
}
105+
106+
public function testSheetWithBadImageRightOfDataThrow(): void
107+
{
108+
$this->expectException(PhpSpreadsheetException::class);
109+
$this->expectExceptionMessage('not found!');
110+
$spreadsheet = new Spreadsheet();
111+
$sheet = $spreadsheet->getActiveSheet();
112+
$sheet->setCellValue('B3', 'foo');
113+
114+
// Add a drawing to the worksheet
115+
$drawing = new Drawing();
116+
$path = 'tests/data/Writer/XLSX/xblue_square.png';
117+
$drawing->setPath($path);
118+
self::assertSame('', $drawing->getPath());
119+
$drawing->setCoordinates('E1');
120+
$drawing->setWorksheet($sheet);
121+
122+
$this->assertMaxColumnAndMaxRow($spreadsheet, 2, 3);
123+
}
124+
84125
private function assertMaxColumnAndMaxRow(Spreadsheet $spreadsheet, int $expectedColumnCount, int $expectedRowCount): void
85126
{
86127
$writer = new Html($spreadsheet);

0 commit comments

Comments
 (0)