feat: Authentication bypass via biometric enrollment change - #7351
feat: Authentication bypass via biometric enrollment change#7351OtavioStasiak wants to merge 129 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a keychain-backed biometric trust store, migration, and resolve flow; replaces expo-local-authentication wiring with trust-store APIs; surfaces enrollment-change reasons through lock modals; defers modal callbacks until hide; and adds tests, translations, docs, and E2E coverage. ChangesBiometric Trust Store & Passcode Authentication
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
GitHub Actions
Error
…plumbing Introduce app/lib/biometricTrustStore: a keychain-backed sentinel bound to ACCESS_CONTROL.BIOMETRY_CURRENT_SET so the OS invalidates it when the device's enrolment set changes (iOS errSecItemNotFound, Android KeyPermanentlyInvalidatedException). The store exposes enrol/disenrol/verify/ probeExists and classifies platform errors into a TrustResult union. Wire the store into handleLocalAuthentication via the Option C pattern: the upstream verify() runs before the modal opens and its outcome decides whether to unlock (success), open the passcode modal with biometry available but auto-prompt suppressed (canceled/error), or fall back to passcode-only (unavailable / enrollmentChanged — slice 02 will add the disenrol + flag-clear side effects). PasscodeEnter and ScreenLockedView take a new skipAutoBiometry prop carried over LOCAL_AUTHENTICATE_EMITTER so the biometry button stays visible without re-firing the prompt the user just dismissed. Screen-lock toggle now enrols/disenrols the sentinel alongside flipping BIOMETRY_ENABLED_KEY so the keychain item and the flag stay in lockstep. Part of VLN-216. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add handleBiometricTrustResult, a shared helper that maps a TrustResult into
an (unlocked, modal-config) outcome. Both call sites — handleLocalAuthentication's
upstream verify() preflight and PasscodeEnter's biometry-button retry — route
through it so the invalidation policy lives in one place.
On {kind: 'enrollmentChanged'} the helper runs disenrol() BEFORE clearing
BIOMETRY_ENABLED_KEY, so a crash between the two leaves a state slice 04's
reconciliation can clean up (a flipped flag with a live sentinel would
otherwise look like a healthy enrolment). The resulting modal carries
reason: 'enrollmentChanged' over LOCAL_AUTHENTICATE_EMITTER so slice 03 can
render an explanatory subtitle.
Cancel/error keep biometry available with skipAutoBiometry; unavailable is
passcode-only; success unlocks without a modal.
In PasscodeEnter the biometry button now mirrors hasBiometry/reason in local
state so an enrolment-change triggered from the button hides the button
within the same modal session without re-emitting the event (which would
orphan the upstream openModal promise).
Closes VLN-216.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… unlock When handleLocalAuthentication invalidates biometric trust because the device enrolment set changed, the passcode modal now displays an explanatory subtitle reading "Biometric enrollment changed, please use your passcode". The signal travels over LOCAL_AUTHENTICATE_EMITTER's existing reason payload (added in the previous commit). PasscodeEnter reads reason from props, mirrors it into local state so a button-triggered invalidation can update it without re-emitting, and renders Base's subtitle slot only when reason === 'enrollmentChanged'. The subtitle clears naturally on the next modal open because reason is reinitialised from props each session. Normal auto-lock unlocks, cancel/error fallbacks, and re-opens after a successful unlock leave the subtitle hidden — it is strictly tied to the invalidation event. Part of VLN-216. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
GitHub Actions
Error
…ers on upgrade Existing installs that had biometry enabled before the trust store existed have BIOMETRY_ENABLED_KEY=true but no keychain sentinel, which would force them through the passcode-only modal on first launch after upgrade. Run a one-shot migration on app init that grandfathers them with a silent enrol(). The marker BIOMETRIC_TRUST_MIGRATION_V1_DONE makes this idempotent and lets the helper distinguish two superficially identical states: !migrated && flag && !sentinel → silent enrol(), set marker. upgrade path migrated && flag && !sentinel → clear flag, no enrol(). reconciliation Without the marker, post-invalidation state (flag=true && !sentinel after a crash between disenrol() and the flag-clear in the enrollmentChanged handler) would silently re-bind and undo the enrollment-change protection. With the marker, that state instead clears the flag — the user re-enables biometry from Settings, which runs a fresh enrol() that observes the new enrolment set. enrol() failure leaves the marker unset so the next boot retries, and leaves the flag alone so the next unlock falls into the unavailable branch and asks for the passcode. probeExists() rejection is swallowed and logged. The trade-off (silent bind vs. theoretical pre-fix compromise) follows the product decision in DECISIONS.md / ADR 0006. Part of VLN-216. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
GitHub Actions
Error
49e4acd to
7ec65a4
Compare
GitHub Actions
Error
GitHub Actions
Error
|
@CodeRabbit review |
GitHub Actions
Inactive
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 43-56: The biometry() async function can reject and is currently
invoked as a floating promise from readStorage(); wrap the internal async calls
in biometry() (calls to biometryAuth() and handleBiometricTrustResult()) in a
try/catch and surface/log/handle errors (e.g., set UI state or clear modal)
before returning, and also ensure every call site (e.g., where readStorage()
calls biometry()) either awaits biometry() or attaches .catch(...) to handle
rejections so no unhandled promise rejections occur; update symbols involved:
biometry(), biometryAuth, handleBiometricTrustResult, finishProcess,
setHasBiometry, setReason, and the readStorage() call sites to explicitly handle
errors.
In `@app/views/ScreenLockConfigView.tsx`:
- Around line 165-173: The async setState callback that calls
biometricTrustStore.enrol()/disenrol() lacks error handling and unconditionally
persists userPreferences.setBool(BIOMETRY_ENABLED_KEY, biometry), which can
desync UI and the trust store; wrap the enrol/disenrol calls in a try/catch
inside the callback, only call userPreferences.setBool(BIOMETRY_ENABLED_KEY,
biometry) after the operation succeeds, and on failure revert the UI toggle
(reset this.state.biometry or call setState to the previous value) and
surface/log the error (e.g., show an error toast or processLogger.error) so the
preference and keychain remain consistent with biometricTrustStore state.
🪄 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: d18c30d1-9315-4a3e-b18c-b8ff74efb8f9
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
app/containers/Passcode/PasscodeEnter.test.tsxapp/containers/Passcode/PasscodeEnter.tsxapp/i18n/locales/ar.jsonapp/i18n/locales/bn-IN.jsonapp/i18n/locales/cs.jsonapp/i18n/locales/de.jsonapp/i18n/locales/en.jsonapp/i18n/locales/fi.jsonapp/i18n/locales/fr.jsonapp/i18n/locales/hi-IN.jsonapp/i18n/locales/hu.jsonapp/i18n/locales/it.jsonapp/i18n/locales/nl.jsonapp/i18n/locales/no.jsonapp/i18n/locales/pt-BR.jsonapp/i18n/locales/ru.jsonapp/i18n/locales/sl-SI.jsonapp/i18n/locales/sv.jsonapp/i18n/locales/ta-IN.jsonapp/i18n/locales/te-IN.jsonapp/i18n/locales/tr.jsonapp/i18n/locales/zh-CN.jsonapp/i18n/locales/zh-TW.jsonapp/lib/biometricTrustStore/handleResult.test.tsapp/lib/biometricTrustStore/handleResult.tsapp/lib/biometricTrustStore/index.test.tsapp/lib/biometricTrustStore/index.tsapp/lib/biometricTrustStore/migration.test.tsapp/lib/biometricTrustStore/migration.tsapp/lib/constants/localAuthentication.tsapp/lib/methods/helpers/events.tsapp/lib/methods/helpers/localAuthentication.test.tsapp/lib/methods/helpers/localAuthentication.tsapp/sagas/init.jsapp/views/ScreenLockConfigView.tsxapp/views/ScreenLockedView.tsxjest.setup.jspackage.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/lib/constants/localAuthentication.tsapp/lib/methods/helpers/events.tsjest.setup.jsapp/views/ScreenLockConfigView.tsxapp/sagas/init.jsapp/lib/biometricTrustStore/migration.tsapp/views/ScreenLockedView.tsxapp/lib/biometricTrustStore/index.tsapp/lib/biometricTrustStore/index.test.tsapp/lib/biometricTrustStore/handleResult.tsapp/containers/Passcode/PasscodeEnter.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/lib/biometricTrustStore/handleResult.test.tsapp/lib/methods/helpers/localAuthentication.test.tsapp/containers/Passcode/PasscodeEnter.tsxapp/lib/biometricTrustStore/migration.test.ts
**/*.{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 and baseUrl set to app/ for import resolution
Files:
app/lib/constants/localAuthentication.tsapp/lib/methods/helpers/events.tsapp/views/ScreenLockConfigView.tsxapp/lib/biometricTrustStore/migration.tsapp/views/ScreenLockedView.tsxapp/lib/biometricTrustStore/index.tsapp/lib/biometricTrustStore/index.test.tsapp/lib/biometricTrustStore/handleResult.tsapp/containers/Passcode/PasscodeEnter.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/lib/biometricTrustStore/handleResult.test.tsapp/lib/methods/helpers/localAuthentication.test.tsapp/containers/Passcode/PasscodeEnter.tsxapp/lib/biometricTrustStore/migration.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/lib/constants/localAuthentication.tsapp/lib/methods/helpers/events.tsjest.setup.jsapp/views/ScreenLockConfigView.tsxapp/sagas/init.jsapp/lib/biometricTrustStore/migration.tsapp/views/ScreenLockedView.tsxapp/lib/biometricTrustStore/index.tsapp/lib/biometricTrustStore/index.test.tsapp/lib/biometricTrustStore/handleResult.tsapp/containers/Passcode/PasscodeEnter.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/lib/biometricTrustStore/handleResult.test.tsapp/lib/methods/helpers/localAuthentication.test.tsapp/containers/Passcode/PasscodeEnter.tsxapp/lib/biometricTrustStore/migration.test.ts
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
View components (70+ screen components) should be placed in app/views/ directory
Files:
app/views/ScreenLockConfigView.tsxapp/views/ScreenLockedView.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable UI components should be placed in app/containers/ directory
Files:
app/containers/Passcode/PasscodeEnter.test.tsxapp/containers/Passcode/PasscodeEnter.tsx
🧠 Learnings (5)
📚 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/lib/constants/localAuthentication.tsapp/lib/methods/helpers/events.tsapp/views/ScreenLockConfigView.tsxapp/lib/biometricTrustStore/migration.tsapp/views/ScreenLockedView.tsxapp/lib/biometricTrustStore/index.tsapp/lib/biometricTrustStore/index.test.tsapp/lib/biometricTrustStore/handleResult.tsapp/containers/Passcode/PasscodeEnter.test.tsxapp/lib/methods/helpers/localAuthentication.tsapp/lib/biometricTrustStore/handleResult.test.tsapp/lib/methods/helpers/localAuthentication.test.tsapp/containers/Passcode/PasscodeEnter.tsxapp/lib/biometricTrustStore/migration.test.ts
📚 Learning: 2026-03-30T15:49:26.708Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: app/containers/RoomItem/Actions.tsx:12-12
Timestamp: 2026-03-30T15:49:26.708Z
Learning: In Rocket.Chat.ReactNative, do not rely on `react-native-worklets` v0.6.1 exporting a built-in Jest mock (e.g., `react-native-worklets/lib/module/mock` does not exist for this version). Instead, add the Jest manual mock in your repo’s `jest.setup.js`/`jest.setup.ts`, mocking `react-native-worklets` to provide `scheduleOnRN: jest.fn((fn, ...args) => fn(...args))`. This ensures Jest can import the module and that `scheduleOnRN` executes the passed function during tests.
Applied to files:
jest.setup.js
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.
Applied to files:
app/sagas/init.js
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
📚 Learning: 2026-05-07T17:47:14.516Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7303
File: package.json:5-5
Timestamp: 2026-05-07T17:47:14.516Z
Learning: When reviewing pnpm `packageManager` version pins in any `package.json` (e.g., `"packageManager": "pnpm@<version>"`), don’t rely solely on web-search results to determine whether a version exists. For very recently published versions, cross-check the target version against the official pnpm release page (https://github.com/pnpm/pnpm/releases) and the npm registry page for pnpm (https://www.npmjs.com/package/pnpm) before flagging the pinned version as non-existent.
Applied to files:
package.json
🔇 Additional comments (36)
app/lib/biometricTrustStore/index.ts (1)
1-107: LGTM!app/lib/biometricTrustStore/index.test.ts (1)
1-153: LGTM!jest.setup.js (1)
319-327: LGTM!package.json (1)
106-106: LGTM!app/lib/biometricTrustStore/handleResult.ts (1)
1-40: LGTM!app/lib/biometricTrustStore/handleResult.test.ts (1)
1-89: LGTM!app/lib/biometricTrustStore/migration.ts (1)
1-48: LGTM!app/lib/biometricTrustStore/migration.test.ts (1)
1-135: LGTM!app/lib/constants/localAuthentication.ts (1)
5-5: LGTM!app/lib/methods/helpers/localAuthentication.ts (1)
11-13: LGTM!Also applies to: 55-63, 81-87, 94-97, 119-134
app/lib/methods/helpers/localAuthentication.test.ts (1)
1-149: LGTM!app/lib/methods/helpers/events.ts (1)
13-14: LGTM!app/sagas/init.js (1)
13-13: LGTM!Also applies to: 27-27
app/containers/Passcode/PasscodeEnter.test.tsx (1)
47-61: LGTM!Also applies to: 63-76, 78-93, 101-113
app/views/ScreenLockedView.tsx (1)
20-21: LGTM!Also applies to: 83-88
app/i18n/locales/ar.json (1)
327-327: LGTM!app/i18n/locales/bn-IN.json (1)
454-454: LGTM!app/i18n/locales/cs.json (1)
486-486: LGTM!app/i18n/locales/de.json (1)
448-448: LGTM!app/i18n/locales/en.json (1)
501-501: LGTM!app/i18n/locales/fi.json (1)
429-429: LGTM!app/i18n/locales/fr.json (1)
394-394: LGTM!app/i18n/locales/hi-IN.json (1)
454-454: LGTM!app/i18n/locales/hu.json (1)
455-455: LGTM!app/i18n/locales/it.json (1)
356-356: LGTM!app/i18n/locales/nl.json (1)
394-394: LGTM!app/i18n/locales/no.json (1)
480-480: LGTM!app/i18n/locales/pt-BR.json (1)
496-496: LGTM!app/i18n/locales/ru.json (1)
419-419: LGTM!app/i18n/locales/sl-SI.json (1)
404-404: LGTM!app/i18n/locales/sv.json (1)
428-428: LGTM!app/i18n/locales/ta-IN.json (1)
454-454: LGTM!app/i18n/locales/te-IN.json (1)
453-453: LGTM!app/i18n/locales/tr.json (1)
341-341: LGTM!app/i18n/locales/zh-CN.json (1)
326-326: LGTM!app/i18n/locales/zh-TW.json (1)
342-342: LGTM!
|
iOS Build Available Rocket.Chat 4.73.0.108982 |
…enrollment-change
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
…enrollment-change
|
iOS Build Available Rocket.Chat 4.76.0.1 |
|
Android Build Available Rocket.Chat 4.76.0.1 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRF4SIBJMrgyOqGNBudYh3moHSFWvj0LAWPvpO6WJSb3q618oDzaYZb2CoYyvotXnAHvFNFjc6PEEohe0qj |
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Review summary
Verified every finding below against source (including expo-local-authentication, react-native-keychain, the Kotlin module, AsyncStorage's native serial executors, and the test suites), and checked each against develop to separate PR-introduced from pre-existing. Inline comments carry severity, likelihood, repro steps, a proposed fix, and a plain-English summary.
5 blockers:
| Item | Where |
|---|---|
| Unlock prompt admits Class-2 biometrics — the bypass this PR exists to close | app/lib/methods/helpers/localAuthentication.ts:155 |
| The only E2E asserts nothing on unlock | .maestro/tests/assorted/screen-lock.yaml:259 |
Saga test mock omits logUnlessUserCanceled, so the new catches throw under test and still pass |
app/sagas/__tests__/deepLinking.test.ts |
| Deep-link catch lost the anti-splash-hang fallback | app/sagas/deepLinking.js:234 |
| Transient biometric/keychain errors permanently disenroll the user (borderline) | app/lib/methods/helpers/localAuthentication.ts:113 |
Everything else is non-blocking: the keystore fail-open scope, the shared reason code behind two inaccurate copy strings, the unguarded state.js call site, the discarded enrollProbe() result, the unpersisted 6th passcode attempt, the missing remount key, the dead iOS cancel option, a cleanup bundle, docs volume, and the keypad font-scale trade.
Two notes on things that are correct as written and shouldn't be changed: the grandfather teardown in the migration (removing it reopens this PR's bypass — details inline), and KEY_ALIAS, which must keep its current literal value even if the surrounding identifiers are renamed.
app/sagas/state.js:30 — the only localAuthenticate call site without the cancel guard
Commenting here because the line isn't in the diff.
The code is unchanged, but this PR makes the rejection path live: useDeferredModalSettle.ts:16-18 cancels the prior request on any second emit, and openModal's cancel now rejects with UserCanceledError (localAuthentication.ts:82). Pre-PR that path was dead for every saga caller — develop's localAuthenticate called handleLocalAuthentication() with no argument, so canCloseModal defaulted false, no close button rendered, and data.cancel was unreachable.
All eight other sites guard (deepLinking.js:214/232/330/342, init.js:52-57, login.js:94-99, ServersList.tsx:93-96); this one lets the throw skip recoverSocket() (:32), checkPendingNotification() (:35) and setUserPresenceOnline() (:38), then logs a false error at :39-41.
Severity: medium — presence stuck Away until the next foreground cycle, one dropped Android notification tap. Not message loss: superseding handlers call localAuthenticate only inside if (!connected) (deepLinking.js:211, :326) and on success immediately put(selectServerRequest(host, …, true)), which reconnects. Likelihood: low — needs a deep link or call push inside the seconds-long unlock window with the socket already down. Blocker: no.
Steps to reproduce
- Enable Screen Lock with a passcode and a short auto-lock time.
- Background past auto-lock; drop the socket (airplane mode on, then off).
- Foreground so the passcode screen appears. Do not enter it.
- With the lock screen up, tap a
rocketchat://deep link (or call push) for the same server. - Enter the passcode on the newly-presented lock screen.
- The deep link resolves and the socket reconnects, but presence stays Away, the notification queued at step 2 is never handled, and the log shows a
UserCanceledErrorfromstate.js.
Proposed fix — match the other eight sites:
const server = yield select(state => state.server.server);
try {
yield localAuthenticate(server);
} catch (e) {
logUnlessUserCanceled(e);
return;
}
try {
recoverSocket().catch(e => log(e));
checkPendingNotification().catch(e => { log('[state.js] Error checking pending notification:', e); });
return yield setUserPresenceOnline();
} catch (e) {
log(e);
}Worth stating in the PR description: the guard kills the false log but does not re-run the tidy-up for the superseded request. Doing that needs the superseding unlock to re-trigger foreground work, which is bigger than this PR warrants.
ELI5
When you come back to the app it does three housekeeping chores after you unlock. If a link arrives mid-unlock, this one spot treats "the user unlocked via a different prompt" as a crash, skips all three chores, and writes a scary line in the log about a problem that didn't happen.
app/sagas/init.js:65-70 — out of scope, separate ticket
Also outside the diff. Not for this PR, but worth recording while we're here. This block has two defects and appears to be dead code:
- The inner
const pushNotificationat:68shadows the outer one and binds toremoveItem's resolvedundefined, soJSON.parse(undefined)throwsSyntaxErrorinto the outer catch (:71-74→appStart({ root: RootEnum.ROOT_OUTSIDE })) — afterappReady({})already fired at:65. yield call(deepLinkingClickCallPush, …)invokes a plain action creator (app/actions/deepLinking.ts:29-34) and discards the action; it needsyield put(...). So the cold-start call-push path has never worked.
But nothing ever writes the pushNotification AsyncStorage key. Searching app, android and ios, the only native hits are Intent bundle extras (CustomPushNotification.java:542 replyIntent.putExtra("pushNotification", bundle), ReplyBroadcast.java:45 getBundleExtra) — unrelated to AsyncStorage — and there's no AsyncStorage/RCTAsyncLocalStorage reference anywhere in android/app/src or the iOS app targets. So :67 is always falsy and the block never executes.
Severity: low (dead code), not "throws a logged-in user to the wrong navigator" — that can't happen today. Likelihood: never fires as written. Blocker: no, and it should stay out of this PR: the call → put change would revive a navigation path that has never run in production and needs its own device verification. Mixing that into a security-behaviour PR muddies review.
Recommendation: delete the block, or ticket it separately as "cold-start call-push has never worked" and fix it with real device verification. Confirmed pre-existing — develop's init.js is byte-identical here.
ELI5
There's a little routine meant to handle "you tapped a call notification while the app was closed". It has two bugs — but it also reads a note that nothing ever writes, so it never runs at all. Fixing it means switching it on for the first time, which deserves its own testing.
One more, related: app/sagas/init.js:58-61 if (!serverRecord) return; returns before appReady/appStart, which is a splash hang. Pre-existing, but the new swallow at :54-56 creates a fresh route to it — worth a look while the deep-link fallback is being restored.
…enrollment-change
3 participants
Proposed changes
Fix authentication bypass, where an attacker who knows the device passcode can enrol a new biometric in OS Settings and use it to unlock the app, the previous implementation never verified that the enrolment set hadn't changed since biometry was enabled in-app.
This PR also fixes the bug that freezes the app when you try to change the passcode. Additionally, we added E2E tests for the passcode feature.
Pre-feature (grandfathered) biometry users. On the first launch after upgrade, a user who had biometry enabled before the trust sentinel existed is migrated by binding a baseline sentinel, then forced through the passcode on their next unlock (regardless of the auto-lock window). Because there's no prior baseline to prove the current enrollment wasn't altered before upgrade, this baseline is not trusted: the forced unlock tears it back down and disables biometry, and the user re-enables biometry from settings if they want it (re-binding with an explicit consent prompt). This is a one-time cost. Note: that forced passcode currently shows the "Biometric enrollment changed" subtitle even though nothing changed — a known copy imprecision for this cohort (the relock marker reuses the single enrollmentChanged reason code); behavior is the correct fail-closed outcome.
Issue(s)
https://rocketchat.atlassian.net/browse/VLN-216
How to test or reproduce
Reproducing the original vulnerability (on develop, before this PR):
Verifying the fix (with this PR):
auto-lock cycle.
Other paths to sanity-check:
button → prompt re-fires; cancel again → button stays, flag untouched.
enrolment. First unlock should be biometric as usual — no passcode interstitial. Migration runs once on app init and is idempotent on subsequent launches.
Upgrade-path (silent-bind migration) — step by step:
unlock works. Quit the app.
BIOMETRY_ENABLED_KEY=true survives but the new sentinel doesn't exist yet).
&& !sentinel && !migrated, called enrol() silently, and set BIOMETRIC_TRUST_MIGRATION_V1_DONE=true.
the app → passcode modal with the "Biometric enrollment changed" subtitle, Screen Lock toggle now off. This proves the migration grandfathered the user in and
the sentinel is bound to the current enrolment set.
Xcode's "Erase All Content" simulator option or by toggling biometry off-then-on at the OS level on Android in a way that wipes Keystore). Relaunch →
migration sees flag=true && !sentinel && migrated, clears BIOMETRY_ENABLED_KEY without re-enrolling; Screen Lock toggle shows off; next unlock is
passcode-only. Re-enabling biometry from Settings works normally.
the Settings toggle. Confirms the migration helper doesn't run on installs that never had pre-fix biometry.
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit