feat: validate password policy length on settings save

.changeset/empty-garlics-reply.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/core-typings': patch
+'@rocket.chat/meteor': patch
+---
+
+Fixes the password policy allowing a maximum length lower than the minimum length to be saved — a combination that made it impossible to set any valid password. The server now rejects such configurations when password policy settings are saved and shows an error explaining the constraint.

apps/meteor/server/api/v1/settings.ts

@@ -26,6 +26,7 @@ import _ from 'underscore';
 
 import { hasPermissionAsync } from '../../lib/authorization/hasPermission';
 import { notifyOnSettingChanged, notifyOnSettingChangedById } from '../../lib/notifyListener';
+import { SettingValidationError, validateSettingRules } from '../../lib/settingValidationRules';
 import { disableCustomScripts } from '../../lib/shared/disableCustomScripts';
 import { addOAuthServiceMethod } from '../../meteor-methods/auth/addOAuthService';
 import { SettingsEvents, settings } from '../../settings';
@@ -334,12 +335,7 @@ API.v1.post(
 		body: settingsUpdateBodySchema,
 		response: {
 			200: settingByIdPostResponseSchema,
-			400: ajv.compile({
-				type: 'object',
-				properties: { success: { type: 'boolean', enum: [false] } },
-				required: ['success'],
-				additionalProperties: true,
-			}),
+			400: validateBadRequestErrorResponse,
 			401: validateUnauthorizedErrorResponse,
 			403: validateForbiddenErrorResponse,
 		},
@@ -393,8 +389,18 @@ API.v1.post(
 		}
 
 		if (isSettingsUpdatePropDefault(bodyParams)) {
+			// TODO(next major): unify both validations into one function with a common API error response
 			checkSettingValueBounds(setting, bodyParams.value);
 
+			try {
+				validateSettingRules([{ _id, value: bodyParams.value }]);
+			} catch (error) {
+				if (error instanceof SettingValidationError) {
+					return API.v1.failure(error.message, 'error-setting-validation-failed');
+				}
+				throw error;
+			}
+
 			const { matchedCount } = await auditSettingOperation(Settings.updateValueNotHiddenById, _id, bodyParams.value);
 
 			if (!matchedCount) {
@@ -433,11 +439,18 @@ API.v1.post(
 		},
 	},
 	async function action() {
-		await saveSettingsBulk(this.userId, this.bodyParams.settings, {
-			username: this.user.username ?? '',
-			ip: this.requestIp ?? '',
-			useragent: this.request.headers.get('user-agent') ?? '',
-		});
+		try {
+			await saveSettingsBulk(this.userId, this.bodyParams.settings, {
+				username: this.user.username ?? '',
+				ip: this.requestIp ?? '',
+				useragent: this.request.headers.get('user-agent') ?? '',
+			});
+		} catch (error) {
+			if (error instanceof SettingValidationError) {
+				return API.v1.failure(error.message, 'error-setting-validation-failed');
+			}
+			throw error;
+		}
 
 		return API.v1.success();
 	},

apps/meteor/server/lib/settingValidationRules.ts

@@ -0,0 +1,128 @@
+import type { ISetting, SettingValidationRule } from '@rocket.chat/core-typings';
+import { Logger } from '@rocket.chat/logger';
+import { createPredicateFromFilter } from '@rocket.chat/mongo-adapter';
+import { isRecord } from '@rocket.chat/tools';
+
+import { settings } from '../settings';
+
+const logger = new Logger('SettingValidation');
+
+export class SettingValidationError extends Error {}
+
+const isAppliesWhenCondition = (value: unknown): value is { _id: ISetting['_id']; value: unknown } =>
+	isRecord(value) && typeof value._id === 'string' && 'value' in value;
+
+const isValidationRule = (rule: unknown): rule is SettingValidationRule =>
+	isRecord(rule) &&
+	isRecord(rule.query) &&
+	(rule.appliesWhen === undefined ||
+		isAppliesWhenCondition(rule.appliesWhen) ||
+		(Array.isArray(rule.appliesWhen) && rule.appliesWhen.every(isAppliesWhenCondition)));
+
+const isValidationRuleArray = (value: unknown): value is SettingValidationRule[] => Array.isArray(value) && value.every(isValidationRule);
+
+const parseValidationRules = (settingId: ISetting['_id'], validation: NonNullable<ISetting['validation']>): SettingValidationRule[] => {
+	if (typeof validation !== 'string') {
+		return validation;
+	}
+
+	let parsed: unknown;
+	try {
+		parsed = JSON.parse(validation);
+	} catch (err) {
+		logger.error({ msg: 'Failed to parse setting validation rules', settingId, err });
+		return [];
+	}
+
+	if (!isValidationRuleArray(parsed)) {
+		logger.error({ msg: 'Setting validation rules are not well-formed', settingId });
+		return [];
+	}
+
+	return parsed;
+};
+
+const isSettingReference = (value: unknown): value is { $setting: ISetting['_id'] } =>
+	isRecord(value) && typeof value.$setting === 'string';
+
+/**
+ * Evaluates a setting's validation rule against the value being saved. Returns `false` only when the rule's filter
+ * rejects the candidate. A rule that references a setting which does not exist is logged and treated as passing, so a
+ * broken declaration never blocks a save.
+ */
+const evaluateSettingValidationRule = (
+	settingId: ISetting['_id'],
+	rule: SettingValidationRule,
+	get: (id: ISetting['_id']) => unknown,
+): boolean => {
+	const references: Record<string, unknown> = {};
+
+	// true when `filter` matches the current value of the setting `id`
+	const matches = (filter: Record<string, unknown>, id: ISetting['_id']): boolean =>
+		createPredicateFromFilter<{ _id: ISetting['_id']; value: unknown }>(filter)({ _id: id, value: get(id) });
+
+	// replaces every `{ $setting: '<id>' }` in the filter with that setting's value,
+	// recording each in `references`
+	const resolveObject = (filter: Record<string, unknown>): Record<string, unknown> =>
+		Object.fromEntries(Object.entries(filter).map(([key, value]) => [key, resolveNode(value)]));
+
+	const resolveNode = (node: unknown): unknown => {
+		if (isSettingReference(node)) {
+			references[node.$setting] = get(node.$setting);
+			return references[node.$setting];
+		}
+		if (Array.isArray(node)) {
+			return node.map(resolveNode);
+		}
+		if (isRecord(node)) {
+			return resolveObject(node);
+		}
+		return node;
+	};
+
+	const conditions = [rule.appliesWhen ?? []].flat();
+	const query = resolveObject(rule.query);
+	for (const condition of conditions) {
+		references[condition._id] = get(condition._id);
+	}
+
+	const unknownReferences = Object.keys(references).filter((id) => references[id] === undefined);
+	if (unknownReferences.length) {
+		logger.error({ msg: 'Setting validation rule references unknown settings', settingId, references: unknownReferences });
+		return true;
+	}
+
+	// the rule only applies while all of its `appliesWhen` conditions hold,
+	// otherwise it is not relevant and passes
+	const conditionHolds = (condition: { _id: ISetting['_id']; value: unknown }): boolean =>
+		matches({ value: condition.value }, condition._id);
+	if (!conditions.every(conditionHolds)) {
+		return true;
+	}
+
+	return matches(query, settingId);
+};
+
+export const validateSettingRules = (changes: { _id: ISetting['_id']; value: ISetting['value'] }[]): void => {
+	// a value being saved in this batch wins over the stored one,
+	// so rules never compare against stale values
+	const getValueOf = (id: ISetting['_id']): unknown => {
+		const beingSaved = changes.find(({ _id }) => _id === id);
+		return beingSaved ? beingSaved.value : settings.get(id);
+	};
+
+	for (const { _id } of changes) {
+		const setting = settings.getSetting(_id);
+		if (!setting?.validation) {
+			continue;
+		}
+
+		for (const rule of parseValidationRules(setting._id, setting.validation)) {
+			if (evaluateSettingValidationRule(setting._id, rule, getValueOf)) {
+				continue;
+			}
+
+			throw new SettingValidationError(`${setting._id}_Invalid`);
+		}
+	}
+};

apps/meteor/server/meteor-methods/settings/saveSetting.ts

@@ -9,6 +9,7 @@ import { twoFactorRequired } from '../../lib/2fa/twoFactorRequired';
 import { hasPermissionAsync, hasAllPermissionAsync } from '../../lib/authorization/hasPermission';
 import { methodDeprecationLogger } from '../../lib/deprecationWarningLogger';
 import { notifyOnSettingChanged } from '../../lib/notifyListener';
+import { SettingValidationError, validateSettingRules } from '../../lib/settingValidationRules';
 import { disableCustomScripts } from '../../lib/shared/disableCustomScripts';
 import { updateAuditedByUser } from '../../settings/lib/auditedSettingUpdates';
 
@@ -69,6 +70,15 @@ Meteor.methods<ServerMethods>({
 				break;
 		}
 
+		try {
+			validateSettingRules([{ _id, value }]);
+		} catch (error) {
+			if (error instanceof SettingValidationError) {
+				throw new Meteor.Error('error-setting-validation-failed', error.message);
+			}
+			throw error;
+		}
+
 		const auditSettingOperation = updateAuditedByUser({
 			_id: uid,
 			username: (await Meteor.userAsync())!.username!,

apps/meteor/server/meteor-methods/settings/saveSettings.ts

@@ -4,6 +4,7 @@ import { Meteor } from 'meteor/meteor';
 
 import { twoFactorRequired } from '../../lib/2fa/twoFactorRequired';
 import { methodDeprecationLogger } from '../../lib/deprecationWarningLogger';
+import { SettingValidationError } from '../../lib/settingValidationRules';
 import { saveSettingsBulk } from '../../settings/lib/saveSettingsBulk';
 
 declare module '@rocket.chat/ddp-client' {
@@ -34,11 +35,18 @@ Meteor.methods<ServerMethods>({
 			});
 		}
 
-		await saveSettingsBulk(uid, params, {
-			username: (await Meteor.userAsync())!.username!,
-			ip: this.connection.clientAddress || '',
-			useragent: this.connection.httpHeaders['user-agent'] || '',
-		});
+		try {
+			await saveSettingsBulk(uid, params, {
+				username: (await Meteor.userAsync())!.username!,
+				ip: this.connection.clientAddress || '',
+				useragent: this.connection.httpHeaders['user-agent'] || '',
+			});
+		} catch (error) {
+			if (error instanceof SettingValidationError) {
+				throw new Meteor.Error('error-setting-validation-failed', error.message);
+			}
+			throw error;
+		}
 
 		return true;
 	}, {}),

apps/meteor/server/settings/accounts.ts

@@ -1,6 +1,7 @@
 import { Random } from '@rocket.chat/random';
 
 import { settingsRegistry } from '.';
+import { positiveOrDisabled, notGreaterThanSetting, notLowerThanSetting } from './functions/validationRuleBuilders';
 
 export const createAccountSettings = () =>
 	settingsRegistry.addGroup('Accounts', async function () {
@@ -826,12 +827,14 @@ export const createAccountSettings = () =>
 				type: 'int',
 				public: true,
 				enableQuery,
+				validation: [positiveOrDisabled(), notGreaterThanSetting('Accounts_Password_Policy_MaxLength')],
 			});
 
 			await this.add('Accounts_Password_Policy_MaxLength', -1, {
 				type: 'int',
 				public: true,
 				enableQuery,
+				validation: [positiveOrDisabled(), notLowerThanSetting('Accounts_Password_Policy_MinLength')],
 			});
 
 			await this.add('Accounts_Password_Policy_ForbidRepeatingCharacters', true, {

apps/meteor/server/settings/functions/getSettingDefaults.ts

@@ -26,6 +26,7 @@ export const getSettingDefaults = (
 		_updatedAt: options._updatedAt ?? new Date(),
 		...options,
 		...(options.enableQuery ? { enableQuery: JSON.stringify(options.enableQuery) } : undefined),
+		...(options.validation ? { validation: JSON.stringify(options.validation) } : undefined),
 		i18nLabel: options.i18nLabel || _id,
 		hidden: options.hidden || hiddenSettings.has(_id),
 		blocked: options.blocked || blockedSettings.has(_id),

apps/meteor/server/settings/functions/validationRuleBuilders.ts

@@ -0,0 +1,14 @@
+import type { ISetting, SettingValidationRule } from '@rocket.chat/core-typings';
+
+export const positiveOrDisabled = (): SettingValidationRule => ({
+	query: { $or: [{ value: -1 }, { value: { $gte: 1 } }] },
+});
+
+export const notGreaterThanSetting = (otherId: ISetting['_id']): SettingValidationRule => ({
+	query: { $or: [{ value: { $lt: 1 } }, { value: { $lte: { $setting: otherId } } }] },
+	appliesWhen: { _id: otherId, value: { $gte: 1 } },
+});
+
+export const notLowerThanSetting = (otherId: ISetting['_id']): SettingValidationRule => ({
+	query: { $or: [{ value: { $lt: 1 } }, { value: { $gte: { $setting: otherId } } }] },
+});

apps/meteor/server/settings/lib/saveSettingsBulk.ts

@@ -9,6 +9,7 @@ import { updateAuditedByUser } from './auditedSettingUpdates';
 import { getSettingPermissionId } from '../../../app/authorization/lib';
 import { hasPermissionAsync } from '../../lib/authorization/hasPermission';
 import { notifyOnSettingChangedById } from '../../lib/notifyListener';
+import { validateSettingRules } from '../../lib/settingValidationRules';
 import { disableCustomScripts } from '../../lib/shared/disableCustomScripts';
 import { checkSettingValueBounds } from '../checkSettingValueBonds';
 
@@ -112,6 +113,8 @@ export const saveSettingsBulk = async (
 		});
 	}
 
+	validateSettingRules(params);
+
 	const auditSettingOperation = updateAuditedByUser({
 		_id: uid,
 		username: audit.username,

apps/meteor/tests/end-to-end/api/users.ts

@@ -3631,14 +3631,23 @@ describe('[Users]', () => {
 		});
 
 		describe('[Password Policy]', () => {
+			let previousMinLength: Awaited<ReturnType<typeof getSettingValueById>>;
+			let previousMaxLength: Awaited<ReturnType<typeof getSettingValueById>>;
+
 			before(async () => {
 				await updateSetting('Accounts_AllowPasswordChange', true);
 				await updateSetting('Accounts_TwoFactorAuthentication_Enabled', false);
+				[previousMinLength, previousMaxLength] = await Promise.all([
+					getSettingValueById('Accounts_Password_Policy_MinLength'),
+					getSettingValueById('Accounts_Password_Policy_MaxLength'),
+				]);
 			});
 
 			after(async () => {
 				await updateSetting('Accounts_AllowPasswordChange', true);
 				await updateSetting('Accounts_TwoFactorAuthentication_Enabled', true);
+				await updateSetting('Accounts_Password_Policy_MaxLength', previousMaxLength);
+				await updateSetting('Accounts_Password_Policy_MinLength', previousMinLength);
 			});
 
 			it('should throw an error if the password length is less than the minimum length', async () => {
@@ -3667,6 +3676,8 @@ describe('[Users]', () => {
 			});
 
 			it('should throw an error if the password length is greater than the maximum length', async () => {
+				// max must stay >= min, so lower the minimum before capping the maximum at 5
+				await updateSetting('Accounts_Password_Policy_MinLength', 1);
 				await updateSetting('Accounts_Password_Policy_MaxLength', 5);
 
 				const expectedError = {