From eb2347bbc3b80927dfea0ae5fa3d5ebe3ff9b2e7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 12 Apr 2021 18:12:12 +0100 Subject: [PATCH 1/5] Be stricter about JSON that is accepted by Sygnal This disables the JSON extensions which Python supports by default (parsing of `Infinity` / `-Infinity` and `NaN`). These shouldn't be accepted since they're not technically valid JSON and other languages won't be able to interpret it properly. --- changelog.d/216.bugfix | 1 + sygnal/gcmpushkin.py | 6 +++--- sygnal/http.py | 4 ++-- sygnal/utils.py | 10 ++++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 changelog.d/216.bugfix diff --git a/changelog.d/216.bugfix b/changelog.d/216.bugfix new file mode 100644 index 00000000..bf9ad489 --- /dev/null +++ b/changelog.d/216.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where invalid JSON would be accepted over the HTTP interfaces. diff --git a/sygnal/gcmpushkin.py b/sygnal/gcmpushkin.py index 1967dd37..c69c277c 100644 --- a/sygnal/gcmpushkin.py +++ b/sygnal/gcmpushkin.py @@ -33,7 +33,7 @@ ) from sygnal.helper.context_factory import ClientTLSOptionsFactory from sygnal.helper.proxy.proxyagent_twisted import ProxyAgent -from sygnal.utils import NotificationLoggerAdapter, twisted_sleep +from sygnal.utils import NotificationLoggerAdapter, twisted_sleep, json_decoder from .exceptions import PushkinSetupException from .notifications import ConcurrencyLimitedPushkin @@ -251,8 +251,8 @@ async def _request_dispatch(self, n, log, body, headers, pushkeys, span): return pushkeys, [] elif 200 <= response.code < 300: try: - resp_object = json.loads(response_text) - except JSONDecodeError: + resp_object = json_decoder.decode(response_text) + except ValueError: raise NotificationDispatchException("Invalid JSON response from GCM.") if "results" not in resp_object: log.error( diff --git a/sygnal/http.py b/sygnal/http.py index 7facc7be..5b1968e1 100644 --- a/sygnal/http.py +++ b/sygnal/http.py @@ -35,7 +35,7 @@ from twisted.web.server import NOT_DONE_YET from sygnal.notifications import NotificationContext -from sygnal.utils import NotificationLoggerAdapter +from sygnal.utils import NotificationLoggerAdapter, json_decoder from .exceptions import InvalidNotificationException, NotificationDispatchException from .notifications import Notification @@ -133,7 +133,7 @@ def _handle_request(self, request): log = NotificationLoggerAdapter(logger, {"request_id": request_id}) try: - body = json.loads(request.content.read()) + body = json_decoder.decode(request.content.read()) except Exception as exc: msg = "Expected JSON request body" log.warning(msg, exc_info=exc) diff --git a/sygnal/utils.py b/sygnal/utils.py index 3ce3bfaa..94618de4 100644 --- a/sygnal/utils.py +++ b/sygnal/utils.py @@ -12,6 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import json import re from logging import LoggerAdapter @@ -62,3 +63,12 @@ def glob_to_regex(glob): # \A anchors at start of string, \Z at end of string return re.compile(r"\A" + res + r"\Z", re.IGNORECASE) + + +def _reject_invalid_json(val): + """Do not allow Infinity, -Infinity, or NaN values in JSON.""" + raise ValueError("Invalid JSON value: '%s'" % val) + + +# a custom JSON decoder which will reject Python extensions to JSON. +json_decoder = json.JSONDecoder(parse_constant=_reject_invalid_json) From fb75f42daef562bcd0412e3bf12f60090e781469 Mon Sep 17 00:00:00 2001 From: Dan Callahan Date: Mon, 12 Apr 2021 21:34:05 +0100 Subject: [PATCH 2/5] Remove unused JSONDecodeError import Signed-off-by: Dan Callahan --- sygnal/gcmpushkin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/sygnal/gcmpushkin.py b/sygnal/gcmpushkin.py index c69c277c..6c249766 100644 --- a/sygnal/gcmpushkin.py +++ b/sygnal/gcmpushkin.py @@ -18,7 +18,6 @@ import logging import time from io import BytesIO -from json import JSONDecodeError from opentracing import logs, tags from prometheus_client import Counter, Gauge, Histogram From 8db733c177fa0d84661a6097744b321afc4f0ec9 Mon Sep 17 00:00:00 2001 From: Dan Callahan Date: Mon, 12 Apr 2021 21:40:34 +0100 Subject: [PATCH 3/5] Satisfy isort Signed-off-by: Dan Callahan --- sygnal/gcmpushkin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sygnal/gcmpushkin.py b/sygnal/gcmpushkin.py index 6c249766..e3bbbec4 100644 --- a/sygnal/gcmpushkin.py +++ b/sygnal/gcmpushkin.py @@ -32,7 +32,7 @@ ) from sygnal.helper.context_factory import ClientTLSOptionsFactory from sygnal.helper.proxy.proxyagent_twisted import ProxyAgent -from sygnal.utils import NotificationLoggerAdapter, twisted_sleep, json_decoder +from sygnal.utils import NotificationLoggerAdapter, json_decoder, twisted_sleep from .exceptions import PushkinSetupException from .notifications import ConcurrencyLimitedPushkin From a941c265f44be6cb14ac95c4de139437ff0b527e Mon Sep 17 00:00:00 2001 From: Dan Callahan Date: Mon, 12 Apr 2021 22:40:23 +0100 Subject: [PATCH 4/5] Fix test failures Signed-off-by: Dan Callahan --- sygnal/gcmpushkin.py | 4 ++-- sygnal/http.py | 4 ++-- sygnal/utils.py | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/sygnal/gcmpushkin.py b/sygnal/gcmpushkin.py index e3bbbec4..df9dd481 100644 --- a/sygnal/gcmpushkin.py +++ b/sygnal/gcmpushkin.py @@ -32,7 +32,7 @@ ) from sygnal.helper.context_factory import ClientTLSOptionsFactory from sygnal.helper.proxy.proxyagent_twisted import ProxyAgent -from sygnal.utils import NotificationLoggerAdapter, json_decoder, twisted_sleep +from sygnal.utils import NotificationLoggerAdapter, safe_json_loads, twisted_sleep from .exceptions import PushkinSetupException from .notifications import ConcurrencyLimitedPushkin @@ -250,7 +250,7 @@ async def _request_dispatch(self, n, log, body, headers, pushkeys, span): return pushkeys, [] elif 200 <= response.code < 300: try: - resp_object = json_decoder.decode(response_text) + resp_object = safe_json_loads(response_text) except ValueError: raise NotificationDispatchException("Invalid JSON response from GCM.") if "results" not in resp_object: diff --git a/sygnal/http.py b/sygnal/http.py index 5b1968e1..f53efe70 100644 --- a/sygnal/http.py +++ b/sygnal/http.py @@ -35,7 +35,7 @@ from twisted.web.server import NOT_DONE_YET from sygnal.notifications import NotificationContext -from sygnal.utils import NotificationLoggerAdapter, json_decoder +from sygnal.utils import NotificationLoggerAdapter, safe_json_loads from .exceptions import InvalidNotificationException, NotificationDispatchException from .notifications import Notification @@ -133,7 +133,7 @@ def _handle_request(self, request): log = NotificationLoggerAdapter(logger, {"request_id": request_id}) try: - body = json_decoder.decode(request.content.read()) + body = safe_json_loads(request.content.read()) except Exception as exc: msg = "Expected JSON request body" log.warning(msg, exc_info=exc) diff --git a/sygnal/utils.py b/sygnal/utils.py index 94618de4..ad6925e5 100644 --- a/sygnal/utils.py +++ b/sygnal/utils.py @@ -14,6 +14,7 @@ # limitations under the License. import json import re +from functools import partial from logging import LoggerAdapter from twisted.internet.defer import Deferred @@ -67,8 +68,8 @@ def glob_to_regex(glob): def _reject_invalid_json(val): """Do not allow Infinity, -Infinity, or NaN values in JSON.""" - raise ValueError("Invalid JSON value: '%s'" % val) + raise ValueError(f"Invalid JSON value: {val!r}") # a custom JSON decoder which will reject Python extensions to JSON. -json_decoder = json.JSONDecoder(parse_constant=_reject_invalid_json) +safe_json_loads = partial(json.loads, parse_constant=_reject_invalid_json) From a8b99d0528b1bd895da0532807fdc6af88b6254b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 13 Apr 2021 11:01:58 +0100 Subject: [PATCH 5/5] back to JSONDecoder --- sygnal/gcmpushkin.py | 4 ++-- sygnal/http.py | 4 ++-- sygnal/utils.py | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/sygnal/gcmpushkin.py b/sygnal/gcmpushkin.py index df9dd481..e3bbbec4 100644 --- a/sygnal/gcmpushkin.py +++ b/sygnal/gcmpushkin.py @@ -32,7 +32,7 @@ ) from sygnal.helper.context_factory import ClientTLSOptionsFactory from sygnal.helper.proxy.proxyagent_twisted import ProxyAgent -from sygnal.utils import NotificationLoggerAdapter, safe_json_loads, twisted_sleep +from sygnal.utils import NotificationLoggerAdapter, json_decoder, twisted_sleep from .exceptions import PushkinSetupException from .notifications import ConcurrencyLimitedPushkin @@ -250,7 +250,7 @@ async def _request_dispatch(self, n, log, body, headers, pushkeys, span): return pushkeys, [] elif 200 <= response.code < 300: try: - resp_object = safe_json_loads(response_text) + resp_object = json_decoder.decode(response_text) except ValueError: raise NotificationDispatchException("Invalid JSON response from GCM.") if "results" not in resp_object: diff --git a/sygnal/http.py b/sygnal/http.py index f53efe70..5558ecb3 100644 --- a/sygnal/http.py +++ b/sygnal/http.py @@ -35,7 +35,7 @@ from twisted.web.server import NOT_DONE_YET from sygnal.notifications import NotificationContext -from sygnal.utils import NotificationLoggerAdapter, safe_json_loads +from sygnal.utils import NotificationLoggerAdapter, json_decoder from .exceptions import InvalidNotificationException, NotificationDispatchException from .notifications import Notification @@ -133,7 +133,7 @@ def _handle_request(self, request): log = NotificationLoggerAdapter(logger, {"request_id": request_id}) try: - body = safe_json_loads(request.content.read()) + body = json_decoder.decode(request.content.read().decode("utf-8")) except Exception as exc: msg = "Expected JSON request body" log.warning(msg, exc_info=exc) diff --git a/sygnal/utils.py b/sygnal/utils.py index ad6925e5..fb71e673 100644 --- a/sygnal/utils.py +++ b/sygnal/utils.py @@ -14,7 +14,6 @@ # limitations under the License. import json import re -from functools import partial from logging import LoggerAdapter from twisted.internet.defer import Deferred @@ -72,4 +71,4 @@ def _reject_invalid_json(val): # a custom JSON decoder which will reject Python extensions to JSON. -safe_json_loads = partial(json.loads, parse_constant=_reject_invalid_json) +json_decoder = json.JSONDecoder(parse_constant=_reject_invalid_json)