Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature] Push notifications #3587

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

[feature] Push notifications #3587

wants to merge 27 commits into from

Conversation

VyrCossont
Copy link
Contributor

@VyrCossont VyrCossont commented Dec 1, 2024

Description

This pull request implements Web Push notifications.

Closes #1350

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have not leveraged AI to create the proposed changes.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

TODO

  • Notify admins when report is created
  • Implement notification policy part of API
  • Test with stub Web Push server
  • Test with Feditext (ID decoding fixed, but other problems exist, see below)
  • Test with Phanpy
    • Safari
    • Firefox
    • Chrome
    • Arc
    • Edge for Mac (Phanpy Web Push doesn't work, seems like a Microsoft problem)
    • Edge for Windows
  • Test with Tusky
  • Fix report titles
  • Cap report body size
  • Stop sending to a Web Push server if HTTP 4xx status codes not related to rate limit or timeout are received (Mastodon and Akkoma both do this)
  • Remove cleartext HTTP URL scheme
  • Try using Topic header for multiple updates to the same status (probably pointless until edits are implemented)

@VyrCossont
Copy link
Contributor Author

In process of testing with Phanpy (using Safari with Apple's Web Push server):

timestamp="01/12/2024 20:53:13.122" func=webpush.(*realSender).Send.func1 level=ERROR msg="error sending Web Push notification for subscription with token ID 01DAD4B0R9CHH0GRSS4RDGG4NV: sendToSubscription: unexpected HTTP status 403 Forbidden received when sending Web Push notification: {\"reason\":\"BadJwtToken\"}"

and Feditext:

timestamp="01/12/2024 20:53:13.048" func=httpclient.(*Client).DoOnce level=ERROR method=POST url=https://apns.feditext.com/push/67b68e16b1ec74cb00502345f647e11a3184de1ac6b90468fa4bb405585aafa2/61A54241-6446-4A26-A334-921B611E52F5 contentType=application/octet-stream attempt="1" msg="http response: 500 Internal Server Error"

@VyrCossont
Copy link
Contributor Author

@tsmethurst
Copy link
Contributor

Looking great so far, very exciting :)

@daenney daenney changed the title Push notifications [feature] Push notifications Dec 2, 2024
@VyrCossont
Copy link
Contributor Author

VyrCossont commented Dec 7, 2024

So here's a fun thing I just learned:

  • Mastodon sends extra HTTP headers called Encryption and Crypto-Key that existed in early versions of RFC 8188 Encrypted Content-Encoding for HTTP but do not exist in the final version of RFC 8188. It also uses the draft Content-Encoding: aesgcm header instead of the final Content-Encoding: aes128gcm
  • Feditext's APNS relay expects those headers
  • webpush-go does not send those headers
  • The headers were transformed into (equivalent?) header fields at the start of the binary content blob in the final version of the RFC, in which case the bug is arguably with Feditext's APNS relay, but other Web Push -> native relays may also expect the old headers and the draft Content-Encoding: aesgcm, definitely including Toot!'s
    • Tusky apparently does Web Push support through UnifiedPush. PushNotificationHelper.kt suggests that UnifiedPush doesn't work with Mastodon's obsolete version of Web Push encryption, in which case implementing Web Push correctly might allow Tusky to do less network I/O when working with GtS in the future.

@VyrCossont
Copy link
Contributor Author

Horrible thought: we could have both standards-compliant Web Push encryption and Mastodon Clown Car Web Push encryption implementations and send both for every Web Push notification, or at least until we get an error back from the Web Push server that implies we should use one or the other

@leo60228
Copy link

leo60228 commented Dec 7, 2024

This is not a Mastodon specific issue. The web-push JS library which is cited by both Mozilla and Google and is seemingly some sort of reference implementation still defaults to using these headers and requires explicit opt-in ({contentEncoding: 'aes128gcm'}) to use the final encryption standard (this is a documentation mistake). Google's documentation also says to include them, despite citing the final specs.

@VyrCossont
Copy link
Contributor Author

VyrCossont commented Dec 7, 2024

Sounds like I need to check whether Mozilla and Google Web Push servers can take aes128gcm. Hopefully they do, in which case we don't have to implement aesgcm.

@VyrCossont
Copy link
Contributor Author

webpush-go last supported aesgcm five years ago, but the code's there if we need it: https://github.com/SherClockHolmes/webpush-go/tree/52c6af17b8d924a03af32c85ffe7077b162e3dec

@tsmethurst
Copy link
Contributor

Let me know if you need any help with this / want an NTFY set up on superseriousbusiness.org or anything like that :) You seem to be pushing through it fine but I'm happy to help if I can!

@VyrCossont
Copy link
Contributor Author

So we apparently don't need to worry about Chrome or Firefox (at least the name-brand versions, as their push servers handle aes128gcm just fine.

Safari is reporting problems with the (VAPID) JWT, and it looks like webpush-go always prepends "mailto:" to whatever string you give it for a subject. This is used for contact information and is supposed to either take a mailto: or https: URL. I hate email and my GtS server doesn't have a contact email configured, so I'd elected to provide the instance's https: URL as contact info, which would result in a malformed mailto: URL actually being used for the sub claim by webpush-go. I'll try giving it a dummy email if no contact email is available, and if that works, I'll open a webpush-go bug to allow https: subjects somehow.

@VyrCossont
Copy link
Contributor Author

Yep, that worked, we're good on Safari now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] WebPush push notification support
4 participants