Full XAPPLEPUSHSERVICE support - #1090
Conversation
2972d0f to
c2eb864
Compare
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 20 days. |
NickOvt
left a comment
There was a problem hiding this comment.
Blocking issues
-
Malformed XAPPLEPUSHSERVICE commands can crash the worker
In imap-core/lib/commands/xapplepushservice.js:84, the handler dereferences
attr.type without checking whether attr is null. For example, mailboxes NIL
is parsed as null and causes an uncaught TypeError. mailboxes (NIL) has the
same problem for list entries.Because command-handler execution is not wrapped in an exception guard, an
authenticated client can terminate the Node worker instead of receiving an
IMAP BAD response.Please validate attributes and list entries before accessing .type, and add
raw-command regression tests for NIL and other malformed values. -
Production inbound mail does not trigger APNs
MessageHandler creates its notifier with apn: options.apn || null, but APNs
is only injected by WildDuck’s IMAP, API, and LMTP entry points.Production inbound delivery uses haraka-plugin-wildduck, which constructs
its own MessageHandler without the apn option. Messages delivered through
Haraka therefore write journal entries but never call apn.notify(). The
LMTP path works, but it is not the normal production delivery path.This needs either coordinated Haraka plugin wiring or a centralized
notification consumer that observes journal/Redis events independently of
the process storing the message.
Additional correctness issues
-
Continuous mail traffic can postpone pushes indefinitely
In lib/apn-client.js:272, every notification clears and restarts the two-
second debounce timer. If messages arrive more frequently than every two
seconds, no push is sent until traffic stops.Please add a maximum coalescing delay or use a leading-edge timer so busy
accounts still receive timely pushes. -
Non-ASCII and case-variant mailbox names do not resolve
The registration handler queries MongoDB using mailbox names exactly as
received from the IMAP command. Unlike other mailbox commands,
XAPPLEPUSHSERVICE does not call normalizeMailbox().Without UTF8=ACCEPT, a mailbox such as Märkmed arrives in modified UTF-7
and cannot match the Unicode path stored in MongoDB. Case variants such as
Inbox also fail to match canonical INBOX.Please normalize/decode mailbox names before passing them to the
application handler and add modified UTF-7 and INBOX-case tests.
Test coverage
The new APNs client, notifier, registration, ID, and REST API tests pass, as
does npm run test:proto. However, coverage is still missing for:
- Raw malformed/NIL XAPPLEPUSHSERVICE commands.
- Production Haraka delivery.
- Sustained debounce traffic.
- Modified UTF-7 mailbox names.
- The successful enabled /pushsubscriptions/notify path.
- Webmail permission and deviceToken redaction behavior.
- User-deletion cleanup of push subscriptions.
The new public API routes also need regenerated API documentation.
Finally, this branch is currently 19 commits behind master. It appears to
merge cleanly, but master has substantial changes in message-handler and imap-
notifier, so the complete test suite should be rerun after rebasing.
This PR adds full XAPPLEPUSHSERVICE support, reusing the existing stub where applicable.
The APN client is meant to be as non-intrusive as possible, so that when APN is inaccessible for some reason, it wouldn't cause issues. The client throttles itself should a lot of failures occur.
New API endpoints for listing and deleting tokens and an admin route for manual testing.
Tested with proper APN certificate and it works with the latest iOS and Apple Mail.