Encode distribution paths to prevent package owners escaping storage containment

Author: codedmonkey

src/Package/PackageDistributionResolver.php

@@ -13,6 +13,7 @@
 use CodedMonkey\Dirigent\Message\ResolveDistribution;
 use Symfony\Component\DependencyInjection\Attribute\Autowire;
 use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\Filesystem\Path;
 use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Lock\SharedLockInterface;
 use Symfony\Component\Messenger\MessageBusInterface;
@@ -34,7 +35,7 @@ public function __construct(
         string $storagePath,
     ) {
         $this->filesystem = new Filesystem();
-        $this->storagePath = "$storagePath/distribution";
+        $this->storagePath = Path::canonicalize("$storagePath/distribution");
     }
 
     public function exists(Metadata $metadata, string $type): bool
@@ -44,12 +45,20 @@ public function exists(Metadata $metadata, string $type): bool
 
     public function path(Metadata $metadata, string $type): string
     {
-        $packageName = $metadata->getPackage()->getName();
-        $versionName = $metadata->getNormalizedVersionName();
+        $packageName = explode('/', $metadata->getPackage()->getName(), 2)
+            |> (fn($x) => array_map($this->encodePathComponent(...), $x))
+            |> (fn($x) => implode('/', $x));
+        $versionName = $this->encodePathComponent($metadata->getNormalizedVersionName());
         $revision = $metadata->getRevision();
-        $reference = $metadata->getReference();
+        $reference = $this->encodePathComponent($metadata->getReference());
+        $type = $this->encodePathComponent($type);
 
-        return "{$this->storagePath}/{$packageName}/{$versionName}-r{$revision}-{$reference}.{$type}";
+        $path = Path::canonicalize("{$this->storagePath}/{$packageName}/{$versionName}-r{$revision}-{$reference}.{$type}");
+        if (!Path::isBasePath($this->storagePath, $path) || $this->storagePath === $path) {
+            throw new \RuntimeException('Distribution path is outside the configured storage directory.');
+        }
+
+        return $path;
     }
 
     public function remove(Distribution $distribution): void
@@ -60,16 +69,11 @@ public function remove(Distribution $distribution): void
         try {
             $this->filesystem->remove($path);
 
-            // Remove the package directory if it's empty
-            $packageDirectory = dirname($path);
-            if (is_dir($packageDirectory) && !new \FilesystemIterator($packageDirectory)->valid()) {
-                $this->filesystem->remove($packageDirectory);
-            }
-
-            // Remove the vendor directory if it's empty
-            $vendorDirectory = dirname($packageDirectory);
-            if (is_dir($vendorDirectory) && !new \FilesystemIterator($vendorDirectory)->valid()) {
-                $this->filesystem->remove($vendorDirectory);
+            // Remove parent directories that aren't empty
+            $directory = dirname($path);
+            while ($this->storagePath !== $directory && Path::isBasePath($this->storagePath, $directory) && is_dir($directory) && !new \FilesystemIterator($directory)->valid()) {
+                $this->filesystem->remove($directory);
+                $directory = dirname($directory);
             }
         } finally {
             $lock->release();
@@ -165,4 +169,16 @@ private function fileExists(string $path): bool
     {
         return $this->filesystem->exists($path);
     }
+
+    private function encodePathComponent(string $component): string
+    {
+        $encodedComponent = rawurlencode($component);
+
+        return match ($encodedComponent) {
+            '' => '%00',
+            '.' => '%2E',
+            '..' => '%2E%2E',
+            default => $encodedComponent,
+        };
+    }
 }

tests/UnitTests/Package/PackageDistributionResolverTest.php

@@ -12,6 +12,7 @@
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\Filesystem\Path;
 use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Lock\SharedLockInterface;
 use Symfony\Component\Messenger\MessageBusInterface;
@@ -35,6 +36,30 @@ protected function tearDown(): void
         new Filesystem()->remove($this->storagePath);
     }
 
+    public function testPathEncodesMetadataAndKeepsTraversalReferenceInsideStorage(): void
+    {
+        [$package, , $metadata] = $this->createMockPackageWithMetadata();
+        $package->setName('../outside');
+        $metadata->setNormalizedVersionName('../../version');
+        $metadata->setDistributionReference('../../../archive');
+
+        $resolver = new PackageDistributionResolver(
+            $this->createStub(MessageBusInterface::class),
+            $this->createStub(ComposerClient::class),
+            $this->createStub(DistributionRepository::class),
+            $this->createStub(LockFactory::class),
+            true,
+            $this->storagePath,
+        );
+
+        $path = $resolver->path($metadata, '../zip');
+
+        self::assertTrue(Path::isBasePath($this->storagePath . '/distribution', $path));
+        self::assertStringNotContainsString('../', $path);
+        self::assertStringNotContainsString('../../../archive', $path);
+        self::assertStringContainsString('..%2F..%2F..%2Farchive', $path);
+    }
+
     public function testRemoveDeletesDistributionFile(): void
     {
         [, , $metadata] = $this->createMockPackageWithMetadata();
@@ -92,6 +117,35 @@ public function testRemoveKeepsNonEmptyPackageDirectory(): void
         self::assertDirectoryExists(dirname($path));
     }
 
+    public function testRemoveWithTraversalReferenceDoesNotDeleteOutsideDistributionStorage(): void
+    {
+        [, , $metadata] = $this->createMockPackageWithMetadata();
+        $metadata->setDistributionReference('../../../../outside');
+        $distribution = new Distribution($metadata, 'zip');
+
+        $lock = $this->createStub(SharedLockInterface::class);
+        $lockFactory = $this->createStub(LockFactory::class);
+        $lockFactory->method('createLock')->willReturn($lock);
+
+        $resolver = new PackageDistributionResolver(
+            $this->createStub(MessageBusInterface::class),
+            $this->createStub(ComposerClient::class),
+            $this->createStub(DistributionRepository::class),
+            $lockFactory,
+            true,
+            $this->storagePath,
+        );
+
+        $distributionPath = $this->dumpStubDistribution($resolver, $distribution);
+        $outsidePath = $this->storagePath . '/outside.zip';
+        new Filesystem()->dumpFile($outsidePath, 'outside');
+
+        $resolver->remove($distribution);
+
+        self::assertFileDoesNotExist($distributionPath);
+        self::assertFileExists($outsidePath);
+    }
+
     private function dumpStubDistribution(PackageDistributionResolver $resolver, Distribution $distribution): string
     {
         $path = $resolver->path($distribution->getMetadata(), $distribution->getType());