Add webxdc notification handler #4400

nicodh · 2024-12-10T07:46:29Z

related to #4366
Example for calendar notification

click opens webxdc app with href if provided

Internal changes:
new flag isWebxdcInfo for DcNotification

WofWca

The idea of the feature itself is great, and this MR seems like a good implementation of it code-wise. But design-wise:

I don't like the idea of opening the app on notification click, at least by default.
At the very least a notification should have a separate "Launch app" button to do it.

The "How private are webxdc apps?" section states:

This also means: it can be a privacy risk to open webxdc apps in chats where you don’t trust the members - as you know it from e-mail attachments, where you only open attachments from senders you trust, and not from spammers.

So this is like opening an attachment right from a notification. Especially with real-time channels now enabled by default (which can leak your real IP). I expressed a similar concern in #4366 (comment).
And, correct me if I'm wrong, such notifications would be displayed even from people who aren't on your contact list? Never mind the contact list: with Chatmail, people share their invite codes, which bypasses the "contact request" thing.
And the other person can even completely control the contents of the notification, the text and the icon, judging by the screenshot, which is powerful for phishing.

Exposing your real IP and system info to anyone on the internet at a click of a button is not great.

What do you think?

packages/target-electron/src/notifications.ts

packages/frontend/src/components/RuntimeAdapter.tsx

nicodh · 2024-12-11T13:34:39Z

Exposing your real IP and system info to anyone on the internet at a click of a button is not great.

What do you think?

Well it's not "anyone" and the information on the notification is clear. The icon comes from a webxdc app in a shared chat, not just from the message.

I remember there was a discussion on that and assumed the DC_EVENT_INCOMING_WEBXDC_NOTIFY should work like that.
Maybe @r10s knows more.

But we also could restrict the direct access to the case where the webxdc app is already opened.

r10s · 2024-12-11T14:02:24Z

on android/ios we're opening the chat with the webxdc-info-message or the webxdc-app-instance instead of opening the app directly.

we thought about opening the app directly, but then did not came to that at the end, idea was, however, to add an explicit "Start" button, but maybe it is also totally fine as it is now. we will know after some experience and real-world use.

so, i would suggest to do the same on desktop, sorry for not being clearer in the issue.

that would postpone that discussion about privacy (where we should also keep in mind, that every normal looking link send in a normal message leaks the IP-address easier. and this does not even discuss the real-world-attack vector of an IP-Address, VPN, realtime-disabled and so on ;)

if a chat is a contact request, so "untrusted", i would not send a notification at all (tbh, not totally sure if that is already the case on android, but otherwise, that'd might be regarded as a bug)

nicodh · 2024-12-11T14:09:39Z

Ok so instead calling openwebxdc we just jump to the message as usual.

r10s · 2024-12-11T14:10:05Z

yes, that seems to be the easier thing for now

resolves #4366 - add new param isWebxdcInfo to DcNotification - openWebxdc on click

WofWca · 2024-12-11T14:36:57Z

Well it's not "anyone" and the information on the notification is clear. The icon comes from a webxdc app in a shared chat, not just from the message.

Anyone can send an email, and they can make an app with whatever name and icon they like.

if a chat is a contact request, so "untrusted"

In my experience, contacts don't get marked as "requests" if they have your invite link, and invite links are supposedly not considered sensitive info: For example, people post their invite links in https://deltachat-bot.github.io/public-bots/#/home

i would suggest to do the same on desktop

Yes, that's better.

- don't open webxdc on notification click

packages/target-electron/static/webxdc-preload.js

packages/target-electron/src/deltachat/webxdc.ts

packages/frontend/src/system-integration/notifications.ts

packages/shared/shared-types.d.ts

packages/target-electron/runtime-electron/runtime.ts

packages/target-browser/runtime-browser/runtime.ts

packages/frontend/src/system-integration/notifications.ts

- pass event text to notification

packages/frontend/src/system-integration/notifications.ts

packages/target-browser/runtime-browser/runtime.ts

WofWca

I did some testing. There seems to be a problem with when we call flushNotifications(), which is the only function that calls showNotification(). flushNotifications() is only called from the IncomingMsgBunch event handler.

Reproduction steps:

Create two accounts, and create a group with those two accounts.
Send the Calendar app to the group
Send a regular message to the group. You'll get a notification on another account.
Create an event in the calendar.
You won't get a notification, but I assume you should.
Create another event in the calendar.
You will get a notification for the previous event!
Send a regular message.
You'll first get a notification for the second calendar event, and then a notification for the regular message!

Update: otherwise this looks better than the previous version, much simpler.

packages/frontend/src/system-integration/notifications.ts

nicodh · 2024-12-13T17:51:56Z

I did some testing. There seems to be a problem with when we call flushNotifications(), which is the only function that calls showNotification(). flushNotifications() is only called from the IncomingMsgBunch event handler.

Well when debugging it seems that in the group the IncomingWebxdcNotify event is not triggered at all. If you set a break point on frontend/../notifications in line 28
BackendRemote.on('IncomingWebxdcNotify', (accountId, { msgId, text }) => {
incomingWebxdcEventHandler(accountId, msgId, text)
})
Then you see there is no IncomingWebxdcNotify event triggered in the group, while in a 1:1 chat it is

WofWca · 2024-12-13T18:24:39Z

Well when debugging it seems that in the group the IncomingWebxdcNotify event is not triggered at all

Are you sure? It's not the case for me. IncomingWebxdcNotify gets triggered, but the notification is not there.

2024-12-13-bYubjUH2As.mp4

I got it. It's because the handler is slow now, so pushing to the notifications queue happens after the IncomingMsgBunch event.

IncomingWebxdcNotify 2 1 created "29" on Jan 25, 2025
22:22:54.855 notifications.ts:31 IncomingMsgBunch
22:22:54.856 notifications.ts:87 queuedNotifications[accountId].push

nicodh · 2024-12-13T18:57:26Z

I got it. It's because the handler is slow now, so pushing to the notifications queue happens after the IncomingMsgBunch event.

Not for me. I found that the missing IncomingWebxdcNotify was caused by a calendar that was added to the group before the other test user was added. So that was correct.

But I can't reproduce your issue...?

nicodh · 2024-12-13T18:59:11Z

But it is easly to imagine that fetching messages takes some time...

nicodh · 2024-12-14T09:54:08Z

Added a solution for the queue. Based on https://stackoverflow.com/questions/53540348/js-async-await-tasks-queue (which is a very profound investigation of the queue topic). Would be much easier with rxjs (just call next() on a Subject() while others can subscribe to it) but not worth to introduce a new library here

nicodh · 2024-12-14T15:56:36Z

Removed the event queue in favor of a more simple solution

Simon-Laux · 2024-12-14T17:34:25Z

packages/frontend/src/system-integration/notifications.ts

+      const summaryText = eventText ?? notificationInfo.summaryText
+      const chatName = notificationInfo.chatName
+      let icon = getNotificationIcon(notificationInfo)
+      if (isWebxdcInfo && chatId === -1) {


why is chatID -1 here?

ah because it isn't there yet.... I'd rather use null or undefined to represent that state.
Or add the chat id to the IncomingWebxdcNotify event in core.

thinking about it more it makes sense to add it to the event, because It already has contact id.

https://github.com/deltachat/deltachat-core-rust/blob/191eb7efdd2765af0b9440a1316e850ed321b2b2/src/events/payload.rs#L111-L123

packages/runtime/runtime.ts

Simon-Laux

code looks ok, didn't test.

packages/frontend/src/system-integration/notifications.ts

Co-authored-by: Simon Laux <[email protected]>

WofWca

Would be nice to clean up commit history in order to do a fast-forward merge so that refactoring changes and different functional changes are not mixed, but it's not super important.

I tested this for a bit. Works alright, and the issues are resolved!

WofWca · 2024-12-16T10:44:33Z

packages/frontend/src/system-integration/notifications.ts

@@ -174,6 +247,14 @@ async function flushNotifications(accountId: number) {
  let notifications = [...queuedNotifications[accountId]]
  queuedNotifications = []

+  for await (const n of notifications) {


for await is no better than a regular for of here, right?

packages/frontend/src/system-integration/notifications.ts

nicodh marked this pull request as draft December 10, 2024 07:46

nicodh force-pushed the add-webxdc-notification-handler branch from 69dc59f to 55c574b Compare December 10, 2024 07:55

nicodh marked this pull request as ready for review December 11, 2024 07:11

nicodh requested review from WofWca, Simon-Laux and r10s December 11, 2024 07:11

WofWca reviewed Dec 11, 2024

View reviewed changes

packages/target-electron/src/notifications.ts Outdated Show resolved Hide resolved

packages/frontend/src/components/RuntimeAdapter.tsx Outdated Show resolved Hide resolved

packages/frontend/src/components/RuntimeAdapter.tsx Outdated Show resolved Hide resolved

nicodh added 3 commits December 11, 2024 15:17

Handle webxdc notifications

164d35a

resolves #4366 - add new param isWebxdcInfo to DcNotification - openWebxdc on click

Add test-results to .prettierignore

91ef68c

Add doc comments in notifications

3edca1f

nicodh force-pushed the add-webxdc-notification-handler branch from 5b46284 to 3edca1f Compare December 11, 2024 14:17

nicodh added 2 commits December 11, 2024 15:50

Expose sendUpdateInterval & sendUpdateMaxSize

a0a45f9

- don't open webxdc on notification click

Update CHANGELOG

beb8061

WofWca reviewed Dec 11, 2024

View reviewed changes

packages/frontend/src/system-integration/notifications.ts Outdated Show resolved Hide resolved

nicodh mentioned this pull request Dec 12, 2024

add webxdc selfAddr/notify/href/ratelimit APIs (M2/M3) #4366

Closed

6 tasks

nicodh and others added 2 commits December 12, 2024 12:23

Remove isWebxdcInfo from DcNotification as it is not needed any more

3beb3ef

- pass event text to notification

fix: webxdc notifications in browser version

c9760ef

WofWca reviewed Dec 13, 2024

View reviewed changes

packages/frontend/src/system-integration/notifications.ts Outdated Show resolved Hide resolved

packages/target-browser/runtime-browser/runtime.ts Outdated Show resolved Hide resolved

WofWca reviewed Dec 13, 2024

View reviewed changes

packages/frontend/src/system-integration/notifications.ts Show resolved Hide resolved

Pass eventText to showNotification

c86c3ab

nicodh temporarily deployed to test December 14, 2024 09:45 — with GitHub Actions Inactive

Add queue to garantuee eventhandler order

d12a326

nicodh force-pushed the add-webxdc-notification-handler branch from 620571c to d12a326 Compare December 14, 2024 09:56

nicodh had a problem deploying to test December 14, 2024 09:56 — with GitHub Actions Failure

nicodh temporarily deployed to test December 14, 2024 09:56 — with GitHub Actions Inactive

nicodh temporarily deployed to test December 14, 2024 10:01 — with GitHub Actions Inactive

Simplify event handling

333f816

nicodh temporarily deployed to test December 14, 2024 14:59 — with GitHub Actions Inactive

nicodh requested a review from WofWca December 14, 2024 15:54

Simon-Laux reviewed Dec 14, 2024

View reviewed changes

packages/runtime/runtime.ts Outdated Show resolved Hide resolved

Simon-Laux approved these changes Dec 14, 2024

View reviewed changes

WofWca reviewed Dec 15, 2024

View reviewed changes

packages/frontend/src/system-integration/notifications.ts Outdated Show resolved Hide resolved

Remove chatId condition

c173f6a

nicodh temporarily deployed to test December 15, 2024 19:53 — with GitHub Actions Inactive

Update comment in runtime

01383fc

Co-authored-by: Simon Laux <[email protected]>

nicodh temporarily deployed to test December 16, 2024 06:22 — with GitHub Actions Inactive

WofWca approved these changes Dec 16, 2024

View reviewed changes

nicodh merged commit d90e328 into main Dec 16, 2024
14 checks passed

nicodh deleted the add-webxdc-notification-handler branch December 16, 2024 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webxdc notification handler #4400

Add webxdc notification handler #4400

nicodh commented Dec 10, 2024 •

edited

Loading

WofWca left a comment

nicodh commented Dec 11, 2024

r10s commented Dec 11, 2024 •

edited

Loading

nicodh commented Dec 11, 2024

r10s commented Dec 11, 2024

WofWca commented Dec 11, 2024

WofWca left a comment •

edited

Loading

nicodh commented Dec 13, 2024

WofWca commented Dec 13, 2024

nicodh commented Dec 13, 2024

nicodh commented Dec 13, 2024

nicodh commented Dec 14, 2024

nicodh commented Dec 14, 2024

Simon-Laux Dec 14, 2024

Simon-Laux Dec 14, 2024

Simon-Laux Dec 14, 2024 •

edited

Loading

Simon-Laux left a comment

WofWca left a comment

WofWca Dec 16, 2024

Add webxdc notification handler #4400

Add webxdc notification handler #4400

Conversation

nicodh commented Dec 10, 2024 • edited Loading

WofWca left a comment

Choose a reason for hiding this comment

nicodh commented Dec 11, 2024

r10s commented Dec 11, 2024 • edited Loading

nicodh commented Dec 11, 2024

r10s commented Dec 11, 2024

WofWca commented Dec 11, 2024

WofWca left a comment • edited Loading

Choose a reason for hiding this comment

nicodh commented Dec 13, 2024

WofWca commented Dec 13, 2024

nicodh commented Dec 13, 2024

nicodh commented Dec 13, 2024

nicodh commented Dec 14, 2024

nicodh commented Dec 14, 2024

Simon-Laux Dec 14, 2024

Choose a reason for hiding this comment

Simon-Laux Dec 14, 2024

Choose a reason for hiding this comment

Simon-Laux Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Simon-Laux left a comment

Choose a reason for hiding this comment

WofWca left a comment

Choose a reason for hiding this comment

WofWca Dec 16, 2024

Choose a reason for hiding this comment

nicodh commented Dec 10, 2024 •

edited

Loading

r10s commented Dec 11, 2024 •

edited

Loading

WofWca left a comment •

edited

Loading

Simon-Laux Dec 14, 2024 •

edited

Loading