feat(messaging): support unregistering push tokens via DELETE - #72425
feat(messaging): support unregistering push tokens via DELETE#72425dmarchuk wants to merge 1 commit into
Conversation
4a03a0b to
b2d2003
Compare
|
Add DELETE to /api/push_subscriptions so a device can be explicitly unregistered, unsetting the $device_push_subscription_<app_id> person property (the same operation the send path already performs when a provider reports a dead token). This gives the mobile SDKs a clean call to make on reset()/logout: unregister the token from the outgoing identity, then re-register under the new anonymous id. Without it a logged-out user's person keeps the token, so on a shared device the next user could receive the previous user's notifications. Unregister reuses the register path's auth and integration lookup and needs only distinct_id + app_id (not device_token/platform). Both methods read the JSON request body directly (load_data_from_request only reads the body for POST), and the OPTIONS preflight now advertises DELETE so a browser client isn't rejected before the branch runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbav6vE7Tm24KkdRhZmvNM
b2d2003 to
d0172d0
Compare
|
Thanks for confirming. The finding remains applicable to this PR as it currently adds the unauthenticated destructive path, but it is reasonable to address the shared caller-identity binding in #72488. Please ensure that work covers both POST registration and DELETE unregistration, and prevents a public project token from authorizing operations on an arbitrary |
| @@ -195,28 +217,24 @@ def push_subscriptions(request: Request): | |||
| event_source="push_subscriptions", | |||
| distinct_id=distinct_id, | |||
There was a problem hiding this comment.
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
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/messaging/backend/api/push_subscriptions.py:53-57
**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.
Reviews (2): Last reviewed commit: "feat(messaging): support unregistering p..." | Re-trigger Greptile |
| 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) |
There was a problem hiding this comment.
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.|
Superseded with #72490 |
Problem
POST /api/push_subscriptionslets a device register a push token, which is stored as the$device_push_subscription_<app_id>person property. But there was no way to un-register. The only removal today is reactive: the CDP send path prunes a token after a provider reports it dead.The mobile SDKs (iOS/Android, in progress) need an explicit unregister call for
reset()/ logout. Without it, a logged-out user's person keeps the token, so on a shared device the next user could receive the previous user's workflow notifications.Changes
Add
DELETE /api/push_subscriptions. It unsets the$device_push_subscription_<app_id>person property (the same operation the send path already performs when pruning a dead token). It reuses the register path's auth and integration lookup, and needs onlydistinct_id+app_id(notdevice_token/platform).POSTbehavior is unchanged.This gives the SDKs a clean lifecycle on
reset(): unregister the token from the outgoing identity, then re-register under the new anonymous id (so it follows the device to whoever uses it next via the normal identify-merge).Note
This endpoint authenticates with the public project token, same as register. So the notification-takeover risk on register (an attacker binding a victim's
distinct_id) has a mirror here: an attacker could silence a victim by unregistering their device. Closing that needs caller-identity verification, which is a separate, still-open effort covering both register and unregister — out of scope for this PR, which matches the endpoint's current posture.How did you test this code?
I (Claude) could not run the Django suite in this environment (no Postgres), so the test below has not run locally and I'm relying on CI.
ruff checkandruff formatare clean on both files.Added one endpoint test,
test_unregister_unsets_the_subscription: aDELETEwith onlydistinct_id+app_idreturns 200 and calls capture withproperties={"$unset": [...]}. It guards the regression that matters here — ifDELETEreused the register required-fields list (400 on missingdevice_token) or emitted$setinstead of$unset, logout would silently leave the device subscribed. No existing test exercisedDELETE. Invoked/writing-testsfirst; the integration-not-found and team-isolation paths are shared withPOSTand already covered, so I didn't duplicate them.Automatic notifications
Docs update
No user-facing posthog.com docs yet. The customer-facing setup doc should land with the SDK support that consumes this endpoint.
🤖 Agent context
Autonomy: Human-driven (agent-assisted). Directed by @dmarchuk, assigned as DRI.
Authored with Claude Code. Skills invoked:
/writing-tests. The work came out of a client-libraries question aboutreset()behavior on logout — the conclusion was that the SDK should unset the token on the outgoing identity and re-register under the anonymous id, and that a server-side unregister endpoint gives it a clean call rather than hand-rolling a$unset.Decision: modeled unregister as
DELETEon the same path (symmetric withPOSTregister) over aPOSTwith anactionfield. Based purely onmaster— a caller-identity-verification effort exists separately and is not merged, so this PR does not depend on it (see the note above).Generated by Claude Code