Merge pull request #19084 from nextcloud/bug/13556/wrong-paths-for-svg

Make it possible to resolve svg's outside \OC::$SERVERROOT

core/Controller/SvgController.php

@@ -32,6 +32,7 @@ declare(strict_types=1);
 namespace OC\Core\Controller;
 
 use OC\Template\IconsCacher;
+use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
@@ -97,13 +98,13 @@ class SvgController extends Controller {
 	 * @return DataDisplayResponse|NotFoundResponse
 	 */
 	public function getSvgFromApp(string $app, string $fileName, string $color = 'ffffff') {
-		$appRootPath = $this->appManager->getAppPath($app);
-		$appPath = substr($appRootPath, strlen($this->serverRoot));
-
-		if (!$appPath) {
+		try {
+			$appPath = $this->appManager->getAppPath($app);
+		} catch (AppPathNotFoundException $e) {
 			return new NotFoundResponse();
 		}
-		$path = $this->serverRoot . $appPath ."/img/$fileName.svg";
+
+		$path = $appPath . "/img/$fileName.svg";
 		return $this->getSvg($path, $color, $fileName);
 	}
 

tests/Core/Controller/SvgControllerTest.php

@@ -28,6 +28,7 @@ namespace Tests\Core\Controller;
 use OC\AppFramework\Http;
 use OC\Core\Controller\SvgController;
 use OC\Template\IconsCacher;
+use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IRequest;
@@ -46,6 +47,9 @@ class SvgControllerTest extends TestCase {
 		self::TEST_IMAGE_RECT,
 	];
 
+	/** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */
+	private $appManager;
+
 	/**
 	 * @var SvgController
 	 */
@@ -87,17 +91,20 @@ class SvgControllerTest extends TestCase {
 	 * @return void
 	 */
 	public function setupSvgController() {
+		/** @var IRequest */
 		$request = $this->getMockBuilder(IRequest::class)->getMock();
+		/** @var ITimeFactory $timeFactory */
 		$timeFactory = $this->getMockBuilder(ITimeFactory::class)->getMock();
-		$appManager = $this->getMockBuilder(IAppManager::class)->getMock();
+		/** @var IAppManager */
+		$this->appManager = $this->getMockBuilder(IAppManager::class)->getMock();
+		/** @var IconsCacher $iconsCacher */
 		$iconsCacher = $this->getMockBuilder(IconsCacher::class)->disableOriginalConstructor()->setMethods(['__construct'])->getMock();
-		$this->svgController = new SvgController('core', $request, $timeFactory, $appManager, $iconsCacher);
+		$this->svgController = new SvgController('core', $request, $timeFactory, $this->appManager, $iconsCacher);
 	}
 
 	/**
 	 * Checks that requesting an unknown image results in a 404.
 	 *
-	 * @test
 	 * @return void
 	 */
 	public function testGetSvgFromCoreNotFound() {
@@ -120,7 +127,6 @@ class SvgControllerTest extends TestCase {
 	/**
 	 * Tests that retrieving a colored SVG works.
 	 *
-	 * @test
 	 * @dataProvider provideGetSvgFromCoreTestData
 	 * @param string $name The requested svg name
 	 * @param string $color The requested color
@@ -138,4 +144,55 @@ class SvgControllerTest extends TestCase {
 
 		self::assertEquals($expected, $response->getData());
 	}
+
+	/**
+	 * Checks that requesting an unknown image results in a 404.
+	 */
+	public function testGetSvgFromAppNotFound(): void {
+		$this->appManager->expects($this->once())
+			->method('getAppPath')
+			->with('invalid_app')
+			->willThrowException(new AppPathNotFoundException('Could not find path for invalid_app'));
+
+		$response = $this->svgController->getSvgFromApp('invalid_app', 'some-icon', '#ff0000');
+		self::assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus());
+	}
+
+	/**
+	 * Provides svg coloring test data.
+	 *
+	 * @return array
+	 */
+	public function provideGetSvgFromAppTestData(): array {
+		return [
+			'settings admin' => ['settings', 'admin', 'f00', file_get_contents(self::TEST_IMAGES_SOURCE_PATH . '/settings-admin-red.svg')],
+			'files app' => ['files', 'app', 'f00', file_get_contents(self::TEST_IMAGES_SOURCE_PATH . '/files-app-red.svg')],
+		];
+	}
+
+	/**
+	 * Tests that retrieving a colored SVG works.
+	 *
+	 * @dataProvider provideGetSvgFromAppTestData
+	 * @param string $appName
+	 * @param string $name The requested svg name
+	 * @param string $color The requested color
+	 * @param string $expected
+	 */
+	public function testGetSvgFromApp(string $appName, string $name, string $color, string $expected): void {
+		$this->appManager->expects($this->once())
+			->method('getAppPath')
+			->with($appName)
+			->willReturn(__DIR__ . '/../../../apps/' . $appName);
+
+		$response = $this->svgController->getSvgFromApp($appName, $name, $color);
+
+		self::assertEquals(Http::STATUS_OK, $response->getStatus());
+
+		$headers = $response->getHeaders();
+		self::assertArrayHasKey('Content-Type', $headers);
+		self::assertEquals($headers['Content-Type'], 'image/svg+xml');
+
+		self::assertEquals($expected, $response->getData());
+	}
 }

tests/data/svg/files-app-red.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" height="32" viewBox="0 0 32 32" width="32" version="1.1"><path fill="#f00" d="m3 4c-0.5 0-1 0.5-1 1v22c0 0.52 0.48 1 1 1h26c0.52 0 1-0.482 1-1v-18c0-0.5-0.5-1-1-1h-13l-4-4z"/></svg>

tests/data/svg/settings-admin-red.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewbox="0 0 16 16" height="16" width="16" version="1.1"><path color="#000" d="m1 1v4h4v-4h-4zm5 1v2h8v-2h-8zm-5 4v4h4v-4h-4zm5 1v2h8v-2h-8zm-5 4v4h4v-4h-4zm1 1h2v2h-2v-2zm4 0v2h8v-2h-8z" fill="#f00"/></svg>

tests/lib/App/AppManagerTest.php

@@ -338,6 +338,35 @@ class AppManagerTest extends TestCase {
 		$this->assertEquals(\OC::$SERVERROOT . '/apps/files', $this->manager->getAppPath('files'));
 	}
 
+	public function testGetAppPathSymlink() {
+		$fakeAppDirname = sha1(uniqid('test', true));
+		$fakeAppPath = sys_get_temp_dir() . '/' . $fakeAppDirname;
+		$fakeAppLink = \OC::$SERVERROOT . '/' . $fakeAppDirname;
+
+		mkdir($fakeAppPath);
+		if (symlink($fakeAppPath, $fakeAppLink) === false) {
+			$this->markTestSkipped('Failed to create symlink');
+		}
+
+		// Use the symlink as the app path
+		\OC::$APPSROOTS[] = [
+			'path' => $fakeAppLink,
+			'url' => \OC::$WEBROOT . '/' . $fakeAppDirname,
+			'writable' => false,
+		];
+
+		$fakeTestAppPath = $fakeAppPath . '/' . 'test-test-app';
+		mkdir($fakeTestAppPath);
+
+		$generatedAppPath = $this->manager->getAppPath('test-test-app');
+
+		rmdir($fakeTestAppPath);
+		unlink($fakeAppLink);
+		rmdir($fakeAppPath);
+
+		$this->assertEquals($fakeAppLink . '/test-test-app', $generatedAppPath);
+	}
+
 	public function testGetAppPathFail() {
 		$this->expectException(AppPathNotFoundException::class);
 		$this->manager->getAppPath('testnotexisting');