From d817d1354696fb1b7f718e0cca562896b6754c59 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:16:05 +0100 Subject: [PATCH 01/10] create claims dict on every request, as it is modified by webpush --- sygnal/webpushpushkin.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 6ed57d9a..1dab7f05 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -104,10 +104,9 @@ def __init__(self, name, sygnal, config): self.vapid_private_key = Vapid.from_file(private_key_file=privkey_filename) except VapidException as e: raise PushkinSetupException("invalid 'vapid_private_key' file") from e - vapid_contact_email = self.get_config("vapid_contact_email") - if not vapid_contact_email: + self.vapid_contact_email = self.get_config("vapid_contact_email") + if not self.vapid_contact_email: raise PushkinSetupException("'vapid_contact_email' not set in config") - self.vapid_claims = {"sub": "mailto:{}".format(vapid_contact_email)} async def _dispatch_notification_unlimited(self, n, device, context): p256dh = device.pushkey @@ -137,6 +136,11 @@ async def _dispatch_notification_unlimited(self, n, device, context): payload = WebpushPushkin._build_payload(n, device) data = json.dumps(payload) + # not that webpush modifies vapid_claims, so make sure it's only used once + vapid_claims = { + "sub": "mailto:{}".format(self.vapid_contact_email), + "exp": self._get_vapid_exp(), + } # we use the semaphore to actually limit the number of concurrent # requests, since the HTTPConnectionPool will actually just lead to more # requests being created but not pooled – it does not perform limiting. @@ -151,7 +155,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): subscription_info=subscription_info, data=data, vapid_private_key=self.vapid_private_key, - vapid_claims=self.vapid_claims, + vapid_claims=vapid_claims, requests_session=self.http_agent_wrapper, ) response = await response_wrapper.deferred From 44f63f834c5ff8efd16865d288a801f9c139dd11 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:17:02 +0100 Subject: [PATCH 02/10] logging when gateway rejects pushkey, make other logging uniform --- sygnal/webpushpushkin.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 1dab7f05..6c61da74 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -16,7 +16,7 @@ import logging import os.path from io import BytesIO - +from urllib.parse import urlparse from prometheus_client import Gauge, Histogram from py_vapid import Vapid, VapidException from pywebpush import webpush @@ -112,7 +112,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): p256dh = device.pushkey if not isinstance(device.data, dict): logger.warn( - "device.data is not a dict for pushkey %s, rejecting pushkey", p256dh + "Rejecting pushkey %s; device.data is not a dict", device.pushkey ) return [device.pushkey] @@ -121,8 +121,8 @@ async def _dispatch_notification_unlimited(self, n, device, context): if not p256dh or not endpoint or not auth: logger.warn( - "subscription info incomplete " - + "(p256dh: %s, endpoint: %s, auth: %s), rejecting pushkey", + "Rejecting pushkey; subscription info incomplete " + + "(p256dh: %s, endpoint: %s, auth: %s)", p256dh, endpoint, auth, @@ -165,6 +165,13 @@ async def _dispatch_notification_unlimited(self, n, device, context): # assume 4xx is permanent and 5xx is temporary if 400 <= response.code < 500: + logger.warn( + "Rejecting pushkey %s; gateway %s failed with %d: %s", + device.pushkey, + urlparse(endpoint).netloc, + response.code, + response_text, + ) return [device.pushkey] return [] From 03f22d305bd35ce42743cb09c468dc1d72693830 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:17:32 +0100 Subject: [PATCH 03/10] make webpush easier to unit tests by allowing overriding time and I/O --- sygnal/webpushpushkin.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 6c61da74..fffab243 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -15,6 +15,7 @@ import json import logging import os.path +import time from io import BytesIO from urllib.parse import urlparse from prometheus_client import Gauge, Histogram @@ -159,7 +160,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): requests_session=self.http_agent_wrapper, ) response = await response_wrapper.deferred - await readBody(response) + response_text = await response_wrapper.read_body(response) finally: self.connection_semaphore.release() @@ -175,6 +176,17 @@ async def _dispatch_notification_unlimited(self, n, device, context): return [device.pushkey] return [] + def _get_vapid_exp(self): + """ + Returns the expire value for the vapid claims. + + Having this in a separate method allows to + provide a different time source in unit tests. + """ + + # current time + 12h + return int(time.time()) + (12 * 60 * 60) + @staticmethod def _build_payload(n, device): """ @@ -283,3 +295,6 @@ class HttpResponseWrapper: def __init__(self, deferred): self.deferred = deferred + + async def read_body(self, response): + return (await readBody(response)).decode() From 5c10785f43d1a2690d065f34b7f356c7cbddf878 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:18:10 +0100 Subject: [PATCH 04/10] changelog for fix --- changelog.d/180.bugfix | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog.d/180.bugfix diff --git a/changelog.d/180.bugfix b/changelog.d/180.bugfix new file mode 100644 index 00000000..ac628fff --- /dev/null +++ b/changelog.d/180.bugfix @@ -0,0 +1,6 @@ +Fix vapid_claims from one webpush request bleeding into others + +The vapid_claims dict was reused across requests, but pywebpush actually modified this dictionary. +This made the pushkin usable with only a single pusher for a single user, any other pushers +would be rejected by their gateway as the aud claim (derived from the endpoint url) +didn't match the server name, and the pushkey would be rejected, removing the pusher. \ No newline at end of file From 5bfc562d144e9c855a6d2f4819d6fc074c665530 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:21:25 +0100 Subject: [PATCH 05/10] remove whitespace on blank line --- sygnal/webpushpushkin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index fffab243..a07d520d 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -179,7 +179,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): def _get_vapid_exp(self): """ Returns the expire value for the vapid claims. - + Having this in a separate method allows to provide a different time source in unit tests. """ From b1ecc771c05dfa627d63f7d6c80bbf055ad6b57c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:24:10 +0100 Subject: [PATCH 06/10] code style check wants newline here --- sygnal/webpushpushkin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index a07d520d..499217d1 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -18,6 +18,7 @@ import time from io import BytesIO from urllib.parse import urlparse + from prometheus_client import Gauge, Histogram from py_vapid import Vapid, VapidException from pywebpush import webpush From 96d57112dbd8aca9fb70a34c8b1c2d1a29f4c0cc Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 13:30:51 +0100 Subject: [PATCH 07/10] better spelling and bug explanation --- changelog.d/180.bugfix | 8 +++++--- sygnal/webpushpushkin.py | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/changelog.d/180.bugfix b/changelog.d/180.bugfix index ac628fff..184cc00a 100644 --- a/changelog.d/180.bugfix +++ b/changelog.d/180.bugfix @@ -1,6 +1,8 @@ Fix vapid_claims from one webpush request bleeding into others -The vapid_claims dict was reused across requests, but pywebpush actually modified this dictionary. +The vapid_claims dict was reused across requests, but pywebpush actually modifies this dictionary. This made the pushkin usable with only a single pusher for a single user, any other pushers -would be rejected by their gateway as the aud claim (derived from the endpoint url) -didn't match the server name, and the pushkey would be rejected, removing the pusher. \ No newline at end of file +would be rejected by their gateway, at least if from a different provider, as the aud claim +(the origin of the endpoint url) didn't match the server name, but would likely even fail for +a single provider as the expiry time wasn't refreshed. +The resut was the pushkey would be rejected, removing the pusher. \ No newline at end of file diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 499217d1..70824516 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -138,7 +138,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): payload = WebpushPushkin._build_payload(n, device) data = json.dumps(payload) - # not that webpush modifies vapid_claims, so make sure it's only used once + # note that webpush modifies vapid_claims, so make sure it's only used once vapid_claims = { "sub": "mailto:{}".format(self.vapid_contact_email), "exp": self._get_vapid_exp(), From dfea6cac3337af65572396e088031d769fecb0de Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 15:44:59 +0100 Subject: [PATCH 08/10] don't make adjustments for unit tests until we need them --- sygnal/webpushpushkin.py | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 70824516..125dbb9b 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -141,7 +141,6 @@ async def _dispatch_notification_unlimited(self, n, device, context): # note that webpush modifies vapid_claims, so make sure it's only used once vapid_claims = { "sub": "mailto:{}".format(self.vapid_contact_email), - "exp": self._get_vapid_exp(), } # we use the semaphore to actually limit the number of concurrent # requests, since the HTTPConnectionPool will actually just lead to more @@ -161,7 +160,7 @@ async def _dispatch_notification_unlimited(self, n, device, context): requests_session=self.http_agent_wrapper, ) response = await response_wrapper.deferred - response_text = await response_wrapper.read_body(response) + response_text = (await readBody(response)).decode() finally: self.connection_semaphore.release() @@ -177,17 +176,6 @@ async def _dispatch_notification_unlimited(self, n, device, context): return [device.pushkey] return [] - def _get_vapid_exp(self): - """ - Returns the expire value for the vapid claims. - - Having this in a separate method allows to - provide a different time source in unit tests. - """ - - # current time + 12h - return int(time.time()) + (12 * 60 * 60) - @staticmethod def _build_payload(n, device): """ @@ -296,6 +284,3 @@ class HttpResponseWrapper: def __init__(self, deferred): self.deferred = deferred - - async def read_body(self, response): - return (await readBody(response)).decode() From 7c70c3ef06264a22f3e679ebf79881ff9c109dad Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 15:45:12 +0100 Subject: [PATCH 09/10] just need one line for the changelogs --- changelog.d/180.bugfix | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/changelog.d/180.bugfix b/changelog.d/180.bugfix index 184cc00a..216e5ee4 100644 --- a/changelog.d/180.bugfix +++ b/changelog.d/180.bugfix @@ -1,8 +1 @@ -Fix vapid_claims from one webpush request bleeding into others - -The vapid_claims dict was reused across requests, but pywebpush actually modifies this dictionary. -This made the pushkin usable with only a single pusher for a single user, any other pushers -would be rejected by their gateway, at least if from a different provider, as the aud claim -(the origin of the endpoint url) didn't match the server name, but would likely even fail for -a single provider as the expiry time wasn't refreshed. -The resut was the pushkey would be rejected, removing the pusher. \ No newline at end of file +Fix vapid_claims from one webpush request bleeding into others \ No newline at end of file From 35907b4750aa1f3d5c97716f8a4bd44bf396fc3f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 22 Mar 2021 15:56:46 +0100 Subject: [PATCH 10/10] remove unused import --- sygnal/webpushpushkin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/sygnal/webpushpushkin.py b/sygnal/webpushpushkin.py index 125dbb9b..ff53594d 100644 --- a/sygnal/webpushpushkin.py +++ b/sygnal/webpushpushkin.py @@ -15,7 +15,6 @@ import json import logging import os.path -import time from io import BytesIO from urllib.parse import urlparse