security(compliance-hub): fix the consents table projection server side - #7859
Open
ar2rsawseen wants to merge 1 commit into
Open
security(compliance-hub): fix the consents table projection server side#7859ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
/o/app_users/consents built its projection as
`params.qstring.project || params.qstring.projection || {defaults}`, so the
narrow default applied only when the caller omitted both parameters. Because a
query-string value arrives as a string, `project={}` is truthy and short-circuits
the chain, and an empty projection means "all fields" to MongoDB. The projection
was then forwarded unvalidated into appUsers.search() and on to
find(query, project).
A member holding nothing but compliance_hub read on an app could therefore turn a
consent-status table into full app_users documents: name, email, username, phone,
organization, customer-defined custom properties, payment totals, city/region,
acquisition source and device details. The Compliance Hub UI never sends a
projection, so the parameter was attacker-facing only.
The consent table renders a fixed set of columns, so the projection is now a
server-side constant and any project/projection on the request is ignored. The
allowlist matches the columns bound in the users table template, plus uid and
appUserExport for the per-row history and export actions.
/o/consent/search is fixed the same way. Its response shape is unchanged - full
consent_history documents, which hold only consent state and the device context
it changed in - but the projection is no longer taken from the request, and the
now-unreachable JSON.parse of the caller's value is removed.
Note the surrounding hardening pass validated user-supplied *queries* via
common.parseUserQuery but never covered projections; the only projection
sanitizer in the tree (dbviewer's sanitizeProjection) constrains values to 0/1
and not which fields may be requested, so it would not have closed this.
Regression tests assert that project={}, an explicit projection naming
name/email/username/custom, and the projection alias parameter all return only
the allowed fields.
Co-Authored-By: Claude <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
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.
Reported via
security@count.ly. Assessed against master321a6645591e21c6e41f9834614d56126b780f45.The issue
/o/app_users/consentsbuilt its projection asparams.qstring.project || params.qstring.projection || {defaults}, so the narrow default applied only when the caller omitted both parameters. A query-string value arrives as a string, soproject={}is truthy and short-circuits the chain, and an empty projection means "all fields" to MongoDB. The projection was then forwarded unvalidated intoappUsers.search()and on tofind(query, project).A member holding nothing but
compliance_hubread on an app could therefore turn a consent-status table into fullapp_usersdocuments:name,email,username,phone,organization, customer-definedcustomproperties, payment totals (lp/lpa/tp/tpc), city/region, acquisition source and device details.The Compliance Hub UI never sends a projection, so the parameter was attacker-facing only.
What changes
The consent table renders a fixed set of columns, so the projection is now a server-side constant and any
project/projectionon the request is ignored. The allowlist matches the columns bound infrontend/public/templates/user.html(did,d,av,consent,lac) plusuidandappUserExportfor the per-row history and export actions./o/consent/searchis fixed the same way. Its response shape is unchanged, still fullconsent_historydocuments, which hold only consent state and the device context it changed in, but the projection is no longer taken from the request. The now-unreachableJSON.parseof the caller's value is removed so the pattern does not look like it is still there.Worth noting for reviewers
The surrounding hardening pass validated user-supplied queries via
common.parseUserQuerybut never covered projections. The only projection sanitizer in the tree, DB Viewer'ssanitizeProjection, constrains projection values to 0/1 and not which fields may be requested, so reusing it would not have closed this.Tests
Four cases in
plugins/compliance-hub/tests.js:project={}, an explicit projection namingname/email/username/custom, and theprojectionalias, each asserting only allowlisted fields come back._idis in the allowed set because Mongo returns it unless explicitly excluded.Verified locally:
npx eslint plugins/compliance-hub/andnode --checkpass. The plugin suite runs in CI (test-api-plugins).🤖 Generated with Claude Code