Skip to content

feat(messaging): support unregistering push tokens via DELETE #72425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dmarchuk wants to merge 1 commit into from
+86 −41
Closed

feat(messaging): support unregistering push tokens via DELETE #72425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 59 additions & 41 deletions products/messaging/backend/api/push_subscriptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from datetime import UTC, datetime
from typing import Any

from django.db.models import Q
from django.http import HttpResponse, JsonResponse
Expand All @@ -17,7 +18,7 @@
from posthog.helpers.encrypted_fields import EncryptedFieldMixin
from posthog.models.integration import Integration
from posthog.models.team.team import Team
from posthog.utils import load_data_from_request
from posthog.utils import decompress
from posthog.utils_cors import cors_response

VALID_PLATFORMS = ("android", "ios")
Expand Down Expand Up @@ -46,23 +47,41 @@ def _find_integration(team_id: int, app_id: str) -> Integration | None:
)


# load_data_from_request reads the request body only for POST (other methods read the ?data= query
# param), but both register (POST) and unregister (DELETE) send a JSON body — so read and decompress
# the body directly for either method, mirroring that helper's POST branch.
def _load_json_body(request: Request) -> Any:
compression = (
request.GET.get("compression") or request.POST.get("compression") or request.headers.get("content-encoding", "")
).lower()
return decompress(request.body, compression)
Comment on lines +53 to +57

@greptile-apps greptile-apps Bot Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Preserve POST form parsing

This fixes JSON DELETE bodies by always passing request.body to decompress, but it also changes the existing POST contract. load_data_from_request used request.POST["data"] for form-encoded and multipart POST requests. The new helper instead tries to decode the raw form body as JSON, so previously supported registration requests return 400 Invalid JSON body. Keep the existing parser for POST and use direct body parsing only for DELETE.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: products/messaging/backend/api/push_subscriptions.py
Line: 53-57

Comment:
**Preserve POST form parsing**

This fixes JSON DELETE bodies by always passing `request.body` to `decompress`, but it also changes the existing POST contract. `load_data_from_request` used `request.POST["data"]` for form-encoded and multipart POST requests. The new helper instead tries to decode the raw form body as JSON, so previously supported registration requests return `400 Invalid JSON body`. Keep the existing parser for POST and use direct body parsing only for DELETE.

How can I resolve this? If you propose a fix, please make it concise.



@csrf_exempt
def push_subscriptions(request: Request):
if request.method == "OPTIONS":
return cors_response(request, HttpResponse(""))
# cors_response advertises GET, POST, OPTIONS by default; also allow DELETE so a browser
# preflight for the unregister call isn't rejected before reaching the DELETE branch.
preflight = cors_response(request, HttpResponse(""))
preflight["Access-Control-Allow-Methods"] = "GET, POST, DELETE, OPTIONS"
return preflight

if request.method != "POST":
if request.method not in ("POST", "DELETE"):
Comment thread
dmarchuk marked this conversation as resolved.
Comment thread
dmarchuk marked this conversation as resolved.
return cors_response(
request,
generate_exception_response(
"push_subscriptions",
"Only POST requests are supported.",
"Only POST and DELETE requests are supported.",
type="validation_error",
code="method_not_allowed",
status_code=status.HTTP_405_METHOD_NOT_ALLOWED,
),
)

# POST registers a device (stores the token); DELETE unregisters it (unsets the property) so a
# logged-out user stops receiving another user's notifications on a shared device.
is_register = request.method == "POST"

if len(request.body) > MAX_BODY_BYTES:
return cors_response(
request,
Expand All @@ -76,7 +95,7 @@ def push_subscriptions(request: Request):
)

try:
data = load_data_from_request(request)
data = _load_json_body(request)
except (RequestParsingError, UnspecifiedCompressionFallbackParsingError):
return cors_response(
request,
Expand Down Expand Up @@ -132,16 +151,12 @@ def push_subscriptions(request: Request):
platform = data.get("platform")
app_id = data.get("app_id")

missing_fields = [
field_name
for field_name, value in [
("distinct_id", distinct_id),
("device_token", device_token),
("platform", platform),
("app_id", app_id),
]
if not value or not isinstance(value, str)
]
# distinct_id + app_id identify which person property to set or unset, so both methods need them.
# device_token + platform describe the device being stored, so they're required only to register.
required_fields = [("distinct_id", distinct_id), ("app_id", app_id)]
if is_register:
required_fields += [("device_token", device_token), ("platform", platform)]
missing_fields = [field_name for field_name, value in required_fields if not value or not isinstance(value, str)]
if missing_fields:
return cors_response(
request,
Expand All @@ -155,21 +170,21 @@ def push_subscriptions(request: Request):
)

assert isinstance(distinct_id, str)
assert isinstance(device_token, str)
assert isinstance(platform, str)
assert isinstance(app_id, str)

if platform not in VALID_PLATFORMS:
return cors_response(
request,
generate_exception_response(
"push_subscriptions",
f"Invalid platform. Must be one of: {', '.join(VALID_PLATFORMS)}.",
type="validation_error",
code="invalid_platform",
status_code=status.HTTP_400_BAD_REQUEST,
),
)
if is_register:
assert isinstance(platform, str)
if platform not in VALID_PLATFORMS:
return cors_response(
request,
generate_exception_response(
"push_subscriptions",
f"Invalid platform. Must be one of: {', '.join(VALID_PLATFORMS)}.",
type="validation_error",
code="invalid_platform",
status_code=status.HTTP_400_BAD_REQUEST,
),
)

integration = _find_integration(team.id, app_id)
if not integration:
Expand All @@ -185,8 +200,15 @@ def push_subscriptions(request: Request):
),
)

encrypted_token = _encrypted_fields.encrypt(device_token)
property_key = f"$device_push_subscription_{app_id}"
if is_register:
assert isinstance(device_token, str)
# Store the token encrypted; the send path rejects any value that fails to decrypt.
person_properties: dict = {"$set": {property_key: _encrypted_fields.encrypt(device_token)}}
else:
# Unregister mirrors the send path's dead-token pruning: unset the person property so the
# device stops matching. There is one subscription per app per person, so app_id is enough.
person_properties = {"$unset": [property_key]}
Comment thread
dmarchuk marked this conversation as resolved.

try:
capture_internal(
Expand All @@ -195,28 +217,24 @@ def push_subscriptions(request: Request):
event_source="push_subscriptions",
distinct_id=distinct_id,
Comment thread
dmarchuk marked this conversation as resolved.

@hex-security-app hex-security-app Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Authorize push subscription deletion against the caller identity

The public project token scopes a request only to a team and is embedded in client apps; it does not authorize an arbitrary request-supplied distinct_id. An attacker who knows a project's app ID and a victim's distinct ID can call this DELETE endpoint and reach capture_internal with $unset, removing the victim's subscription and silencing their workflow notifications.

Prompt To Fix With AI 
Require a caller-bound, verifiable assertion that authorizes the exact `(distinct_id, app_id)` before emitting the `$unset`; a public project token must not authorize operations on an arbitrary person. Apply the same identity binding to both POST registration and DELETE unregistration, and add tests that a caller cannot remove another user's subscription.

Severity: medium | Confidence: 98% | React with 👍 if useful or 👎 if not

timestamp=datetime.now(UTC),
properties={"$set": {property_key: encrypted_token}},
properties=person_properties,
process_person_profile=True,
)
except Exception:
return cors_response(
request,
generate_exception_response(
"push_subscriptions",
"Failed to store push subscription.",
"Failed to store push subscription." if is_register else "Failed to remove push subscription.",
type="server_error",
code="capture_failed",
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
),
)

return cors_response(
request,
JsonResponse(
{
"distinct_id": distinct_id,
"platform": platform,
},
status=status.HTTP_200_OK,
),
)
if is_register:
return cors_response(
request,
JsonResponse({"distinct_id": distinct_id, "platform": platform}, status=status.HTTP_200_OK),
)
return cors_response(request, JsonResponse({"distinct_id": distinct_id}, status=status.HTTP_200_OK))
27 changes: 27 additions & 0 deletions products/messaging/backend/api/test/test_push_subscriptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@ def _post(self, data: dict, api_key: str | None = None):
content_type="application/json",
)

def _delete(self, data: dict, api_key: str | None = None):
payload = {**data, "api_key": api_key or self.team.api_token}
return self.client.delete(
"/api/push_subscriptions/",
data=json.dumps(payload),
content_type="application/json",
)

@patch("products.messaging.backend.api.push_subscriptions.capture_internal")
def test_register_android_token(self, mock_capture: MagicMock):
mock_capture.return_value = MagicMock(status_code=200)
Expand Down Expand Up @@ -214,6 +222,8 @@ def test_options_returns_200(self):
response = self.client.options("/api/push_subscriptions/")

assert response.status_code == status.HTTP_200_OK
# The preflight must advertise DELETE, or a browser rejects the unregister call before it runs.
assert "DELETE" in response["Access-Control-Allow-Methods"]

@patch("products.messaging.backend.api.push_subscriptions.capture_internal")
def test_gzip_compressed_body(self, mock_capture: MagicMock):
Expand Down Expand Up @@ -253,3 +263,20 @@ def test_oversized_body_is_rejected_before_parsing(self, mock_capture: MagicMock

assert response.status_code == status.HTTP_413_REQUEST_ENTITY_TOO_LARGE
mock_capture.assert_not_called()

@patch("products.messaging.backend.api.push_subscriptions.capture_internal")
def test_unregister_unsets_the_subscription(self, mock_capture: MagicMock):
# DELETE needs only distinct_id + app_id (not device_token/platform) and must $unset the
# property, so a logged-out user stops receiving another user's notifications on the device.
mock_capture.return_value = MagicMock(status_code=200)

response = self._delete({"distinct_id": "user-1", "app_id": "my-firebase-project"})

assert response.status_code == status.HTTP_200_OK
assert response.json() == {"distinct_id": "user-1"}

mock_capture.assert_called_once()
call_kwargs = mock_capture.call_args.kwargs
assert call_kwargs["distinct_id"] == "user-1"
assert call_kwargs["process_person_profile"] is True
assert call_kwargs["properties"] == {"$unset": ["$device_push_subscription_my-firebase-project"]}
Loading