Send a filter the server can read
Enabling background notifications failed with "Invalid filter". The
subscription asked to be notified about mail matching:
filter: { inMailbox: null, notKeyword: "$seen" }
`inMailbox: null` meant "the inbox" in my head and nothing at all to
Stalwart, which needs a mailbox id there. It refused the whole
subscription, so the feature did not work at all for anyone who tried
it.
The Inbox's id is now passed in and used. Where it is not known the
condition is left out rather than sent empty: notifying more widely is a
worse default than filtering to the Inbox, but it is a working one, and
sending a malformed filter is not a fallback.
Two reasons this got out, both worth fixing rather than just the bug:
- The tests checked the properties list and its ordering, and never
looked at the filter. There is now one that walks every condition
and fails on a null or undefined value, for both the known-inbox and
unknown-inbox cases.
- The mock accepted it happily, so nothing local disagreed with the
code. It now refuses a filter condition with a null value and
answers "Invalid filter.", which is what the live server said.
Reproduced: the old payload is rejected, the new one accepted.
This commit is contained in:
+15
-3
@@ -105,8 +105,17 @@ export function deviceClientId(): string {
|
||||
}
|
||||
}
|
||||
|
||||
/** What to send Stalwart for a browser subscription. */
|
||||
export function subscriptionPayload(sub: PushSubscription, accountId: Id | null): Record<string, unknown> {
|
||||
/**
|
||||
* What to send Stalwart for a browser subscription.
|
||||
*
|
||||
* `inboxId` is the Inbox's mailbox id. It is a parameter rather than something
|
||||
* looked up here because an `inMailbox` condition needs a real id: the first
|
||||
* version of this passed `null`, meaning "the inbox" in the author's head and
|
||||
* nothing at all to the server, which answered "Invalid filter" and refused the
|
||||
* whole subscription. Without an id the filter simply leaves `inMailbox` out
|
||||
* and notifies more widely, which is a worse default but a working one.
|
||||
*/
|
||||
export function subscriptionPayload(sub: PushSubscription, accountId: Id | null, inboxId: Id | null = null): Record<string, unknown> {
|
||||
const json = sub.toJSON();
|
||||
const body: Record<string, unknown> = {
|
||||
deviceClientId: deviceClientId(),
|
||||
@@ -121,7 +130,10 @@ export function subscriptionPayload(sub: PushSubscription, accountId: Id | null)
|
||||
[accountId]: {
|
||||
// Only mail that actually lands in the inbox. Filtering here rather
|
||||
// than in the service worker means spam never leaves the server.
|
||||
filter: { inMailbox: null, notKeyword: "$seen" },
|
||||
// Unread mail only, and only in the Inbox when we know which it is.
|
||||
// Filtering here rather than in the service worker means spam and
|
||||
// filed mail never leave the server at all.
|
||||
filter: { ...(inboxId ? { inMailbox: inboxId } : {}), notKeyword: "$seen" },
|
||||
properties: PAYLOAD_PROPS,
|
||||
urgency: "normal",
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user