security(star-rating): forward only the widget's own parameters from /i/feedback/input (24.05) - #7953
Open
ar2rsawseen wants to merge 1 commit into
Open
Conversation
…/i/feedback/input Backport of #7952 to release.24.05. /i/feedback/input accepts a feedback submission from the web widget, which cannot compute a checksum because it does not hold the app's salt, so the handler replays the request into the generic /i processor with no_checksum set. It built that replayed request from the caller's entire query string: url: "/i?" + ob.params.href.split("/i/feedback/input?")[1] The handler's only check is on the events parameter, which has to be a single [CLY]_star_rating event. Every other parameter was forwarded untouched and then processed with checksum verification disabled, so a caller holding just the public app key could append unrelated write parameters and have them accepted unsigned on an app that has a checksum salt configured. old_device_id reaches appUsers.merge(), and token_session reaches the push token binding; begin_session, user_details, consent and crash ride along the same way. Rebuild the forwarded query from the parameters the widget actually sends (events, app_key, device_id, sdk_name, sdk_version, timestamp, hour, dow, app_version) and drop everything else, so the star rating submission keeps working while any other operation has to go through /i and satisfy the checksum. Values are URL encoded, so a parameter value cannot inject a second parameter, and non scalar values are dropped rather than stringified, since a JSON body can put an object in a query string parameter. The helper lives in api/input-utils.js so it can be unit tested; the existing plugin tests only vary device_id and never sent old_device_id here, so they are unaffected. Note this is the only unauthenticated no_checksum forwarder: the sole other no_checksum caller is api/utils/taskmanager.js, which replays a stored task URL created by an authenticated user. Reported through the security bug bounty programme (received 2026-08-18). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Backport of #7952 to
release.24.05./i/feedback/inputreplays its request into the generic/iprocessor withno_checksumset, because the web feedback widget cannot compute a checksum (it does not hold the app's salt). It built that replayed request from the caller's entire query string, and the handler's only check is onevents(a single[CLY]_star_ratingevent). Every other parameter was forwarded and then processed with checksum verification disabled.On an app with a checksum salt configured, a caller holding only the public app key could therefore append unrelated write parameters and have them accepted unsigned:
old_device_idreachesappUsers.merge(),token_sessionreaches the push token binding, andbegin_session/user_details/consent/crashride along the same way.Fix (identical to #7952): rebuild the forwarded query from the parameters the widget actually sends —
events,app_key,device_id,sdk_name,sdk_version,timestamp,hour,dow,app_version— and drop everything else. Values are URL encoded so a value cannot inject a second parameter, and non-scalars are dropped rather than stringified. The 24.05 widget sends the same nine parameters as master, so the allowlist is unchanged.Tests:
test/unit-tests/star-rating.input-utils.js, 8 cases, all passing on this branch;eslintclean.🤖 Generated with Claude Code