iOS notifications can lose permission to push if the user is currently active

Bug
1

iOS notifications can lose permission to push if the notification is suppressed. Discourse’s code is configured to optionally suppress notifications, and will thereby lose permission to push when this happens three times.

Here’s the code for the push-notification service worker.

This code has a critical bug in it, at line 178. There, the service worker checks to see if the user is active (i.e. not idle). In that case, the push event returns false without showing a notification.

(It also checks payload.hide_when_active, but it turns out that hide_when_active is always true, and so this code always returns false when the user is active.)

Apple forbids silent pushes, revoking push permission after three events that don’t show notifications

This is not acceptable under Apple’s rules for push notifications.

https://webkit.org/blog/12945/meet-web-push/

Power and privacy

Both the WebKit open source project and Apple treat privacy as a fundamental human right. As with other privileged features of the web platform, requesting a push subscription requires an explicit user gesture. It also requires you set the userVisibleOnly flag to true, and fulfill that promise by always showing a notification in response to a push message.

The Web Push API is not an invitation for silent background runtime, as that would both violate a user’s trust and impact a user’s battery life.

Violations of the userVisibleOnly promise will result in a push subscription being revoked.

(emphasis mine)

This is explained in further detail in Apple’s WWDC video on Web Pushes, at 9:57

https://developer.apple.com/videos/play/wwdc2022/10098/?time=596

Notice we are explicitly stating that we promise to always make pushes user visible. While the standard for the JavaScript Push API optionally accommodates silent JavaScript runtime in response to a push, most browsers do not support that. Safari does not support that.

… and then at 13:35:

https://developer.apple.com/videos/play/wwdc2022/10098/?time=814

As mentioned when I showed you the code on how to request a push subscription, you must promise that pushes will be user visible. Handling a push event is not an invitation for your JavaScript to get silent background runtime. Doing so would violate both a user’s trust and a user’s battery life. When handling a push event, you are in fact required to post a notification to Notification Center. Other browsers all have countermeasures against violating the promise to make pushes user visible, and so does Safari. In the beta build of macOS Ventura, after three push events where you fail to post a notification in a timely manner, your site’s push subscription will be revoked. You will need to go through the permission workflow again.

(emphasis mine)

Apple recommends showing notifications immediately, not after closing notifications

Apple’s recommended code looks like this, at 11:39:

https://developer.apple.com/videos/play/wwdc2022/10098/?time=699

self.addEventListener('push', (event) => {
    let pushMessageJSON = event.data.json();

    // Our server puts everything needed to show the notification
    // in our JSON data.
    event.waitUntil(self.registration.showNotification(pushMessageJSON.title, {
        body: pushMessageJSON.body,
        tag: pushMessageJSON.tag,
        actions: [{
            action: pushMessageJSON.actionURL,
            title: pushMessageJSON.actionTitle,
        }]
    }));
}

Remember how when we subscribed for push, our JavaScript promised they would always be user visible? That means we must always show a platform native notification in response to each push. It is best to do this as early as possible in your push event handler.

Discourse’s code is not following the recommended best practice. Discourse’s code is first closing all notifications, and only then showing a notification.

Discourse should always call showNotification in response to a push event, and it should always do it as soon as possible.

8 Likes
2

@Falco / @featheredtoast thoughts on this?

What impact would removing both the closing and idle check have?

Overall I am not sure at all about this idle check, feels like it is just causing confusion.

Worst case if we require this on Android or Desktop (non Safari) we can always add a conditional here, but my feeling here is that we should just remove code here.

Excellent debugging @dfabulich :hugs:

3 Likes
4

I note that it’s OK to do the closing, I guess, as long as it’s done after calling showNotification.

… but I honestly don’t see the point of closing notifications at all. Might as well just let them pile up in the notification tray, IMO.

Note: This API shouldn’t be used just to have the notification removed from the screen after a fixed delay since this method will also remove the notification from any notification tray, preventing users from interacting with it after it was initially shown. A valid use for this API would be to remove a notification that is no longer relevant (e.g. the user already read the notification on the webpage in the case of a messaging app or the following song is already playing in a music app).

3 Likes
5

From what I’ve read on this subject over the weekend we can’t even use async/promises in the push event handler, must show it ASAP or Apple will revoke your notification rights.

This all tracks with what @dfabulich says.

I think we’re being too “smart” for our sake here, we should indeed remove all checks and just show th notification.

6 Likes
6

Can’t thank you enough @dfabulich for discovering this!

3 Likes
7

OK I have a PR for this.

@Falco / @featheredtoast should we merge it?

If we need to skip push notifications we got to do it on the server not the client. Collapsing logic was always somewhat questionable, apps don’t tend to do that.

2 Likes
8

Ohh yes it does make much more sense to only push notifications when they need to be shown. Should squelch excessive notifications elsewhere eventual-soon, but I’m good with merging this here :+1:

Re: collapsing notifications, it’s a shame to lose but… ideally we’d be able to do similar to other apps’ auto dismissal as a user makes their way through notifications on any logged in device, but that might take some extra gold plating. The intent is the same there to never have just a tide of stale notifications piling its way up. Again, it’s fine, but we need to revisit later, depending on how much pain it causes.

2 Likes