Page MenuHomePhabricator
Differential D10245

Suffix web olm notifs session with cookieID to avoid race condition during multiple simultaneous log-in processes.
ClosedPublic
Actions

Authored by marcin on Dec 8 2023, 4:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 3, 11:22 PM
Unknown Object (File)
Tue, Mar 18, 9:12 AM
Unknown Object (File)
Tue, Mar 18, 9:12 AM
Unknown Object (File)
Tue, Mar 18, 9:12 AM
Unknown Object (File)
Tue, Mar 18, 9:12 AM
Unknown Object (File)
Tue, Mar 18, 9:12 AM
Unknown Object (File)
Tue, Mar 18, 9:11 AM
Unknown Object (File)
Tue, Mar 18, 9:11 AM
View All Files
Subscribers
ashoat
inka

Details

Reviewers
tomek
kamil
michal
Commits
rCOMM676b627236a3: Suffix web olm notifs session with cookieID to avoid race condition during…
Summary

This differential implements solution for a bug described in https://linear.app/comm/issue/ENG-5911/olmbad-message-mac-on-web. The solution works as follows:

  1. Web client creates olm session and corresponding encryption key and persists it under key that is suffixed with its cookieID.
  2. When notifications arrives all keys in IndexedDB are retrieved and filtered against those that keep olm sessions and their encryption keys.
  3. Keys are sorted lexicographically and last ones are used to actually perform decryption. This means that for decryption we will use olm session that was created by lexicographically largest cookie ID. We have strong reasons to believe that keyserver uses olm session associated with the largest cookie ID for encryption.
  4. Other sessions are deleted

It is important to review this code closely since it implements quite complex solution for rather infrequent bug.

Test Plan
  1. Open three pages with logged-out modal. Paste credentials for the same user in each and start log in process in each card. Initially this was the way to reproduce the issue.
  2. Open developer tools in one page and see that there are multiple sessions and encryption keys and each of them is suffixed with cookie id that exists in your MariaDB instance.
  3. Send notification. Ensure that:
    • it is correctly decrypted
    • once it is received there is just one olm session in IndexedDB and it is the one that initially had largest cookie ID

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-5911
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Dec 8 2023, 4:19 AM
Herald added a subscriber: ashoat. · View Herald TranscriptDec 8 2023, 4:19 AM
marcin edited the summary of this revision. (Show Details)Dec 8 2023, 4:31 AM
marcin edited the test plan for this revision. (Show Details)
marcin added inline comments.Dec 8 2023, 4:40 AM
web/push-notif/notif-crypto-utils.js
310 ↗(On Diff #34411)

This scenario can occur when notification is received but there is olm session creation process in progress - for example encryption key was created and persisted but olm session hasn't been persisted yet.
I spent some time thinking what should we do in such case. Intuitively we could use encryption key and olm session associated with lower cookieID (the highest cookie ID for which we have both encryption key and olm session). but in fact we can't assume that the keyserver used this session to encrypt notification. I actually think that if there is olm session creation process in progress then the keyserver has already associated device_token with cookie ID that we are creating session for. In such case the keyserver will probably see that there is no olm session for this cookie ID and notificaion won't be encrypted.

350 ↗(On Diff #34411)

We need to cover the case of users that won't log-out and log-in after this code is landed and release. They will have session and encryption key without cookie ID suffix. We could return undefined or empty string from this function but intuitively I thought that returning some string (such as no cookie) should be safest.

Harbormaster completed remote builds in B24882: Diff 34411.Dec 8 2023, 4:56 AM
marcin requested review of this revision.Dec 8 2023, 4:56 AM
marcin planned changes to this revision.Dec 8 2023, 6:25 AM
marcin added a subscriber: inka.

I talked to @inka and was advices to refactor this code so that session creator function takes keyserver id as an argument and ashoatKeyserverID is hardcoded in web/socket.react.js

marcin updated this revision to Diff 34437.Dec 8 2023, 8:04 AM
marcin edited the test plan for this revision. (Show Details)

Address inka's advice

marcin updated this revision to Diff 34438.Dec 8 2023, 8:07 AM

Rebase

Harbormaster completed remote builds in B24903: Diff 34437.Dec 8 2023, 9:09 AM
Harbormaster completed remote builds in B24904: Diff 34438.Dec 8 2023, 9:19 AM
michal requested changes to this revision.Dec 11 2023, 3:13 AM

Keys are sorted lexicographically

Are you sure about that? Aren't the cookie ids numerically increasing?

web/push-notif/notif-crypto-utils.js
284–285 ↗(On Diff #34438)

Nit: I would have preferred for them to have the same naming convention (either either DBLabel or DBKey)

296 ↗(On Diff #34438)

It's not good practice in JS to throw non-Error values. In particular the try catch in decryptWebNotification depends on the error value to have .message.

348 ↗(On Diff #34438)

This won't give us any more type safety, but is a bit more explicit.

This revision now requires changes to proceed.Dec 11 2023, 3:13 AM
marcin added a comment.EditedDec 14 2023, 5:48 AM
In D10245#297865, @michal wrote:

Keys are sorted lexicographically

Are you sure about that? Aren't the cookie ids numerically increasing?

In this code we use cookieID strings which are stringified numbers. We sort stringified numbers lexicographically which has the same effect as sorting original numbers.

marcin updated this revision to Diff 34637.Dec 14 2023, 5:54 AM
  1. Throw Error instance instead of raw string.
  2. Rename encryptionKeyDBLabel -> encryptionKeyDBKey
  3. Enhance typing of getCookieIDFromOlmDBKey
marcin planned changes to this revision.Dec 14 2023, 6:50 AM

@michal point is valid - if cookieIDs differ in length lexicographic sort can fail to match numerical sort. We need to address this issue by temporarily converting cookieIDs to numbers.

marcin updated this revision to Diff 34639.Dec 14 2023, 6:51 AM

Temporarily convert cookieIDs to numbers during sorting stage.

Harbormaster completed remote builds in B25072: Diff 34637.Dec 14 2023, 7:02 AM
Harbormaster completed remote builds in B25074: Diff 34639.Dec 14 2023, 7:22 AM
michal accepted this revision.Dec 14 2023, 7:55 AM
michal added inline comments.
web/push-notif/notif-crypto-utils.js
358–367

We probably don't need to keep the original string cookie here and just convert the resulting number to string again, but that's fine.

This revision is now accepted and ready to land.Dec 14 2023, 7:55 AM
marcin added inline comments.Dec 15 2023, 2:47 AM
web/push-notif/notif-crypto-utils.js
358–367

Why do we need to convert cookieID to string back? This function doesn't return any numbers. It returns string keys suffixed by cookieID in the same form as they entered this function. The only difference is that they are sorted according to numerical order of cookieIDs they are suffixed with.

inka added a comment.Dec 18 2023, 1:18 AM

Address inka's advice

Thank you!

marcin updated this revision to Diff 34771.Dec 18 2023, 1:29 AM

Rebase before landing

Harbormaster completed remote builds in B25160: Diff 34771.Dec 18 2023, 1:47 AM
Closed by commit rCOMM676b627236a3: Suffix web olm notifs session with cookieID to avoid race condition during… (authored by marcin). · Explain WhyDec 18 2023, 1:50 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM676b627236a3: Suffix web olm notifs session with cookieID to avoid race condition during….
tomek mentioned this in D10831: [lib] Run the keyserver auth only when it isn't running and a user isn't already authenticated.Jan 30 2024, 5:18 AM

Revision Contents
Changeset List

PathSize
lib/
types/
1 line
utils/
8 lines
web/
account/
20 lines
push-notif/
153 lines
16 lines

Diff 34639

View Options

lib/types/crypto-types.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

lib/utils/cookie-utils.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

web/account/account-hooks.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

web/push-notif/notif-crypto-utils.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

web/socket.react.js

Loading...