dbviewer: validate aggregation stages at every depth#7685
Closed
ar2rsawseen wants to merge 5 commits into
Closed
Conversation
The aggregation stage allow-list was only applied to the top level of the pipeline. $facet is allowed but carries sub-pipelines that were not inspected, so the allow-list was not enforced consistently inside them. Extract the allow-list and sanitizer into plugins/dbviewer/api/parts/ aggregation_guard.js and apply it recursively, descending into $facet sub-pipelines at every depth (dropping sub-pipelines/stages emptied by sanitization so no empty $facet pipeline is produced). Top-level behaviour is unchanged. Adds unit tests for the recursive handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ar2rsawseen
force-pushed
the
fix/dbviewer-recursive-agg-guard
branch
from
f97b608 to
2ebbac8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the DB Viewer plugin’s aggregation-stage guard by moving the allow-list/sanitizer into a dedicated module and ensuring sanitization recurses into $facet sub-pipelines at any nesting depth, closing a bypass where blocked stages (e.g. $lookup, $unionWith) could be smuggled via $facet.
Changes:
- Extracted the aggregation stage allow-list and sanitization logic into
plugins/dbviewer/api/parts/aggregation_guard.js. - Updated the sanitizer to recursively inspect and clean
$facetsub-pipelines, removing emptied sub-pipelines/stages to avoid invalid empty$facetpipelines. - Added unit tests covering top-level behavior, nested
$facetbypass cases (including$facet-within-$facet), and empty-sub-pipeline cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
plugins/dbviewer/api/parts/aggregation_guard.js |
New dedicated module implementing recursive $facet sanitization for aggregation pipelines. |
plugins/dbviewer/api/api.js |
Switched to importing the sanitizer from the new module (removing inline implementation). |
test/unit-tests/plugins.dbviewer.aggregation-guard.js |
Added unit tests validating top-level and deeply nested $facet sanitization behavior. |
ar2rsawseen
changed the title
…ecific Recurse into any kept stage's sub-pipelines by structure ($facet's sub-pipelines today, plus any stage exposing a .pipeline array) so the guard keeps holding if the allow-list ever gains another pipeline-bearing stage. Adds a test simulating a future allow-listed pipeline-bearing stage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r admins) The members / auth_tokens redaction is only applied to the top-level source collection, so a join into them (//) returns raw, un-redacted documents (api_key, password, token values). Global admins skip the stage sanitizer, so they could read these via a join even though the top-level redaction intentionally denies them. Detect joins/unions into the redacted collections (members, auth_tokens) at any depth — including sub-pipelines and nested .pipeline arrays — and reject such aggregations on both the admin and non-admin paths. Adds unit tests for top-level, $facet-nested, .pipeline-nested, $unionWith (object and string forms) and $graphLookup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use === true (allow-list) and === true (protected-collection set) so inherited Object.prototype keys (constructor, __proto__, …) on a user-controlled stage object are never mistaken for allow-listed/known entries. Adds a regression test for prototype-key stage names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
findProtectedCollectionJoin now walks every nested object/array rather than only $facet sub-pipelines and .pipeline arrays, so a join into a redacted collection smuggled inside any (incl. future) stage shape is still detected. Detection-only, so a blanket deep walk is safe. Adds a test for an arbitrary nested stage shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
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.
Summary
Hardening of the DB Viewer aggregation guard.
Changes
Validate aggregation stages at every depth. The stage allow-list was only applied to the top level of the pipeline.
$facetis allowed but carries sub-pipelines that were not inspected, so the allow-list was not enforced consistently inside them. The sanitizer now applies recursively, descending into$facetsub-pipelines (and, structurally, any kept stage exposing a.pipeline) at every depth, dropping sub-pipelines/stages emptied by sanitization.Block joins into redacted collections. The
members/auth_tokensfield redaction is only applied to the top-level source collection, so a join ($lookup/$unionWith/$graphLookup) into them would return un-redacted documents. Aggregations that join/union one of these collections at any depth are now rejected on both the admin and non-admin paths.Extracted the allow-list + guards into
plugins/dbviewer/api/parts/aggregation_guard.jsso they can be unit-tested in isolation.Top-level behaviour for ordinary pipelines is unchanged.
Tests
test/unit-tests/plugins.dbviewer.aggregation-guard.js— 15 cases covering recursive stage sanitization (top-level, nested$facet,$facet-in-$facet, generic.pipeline, empty-cleanup) and protected-collection join detection ($lookup/$unionWith/$graphLookup, nested forms). All passing.🤖 Generated with Claude Code