refactor(dbviewer): validate aggregation operators against an allow-list (release.24.05) - #7869
Open
ar2rsawseen wants to merge 4 commits into
Open
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>
…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.
Backport of #7868 to
release.24.05.Same change: the aggregation guard no longer guesses which nested arrays are sub-pipelines and no longer strips anything. It walks every object and array to any depth, checks only keys beginning with
$against a per-role allow-list, and rejects the request naming the operator and its path.find()projections reject rather than having offending fields dropped.Also carries the
password_reset.pridfix: the reset route looks that value up directly aspassword_reset.findOne({prid}), so it is withheld on all three read paths and refused as a join target for every role. The per-site copies of the redaction field list moved intoparts/redaction.jsas one table.Full reasoning in #7868.
Differences from the master PR
Not a clean cherry-pick,
api.jsneeded three adjustments:return falserather thanreturn.aggregate()takes two arguments here, with nochangesparameter, so there was nothing to keep passing.if (hasAccess). Master has extra collection-name alternatives in that condition. Kept this branch's form; it is the tighter of the two.Tests
56 cases across
plugins.dbviewer.aggregation-guard.js,plugins.dbviewer.query-guard.jsand the newplugins.dbviewer.redaction.js, all passing on this branch.Full unit suite here: 116 passing, 5 failing. All five are pre-existing and unrelated: two
Countly Requesttests make real HTTP calls, and threevalidateArgstests fail onmongodb.ObjectID is not a function, a driver rename. None touch dbviewer.🤖 Generated with Claude Code