refactor(dbviewer): validate aggregation operators against an allow-list - #7868
Open
ar2rsawseen wants to merge 5 commits into
Open
refactor(dbviewer): validate aggregation operators against an allow-list#7868ar2rsawseen wants to merge 5 commits into
ar2rsawseen wants to merge 5 commits into
Conversation
The guard used to decide which nested arrays were sub-pipelines by looking for a recognised stage name inside them, then strip whatever it disallowed. That made correctness depend on knowing every stage name MongoDB has, including undocumented internal ones, and an unrecognised name made a whole branch invisible to the filter. Replaced with a blind traversal that visits every object and array at any depth and checks only keys beginning with "$" against a per-role allow-list, matched exactly. Nothing is inferred about structure, so there is no shape to get wrong. Values are never inspected, which is what makes keys-only checking sound: MongoDB reaches a "$"-prefixed field name through $getField or $setField, where the name is a value, never a key. The pipeline is no longer modified. A disallowed operator rejects the request with its name and path instead of being deleted and the query run anyway, so a caller is told rather than silently given different results. find() projections behave the same way now. Operator lists were verified against a running MongoDB rather than the docs; verify_operators.js is that probe, kept so the lists can be rechecked on an upgrade. It found "$sharedDataDistribution" in the old list, which MongoDB spells "$shardedDataDistribution". Existing protections are unchanged: server-side JavaScript, write stages and joins into redacted collections are refused for every role at any depth. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…ipping The suite still called sanitizeProjection, which findDisallowedProjectionValue replaced, so it failed to load. Rewritten for the new contract and extended to assert the projection is left untouched when rejected. Co-Authored-By: Claude <noreply@anthropic.com>
password_reset.prid is the password-reset token: the reset route looks it up
directly as password_reset.findOne({prid}), so the value is the reset link.
The viewer returned it in full.
Withheld on all three read paths and refused as a join target for every role,
matching how members credentials are already handled.
The three redaction sites (single-document read, collection read, aggregation)
each carried their own copy of the field list, which is how a collection gets
covered in two of the three. Moved to parts/redaction.js as one table, so
adding a collection covers all three, and the module is unit-testable like the
other guards. A test asserts the aggregation and document paths cover the same
fields, so they cannot drift apart.
auth_tokens stays at the call sites: its secret is the _id, which cannot be
dropped without breaking the row, so the value is replaced instead.
Co-Authored-By: Claude <noreply@anthropic.com>
The script prints through mongosh's print(), never console, so disabling no-console was pointless and warns as an unused directive under the countly-platform eslint config. 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.
What
Reworks how DB Viewer validates a user-supplied aggregation pipeline.
The guard decided which nested arrays were sub-pipelines by looking for a recognised stage name inside them, then stripped whatever it disallowed. Two problems with that:
Now the guard walks every object and array to any depth and checks only keys beginning with
$against a per-role allow-list, matched exactly. Nothing is inferred about the pipeline's shape, so there is no shape to get wrong.Values are never inspected. That is what makes a keys-only check sound: MongoDB reaches a
$-prefixed field name through$getField/$setField, where the name is an argument value, not a key. So{ "$literal": "$lookup" }and{ "$setField": { "field": "$lookup", ... } }stay valid, while a key named$lookupis an operator every time.The pipeline is no longer modified. A disallowed operator rejects the request and names the operator and its path, instead of being deleted and the query run anyway.
find()projections behave the same way now, rather than having offending fields dropped.Allow-list vs deny-list
Deliberate. A deny-list fails open: an operator a future MongoDB adds is permitted until someone notices. An allow-list fails safe: the worst case is a valid query rejected, which arrives as a support ticket rather than as a problem.
Lists were verified against a running MongoDB instead of the docs.
verify_operators.jsis that probe, committed so the lists can be re-checked on an upgrade. It found$sharedDataDistributionin the old list, which MongoDB spells$shardedDataDistribution, so the real stage was never recognised.Unchanged
Server-side JavaScript (
$function,$accumulator,$where), write stages ($out,$merge), and joins into redacted collections are refused for every role at any depth, global admins included.Also here: the password-reset token
password_reset.pridwas returned in full. The reset route looks it up directly aspassword_reset.findOne({prid}), so the value is the reset link. It is now withheld on all three read paths and refused as a join target for every role, the same way member credentials already were.The three redaction sites (single-document read, collection read, aggregation) each carried their own copy of the field list, which is how a collection ends up covered in two of the three. Moved into
parts/redaction.jsas one table, so adding a collection covers all three, with a test asserting the aggregation and document paths cover the same fields so they cannot drift apart.auth_tokensdeliberately stays at the call sites: its secret is the_id, which cannot be dropped without breaking the row, so the value is replaced instead.Tests
56 cases across
plugins.dbviewer.aggregation-guard.js,plugins.dbviewer.query-guard.jsand the newplugins.dbviewer.redaction.js.Both existing suites needed rewriting: they asserted the stripping behaviour, and the query-guard one called the removed
sanitizeProjection, so it failed to even load. Extended on both sides: nesting the old classification handled inconsistently, plus the near-miss cases that must keep working ($mergeObjectsnext to$merge,$literaland$setFieldwith$-prefixed values, window functions, real$facetbranches).Full unit suite on this branch: 199 passing. The two failures are
Countly Request, which makes real HTTP calls and fails without network; they fail the same way on a clean checkout.🤖 Generated with Claude Code