dbviewer: harden aggregation and query handling#7686
Merged
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>
…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>
…ope, write stages) - restrict find() projections to plain field include/exclude, dropping expression / field-path alias values - treat the _id search term as a literal (escape regex metacharacters) - scope single-document lookups to the caller's apps, like the listing path - reject write stages ($out / $merge) in aggregations on all paths - exclude members.two_factor_auth from viewer output alongside password/api_key Adds unit tests for projection sanitization, search-term escaping and write-stage detection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the DB Viewer plugin’s handling of user-supplied MongoDB queries/aggregations to reduce credential exposure and prevent unsafe query features (regex ReDoS, write stages, projection expressions).
Changes:
- Added a query-guard module to sanitize
find()projections (include/exclude only) and escape_idsearch terms for literal regex matching. - Scoped single-document lookups to the same app-based base filter used in listing paths, and expanded member redaction to include
two_factor_auth. - Rejected aggregation write stages (
$out/$merge) on both global-admin and non-admin aggregation paths; added unit tests for the new guards.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
plugins/dbviewer/api/api.js |
Applies projection sanitization and literal _id search escaping; scopes document fetches; blocks aggregation write stages; redacts two_factor_auth. |
plugins/dbviewer/api/parts/query_guard.js |
New helper module for projection sanitization and regex escaping. |
plugins/dbviewer/api/parts/aggregation_guard.js |
Adds recursive detection of write stages and exports guard helpers. |
test/unit-tests/plugins.dbviewer.query-guard.js |
Adds unit tests for projection sanitization, regex escaping, and write-stage detection. |
…on stage Previously the members/auth_tokens redaction was inserted after any leading $match stages, so a leading $match (e.g. using $expr) could reference the raw credential fields before they were removed. Insert the redaction at the front so no user-supplied stage ever sees the raw password / api_key / two_factor_auth (members) or token _id (auth_tokens). 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>
Consistent with the allow-list / protected-collection hardening: WRITE_STAGES lookup compares === true so inherited Object.prototype keys can't be mistaken for a known write stage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t 0/1
- coerce a projection that parses to null/array (or non-object) to {} before
sanitizing/use, so an invalid projection can't reach find()
- sanitizeProjection now keeps only 0, 1 or booleans; other numbers (2, NaN, …)
are dropped, keeping the projection within valid include/exclude semantics
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The stage allow-list only inspects pipeline stages, and stripUnsafeMongoOperators was applied to the find() filter/sort but not to the aggregation pipeline. So $function / $accumulator / $where could sit inside an allowed stage's expression ($project / $group / $addFields …) and execute server-side JavaScript — reachable on both the admin and non-admin aggregate paths. Deep-scan the whole pipeline (objects/arrays at every depth, including expression values) for these operators and reject the request on both paths, matching how the find() path already strips them. Adds unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- cap find() limit and aggregation iDisplayLength at 10000 (NaN-safe), and guard skip, so a crafted page size can't request an unbounded result set - return a generic 500 message (logging the real error server-side) instead of passing raw MongoDB error objects back to the client 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>
findWriteStage now walks every nested object/array (like findProtectedCollectionJoin and findServerSideJs), so $out/$merge nested in any future stage shape is still detected. Detection-only, so the blanket deep walk is safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
ar2rsawseen
changed the title
ar2rsawseen
changed the base branch from
fix/dbviewer-recursive-agg-guard
to
master
The stage sanitizer descended into nested sub-pipelines only for $facet and a .pipeline field. Recognize sub-pipelines by shape instead (an array whose elements are stage objects), so a blocked stage nested in any — including future — pipeline-bearing shape is stripped, while ordinary expression arrays (e.g. $concat operands) are left intact. Adds tests for an arbitrary nested shape and for the expression-array safety case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the four separate aggregation checks (stage allow-list + three deny detectors) with a single sanitizeAggregation(pipeline, allowedStages): - two role lists: ALLOWED_STAGES_USER (no joins/writes) and ALLOWED_STAGES_GLOBAL_ADMIN (adds $lookup/$graphLookup/$unionWith). The same list-driven recursive stripper runs for both paths; anything not in the applicable list is stripped at every depth. - two role-independent hard rules, rejected at any depth: server-side-JS operators ($function/$accumulator/$where) and joins/unions into the redacted collections (members/auth_tokens). Write stages ($out/$merge) are in no list, so they are simply stripped. Global admins now go through the (broader) allow-list too, so the previous admin-only deny detectors are no longer needed. Both /o/db aggregation paths call the same routine, differing only by which list they pass. Tests rewritten around the new API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cookiezaurs
approved these changes
Jun 9, 2026
This was referenced Jun 9, 2026
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.
Consolidated DB Viewer hardening — this PR now contains the full commit history of both the aggregation-guard work and the query-hardening work (previously split as #7685 and #7686-stacked). #7685 is folded in here; retargeted to master.
Aggregation
$facetsub-pipelines (recursive allow-list); require an explicit allow-list match so inheritedObject.prototypekeys (constructor/__proto__) are never treated as allowed.members/auth_tokens), write stages ($out/$merge), and server-side-JS operators ($function/$accumulator/$where) — all via a full deep walk, so any (incl. future) nested stage shape is covered. Applied on both admin and non-admin paths.members/auth_tokensredaction as the first pipeline stage.Find / document
0/1/booleans); non-object projection normalized to{}._idsearch term (sSearch) treated as a literal (no ReDoS).members.two_factor_authredacted alongsidepassword/api_key.limit/iDisplayLength); generic 500s instead of raw MongoDB errors.Tests
33 cases across
plugins.dbviewer.aggregation-guard.jsandplugins.dbviewer.query-guard.js.🤖 Generated with Claude Code