From e78a742ee51d749062aa07989616b55d1689d108 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 11:51:31 +0100 Subject: [PATCH 01/16] add ttl option for web pushkin with default of 15 minutes --- sygnal/webpushpushkin.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index ea73d544..af1abf4e 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -191,7 +191,6 @@ async def _dispatch_notification_unlimited(self, n, device, context): response_text = (await readBody(response)).decode() finally: self.connection_semaphore.release() - # assume 4xx is permanent and 5xx is temporary if 400 <= response.code < 500: logger.warn( @@ -202,6 +201,13 @@ async def _dispatch_notification_unlimited(self, n, device, context): response_text, ) return [device.pushkey] + logger.info( + "Sent! pushkey %s; gateway %s response: %d: %s", + device.pushkey, + endpoint_domain, + response.code, + response_text, + ) return [] @staticmethod From ed0cc9ced40727e636b92c6965b047ff9846d5c2 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 11:56:33 +0100 Subject: [PATCH 02/16] changelog --- changelog.d/185.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/185.bugfix diff --git a/changelog.d/185.bugfix b/changelog.d/185.bugfix new file mode 100644 index 00000000..dfd5a4e1 --- /dev/null +++ b/changelog.d/185.bugfix @@ -0,0 +1 @@ +Add `ttl` option as understood config option for WebPush pushkins to make delivery more reliable \ No newline at end of file From 640a6ac25a9191910c3e781d90b12c8021d797b1 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 11:57:34 +0100 Subject: [PATCH 03/16] remove debug logging --- sygnal/webpushpushkin.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index af1abf4e..73d99b64 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -201,13 +201,6 @@ async def _dispatch_notification_unlimited(self, n, device, context): response_text, ) return [device.pushkey] - logger.info( - "Sent! pushkey %s; gateway %s response: %d: %s", - device.pushkey, - endpoint_domain, - response.code, - response_text, - ) return [] @staticmethod From e032998478063ca84764186da7d25b956a9db571 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 11:58:32 +0100 Subject: [PATCH 04/16] keep newline --- sygnal/webpushpushkin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 73d99b64..3991193e 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -191,6 +191,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): response_text = (await readBody(response)).decode() finally: self.connection_semaphore.release() + # assume 4xx is permanent and 5xx is temporary if 400 <= response.code < 500: logger.warn( From ab95ae90a6872cf6d45aabbea8af7d3516f62c56 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 11:59:06 +0100 Subject: [PATCH 05/16] whitespace --- sygnal/webpushpushkin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 3991193e..ea73d544 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -191,7 +191,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): response_text = (await readBody(response)).decode() finally: self.connection_semaphore.release() - + # assume 4xx is permanent and 5xx is temporary if 400 <= response.code < 500: logger.warn( From d8871160e50d35300f710ba7ebf1b014eaa40f71 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 14:59:08 +0100 Subject: [PATCH 06/16] only reject pushkey on 404 or 410, add logging --- sygnal/webpushpushkin.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index ea73d544..c4208342 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -192,16 +192,33 @@ async def _dispatch_notification_unlimited(self, n, device, context): finally: self.connection_semaphore.release() - # assume 4xx is permanent and 5xx is temporary - if 400 <= response.code < 500: + # permanent errors + if response.code is 404 or response.code is 410: logger.warn( - "Rejecting pushkey %s; gateway %s failed with %d: %s", + "Rejecting pushkey %s; subscription is invalid on %s: %d: %s", device.pushkey, endpoint_domain, response.code, response_text, ) return [device.pushkey] + # and temporary ones + if response.code >= 400: + logger.warn( + "webpush request failed for pushkey %s; %s responded with %d: %s", + device.pushkey, + endpoint_domain, + response.code, + response_text, + ) + elif response.code is not 201: + logger.info( + "webpush request for pushkey %s didn't respond with 201; %s responded with %d: %s", + device.pushkey, + endpoint_domain, + response.code, + response_text, + ) return [] @staticmethod From 8b25dfaf1024e4f6031ae1ef401bd393c3237938 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 16:17:34 +0100 Subject: [PATCH 07/16] support events_only push data flag to block messages without event_id --- docs/applications.md | 6 ++++++ sygnal/webpushpushkin.py | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/docs/applications.md b/docs/applications.md index 3db22809..0e17012c 100644 --- a/docs/applications.md +++ b/docs/applications.md @@ -253,6 +253,12 @@ any properties set in it will be present in the push messages you receive, so it can be used to pass identifiers specific to your client (like which account the notification is for). +##### events_only + +As of the time of writing, all webpush-supporting browsers require you to set `userVisibleOnly: true` when calling (`pushManager.subscribe`)[https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe], to (prevent abusing webpush to track users)[https://goo.gl/yqv4Q4] without their knowledge. With this (mandatory) flag, the browser will show a "site has been updated in the background" notification if no notifications are visible after your service worker processes a `push` event. This can easily happen when sygnal sends a push message to clear the unread count, which is not specific to an event. With `events_only: true` in the pusher data, sygnal won't forward any push message without a event id. This prevents your service worker being forced to show a notification to push messages that clear the unread count. + +##### Multiple pushers on one origin + Also note that because you can only have one push subscription per service worker, and hence per origin, you might create pushers for different accounts with the same p256dh push key. To prevent the server from removing other pushers with the same diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index c4208342..a7eff794 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -133,6 +133,11 @@ async def _dispatch_notification_unlimited(self, n, device, context): ) return [device.pushkey] + # drop notifications without an event id if requested, + # see https://github.com/matrix-org/sygnal/issues/186 + if device.data.get("events_only") is True and not n.event_id: + return []; + endpoint = device.data.get("endpoint") auth = device.data.get("auth") endpoint_domain = urlparse(endpoint).netloc From b1a80d60a62fd991c5838def46724ac4efe52ae8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 18:11:35 +0100 Subject: [PATCH 08/16] "is" should apparently only be used with variables, not literals --- sygnal/webpushpushkin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index a7eff794..865e687a 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -198,7 +198,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): self.connection_semaphore.release() # permanent errors - if response.code is 404 or response.code is 410: + if response.code == 404 or response.code == 410: logger.warn( "Rejecting pushkey %s; subscription is invalid on %s: %d: %s", device.pushkey, @@ -216,7 +216,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): response.code, response_text, ) - elif response.code is not 201: + elif response.code != 201: logger.info( "webpush request for pushkey %s didn't respond with 201; %s responded with %d: %s", device.pushkey, From c1b0a6852d29ecf8204b5cb6a9505ff78e0710f3 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 26 Mar 2021 18:12:00 +0100 Subject: [PATCH 09/16] trim content of notification in an attempt to stay below the max size --- sygnal/webpushpushkin.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 865e687a..d31a8209 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -55,6 +55,8 @@ DEFAULT_MAX_CONNECTIONS = 20 DEFAULT_TTL = 15 * 60 # in seconds +MAX_BODY_LENGTH = 2000 +MAX_PAYLOAD_LENGTH = 4096 class WebpushPushkin(ConcurrencyLimitedPushkin): @@ -255,7 +257,6 @@ def _build_payload(n, device): "sender_display_name", "user_is_target", "type", - "content", ]: value = getattr(n, attr, None) if value: @@ -268,6 +269,20 @@ def _build_payload(n, device): if count_value is not None: payload[attr] = count_value + if n.content and isinstance(n.content, dict): + content = n.content.copy() + # we can't show formatted_body in a notification anyway on web + # so remove it + content.pop("formatted_body", None) + body = content.get("body") + # make some attempts to not go over the max payload length + if isinstance(body, str) and len(body) > MAX_BODY_LENGTH: + content["body"] = body[0: MAX_BODY_LENGTH - 1] + "…" + ciphertext = content.get("ciphertext") + if isinstance(ciphertext, str) and len(ciphertext) > MAX_BODY_LENGTH: + content.pop("ciphertext", None) + payload["content"] = content + return payload From 85eae6ab46acac25fca693de412d6fdfdf2fa063 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 6 Apr 2021 18:12:44 +0200 Subject: [PATCH 10/16] WIP: log if ttl is changed --- sygnal/webpushpushkin.py | 79 +++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index d31a8209..457d8781 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -199,33 +199,10 @@ async def _dispatch_notification_unlimited(self, n, device, context): finally: self.connection_semaphore.release() - # permanent errors - if response.code == 404 or response.code == 410: - logger.warn( - "Rejecting pushkey %s; subscription is invalid on %s: %d: %s", - device.pushkey, - endpoint_domain, - response.code, - response_text, - ) + reject_pushkey = self._handle_response( + response, response_text, device.pushkey, endpoint_domain) + if reject_pushkey: return [device.pushkey] - # and temporary ones - if response.code >= 400: - logger.warn( - "webpush request failed for pushkey %s; %s responded with %d: %s", - device.pushkey, - endpoint_domain, - response.code, - response_text, - ) - elif response.code != 201: - logger.info( - "webpush request for pushkey %s didn't respond with 201; %s responded with %d: %s", - device.pushkey, - endpoint_domain, - response.code, - response_text, - ) return [] @staticmethod @@ -285,6 +262,56 @@ def _build_payload(n, device): return payload + def _handle_response(self, response, response_text, pushkey, endpoint_domain): + """ + Logs and determines the outcome of the response + + Returns: + Boolean whether the puskey should be rejected + """ + ttl_response_headers = response.headers.getRawHeaders(b"TTL") + if ttl_response_headers: + try: + ttl_given = int(ttl_response_headers[0]) + if ttl_given != self.ttl: + logger.info( + "requested TTL of %d to endpoint %s but got %d", + self.ttl, + endpoint_domain, + ttl_given, + ) + except: + pass + + # permanent errors + if response.code == 404 or response.code == 410: + logger.warn( + "Rejecting pushkey %s; subscription is invalid on %s: %d: %s", + pushkey, + endpoint_domain, + response.code, + response_text, + ) + return True + # and temporary ones + if response.code >= 400: + logger.warn( + "webpush request failed for pushkey %s; %s responded with %d: %s", + pushkey, + endpoint_domain, + response.code, + response_text, + ) + elif response.code != 201: + logger.info( + "webpush request for pushkey %s didn't respond with 201; %s responded with %d: %s", + pushkey, + endpoint_domain, + response.code, + response_text, + ) + return False + class HttpAgentWrapper: """ From b3805fa9f8fb1d209a96b455870b4b60fc53e2b9 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 7 Apr 2021 11:39:31 +0200 Subject: [PATCH 11/16] code style changes --- sygnal/webpushpushkin.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 457d8781..3af1a10e 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -138,7 +138,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): # drop notifications without an event id if requested, # see https://github.com/matrix-org/sygnal/issues/186 if device.data.get("events_only") is True and not n.event_id: - return []; + return [] endpoint = device.data.get("endpoint") auth = device.data.get("auth") @@ -182,7 +182,6 @@ async def _dispatch_notification_unlimited(self, n, device, context): with QUEUE_TIME_HISTOGRAM.time(): with PENDING_REQUESTS_GAUGE.track_inprogress(): await self.connection_semaphore.acquire() - try: with SEND_TIME_HISTOGRAM.time(): with ACTIVE_REQUESTS_GAUGE.track_inprogress(): @@ -200,7 +199,8 @@ async def _dispatch_notification_unlimited(self, n, device, context): self.connection_semaphore.release() reject_pushkey = self._handle_response( - response, response_text, device.pushkey, endpoint_domain) + response, response_text, device.pushkey, endpoint_domain + ) if reject_pushkey: return [device.pushkey] return [] @@ -254,7 +254,7 @@ def _build_payload(n, device): body = content.get("body") # make some attempts to not go over the max payload length if isinstance(body, str) and len(body) > MAX_BODY_LENGTH: - content["body"] = body[0: MAX_BODY_LENGTH - 1] + "…" + content["body"] = body[0 : MAX_BODY_LENGTH - 1] + "…" ciphertext = content.get("ciphertext") if isinstance(ciphertext, str) and len(ciphertext) > MAX_BODY_LENGTH: content.pop("ciphertext", None) @@ -282,7 +282,6 @@ def _handle_response(self, response, response_text, pushkey, endpoint_domain): ) except: pass - # permanent errors if response.code == 404 or response.code == 410: logger.warn( From d42d51e9e08d2e8fc55a8ecb774ace4f338b33a0 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 7 Apr 2021 11:40:34 +0200 Subject: [PATCH 12/16] make body even shorter to be on the safe side and turn unused var into comment --- sygnal/webpushpushkin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 3af1a10e..e945bbac 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -55,8 +55,8 @@ DEFAULT_MAX_CONNECTIONS = 20 DEFAULT_TTL = 15 * 60 # in seconds -MAX_BODY_LENGTH = 2000 -MAX_PAYLOAD_LENGTH = 4096 +# Max payload size is 4096 +MAX_BODY_LENGTH = 1000 class WebpushPushkin(ConcurrencyLimitedPushkin): From 640d7140b0d9eeca9cf32d58c9128122904d0c97 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 7 Apr 2021 11:48:28 +0200 Subject: [PATCH 13/16] 2000 is probably more reasonable for ciphertext --- sygnal/webpushpushkin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index e945bbac..4da94be2 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -57,6 +57,7 @@ DEFAULT_TTL = 15 * 60 # in seconds # Max payload size is 4096 MAX_BODY_LENGTH = 1000 +MAX_CIPHERTEXT_LENGTH = 2000 class WebpushPushkin(ConcurrencyLimitedPushkin): @@ -256,7 +257,7 @@ def _build_payload(n, device): if isinstance(body, str) and len(body) > MAX_BODY_LENGTH: content["body"] = body[0 : MAX_BODY_LENGTH - 1] + "…" ciphertext = content.get("ciphertext") - if isinstance(ciphertext, str) and len(ciphertext) > MAX_BODY_LENGTH: + if isinstance(ciphertext, str) and len(ciphertext) > MAX_CIPHERTEXT_LENGTH: content.pop("ciphertext", None) payload["content"] = content From b544e0e1d61de1ebac0550aca48f931a82ba9b2c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 7 Apr 2021 11:52:27 +0200 Subject: [PATCH 14/16] add changelog --- changelog.d/185.bugfix | 1 - changelog.d/212.feature | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/185.bugfix create mode 100644 changelog.d/212.feature diff --git a/changelog.d/185.bugfix b/changelog.d/185.bugfix deleted file mode 100644 index dfd5a4e1..00000000 --- a/changelog.d/185.bugfix +++ /dev/null @@ -1 +0,0 @@ -Add `ttl` option as understood config option for WebPush pushkins to make delivery more reliable \ No newline at end of file diff --git a/changelog.d/212.feature b/changelog.d/212.feature new file mode 100644 index 00000000..d615e171 --- /dev/null +++ b/changelog.d/212.feature @@ -0,0 +1 @@ +Prevent the push key from being rejected for temporary errors and oversized payloads, add TTL logging, and support events_only push data flag. \ No newline at end of file From 8462d24a6fafdb881c658e7fe8d96b23136aea2c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 7 Apr 2021 12:02:55 +0200 Subject: [PATCH 15/16] only except value errors from int(...) --- sygnal/webpushpushkin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 4da94be2..0bbed559 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -281,7 +281,7 @@ def _handle_response(self, response, response_text, pushkey, endpoint_domain): endpoint_domain, ttl_given, ) - except: + except ValueError: pass # permanent errors if response.code == 404 or response.code == 410: From c79ee64a5477d3ca4b2e416534347f76f6849a67 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Wed, 7 Apr 2021 12:04:01 +0200 Subject: [PATCH 16/16] shorten line --- sygnal/webpushpushkin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 0bbed559..faa9e843 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -304,7 +304,8 @@ def _handle_response(self, response, response_text, pushkey, endpoint_domain): ) elif response.code != 201: logger.info( - "webpush request for pushkey %s didn't respond with 201; %s responded with %d: %s", + "webpush request for pushkey %s didn't respond with 201; " + + "%s responded with %d: %s", pushkey, endpoint_domain, response.code,