fix(manifest): Check if app exists instead of accessing null as an array

Signed-off-by: Joas Schilling <coding@schilljs.com>
2026-02-27 18:37:17 +01:00 · 2023-12-11 08:59:45 +01:00
parent ad12af8d06
commit 2fccd090b3
7 changed files with 48 additions and 17 deletions
--- a/apps/theming/lib/Controller/IconController.php
+++ b/apps/theming/lib/Controller/IconController.php
@@ -32,6 +32,7 @@ use OC\IntegrityCheck\Helpers\FileAccessHelper;
 use OCA\Theming\IconBuilder;
 use OCA\Theming\ImageManager;
 use OCA\Theming\ThemingDefaults;
+use OCP\App\IAppManager;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataDisplayResponse;
@@ -50,24 +51,17 @@ class IconController extends Controller {
 	private $imageManager;
 	/** @var FileAccessHelper */
 	private $fileAccessHelper;
+	/** @var IAppManager */
+	private $appManager;

-	/**
-	 * IconController constructor.
-	 *
-	 * @param string $appName
-	 * @param IRequest $request
-	 * @param ThemingDefaults $themingDefaults
-	 * @param IconBuilder $iconBuilder
-	 * @param ImageManager $imageManager
-	 * @param FileAccessHelper $fileAccessHelper
-	 */
 	public function __construct(
 		$appName,
 		IRequest $request,
 		ThemingDefaults $themingDefaults,
 		IconBuilder $iconBuilder,
 		ImageManager $imageManager,
-		FileAccessHelper $fileAccessHelper
+		FileAccessHelper $fileAccessHelper,
+		IAppManager $appManager
 	) {
 		parent::__construct($appName, $request);

@@ -75,6 +69,7 @@ class IconController extends Controller {
 		$this->iconBuilder = $iconBuilder;
 		$this->imageManager = $imageManager;
 		$this->fileAccessHelper = $fileAccessHelper;
+		$this->appManager = $appManager;
 	}

 	/**
@@ -92,6 +87,11 @@ class IconController extends Controller {
 	 * 404: Themed icon not found
 	 */
 	public function getThemedIcon(string $app, string $image): Response {
+		if ($app !== 'core' && !$this->appManager->isEnabledForUser($app)) {
+			$app = 'core';
+			$image = 'favicon.png';
+		}
+
 		$color = $this->themingDefaults->getColorPrimary();
 		try {
 			$iconFileName = $this->imageManager->getCachedImage('icon-' . $app . '-' . $color . str_replace('/', '_', $image));
@@ -121,6 +121,10 @@ class IconController extends Controller {
 	 * 404: Favicon not found
 	 */
 	public function getFavicon(string $app = 'core'): Response {
+		if ($app !== 'core' && !$this->appManager->isEnabledForUser($app)) {
+			$app = 'core';
+		}
+
 		$response = null;
 		$iconFile = null;
 		try {
@@ -163,6 +167,10 @@ class IconController extends Controller {
 	 * 404: Touch icon not found
 	 */
 	public function getTouchIcon(string $app = 'core'): Response {
+		if ($app !== 'core' && !$this->appManager->isEnabledForUser($app)) {
+			$app = 'core';
+		}
+
 		$response = null;
 		try {
 			$iconFile = $this->imageManager->getImage('favicon');
--- a/apps/theming/lib/Controller/ThemingController.php
+++ b/apps/theming/lib/Controller/ThemingController.php
@@ -445,16 +445,18 @@ class ThemingController extends Controller {
 	/**
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=manifest)
 	 *
 	 * Get the manifest for an app
 	 *
 	 * @param string $app ID of the app
 	 * @psalm-suppress LessSpecificReturnStatement The content of the Manifest doesn't need to be described in the return type
-	 * @return JSONResponse<Http::STATUS_OK, array{name: string, short_name: string, start_url: string, theme_color: string, background_color: string, description: string, icons: array{src: non-empty-string, type: string, sizes: string}[], display: string}, array{}>
+	 * @return JSONResponse<Http::STATUS_OK, array{name: string, short_name: string, start_url: string, theme_color: string, background_color: string, description: string, icons: array{src: non-empty-string, type: string, sizes: string}[], display: string}, array{}>|JSONResponse<Http::STATUS_NOT_FOUND, array{}, array{}>
 	 *
 	 * 200: Manifest returned
+	 * 404: App not found
 	 */
-	public function getManifest(string $app) {
+	public function getManifest(string $app): JSONResponse {
 		$cacheBusterValue = $this->config->getAppValue('theming', 'cachebuster', '0');
 		if ($app === 'core' || $app === 'settings') {
 			$name = $this->themingDefaults->getName();
@@ -462,6 +464,12 @@ class ThemingController extends Controller {
 			$startUrl = $this->urlGenerator->getBaseUrl();
 			$description = $this->themingDefaults->getSlogan();
 		} else {
+			if (!$this->appManager->isEnabledForUser($app)) {
+				$response = new JSONResponse([], Http::STATUS_NOT_FOUND);
+				$response->throttle(['action' => 'manifest', 'app' => $app]);
+				return $response;
+			}
+
 			$info = $this->appManager->getAppInfo($app, false, $this->l10n->getLanguageCode());
 			$name = $info['name'] . ' - ' . $this->themingDefaults->getName();
 			$shortName = $info['name'];
--- a/apps/theming/openapi.json
+++ b/apps/theming/openapi.json
@@ -386,6 +386,16 @@
                                }
                            }
                        }
+                    },
+                    "404": {
+                        "description": "App not found",
+                        "content": {
+                            "application/json": {
+                                "schema": {
+                                    "type": "object"
+                                }
+                            }
+                        }
                    }
                }
            }
--- a/apps/theming/tests/Controller/IconControllerTest.php
+++ b/apps/theming/tests/Controller/IconControllerTest.php
@@ -33,6 +33,7 @@ use OCA\Theming\Controller\IconController;
 use OCA\Theming\IconBuilder;
 use OCA\Theming\ImageManager;
 use OCA\Theming\ThemingDefaults;
+use OCP\App\IAppManager;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataDisplayResponse;
 use OCP\AppFramework\Http\FileDisplayResponse;
@@ -57,6 +58,8 @@ class IconControllerTest extends TestCase {
 	private $iconBuilder;
 	/** @var FileAccessHelper|\PHPUnit\Framework\MockObject\MockObject */
 	private $fileAccessHelper;
+	/** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */
+	private $appManager;
 	/** @var ImageManager */
 	private $imageManager;

@@ -66,6 +69,7 @@ class IconControllerTest extends TestCase {
 		$this->iconBuilder = $this->createMock(IconBuilder::class);
 		$this->imageManager = $this->createMock(ImageManager::class);
 		$this->fileAccessHelper = $this->createMock(FileAccessHelper::class);
+		$this->appManager = $this->createMock(IAppManager::class);

 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->timeFactory->expects($this->any())
@@ -80,7 +84,8 @@ class IconControllerTest extends TestCase {
 			$this->themingDefaults,
 			$this->iconBuilder,
 			$this->imageManager,
-			$this->fileAccessHelper
+			$this->fileAccessHelper,
+			$this->appManager,
 		);

 		parent::setUp();
--- a/core/templates/layout.guest.php
+++ b/core/templates/layout.guest.php
@@ -20,7 +20,7 @@ p($theme->getTitle());
 		<link rel="icon" href="<?php print_unescaped(image_path('core', 'favicon.ico')); /* IE11+ supports png */ ?>">
 		<link rel="apple-touch-icon" href="<?php print_unescaped(image_path('core', 'favicon-touch.png')); ?>">
 		<link rel="mask-icon" sizes="any" href="<?php print_unescaped(image_path('core', 'favicon-mask.svg')); ?>" color="<?php p($theme->getColorPrimary()); ?>">
-		<link rel="manifest" href="<?php print_unescaped(image_path('core', 'manifest.json')); ?>">
+		<link rel="manifest" href="<?php print_unescaped(image_path('core', 'manifest.json')); ?>" crossorigin="use-credentials">
 		<?php emit_css_loading_tags($_); ?>
 		<?php emit_script_loading_tags($_); ?>
 		<?php print_unescaped($_['headers']); ?>
--- a/core/templates/layout.public.php
+++ b/core/templates/layout.public.php
@@ -21,7 +21,7 @@ p($theme->getTitle());
 	<link rel="apple-touch-icon" href="<?php print_unescaped(image_path($_['appid'], 'favicon-touch.png')); ?>">
 	<link rel="apple-touch-icon-precomposed" href="<?php print_unescaped(image_path($_['appid'], 'favicon-touch.png')); ?>">
 	<link rel="mask-icon" sizes="any" href="<?php print_unescaped(image_path($_['appid'], 'favicon-mask.svg')); ?>" color="<?php p($theme->getColorPrimary()); ?>">
-	<link rel="manifest" href="<?php print_unescaped(image_path($_['appid'], 'manifest.json')); ?>">
+	<link rel="manifest" href="<?php print_unescaped(image_path($_['appid'], 'manifest.json')); ?>" crossorigin="use-credentials">
 	<?php emit_css_loading_tags($_); ?>
 	<?php emit_script_loading_tags($_); ?>
 	<?php print_unescaped($_['headers']); ?>
--- a/core/templates/layout.user.php
+++ b/core/templates/layout.user.php
@@ -37,7 +37,7 @@ p($theme->getTitle());
 		<link rel="apple-touch-icon" href="<?php print_unescaped(image_path($_['appid'], 'favicon-touch.png')); ?>">
 		<link rel="apple-touch-icon-precomposed" href="<?php print_unescaped(image_path($_['appid'], 'favicon-touch.png')); ?>">
 		<link rel="mask-icon" sizes="any" href="<?php print_unescaped(image_path($_['appid'], 'favicon-mask.svg')); ?>" color="<?php p($theme->getColorPrimary()); ?>">
-		<link rel="manifest" href="<?php print_unescaped(image_path($_['appid'], 'manifest.json')); ?>">
+		<link rel="manifest" href="<?php print_unescaped(image_path($_['appid'], 'manifest.json')); ?>" crossorigin="use-credentials">
 		<?php emit_css_loading_tags($_); ?>
 		<?php emit_script_loading_tags($_); ?>
 		<?php print_unescaped($_['headers']); ?>