[fix][core] keep caller input out of the export query specification - #7894
Open
ar2rsawseen wants to merge 2 commits into
Open
[fix][core] keep caller input out of the export query specification#7894ar2rsawseen wants to merge 2 commits into
ar2rsawseen wants to merge 2 commits into
Conversation
/o/export/requestQuery re-runs an internal endpoint and uses the collection and pipeline that endpoint returns. That is sound while the specification is the endpoint's own work: the endpoint authorizes the request and builds the query itself, so the query inherits its authorization. views, heatmaps and surveys all work that way. It stopped being the endpoint's own work because the caller's `data` was forwarded as the inner request's body. processRequest copies every body key onto params.qstring, so anything in `data` arrives at the endpoint indistinguishable from a query parameter. An endpoint that keeps parameters it does not recognise and then returns what it stored will hand a caller-supplied collection and pipeline straight back, and the export runs it. The authorization that took place was for the endpoint's own action and had nothing to do with the resulting read. No caller sends `data`. views, heatmaps, surveys, the shared datatable export and countly.helpers all pass their parameters in the path's query string, so nothing is forwarded any more. Write stages are refused as a second, independent check. No endpoint that builds an export query has any reason to write, and none of the three does, so a specification containing $out or $merge is either a bug or an attempt. The search covers sub-pipelines inside $lookup, $unionWith and $facet, so a write cannot sit one level down. exports.fromRequest forges the same request shape but exports the endpoint's response rather than running a query built from it, so it is left alone here. Deliberately not included: restricting which endpoints may be named, and restricting pipeline stages by role. The first needs each producing plugin to declare itself and belongs with those plugins. The second cannot be applied as it stands because views legitimately emits $lookup and surveys legitimately emits $unionWith, and the existing dbviewer guard strips rather than rejects, so it would quietly return wrong export data for non-global members.
…y producers The endpoint named in `path` supplies the collection and pipeline that /o/export/requestQuery executes, so it decides what the export reads. Any endpoint could be named, and the returned object was trusted whenever it carried `collection` and `pipeline` keys, so the guarantee that a query was built by our own code under its own authorization rested on nothing. Endpoints that build export queries now say so by answering /export/query/producers, declaring the path they answer on, the parameter values that select their export branch, and the database their query targets. Anything not declared is refused instead of being re-run. The path is parsed rather than matched on substrings, since a repeated parameter leaves "method=views" present in a string whose effective value is something else. Paths outside /o are refused outright: a write endpoint is never a producer. The declared parameters are then pinned into the inner request, so the caller cannot select a producer and then steer it into a different mode. The database comes from the declaration too, rather than from the caller's query string. The surveys client sends db=countly_drill from the browser today, which is why that had to move to the producer. views declares itself here. heatmaps and surveys declare themselves in countly-enterprise-plugins, and heatmaps also in countly-platform. Not applied here, deliberately: restricting pipeline stages by role. views legitimately emits $lookup and surveys legitimately emits $unionWith, and the dbviewer aggregation guard strips rather than rejects, so reusing it would quietly return wrong export data to non-global members. A spec built by a producer under its own authorization is trusted; only its provenance needed establishing.
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.
/o/export/requestQueryre-runs an internal endpoint and uses the collection and pipeline that endpoint returns. That is sound while the specification is the endpoint's own work: the endpoint authorises the request and builds the query itself, so the query inherits that authorisation.views,heatmapsandsurveysall work this way, returning{db, collection, pipeline, projection}from a dedicated branch.It stopped being the endpoint's own work because the caller's
datawas forwarded as the inner request's body, andprocessRequestcopies every body key ontoparams.qstring:So anything in
datareaches the endpoint indistinguishable from a query parameter. An endpoint that keeps parameters it does not recognise and then returns what it stored will hand a caller-suppliedcollectionandpipelinestraight back, and the export runs it. The authorisation that took place was for the endpoint's own action and had nothing to do with the resulting read.What changed
datais no longer forwarded. No caller sends it:views,heatmaps,surveys, the shared datatable export andcountly.helpersall pass their parameters in the path's query string. So the inner body is empty and there is nothing for any endpoint to echo.Write stages are refused, as a second and independent check. No endpoint building an export query has any reason to write, and none of the three emits one, so a specification containing
$outor$mergeis a bug or worse. The search covers sub-pipelines inside$lookup,$unionWithand$facet, so a write cannot sit one level down.exports.fromRequestforges the same request shape but exports the endpoint's response rather than running a query built from it, so it is deliberately untouched here.Also included: only declared producers may be named
The endpoint named in
pathdecides what the export reads, so it is now refused unless it declares itself by answering/export/query/producers, giving the path it answers on, the parameter values that select its export branch, and the database its query targets.viewsdeclares itself here;heatmapsandsurveysdo so in countly-enterprise-plugins (PRs #3344 and #3345), andheatmapsalso in countly-platform.The path is parsed rather than substring-matched, since a repeated parameter leaves
method=viewspresent in a string whose effective value is something else. Paths outside/oare refused outright, because a write endpoint is never a producer. The declared parameters are then pinned into the inner request so the caller cannot select a producer and steer it into a different mode.dbcomes from the declaration too: the surveys client sendsdb=countly_drillfrom the browser today, which is why that had to move to the producer.The three producers have no shared convention —
viewsis/oplusmethod=views&action=getExportQuery,heatmapsis its own path/o/heatmaps/exportwith no parameters,surveysis/o/surveys/survey/dataplusmethod=export— so the contract is path plus required parameter values rather than fixed fields.Deliberately not included
dbvieweraggregation guard cannot be reused as-is:viewslegitimately emits$lookupandsurveyslegitimately emits$unionWith, both of whichALLOWED_STAGES_USERexcludes, andsanitizeAggregationstrips rather than rejects. Applying it would quietly return wrong export data to non-global members.Verification
The write-stage walk was exercised against pipelines shaped like all three real producers (which pass untouched) and against
$out/$mergeat the top level, inside a$unionWithsub-pipeline, inside$facet, and two levels down inside$lookup, plus non-array and null inputs.node --checkandnpx eslintclean on the countly-server branches. On countly-platform,tscreports the same 2 pre-existing errors in this file and the same 5876 repo-wide, before and after.