Merge pull request #16619 from nextcloud/bugfix/16446/theming-url-validate

Validate urls in theming settings and properly handle error messages

apps/theming/js/settings-admin.js

@@ -32,7 +32,7 @@ function setThemingValue(setting, value) {
 		hideUndoButton(setting, value);
 		preview(setting, value, response.data.serverCssUrl);
 	}).fail(function(response) {
-		OC.msg.finishedSaving('#theming_settings_msg', response);
+		OC.msg.finishedSaving('#theming_settings_msg', response.responseJSON);
 		$('#theming_settings_loading').hide();
 	});
 }
@@ -159,7 +159,7 @@ $(document).ready(function () {
 				return true;
 			} else {
 				throw t('theming', 'Name cannot be empty');
-			} 
+			}
 		} catch (error) {
 			$('#theming-name').attr('title', error);
 			$('#theming-name').tooltip({placement: 'top', trigger: 'manual'});
@@ -174,7 +174,7 @@ $(document).ready(function () {
 		if (checkName()) {
 			$('#theming-name').tooltip('hide');
 			$('#theming-name').removeClass('error');
-		}  
+		}
 	});
 
 	$('#theming-name').change(function(e) {

apps/theming/lib/Controller/ThemingController.php

@@ -135,68 +135,56 @@ class ThemingController extends Controller {
 	 */
 	public function updateStylesheet($setting, $value) {
 		$value = trim($value);
+		$error = null;
 		switch ($setting) {
 			case 'name':
 				if (strlen($value) > 250) {
-					return new DataResponse([
-						'data' => [
-							'message' => $this->l10n->t('The given name is too long'),
-						],
-						'status' => 'error'
-					]);
+					$error = $this->l10n->t('The given name is too long');
 				}
 				break;
 			case 'url':
 				if (strlen($value) > 500) {
-					return new DataResponse([
-						'data' => [
-							'message' => $this->l10n->t('The given web address is too long'),
-						],
-						'status' => 'error'
-					]);
+					$error = $this->l10n->t('The given web address is too long');
+				}
+				if (!$this->isValidUrl($value)) {
+					$error = $this->l10n->t('The given web address is not a valid URL');
 				}
 				break;
 			case 'imprintUrl':
 				if (strlen($value) > 500) {
-					return new DataResponse([
-						'data' => [
-							'message' => $this->l10n->t('The given legal notice address is too long'),
-						],
-						'status' => 'error'
-					]);
+					$error = $this->l10n->t('The given legal notice address is too long');
+				}
+				if (!$this->isValidUrl($value)) {
+					$error = $this->l10n->t('The given legal notice address is not a valid URL');
 				}
 				break;
 			case 'privacyUrl':
 				if (strlen($value) > 500) {
-					return new DataResponse([
-						'data' => [
-							'message' => $this->l10n->t('The given privacy policy address is too long'),
-						],
-						'status' => 'error'
-					]);
+					$error = $this->l10n->t('The given privacy policy address is too long');
+				}
+				if (!$this->isValidUrl($value)) {
+					$error = $this->l10n->t('The given privacy policy address is not a valid URL');
 				}
 				break;
 			case 'slogan':
 				if (strlen($value) > 500) {
-					return new DataResponse([
-						'data' => [
-							'message' => $this->l10n->t('The given slogan is too long'),
-						],
-						'status' => 'error'
-					]);
+					$error = $this->l10n->t('The given slogan is too long');
 				}
 				break;
 			case 'color':
 				if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) {
-					return new DataResponse([
-						'data' => [
-							'message' => $this->l10n->t('The given color is invalid'),
-						],
-						'status' => 'error'
-					]);
+					$error = $this->l10n->t('The given color is invalid');
 				}
 				break;
 		}
+		if ($error !== null) {
+			return new DataResponse([
+				'data' => [
+					'message' => $error,
+				],
+				'status' => 'error'
+			], Http::STATUS_BAD_REQUEST);
+		}
 
 		$this->themingDefaults->set($setting, $value);
 
@@ -215,6 +203,14 @@ class ThemingController extends Controller {
 		);
 	}
 
+	/**
+	 * Check that a string is a valid http/https url
+	 */
+	private function isValidUrl(string $url): bool {
+		return ((strpos($url, 'http://') === 0 || strpos($url, 'https://') === 0) &&
+			filter_var($url, FILTER_VALIDATE_URL) !== false);
+	}
+
 	/**
 	 * @return DataResponse
 	 * @throws NotPermittedException

apps/theming/tests/Controller/ThemingControllerTest.php

@@ -123,10 +123,13 @@ class ThemingControllerTest extends TestCase {
 	public function dataUpdateStylesheetSuccess() {
 		return [
 			['name', str_repeat('a', 250), 'Saved'],
-			['url', str_repeat('a', 500), 'Saved'],
+			['url', 'https://nextcloud.com/' . str_repeat('a', 478), 'Saved'],
 			['slogan', str_repeat('a', 500), 'Saved'],
 			['color', '#0082c9', 'Saved'],
 			['color', '#0082C9', 'Saved'],
+			['color', '#0082C9', 'Saved'],
+			['imprintUrl', 'https://nextcloud.com/' . str_repeat('a', 478), 'Saved'],
+			['privacyUrl', 'https://nextcloud.com/' . str_repeat('a', 478), 'Saved'],
 		];
 	}
 
@@ -175,11 +178,17 @@ class ThemingControllerTest extends TestCase {
 	public function dataUpdateStylesheetError() {
 		return [
 			['name', str_repeat('a', 251), 'The given name is too long'],
-			['url', str_repeat('a', 501), 'The given web address 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'],
 			['color', '0082C9', 'The given color is invalid'],
 			['color', '#0082Z9', 'The given color is invalid'],
 			['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'],
 		];
 	}
 
@@ -196,7 +205,7 @@ class ThemingControllerTest extends TestCase {
 			->method('set')
 			->with($setting, $value);
 		$this->l10n
-			->expects($this->once())
+			->expects($this->any())
 			->method('t')
 			->will($this->returnCallback(function($str) {
 				return $str;
@@ -209,7 +218,8 @@ class ThemingControllerTest extends TestCase {
 						'message' => $message,
 					],
 				'status' => 'error',
-			]
+			],
+			Http::STATUS_BAD_REQUEST
 		);
 		$this->assertEquals($expected, $this->themingController->updateStylesheet($setting, $value));
 	}