From 3e5af466e4ef6971fdf6b81644ceee3845196af6 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 16 Jul 2025 04:43:47 +0000 Subject: [PATCH] push_notifications: Remove vestiges of base64 storage of tokens. APNs tokens are provided by the client in hex, and we store them in hex. The existing code which attempts to "validate" them by parsing them as base64 only works because base64 is a superset of hex. Enforce that APNs tokens are hex, and remove all of the pieces of test code which were incorrectly passing them in as base64 strings. --- analytics/tests/test_counts.py | 12 ++-- zerver/lib/push_notifications.py | 15 +---- zerver/lib/test_classes.py | 14 ++--- zerver/openapi/python_examples.py | 4 +- zerver/openapi/zulip.yaml | 4 +- zerver/tests/test_handle_push_notification.py | 9 ++- zerver/tests/test_push_notifications.py | 61 ++++++++----------- zerver/tests/test_push_registration.py | 4 +- zilencer/views.py | 6 +- 9 files changed, 52 insertions(+), 77 deletions(-) diff --git a/analytics/tests/test_counts.py b/analytics/tests/test_counts.py index 834ef2359d..d300bf4aeb 100644 --- a/analytics/tests/test_counts.py +++ b/analytics/tests/test_counts.py @@ -50,11 +50,7 @@ from zerver.actions.user_activity import update_user_activity_interval from zerver.actions.users import do_deactivate_user from zerver.lib.create_user import create_user from zerver.lib.exceptions import InvitationError -from zerver.lib.push_notifications import ( - get_message_payload_apns, - get_message_payload_gcm, - hex_to_b64, -) +from zerver.lib.push_notifications import get_message_payload_apns, get_message_payload_gcm from zerver.lib.streams import get_default_values_for_stream_permission_group_settings from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import activate_push_notification_service @@ -1369,19 +1365,19 @@ class TestLoggingCountStats(AnalyticsTestCase): RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64(token), + token=token, user_uuid=(hamlet.uuid), server=self.server, ) RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64(token + "aa"), + token=token + "aa", user_uuid=(hamlet.uuid), server=self.server, ) RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.APNS, - token=hex_to_b64(token), + token=token, user_uuid=str(hamlet.uuid), server=self.server, ) diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index 7558a7c57b..09e683fa82 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -1,7 +1,6 @@ # See https://zulip.readthedocs.io/en/latest/subsystems/notifications.html import asyncio -import base64 import copy import logging import re @@ -83,23 +82,13 @@ PUSH_REGISTRATION_LIVENESS_TIMEOUT = 24 * 60 * 60 DeviceToken: TypeAlias = Union[PushDeviceToken, "RemotePushDeviceToken"] -# We store the token as b64, but apns-client wants hex strings -def b64_to_hex(data: str) -> str: - return base64.b64decode(data).hex() - - -def hex_to_b64(data: str) -> str: - return base64.b64encode(bytes.fromhex(data)).decode() - - def validate_token(token_str: str, kind: int) -> None: if token_str == "" or len(token_str) > 4096: raise JsonableError(_("Empty or invalid length token")) if kind == PushDeviceToken.APNS: - # Validate that we can actually decode the token. try: - b64_to_hex(token_str) - except Exception: + bytes.fromhex(token_str) + except ValueError: raise JsonableError(_("Invalid APNS token")) diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index 90e6c0216d..10a43b03f4 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -57,7 +57,7 @@ from zerver.lib.mdiff import diff_strings from zerver.lib.message import access_message from zerver.lib.notification_data import UserMessageNotificationsData from zerver.lib.per_request_cache import flush_per_request_caches -from zerver.lib.push_notifications import APNsContext, hex_to_b64 +from zerver.lib.push_notifications import APNsContext from zerver.lib.redis_utils import bounce_redis_key_prefix_for_testing from zerver.lib.response import MutableJsonResponse from zerver.lib.sessions import get_session_dict_user @@ -2761,7 +2761,7 @@ class PushNotificationTestCase(BouncerTestCase): for token, appid in self.tokens: PushDeviceToken.objects.create( kind=PushDeviceToken.APNS, - token=hex_to_b64(token), + token=token, user=self.user_profile, ios_app_id=appid, ) @@ -2777,14 +2777,14 @@ class PushNotificationTestCase(BouncerTestCase): # do their own setup. RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.APNS, - token=hex_to_b64(id_token), + token=id_token, ios_app_id=appid, user_id=self.user_profile.id, server=self.server, ) RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.APNS, - token=hex_to_b64(uuid_token), + token=uuid_token, ios_app_id=appid, user_uuid=self.user_profile.uuid, server=self.server, @@ -2803,7 +2803,7 @@ class PushNotificationTestCase(BouncerTestCase): for token in self.fcm_tokens: PushDeviceToken.objects.create( kind=PushDeviceToken.FCM, - token=hex_to_b64(token), + token=token, user=self.user_profile, ios_app_id=None, ) @@ -2812,13 +2812,13 @@ class PushNotificationTestCase(BouncerTestCase): for id_token, uuid_token in self.remote_fcm_tokens: RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64(id_token), + token=id_token, user_id=self.user_profile.id, server=self.server, ) RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64(uuid_token), + token=uuid_token, user_uuid=self.user_profile.uuid, server=self.server, ) diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index 1026abb6b3..f004536c58 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -1632,7 +1632,7 @@ def get_stream_topics(client: Client, stream_id: int) -> None: @openapi_test_function("/users/me/apns_device_token:post") def add_apns_token(client: Client) -> None: # {code_example|start} - request = {"token": "apple-tokenbb", "appid": "org.zulip.Zulip"} + request = {"token": "c0ffee", "appid": "org.zulip.Zulip"} result = client.call_endpoint(url="/users/me/apns_device_token", method="POST", request=request) # {code_example|end} assert_success_response(result) @@ -1643,7 +1643,7 @@ def add_apns_token(client: Client) -> None: def remove_apns_token(client: Client) -> None: # {code_example|start} request = { - "token": "apple-tokenbb", + "token": "c0ffee", } result = client.call_endpoint( url="/users/me/apns_device_token", method="DELETE", request=request diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index ea732f0050..95d02886b2 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -12670,7 +12670,7 @@ paths: description: | The token provided by the device. type: string - example: apple-tokenbb + example: c0ffee appid: description: | The ID of the Zulip app that is making the request. @@ -12731,7 +12731,7 @@ paths: description: | The token provided by the device. type: string - example: apple-tokenbb + example: c0ffee required: - token responses: diff --git a/zerver/tests/test_handle_push_notification.py b/zerver/tests/test_handle_push_notification.py index ba1b22a3ca..80c4436a6f 100644 --- a/zerver/tests/test_handle_push_notification.py +++ b/zerver/tests/test_handle_push_notification.py @@ -18,7 +18,6 @@ from zerver.actions.user_settings import do_change_user_setting from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.lib.push_notifications import ( UserPushIdentityCompat, - b64_to_hex, handle_push_notification, handle_remove_push_notification, ) @@ -92,11 +91,11 @@ class HandlePushNotificationTest(PushNotificationTestCase): self.assertLogs("zilencer.views", level="INFO") as views_logger, ): apns_devices = [ - (b64_to_hex(device.token), device.ios_app_id, device.token) + (device.token, device.ios_app_id, device.token) for device in RemotePushDeviceToken.objects.filter(kind=PushDeviceToken.APNS) ] gcm_devices = [ - (b64_to_hex(device.token), device.ios_app_id, device.token) + (device.token, device.ios_app_id, device.token) for device in RemotePushDeviceToken.objects.filter(kind=PushDeviceToken.FCM) ] mock_fcm_messaging.send_each.return_value = self.make_fcm_success_response( @@ -260,11 +259,11 @@ class HandlePushNotificationTest(PushNotificationTestCase): self.assertLogs("zilencer.views", level="INFO") as views_logger, ): apns_devices = [ - (b64_to_hex(device.token), device.ios_app_id, device.token) + (device.token, device.ios_app_id, device.token) for device in RemotePushDeviceToken.objects.filter(kind=PushDeviceToken.APNS) ] gcm_devices = [ - (b64_to_hex(device.token), device.ios_app_id, device.token) + (device.token, device.ios_app_id, device.token) for device in RemotePushDeviceToken.objects.filter(kind=PushDeviceToken.FCM) ] diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 99a5db94c7..8a78032e9a 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -38,7 +38,6 @@ from zerver.lib.push_notifications import ( get_message_payload_apns, get_message_payload_gcm, get_mobile_push_content, - hex_to_b64, modernize_apns_payload, parse_fcm_options, send_android_push_notification_to_user, @@ -573,13 +572,13 @@ class PushBouncerNotificationTest(BouncerTestCase): android_token = RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64("aaaa"), + token="aaaa", user_uuid=hamlet.uuid, server=server, ) apple_token = RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.APNS, - token=hex_to_b64("bbbb"), + token="bbbb", user_uuid=hamlet.uuid, server=server, ) @@ -626,7 +625,7 @@ class PushBouncerNotificationTest(BouncerTestCase): android_tokens.append( RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64(token + i), + token=token + i, user_id=hamlet.id, server=server, ) @@ -638,7 +637,7 @@ class PushBouncerNotificationTest(BouncerTestCase): uuid_android_tokens.append( RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64(token + i), + token=token + i, user_uuid=str(hamlet.uuid), server=server, ) @@ -646,7 +645,7 @@ class PushBouncerNotificationTest(BouncerTestCase): apple_token = RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.APNS, - token=hex_to_b64(token), + token=token, user_id=hamlet.id, server=server, ) @@ -738,7 +737,7 @@ class PushBouncerNotificationTest(BouncerTestCase): remote_server = self.server RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64("aaaaaa"), + token="aaaaaa", user_id=hamlet.id, server=remote_server, ) @@ -949,7 +948,7 @@ class PushBouncerNotificationTest(BouncerTestCase): hamlet = self.example_user("hamlet") RemotePushDeviceToken.objects.create( kind=RemotePushDeviceToken.FCM, - token=hex_to_b64("aaaaaa"), + token="aaaaaa", user_id=hamlet.id, server=self.server, ) @@ -1030,18 +1029,13 @@ class PushBouncerNotificationTest(BouncerTestCase): self.assert_length(remote_tokens, 0) def test_invalid_apns_token(self) -> None: - endpoints = [ - ("/api/v1/remotes/push/register", "apple-token"), - ] - - for endpoint, method in endpoints: - payload = { - "user_id": 10, - "token": "xyz uses non-hex characters", - "token_kind": PushDeviceToken.APNS, - } - result = self.uuid_post(self.server_uuid, endpoint, payload) - self.assert_json_error(result, "Invalid APNS token") + payload = { + "user_id": 10, + "token": "xyz uses non-hex characters", + "token_kind": PushDeviceToken.APNS, + } + result = self.uuid_post(self.server_uuid, "/api/v1/remotes/push/register", payload) + self.assert_json_error(result, "Invalid APNS token") def test_initialize_push_notifications(self) -> None: realm = get_realm("zulip") @@ -1130,7 +1124,7 @@ class PushBouncerNotificationTest(BouncerTestCase): remote_realm.save() endpoint = "/json/users/me/apns_device_token" - token = "apple-tokenaz" + token = "c0ffee" with self.assertLogs("zilencer.views", level="WARN") as warn_log: result = self.client_post( endpoint, {"token": token, "appid": "org.zulip.Zulip"}, subdomain="zulip" @@ -1163,7 +1157,7 @@ class PushBouncerNotificationTest(BouncerTestCase): endpoints: list[tuple[str, str, int, Mapping[str, str]]] = [ ( "/json/users/me/apns_device_token", - "apple-tokenaz", + "c0ffee", RemotePushDeviceToken.APNS, {"appid": "org.zulip.Zulip"}, ), @@ -2306,7 +2300,7 @@ class TestAddRemoveDeviceTokenAPI(BouncerTestCase): self.login_user(user) endpoints: list[tuple[str, str, Mapping[str, str]]] = [ - ("/json/users/me/apns_device_token", "apple-tokenaz", {"appid": "org.zulip.Zulip"}), + ("/json/users/me/apns_device_token", "c0ffee", {"appid": "org.zulip.Zulip"}), ("/json/users/me/android_gcm_reg_id", "android-token", {}), ] @@ -2317,7 +2311,7 @@ class TestAddRemoveDeviceTokenAPI(BouncerTestCase): result = self.client_post(endpoint, {"token": broken_token, **appid}) self.assert_json_error(result, "Empty or invalid length token") - if label == "apple-tokenaz": + if "apns" in endpoint: result = self.client_post( endpoint, {"token": "xyz has non-hex characters", **appid} ) @@ -2356,12 +2350,12 @@ class TestAddRemoveDeviceTokenAPI(BouncerTestCase): self.login_user(user) no_bouncer_requests: list[tuple[str, str, Mapping[str, str]]] = [ - ("/json/users/me/apns_device_token", "apple-tokenaa", {"appid": "org.zulip.Zulip"}), + ("/json/users/me/apns_device_token", "c0ffee01", {"appid": "org.zulip.Zulip"}), ("/json/users/me/android_gcm_reg_id", "android-token-1", {}), ] bouncer_requests: list[tuple[str, str, Mapping[str, str]]] = [ - ("/json/users/me/apns_device_token", "apple-tokenbb", {"appid": "org.zulip.Zulip"}), + ("/json/users/me/apns_device_token", "c0ffee02", {"appid": "org.zulip.Zulip"}), ("/json/users/me/android_gcm_reg_id", "android-token-2", {}), ] @@ -2402,13 +2396,13 @@ class TestAddRemoveDeviceTokenAPI(BouncerTestCase): # PushDeviceToken will include all the device tokens. token_values = list(PushDeviceToken.objects.values_list("token", flat=True)) self.assertEqual( - token_values, ["apple-tokenaa", "android-token-1", "apple-tokenbb", "android-token-2"] + token_values, ["c0ffee01", "android-token-1", "c0ffee02", "android-token-2"] ) # RemotePushDeviceToken will only include tokens of # the devices using push notification bouncer. remote_token_values = list(RemotePushDeviceToken.objects.values_list("token", flat=True)) - self.assertEqual(sorted(remote_token_values), ["android-token-2", "apple-tokenbb"]) + self.assertEqual(sorted(remote_token_values), ["android-token-2", "c0ffee02"]) # Test removing tokens without using push notification bouncer. for endpoint, token, appid in no_bouncer_requests: @@ -2511,8 +2505,8 @@ class FCMSendTest(PushNotificationTestCase): send_android_push_notification_to_user(self.user_profile, data, {}) self.assert_length(logger.output, 3) log_msg1 = f"INFO:zerver.lib.push_notifications:FCM: Sending notification for local user to 2 devices" - log_msg2 = f"INFO:zerver.lib.push_notifications:FCM: Sent message with ID: {response.responses[0].message_id} to {hex_to_b64(self.fcm_tokens[0])}" - log_msg3 = f"INFO:zerver.lib.push_notifications:FCM: Sent message with ID: {response.responses[1].message_id} to {hex_to_b64(self.fcm_tokens[1])}" + log_msg2 = f"INFO:zerver.lib.push_notifications:FCM: Sent message with ID: {response.responses[0].message_id} to {self.fcm_tokens[0]}" + log_msg3 = f"INFO:zerver.lib.push_notifications:FCM: Sent message with ID: {response.responses[1].message_id} to {self.fcm_tokens[1]}" self.assertEqual([log_msg1, log_msg2, log_msg3], logger.output) mock_warning.assert_not_called() @@ -2520,14 +2514,13 @@ class FCMSendTest(PushNotificationTestCase): def test_not_registered( self, mock_fcm_messaging: mock.MagicMock, fcm_app: mock.MagicMock ) -> None: - token = hex_to_b64("1111") + token = "1111" response = self.make_fcm_error_response( token, firebase_messaging.UnregisteredError("Requested entity was not found.") ) mock_fcm_messaging.send_each.return_value = response - def get_count(hex_token: str) -> int: - token = hex_to_b64(hex_token) + def get_count(token: str) -> int: return PushDeviceToken.objects.filter(token=token, kind=PushDeviceToken.FCM).count() self.assertEqual(get_count("1111"), 1) @@ -2546,7 +2539,7 @@ class FCMSendTest(PushNotificationTestCase): self.assertEqual(get_count("1111"), 0) def test_failure(self, mock_fcm_messaging: mock.MagicMock, fcm_app: mock.MagicMock) -> None: - token = hex_to_b64("1111") + token = "1111" response = self.make_fcm_error_response(token, firebase_exceptions.UnknownError("Failed")) mock_fcm_messaging.send_each.return_value = response diff --git a/zerver/tests/test_push_registration.py b/zerver/tests/test_push_registration.py index 8a6e9506b2..c209c7a6d3 100644 --- a/zerver/tests/test_push_registration.py +++ b/zerver/tests/test_push_registration.py @@ -27,7 +27,7 @@ class RegisterPushDeviceToBouncer(BouncerTestCase): def get_register_push_device_payload( self, - token: str = "apple-tokenaz", + token: str = "c0ffee", token_kind: str = RemotePushDevice.TokenKind.APNS, ios_app_id: str | None = "example.app", timestamp: int | None = None, @@ -205,7 +205,7 @@ class RegisterPushDeviceToBouncer(BouncerTestCase): class RegisterPushDeviceToServer(BouncerTestCase): def get_register_push_device_payload( self, - token: str = "apple-tokenaz", + token: str = "c0ffee", token_kind: str = RemotePushDevice.TokenKind.APNS, ios_app_id: str | None = "example.app", timestamp: int | None = None, diff --git a/zilencer/views.py b/zilencer/views.py index 7bbabe0f85..7fd3a3424b 100644 --- a/zilencer/views.py +++ b/zilencer/views.py @@ -57,7 +57,6 @@ from zerver.lib.push_notifications import ( HostnameAlreadyInUseBouncerError, InvalidRemotePushDeviceTokenError, UserPushIdentityCompat, - b64_to_hex, send_android_push_notification, send_apple_push_notification, send_test_push_notification_directly_to_devices, @@ -498,10 +497,9 @@ class PushRegistration(BaseModel): return False if self.token_kind == RemotePushDevice.TokenKind.APNS: - # Validate that we can actually decode the token. try: - b64_to_hex(self.token) - except Exception: + bytes.fromhex(self.token) + except ValueError: return False return True