fix(security): harden Android notification receivers, token storage, and deep-link login - #7447
fix(security): harden Android notification receivers, token storage, and deep-link login#7447OtavioStasiak wants to merge 27 commits into
Conversation
…otification receivers, confirm deep-link login
GitHub Actions
Inactive
|
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:
WalkthroughTokens now use server-scoped keys across JS, Android, and iOS, with migration and legacy fallback support. Deep-link resume login now shows a confirmation prompt before proceeding, and Android notification receivers are no longer exported. ChangesToken scoping and migration
Deep-link login confirmation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/methods/logout.ts (1)
76-80: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep the legacy fallback before server removal.
If migration has not run or failed,
resumeisundefined,sdk.login({ resume })fails, and the server-side push token cleanup is skipped before local data is deleted. Fall back to${TOKEN_KEY}-${userId}here as well.Proposed fix
- const resume = UserPreferences.getString(getUserTokenKey(server, userId)); + const resume = UserPreferences.getString(getUserTokenKey(server, userId)) ?? UserPreferences.getString(`${TOKEN_KEY}-${userId}`); + if (!resume) { + throw new Error('Missing resume token for server removal'); + }🤖 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/lib/methods/logout.ts` around lines 76 - 80, The logout flow in `logout` uses only `getUserTokenKey(server, userId)` for `resume`, so when migration hasn’t run the `RocketchatClient.login` call fails and token cleanup is skipped. Update the `resume` lookup to fall back to the legacy `${TOKEN_KEY}-${userId}` key before calling `sdk.login({ resume })`, keeping the existing cleanup flow in `app/lib/methods/logout.ts` intact.
🤖 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/sagas/init.js`:
- Around line 31-60: The migration in migrateTokenKeysToServerScoped is
incorrectly copying a shared legacy token into every server-scoped slot when
multiple servers reuse the same userId. Update the first pass to detect
ambiguous userId mappings and skip migrating those legacy tokens into new slots,
while still removing the legacy token afterward so affected sessions
re-authenticate safely. Keep the fix localized to migrateTokenKeysToServerScoped
and its use of UserPreferences, TOKEN_KEY_SERVER_SCOPED_MIGRATED, and
getUserTokenKey.
In `@app/sagas/selectServer.ts`:
- Line 174: The server token lookup in selectServerRequest/selectServer should
handle legacy sessions before migration has completed. Update the token read
around UserPreferences.getString(getUserTokenKey(server, userId)) to fall back
to the legacy token key when the server-scoped value is missing, or otherwise
gate the deep-link/share-extension callers until appInit() finishes migration.
Keep the change localized to the token resolution path so direct dispatches
still succeed for legacy users.
---
Outside diff comments:
In `@app/lib/methods/logout.ts`:
- Around line 76-80: The logout flow in `logout` uses only
`getUserTokenKey(server, userId)` for `resume`, so when migration hasn’t run the
`RocketchatClient.login` call fails and token cleanup is skipped. Update the
`resume` lookup to fall back to the legacy `${TOKEN_KEY}-${userId}` key before
calling `sdk.login({ resume })`, keeping the existing cleanup flow in
`app/lib/methods/logout.ts` intact.
🪄 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: 124eb5ef-1af7-4051-a141-833d7f08c334
⛔ Files ignored due to path filters (1)
app/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/chat/rocket/reactnative/notification/Ejson.javaapp/i18n/locales/en.jsonapp/lib/constants/keys.tsapp/lib/methods/logout.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/deepLinking.jsapp/sagas/init.jsapp/sagas/login.jsapp/sagas/selectServer.tsios/Shared/RocketChat/MMKV.swiftios/Shared/RocketChat/Storage.swiftios/Watch/WatchConnection.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: Build Android / Build
- GitHub Check: Build iOS / Build
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/i18n/locales/en.jsonapp/lib/constants/keys.tsapp/sagas/selectServer.tsapp/sagas/init.jsapp/lib/methods/logout.tsapp/sagas/deepLinking.jsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/login.js
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
**/*.{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/keys.tsapp/sagas/selectServer.tsapp/sagas/init.jsapp/lib/methods/logout.tsapp/sagas/deepLinking.jsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/login.js
**/*.{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/lib/constants/keys.tsapp/sagas/selectServer.tsapp/lib/methods/logout.tsapp/sagas/__tests__/deepLinking.test.ts
**/*.{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/lib/constants/keys.tsapp/sagas/selectServer.tsapp/sagas/init.jsapp/lib/methods/logout.tsapp/sagas/deepLinking.jsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/login.js
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place sagas in 'app/sagas/' directory for handling side effects
Files:
app/sagas/selectServer.tsapp/sagas/__tests__/deepLinking.test.ts
🧠 Learnings (4)
📚 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/keys.tsapp/sagas/selectServer.tsapp/lib/methods/logout.tsapp/sagas/__tests__/deepLinking.test.ts
📚 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.jsapp/sagas/deepLinking.jsapp/sagas/login.js
📚 Learning: 2026-06-15T16:17:10.741Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7398
File: android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java:211-211
Timestamp: 2026-06-15T16:17:10.741Z
Learning: When querying the Android SQLCipher WatermelonDB `subscriptions` table, treat the table’s primary-key column `id` as the Rocket.Chat room id (`rid`). In other words, a clause like `WHERE id = ?` should bind `ejson.rid` (this is equivalent to the iOS-side `WHERE rid = ?`). Do not flag this as a column-mismatch issue, because SQLite `subscriptions.id` stores the same value as the Rocket.Chat `rid` field.
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/sagas/__tests__/deepLinking.test.ts
🔇 Additional comments (13)
app/i18n/locales/en.json (1)
233-234: LGTM!app/sagas/deepLinking.js (1)
24-24: LGTM!Also applies to: 41-64, 270-276
app/sagas/__tests__/deepLinking.test.ts (1)
85-90: LGTM!Also applies to: 109-114, 316-335
app/lib/constants/keys.ts (1)
26-36: LGTM!app/lib/methods/logout.ts (1)
11-18: LGTM!Also applies to: 27-29
app/sagas/init.js (1)
5-5: LGTM!Also applies to: 67-70
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
138-178: LGTM!ios/Watch/WatchConnection.swift (1)
44-44: LGTM!app/sagas/login.js (1)
25-25: LGTM!Also applies to: 344-344
app/sagas/selectServer.ts (1)
32-32: LGTM!android/app/src/main/AndroidManifest.xml (1)
105-117: LGTM!ios/Shared/RocketChat/MMKV.swift (1)
21-31: LGTM!ios/Shared/RocketChat/Storage.swift (1)
15-15: LGTM!
…t before direct server selects
|
iOS Build Available Rocket.Chat 4.75.0.109259 |
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109258 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNQeFdGa37dcYES0nomT6vqYCR3_JSDuS8adBZQpBOs9PLUesMFb2HE24l75n-6m2xNuwKzoM0qtktXM9Ug_ |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109262 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNS4Eg8yH-E9IJrDRU9IucfqUsEIqRBD6L09EQsLlfVgVZ_O1cT4AV1Sah2cTMaj4wKFtWpMqhpy6mC7LGSz |
|
iOS Build Available Rocket.Chat 4.75.0.109263 |
julio-rocketchat
left a comment
julio-rocketchat
left a comment
There was a problem hiding this comment.
It looks good to me. It seems like all the gaps that could lead to security issues have been closed by this PR. Tks.
|
iOS Build Available Rocket.Chat 4.76.0.109513 |
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Waiting
There was a problem hiding this comment.
Two-axis review (standards + spec against the linked ticket), with each finding independently re-verified against the tree at e0d7a12. Every inline comment carries severity, likelihood, repro steps, and a proposed fix.
Highest value per line touched: hoisting the deep-link consent gate above the side effects it guards (it also deletes both new race guards and their comments), and collapsing the migration's three data structures into one group-by.
The finding that decides whether this PR actually closes the reported hole is the native one: the legacy fallback in Ejson.java and MMKV.swift keeps the userId-only lookup live in the notification handler, share extension, and Watch, with no way for native code to observe the migration flag. Worth settling that design ("flag-check natively" vs "migrate natively") before more code lands.
One thing not inline-able: the PR description says the migration copies every server first "so a userId shared by multiple servers isn't dropped", but migrateTokenKeysToServerScoped.ts:38-40 deliberately does drop shared userIds, forcing re-auth. The code is right and the description is wrong. The description also still carries a conversational tail ("Want me to draft the How to test section too?") that should be stripped.
Also worth a maintainer's confirmation: 25 locale files were machine-translated in-PR — check that this matches the project's convention versus en-only plus the translator workflow.
Not a defect (raised and dismissed during review): yield* call(...) in selectServer.ts is correct — call comes from typed-redux-saga there.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
Security re-review against 5e04c0d
Re-checked every finding from the previous pass against the current head, including the threads that were already resolved. Most of them really are fixed — details below so nothing gets re-litigated.
One resolved thread should be reopened
The native userId-only token fallback is still live on both platforms and the flag that gates it has exactly one writer, in JS. I've replied in the Ejson.java and MMKV.swift threads with the full reasoning, including why the "restore on APP.INIT runs the migration early enough" argument doesn't cover a cold data-only push.
Two items that have no diff hunk to anchor to
- Dead constant.
LoadNotification.java:65still declaresprivate String TOKEN_KEY = "reactnativemeteor_usertoken-";. It is the only occurrence in the file and is never used — the file's actual token access goes throughejson.token()(:96) and thex-auth-tokenheader (:128). Two native declarations of the same key is how the next drift starts; worth deleting while it's cheap. - Scope. The diff is 395 files against
develop— fonts, a UIKit Button, snapshots, CI scripts — plus a Meteor Connect race fix atdeepLinking.js:281-285(thetake(types.METEOR.SUCCESS)wait) and a new 183-lineapp/sagas/__tests__/deepLinking.test.ts. The Meteor Connect fix looks correct, it's just not security. Splitting the unrelated churn out would make the security diff reviewable on its own. Not blocking, and I realise some of this may be unavoidable at this point in the branch's life.
Confirmed fixed — no action
- Consent gate ordering. Now strictly ahead of
getServerInfo(:259),serverInitAdd/NewServer(:271-276) andloginRequest(:287), at both entry points. Declining runs onlyshowToast+fallbackNavigation(), leaving no Server row, no Meteor Connect, no token. It also now sits after the already-connected branch, so a resume link for a Server the User is signed into no longer prompts. - Exported receivers.
ReplyBroadcastandDismissNotificationflipped toexported="false"; the other two were already false. The remaining exported components are MainActivity, ShareActivity, and CallKeep's service behindBIND_TELECOM_CONNECTION_SERVICE. - Key namespace. All the JS call sites route through the
keys.tshelpers now; the only hand-built literals left are native, where sharing a helper isn't possible. - Migration correctness. Every branch drops the legacy slot, the ambiguous-
userIdcase is handled, the orphan sweep is correctly anchored on theuserIdshape, and the flag is only set on success so a throw retries next launch. Tests cover all four branches. - Logging. Every key-revealing
Ejsonlog isBuildConfig.DEBUG-gated and prints key names, not values; the ungated warnings carry no key or token and:214sanitises the URL.
Withdrawing one earlier comment
The RUNNING_E2E_TESTS branch in confirmDeepLinkLogin — @OtavioStasiak was right and I was wrong to push on it. I checked the mechanism properly this time: nothing inlines the variable (babel-preset-expo only rewrites EXPO_PUBLIC_*, and react-native-dotenv only rewrites @env imports), but in a release bundle it's a runtime read against RN's polyfilled process.env, which holds only NODE_ENV — so the gate can never open, and forceLoginPrompt !== 'true' means a deep link parameter can only turn the prompt on. Removing it would also genuinely break the 51 flows that go through login-with-deeplink.yaml, exactly as you said. Not a security issue, and the same idiom is used in 14 other files.
I also withdraw two smaller points from my earlier notes: the orphan sweep is correctly anchored (a Server id always carries a dot, a scheme or a path separator), and the comment-line churn was already your own feedback.
3 participants
Proposed changes
This PR hardens authentication and deep-link handling against a set of related token-confusion /
silent-authentication issues, and stops exporting Android notification receivers.
The resume token was stored under reactnativemeteor_usertoken-${userId}, with no server component.
Because two servers can legitimately share the same userId — and a malicious server can deliberately
force a collision — that storage slot was ambiguous: the last writer won, and a lookup by a different
server could resolve a token belonging to another server, enabling token confusion / exfiltration.
The token is now keyed by reactnativemeteor_usertoken-${server}-${userId} via a shared
getUserTokenKey(server, userId) helper, used consistently across the login, server-select, and logout
paths, and mirrored on the native side (MMKV.swift on iOS, Ejson.java on Android).
A one-time, two-pass migration in the init saga moves existing tokens from the legacy slot to the new
server-scoped slot (copy every server first, then remove legacy slots, so a userId shared by multiple
servers isn't dropped mid-migration). Logout removes both the new and the legacy slots.
A rocketchat://auth?host=…&token=… deep link could silently authenticate the user into an arbitrary
server, and any app or web page can fire it. Before consuming a resume token for a server the user
isn't already signed in to, the app now asks for explicit confirmation and lands on the login screen if
declined. (Auto-confirmed under RUNNING_E2E_TESTS so the E2E harness, which bootstraps via deep link,
isn't blocked by the native alert.)
The notification receivers in AndroidManifest.xml are no longer exported, closing them off from other
apps on the device.
Notes:
they're added in app/i18n/locales/en.json.
token-scoping and the deep-link-confirmation behaviors.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1342
How to test or reproduce
Deep-link login confirmation
"rocketchat://auth?host=your.server.com&token=" (or the equivalent adb shell am
start).
leaves you on the login/outside screen with no session. Before this PR the login happened silently.
Token scoped to (server, userId)
session bleed between servers.
this build over it and relaunch. Expected: you stay logged in (token migrated from the legacy slot to
the server-scoped one), and logging out clears it fully.
Affected screens: deep-link entry (confirmation alert), Servers list / server switching, and the
login/logout flow. No visual changes beyond the new confirmation alert.
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Security