Merge pull request #50293 from nextcloud/fix/harden-admin-settings

fix(theming): Harden admin theming settings

apps/theming/lib/Controller/ThemingController.php

@@ -194,11 +194,13 @@ public function updateAppMenu($setting, $value) {
 	}
 
 	/**
-	 * 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, '"');
 	}
 
 	/**

apps/theming/src/mixins/admin/TextValueMixin.js

@@ -21,25 +21,56 @@ export default {
 
 	data() {
 		return {
+			/** @type {string|boolean} */
 			localValue: this.value,
 		}
 	},
 
+	computed: {
+		valueToPost() {
+			if (this.type === 'url') {
+				// if this is already encoded just make sure there is no doublequote (HTML XSS)
+				// otherwise simply URL encode
+				return this.isUrlEncoded(this.localValue)
+					? this.localValue.replaceAll('"', '%22')
+					: encodeURI(this.localValue)
+			}
+			// Convert boolean to string as server expects string value
+			if (typeof this.localValue === 'boolean') {
+				return this.localValue ? 'yes' : 'no'
+			}
+			return this.localValue
+		},
+	},
+
 	methods: {
+		/**
+		 * Check if URL is percent-encoded
+		 * @param {string} url The URL to check
+		 * @return {boolean}
+		 */
+		isUrlEncoded(url) {
+			try {
+				return decodeURI(url) !== url
+			} catch {
+				return false
+			}
+		},
+
 		async save() {
 			this.reset()
 			const url = generateUrl('/apps/theming/ajax/updateStylesheet')
-			// Convert boolean to string as server expects string value
-			const valueToPost = this.localValue === true ? 'yes' : this.localValue === false ? 'no' : this.localValue
+
 			try {
 				await axios.post(url, {
 					setting: this.name,
-					value: valueToPost,
+					value: this.valueToPost,
 				})
 				this.$emit('update:value', this.localValue)
 				this.handleSuccess()
 			} catch (e) {
-				this.errorMessage = e.response.data.data?.message
+				console.error('Failed to save changes', e)
+				this.errorMessage = e.response?.data.data?.message
 			}
 		},
 

apps/theming/tests/Controller/ThemingControllerTest.php

@@ -125,22 +125,33 @@ public function testUpdateStylesheetSuccess($setting, $value, $message): void {
 	}
 
 	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'],
 			['primary_color', 'Nextcloud', 'The given color is invalid'],
 			['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,
 		];
 	}
 

cypress/e2e/theming/admin-settings_urls.cy.ts

@@ -0,0 +1,143 @@
+/*!
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+import { User } from '@nextcloud/cypress'
+
+const admin = new User('admin', 'admin')
+
+describe('Admin theming: Setting custom project URLs', function() {
+	this.beforeEach(() => {
+		// Just in case previous test failed
+		cy.resetAdminTheming()
+		cy.login(admin)
+		cy.visit('/settings/admin/theming')
+		cy.intercept('POST', '**/apps/theming/ajax/updateStylesheet').as('updateTheming')
+	})
+
+	it('Setting the web link', () => {
+		cy.findByRole('textbox', { name: /web link/i })
+			.and('have.attr', 'type', 'url')
+			.as('input')
+			.scrollIntoView()
+		cy.get('@input')
+			.should('be.visible')
+			.type('{selectAll}http://example.com/path?query#fragment{enter}')
+
+		cy.wait('@updateTheming')
+
+		cy.logout()
+
+		cy.visit('/')
+		cy.contains('a', 'Nextcloud')
+			.should('be.visible')
+			.and('have.attr', 'href', 'http://example.com/path?query#fragment')
+	})
+
+	it('Setting the legal notice link', () => {
+		cy.findByRole('textbox', { name: /legal notice link/i })
+			.should('exist')
+			.and('have.attr', 'type', 'url')
+			.as('input')
+			.scrollIntoView()
+		cy.get('@input')
+			.type('http://example.com/path?query#fragment{enter}')
+
+		cy.wait('@updateTheming')
+
+		cy.logout()
+
+		cy.visit('/')
+		cy.contains('a', /legal notice/i)
+			.should('be.visible')
+			.and('have.attr', 'href', 'http://example.com/path?query#fragment')
+	})
+
+	it('Setting the privacy policy link', () => {
+		cy.findByRole('textbox', { name: /privacy policy link/i })
+			.should('exist')
+			.as('input')
+			.scrollIntoView()
+		cy.get('@input')
+			.should('have.attr', 'type', 'url')
+			.type('http://privacy.local/path?query#fragment{enter}')
+
+		cy.wait('@updateTheming')
+
+		cy.logout()
+
+		cy.visit('/')
+		cy.contains('a', /privacy policy/i)
+			.should('be.visible')
+			.and('have.attr', 'href', 'http://privacy.local/path?query#fragment')
+	})
+
+})
+
+describe('Admin theming: Web link corner cases', function() {
+	this.beforeEach(() => {
+		// Just in case previous test failed
+		cy.resetAdminTheming()
+		cy.login(admin)
+		cy.visit('/settings/admin/theming')
+		cy.intercept('POST', '**/apps/theming/ajax/updateStylesheet').as('updateTheming')
+	})
+
+	it('Already URL encoded', () => {
+		cy.findByRole('textbox', { name: /web link/i })
+			.and('have.attr', 'type', 'url')
+			.as('input')
+			.scrollIntoView()
+		cy.get('@input')
+			.should('be.visible')
+			.type('{selectAll}http://example.com/%22path%20with%20space%22{enter}')
+
+		cy.wait('@updateTheming')
+
+		cy.logout()
+
+		cy.visit('/')
+		cy.contains('a', 'Nextcloud')
+			.should('be.visible')
+			.and('have.attr', 'href', 'http://example.com/%22path%20with%20space%22')
+	})
+
+	it('URL with double quotes', () => {
+		cy.findByRole('textbox', { name: /web link/i })
+			.and('have.attr', 'type', 'url')
+			.as('input')
+			.scrollIntoView()
+		cy.get('@input')
+			.should('be.visible')
+			.type('{selectAll}http://example.com/"path"{enter}')
+
+		cy.wait('@updateTheming')
+
+		cy.logout()
+
+		cy.visit('/')
+		cy.contains('a', 'Nextcloud')
+			.should('be.visible')
+			.and('have.attr', 'href', 'http://example.com/%22path%22')
+	})
+
+	it('URL with double quotes and already encoded', () => {
+		cy.findByRole('textbox', { name: /web link/i })
+			.and('have.attr', 'type', 'url')
+			.as('input')
+			.scrollIntoView()
+		cy.get('@input')
+			.should('be.visible')
+			.type('{selectAll}http://example.com/"the%20path"{enter}')
+
+		cy.wait('@updateTheming')
+
+		cy.logout()
+
+		cy.visit('/')
+		cy.contains('a', 'Nextcloud')
+			.should('be.visible')
+			.and('have.attr', 'href', 'http://example.com/%22the%20path%22')
+	})
+
+})