fix: Do not DI the database connection to prevent cyclic dependency

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
nextcloud · Aug 26, 2024 · 2d1b6a2 · 2d1b6a2
1 parent 1547eab
commit 2d1b6a2
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 18 deletions.
diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php
@@ -27,6 +27,8 @@ class FilenameValidator implements IFilenameValidator {
 
 	private IL10N $l10n;
 
+	private ?IDBConnection $database;
+
 	/**
 	 * @var list<string>
 	 */
@@ -48,7 +50,6 @@ class FilenameValidator implements IFilenameValidator {
 
 	public function __construct(
 		IFactory $l10nFactory,
-		private IDBConnection $database,
 		private IConfig $config,
 		private LoggerInterface $logger,
 	) {
@@ -187,6 +188,11 @@ public function validateFilename(string $filename): void {
 			throw new FileNameTooLongException();
 		}
 
+		// We need to lazy load the database as otherwise there is a cyclic dependency
+		if (!isset($this->database)) {
+			$this->database = \OCP\Server::get(IDBConnection::class);
+		}
+
 		if (!$this->database->supports4ByteText()) {
 			// verify database - e.g. mysql only 3-byte chars
 			if (preg_match('%(?:

diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php
@@ -24,12 +24,16 @@
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
+/**
+ * @group DB
+ */
 class FilenameValidatorTest extends TestCase {
 
 	protected IFactory&MockObject $l10n;
 	protected IConfig&MockObject $config;
 	protected IDBConnection&MockObject $database;
 	protected LoggerInterface&MockObject $logger;
+	protected bool $dbSupportsUtf8 = true;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -45,7 +49,13 @@ protected function setUp(): void {
 		$this->config = $this->createMock(IConfig::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->database = $this->createMock(IDBConnection::class);
-		$this->database->method('supports4ByteText')->willReturn(true);
+		$this->database->method('supports4ByteText')->willReturnCallback(fn () => $this->dbSupportsUtf8);
+		$this->overwriteService(IDBConnection::class, $this->database);
+	}
+
+	protected function tearDown(): void {
+		$this->restoreAllServices();
+		parent::tearDown();
 	}
 
 	/**
@@ -67,7 +77,7 @@ public function testValidateFilename(
 				'getForbiddenExtensions',
 				'getForbiddenFilenames',
 			])
-			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
 			->getMock();
 
 		$validator->method('getForbiddenBasenames')
@@ -106,7 +116,7 @@ public function testIsFilenameValid(
 				'getForbiddenFilenames',
 				'getForbiddenCharacters',
 			])
-			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
 			->getMock();
 
 		$validator->method('getForbiddenBasenames')
@@ -186,20 +196,17 @@ public function dataValidateFilename(): array {
 	 * @dataProvider data4ByteUnicode
 	 */
 	public function testDatabaseDoesNotSupport4ByteText($filename): void {
-		$database = $this->createMock(IDBConnection::class);
-		$database->expects($this->once())
-			->method('supports4ByteText')
-			->willReturn(false);
+		$this->dbSupportsUtf8 = false;
+
 		$this->expectException(InvalidCharacterInPathException::class);
-		$validator = new FilenameValidator($this->l10n, $database, $this->config, $this->logger);
+		$validator = new FilenameValidator($this->l10n, $this->config, $this->logger);
 		$validator->validateFilename($filename);
 	}
 
 	public function data4ByteUnicode(): array {
 		return [
 			['plane 1 𐪅'],
 			['emoji 😶‍🌫️'],
-
 		];
 	}
 
@@ -208,7 +215,7 @@ public function data4ByteUnicode(): array {
 	 */
 	public function testInvalidAsciiCharactersAreAlwaysForbidden(string $filename): void {
 		$this->expectException(InvalidPathException::class);
-		$validator = new FilenameValidator($this->l10n, $this->database, $this->config, $this->logger);
+		$validator = new FilenameValidator($this->l10n, $this->config, $this->logger);
 		$validator->validateFilename($filename);
 	}
 
@@ -256,7 +263,7 @@ public function testIsForbidden(string $filename, array $forbiddenNames, bool $e
 		/** @var FilenameValidator&MockObject */
 		$validator = $this->getMockBuilder(FilenameValidator::class)
 			->onlyMethods(['getForbiddenFilenames'])
-			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
 			->getMock();
 
 		$validator->method('getForbiddenFilenames')

diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php
@@ -508,11 +508,6 @@ protected function getGroupAnnotations(): array {
 	}
 
 	protected function IsDatabaseAccessAllowed() {
-		// on travis-ci.org we allow database access in any case - otherwise
-		// this will break all apps right away
-		if (getenv('TRAVIS') == true) {
-			return true;
-		}
 		$annotations = $this->getGroupAnnotations();
 		if (isset($annotations)) {
 			if (in_array('DB', $annotations) || in_array('SLOWDB', $annotations)) {
@@ -548,7 +543,7 @@ protected function assertTemplate($expectedHtml, $template, $vars = []) {
 				return vsprintf($text, $parameters);
 			});
 
-		$t = new Base($template, $requestToken, $l10n, $theme);
+		$t = new Base($template, $requestToken, $l10n, $theme, base64_encode('csp-nonce'));
 		$buf = $t->fetchPage($vars);
 		$this->assertHtmlStringEqualsHtmlString($expectedHtml, $buf);
 	}