Fix expiration time on remote notifications for Loop

[ps2]

Because APNS can sometimes deliver notifications late, it is critical that Loop not enact remote overrides if this happens. The previous method attempting to do this was not working as expected.

[sulkaharo]

Do you know if the logic on APNS to never deliver the message after expiration is rock solid? If not, having a field in the actual message that states when the message was generated would be a good precaution to filter the messages in case of reception after expiration. Also note that according to APNS docs, when the apns-expiration message is present, the message delivery is attempted multiple times until the system thinks it's delivered (as opposed to never redelivering when it's not sent) so adding this field could cause the message be delivered multiple times in case of an APNS error, so you probably want to also have a mechanism to never accept a message with the same originating timestamp multiple times.

Another potential breakage case is if:

1. User sends message, it fails to deliver
2. User thinks delivery failed, sends another 4 minutes later
3. APNS re-sends the first message 5 minutes later due to re-delivery mechanism being turned on due to expiration being enables -> user has now triggered 2 boluses

[ps2]

Yes, that's what this change does; it's putting the expiration date in the content of the message, so Loop can evaluate it independently.

[ps2]

Associated Loop PR here: LoopKit/Loop#1639

[ps2]

And yes, protection against multiple enactments of the same command is also a good safety mitigation, and we achieve that by only allowing one usage of a particular OTP code.

[sulkaharo]

Ah right, sorry I misread the change in inverse. 😅 I'd still personally implement the logic in a manner where the Nightscout entry just contains information on when it was entered and any decisions thereof (like expiration interval) is fully implemented in Loop side. That'd guarantee there'll never be logic that takes the expiry time and adjusts that based on conditions, as opposed to calculating an expiry time based on conditions from the source, which is a lot cleaner.

[bewest]

Since 5 minutes is hardcoded in, is this value ever likely to change, and would providing the origin request time be clearer? Currently this would apply to all requests, even requests to cancel previous overrides. I can easily adjust the patches here to provide a time that represents the creation of this notification. If it will help, I can provide matching patches to https://github.com/LoopKit/Loop/pull/1639/files, although I've never worked with swift.

[ps2]

In this case, remote control by a caregiver, the concerns about what constitutes a desired expiration time lie with the caregiver, so I would like to keep the protocol such that loop respects the expiration time dictated by the command. This allows, for instance, a future loop follow client to dictate different expiration tolerances for different commands, which might have different risks associated with different amounts of latency.

…

Sent from my iPhone

On Mar 15, 2022, at 10:59 AM, Ben West ***@***.***> wrote:  Since 5 minutes is hardcoded in, is this value ever likely to change, and would providing the origin request time be clearer? Currently this would apply to all requests, even requests to cancel previous overrides. I can easily adjust the patches here to provide a time that represents the creation of this notification. If it will help, I can provide matching patches to https://github.com/LoopKit/Loop/pull/1639/files, although I've never worked with swift. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.

[bewest]

We can have both expiration and creation times. Having both allows another way to establish data integrity for these requests. Adding some kind of version for this schema seems wise as well, even if it's 0.0.0.

[ps2]

Yeah, no issues with adding creation time.

…

Sent from my iPhone

On Mar 15, 2022, at 11:13 AM, Ben West ***@***.***> wrote:  We can have both expiration and creation times. Having both allows another way to establish data integrity for these requests. Adding some kind of version for this schema seems wise as well, even if it's 0.0.0. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.

[bewest]

How would you feel about renaming expiration to notify_until?

[ps2]

Would prefer expiration. notify_until connotes a kind of ongoing notification.

[bewest]

Fair enough, I ran into that when trying to amend the comment. What about adding setting version to 0.0.0?

index 66d04476..18deff51 100644
--- a/lib/server/loop.js
+++ b/lib/server/loop.js
@@ -112,9 +112,13 @@ function init (env, ctx) {
       alert += " - " + data.enteredBy
     }
 
+    let originated_at = new Date( );
     // Expire after 5 minutes.
-    let expiration = new Date((new Date()).getTime() + 5 * 60 * 1000)
-    payload['expiration'] = expiration.toISOString();
+    let notify_until = new Date(originated_at.getTime() + 5 * 60 * 1000);
+    payload['originated_at'] = originated_at.toISOString();
+    payload['notify_until'] = notify_until.toISOString();
+    // pull the attributes needed, ignore everything else
+    payload['version'] = '0.0.0';
 
     let notification = new apn.Notification();
     notification.alert = alert;

[ps2]

Not opposed to a version, though it can be delayed until we have a breaking change, and added then (lack of version implies version 0). I do think 3 part semantic versioning style is way overkill, though. :)

[ps2]

This issue is mitigated somewhat by the fact that remote bolus and remote carbs only accept the two most recent OTP codes, which is a window of at most 1 minute. It's still a good idea to get this in so that overrides are not enacted late as well.

[bewest]

Thanks for the additional information. I think Sulka and I would feel better if you could add either originated_at or created_at to capture the original time this message was authored. In the event that some uncontrolled code sets a questionable expiration, or people start questioning the source of Loop's actions, we'll want to know when Nightscout created these messages.

I have a few questions about the connectivity and security stories here besides the expiration. Where does the OTP code come from, can it come form Google Authenticator app, etc... Can authorization to perform these actions be encoded in a signed JWT, the way we do for other permissions?

[ps2]

Thanks @bewest. Yes, I'll add a sent_at field for tracking when this was sent.

The OTP codes are standard RFC 4226/RFC 6238 codes that can be generated from Google Auth, iOS keychain, etc, using a pre shared secret, that Loop can display using a qrcode for scanning.

As for your second question about authorization to perform these actions, that might be beyond my NS understanding. I have put the post request behind the authorization system like this:

api.post('/loop', ctx.authorization.isPermitted('notifications:loop:push'), function (req, res) {

Is there more that should be done?

[ps2]

I pushed the requested updates, but I noticed that some of the tests failed. I looked through the ci logs, and the failure doesn't appear to be related to my changes. I also ran the tests locally, and they ran fine. Any ideas? Are there some indeterminant tests? Can the tests be re-run?

[ps2]

Thanks for kicking the tests! Looks like they're passing now.

[bewest]

Thanks for all the contributions and additional information, @ps2. I was wondering how people would use an OTP code if it could only come from Loop and now understand that Loop generates the shared secret. Thanks for the clarification!