From 47a0254bb372cf68626302175d2e5f9d0c10e73b Mon Sep 17 00:00:00 2001 From: =?utf8?q?Julius=20H=C3=A4rtl?= Date: Wed, 31 Jul 2019 10:05:46 +0200 Subject: [PATCH] Validate urls in theming settings and properly handle error messages MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/theming/js/settings-admin.js | 6 +- .../lib/Controller/ThemingController.php | 68 +++++++++---------- .../Controller/ThemingControllerTest.php | 18 +++-- 3 files changed, 49 insertions(+), 43 deletions(-) diff --git a/apps/theming/js/settings-admin.js b/apps/theming/js/settings-admin.js index af9c584265c..81b10bcc08e 100644 --- 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 cc8af2cae3e..47895335640 100644 --- 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 457e9900b5e..93a1e040b4b 100644 --- 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)); } -- 2.39.5