[fix][core] validate scalar request parameter types before using them in queries (24.05) - #7938
Open
ar2rsawseen wants to merge 1 commit into
Open
[fix][core] validate scalar request parameter types before using them in queries (24.05)#7938ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
… in queries (24.05)
params.qstring values are not guaranteed to be strings. api/api.js fills
params.qstring straight from formidable's fields, and a POST body sent as
application/json puts nested objects there. So a body like
{"view": {"$ne": null}} leaves params.qstring.view an object rather than the
string the endpoint expects. Form-urlencoded bracket syntax does not do this,
it keeps a literal string key, so JSON bodies are the case that matters.
Several endpoints then use such a parameter as a plain value inside a Mongo
query document. In a value position Mongo reads an object as a query
expression, so an equality match on one document becomes a match on many. Two
consequences: the query returns rows the endpoint never meant to return, and it
loses the bound on how much it has to scan. getHeatmap is the expensive one: it
builds one query from view, actionType and segment and runs it against the
drill action collection over a caller-chosen period, so widening the match
turns a single cheap request into a full scan of that collection.
Adds common.isQueryScalar and applies it where a scalar parameter reaches a
query with no type check:
plugins/views/api/api.js getHeatmap: view, actionType, segment
plugins/star-rating/api/api.js /o/feedback/data: widget_id, version,
platform, uid
/o/feedback/widgets: is_active
plugins/crashes/api/api.js method=user_crashes: uid
api/parts/mgmt/users.js fetchNotes: note_type
api/utils/requestProcessor.js /i/token/delete: tokenid
plugins/systemlogs/api/api.js member lookup: api_key
Strings and numbers pass through unchanged, so legitimate callers are
unaffected. /o/actions in particular still takes view as a string URL,
actionType as "click" or "scroll" and segment as a string; only non-scalars are
refused, with 400 and the parameter name, the same way device and period were
already checked there. null and undefined stay scalars so the existing
truthiness checks at each call site keep deciding whether an absent parameter
belongs in the query at all.
Backport of #7937.
Co-Authored-By: Claude Opus 5 <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.
Backport of #7937 to
release.24.05.What
params.qstringvalues are not guaranteed to be strings.api/api.jsfillsparams.qstringstraight from formidable'sfields, and a POST body sent asapplication/jsonputs nested objects there. A body like{"view": {"$ne": null}}leavesparams.qstring.viewan object rather than the string the endpoint expects. Form-urlencoded bracket syntax (view[$ne]=x) does not do this, it keeps a literal string key, so JSON bodies are the case that matters.Several endpoints then use such a parameter as a plain value inside a Mongo query document. In a value position Mongo reads an object as a query expression, so an equality match on one document becomes a match on many. Two consequences: the query returns rows the endpoint never meant to return, and it loses the bound on how much data it has to scan.
getHeatmapis the expensive one. It builds one query fromview,actionTypeandsegmentand runs it against the drill action collection over a caller-chosenperiod, so widening that match turns a single cheap request into a full scan of that collection. Note this differs frommaster, where the same function is an aggregation that also$unionWiths the older drill collection; the parameters and the fix are the same, the cost profile on 24.05 is a largefindrather than a large aggregation.How
Adds
common.isQueryScalar(value): true for strings, numbers, booleans,nullandundefined, false for objects and arrays.null/undefinedstay scalars on purpose, so the existing truthiness checks at each call site keep deciding whether an absent parameter belongs in the query at all.Applied where a scalar parameter reaches a query with no type check:
plugins/views/api/api.jsgetHeatmapview,actionType,segmentplugins/star-rating/api/api.js/o/feedback/datawidget_id,version,platform,uidplugins/star-rating/api/api.js/o/feedback/widgetsis_activeplugins/crashes/api/api.jsmethod=user_crashesuidapi/parts/mgmt/users.jsfetchNotesnote_typeapi/utils/requestProcessor.js/i/token/deletetokenidplugins/systemlogs/api/api.jsmember lookupapi_keyThe full survey of the pattern across
api/and every pluginapi/directory, including the sites that were already validated and the false positives, is in #7937.common.parseUserQueryandcommon.findUnsafeMongoOperatorwere considered first and are not the right tool here: they validate parameters that are queries in their own right, and they only reject the JS-executing operators, so they accept$neand$regexby design. Endpoints that legitimately take a whole user query already go through them and are untouched.Compatibility
Strings and numbers pass through unchanged, so legitimate callers see no difference.
/o/actionsstill takesviewas a string URL,actionTypeas"click"or"scroll"andsegmentas a string. Only non-scalars are refused, with a 400 naming the parameter, the same waydeviceandperiodwere already checked in the same function.Tests
test/unit-tests/api.utils.common.jsgains unit tests forcommon.isQueryScalar. The heatmap integration cases that #7937 adds are not backported:plugins/views/tests/heatmaps.jsdoes not exist on this branch.Verification
node --checkon all 8 changed files: clean.eslint -c .eslintrc.jsonon all 8 changed files: 0 errors, 0 warnings.mocha test/unit-tests/api.utils.common.js: 32 passing with this change, 28 passing on the unmodified branch, so all 4 new tests pass and nothing regressed. Both runs also show the same 3 failures in the pre-existingvalidateArgsObjectID cases; those come from the local setup (the mongodb 6.x driver was linked into this 24.05 checkout, which pins 4.17.2, andmongodb.ObjectIDwas removed in 6.x) and are identical before and after this change.