sascha
/
nextcloud-server
mirror of https://github.com/nextcloud/server.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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

fix(theming): Harden admin theming settings
pull/47889/head
Andy Scherzinger 2025-01-27 18:55:36 +07:00 committed by GitHub
parent c483a84e9b 8aa3a153ba
commit 6dc83b96c4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 204 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
apps/theming/lib/Controller/ThemingController.php
Unescape Escape View File

@ -194,11 +194,13 @@ class ThemingController extends Controller {
}
/**
* 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, '"');
}
/**

39
apps/theming/src/mixins/admin/TextValueMixin.js
Unescape Escape View File

@ -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
}
},

25
apps/theming/tests/Controller/ThemingControllerTest.php
Unescape Escape View File

@ -125,11 +125,24 @@ class ThemingControllerTest extends TestCase {
}
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'],
@ -137,10 +150,8 @@ class ThemingControllerTest extends TestCase {
['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,
];
}

143
cypress/e2e/theming/admin-settings_urls.cy.ts
Unescape Escape View File

@ -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')
})
})

4
dist/theming-admin-theming.js vendored
View File

File diff suppressed because one or more lines are too long

2
dist/theming-admin-theming.js.map vendored
View File

File diff suppressed because one or more lines are too long