[fix][push] bind push action counters to the ingesting app - #7899
Open
ar2rsawseen wants to merge 1 commit into
Open
[fix][push] bind push action counters to the ingesting app#7899ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
The [CLY]_push_action handler looked the message up by _id and later updated it by _id, with no reference to the app the event arrived under. The message id is delivered to devices in the notification payload as c.i, so it is not private to the app that sent the campaign, and an event submitted with one app's key could move another app's actioned counters and their platform and language subcounters. Both the lookup and the write now carry the request's app. `app` is stored as an ObjectID on messages, matching how api-reset selects them, so the request's id is converted rather than compared as a string; comparing a string here would have matched nothing and silently stopped all action counting. This closes an app boundary rather than a capability. A recipient of a campaign holds the sending app's public key already, since it ships inside the application they installed, and reporting an action against a message id through that key is the intended ingestion path. So these counters were always movable by a recipient. What was wrong is that a different app's key could move them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
[CLY]_push_actionhandler resolved the message by_idand later updated it by_id, with no reference to the app the event arrived under:The message id is delivered to devices in the notification payload as
c.i, so it is not private to the app that sent the campaign. An event submitted with one app's key could therefore move another app'sresult.actionedand its per-platform and per-language subcounters.Both the lookup and the write now carry the request's app.
One detail worth checking in review
appis stored as an ObjectID on messages —message.js:42declarestype: 'ObjectID', andapi-reset.jsselects withdbext.oid(ob.appId). So the request's id is converted rather than compared as a string. Comparing a string would have matched nothing and silently stopped all push action counting, which is the failure mode to watch for here.What this is and is not
It closes an app boundary rather than a capability. A recipient of a campaign already holds the sending app's public key, since it ships inside the application they installed, and reporting an action against a message id through that key is the intended ingestion path. So these counters were always movable by a recipient; what was wrong is that a different app's key could move them.
Related and not addressed here: there is no deduplication by message, device and action, so a single recipient can report the same action repeatedly through the intended path. That is an analytics integrity question rather than an app boundary one, and it applies equally to the star-rating widget counters.