Page MenuHomePhabricator
Differential D10188

Put collapse key in encrypted MacOS notification payload
ClosedPublic
Actions

Authored by marcin on Dec 4 2023, 11:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 8:22 AM
Unknown Object (File)
Fri, Nov 22, 8:02 AM
Unknown Object (File)
Fri, Nov 22, 7:53 AM
Unknown Object (File)
Fri, Nov 22, 2:22 AM
Unknown Object (File)
Thu, Nov 21, 4:49 AM
Unknown Object (File)
Thu, Nov 21, 4:49 AM
Unknown Object (File)
Thu, Nov 21, 4:49 AM
Unknown Object (File)
Thu, Nov 21, 4:49 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
michal
kamil
Commits
rCOMM847cb1f21c04: Put collapse key in encrypted MacOS notification payload
Summary

This differential fixes urget issue:
https://linear.app/comm/issue/ENG-6010/invariant-violation-collapsible-notifications-encryption-currently-not.
We want to keep the invariant active since it protects us against accidentally exposing unencrypted
collapseID in notification headers. We just need to put it in notification payload for MacOS notifs as well.
Putting collapseID in notification.collapseID results in collapseID being exposed in notification
headers. Putting it in notification.payload results in collapseID being encrypted.

Test Plan
  1. Build MacOS desktop app.
  2. Send collapsible notifications (chenge the color of the thread for instance).
  3. Ensure that notifications are displayed correctly and version field in MariaDB of relevant olm session

is incremented with each notification.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Dec 4 2023, 11:20 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 4 2023, 11:20 PM
Harbormaster completed remote builds in B24791: Diff 34262.Dec 5 2023, 12:22 AM
marcin requested review of this revision.Dec 5 2023, 12:22 AM
ashoat added inline comments.Dec 5 2023, 4:29 AM
keyserver/src/push/send.js
970 ↗(On Diff #34262)

This is a weird condition to see. Why are the conditions for iOS and macOS named differently? Can we rename them to be more similar? And maybe canDecryptIOSNotifs can be more like iosCanOnlyDecryptNonCollapsibleNotifs or something?

marcin updated this revision to Diff 34277.Dec 5 2023, 6:20 AM

Enhance variable naming in contitions that determine whether notifications should be encrypted

marcin added a child revision: D10192: Pass majorDesktopVersion to platformDetails when creating macos badgeonly notifications..Dec 5 2023, 6:21 AM
michal accepted this revision.Dec 5 2023, 6:43 AM

Much better with the naming change

This revision is now accepted and ready to land.Dec 5 2023, 6:43 AM
marcin updated this revision to Diff 34280.Dec 5 2023, 6:48 AM

Rebase before landing

Harbormaster completed remote builds in B24803: Diff 34277.Dec 5 2023, 6:54 AM
Harbormaster failed remote builds in B24805: Diff 34280!Dec 5 2023, 7:22 AM
Harbormaster completed remote builds in B24805: Diff 34280.Dec 5 2023, 7:35 AM
marcin updated this revision to Diff 34286.Dec 5 2023, 8:39 AM

Rebase before landing

Harbormaster completed remote builds in B24809: Diff 34286.Dec 5 2023, 9:08 AM
Closed by commit rCOMM847cb1f21c04: Put collapse key in encrypted MacOS notification payload (authored by marcin). · Explain WhyDec 5 2023, 9:11 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM847cb1f21c04: Put collapse key in encrypted MacOS notification payload.

Revision Contents
Changeset List

PathSize
keyserver/
src/
push/
3 lines
15 lines

Diff 34280

View Options

keyserver/src/push/crypto.js

Loading...
View Options

keyserver/src/push/send.js

Loading...