fix: password bypass fix#7019
fix: password bypass fix#7019Yaddalapalli-Charan-Kumar-Naidu wants to merge 1 commit intoRocketChat:developfrom
Conversation
WalkthroughThis change fixes a security vulnerability where the passcode attempt counter reset on component remount, enabling users to indefinitely bypass the lockout threshold. The fix adds AsyncStorage readback on mount and replaces the transient attempts variable with a persistent ref to maintain state across remounts. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/Passcode/PasscodeEnter.tsx (1)
41-49:⚠️ Potential issue | 🟠 MajorReset in-memory attempts after lock expiry and sanitize parsed storage value.
When
diff <= 1, storage is reset butattemptsRef.currentis not, so the next wrong attempt can immediately re-lock. Also, invalid stored values can becomeNaNand break threshold checks.Proposed fix
const readStorage = async () => { - const stored = await getAttempts(); - attemptsRef.current = parseInt(stored || '0', 10); + const stored = await getAttempts(); + const parsedAttempts = Number.parseInt(stored ?? '0', 10); + attemptsRef.current = Number.isFinite(parsedAttempts) && parsedAttempts >= 0 ? parsedAttempts : 0; lockedUntil = await getLockedUntil(); if (lockedUntil) { const diff = getDiff(lockedUntil); if (diff <= 1) { await resetAttempts(); + attemptsRef.current = 0; setStatus(TYPE.ENTER); } else { setStatus(TYPE.LOCKED); } } else { setStatus(TYPE.ENTER); } biometry(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/Passcode/PasscodeEnter.tsx` around lines 41 - 49, When handling stored attempt and lock expiry in PasscodeEnter.tsx, sanitize the parsed value from getAttempts by using a safe fallback if parseInt returns NaN (e.g., treat non-numeric stored as 0) and, when getLockedUntil() indicates expiry (getDiff(lockedUntil) <= 1), also reset the in-memory counter attemptsRef.current to 0 after calling resetAttempts() and before setStatus(TYPE.ENTER) so the next wrong attempt doesn't immediately re-lock; update logic around getAttempts, attemptsRef.current, getLockedUntil, getDiff, resetAttempts and setStatus accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/containers/Passcode/PasscodeEnter.tsx`:
- Around line 41-49: When handling stored attempt and lock expiry in
PasscodeEnter.tsx, sanitize the parsed value from getAttempts by using a safe
fallback if parseInt returns NaN (e.g., treat non-numeric stored as 0) and, when
getLockedUntil() indicates expiry (getDiff(lockedUntil) <= 1), also reset the
in-memory counter attemptsRef.current to 0 after calling resetAttempts() and
before setStatus(TYPE.ENTER) so the next wrong attempt doesn't immediately
re-lock; update logic around getAttempts, attemptsRef.current, getLockedUntil,
getDiff, resetAttempts and setStatus accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/containers/Passcode/PasscodeEnter.tsx
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/Passcode/PasscodeEnter.tsx (1)
app/lib/methods/userPreferences.ts (1)
useUserPreferences(63-90)
🔇 Additional comments (2)
app/containers/Passcode/PasscodeEnter.tsx (2)
24-29: Good fix for remount-based attempt reset.Using
attemptsRefplusgetItemis the right direction and closes the core bypass described in the issue.
68-75:⚠️ Potential issue | 🟠 MajorAwait AsyncStorage writes before finalizing lockout state.
setAttempts(...)andsetLockedUntil(...)are fire-and-forget calls (lines 71, 74). These are async operations that may not complete before the function returns. If either write is delayed or fails, subsequent reads will observe incomplete lockout state.Make the
setTimeoutcallback async and await both AsyncStorage writes before transitioning to the LOCKED state:Example fix
const onEndProcess = (p: string) => { - setTimeout(() => { + setTimeout(async () => { if (sha256(p) === passcode) { finishProcess(); } else { attemptsRef.current += 1; - // Always persist the updated count BEFORE checking threshold, - // so AsyncStorage is accurate even when lockout triggers. - setAttempts(attemptsRef.current.toString()); + await setAttempts(attemptsRef.current.toString()); if (attemptsRef.current >= MAX_ATTEMPTS) { + await setLockedUntil(new Date().toISOString()); setStatus(TYPE.LOCKED); - setLockedUntil(new Date().toISOString()); Haptics.notificationAsync(Haptics.NotificationFeedbackType.Error); } else { ref?.current?.wrongPasscode(); Haptics.notificationAsync(Haptics.NotificationFeedbackType.Warning); } } }, 200); };Likely an incorrect or invalid review comment.
1 participant
Proposed changes
Fix a security bypass in the passcode lockout mechanism. The failed attempt counter was a plain local variable (
let attempts = 0) that reset to0on every component remount. SincegetItemwas never called forATTEMPTS_KEY, the persisted count in AsyncStorage was ignored on mount. This allowed a user to background/foreground the app repeatedly to reset the counter and gain unlimited passcode attempts.Changes made:
let attemptswithuseRefso the count survives re-renders within the same lifecyclegetItemfromuseAsyncStorageand call it in [readStorage()]to restore the persisted count on every mountsetAttempts()before theMAX_ATTEMPTSthreshold check so AsyncStorage always holds the accurate count, even when the lockout-triggering attempt is madeIssue(s)
Closes #7018
How to test or reproduce
Affected screen: Passcode entry screen ([app/containers/Passcode/PasscodeEnter.tsx]
Screenshots
Types of changes
Checklist
Further comments
The root cause was two separate issues compounding each other:
let attempts = 0— a plain variable that resets every remount (should be auseRef)useAsyncStorage(ATTEMPTS_KEY)only destructuredsetItem, nevergetItem— so the persisted value was write-only and never read backSummary by CodeRabbit