security(passcode): persist attempt counter to MMKV to prevent bypass on remount#7436
security(passcode): persist attempt counter to MMKV to prevent bypass on remount#7436Shevilll wants to merge 4 commits into
Conversation
Validate the username field against the server's UTF8_User_Names_Validation setting (with the default pattern as a fallback) before submitting the profile. This prevents invalid characters such as spaces from reaching the server, which previously surfaced only as a generic save error, and shows an inline message explaining the allowed characters. Closes RocketChat#1682
Drop the stale isMasterDetail destructured from useAppSelector (the selector no longer returns it) so only the useMasterDetail() hook declares it, fixing the no-redeclare lint/build failure introduced by merging develop. Also annotate isValidUsername with an explicit boolean return type per the TS guideline.
WalkthroughPasscode lockout state now uses ChangesPasscode lockout persistence
Username validation in ProfileView
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/views/ProfileView/index.test.tsx (1)
125-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the correction flow in the same render.
This test starts from a clean form, so it never proves that the
Username_invaliderror clears after a failed submit. A regression where the old error keeps submit blocked would still pass here. Submit an invalid username first in the same render, then correct it and assert thatsaveUserProfileis finally called.🤖 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.test.tsx` around lines 125 - 134, The ProfileView username test only covers a clean valid submission and misses the correction path; update the `saves when the username is corrected to a valid value` test to use the same `renderProfile()` instance for an initial invalid submit, verify the `Username_invalid` state is exercised, then change the username to a valid value and submit again so `saveUserProfile` is only called after the error clears. Keep the fix within the existing `ProfileView` test helpers and assertions (`renderProfile`, `saveUserProfile`, `profile-view-username`, `profile-view-submit`).
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/containers/Passcode/PasscodeEnter.tsx`:
- 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.
In `@app/containers/Passcode/utils.ts`:
- Around line 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.
In `@app/i18n/locales/en.json`:
- 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.
In `@app/views/ProfileView/index.tsx`:
- Around line 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.
---
Nitpick comments:
In `@app/views/ProfileView/index.test.tsx`:
- Around line 125-134: The ProfileView username test only covers a clean valid
submission and misses the correction path; update the `saves when the username
is corrected to a valid value` test to use the same `renderProfile()` instance
for an initial invalid submit, verify the `Username_invalid` state is exercised,
then change the username to a valid value and submit again so `saveUserProfile`
is only called after the error clears. Keep the fix within the existing
`ProfileView` test helpers and assertions (`renderProfile`, `saveUserProfile`,
`profile-view-username`, `profile-view-submit`).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2faf29fa-107c-4bb1-bc17-00c51971af09
📒 Files selected for processing (6)
app/containers/Passcode/PasscodeEnter.tsxapp/containers/Passcode/utils.tsapp/i18n/locales/en.jsonapp/lib/methods/helpers/localAuthentication.tsapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/ProfileView/index.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/views/ProfileView/index.tsxapp/containers/Passcode/utils.tsapp/containers/Passcode/PasscodeEnter.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/views/ProfileView/index.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/views/ProfileView/index.tsxapp/containers/Passcode/utils.tsapp/containers/Passcode/PasscodeEnter.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/ProfileView/index.test.tsxapp/i18n/locales/en.jsonapp/lib/methods/helpers/localAuthentication.tsapp/views/ProfileView/index.tsxapp/containers/Passcode/utils.tsapp/containers/Passcode/PasscodeEnter.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/ProfileView/index.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/views/ProfileView/index.tsxapp/containers/Passcode/utils.tsapp/containers/Passcode/PasscodeEnter.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
app/i18n/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Place internationalization (i18n) configuration in 'app/i18n/' directory with support for 40+ locales and RTL
Files:
app/i18n/locales/en.json
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/Passcode/utils.tsapp/containers/Passcode/PasscodeEnter.tsx
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/ProfileView/index.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/views/ProfileView/index.tsxapp/containers/Passcode/utils.tsapp/containers/Passcode/PasscodeEnter.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
🪛 ESLint
app/containers/Passcode/utils.ts
[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)
🔇 Additional comments (1)
app/lib/methods/helpers/localAuthentication.ts (1)
50-54: LGTM!
| const ref = useRef<IBase>(null); | ||
| let attempts = 0; | ||
| let lockedUntil: any = false; | ||
| const attempts = parseInt(UserPreferences.getString(ATTEMPTS_KEY) || '0', 10); |
There was a problem hiding this comment.
🎯 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/PasscodeRepository: 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.
| 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; | ||
| }; |
There was a problem hiding this comment.
📐 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.
| 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
| "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", |
There was a problem hiding this comment.
📐 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.
| 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)) | ||
| }); |
There was a problem hiding this comment.
🎯 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.
| 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.
1 participant
Summary
This PR addresses passcode counter bypasses (Issue #7018) where local component state resets on unmount/remount. By migrating validation and the attempt counter to MMKV secure storage, we close this security gap and ensure persistence across component lifecycles.
Details
app/lib/methods/helpers/localAuthentication.tsandapp/containers/Passcode/utils.ts.Summary by CodeRabbit