Skip to content

security(passcode): persist attempt counter to MMKV to prevent bypass on remount #7436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Shevilll wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

security(passcode): persist attempt counter to MMKV to prevent bypass on remount #7436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63a00c1
fix: validate username characters in profile before saving
Shevilll Jun 22, 2026
d3f11f8
Merge branch 'develop' into fix/username-validation-1682
Shevilll Jun 24, 2026
193cf24
fix(profile): remove duplicate isMasterDetail binding and type guard
Shevilll Jun 24, 2026
66bf48a
security(passcode): persist attempt counter to MMKV to prevent bypass…
Shevilll Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions app/containers/Passcode/PasscodeEnter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useEffect, useRef, useState } from 'react';
import { useAsyncStorage } from '@react-native-async-storage/async-storage';
import { gestureHandlerRootHOC } from 'react-native-gesture-handler';
import * as Haptics from 'expo-haptics';
import { sha256 } from 'js-sha256';
Expand All @@ -10,7 +9,7 @@ import { TYPE } from './constants';
import { ATTEMPTS_KEY, LOCKED_OUT_TIMER_KEY, MAX_ATTEMPTS, PASSCODE_KEY } from '../../lib/constants/localAuthentication';
import { biometryAuth, resetAttempts } from '../../lib/methods/helpers/localAuthentication';
import { getDiff, getLockedUntil } from './utils';
import { useUserPreferences } from '../../lib/methods/userPreferences';
import UserPreferences, { useUserPreferences } from '../../lib/methods/userPreferences';
import I18n from '../../i18n';

interface IPasscodePasscodeEnter {
Expand All @@ -20,12 +19,9 @@ interface IPasscodePasscodeEnter {

const PasscodeEnter = ({ hasBiometry, finishProcess }: IPasscodePasscodeEnter) => {
const ref = useRef<IBase>(null);
let attempts = 0;
let lockedUntil: any = false;
const attempts = parseInt(UserPreferences.getString(ATTEMPTS_KEY) || '0', 10);

@coderabbitai coderabbitai Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect Base/wrongPasscode and PasscodeEnter for re-render triggers on failed attempts
fd -t f 'PasscodeEnter|Base' app/containers/Passcode --exec cat -n {}
rg -nP -C3 'wrongPasscode|useUserPreferences|setStatus' app/containers/Passcode

Repository: RocketChat/Rocket.Chat.ReactNative

Length of output: 50390

Critical security vulnerability: In-session passcode lockout bypass

The attempts counter is captured as a constant at render time (Line 22). When a wrong passcode is entered, the failure handler (Lines 60–67) updates the persistent storage (UserPreferences) but does not update the local attempts variable or trigger a re-render.

Consequently, nextAttempts is calculated using the initial stale value for every iteration (e.g., always 0 + 1), preventing the nextAttempts >= MAX_ATTEMPTS check from ever triggering a lockout while the component remains mounted.

Move the parseInt call inside the onEndProcess callback to read the current persisted value on every attempt.

🐛 Proposed fix 
-	const attempts = parseInt(UserPreferences.getString(ATTEMPTS_KEY) || '0', 10);
 	const [status, setStatus] = useState<TYPE | null>(null);

 	const onEndProcess = (p: string) => {
 		setTimeout(() => {
 			if (sha256(p) === passcode) {
 				finishProcess();
 			} else {
+				const currentAttempts = parseInt(UserPreferences.getString(ATTEMPTS_KEY) || '0', 10);
 				const nextAttempts = attempts + 1;
+				const nextAttempts = currentAttempts + 1;
 				if (nextAttempts >= MAX_ATTEMPTS) {
 					setStatus(TYPE.LOCKED);
 					UserPreferences.setString(LOCKED_OUT_TIMER_KEY, new Date().toISOString());
 					Haptics.notificationAsync(Haptics.NotificationFeedbackType.Error);
 				} else {
 					ref?.current?.wrongPasscode();
 					UserPreferences.setString(ATTEMPTS_KEY, nextAttempts.toString());
 					Haptics.notificationAsync(Haptics.NotificationFeedbackType.Warning);
 				}
 			}
 		}, 200);
 	};
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/containers/Passcode/PasscodeEnter.tsx` at line 22, The passcode lockout
logic in PasscodeEnter is using a stale attempts value captured at render time,
so repeated failures never reach MAX_ATTEMPTS while the component stays mounted.
Update onEndProcess to read the latest persisted count from UserPreferences with
parseInt on each failed attempt, then compute nextAttempts from that fresh value
before writing it back and checking the lockout condition. Keep the fix centered
around PasscodeEnter and its onEndProcess failure path so the counter reflects
every attempt correctly.

const [passcode] = useUserPreferences(PASSCODE_KEY);
const [status, setStatus] = useState<TYPE | null>(null);
const { setItem: setAttempts } = useAsyncStorage(ATTEMPTS_KEY);
const { setItem: setLockedUntil } = useAsyncStorage(LOCKED_OUT_TIMER_KEY);

const biometry = async () => {
if (hasBiometry && status === TYPE.ENTER) {
Expand All @@ -37,7 +33,7 @@ const PasscodeEnter = ({ hasBiometry, finishProcess }: IPasscodePasscodeEnter) =
};

const readStorage = async () => {
lockedUntil = await getLockedUntil();
const lockedUntil = await getLockedUntil();
if (lockedUntil) {
const diff = getDiff(lockedUntil);
if (diff <= 1) {
Expand All @@ -61,14 +57,14 @@ const PasscodeEnter = ({ hasBiometry, finishProcess }: IPasscodePasscodeEnter) =
if (sha256(p) === passcode) {
finishProcess();
} else {
attempts += 1;
if (attempts >= MAX_ATTEMPTS) {
const nextAttempts = attempts + 1;
if (nextAttempts >= MAX_ATTEMPTS) {
setStatus(TYPE.LOCKED);
setLockedUntil(new Date().toISOString());
UserPreferences.setString(LOCKED_OUT_TIMER_KEY, new Date().toISOString());
Haptics.notificationAsync(Haptics.NotificationFeedbackType.Error);
} else {
ref?.current?.wrongPasscode();
setAttempts(attempts?.toString());
UserPreferences.setString(ATTEMPTS_KEY, nextAttempts.toString());
Haptics.notificationAsync(Haptics.NotificationFeedbackType.Warning);
}
}
Expand Down
5 changes: 3 additions & 2 deletions app/containers/Passcode/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import AsyncStorage from '@react-native-async-storage/async-storage';
import UserPreferences from '../../lib/methods/userPreferences';

import dayjs from '../../lib/dayjs';
import { LOCKED_OUT_TIMER_KEY, TIME_TO_LOCK } from '../../lib/constants/localAuthentication';

export const getLockedUntil = async () => {
const t = await AsyncStorage.getItem(LOCKED_OUT_TIMER_KEY);
const t = UserPreferences.getString(LOCKED_OUT_TIMER_KEY);
if (t) {
return dayjs(t).add(TIME_TO_LOCK, 'millisecond').toDate();
}
return null;
};
Comment on lines +1 to 12

@coderabbitai coderabbitai Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Resolve the two ESLint errors in this file.

Static analysis reports an empty line within the import group (Line 1–2) and an async arrow function with no await (Line 6). UserPreferences.getString is synchronous, so the async keyword is unnecessary; dropping it clears require-await. Callers in PasscodeEnter.tsx use await getLockedUntil(), which continues to work against a non-promise return value.

♻️ Proposed fix 
 import UserPreferences from '../../lib/methods/userPreferences';
-
 import dayjs from '../../lib/dayjs';
 import { LOCKED_OUT_TIMER_KEY, TIME_TO_LOCK } from '../../lib/constants/localAuthentication';

-export const getLockedUntil = async () => {
+export const getLockedUntil = (): Date | null => {
 	const t = UserPreferences.getString(LOCKED_OUT_TIMER_KEY);
 	if (t) {
 		return dayjs(t).add(TIME_TO_LOCK, 'millisecond').toDate();
 	}
 	return null;
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import UserPreferences from '../../lib/methods/userPreferences';
import dayjs from '../../lib/dayjs';
import { LOCKED_OUT_TIMER_KEY, TIME_TO_LOCK } from '../../lib/constants/localAuthentication';
export const getLockedUntil = async () => {
const t = await AsyncStorage.getItem(LOCKED_OUT_TIMER_KEY);
const t = UserPreferences.getString(LOCKED_OUT_TIMER_KEY);
if (t) {
return dayjs(t).add(TIME_TO_LOCK, 'millisecond').toDate();
}
return null;
};
import UserPreferences from '../../lib/methods/userPreferences';
import dayjs from '../../lib/dayjs';
import { LOCKED_OUT_TIMER_KEY, TIME_TO_LOCK } from '../../lib/constants/localAuthentication';
export const getLockedUntil = (): Date | null => {
const t = UserPreferences.getString(LOCKED_OUT_TIMER_KEY);
if (t) {
return dayjs(t).add(TIME_TO_LOCK, 'millisecond').toDate();
}
return null;
};
🧰 Tools
🪛 ESLint

[error] 1-1: There should be no empty line within import group

(import/order)

[error] 6-6: Async arrow function has no 'await' expression.

(require-await)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/containers/Passcode/utils.ts` around lines 1 - 12, Resolve the ESLint
issues in getLockedUntil within the Passcode utils module by removing the
unnecessary blank line inside the import block and making getLockedUntil a
non-async arrow function since it only calls the synchronous
UserPreferences.getString and has no await. Keep the existing return shape
intact so PasscodeEnter.tsx can continue to call getLockedUntil without any
behavioral change.

Source: Linters/SAST tools



export const getDiff = (t: string | number | Date) => new Date(t).getTime() - new Date().getTime();
1 change: 1 addition & 0 deletions app/i18n/locales/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,7 @@
"User_not_found_or": "User not found or incorrect password",
"User_sent_an_attachment": "{{user}} sent an attachment",
"Username": "Username",
"Username_invalid": "Use only letters, numbers, dots, hyphens and underscores",

@coderabbitai coderabbitai Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Make the validation message regex-agnostic.

ProfileView now validates against UTF8_User_Names_Validation, but this copy still describes only the default [0-9a-zA-Z-_.]+ fallback. On servers with customized username rules, users will get the wrong guidance. A generic message like “Username contains invalid characters” would stay accurate.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/i18n/locales/en.json` at line 1005, Update the Username_invalid copy in
the locale entry used by ProfileView so it no longer mentions specific allowed
characters from the default fallback regex. The validation now uses
UTF8_User_Names_Validation, so make the message generic and accurate for all
server-configured username rules; locate the string by the Username_invalid key
and replace it with neutral wording such as an invalid-characters message.

"Username_is_already_in_use": "Username is already in use.",
"Username_not_available": "Username not available",
"Username_or_email": "Username or email",
Expand Down
7 changes: 5 additions & 2 deletions app/lib/methods/helpers/localAuthentication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as LocalAuthentication from 'expo-local-authentication';
import RNBootSplash from 'react-native-bootsplash';
import AsyncStorage from '@react-native-async-storage/async-storage';
import { sha256 } from 'js-sha256';

import dayjs from '../../dayjs';
Expand Down Expand Up @@ -48,7 +47,11 @@ export const saveLastLocalAuthenticationSession = async (
});
};

export const resetAttempts = (): Promise<void> => AsyncStorage.multiRemove([LOCKED_OUT_TIMER_KEY, ATTEMPTS_KEY]);
export const resetAttempts = (): Promise<void> => {
UserPreferences.removeItem(LOCKED_OUT_TIMER_KEY);
UserPreferences.removeItem(ATTEMPTS_KEY);
return Promise.resolve();
};

const openModal = (hasBiometry: boolean, force?: boolean) =>
new Promise<void>((resolve, reject) => {
Expand Down
25 changes: 24 additions & 1 deletion app/views/ProfileView/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { twoFactor } from '../../lib/services/twoFactor';
import handleSaveUserProfileError from '../../lib/methods/helpers/handleSaveUserProfileError';
import EventEmitter from '../../lib/methods/helpers/events';
import { setUser } from '../../actions/login';
import I18n from '../../i18n';

jest.mock('react-redux', () => ({
useDispatch: jest.fn()
Expand Down Expand Up @@ -69,7 +70,8 @@ const buildState = () => ({
Accounts_AllowUserAvatarChange: true,
Accounts_AllowUsernameChange: true,
Accounts_CustomFields: '',
Accounts_AllowDeleteOwnAccount: true
Accounts_AllowDeleteOwnAccount: true,
UTF8_User_Names_Validation: '[0-9a-zA-Z-_.]+'
}
});

Expand Down Expand Up @@ -110,6 +112,27 @@ describe('ProfileView submit', () => {
expect(handleSaveUserProfileError).not.toHaveBeenCalled();
});

it('does not save when the username contains invalid characters', async () => {
const { getByTestId, findByText } = renderProfile();

fireEvent.changeText(getByTestId('profile-view-username'), 'cygnus b');
fireEvent.press(getByTestId('profile-view-submit'));

await findByText(I18n.t('Username_invalid'));
expect(saveUserProfile).not.toHaveBeenCalled();
});

it('saves when the username is corrected to a valid value', async () => {
(saveUserProfile as jest.Mock).mockResolvedValue(true);

const { getByTestId } = renderProfile();

fireEvent.changeText(getByTestId('profile-view-username'), 'cygnus.b');
fireEvent.press(getByTestId('profile-view-submit'));

await waitFor(() => expect(saveUserProfile).toHaveBeenCalledWith({ username: 'cygnus.b' }, {}));
});

it('asks for the current password before changing the email', async () => {
const { getByTestId } = renderProfile();

Expand Down
32 changes: 26 additions & 6 deletions app/views/ProfileView/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,25 @@ import ConfirmEmailChangeActionSheetContent from './components/ConfirmEmailChang
const MAX_BIO_LENGTH = 260;
const MAX_NICKNAME_LENGTH = 120;

// Mirror the server's username validation, which matches the value against the
// UTF8_User_Names_Validation setting anchored as a full match (`^pattern$`),
// falling back to the default pattern when the setting is empty or invalid.
// https://github.com/RocketChat/Rocket.Chat/blob/develop/apps/meteor/app/lib/server/functions/validateUsername.ts
const DEFAULT_USERNAME_VALIDATION = '[0-9a-zA-Z-_.]+';

const isValidUsername = (username: string, pattern: string): boolean => {
try {
return new RegExp(`^(${pattern || DEFAULT_USERNAME_VALIDATION})$`).test(username);
} catch {
// Fall back to the default pattern if the server provides an invalid regex.
return new RegExp(`^(${DEFAULT_USERNAME_VALIDATION})$`).test(username);
}
};

interface IProfileViewProps {
navigation: NativeStackNavigationProp<ProfileStackParamList, 'ProfileView'>;
}
const ProfileView = ({ navigation }: IProfileViewProps): ReactElement => {
const validationSchema = yup.object().shape({
name: yup.string().required(I18n.t('Name_required')),
email: yup.string().email(I18n.t('Email_must_be_a_valid_email')).required(I18n.t('Email_required')),
username: yup.string().required(I18n.t('Username_required'))
});

const { showActionSheet, hideActionSheet } = useActionSheet();
const { colors } = useTheme();
const dispatch = useDispatch();
Expand All @@ -67,6 +76,7 @@ const ProfileView = ({ navigation }: IProfileViewProps): ReactElement => {
Accounts_AllowUserAvatarChange,
Accounts_AllowUsernameChange,
Accounts_CustomFields,
UTF8_User_Names_Validation,
serverVersion,
user
} = useAppSelector(state => ({
Expand All @@ -77,9 +87,19 @@ const ProfileView = ({ navigation }: IProfileViewProps): ReactElement => {
Accounts_AllowUserAvatarChange: state.settings.Accounts_AllowUserAvatarChange as boolean,
Accounts_AllowUsernameChange: state.settings.Accounts_AllowUsernameChange as boolean,
Accounts_CustomFields: state.settings.Accounts_CustomFields as string,
UTF8_User_Names_Validation: state.settings.UTF8_User_Names_Validation as string,
serverVersion: state.server.version,
Accounts_AllowDeleteOwnAccount: state.settings.Accounts_AllowDeleteOwnAccount as boolean
}));

const validationSchema = yup.object().shape({
name: yup.string().required(I18n.t('Name_required')),
email: yup.string().email(I18n.t('Email_must_be_a_valid_email')).required(I18n.t('Email_required')),
username: yup
.string()
.required(I18n.t('Username_required'))
.test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation))
});
Comment on lines +95 to +102

@coderabbitai coderabbitai Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Skip regex validation for unchanged usernames.

This check runs on every submit, even when the user cannot edit username or leaves it unchanged. If an admin tightens UTF8_User_Names_Validation after an account was created, a legacy username can fail local validation and block saving unrelated profile changes. Only apply the regex when the username is actually being edited.

Suggested fix 
 	username: yup
 		.string()
 		.required(I18n.t('Username_required'))
-		.test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation))
+		.test('valid-username', I18n.t('Username_invalid'), value => {
+			if (!value || value === user?.username || !Accounts_AllowUsernameChange) {
+				return true;
+			}
+			return isValidUsername(value, UTF8_User_Names_Validation);
+		})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const validationSchema = yup.object().shape({
name: yup.string().required(I18n.t('Name_required')),
email: yup.string().email(I18n.t('Email_must_be_a_valid_email')).required(I18n.t('Email_required')),
username: yup
.string()
.required(I18n.t('Username_required'))
.test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation))
});
const validationSchema = yup.object().shape({
name: yup.string().required(I18n.t('Name_required')),
email: yup.string().email(I18n.t('Email_must_be_a_valid_email')).required(I18n.t('Email_required')),
username: yup
.string()
.required(I18n.t('Username_required'))
.test('valid-username', I18n.t('Username_invalid'), value => {
if (!value || value === user?.username || !Accounts_AllowUsernameChange) {
return true;
}
return isValidUsername(value, UTF8_User_Names_Validation);
})
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/ProfileView/index.tsx` around lines 95 - 102, The username
validation in ProfileView’s validationSchema currently applies the
UTF8_User_Names_Validation regex on every submit, even when username is
unchanged or not editable. Update the yup test on username so it only runs
isValidUsername when the field is actually being edited or has changed from its
original value, and otherwise skips the regex check while still preserving the
required rule and other validations.

const isMasterDetail = useMasterDetail();
const {
control,
Expand Down