From ff835faf7b32b49e8161cc1f31a0a5d1ef7de92a Mon Sep 17 00:00:00 2001
From: Ferdinand Thiessen <opensource@fthiessen.de>
Date: Tue, 21 Jan 2025 16:01:17 +0100
Subject: [PATCH] fix(theming): Harden admin web link settings

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
---
 .../lib/Controller/ThemingController.php      |  8 +++---
 .../Controller/ThemingControllerTest.php      | 25 +++++++++++++------
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php
index e649ea78530..983a738ea70 100644
--- a/apps/theming/lib/Controller/ThemingController.php
+++ b/apps/theming/lib/Controller/ThemingController.php
@@ -194,11 +194,13 @@ class ThemingController extends Controller {
 	}
 
 	/**
-	 * Check that a string is a valid http/https url
+	 * Check that a string is a valid http/https url.
+	 * Also validates that there is no way for XSS through HTML
 	 */
 	private function isValidUrl(string $url): bool {
-		return ((str_starts_with($url, 'http://') || str_starts_with($url, 'https://')) &&
-			filter_var($url, FILTER_VALIDATE_URL) !== false);
+		return ((str_starts_with($url, 'http://') || str_starts_with($url, 'https://'))
+			&& filter_var($url, FILTER_VALIDATE_URL) !== false)
+			&& !str_contains($url, '"');
 	}
 
 	/**
diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php
index eead85d1ca3..3c283fcb697 100644
--- a/apps/theming/tests/Controller/ThemingControllerTest.php
+++ b/apps/theming/tests/Controller/ThemingControllerTest.php
@@ -125,11 +125,24 @@ class ThemingControllerTest extends TestCase {
 	}
 
 	public function dataUpdateStylesheetError() {
+		$urls = [
+			'url' => 'web address',
+			'imprintUrl' => 'legal notice address',
+			'privacyUrl' => 'privacy policy address',
+		];
+
+		$urlTests = [];
+		foreach ($urls as $urlKey => $urlName) {
+			// Check length limit
+			$urlTests[] = [$urlKey, 'http://example.com/' . str_repeat('a', 501), "The given {$urlName} is too long"];
+			// Check potential evil javascript
+			$urlTests[] = [$urlKey, 'javascript:alert(1)', "The given {$urlName} is not a valid URL"];
+			// Check XSS
+			$urlTests[] = [$urlKey, 'https://example.com/"><script/src="alert(\'1\')"><a/href/="', "The given {$urlName} is not a valid URL"];
+		}
+
 		return [
 			['name', str_repeat('a', 251), 'The given name is too long'],
-			['url', 'http://example.com/' . str_repeat('a', 501), 'The given web address is too long'],
-			['url', str_repeat('a', 501), 'The given web address is not a valid URL'],
-			['url', 'javascript:alert(1)', 'The given web address is not a valid URL'],
 			['slogan', str_repeat('a', 501), 'The given slogan is too long'],
 			['primary_color', '0082C9', 'The given color is invalid'],
 			['primary_color', '#0082Z9', 'The given color is invalid'],
@@ -137,10 +150,8 @@ class ThemingControllerTest extends TestCase {
 			['background_color', '0082C9', 'The given color is invalid'],
 			['background_color', '#0082Z9', 'The given color is invalid'],
 			['background_color', 'Nextcloud', 'The given color is invalid'],
-			['imprintUrl', '0082C9', 'The given legal notice address is not a valid URL'],
-			['imprintUrl', '0082C9', 'The given legal notice address is not a valid URL'],
-			['imprintUrl', 'javascript:foo', 'The given legal notice address is not a valid URL'],
-			['privacyUrl', '#0082Z9', 'The given privacy policy address is not a valid URL'],
+
+			...$urlTests,
 		];
 	}