From 3d1bd21880e52ec8a2e79bccaf1b2e58b27839f8 Mon Sep 17 00:00:00 2001
From: Adam G-H <32250-phenaproxima@users.noreply.drupalcode.org>
Date: Fri, 15 Dec 2023 03:03:51 +0000
Subject: [PATCH] Issue #3408835: SqliteDatabaseExcluder accidentally includes
the database in stage operations if it is located inside the web root
---
.../src/Event/CollectPathsToExcludeEvent.php | 3 +
.../PathExcluder/SqliteDatabaseExcluder.php | 42 ++--
.../SqliteDatabaseExcluderTest.php | 224 ++++++++----------
3 files changed, 124 insertions(+), 145 deletions(-)
diff --git a/package_manager/src/Event/CollectPathsToExcludeEvent.php b/package_manager/src/Event/CollectPathsToExcludeEvent.php
index f6e6c65dab..59c9c8f438 100644
--- a/package_manager/src/Event/CollectPathsToExcludeEvent.php
+++ b/package_manager/src/Event/CollectPathsToExcludeEvent.php
@@ -85,6 +85,9 @@ final class CollectPathsToExcludeEvent extends StageEvent implements PathListInt
* The paths to ignore. Absolute paths will be made relative to the project
* root; relative paths will be assumed to already be relative to the
* project root, and ignored as given.
+ *
+ * @throws \LogicException
+ * If any of the given paths are absolute, but not inside the project root.
*/
public function addPathsRelativeToProjectRoot(array $paths): void {
$project_root = $this->pathLocator->getProjectRoot();
diff --git a/package_manager/src/PathExcluder/SqliteDatabaseExcluder.php b/package_manager/src/PathExcluder/SqliteDatabaseExcluder.php
index cfcf1fc95b..f21f74fde8 100644
--- a/package_manager/src/PathExcluder/SqliteDatabaseExcluder.php
+++ b/package_manager/src/PathExcluder/SqliteDatabaseExcluder.php
@@ -6,7 +6,7 @@ namespace Drupal\package_manager\PathExcluder;
use Drupal\Core\Database\Connection;
use Drupal\package_manager\Event\CollectPathsToExcludeEvent;
-use Drupal\package_manager\PathLocator;
+use PhpTuf\ComposerStager\API\Path\Factory\PathFactoryInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
/**
@@ -22,16 +22,14 @@ class SqliteDatabaseExcluder implements EventSubscriberInterface {
/**
* Constructs a SqliteDatabaseExcluder object.
*
- * @param \Drupal\package_manager\PathLocator $pathLocator
- * The path locator service.
+ * @param \PhpTuf\ComposerStager\API\Path\Factory\PathFactoryInterface $pathFactory
+ * The path factory service.
* @param \Drupal\Core\Database\Connection $database
* The database connection.
*/
public function __construct(
- private readonly PathLocator $pathLocator,
- // TRICKY: this cannot be private nor readonly for testing purposes.
- // @see \Drupal\Tests\package_manager\Kernel\PathExcluder\SqliteDatabaseExcluderTest::mockDatabase()
- protected Connection $database
+ private readonly PathFactoryInterface $pathFactory,
+ private readonly Connection $database,
) {}
/**
@@ -50,19 +48,27 @@ class SqliteDatabaseExcluder implements EventSubscriberInterface {
* The event object.
*/
public function excludeDatabaseFiles(CollectPathsToExcludeEvent $event): void {
- // If the database is SQLite, it might be located in the active directory
- // and we should exclude it. Always treat it as relative to the project root.
+ // If the database is SQLite, it might be located in the project directory
+ // and we should exclude it.
if ($this->database->driver() === 'sqlite') {
- $options = $this->database->getConnectionOptions();
- // Nothing to exclude if the database lives outside the project root.
- if (str_starts_with($options['database'], '/') && !str_starts_with($options['database'], $this->pathLocator->getProjectRoot())) {
- return;
+ $db_path = $this->database->getConnectionOptions()['database'];
+ // Exclude the database file and auxiliary files created by SQLite.
+ $paths = [$db_path, "$db_path-shm", "$db_path-wal"];
+
+ // If the database path is absolute, it might be outside the project root,
+ // in which case we don't need to do anything.
+ if ($this->pathFactory->create($db_path)->isAbsolute()) {
+ try {
+ $event->addPathsRelativeToProjectRoot($paths);
+ }
+ catch (\LogicException) {
+ // The database is outside of the project root, so we're done.
+ }
+ }
+ else {
+ // The database is in the web root, and must be excluded relative to it.
+ $event->addPathsRelativeToWebRoot($paths);
}
- $event->addPathsRelativeToProjectRoot([
- $options['database'],
- $options['database'] . '-shm',
- $options['database'] . '-wal',
- ]);
}
}
diff --git a/package_manager/tests/src/Kernel/PathExcluder/SqliteDatabaseExcluderTest.php b/package_manager/tests/src/Kernel/PathExcluder/SqliteDatabaseExcluderTest.php
index 37d3fb1223..7ef8a0b615 100644
--- a/package_manager/tests/src/Kernel/PathExcluder/SqliteDatabaseExcluderTest.php
+++ b/package_manager/tests/src/Kernel/PathExcluder/SqliteDatabaseExcluderTest.php
@@ -11,6 +11,7 @@ use Drupal\package_manager\PathExcluder\SqliteDatabaseExcluder;
use Drupal\package_manager\PathLocator;
use Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase;
use PhpTuf\ComposerStager\API\Path\Factory\PathFactoryInterface;
+use Prophecy\Prophecy\ObjectProphecy;
/**
* @covers \Drupal\package_manager\PathExcluder\SqliteDatabaseExcluder
@@ -20,161 +21,130 @@ use PhpTuf\ComposerStager\API\Path\Factory\PathFactoryInterface;
class SqliteDatabaseExcluderTest extends PackageManagerKernelTestBase {
/**
- * {@inheritdoc}
- */
- public function register(ContainerBuilder $container) {
- parent::register($container);
-
- $container->getDefinition(SqliteDatabaseExcluder::class)
- ->setClass(TestSqliteDatabaseExcluder::class);
- }
-
- /**
- * Mocks a SQLite database connection for the event subscriber.
+ * The mocked database connection.
*
- * @param array $connection_options
- * The connection options for the mocked database connection.
+ * @var \Drupal\Core\Database\Connection|\Prophecy\Prophecy\ObjectProphecy
*/
- private function mockDatabase(array $connection_options): void {
- $database = $this->prophesize(Connection::class);
- $database->driver()->willReturn('sqlite');
- $database->getConnectionOptions()->willReturn($connection_options);
-
- /** @var \Drupal\Tests\package_manager\Kernel\PathExcluder\TestSiteConfigurationExcluder $sqlite_excluder */
- $sqlite_excluder = $this->container->get(SqliteDatabaseExcluder::class);
- $sqlite_excluder->database = $database->reveal();
- }
+ private Connection|ObjectProphecy $mockDatabase;
/**
- * Tests that SQLite database files are excluded from stage operations.
+ * {@inheritdoc}
*/
- public function testSqliteDatabaseFilesExcluded(): void {
- // In this test, we want to perform the actual stage operations so that we
- // can be sure that files are staged as expected.
- $this->setSetting('package_manager_bypass_composer_stager', FALSE);
- // Ensure we have an up-to-date container.
- $this->container = $this->container->get('kernel')->rebuildContainer();
-
- $active_dir = $this->container->get(PathLocator::class)->getProjectRoot();
-
- // Mock a SQLite database connection to a file in the active directory. The
- // file should not be staged.
- $this->mockDatabase([
- 'database' => 'sites/example.com/db.sqlite',
- ]);
-
- $stage = $this->createStage();
- $stage->create();
- $stage->require(['ext-json:*']);
- $stage_dir = $stage->getStageDirectory();
+ public function register(ContainerBuilder $container) {
+ parent::register($container);
- $excluded = [
- "sites/example.com/db.sqlite",
- "sites/example.com/db.sqlite-shm",
- "sites/example.com/db.sqlite-wal",
- ];
- foreach ($excluded as $path) {
- $this->assertFileExists("$active_dir/$path");
- $this->assertFileDoesNotExist("$stage_dir/$path");
- }
+ $this->mockDatabase = $this->prophesize(Connection::class);
+ $this->mockDatabase->driver()
+ ->willReturn('sqlite')
+ ->shouldBeCalled();
- $stage->apply();
- // The excluded files should still be in the active directory.
- foreach ($excluded as $path) {
- $this->assertFileExists("$active_dir/$path");
- }
+ $container->getDefinition(SqliteDatabaseExcluder::class)
+ ->setArgument('$database', $this->mockDatabase->reveal());
}
/**
- * Data provider for testPathProcessing().
+ * Data provider for ::testSqliteDatabaseFilesExcluded().
*
- * @return string[][]
+ * @return array[]
* The test cases.
*/
- public function providerPathProcessing(): array {
+ public function providerSqliteDatabaseFilesExcluded(): array {
return [
- 'relative path, in site directory' => [
- 'sites/example.com/db.sqlite',
- [
- 'sites/example.com/db.sqlite',
- 'sites/example.com/db.sqlite-shm',
- 'sites/example.com/db.sqlite-wal',
- ],
+ // If the database is at a relative path, it should be excluded relative
+ // to the web root.
+ 'relative path in relocated web root' => [
+ 'www',
+ 'db.sqlite',
+ 'www/db.sqlite',
+ ],
+ 'relative path, web root is project root' => [
+ '',
+ 'db.sqlite',
+ 'db.sqlite',
+ ],
+ // If the database is at an absolute path in the project root, it should
+ // be excluded relative to the project root.
+ 'absolute path in relocated web root' => [
+ 'www',
+ '<PROJECT_ROOT>/www/db.sqlite',
+ 'www/db.sqlite',
],
- 'relative path, at root' => [
+ 'absolute path, web root is project root' => [
+ '',
+ '<PROJECT_ROOT>/db.sqlite',
'db.sqlite',
- [
- 'db.sqlite',
- 'db.sqlite-shm',
- 'db.sqlite-wal',
- ],
],
- 'absolute path, in site directory' => [
- '/sites/example.com/db.sqlite',
- [
- 'sites/example.com/db.sqlite',
- 'sites/example.com/db.sqlite-shm',
- 'sites/example.com/db.sqlite-wal',
- ],
+ // If the database is outside the project root, the excluder doesn't need
+ // to do anything.
+ 'absolute path outside of project, relocated web root' => [
+ 'www',
+ '/path/to/database.sqlite',
+ FALSE,
],
- 'absolute path, at root' => [
- '/db.sqlite',
- [
- 'db.sqlite',
- 'db.sqlite-shm',
- 'db.sqlite-wal',
- ],
+ 'absolute path outside of project, web root is project root' => [
+ '',
+ '/path/to/database.sqlite',
+ FALSE,
],
];
}
/**
- * Tests SQLite database path processing.
- *
- * This test ensures that SQLite database paths are processed properly (e.g.,
- * converting an absolute path to a relative path) before being flagged for
- * exclusion.
+ * Tests that SQLite database files are excluded from stage operations.
*
- * @param string $database_path
- * The path of the SQLite database, as set in the database connection
- * options. If it begins with a slash, it will be prefixed with the path of
- * the active directory.
- * @param string[] $expected_exclusions
- * The database paths which should be flagged for exclusion.
+ * @param string $web_root
+ * The web root that should be returned by the path locator. See
+ * \Drupal\package_manager\PathLocator::getWebRoot().
+ * @param string $db_path
+ * The path of the SQLite database, as it should be reported by the database
+ * connection. This can be a relative or absolute path; it does not need to
+ * actually exist.
+ * @param string|false $expected_excluded_path
+ * The path to the database, as it should be given to
+ * CollectPathsToExcludeEvent. If FALSE, the database is located outside the
+ * project and therefore is not excluded.
*
- * @dataProvider providerPathProcessing
+ * @dataProvider providerSqliteDatabaseFilesExcluded
*/
- public function testPathProcessing(string $database_path, array $expected_exclusions): void {
+ public function testSqliteDatabaseFilesExcluded(string $web_root, string $db_path, string|false $expected_excluded_path): void {
+ /** @var \Drupal\package_manager_bypass\MockPathLocator $path_locator */
$path_locator = $this->container->get(PathLocator::class);
- $path_factory = $this->container->get(PathFactoryInterface::class);
- // If the database path should be treated as absolute, prefix it with the
- // path of the active directory.
- if (str_starts_with($database_path, '/')) {
- $database_path = $path_locator->getProjectRoot() . $database_path;
+ $project_root = $path_locator->getProjectRoot();
+
+ // Set the mocked web root, keeping everything else as-is.
+ $path_locator->setPaths(
+ $project_root,
+ $path_locator->getVendorDirectory(),
+ $web_root,
+ $path_locator->getStagingRoot(),
+ );
+ $db_path = str_replace('<PROJECT_ROOT>', $project_root, $db_path);
+ $this->mockDatabase->getConnectionOptions()
+ ->willReturn(['database' => $db_path])
+ ->shouldBeCalled();
+
+ $event = new CollectPathsToExcludeEvent(
+ $this->createStage(),
+ $path_locator,
+ $this->container->get(PathFactoryInterface::class),
+ );
+ $actual_excluded_paths = $this->container->get('event_dispatcher')
+ ->dispatch($event)
+ ->getAll();
+
+ if (is_string($expected_excluded_path)) {
+ $expected_exclusions = [
+ $expected_excluded_path,
+ $expected_excluded_path . '-shm',
+ $expected_excluded_path . '-wal',
+ ];
+ $this->assertEmpty(array_diff($expected_exclusions, $actual_excluded_paths));
+ }
+ else {
+ // The path of the database should not appear anywhere in the list of
+ // excluded paths.
+ $this->assertStringNotContainsString($db_path, serialize($actual_excluded_paths));
}
- $this->mockDatabase([
- 'database' => $database_path,
- ]);
-
- $event = new CollectPathsToExcludeEvent($this->createStage(), $path_locator, $path_factory);
- // Invoke the event subscriber directly, so we can check that the database
- // was correctly excluded.
- $this->container->get(SqliteDatabaseExcluder::class)
- ->excludeDatabaseFiles($event);
- // All of the expected exclusions should be flagged.
- $this->assertEquals($expected_exclusions, $event->getAll());
}
}
-
-/**
- * A test-only version of the SQLite database excluder, to expose internals.
- */
-class TestSqliteDatabaseExcluder extends SqliteDatabaseExcluder {
-
- /**
- * {@inheritdoc}
- */
- public Connection $database;
-
-}
--
GitLab