feat(messaging): optional identity verification for push subscriptions - #72488
Conversation
62c9833 to
2018599
Compare
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 0 consider. Published 2 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
2018599 to
78bf4f9
Compare
Adds a signed identity token mechanism so customers can optionally require proof that a registration/unregistration call is authorized to act for the given distinct_id, closing the endpoint takeover vector in the default mode. The customer backend mints a short-lived HS256 token (keyed by the project's secret API key) asserting (distinct_id, app_id); the endpoint re-verifies it. Per-integration config["push_identity_verification"] selects disabled (default), optional, or required behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbav6vE7Tm24KkdRhZmvNM
78bf4f9 to
20f86f1
Compare
|
mayteio
left a comment
There was a problem hiding this comment.
Nice one mate, just check the bot comments but looks good to me 😎
…te integrations An app_id can match more than one integration (project_id/bundle_id aren't unique), so reading the verification mode off an arbitrary .first() match let a disabled duplicate downgrade a sibling's required policy. Resolve the strictest mode across every matching integration instead, so verification fails closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bbav6vE7Tm24KkdRhZmvNM
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Problem
A push device token (FCM registration token / APNs device token) is a delivery address, not a credential — FCM/APNs hand one to any app instance, and an attacker owns their own token legitimately. Registration authenticates with the public project token, which is embedded in the mobile app and world-readable. So a caller who knows a victim's
distinct_idand a configuredapp_idcan:distinct_idand receive that user's workflow notifications (takeover), orDELETEthe victim's subscription and stop them receiving notifications (silence).Both the hex and Greptile security reviews on the register/unregister endpoint flagged this. It's a real gap — but forcing authentication on everyone isn't what we want either (we deliberately keep client integration optional, like Customer.io).
Changes
An opt-in, per-integration signed identity token, following Braze's SDK Authentication pattern:
push_identity_tokens.py— the customer's backend mints a short-lived HMAC (HS256) token signed with the project secret key, assertingsub = distinct_idandapp_id. PostHog re-verifies it (accepting the current or backup secret, so key rotation doesn't reject in-flight tokens). An attacker with only the public token can't forge it.push_identity_verificationin the integration config:disabled(default)optionalrequired401)POST) and unregister (DELETE), keyed by anoperationmetric label.disabled, so today's token-less SDK builds keep working. Turning it on is a per-integration config flip; the SDK adds the token as one more field in the same JSON body — additive, no contract change.The register/unregister endpoint this hardens landed in #72490 (now on
master); this PR is rebased on top of it.How did you test this code?
I (Claude) could not run the Django suite here (no Postgres), so the app-side tests rely on CI.
ruffclean.test_push_identity_tokens.py(pureSimpleTestCase, no DB) — the signature/claim matrix onverify_push_identity_token: valid current-secret and backup-secret tokens pass; wrongsub, wrongapp_id, wrong secret, expired, missing-exp, malformed all fail. Catches any regression that would let a token authorize a(distinct_id, app_id)it wasn't minted for.test_push_subscriptions.py(endpoint wiring guards) —requiredaccepts a valid token and rejects both a missing token and a token minted for a differentdistinct_id(the actual attack, capture never called), for register and unregister;optionalstores without a token. These prove the mode wiring end to end for both operations.Verified end to end by the client libraries team. The SDK client half is implemented and tested against this exact contract — iOS PostHog/posthog-ios#735 and Android PostHog/posthog-android#656. Confirmed aligned: the token is sent as
identity_tokenon both the registerPOSTand unregisterDELETE; the host app mints it via apushIdentityProvider(distinctId, appId, completion)hook; and on a401the SDK drops its per-(distinctId, appId)cache, re-mints, and retries once before halting for the session. Those are the last two SDK pieces before the release PRs.Invoked
/writing-tests; pushed the claim matrix down to the pure test and kept the endpoint cases as thin wiring guards.Automatic notifications
Docs update
No user-facing posthog.com doc yet — that lands with the customer-facing Channels UI toggle and SDK support. The token format, claims, and rollout modes are documented inline in
push_identity_tokens.py.🤖 Agent context
Autonomy: Human-driven (agent-assisted). Directed by @dmarchuk, assigned as DRI.
Authored with Claude Code. Skills invoked:
/writing-tests. This revives the signed-token mechanism from the closed draft #70678 (register-only) and extends it to the unregister endpoint added in #72490, in response to the hex/Greptile findings on that work. Chose HMAC off the existing project secret over an asymmetric key pair (PostHog verifies its own ingestion, so there's no third-party verifier needing a public key). Kept enforcementdisabledby default and per-integration so it's safe to enable incrementally and never forces authentication on customers who don't want it. The SDK half (attaching/refreshing the token) is owned by the mobile SDK team and verified against this contract (iOS #735, Android #656).