Merge pull request #1666 from solracsf/fix/empty-app-id-whitelist

fix(whitelisting): Never treat the empty app id as whitelisted

Author: CarlSchwan

lib/AppWhitelist.php

@@ -28,7 +28,7 @@ class AppWhitelist {
 
 	private readonly int $baseUrlLength;
 
-	public const WHITELIST_ALWAYS = ',core,theming,settings,avatar,files,heartbeat,dav,guests,impersonate,accessibility,terms_of_service,dashboard,weather_status,user_status,apporder,twofactor_totp,twofactor_webauthn,twofactor_backupcodes,twofactor_nextcloud_notification';
+	public const WHITELIST_ALWAYS = 'core,theming,settings,avatar,files,heartbeat,dav,guests,impersonate,accessibility,terms_of_service,dashboard,weather_status,user_status,apporder,twofactor_totp,twofactor_webauthn,twofactor_backupcodes,twofactor_nextcloud_notification';
 
 	public const DEFAULT_WHITELIST = 'files_trashbin,files_versions,files_sharing,files_texteditor,text,activity,firstrunwizard,photos,notifications,dashboard,user_status,weather_status';
 

lib/Config.php

@@ -104,7 +104,7 @@ public function setUseWhitelist(bool $use): void {
 	 * @return list<string>
 	 */
 	public function getAppWhitelist(): array {
-		return explode(',', $this->appConfig->getAppValueString(ConfigLexicon::WHITE_LIST));
+		return array_values(array_filter(explode(',', $this->appConfig->getAppValueString(ConfigLexicon::WHITE_LIST)), static fn (string $app): bool => $app !== ''));
 	}
 
 	public function setAppWhitelist(array $whitelist): void {

tests/unit/AppWhitelistTest.php

@@ -82,4 +82,15 @@ public function testIsUrlAllowedNoWhitelist(): void {
 		$this->assertTrue($this->appWhitelist->isUrlAllowed($user, '/apps/news/...'));
 		$this->assertTrue($this->appWhitelist->isUrlAllowed($user, '/apps/foo/...'));
 	}
+
+	/**
+	 * getRequestedApp() returns an empty string for urls that carry no
+	 * resolvable app id, so it must never be treated as whitelisted.
+	 */
+	public function testEmptyAppIdIsNotWhitelisted(): void {
+		$this->config->method('getAppWhitelist')
+			->willReturn(['foo', 'bar']);
+
+		$this->assertFalse($this->appWhitelist->isAppWhitelisted(''));
+	}
 }

tests/unit/ConfigTest.php

@@ -123,6 +123,17 @@ public function testGetAppWhitelist(): void {
 		$this->assertEquals(['app1', 'app2', 'app3'], $this->guestConfig->getAppWhitelist());
 	}
 
+	public function testGetAppWhitelistEmpty(): void {
+		$this->appConfig->method('getAppValueString')
+			->with('whitelist')
+			->willReturn('');
+
+		// explode() on an empty string yields a single empty entry, which
+		// would both whitelist the empty app id and show up as a blank entry
+		// in the admin settings.
+		$this->assertEquals([], $this->guestConfig->getAppWhitelist());
+	}
+
 	public function testSetAppWhitelistArray(): void {
 		$this->appConfig->expects($this->once())
 			->method('setAppValueString')