Page MenuHomePhabricator

Implement function to encrypt relevant parts of iOS notification
ClosedPublic

Authored by marcin on May 15 2023, 8:18 AM.
Tags
None
Referenced Files
F1431426: D7813.diff
Thu, Mar 28, 2:31 AM
Unknown Object (File)
Sun, Mar 3, 5:42 AM
Unknown Object (File)
Sun, Mar 3, 5:41 AM
Unknown Object (File)
Sun, Mar 3, 4:30 AM
Unknown Object (File)
Sun, Mar 3, 4:30 AM
Unknown Object (File)
Sun, Mar 3, 4:30 AM
Unknown Object (File)
Sun, Mar 3, 4:30 AM
Unknown Object (File)
Sun, Mar 3, 4:30 AM
Subscribers

Details

Summary

This differential implements a function that encrypts parts of iOS notification payload that are important to user's privacy

Test Plan
  1. Pull cookie of currently logged in user from MariaDB.
  2. Call this function in the keyserver code where iOS notification is constructed with hardcoded cookie id.
  3. Send notification to the physical iOS device. Log content of send notification and ensure relevant fields are encrypted.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I can't tell where these functions will be used, so it's a little harder to review

keyserver/src/push/crypto.js
1 ↗(On Diff #26469)

Newline here

38 ↗(On Diff #26469)

Can we save space and set this to 1?

42 ↗(On Diff #26469)

We can remove the async keyword here

46–48 ↗(On Diff #26469)

We can use a shorthand

52 ↗(On Diff #26469)

Does encryptIOSNotification need to be exported?

ashoat requested changes to this revision.May 15 2023, 9:57 AM

Back to you

This revision now requires changes to proceed.May 15 2023, 9:57 AM
  1. Validate every notification field before encryption attempt
  2. Encrypt more fields in notification payload

One request inline; please address before landing, or feel free to re-request review if you disagree or if I'm missing something

keyserver/src/push/crypto.js
41 ↗(On Diff #26554)

Can we construct a new Notification rather than mutating the existing one?

This revision is now accepted and ready to land.May 16 2023, 9:56 AM

Create new notification instead of mutating the existing one

I should have been more specific

keyserver/src/push/crypto.js
12 ↗(On Diff #26611)

Rather than cloning the existing notification, can you create a new empty one, and fill in its fields as necessary?

Return apn.Notifications object in case of encryption failure. Previous apprhac used '...' syntax, and it failed further since the returned object didn't have 'length()' method.

keyserver/src/push/crypto.js
12 ↗(On Diff #26651)

Let's construct a new apn.Notification instead of cloning the one we're passed in

Create new notification instead of copying

keyserver/src/push/crypto.js
33–34 ↗(On Diff #26784)

Nit: can we swap the order here?

39–40 ↗(On Diff #26784)

Nit: can we swap the order here?

75 ↗(On Diff #26784)

Is this line necessary now that we're no longer cloning the notification? I think undefined is the default value