From be0407f907fc982405c7892ae3fd231c96ecc9d9 Mon Sep 17 00:00:00 2001 From: Stanislas Kita <7335054+stonebuzz@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:12:32 +0200 Subject: [PATCH] Fix: use a unique temporary file for the error CSV export --- CHANGELOG.md | 6 + inc/clientinjection.class.php | 91 ++++++++++--- .../ClientInjectionWriteErrorsCsvTest.php | 122 ++++++++++++++++++ 3 files changed, 200 insertions(+), 19 deletions(-) create mode 100644 tests/unit/ClientInjectionWriteErrorsCsvTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 86a8e183..1b25da20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +### Fixed + +- Use a unique temporary file when exporting import errors to CSV + ## [2.15.11] - 2026-09-11 ### Fixed diff --git a/inc/clientinjection.class.php b/inc/clientinjection.class.php index cf244faf..a8a12128 100644 --- a/inc/clientinjection.class.php +++ b/inc/clientinjection.class.php @@ -30,6 +30,7 @@ use Glpi\Application\View\TemplateRenderer; use Glpi\Debug\Profile; use Glpi\Error\ErrorHandler; +use Safe\Exceptions\FilesystemException; use Safe\Exceptions\InfoException; use function Safe\fclose; @@ -40,6 +41,8 @@ use function Safe\json_decode; use function Safe\json_encode; use function Safe\readfile; +use function Safe\realpath; +use function Safe\tempnam; use function Safe\unlink; class PluginDatainjectionClientInjection @@ -334,35 +337,85 @@ private static function escapeCsvFormula(mixed $value): mixed return $value; } - public static function exportErrorsInCSV() + private static function writeErrorsCsv(string $dir, array $error_lines, array $headers, string $delimiter): string { + $upload_dir = realpath($dir); + // tempnam() silently falls back to the system temp dir when the target dir is unusable + if (!is_writable($upload_dir)) { + throw new FilesystemException(sprintf('Upload directory "%s" is not writable.', $dir)); + } - $error_lines = json_decode(PluginDatainjectionSession::getParam('error_lines'), true); - self::stripslashes_array($error_lines); - - if (!in_array($error_lines, ['', '0', []], true)) { - $model = PluginDatainjectionSession::unserialize(PluginDatainjectionSession::getParam('currentmodel')); - $file = PLUGIN_DATAINJECTION_UPLOAD_DIR . basename((string) PluginDatainjectionSession::getParam('file_name')); - - $mappings = $model->getMappings(); - $tmpfile = fopen($file, 'w'); + $file = tempnam($upload_dir, 'ERR'); + $tmpfile = null; + try { + $tmpfile = fopen($file, 'w'); - //If headers present - if ($model->getBackend()->isHeaderPresent()) { - $headers = PluginDatainjectionMapping::getMappingsSortedByRank($model->fields['id']); - fputcsv($tmpfile, $headers, $model->getBackend()->getDelimiter()); + if ($headers !== []) { + fputcsv($tmpfile, $headers, $delimiter); } - //Write lines foreach ($error_lines as $line) { - fputcsv($tmpfile, array_map(self::escapeCsvFormula(...), $line), $model->getBackend()->getDelimiter()); + fputcsv($tmpfile, array_map(self::escapeCsvFormula(...), $line), $delimiter); } fclose($tmpfile); + } catch (FilesystemException $filesystemException) { + self::discardErrorsCsv($tmpfile, $file); + throw $filesystemException; + } + + return $file; + } + + /** + * @param resource|null $handle + */ + private static function discardErrorsCsv($handle, string $file): void + { + // Cleanup failures are swallowed so they never mask the original write error + try { + if (is_resource($handle)) { + fclose($handle); + } + } catch (FilesystemException) { + } + + if (!file_exists($file)) { + return; + } + + try { + unlink($file); + } catch (FilesystemException) { + } + } + + public static function exportErrorsInCSV() + { + + $error_lines = json_decode(PluginDatainjectionSession::getParam('error_lines'), true); + self::stripslashes_array($error_lines); + + if (!in_array($error_lines, ['', '0', []], true)) { + $model = PluginDatainjectionSession::unserialize(PluginDatainjectionSession::getParam('currentmodel')); + $backend = $model->getBackend(); + $headers = $backend->isHeaderPresent() + ? PluginDatainjectionMapping::getMappingsSortedByRank($model->fields['id']) + : []; + + try { + $file = self::writeErrorsCsv(PLUGIN_DATAINJECTION_UPLOAD_DIR, $error_lines, $headers, $backend->getDelimiter()); + } catch (FilesystemException $e) { + ErrorHandler::logCaughtException($e); + Session::addMessageAfterRedirect( + __s('Unable to generate the error file', 'datainjection'), + false, + ERROR, + ); + Html::back(); + } - $name = "Error-" . basename((string) PluginDatainjectionSession::getParam('file_name')); - $name = str_replace(' ', '', $name); - header('Content-disposition: attachment; filename=' . $name); + header('Content-disposition: attachment; filename=Errors.csv'); header('Content-Type: application/octet-stream'); header('Content-Transfer-Encoding: fichier'); header('Content-Length: ' . filesize($file)); diff --git a/tests/unit/ClientInjectionWriteErrorsCsvTest.php b/tests/unit/ClientInjectionWriteErrorsCsvTest.php new file mode 100644 index 00000000..5879eeb2 --- /dev/null +++ b/tests/unit/ClientInjectionWriteErrorsCsvTest.php @@ -0,0 +1,122 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2007-2023 by DataInjection plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/datainjection + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Datainjection\Tests\Unit; + +use Glpi\Tests\DbTestCase; +use PluginDatainjectionClientInjection; +use ReflectionMethod; +use Safe\Exceptions\FilesystemException; + +require_once dirname(__DIR__, 2) . '/inc/clientinjection.class.php'; + +final class ClientInjectionWriteErrorsCsvTest extends DbTestCase +{ + /** @var string[] */ + private array $created_files = []; + + public function tearDown(): void + { + foreach ($this->created_files as $created_file) { + if (file_exists($created_file)) { + unlink($created_file); + } + } + + $this->created_files = []; + + parent::tearDown(); + } + + private function writeErrorsCsv(array $error_lines, array $headers, string $dir = PLUGIN_DATAINJECTION_UPLOAD_DIR): string + { + $write_errors_csv = new ReflectionMethod(PluginDatainjectionClientInjection::class, 'writeErrorsCsv'); + $file = $write_errors_csv->invoke(null, $dir, $error_lines, $headers, ';'); + $this->created_files[] = $file; + + return $file; + } + + public function testFileIsCreatedInUploadDirWithErrPrefix(): void + { + $file = $this->writeErrorsCsv([['a', 'b']], []); + + $this->assertSame(realpath(PLUGIN_DATAINJECTION_UPLOAD_DIR), realpath(dirname($file))); + $this->assertStringStartsWith('ERR', basename($file)); + } + + public function testMissingDirDoesNotFallBackToSystemTempDir(): void + { + $this->expectException(FilesystemException::class); + + $this->writeErrorsCsv([['a']], [], PLUGIN_DATAINJECTION_UPLOAD_DIR . '/missing_dir'); + } + + public function testEachCallUsesADistinctFile(): void + { + $first = $this->writeErrorsCsv([['a']], []); + $second = $this->writeErrorsCsv([['a']], []); + + $this->assertNotSame($first, $second); + } + + public function testContentContainsHeadersAndEscapedLines(): void + { + $file = $this->writeErrorsCsv([['=SUM(A1)', 'safe']], ['name', 'serial']); + + $this->assertSame("name;serial\n'=SUM(A1);safe\n", file_get_contents($file)); + } + + public function testDiscardClosesHandleAndRemovesFile(): void + { + $file = $this->writeErrorsCsv([['a']], []); + $handle = fopen($file, 'r'); + + $this->discardErrorsCsv($handle, $file); + + $this->assertFalse(is_resource($handle)); + $this->assertFileDoesNotExist($file); + } + + public function testDiscardIgnoresCleanupFailures(): void + { + $missing_file = PLUGIN_DATAINJECTION_UPLOAD_DIR . '/missing_file.csv'; + + $this->discardErrorsCsv(null, $missing_file); + + $this->assertFileDoesNotExist($missing_file); + } + + private function discardErrorsCsv($handle, string $file): void + { + $discard_errors_csv = new ReflectionMethod(PluginDatainjectionClientInjection::class, 'discardErrorsCsv'); + $discard_errors_csv->invoke(null, $handle, $file); + } +}