Merge pull request #1281 from CybotTM/fix/plantuml-renderer-robustness

[BUGFIX] Improve PlantumlRenderer robustness and temp file cleanup

Author: jaapio

packages/guides-graphs/src/Graphs/Renderer/PlantumlRenderer.php

@@ -25,6 +25,7 @@
 use function mkdir;
 use function sys_get_temp_dir;
 use function tempnam;
+use function unlink;
 
 final class PlantumlRenderer implements DiagramRenderer
 {
@@ -53,11 +54,25 @@ public function render(RenderContext $renderContext, string $diagram): string|nu
 @enduml
 PUML;
 
-        if (!is_dir($this->tempDirectory)) {
-            mkdir($this->tempDirectory, 0o755, true);
+        if (!$this->ensureDirectoryExists($this->tempDirectory)) {
+            $this->logger->error(
+                'Failed to create temp directory: ' . $this->tempDirectory,
+                $renderContext->getLoggerInformation(),
+            );
+
+            return null;
         }
 
         $pumlFileLocation = tempnam($this->tempDirectory, 'pu_');
+        if ($pumlFileLocation === false) {
+            $this->logger->error(
+                'Failed to create temporary file for diagram',
+                $renderContext->getLoggerInformation(),
+            );
+
+            return null;
+        }
+
         file_put_contents($pumlFileLocation, $output);
         try {
             $process = new Process([$this->plantUmlBinaryPath, '-tsvg', $pumlFileLocation], __DIR__, null, null, 600.0);
@@ -86,6 +101,31 @@ public function render(RenderContext $renderContext, string $diagram): string|nu
             return null;
         }
 
-        return file_get_contents($pumlFileLocation . '.svg') ?: null;
+        $svg = file_get_contents($pumlFileLocation . '.svg') ?: null;
+
+        @unlink($pumlFileLocation);
+        @unlink($pumlFileLocation . '.svg');
+
+        return $svg;
+    }
+
+    /**
+     * Ensures the directory exists, handling race conditions safely.
+     *
+     * @return bool True if directory exists or was created, false on failure
+     */
+    private function ensureDirectoryExists(string $directory): bool
+    {
+        if (is_dir($directory)) {
+            return true;
+        }
+
+        // Attempt to create the directory (suppress warning if concurrent process creates it)
+        if (@mkdir($directory, 0o755, true)) {
+            return true;
+        }
+
+        // mkdir failed - check if another process created it (race condition)
+        return is_dir($directory);
     }
 }

packages/guides-graphs/tests/unit/Renderer/PlantumlRendererTest.php

@@ -13,35 +13,93 @@
 
 namespace phpDocumentor\Guides\Graphs\Renderer;
 
+use FilesystemIterator;
 use phpDocumentor\Guides\RenderContext;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\NullLogger;
+use RecursiveDirectoryIterator;
+use RecursiveIteratorIterator;
+use SplFileInfo;
+use Throwable;
 
+use function assert;
+use function basename;
+use function is_dir;
+use function realpath;
 use function rmdir;
 use function sys_get_temp_dir;
 use function uniqid;
+use function unlink;
+
+use const DIRECTORY_SEPARATOR;
 
 final class PlantumlRendererTest extends TestCase
 {
+    private const TEMP_DIR_PREFIX = 'plantuml-test-';
+
     public function testRenderCreatesTempDirectoryWhenMissing(): void
     {
-        $tempDir = sys_get_temp_dir() . '/plantuml-test-' . uniqid();
+        $tempDir = sys_get_temp_dir() . '/' . self::TEMP_DIR_PREFIX . uniqid('', true);
 
-        // Ensure the directory does not exist
         self::assertDirectoryDoesNotExist($tempDir);
 
-        // Use a non-existent binary path - the render will fail but directory should be created first
         $renderer = new PlantumlRenderer(new NullLogger(), '/non/existent/plantuml', $tempDir);
 
         $renderContext = $this->createMock(RenderContext::class);
         $renderContext->method('getLoggerInformation')->willReturn([]);
 
-        // The render will fail due to missing binary, but the temp directory should be created
-        $renderer->render($renderContext, 'A -> B');
+        try {
+            // Render will fail (returns null) or throw due to non-existent plantuml binary.
+            // We only care about verifying that the temp directory was created.
+            $renderer->render($renderContext, 'A -> B');
+        } catch (Throwable) {
+            // Expected: plantuml binary doesn't exist
+        }
+
+        self::assertDirectoryExists($tempDir, 'Temp directory should have been created');
+
+        $this->removeTempDirSafely($tempDir);
+    }
+
+    private function removeTempDirSafely(string $dir): void
+    {
+        if ($dir === '' || !is_dir($dir)) {
+            return;
+        }
+
+        $realDir = realpath($dir);
+        if ($realDir === false) {
+            return;
+        }
+
+        $realSysTmp = realpath(sys_get_temp_dir());
+        if ($realSysTmp === false) {
+            return;
+        }
+
+        // Safety: must be under system temp and have our prefix
+        self::assertStringStartsWith(
+            $realSysTmp . DIRECTORY_SEPARATOR,
+            $realDir . DIRECTORY_SEPARATOR,
+            'Refusing to delete directory outside system temp',
+        );
+        self::assertStringContainsString(
+            self::TEMP_DIR_PREFIX,
+            basename($realDir),
+            'Refusing to delete directory without expected prefix',
+        );
+
+        /** @var RecursiveIteratorIterator<RecursiveDirectoryIterator> $iterator */
+        $iterator = new RecursiveIteratorIterator(
+            new RecursiveDirectoryIterator($realDir, FilesystemIterator::SKIP_DOTS),
+            RecursiveIteratorIterator::CHILD_FIRST,
+        );
 
-        self::assertDirectoryExists($tempDir);
+        foreach ($iterator as $file) {
+            assert($file instanceof SplFileInfo);
+            $file->isDir() ? @rmdir($file->getPathname()) : @unlink($file->getPathname());
+        }
 
-        // Clean up
-        @rmdir($tempDir);
+        @rmdir($realDir);
     }
 }