Page MenuHomePhabricator

Implement function to encrypt relevant parts of iOS notification

Authored by marcin on May 15 2023, 8:18 AM.
Referenced Files
Unknown Object (File)
Sat, Nov 18, 3:05 AM
Unknown Object (File)
Tue, Nov 14, 12:33 PM
Unknown Object (File)
Sun, Nov 12, 4:58 PM
Unknown Object (File)
Sat, Nov 11, 5:15 PM
Unknown Object (File)
Wed, Nov 8, 1:42 PM
Unknown Object (File)
Oct 22 2023, 9:59 AM
Unknown Object (File)
Oct 22 2023, 9:59 AM
Unknown Object (File)
Oct 22 2023, 9:59 AM



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

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

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

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

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.

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

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