Merge pull request #50700 from nextcloud/backport/50678/stable31

[stable31] fix(AccountProperty): better validation of twitter and fediverse handles

apps/provisioning_api/lib/Controller/UsersController.php

@@ -894,7 +894,7 @@ public function editUserMultiValue(
 	 */
 	#[PasswordConfirmationRequired]
 	#[NoAdminRequired]
-	#[UserRateLimit(limit: 50, period: 60)]
+	#[UserRateLimit(limit: 50, period: 600)]
 	public function editUser(string $userId, string $key, string $value): DataResponse {
 		$currentLoggedInUser = $this->userSession->getUser();
 

apps/settings/lib/Controller/UsersController.php

@@ -32,6 +32,7 @@
 use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
 use OCP\AppFramework\Http\Attribute\OpenAPI;
 use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
+use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\TemplateResponse;
@@ -314,6 +315,7 @@ protected function canAdminChangeUserPasswords(): bool {
 	 */
 	#[NoAdminRequired]
 	#[PasswordConfirmationRequired]
+	#[UserRateLimit(limit: 5, period: 60)]
 	public function setUserSettings(?string $avatarScope = null,
 		?string $displayname = null,
 		?string $displaynameScope = null,

apps/settings/src/components/PersonalInfo/FediverseSection.vue

@@ -4,30 +4,40 @@
 -->
 
 <template>
-	<AccountPropertySection v-bind.sync="fediverse"
+	<AccountPropertySection v-bind.sync="value"
+		:readable="readable"
+		:on-validate="onValidate"
 		:placeholder="t('settings', 'Your handle')" />
 </template>
 
-<script>
+<script setup lang="ts">
+import type { AccountProperties } from '../../constants/AccountPropertyConstants.js'
 import { loadState } from '@nextcloud/initial-state'
-
-import AccountPropertySection from './shared/AccountPropertySection.vue'
-
+import { t } from '@nextcloud/l10n'
+import { ref } from 'vue'
 import { NAME_READABLE_ENUM } from '../../constants/AccountPropertyConstants.js'
 
-const { fediverse } = loadState('settings', 'personalInfoParameters', {})
-
-export default {
-	name: 'FediverseSection',
-
-	components: {
-		AccountPropertySection,
-	},
+import AccountPropertySection from './shared/AccountPropertySection.vue'
 
-	data() {
-		return {
-			fediverse: { ...fediverse, readable: NAME_READABLE_ENUM[fediverse.name] },
-		}
-	},
+const { fediverse } = loadState<AccountProperties>('settings', 'personalInfoParameters', {})
+
+const value = ref({ ...fediverse })
+const readable = NAME_READABLE_ENUM[fediverse.name]
+
+/**
+ * Validate a fediverse handle
+ * @param text The potential fediverse handle
+ */
+function onValidate(text: string): boolean {
+	const result = text.match(/^@?([^@/]+)@([^@/]+)$/)
+	if (result === null) {
+		return false
+	}
+
+	try {
+		return URL.parse(`https://${result[2]}/`) !== null
+	} catch {
+		return false
+	}
 }
 </script>

apps/settings/src/components/PersonalInfo/TwitterSection.vue

@@ -4,30 +4,31 @@
 -->
 
 <template>
-	<AccountPropertySection v-bind.sync="twitter"
+	<AccountPropertySection v-bind.sync="value"
+		:readable="readable"
+		:on-validate="onValidate"
 		:placeholder="t('settings', 'Your X (formerly Twitter) handle')" />
 </template>
 
-<script>
-import { loadState } from '@nextcloud/initial-state'
+<script setup lang="ts">
+import type { AccountProperties } from '../../constants/AccountPropertyConstants.js'
 
+import { loadState } from '@nextcloud/initial-state'
+import { t } from '@nextcloud/l10n'
+import { ref } from 'vue'
+import { NAME_READABLE_ENUM } from '../../constants/AccountPropertyConstants.ts'
 import AccountPropertySection from './shared/AccountPropertySection.vue'
 
-import { NAME_READABLE_ENUM } from '../../constants/AccountPropertyConstants.js'
-
-const { twitter } = loadState('settings', 'personalInfoParameters', {})
-
-export default {
-	name: 'TwitterSection',
+const { twitter } = loadState<AccountProperties>('settings', 'personalInfoParameters', {})
 
-	components: {
-		AccountPropertySection,
-	},
+const value = ref({ ...twitter })
+const readable = NAME_READABLE_ENUM[twitter.name]
 
-	data() {
-		return {
-			twitter: { ...twitter, readable: NAME_READABLE_ENUM[twitter.name] },
-		}
-	},
+/**
+ * Validate that the text might be a twitter handle
+ * @param text The potential twitter handle
+ */
+function onValidate(text: string): boolean {
+	return text.match(/^@?([a-zA-Z0-9_]{2,15})$/) !== null
 }
 </script>

apps/settings/src/constants/AccountPropertyConstants.js → apps/settings/src/constants/AccountPropertyConstants.ts

@@ -114,12 +114,12 @@ export const ACCOUNT_SETTING_PROPERTY_READABLE_ENUM = Object.freeze({
 })
 
 /** Enum of scopes */
-export const SCOPE_ENUM = Object.freeze({
-	PRIVATE: 'v2-private',
-	LOCAL: 'v2-local',
-	FEDERATED: 'v2-federated',
-	PUBLISHED: 'v2-published',
-})
+export enum SCOPE_ENUM {
+	PRIVATE = 'v2-private',
+	LOCAL = 'v2-local',
+	FEDERATED = 'v2-federated',
+	PUBLISHED = 'v2-published',
+}
 
 /** Enum of readable account properties to supported scopes */
 export const PROPERTY_READABLE_SUPPORTED_SCOPES_ENUM = Object.freeze({
@@ -193,11 +193,11 @@ export const SCOPE_PROPERTY_ENUM = Object.freeze({
 export const DEFAULT_ADDITIONAL_EMAIL_SCOPE = SCOPE_ENUM.LOCAL
 
 /** Enum of verification constants, according to IAccountManager */
-export const VERIFICATION_ENUM = Object.freeze({
-	NOT_VERIFIED: 0,
-	VERIFICATION_IN_PROGRESS: 1,
-	VERIFIED: 2,
-})
+export enum VERIFICATION_ENUM {
+	NOT_VERIFIED = 0,
+	VERIFICATION_IN_PROGRESS = 1,
+	VERIFIED = 2,
+}
 
 /**
  * Email validation regex
@@ -206,3 +206,12 @@ export const VERIFICATION_ENUM = Object.freeze({
  */
 // eslint-disable-next-line no-control-regex
 export const VALIDATE_EMAIL_REGEX = /^(?!(?:(?:\x22?\x5C[\x00-\x7E]\x22?)|(?:\x22?[^\x5C\x22]\x22?)){255,})(?!(?:(?:\x22?\x5C[\x00-\x7E]\x22?)|(?:\x22?[^\x5C\x22]\x22?)){65,}@)(?:(?:[\x21\x23-\x27\x2A\x2B\x2D\x2F-\x39\x3D\x3F\x5E-\x7E]+)|(?:\x22(?:[\x01-\x08\x0B\x0C\x0E-\x1F\x21\x23-\x5B\x5D-\x7F]|(?:\x5C[\x00-\x7F]))*\x22))(?:\.(?:(?:[\x21\x23-\x27\x2A\x2B\x2D\x2F-\x39\x3D\x3F\x5E-\x7E]+)|(?:\x22(?:[\x01-\x08\x0B\x0C\x0E-\x1F\x21\x23-\x5B\x5D-\x7F]|(?:\x5C[\x00-\x7F]))*\x22)))*@(?:(?:(?!.*[^.]{64,})(?:(?:(?:xn--)?[a-z0-9]+(?:-+[a-z0-9]+)*\.){1,126}){1,}(?:(?:[a-z][a-z0-9]*)|(?:(?:xn--)[a-z0-9]+))(?:-+[a-z0-9]+)*)|(?:\[(?:(?:IPv6:(?:(?:[a-f0-9]{1,4}(?::[a-f0-9]{1,4}){7})|(?:(?!(?:.*[a-f0-9][:\]]){7,})(?:[a-f0-9]{1,4}(?::[a-f0-9]{1,4}){0,5})?::(?:[a-f0-9]{1,4}(?::[a-f0-9]{1,4}){0,5})?)))|(?:(?:IPv6:(?:(?:[a-f0-9]{1,4}(?::[a-f0-9]{1,4}){5}:)|(?:(?!(?:.*[a-f0-9]:){5,})(?:[a-f0-9]{1,4}(?::[a-f0-9]{1,4}){0,3})?::(?:[a-f0-9]{1,4}(?::[a-f0-9]{1,4}){0,3}:)?)))?(?:(?:25[0-5])|(?:2[0-4][0-9])|(?:1[0-9]{2})|(?:[1-9]?[0-9]))(?:\.(?:(?:25[0-5])|(?:2[0-4][0-9])|(?:1[0-9]{2})|(?:[1-9]?[0-9]))){3}))\]))$/i
+
+export interface IAccountProperty {
+	name: string
+	value: string
+	scope: SCOPE_ENUM
+	verified: VERIFICATION_ENUM
+}
+
+export type AccountProperties = Record<(typeof ACCOUNT_PROPERTY_ENUM)[keyof (typeof ACCOUNT_PROPERTY_ENUM)], IAccountProperty>

apps/settings/src/service/PersonalInfo/EmailService.js

@@ -8,7 +8,7 @@ import { generateOcsUrl } from '@nextcloud/router'
 import { confirmPassword } from '@nextcloud/password-confirmation'
 import axios from '@nextcloud/axios'
 
-import { ACCOUNT_PROPERTY_ENUM, SCOPE_SUFFIX } from '../../constants/AccountPropertyConstants.js'
+import { ACCOUNT_PROPERTY_ENUM, SCOPE_SUFFIX } from '../../constants/AccountPropertyConstants.ts'
 
 import '@nextcloud/password-confirmation/dist/style.css'
 

apps/settings/src/service/PersonalInfo/PersonalInfoService.js

@@ -8,7 +8,7 @@ import { generateOcsUrl } from '@nextcloud/router'
 import { confirmPassword } from '@nextcloud/password-confirmation'
 import axios from '@nextcloud/axios'
 
-import { SCOPE_SUFFIX } from '../../constants/AccountPropertyConstants.js'
+import { SCOPE_SUFFIX } from '../../constants/AccountPropertyConstants.ts'
 
 import '@nextcloud/password-confirmation/dist/style.css'
 

apps/settings/src/utils/validate.js

@@ -9,7 +9,7 @@
  * TODO add nice validation errors for Profile page settings modal
  */
 
-import { VALIDATE_EMAIL_REGEX } from '../constants/AccountPropertyConstants.js'
+import { VALIDATE_EMAIL_REGEX } from '../constants/AccountPropertyConstants.ts'
 
 /**
  * Validate the email input

apps/settings/tests/UserMigration/AccountMigratorTest.php

@@ -12,6 +12,7 @@
 use OCP\Accounts\IAccountManager;
 use OCP\AppFramework\App;
 use OCP\IAvatarManager;
+use OCP\IConfig;
 use OCP\IUserManager;
 use OCP\UserMigration\IExportDestination;
 use OCP\UserMigration\IImportSource;
@@ -50,8 +51,11 @@ class AccountMigratorTest extends TestCase {
 	private const REGEX_CONFIG_FILE = '/^' . Application::APP_ID . '\/' . '[a-z]+\.json' . '$/';
 
 	protected function setUp(): void {
+		parent::setUp();
+
 		$app = new App(Application::APP_ID);
 		$container = $app->getContainer();
+		$container->get(IConfig::class)->setSystemValue('has_internet_connection', false);
 
 		$this->userManager = $container->get(IUserManager::class);
 		$this->avatarManager = $container->get(IAvatarManager::class);
@@ -62,6 +66,11 @@ protected function setUp(): void {
 		$this->output = $this->createMock(OutputInterface::class);
 	}
 
+	protected function tearDown(): void {
+		\OCP\Server::get(IConfig::class)->setSystemValue('has_internet_connection', true);
+		parent::tearDown();
+	}
+
 	public function dataImportExportAccount(): array {
 		return array_map(
 			function (string $filename) {

apps/settings/tests/UserMigration/assets/account-complex.json

@@ -1 +1 @@
-{"displayname":{"name":"displayname","value":"Steve Smith","scope":"v2-local","verified":"0","verificationData":""},"address":{"name":"address","value":"123 Water St","scope":"v2-local","verified":"0","verificationData":""},"website":{"name":"website","value":"https://example.org","scope":"v2-local","verified":"0","verificationData":""},"email":{"name":"email","value":"[email protected]","scope":"v2-federated","verified":"1","verificationData":""},"avatar":{"name":"avatar","value":"","scope":"v2-local","verified":"0","verificationData":""},"phone":{"name":"phone","value":"+12178515387","scope":"v2-private","verified":"0","verificationData":""},"twitter":{"name":"twitter","value":"steve","scope":"v2-federated","verified":"0","verificationData":""},"fediverse":{"name":"fediverse","value":"@[email protected]","scope":"v2-federated","verified":"0","verificationData":""},"organisation":{"name":"organisation","value":"Mytery Machine","scope":"v2-private","verified":"0","verificationData":""},"role":{"name":"role","value":"Manager","scope":"v2-private","verified":"0","verificationData":""},"headline":{"name":"headline","value":"I am Steve","scope":"v2-local","verified":"0","verificationData":""},"biography":{"name":"biography","value":"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Mauris porttitor ullamcorper dictum. Sed fermentum ut ligula scelerisque semper. Aliquam interdum convallis tellus eu dapibus. Integer in justo sollicitudin, hendrerit ligula sit amet, blandit sem.\n\nSuspendisse consectetur ultrices accumsan. Quisque sagittis bibendum lectus ut placerat. Mauris tincidunt ornare neque, et pulvinar tortor porttitor eu.","scope":"v2-local","verified":"0","verificationData":""},"birthdate":{"name":"birthdate","value":"","scope":"v2-local","verified":"0","verificationData":""},"profile_enabled":{"name":"profile_enabled","value":"1","scope":"v2-local","verified":"0","verificationData":""},"pronouns":{"name":"pronouns","value":"they/them","scope":"v2-local","verified":"0","verificationData":""},"additional_mail":[{"name":"additional_mail","value":"[email protected]","scope":"v2-published","verified":"0","verificationData":""},{"name":"additional_mail","value":"[email protected]","scope":"v2-local","verified":"0","verificationData":""}]}
+{"displayname":{"name":"displayname","value":"Steve Smith","scope":"v2-local","verified":"0","verificationData":""},"address":{"name":"address","value":"123 Water St","scope":"v2-local","verified":"0","verificationData":""},"website":{"name":"website","value":"https://example.org","scope":"v2-local","verified":"0","verificationData":""},"email":{"name":"email","value":"[email protected]","scope":"v2-federated","verified":"1","verificationData":""},"avatar":{"name":"avatar","value":"","scope":"v2-local","verified":"0","verificationData":""},"phone":{"name":"phone","value":"+12178515387","scope":"v2-private","verified":"0","verificationData":""},"twitter":{"name":"twitter","value":"steve","scope":"v2-federated","verified":"0","verificationData":""},"fediverse":{"name":"fediverse","value":"[email protected]","scope":"v2-federated","verified":"0","verificationData":""},"organisation":{"name":"organisation","value":"Mytery Machine","scope":"v2-private","verified":"0","verificationData":""},"role":{"name":"role","value":"Manager","scope":"v2-private","verified":"0","verificationData":""},"headline":{"name":"headline","value":"I am Steve","scope":"v2-local","verified":"0","verificationData":""},"biography":{"name":"biography","value":"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Mauris porttitor ullamcorper dictum. Sed fermentum ut ligula scelerisque semper. Aliquam interdum convallis tellus eu dapibus. Integer in justo sollicitudin, hendrerit ligula sit amet, blandit sem.\n\nSuspendisse consectetur ultrices accumsan. Quisque sagittis bibendum lectus ut placerat. Mauris tincidunt ornare neque, et pulvinar tortor porttitor eu.","scope":"v2-local","verified":"0","verificationData":""},"birthdate":{"name":"birthdate","value":"","scope":"v2-local","verified":"0","verificationData":""},"profile_enabled":{"name":"profile_enabled","value":"1","scope":"v2-local","verified":"0","verificationData":""},"pronouns":{"name":"pronouns","value":"they/them","scope":"v2-local","verified":"0","verificationData":""},"additional_mail":[{"name":"additional_mail","value":"[email protected]","scope":"v2-published","verified":"0","verificationData":""},{"name":"additional_mail","value":"[email protected]","scope":"v2-local","verified":"0","verificationData":""}]}

build/integration/features/bootstrap/BasicStructure.php

@@ -121,7 +121,11 @@ public function sendingTo($verb, $url) {
 	 * @return string
 	 */
 	public function getOCSResponse($response) {
-		return simplexml_load_string($response->getBody())->meta[0]->statuscode;
+		$body = simplexml_load_string((string)$response->getBody());
+		if ($body === false) {
+			throw new \RuntimeException('Could not parse OCS response, body is not valid XML');
+		}
+		return $body->meta[0]->statuscode;
 	}
 
 	/**

build/integration/features/bootstrap/FeatureContext.php

@@ -13,9 +13,16 @@
  * Features context.
  */
 class FeatureContext implements Context, SnippetAcceptingContext {
+	use AppConfiguration;
 	use ContactsMenu;
 	use ExternalStorage;
 	use Search;
 	use WebDav;
 	use Trashbin;
+
+	protected function resetAppConfigs(): void {
+		$this->deleteServerConfig('bruteForce', 'whitelist_0');
+		$this->deleteServerConfig('bruteForce', 'whitelist_1');
+		$this->deleteServerConfig('bruteforcesettings', 'apply_allowlist_to_ratelimit');
+	}
 }

build/integration/features/provisioning-v1.feature

@@ -4,6 +4,9 @@
 Feature: provisioning
   Background:
     Given using api version "1"
+    Given parameter "whitelist_0" of app "bruteForce" is set to "127.0.0.1"
+    Given parameter "whitelist_1" of app "bruteForce" is set to "::1"
+    Given parameter "apply_allowlist_to_ratelimit" of app "bruteforcesettings" is set to "true"
 
   Scenario: Getting an not existing user
     Given As an "admin"
@@ -604,6 +607,7 @@ Feature: provisioning
       | settings |
       | sharebymail |
       | systemtags |
+      | testing |
       | theming |
       | twofactor_backupcodes |
       | updatenotification |
@@ -629,6 +633,7 @@ Feature: provisioning
     And the HTTP status code should be "200"
 
   Scenario: enable an app
+    Given invoking occ with "app:disable testing"
     Given As an "admin"
     And app "testing" is disabled
     When sending "POST" to "/cloud/apps/testing"
@@ -643,12 +648,14 @@ Feature: provisioning
     And the HTTP status code should be "200"
 
   Scenario: disable an app
+    Given invoking occ with "app:enable testing"
     Given As an "admin"
     And app "testing" is enabled
     When sending "DELETE" to "/cloud/apps/testing"
     Then the OCS status code should be "100"
     And the HTTP status code should be "200"
     And app "testing" is disabled
+    Given invoking occ with "app:enable testing"
 
   Scenario: disable an user
     Given As an "admin"

build/psalm-baseline.xml

@@ -2277,6 +2277,14 @@
       <code><![CDATA[?IImage]]></code>
     </InvalidReturnType>
   </file>
+  <file src="lib/private/Profile/Actions/FediverseAction.php">
+    <NoValue>
+      <code><![CDATA[$instance]]></code>
+    </NoValue>
+    <RedundantCondition>
+      <code><![CDATA[$instance === '']]></code>
+    </RedundantCondition>
+  </file>
   <file src="lib/private/RedisFactory.php">
     <InvalidArgument>
       <code><![CDATA[\RedisCluster::OPT_SLAVE_FAILOVER]]></code>

cypress/e2e/settings/personal-info.cy.ts

@@ -98,13 +98,19 @@ const checkSettingsVisibility = (property: string, defaultVisibility: Visibility
 	}) */
 }
 
-const genericProperties = ['Location', 'X (formerly Twitter)', 'Fediverse']
+const genericProperties = [
+	['Location', 'Berlin'],
+	['X (formerly Twitter)', 'nextclouders'],
+	['Fediverse', '[email protected]'],
+]
 const nonfederatedProperties = ['Organisation', 'Role', 'Headline', 'About']
 
 describe('Settings: Change personal information', { testIsolation: true }, () => {
 	let snapshot: string = ''
 
 	before(() => {
+		// make sure the fediverse check does not do http requests
+		cy.runOccCommand('config:system:set has_internet_connection --value false')
 		// ensure we can set locale and language
 		cy.runOccCommand('config:system:delete force_language')
 		cy.runOccCommand('config:system:delete force_locale')
@@ -125,6 +131,8 @@ describe('Settings: Change personal information', { testIsolation: true }, () =>
 	})
 
 	after(() => {
+		cy.runOccCommand('config:system:delete has_internet_connection')
+
 		cy.runOccCommand('config:system:set force_language --value en')
 		cy.runOccCommand('config:system:set force_locale --value en_US')
 	})
@@ -349,22 +357,21 @@ describe('Settings: Change personal information', { testIsolation: true }, () =>
 	})
 
 	// Check generic properties that allow any visibility and any value
-	genericProperties.forEach((property) => {
+	genericProperties.forEach(([property, value]) => {
 		it(`Can set ${property} and change its visibility`, () => {
-			const uniqueValue = `${property.toUpperCase()} ${property.toLowerCase()}`
 			cy.contains('label', property).scrollIntoView()
-			inputForLabel(property).type(uniqueValue)
+			inputForLabel(property).type(value)
 			handlePasswordConfirmation(user.password)
 
 			cy.wait('@submitSetting')
 			cy.reload()
-			inputForLabel(property).should('have.value', uniqueValue)
+			inputForLabel(property).should('have.value', value)
 
 			checkSettingsVisibility(property)
 
 			// check it is visible on the profile
 			cy.visit(`/u/${user.userId}`)
-			cy.contains(uniqueValue).should('be.visible')
+			cy.contains(value).should('be.visible')
 		})
 	})