Validate urls in theming settings and properly handle error messages

Signed-off-by: Julius Härtl <jus@bitgrid.net>

diff --git a/apps/theming/js/settings-admin.js b/apps/theming/js/settings-admin.js
index af9c584265c0e49e4d778c9b2f05a572e1b7c6ec..81b10bcc08e860c50dc3e26847c729469673a7d3 100644 (file)
--- a/apps/theming/js/settings-admin.js
+++ b/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) {

diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php
index cc8af2cae3e13061d16827ac465bcf49f041668f..4789533564001d2089c0bba93fa641cfde2ec421 100644 (file)
--- a/apps/theming/lib/Controller/ThemingController.php
+++ b/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

diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php
index 457e9900b5e84a321451cd070ff75f9d4176da46..93a1e040b4ba5a891b557213ca6c02eb5c166efe 100644 (file)
--- a/apps/theming/tests/Controller/ThemingControllerTest.php
+++ b/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));
        }