Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
9 changes: 7 additions & 2 deletions android/app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,19 @@
<action android:name="com.google.firebase.MESSAGING_EVENT" />
</intent-filter>
</service>
<!-- exported="false": these receivers are only ever fired by the app's own
notification action PendingIntents (explicit, component-targeted intents from
the same UID). Exporting them let any installed app broadcast a crafted intent
and have the app reply/send as the authenticated user (message impersonation /
token exfiltration). -->
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
<receiver
android:name="chat.rocket.reactnative.notification.ReplyBroadcast"
android:enabled="true"
android:exported="true" />
android:exported="false" />
<receiver
android:name="chat.rocket.reactnative.notification.DismissNotification"
android:enabled="true"
android:exported="true" >
android:exported="false" >
</receiver>
<receiver
android:name="chat.rocket.reactnative.notification.VideoConfBroadcast"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,32 +135,47 @@ public String getCallerAvatarUri(int sizePx) {
}

public String token() {
String serverURL = serverURL();
String userId = userId();
MMKV mmkv = getMMKV();

if (mmkv == null) {
Log.e(TAG, "token() called but MMKV is null");
return "";
}

if (userId == null || userId.isEmpty()) {
Log.w(TAG, "token() called but userId is null or empty");
return "";
}

String key = TOKEN_KEY.concat(userId);
if (BuildConfig.DEBUG) {
Log.d(TAG, "Looking up token with key: " + key);

// Server-scoped key: reactnativemeteor_usertoken-{server}-{userId}.
// Keep this format in sync with getUserTokenKey() on the JS side. The token used to be
// stored under reactnativemeteor_usertoken-{userId} (no server component), which let a
// lookup resolve a token belonging to a different server when two servers shared a userId
// (token confusion). Read the server-scoped slot first and only fall back to the legacy
// slot for the window between an app update and the JS migration running on next launch.
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
String token = null;
if (serverURL != null && !serverURL.isEmpty()) {
String key = TOKEN_KEY.concat(serverURL).concat("-").concat(userId);
if (BuildConfig.DEBUG) {
Log.d(TAG, "Looking up token with server-scoped key");
}
token = mmkv.decodeString(key);
}

String token = mmkv.decodeString(key);


if (token == null || token.isEmpty()) {
// Legacy fallback (pre-migration). Safe because the exported-receiver vector that
// abused this ambiguity is closed (ReplyBroadcast/DismissNotification are not exported).
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
token = mmkv.decodeString(TOKEN_KEY.concat(userId));
}

if (token == null || token.isEmpty()) {
Log.w(TAG, "No token found in MMKV for userId");
} else if (BuildConfig.DEBUG) {
Log.d(TAG, "Successfully retrieved token from MMKV");
}

return token != null ? token : "";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5882,7 +5882,7 @@ exports[`Story Snapshots: Timestamp should match snapshot 1`] = `
]
}
>
a year ago
2 years ago
</Text>
</Text>
</Text>
Expand Down
2 changes: 2 additions & 0 deletions app/i18n/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@
"Dark": "Dark",
"Dark_level": "Dark level",
"decline": "Decline",
"Deep_link_login_description": "A link is asking to sign you in to {{server}}. Only continue if you opened this link yourself and trust it.",
"Deep_link_login_title": "Sign in to this server?",
Comment thread
OtavioStasiak marked this conversation as resolved.
"Default": "Default",
"Default_browser": "Default browser",
"Defined_user_as_role": "defined {{user}} as {{role}}",
Expand Down
11 changes: 11 additions & 0 deletions app/lib/constants/keys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,16 @@ export const ALERT_DISPLAY_TYPE_PREFERENCES_KEY = 'RC_ALERT_DISPLAY_TYPE_PREFERE
export const CRASH_REPORT_KEY = 'RC_CRASH_REPORT_KEY';
export const ANALYTICS_EVENTS_KEY = 'RC_ANALYTICS_EVENTS_KEY';
export const TOKEN_KEY = 'reactnativemeteor_usertoken';
/**
* MMKV key for the auth token, scoped to BOTH server and userId.
*
* The token used to be stored under `${TOKEN_KEY}-${userId}` (no server component). Because two
* servers can legitimately share the same userId (and a malicious server can force a collision),
* that slot was ambiguous: the last writer won and lookups by a different server could resolve a
* token that belongs to another server, enabling token confusion / exfiltration. Scoping the key
* to (server, userId) makes the slot unambiguous. Keep this format in sync with
* `Ejson.token()` on the Android side and the migration in the init saga.
*/
Comment thread
OtavioStasiak marked this conversation as resolved.
export const getUserTokenKey = (server: string, userId: string): string => `${TOKEN_KEY}-${server}-${userId}`;
export const CURRENT_SERVER = 'currentServer';
export const CERTIFICATE_KEY = 'RC_CERTIFICATE_KEY';
13 changes: 11 additions & 2 deletions app/lib/methods/logout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ import database, { getDatabase } from '../database';
import log from './helpers/log';
import { disconnect } from '../services/connect';
import sdk from '../services/sdk';
import { CURRENT_SERVER, E2E_PRIVATE_KEY, E2E_PUBLIC_KEY, E2E_RANDOM_PASSWORD_KEY, TOKEN_KEY } from '../constants/keys';
import {
CURRENT_SERVER,
E2E_PRIVATE_KEY,
E2E_PUBLIC_KEY,
E2E_RANDOM_PASSWORD_KEY,
TOKEN_KEY,
getUserTokenKey
} from '../constants/keys';
import UserPreferences from './userPreferences';
import { removePushToken } from '../services/restApi';
import { roomsSubscription } from './subscriptions/rooms';
Expand All @@ -17,6 +24,8 @@ import { _activeUsersSubTimeout } from './getUsersPresence';
function removeServerKeys({ server, userId }: { server: string; userId?: string | null }) {
UserPreferences.removeItem(`${TOKEN_KEY}-${server}`);
if (userId) {
UserPreferences.removeItem(getUserTokenKey(server, userId));
// Also remove the legacy non-server-scoped token slot, in case this server predates the migration.
UserPreferences.removeItem(`${TOKEN_KEY}-${userId}`);
}
UserPreferences.removeItem(`${BASIC_AUTH_KEY}-${server}`);
Expand Down Expand Up @@ -64,7 +73,7 @@ export async function removeServer({ server }: { server: string }): Promise<void
try {
const userId = UserPreferences.getString(`${TOKEN_KEY}-${server}`);
if (userId) {
const resume = UserPreferences.getString(`${TOKEN_KEY}-${userId}`);
const resume = UserPreferences.getString(getUserTokenKey(server, userId));

try {
const sdk = new RocketchatClient({ host: server, protocol: 'ddp', useSsl: isSsl(server) });
Expand Down
29 changes: 28 additions & 1 deletion app/sagas/__tests__/deepLinking.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ jest.mock('i18n-js', () => ({
default: { t: (k: string) => k }
}));

// Deep-link resume login asks for confirmation before authenticating. Default to confirming
// (invoke onPress) so the existing login-path tests run; the decline path is covered explicitly.
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
jest.mock('../../lib/methods/helpers/info', () => ({
showConfirmationAlert: jest.fn(({ onPress }: { onPress: () => void }) => onPress())
}));

// Mock helpers to avoid auxStore (getUidDirectMessage / getRoomTitle call reduxStore.getState())
jest.mock('../../lib/methods/helpers', () => ({
getUidDirectMessage: jest.fn(() => null),
Expand All @@ -100,11 +106,12 @@ import { loginSuccess } from '../../actions/login';
import { selectServerSuccess } from '../../actions/server';
import { connectSuccess } from '../../actions/connect';
import { appStart } from '../../actions/app';
import { LOGIN } from '../../actions/actionsTypes';
import { APP, LOGIN } from '../../actions/actionsTypes';
import { RootEnum } from '../../definitions';
import reducers from '../../reducers';
import deepLinkingRoot from '../deepLinking';
import UserPreferences from '../../lib/methods/userPreferences';
import { showConfirmationAlert } from '../../lib/methods/helpers/info';
import { getServerById } from '../../lib/database/services/Server';
import { canOpenRoom } from '../../lib/methods/canOpenRoom';
import { getServerInfo } from '../../lib/methods/getServerInfo';
Expand Down Expand Up @@ -306,6 +313,26 @@ describe('deepLinking saga — Regression race (new server + token + room path)'
expect(loginRequested()).toBe(true);
});

// Security: a deep-link resume token must not silently authenticate. If the user declines
// the confirmation, no login is attempted and the app lands outside (manual login screen).
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
it('does not dispatch loginRequest when the deep-link login confirmation is declined', async () => {
jest.mocked(showConfirmationAlert).mockClear();
jest.mocked(showConfirmationAlert).mockImplementationOnce(({ onCancel }: any) => onCancel?.());

const { store, actions } = setupRecordingStore();
const loginRequested = () => actions.some(a => a.type === LOGIN.REQUEST);

store.dispatch(deepLinkingOpen(makeParamsWithToken()));
await flushSagaMicrotasks();
await jest.advanceTimersByTimeAsync(1000);
await flushSagaMicrotasks();

// User declined → confirmation shown, no login, parked outside.
expect(jest.mocked(showConfirmationAlert)).toHaveBeenCalledTimes(1);
expect(loginRequested()).toBe(false);
expect(actions.some(a => a.type === APP.START && (a as any).root === RootEnum.ROOT_OUTSIDE)).toBe(true);
});

/**
* Regression negative: dispatch SERVER.SELECT_SUCCESS, LOGIN.SUCCESS.
* Flush microtasks. Assert goRoom NOT yet called.
Expand Down
32 changes: 32 additions & 0 deletions app/sagas/deepLinking.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { goRoom, navigateToRoom } from '../lib/methods/helpers/goRoom';
import { getIsMasterDetail } from '../lib/hooks/useMasterDetail';
import { localAuthenticate } from '../lib/methods/helpers/localAuthentication';
import log from '../lib/methods/helpers/log';
import { showConfirmationAlert } from '../lib/methods/helpers/info';
import { showToast } from '../lib/methods/helpers/showToast';
import UserPreferences from '../lib/methods/userPreferences';
import { videoConfJoin } from '../lib/methods/videoConf';
Expand All @@ -37,6 +38,30 @@ const roomTypes = {
channels: 'l'
};

/**
* A `rocketchat://auth?host=…&token=…` deep link can silently authenticate the user to an
* arbitrary server. Any app (or web page) can fire it, so before consuming a resume token for a
* server the user is not already signed in to, ask for explicit confirmation. Resolves true if
* the user confirms, false otherwise.
*/
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
const confirmDeepLinkLogin = host =>
new Promise(resolve => {
// E2E tests bootstrap the session via a deep link and can't dismiss a native Alert, so
// auto-confirm under RUNNING_E2E_TESTS. This preserves the pre-fix silent behavior for
// tests only; real users still get the security prompt.
if (process.env.RUNNING_E2E_TESTS === 'true') {
resolve(true);
return;
}
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
showConfirmationAlert({
title: I18n.t('Deep_link_login_title'),
message: I18n.t('Deep_link_login_description', { server: host }),
confirmationText: I18n.t('Login'),
onPress: () => resolve(true),
onCancel: () => resolve(false)
});
});

const handleInviteLink = function* handleInviteLink({ params, requireLogin = false }) {
if (params.path && params.path.startsWith('invite/')) {
const token = params.path.replace('invite/', '');
Expand Down Expand Up @@ -242,6 +267,13 @@ const handleOpen = function* handleOpen({ params }) {
}

if (params.token) {
// A resume token in a deep link silently authenticates the user to `host`. Since any
// app or web page can fire this link, require explicit confirmation before consuming it.
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
const confirmed = yield call(confirmDeepLinkLogin, host);
if (!confirmed) {
yield put(appStart({ root: RootEnum.ROOT_OUTSIDE }));
return;
}
if (!hostAlreadyConnected) {
yield take(types.SERVER.SELECT_SUCCESS);
// SERVER.SELECT_SUCCESS doesn't mean 'connected'; skip the take if it already is.
Expand Down
65 changes: 64 additions & 1 deletion app/sagas/init.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's going to happen if the user updates the app and opens the app from push notification?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the post-update launch, restore (triggered by APP.INIT) runs migrateTokenKeysToServerScoped first, before it reads ${TOKEN_KEY}-${server}. The migration is flag-guarded
(TOKEN_KEY_SERVER_SCOPED_MIGRATED), moves the legacy token into the server-scoped slot, and only runs once.

A push-notification open goes through the same server-selection path, and selectServer.ts also calls migrateTokenKeysToServerScoped before its server-scoped token read — so
even if the deep link/push flow gets there before/instead of the normal restore, the token is already migrated. Result: the user stays logged in and lands in the right room;
migration doesn't re-run on subsequent opens.

The one exception is an ambiguous userId (shared by multiple servers): that legacy token is dropped, so those sessions re-authenticate — the user would hit login instead of
the notification's room.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { call, put, select, takeLatest } from 'redux-saga/effects';
import RNBootSplash from 'react-native-bootsplash';
import AsyncStorage from '@react-native-async-storage/async-storage';

import { CURRENT_SERVER, TOKEN_KEY } from '../lib/constants/keys';
import { CURRENT_SERVER, TOKEN_KEY, getUserTokenKey } from '../lib/constants/keys';
import UserPreferences from '../lib/methods/userPreferences';
import { selectServerRequest } from '../actions/server';
import { setAllPreferences } from '../actions/sortPreferences';
Expand All @@ -21,8 +21,71 @@ export const initLocalSettings = function* initLocalSettings() {
yield put(setAllPreferences(sortPreferences));
};

const TOKEN_KEY_SERVER_SCOPED_MIGRATED = 'RC_TOKEN_KEY_SERVER_SCOPED_MIGRATED';
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated

/**
* One-time migration: move auth tokens from the legacy non-server-scoped slot
* `${TOKEN_KEY}-${userId}` to the server-scoped slot `${TOKEN_KEY}-${server}-${userId}`.
* See `getUserTokenKey` for why the legacy scheme was ambiguous (token confusion).
*
* Only userIds referenced by a single server are migrated: the legacy slot can hold just one
* token, so when several servers share a userId we can't tell which server it belongs to.
* Migrating an ambiguous token would plant one server's token into another server's slot —
* exactly the confusion this change exists to prevent — so those slots are dropped instead,
* forcing a safe re-authentication.
*/
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
export const migrateTokenKeysToServerScoped = function* migrateTokenKeysToServerScoped() {
Comment thread
OtavioStasiak marked this conversation as resolved.
Outdated
try {
if (UserPreferences.getBool(TOKEN_KEY_SERVER_SCOPED_MIGRATED)) {
return;
}
const serversDB = database.servers;
const servers = yield serversDB.get('servers').query().fetch();

// Map each server to its userId and count how many servers reference each userId.
const serverUserIds = [];
const serverCountByUserId = {};
for (let i = 0; i < servers.length; i += 1) {
const server = servers[i].id;
const userId = UserPreferences.getString(`${TOKEN_KEY}-${server}`);
if (!userId) {
continue;
}
serverUserIds.push({ server, userId });
serverCountByUserId[userId] = (serverCountByUserId[userId] || 0) + 1;
}

// Collected in a Set so an ambiguous userId shared by N servers is removed once.
const legacyKeys = new Set();
for (let i = 0; i < serverUserIds.length; i += 1) {
const { server, userId } = serverUserIds[i];
const legacyKey = `${TOKEN_KEY}-${userId}`;
// Ambiguous: don't migrate, just drop the legacy slot so the session re-authenticates.
if (serverCountByUserId[userId] > 1) {
legacyKeys.add(legacyKey);
continue;
}
const newKey = getUserTokenKey(server, userId);
if (!UserPreferences.getString(newKey)) {
const token = UserPreferences.getString(legacyKey);
if (token) {
UserPreferences.setString(newKey, token);
legacyKeys.add(legacyKey);
}
}
}
// Drop the legacy slots (migrated and ambiguous alike) now that the migration is done.
legacyKeys.forEach(key => UserPreferences.removeItem(key));
UserPreferences.setBool(TOKEN_KEY_SERVER_SCOPED_MIGRATED, true);
} catch (e) {
log(e);
}
};

const restore = function* restore() {
try {
yield call(migrateTokenKeysToServerScoped);

const server = UserPreferences.getString(CURRENT_SERVER);
let userId = UserPreferences.getString(`${TOKEN_KEY}-${server}`);

Expand Down
4 changes: 2 additions & 2 deletions app/sagas/login.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { inquiryRequest, inquiryReset } from '../ee/omnichannel/actions/inquiry'
import { isOmnichannelStatusAvailable } from '../ee/omnichannel/lib';
import { RootEnum } from '../definitions';
import sdk from '../lib/services/sdk';
import { CURRENT_SERVER, TOKEN_KEY } from '../lib/constants/keys';
import { CURRENT_SERVER, TOKEN_KEY, getUserTokenKey } from '../lib/constants/keys';
import { getCustomEmojis } from '../lib/methods/getCustomEmojis';
import { getIsMasterDetail } from '../lib/hooks/useMasterDetail';
import { getEnterpriseModules, isOmnichannelModuleAvailable, isVoipModuleAvailable } from '../lib/methods/enterpriseModules';
Expand Down Expand Up @@ -341,7 +341,7 @@ const handleLoginSuccess = function* handleLoginSuccess({ user }) {
});

UserPreferences.setString(`${TOKEN_KEY}-${server}`, user.id);
UserPreferences.setString(`${TOKEN_KEY}-${user.id}`, user.token);
UserPreferences.setString(getUserTokenKey(server, user.id), user.token);
UserPreferences.setString(CURRENT_SERVER, server);
EventEmitter.emit('connected');
const currentRoot = yield select(state => state.app.root);
Expand Down
9 changes: 7 additions & 2 deletions app/sagas/selectServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import UserPreferences from '../lib/methods/userPreferences';
import { encryptionStop } from '../actions/encryption';
import { inquiryReset } from '../ee/omnichannel/actions/inquiry';
import { type IServerInfo, RootEnum, type TServerModel } from '../definitions';
import { CERTIFICATE_KEY, CURRENT_SERVER, TOKEN_KEY } from '../lib/constants/keys';
import { CERTIFICATE_KEY, CURRENT_SERVER, TOKEN_KEY, getUserTokenKey } from '../lib/constants/keys';
import { migrateTokenKeysToServerScoped } from './init';
import { checkSupportedVersions } from '../lib/methods/checkSupportedVersions';
import { getLoginSettings, setSettings } from '../lib/methods/getSettings';
import { getServerInfo } from '../lib/methods/getServerInfo';
Expand Down Expand Up @@ -150,6 +151,10 @@ const handleSelectServer = function* handleSelectServer({ server, version, fetch
yield put(inquiryReset());
yield put(encryptionStop());
yield put(clearActiveUsers());
// Deep-link and share-extension startup can dispatch selectServerRequest() directly, without
// going through appInit(). Run the (idempotent, flag-guarded) token migration here too so the
// server-scoped read below finds legacy sessions instead of falling through to a login screen.
yield* call(migrateTokenKeysToServerScoped);
const userId = UserPreferences.getString(`${TOKEN_KEY}-${server}`);
let user = null;
if (userId) {
Expand All @@ -171,7 +176,7 @@ const handleSelectServer = function* handleSelectServer({ server, version, fetch
requirePasswordChange: userRecord.requirePasswordChange
};
} else {
const token = UserPreferences.getString(`${TOKEN_KEY}-${userId}`);
const token = UserPreferences.getString(getUserTokenKey(server, userId));
Comment thread
OtavioStasiak marked this conversation as resolved.
if (token) {
user = { token };
}
Expand Down
Loading
Loading