feat: attach identity verification token to push subscription requests - #735
Conversation
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
PostHog/PushNotifications/PostHogPushSubscriptionHandler.swift:328-330
**Late mint restores invalidated cache**
When the app opts out while an asynchronous provider invocation is outstanding, the completion stores its token after `onOptOut()` has cleared the cache and before the allowed-state guard runs. A later opt-in for the same identity and app therefore reuses the opt-out-era token instead of re-minting, causing an unnecessary rejected registration when that token has expired.
Reviews (1): Last reviewed commit: "feat: attach identity verification token..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
posthog-ios Compliance ReportDate: 2026-07-29 14:34:34 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Prompt To Fix All With AI### Issue 1
PostHog/PushNotifications/PostHogPushSubscriptionHandler.swift:348-355
**Opt-out cache restoration race**
When opt-out occurs after this completion samples `isAllowedProvider()` as true but before it acquires `stateLock`, `onOptOut()` clears the cache and this block immediately writes the old token back using the stale `allowed` value. A later opt-in for the same distinct ID and app ID then takes the cache-hit path without invoking the provider, causing registration to reuse an expired or revoked token and receive an avoidable 401.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (2): Last reviewed commit: "fix: harden async push identity token mi..." | Re-trigger Greptile |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 3 should fix, 0 consider. Published 3 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Feature
Issues: 3 issues
Files (4)
PostHog/PostHogApi.swiftPostHog/PostHogConfig.swiftPostHog/PostHogSDK.swiftPostHog/PushNotifications/PostHogPushSubscriptionHandler.swift
What were the main changes
- Add config.pushIdentityProvider hook allowing apps to supply a backend-minted identity token, invoked with (distinctId, appId, completion)
- PostHogApi POST/DELETE push subscription calls now accept an identityToken and serialize it as identity_token, omitting the key entirely when absent to preserve backward compatibility
- PostHogPushSubscriptionHandler mints/caches identity tokens per exact (distinctId, appId), reusing across retries, re-minting on identity change, and refreshing once on a 401 response
- Handles opt-out races: onOptOut() clears cached token and pending 401-retry flag under stateLock to prevent stale-token resurrection; mid-mint opt-out aborts pending resend
- Adds a bounded mint timeout so a misbehaving provider that never calls completion falls back to a token-less send instead of wedging the send pipeline
- PostHogSDK now calls pushSubscriptionHandler?.onOptOut() when the user opts out
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
Coalesced resend clears didAuthRetry mid-flight, defeating the one-retry-per-401 cap
Priority: should_fix | File: PostHog/PushNotifications/PostHogPushSubscriptionHandler.swift:249-256, 378-422 | Category: bug
Why we think it's a valid issue
- Checked: Traced the full interleaving across
handleResult(378-400),handleFailure's 401 branch (404-421),attemptIfAllowed's in-flight fold (276-291), andresetRetryState(249-256). - Found:
handleResultsetsisSending = falseat line 380 before dispatching tohandleFailure, so the R2 send thathandleFailurestarts (line 418) genuinely claimsisSending = trueand returns while its mint+POST is async in flight. Control then reaches thehadPendingResendbranch (396-399), whose unconditionalresetRetryState()setsdidAuthRetry = false(line 254) out from under the in-flight R2 — the follow-upattemptIfAllowedmerely folds intopendingResend = true(isSending already true), so it clears the cap without starting anything new. Confirmed the reviewer's mechanism. - Found: No backstop bounds the resulting loop: the 401-fresh-token path returns at line 419 before any
pausedUntilis set (455) orhaltedmarked (432/449), andretryCountis never incremented on it. Worse than a single extra retry — thehadPendingResendfollow-up itself re-armspendingResend = trueeach cycle, so with the server persistently 401ing the state at cycle end equals cycle start (pendingResend=true,didAuthRetry=false, next send in flight): a self-sustaining, backoff-free mint+POST loop. - Impact: A documented invariant ('one fresh-token retry per send-cycle', 63-64/405-406) is violated, and the intended terminal 'rejected (401)' halt is replaced by an unbounded loop of provider mints and network POSTs. Trigger is realistic in
requiredidentity mode: a mid-flight identity change (or new registration) coalescing intopendingResendwhile the original send 401s — the very case that also makes the retried token likely to 401 again. Correctness + reliability defect with a concrete trigger and concrete consequence; meets the keep bar.should_fixis appropriate given the entry condition is a specific interleaving rather than a common path.
Issue description
handleFailure's new 401 fresh-token retry (lines 404-421) sets didAuthRetry = true and calls attemptIfAllowed to start a re-mint-and-resend (call it R2) before returning. Because minting is asynchronous, attemptIfAllowed returns immediately after only kicking off R2's mint. Control then returns synchronously to handleResult (lines 378-401): if a registration/identity-change request had coalesced into pendingResend while the original request (R1) was in flight, handleResult's if hadPendingResend, let record = loadRecord() { resetRetryState(); attemptIfAllowed(...) } branch (lines 396-399) unconditionally runs resetRetryState(), which resets didAuthRetry back to false (line 254) even though R2 (the auth-retry just started) is still in flight and relies on didAuthRetry == true to enforce the documented 'one fresh-token retry per send-cycle' cap. Since isSending is already true from R2, this second attemptIfAllowed call doesn't start a new request - it just re-sets pendingResend = true - but it still wipes didAuthRetry. If R2 itself later also returns 401 (plausible, since the very thing that set pendingResend was often an identity change that made the original token stale), handleFailure sees didAuthRetry == false and grants a SECOND fresh-token retry, and the same coalescing pattern can repeat, defeating the intended one-retry-per-cycle guarantee and causing extra mint/network round-trips instead of the intended terminal 'Push subscription rejected (401)' halt.
Suggested fix
Don't let the pendingResend follow-up in handleResult clobber the retry state of an auth-retry that handleFailure just armed for the same cycle. For example, have handleFailure return whether it re-armed a send (i.e. whether shouldRetryWithFreshToken was true) and, when it did, skip handleResult's own resetRetryState()+attemptIfAllowed() call for that completion (the just-started R2 will itself pick up the latest record via the normal in-flight-fold mechanism). Alternatively, only reset didAuthRetry when isSending is confirmed false at the time of the pendingResend follow-up, so a still-in-flight auth-retry's cap isn't cleared out from under it.
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
The registration path looks well covered, but the unregister path can leave a user's subscription active once the cached JWT expires. Also, the current head has red test checks (test and test-ios-simulator); the simulator job specifically reports the newly added vector-11 reset test failing.
| } else { | ||
| hedgeLog("Push unregister failed (status \(info.statusCode.map(String.init) ?? "none")); ignoring (best-effort).") | ||
| // Same one-verification-state as registration: the DELETE resolves a token for the distinct id | ||
| // it carries (the old identity on the reset() path). Still best-effort — no 401 refresh here. |
There was a problem hiding this comment.
[P1] Refresh expired tokens before giving up on unregister
resolveIdentityToken prefers an in-memory token with no expiry check, while the backend mints these with a 5-minute default TTL. A normal logout/reset later in the same process can therefore send an expired token here; required mode returns 401, this path deliberately does not refresh, and unregisterCurrentToken() has already deleted the local record. The old person's server-side subscription is then left active, so this device can keep receiving their notifications after logout. Please invalidate/remint and retry the DELETE once on 401 (or always mint fresh for DELETE) before dropping this best-effort operation.
There was a problem hiding this comment.
The same data-loss path occurs when the device is offline: unlike registration, unregister does not check connectivity or persist a pending operation, and unregisterCurrentToken() removes the local record before attempting the DELETE. A logout/reset while offline therefore gets one transport failure, is never retried on flush() or next launch, and leaves the device associated with the old user on the backend. We should persist a separate pending-unregister intent and retry it later (or otherwise retain enough state to retry) rather than dropping it.
There was a problem hiding this comment.
Both fixed in f1bd05e. Unregister now persists a pending intent (PendingUnregister) before the DELETE, so an offline or failed attempt gets retried on flush() and next launch instead of dropped. And on a 401 it clears the cached token and re-mints once before giving up, same as the send path.
Need to close the same gap on Android, will do on upcoming release PRs
2 participants
💡 Motivation and Context
PostHog/posthog#72488 (draft) adds opt-in identity verification to
/api/push_subscriptions: the backend mints a short-lived HS256 JWT (sub= distinct_id,app_id,aud=posthog:push_identity,exp) signed with the project's secret API key, and verifies it on POST and DELETE. Integration modes aredisabled(default),optional,required. Without SDK support there is no way to attach the token, sorequiredmode is unusable from iOS. The provider shape was agreed with Dan in #team-messaging.Adds the SDK half:
config.pushIdentityProvider: (distinctId, appId, completion) -> Void. The app hands the backend-minted token (or nil) tocompletion, from any thread, exactly once.identity_tokenon the register POST and unregister DELETE. When the key is absent it is omitted entirely, so the body stays byte-identical to today's contract for apps that don't opt in.(distinctId, appId): reused across in-session backoff retries, re-minted on identity change, refreshed on 401, and never cached once minted after opt-out.💚 How did you test it?
make testgreen (707 tests), including cross-SDK vectors 9-14 (token on both legs, key omission when absent, reset-leg token counts, 500-retry cache reuse, 401 refresh then terminal, 401 with no provider) and exactly-once foreign-thread completion tests.make apiCheckandmake lintclean.📝 Checklist
If releasing new changes
pnpm changesetto generate the changeset file (deliberately omitted: feature branch, not releasing yet)🤖 Agent context
DRI: @ioannisj
Autonomy: Human-driven (agent-assisted)