Skip to content

Commit 8b46771

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #50487 from nextcloud/backport/50293/stable28 I
[stable29] fix(theming): Harden admin theming settings
2 parents 901c645 + e4f9e0c commit 8b46771

File tree

6 files changed

+202
-13
lines changed

6 files changed

+202
-13
lines changed

‎apps/theming/lib/Controller/ThemingController.php

+5-3
Original file line numberDiff line numberDiff line change
@@ -223,11 +223,13 @@ public function updateAppMenu($setting, $value) {
223223
}
224224

225225
/**
226-
* Check that a string is a valid http/https url
226+
* Check that a string is a valid http/https url.
227+
* Also validates that there is no way for XSS through HTML
227228
*/
228229
private function isValidUrl(string $url): bool {
229-
return ((str_starts_with($url, 'http://') || str_starts_with($url, 'https://')) &&
230-
filter_var($url, FILTER_VALIDATE_URL) !== false);
230+
return ((str_starts_with($url, 'http://') || str_starts_with($url, 'https://'))
231+
&& filter_var($url, FILTER_VALIDATE_URL) !== false)
232+
&& !str_contains($url, '"');
231233
}
232234

233235
/**

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

+35-4
Original file line numberDiff line numberDiff line change
@@ -38,25 +38,56 @@ export default {
3838

3939
data() {
4040
return {
41+
/** @type {string|boolean} */
4142
localValue: this.value,
4243
}
4344
},
4445

46+
computed: {
47+
valueToPost() {
48+
if (this.type === 'url') {
49+
// if this is already encoded just make sure there is no doublequote (HTML XSS)
50+
// otherwise simply URL encode
51+
return this.isUrlEncoded(this.localValue)
52+
? this.localValue.replaceAll('"', '%22')
53+
: encodeURI(this.localValue)
54+
}
55+
// Convert boolean to string as server expects string value
56+
if (typeof this.localValue === 'boolean') {
57+
return this.localValue ? 'yes' : 'no'
58+
}
59+
return this.localValue
60+
},
61+
},
62+
4563
methods: {
64+
/**
65+
* Check if URL is percent-encoded
66+
* @param {string} url The URL to check
67+
* @return {boolean}
68+
*/
69+
isUrlEncoded(url) {
70+
try {
71+
return decodeURI(url) !== url
72+
} catch {
73+
return false
74+
}
75+
},
76+
4677
async save() {
4778
this.reset()
4879
const url = generateUrl('/apps/theming/ajax/updateStylesheet')
49-
// Convert boolean to string as server expects string value
50-
const valueToPost = this.localValue === true ? 'yes' : this.localValue === false ? 'no' : this.localValue
80+
5181
try {
5282
await axios.post(url, {
5383
setting: this.name,
54-
value: valueToPost,
84+
value: this.valueToPost,
5585
})
5686
this.$emit('update:value', this.localValue)
5787
this.handleSuccess()
5888
} catch (e) {
59-
this.errorMessage = e.response.data.data?.message
89+
console.error('Failed to save changes', e)
90+
this.errorMessage = e.response?.data.data?.message
6091
}
6192
},
6293

‎apps/theming/tests/Controller/ThemingControllerTest.php

+16-3
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,24 @@ public function testUpdateStylesheetSuccess($setting, $value, $message) {
158158
}
159159

160160
public function dataUpdateStylesheetError() {
161+
$urls = [
162+
'url' => 'web address',
163+
'imprintUrl' => 'legal notice address',
164+
'privacyUrl' => 'privacy policy address',
165+
];
166+
167+
$urlTests = [];
168+
foreach ($urls as $urlKey => $urlName) {
169+
// Check length limit
170+
$urlTests[] = [$urlKey, 'http://example.com/' . str_repeat('a', 501), "The given {$urlName} is too long"];
171+
// Check potential evil javascript
172+
$urlTests[] = [$urlKey, 'javascript:alert(1)', "The given {$urlName} is not a valid URL"];
173+
// Check XSS
174+
$urlTests[] = [$urlKey, 'https://example.com/"><script/src="alert(\'1\')"><a/href/="', "The given {$urlName} is not a valid URL"];
175+
}
176+
161177
return [
162178
['name', str_repeat('a', 251), 'The given name is too long'],
163-
['url', 'http://example.com/' . str_repeat('a', 501), 'The given web address is too long'],
164-
['url', str_repeat('a', 501), 'The given web address is not a valid URL'],
165-
['url', 'javascript:alert(1)', 'The given web address is not a valid URL'],
166179
['slogan', str_repeat('a', 501), 'The given slogan is too long'],
167180
['color', '0082C9', 'The given color is invalid'],
168181
['color', '#0082Z9', 'The given color is invalid'],

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

+143
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
/*!
2+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
3+
* SPDX-License-Identifier: AGPL-3.0-or-later
4+
*/
5+
import { User } from '@nextcloud/cypress'
6+
7+
const admin = new User('admin', 'admin')
8+
9+
describe('Admin theming: Setting custom project URLs', function() {
10+
this.beforeEach(() => {
11+
// Just in case previous test failed
12+
cy.resetAdminTheming()
13+
cy.login(admin)
14+
cy.visit('/settings/admin/theming')
15+
cy.intercept('POST', '**/apps/theming/ajax/updateStylesheet').as('updateTheming')
16+
})
17+
18+
it('Setting the web link', () => {
19+
cy.findByRole('textbox', { name: /web link/i })
20+
.and('have.attr', 'type', 'url')
21+
.as('input')
22+
.scrollIntoView()
23+
cy.get('@input')
24+
.should('be.visible')
25+
.type('{selectAll}http://example.com/path?query#fragment{enter}')
26+
27+
cy.wait('@updateTheming')
28+
29+
cy.logout()
30+
31+
cy.visit('/')
32+
cy.contains('a', 'Nextcloud')
33+
.should('be.visible')
34+
.and('have.attr', 'href', 'http://example.com/path?query#fragment')
35+
})
36+
37+
it('Setting the legal notice link', () => {
38+
cy.findByRole('textbox', { name: /legal notice link/i })
39+
.should('exist')
40+
.and('have.attr', 'type', 'url')
41+
.as('input')
42+
.scrollIntoView()
43+
cy.get('@input')
44+
.type('http://example.com/path?query#fragment{enter}')
45+
46+
cy.wait('@updateTheming')
47+
48+
cy.logout()
49+
50+
cy.visit('/')
51+
cy.contains('a', /legal notice/i)
52+
.should('be.visible')
53+
.and('have.attr', 'href', 'http://example.com/path?query#fragment')
54+
})
55+
56+
it('Setting the privacy policy link', () => {
57+
cy.findByRole('textbox', { name: /privacy policy link/i })
58+
.should('exist')
59+
.as('input')
60+
.scrollIntoView()
61+
cy.get('@input')
62+
.should('have.attr', 'type', 'url')
63+
.type('http://privacy.local/path?query#fragment{enter}')
64+
65+
cy.wait('@updateTheming')
66+
67+
cy.logout()
68+
69+
cy.visit('/')
70+
cy.contains('a', /privacy policy/i)
71+
.should('be.visible')
72+
.and('have.attr', 'href', 'http://privacy.local/path?query#fragment')
73+
})
74+
75+
})
76+
77+
describe('Admin theming: Web link corner cases', function() {
78+
this.beforeEach(() => {
79+
// Just in case previous test failed
80+
cy.resetAdminTheming()
81+
cy.login(admin)
82+
cy.visit('/settings/admin/theming')
83+
cy.intercept('POST', '**/apps/theming/ajax/updateStylesheet').as('updateTheming')
84+
})
85+
86+
it('Already URL encoded', () => {
87+
cy.findByRole('textbox', { name: /web link/i })
88+
.and('have.attr', 'type', 'url')
89+
.as('input')
90+
.scrollIntoView()
91+
cy.get('@input')
92+
.should('be.visible')
93+
.type('{selectAll}http://example.com/%22path%20with%20space%22{enter}')
94+
95+
cy.wait('@updateTheming')
96+
97+
cy.logout()
98+
99+
cy.visit('/')
100+
cy.contains('a', 'Nextcloud')
101+
.should('be.visible')
102+
.and('have.attr', 'href', 'http://example.com/%22path%20with%20space%22')
103+
})
104+
105+
it('URL with double quotes', () => {
106+
cy.findByRole('textbox', { name: /web link/i })
107+
.and('have.attr', 'type', 'url')
108+
.as('input')
109+
.scrollIntoView()
110+
cy.get('@input')
111+
.should('be.visible')
112+
.type('{selectAll}http://example.com/"path"{enter}')
113+
114+
cy.wait('@updateTheming')
115+
116+
cy.logout()
117+
118+
cy.visit('/')
119+
cy.contains('a', 'Nextcloud')
120+
.should('be.visible')
121+
.and('have.attr', 'href', 'http://example.com/%22path%22')
122+
})
123+
124+
it('URL with double quotes and already encoded', () => {
125+
cy.findByRole('textbox', { name: /web link/i })
126+
.and('have.attr', 'type', 'url')
127+
.as('input')
128+
.scrollIntoView()
129+
cy.get('@input')
130+
.should('be.visible')
131+
.type('{selectAll}http://example.com/"the%20path"{enter}')
132+
133+
cy.wait('@updateTheming')
134+
135+
cy.logout()
136+
137+
cy.visit('/')
138+
cy.contains('a', 'Nextcloud')
139+
.should('be.visible')
140+
.and('have.attr', 'href', 'http://example.com/%22the%20path%22')
141+
})
142+
143+
})

‎dist/theming-admin-theming.js

+2-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎dist/theming-admin-theming.js.map

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)