Merge pull request #685 from nextcloud/validate-theming-input

Validate the input of the theming options
nextcloud · Aug 1, 2016 · 368e1c3 · 368e1c3
2 parents ffd29e9 + 13c19e5
commit 368e1c3
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 30 deletions.
diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php
@@ -100,6 +100,50 @@ public function __construct(
 	 * @internal param string $color
 	 */
 	public function updateStylesheet($setting, $value) {
+		$value = trim($value);
+		switch ($setting) {
+			case 'name':
+				if (strlen($value) > 250) {
+					return new DataResponse([
+						'data' => [
+							'message' => $this->l->t('The given name is too long'),
+						],
+						'status' => 'error'
+					]);
+				}
+				break;
+			case 'url':
+				if (strlen($value) > 500) {
+					return new DataResponse([
+						'data' => [
+							'message' => $this->l->t('The given web address is too long'),
+						],
+						'status' => 'error'
+					]);
+				}
+				break;
+			case 'slogan':
+				if (strlen($value) > 500) {
+					return new DataResponse([
+						'data' => [
+							'message' => $this->l->t('The given slogan is too long'),
+						],
+						'status' => 'error'
+					]);
+				}
+				break;
+			case 'color':
+				if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) {
+					return new DataResponse([
+						'data' => [
+							'message' => $this->l->t('The given color is invalid'),
+						],
+						'status' => 'error'
+					]);
+				}
+				break;
+		}
+
 		$this->template->set($setting, $value);
 		return new DataResponse(
 			[

diff --git a/apps/theming/templates/settings-admin.php b/apps/theming/templates/settings-admin.php
@@ -36,25 +36,25 @@
 	<?php } else { ?>
 	<p>
 		<label><span><?php p($l->t('Name')) ?></span>
-			<input id="theming-name" type="text" placeholder="<?php p($l->t('Name')); ?>" value="<?php p($_['name']) ?>" />
+			<input id="theming-name" type="text" placeholder="<?php p($l->t('Name')); ?>" value="<?php p($_['name']) ?>" maxlength="250" />
 		</label>
 		<span data-setting="name" data-toggle="tooltip" data-original-title="<?php p($l->t('reset to default')); ?>" class="theme-undo icon icon-history"></span>
 	</p>
 	<p>
 		<label><span><?php p($l->t('Web address')) ?></span>
-			<input id="theming-url" type="text" placeholder="<?php p($l->t('Web address https://…')); ?>" value="<?php p($_['url']) ?>" />
+			<input id="theming-url" type="text" placeholder="<?php p($l->t('Web address https://…')); ?>" value="<?php p($_['url']) ?>" maxlength="500" />
 		</label>
 		<span data-setting="url" data-toggle="tooltip" data-original-title="<?php p($l->t('reset to default')); ?>" class="theme-undo icon icon-history"></span>
 	</p>
 	<p>
 		<label><span><?php p($l->t('Slogan')) ?></span>
-			<input id="theming-slogan" type="text" placeholder="<?php p($l->t('Slogan')); ?>" value="<?php p($_['slogan']) ?>" />
+			<input id="theming-slogan" type="text" placeholder="<?php p($l->t('Slogan')); ?>" value="<?php p($_['slogan']) ?>" maxlength="500" />
 		</label>
 		<span data-setting="slogan" data-toggle="tooltip" data-original-title="<?php p($l->t('reset to default')); ?>" class="theme-undo icon icon-history"></span>
 	</p>
 	<p>
 		<label><span><?php p($l->t('Color')) ?></span>
-			<input id="theming-color" type="text" class="jscolor" value="<?php p($_['color']) ?>" />
+			<input id="theming-color" type="text" class="jscolor" maxlength="6" value="<?php p($_['color']) ?>" />
 		</label>
 		<span data-setting="color" data-toggle="tooltip" data-original-title="<?php p($l->t('reset to default')); ?>" class="theme-undo icon icon-history"></span>
 	</p>

diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php
@@ -36,34 +36,34 @@
 use Test\TestCase;
 
 class ThemingControllerTest extends TestCase {
-	/** @var IRequest */
+	/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
 	private $request;
-	/** @var IConfig */
+	/** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */
 	private $config;
-	/** @var Template */
+	/** @var Template|\PHPUnit_Framework_MockObject_MockObject */
 	private $template;
 	/** @var Util */
 	private $util;
 	/** @var \OCP\AppFramework\Utility\ITimeFactory */
 	private $timeFactory;
-	/** @var IL10N */
+	/** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */
 	private $l10n;
 	/** @var ThemingController */
 	private $themingController;
-	/** @var IRootFolder */
+	/** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */
 	private $rootFolder;
 
 	public function setUp() {
-		$this->request = $this->getMock('\\OCP\\IRequest');
-		$this->config = $this->getMock('\\OCP\\IConfig');
-		$this->template = $this->getMockBuilder('\\OCA\\Theming\\Template')
+		$this->request = $this->getMockBuilder('OCP\IRequest')->getMock();
+		$this->config = $this->getMockBuilder('OCP\IConfig')->getMock();
+		$this->template = $this->getMockBuilder('OCA\Theming\Template')
 			->disableOriginalConstructor()->getMock();
 		$this->util = new Util();
 		$this->timeFactory = $this->getMockBuilder('OCP\AppFramework\Utility\ITimeFactory')
 			->disableOriginalConstructor()
 			->getMock();
-		$this->l10n = $this->getMock('\\OCP\\IL10N');
-		$this->rootFolder = $this->getMock('\\OCP\\Files\\IRootFolder');
+		$this->l10n = $this->getMockBuilder('OCP\IL10N')->getMock();
+		$this->rootFolder = $this->getMockBuilder('OCP\Files\IRootFolder')->getMock();
 
 		$this->timeFactory->expects($this->any())
 			->method('getTime')
@@ -83,27 +83,48 @@ public function setUp() {
 		return parent::setUp();
 	}
 
-	public function testUpdateStylesheet() {
+	public function dataUpdateStylesheet() {
+		return [
+			['name', str_repeat('a', 250), 'success', 'Saved'],
+			['name', str_repeat('a', 251), 'error', 'The given name is too long'],
+			['url', str_repeat('a', 500), 'success', 'Saved'],
+			['url', str_repeat('a', 501), 'error', 'The given web address is too long'],
+			['slogan', str_repeat('a', 500), 'success', 'Saved'],
+			['slogan', str_repeat('a', 501), 'error', 'The given slogan is too long'],
+			['color', '#0082c9', 'success', 'Saved'],
+			['color', '#0082C9', 'success', 'Saved'],
+			['color', '0082C9', 'error', 'The given color is invalid'],
+			['color', '#0082Z9', 'error', 'The given color is invalid'],
+			['color', 'Nextcloud', 'error', 'The given color is invalid'],
+		];
+	}
+
+	/**
+	 * @dataProvider dataUpdateStylesheet
+	 *
+	 * @param string $setting
+	 * @param string $value
+	 * @param string $status
+	 * @param string $message
+	 */
+	public function testUpdateStylesheet($setting, $value, $status, $message) {
 		$this->template
-			->expects($this->once())
+			->expects($status === 'success' ? $this->once() : $this->never())
 			->method('set')
-			->with('MySetting', 'MyValue');
+			->with($setting, $value);
 		$this->l10n
 			->expects($this->once())
 			->method('t')
-			->with('Saved')
-			->willReturn('Saved');
+			->with($message)
+			->willReturn($message);
 
-		$expected = new DataResponse(
-			[
-				'data' =>
-					[
-						'message' => 'Saved',
-					],
-				'status' => 'success'
-			]
-		);
-		$this->assertEquals($expected, $this->themingController->updateStylesheet('MySetting', 'MyValue'));
+		$expected = new DataResponse([
+			'data' => [
+				'message' => $message,
+			],
+			'status' => $status,
+		]);
+		$this->assertEquals($expected, $this->themingController->updateStylesheet($setting, $value));
 	}
 
 	public function testUpdateLogoNoData() {
@@ -665,5 +686,4 @@ public function testGetStylesheetWithAllCombinedInverted() {
 		$expected->addHeader('Expires', date(\DateTime::RFC2822, 123));
 		@$this->assertEquals($expected, $this->themingController->getStylesheet());
 	}
-
 }