From e0fa42db4efd8e4f102b7c2ffe95fe3454b5a43a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:44:58 +0300 Subject: [PATCH 1/5] refactor(dbviewer): validate aggregation operators against an allow-list 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 --- plugins/dbviewer/api/api.js | 60 ++- .../dbviewer/api/parts/aggregation_guard.js | 444 +++++++++--------- plugins/dbviewer/api/parts/query_guard.js | 37 +- .../dbviewer/api/parts/verify_operators.js | 166 +++++++ .../plugins.dbviewer.aggregation-guard.js | 212 ++++++--- 5 files changed, 589 insertions(+), 330 deletions(-) create mode 100644 plugins/dbviewer/api/parts/verify_operators.js diff --git a/plugins/dbviewer/api/api.js b/plugins/dbviewer/api/api.js index 8ac9d1c731b..27bf7be1a94 100644 --- a/plugins/dbviewer/api/api.js +++ b/plugins/dbviewer/api/api.js @@ -15,11 +15,11 @@ const FEATURE_NAME = 'dbviewer'; // upper bound on rows returned per find()/aggregation page, to keep a crafted // limit/iDisplayLength from requesting an unbounded result set const MAX_DBVIEWER_LIMIT = 10000; -// Aggregation-stage allow-list and the recursive sanitizer that strips blocked -// stages at every depth (including inside $facet sub-pipelines). Kept in a -// dedicated module so it can be unit-tested in isolation. -const { sanitizeAggregation, ALLOWED_STAGES_USER, ALLOWED_STAGES_GLOBAL_ADMIN } = require('./parts/aggregation_guard.js'); -const { sanitizeProjection, escapeRegExp } = require('./parts/query_guard.js'); +// Allow-list of MongoDB operators and the validator that rejects a pipeline using +// anything outside it, at any depth. Kept in a dedicated module so it can be +// unit-tested in isolation. +const { sanitizeAggregation, ALLOWED_OPERATORS_USER, ALLOWED_OPERATORS_GLOBAL_ADMIN } = require('./parts/aggregation_guard.js'); +const { findDisallowedProjectionValue, escapeRegExp } = require('./parts/query_guard.js'); var spawn = require('child_process').spawn, child; @@ -238,10 +238,15 @@ var spawn = require('child_process').spawn, common.returnMessage(params, 400, common.unsafeQueryError(badOp)); return false; } - //restrict the projection to plain field include/exclude — drop any - //expression / field-path alias (e.g. {x:"$password"}) that could - //compute or rename fields the viewer otherwise removes - sanitizeProjection(projection); + //the projection must be plain field include/exclude. An expression or + //field-path alias (e.g. {x:"$password"}) could compute or rename fields + //the viewer otherwise removes, so reject rather than quietly dropping + //the field and returning different results than were asked for + var badProjection = findDisallowedProjectionValue(projection); + if (badProjection) { + common.returnMessage(params, 400, 'Projection for "' + badProjection.name + '" must be 0 or 1'); + return; + } var base_filter = {}; if (!params.member.global_admin) { @@ -573,26 +578,31 @@ var spawn = require('child_process').spawn, } // handle aggregation request else if (isContainDb && params.qstring.aggregation) { - // Validate the pipeline against the caller's allow-list and run - // it. Global admins get the broader list (may join/union, but - // still no writes and never into a redacted collection); other - // users get the restricted list. Disallowed stages are stripped; - // server-side-JS operators and joins into members/auth_tokens - // reject the request. - var runAggregation = function(allowedStages) { + // Validate the pipeline against the caller's allow-list of MongoDB + // operators and run it unchanged. Global admins get the broader + // list (they may join and union, but still not into a redacted + // collection); other users get the restricted one. Anything the + // caller may not use rejects the request rather than being removed + // from the pipeline, so the result is never quietly different from + // what was asked for. + var runAggregation = function(allowedOperators) { try { let aggregation = EJSON.parse(params.qstring.aggregation); - var guard = sanitizeAggregation(aggregation, allowedStages); + var guard = sanitizeAggregation(aggregation, allowedOperators); if (guard.error) { - var msg = guard.error.type === "join" - ? 'Aggregation may not join the "' + guard.error.name + '" collection' - : 'Aggregation may not use the "' + guard.error.name + '" operator'; + var msg; + if (guard.error.type === "join") { + msg = 'Aggregation may not join the "' + guard.error.name + '" collection'; + } + else if (guard.error.type === "stage") { + msg = 'Aggregation stage at ' + guard.error.where + ' does not name a pipeline stage'; + } + else { + msg = 'Aggregation may not use the "' + guard.error.name + '" operator (at ' + guard.error.where + ')'; + } common.returnMessage(params, 400, msg); return; } - if (guard.changes && Object.keys(guard.changes).length > 0) { - log.d("Removed stages from pipeline: ", JSON.stringify(guard.changes)); - } aggregate(params.qstring.collection, aggregation, guard.changes); } catch (e) { @@ -600,12 +610,12 @@ var spawn = require('child_process').spawn, } }; if (params.member.global_admin) { - runAggregation(ALLOWED_STAGES_GLOBAL_ADMIN); + runAggregation(ALLOWED_OPERATORS_GLOBAL_ADMIN); } else { userHasAccess(params, params.qstring.collection, function(hasAccess) { if (hasAccess || params.qstring.collection === "events_data" || params.qstring.collection === "drill_events") { - runAggregation(ALLOWED_STAGES_USER); + runAggregation(ALLOWED_OPERATORS_USER); } else { common.returnMessage(params, 401, 'User does not have right tot view this colleciton'); diff --git a/plugins/dbviewer/api/parts/aggregation_guard.js b/plugins/dbviewer/api/parts/aggregation_guard.js index 0f83b196f2d..a714c82c29d 100644 --- a/plugins/dbviewer/api/parts/aggregation_guard.js +++ b/plugins/dbviewer/api/parts/aggregation_guard.js @@ -1,104 +1,178 @@ /** * @module plugins/dbviewer/api/parts/aggregation_guard * @description Validates a DB Viewer aggregation pipeline against a role-specific - * allow-list of stages, and rejects pipelines that use server-side-JavaScript - * operators or join/union into a redacted (credential) collection. + * allow-list of MongoDB operators, and rejects pipelines that join or union into a + * redacted (credential) collection. * - * One routine, two lists: - * - non-global users get ALLOWED_STAGES_USER (no joins, no writes); - * - global admins get ALLOWED_STAGES_GLOBAL_ADMIN (the same plus join/union - * stages). - * Anything not in the applicable list is stripped from the pipeline at every - * depth. Two hard rules apply to everyone, at any depth, and reject the request: - * - no $function / $accumulator / $where (server-side JavaScript); - * - no join/union into members / auth_tokens (their field redaction only - * applies to the top-level source collection, so a join would return raw - * credentials — denied even for global admins). + * HOW THIS WORKS, AND WHY IT IS SHAPED THIS WAY + * + * The pipeline is walked blindly: every object and array, at every depth, with no + * attempt to work out which arrays are sub-pipelines and which are ordinary + * expression arrays. Only keys are examined, and only keys beginning with "$". + * Any such key that is not on the allow-list rejects the whole request. + * + * Two properties make that both safe and complete: + * + * - MongoDB will not let a "$"-prefixed key be anything other than an operator. + * A document may store a field named "$price", but it can only be read through + * $getField, where the name is a VALUE. {$match:{"$price":5}} is an error + * ("unknown top level operator"), as is projecting or sorting on it. So every + * "$"-prefixed key in a valid pipeline is an operator, and there is no + * legitimate query this rejects. + * - A stage only executes if its name appears as a literal key in the submitted + * pipeline. Computed values never become stages: an object built with $setField + * whose key is "$lookup" is data, and joins nothing. So checking literal keys + * catches everything that can run. + * + * KEYS ONLY, NEVER VALUES. {$literal:"$lookup"} is a legitimate expression + * returning the string "$lookup", and "$fieldname" / "$$ROOT" are ordinary value + * references. Inspecting values would reject valid queries. + * + * EXACT MATCH ONLY. $mergeObjects is legitimate and must not be caught by a prefix + * or substring test for $merge. + * + * WHY AN ALLOW-LIST RATHER THAN A LIST OF DANGEROUS OPERATORS + * + * An omission here rejects a valid query, which is a support ticket. An omission + * from a list of dangerous operators permits an exfiltration, which is a + * vulnerability. Those costs are not equal, so this fails closed: an operator + * introduced by a future MongoDB release is rejected until reviewed and added. + * + * The previous two versions of this guard both failed by trying to identify + * sub-pipelines from their contents, which let one unrecognised sibling stage hide + * a $lookup from the stage filter. Nothing here infers structure. If a future + * change reintroduces inference, make it fail closed. + * + * MAINTENANCE. The lists below were verified against a live MongoDB 8.0 by probing + * every entry; verify_operators.js in this directory is the means of re-verifying + * them. Run it on a MongoDB upgrade: operators added by the new release will be + * rejected until added here, and entries that no longer exist should be removed. + * That probe found a real misspelling in the previous version of this file, + * "$sharedDataDistribution" for "$shardedDataDistribution", so do not hand-edit + * these lists without re-running it. + * + * DELIBERATELY ABSENT, and therefore rejected: + * - joins and unions for non-global users: $lookup, $graphLookup, $unionWith + * (they read a second collection, bypassing the per-collection access check) + * - writes: $out, $merge + * - server-side JavaScript: $function, $accumulator, $where + * - server, cluster and internal introspection: $currentOp, $collStats, + * $indexStats, $planCacheStats, $listSessions, $listLocalSessions, + * $listSampledQueries, $listSearchIndexes, $shardedDataDistribution, + * $changeStream, $changeStreamSplitLargeEvents, $mergeCursors, $documents, + * $listClusterCatalog, and every $_internal* stage */ 'use strict'; -// Stages a non-global user may run. No joins/unions (they read a second -// collection, bypassing the per-collection access check) and no writes. -const ALLOWED_STAGES_USER = { - "$addFields": true, - "$bucket": true, - "$bucketAuto": true, - "$count": true, - "$densify": true, - "$facet": true, - "$fill": true, - "$geoNear": true, - "$group": true, - "$limit": true, - "$match": true, - "$project": true, - "$querySettings": true, - "$redact": true, - "$replaceRoot": true, - "$replaceWith": true, - "$sample": true, - "$search": true, - "$searchMeta": true, - "$set": true, - "$setWindowFields": true, - "$skip": true, - "$sort": true, - "$sortByCount": true, - "$unset": true, - "$unwind": true, - "$vectorSearch": true //atlas specific -}; +// Pipeline stages a non-global user may run. Unchanged from the previous version of +// this guard, so this is not a widening of what anyone can do. +const STAGES_USER = [ + "$addFields", "$bucket", "$bucketAuto", "$count", "$densify", "$facet", + "$fill", "$geoNear", "$group", "$limit", "$match", "$project", + "$querySettings", "$redact", "$replaceRoot", "$replaceWith", "$sample", + "$search", "$searchMeta", "$set", "$setWindowFields", "$skip", "$sort", + "$sortByCount", "$unset", "$unwind", "$vectorSearch" +]; -// Global admins may additionally use the join/union stages. Still no write -// stages, and still never into a protected collection (see findHardViolation). -const ALLOWED_STAGES_GLOBAL_ADMIN = Object.assign({}, ALLOWED_STAGES_USER, { - "$lookup": true, - "$graphLookup": true, - "$unionWith": true -}); +// Join and union stages, for global admins only. Still never into a protected +// collection: see findProtectedJoin below. +const STAGES_GLOBAL_ADMIN_ONLY = ["$lookup", "$graphLookup", "$unionWith"]; -// Expression operators that run server-side JavaScript. Never allowed for -// anyone, at any depth — they live inside otherwise-allowed stages. -const DENIED_OPERATORS = { - "$function": true, - "$accumulator": true, - "$where": true -}; +// Expression operators, accumulators and window operators. These live inside +// stages rather than being stages, and a blind walk meets them constantly, so they +// have to be listed or every real query is rejected. +const EXPRESSION_OPERATORS = [ + // arithmetic + "$abs", "$add", "$ceil", "$divide", "$exp", "$floor", "$ln", "$log", + "$log10", "$mod", "$multiply", "$pow", "$round", "$sqrt", "$subtract", + "$trunc", + // array + "$arrayElemAt", "$arrayToObject", "$concatArrays", "$filter", "$first", + "$firstN", "$in", "$indexOfArray", "$isArray", "$last", "$lastN", "$map", + "$maxN", "$minN", "$objectToArray", "$range", "$reduce", "$reverseArray", + "$size", "$slice", "$sortArray", "$zip", + // boolean and comparison + "$and", "$not", "$or", "$cmp", "$eq", "$gt", "$gte", "$lt", "$lte", "$ne", + // conditional + "$cond", "$ifNull", "$switch", + // data size + "$binarySize", "$bsonSize", + // date + "$dateAdd", "$dateDiff", "$dateFromParts", "$dateFromString", + "$dateSubtract", "$dateToParts", "$dateToString", "$dateTrunc", + "$dayOfMonth", "$dayOfWeek", "$dayOfYear", "$hour", "$isoDayOfWeek", + "$isoWeek", "$isoWeekYear", "$millisecond", "$minute", "$month", "$second", + "$week", "$year", "$tsIncrement", "$tsSecond", + // literal, object, variable and misc + "$literal", "$getField", "$rand", "$setField", "$unsetField", + "$mergeObjects", "$let", + // set + "$allElementsTrue", "$anyElementTrue", "$setDifference", "$setEquals", + "$setIntersection", "$setIsSubset", "$setUnion", + // string + "$concat", "$indexOfBytes", "$indexOfCP", "$ltrim", "$regexFind", + "$regexFindAll", "$regexMatch", "$replaceOne", "$replaceAll", "$rtrim", + "$split", "$strLenBytes", "$strLenCP", "$strcasecmp", "$substr", + "$substrBytes", "$substrCP", "$toLower", "$trim", "$toUpper", + // text score + "$meta", + // trigonometry + "$sin", "$cos", "$tan", "$asin", "$acos", "$atan", "$atan2", "$asinh", + "$acosh", "$atanh", "$sinh", "$cosh", "$tanh", "$degreesToRadians", + "$radiansToDegrees", + // type conversion + "$convert", "$isNumber", "$toBool", "$toDate", "$toDecimal", "$toDouble", + "$toInt", "$toLong", "$toObjectId", "$toString", "$type", "$toUUID", + // accumulators, valid in $group and/or $setWindowFields + "$addToSet", "$avg", "$bottom", "$bottomN", "$covariancePop", + "$covarianceSamp", "$denseRank", "$derivative", "$documentNumber", + "$expMovingAvg", "$integral", "$linearFill", "$locf", "$max", "$median", + "$min", "$percentile", "$push", "$rank", "$shift", "$stdDevPop", + "$stdDevSamp", "$sum", "$top", "$topN" +]; -// Collections whose contents DB Viewer redacts. A join/union into them would -// return the raw documents (redaction only applies to the top-level source), -// so such joins are rejected for everyone — including global admins. +// Query operators, reachable through $match. $where is absent on purpose: it runs +// server-side JavaScript. +const QUERY_OPERATORS = [ + "$eq", "$gt", "$gte", "$in", "$lt", "$lte", "$ne", "$nin", + "$and", "$not", "$nor", "$or", + "$exists", "$type", "$expr", "$jsonSchema", "$mod", "$regex", "$options", + "$text", "$geoIntersects", "$geoWithin", "$near", "$nearSphere", + "$all", "$elemMatch", "$size", + "$bitsAllClear", "$bitsAllSet", "$bitsAnyClear", "$bitsAnySet", + "$comment" +]; + +/** + * Build a lookup object from operator name lists. + * @returns {object} map of operator name to true + */ +function toMap() { + var map = {}; + for (var i = 0; i < arguments.length; i++) { + var list = arguments[i]; + for (var j = 0; j < list.length; j++) { + map[list[j]] = true; + } + } + return map; +} + +const ALLOWED_OPERATORS_USER = toMap(STAGES_USER, EXPRESSION_OPERATORS, QUERY_OPERATORS); +const ALLOWED_OPERATORS_GLOBAL_ADMIN = toMap(STAGES_USER, STAGES_GLOBAL_ADMIN_ONLY, EXPRESSION_OPERATORS, QUERY_OPERATORS); + +// Collections whose contents DB Viewer redacts. A join or union into them would +// return the raw documents, because the redaction only applies to the top-level +// source collection, so those are rejected for everyone including global admins. const PROTECTED_JOIN_COLLECTIONS = { "members": true, "auth_tokens": true }; -// All recognized aggregation STAGE operators (any role's allow-list plus the -// known blocked stages), used to recognize a nested array as a sub-pipeline -// (array of stage objects) and tell it apart from an ordinary expression array. -const KNOWN_STAGE_OPERATORS = Object.assign({ - "$out": true, - "$merge": true, - "$documents": true, - "$collStats": true, - "$indexStats": true, - "$currentOp": true, - "$listSessions": true, - "$listLocalSessions": true, - "$listSampledQueries": true, - "$listSearchIndexes": true, - "$planCacheStats": true, - "$changeStream": true, - "$changeStreamSplitLargeEvents": true, - "$mergeCursors": true, - "$sharedDataDistribution": true -}, ALLOWED_STAGES_GLOBAL_ADMIN); - /** - * Extract the collection name from a join "from"/"coll" reference, which may be - * a plain string ("members") or the cross-database object form - * ({ db, coll }). Returns [] when no collection name can be determined. + * Extract the collection name from a join "from"/"coll" reference, which may be a + * plain string ("members") or the cross-database object form ({ db, coll }). * @param {*} from - a $lookup.from / $graphLookup.from / $unionWith.coll value * @returns {string[]} the referenced collection name(s) */ @@ -113,7 +187,7 @@ function collectionOf(from) { } /** - * Collection names a single stage joins / unions from. + * Collection names a single stage joins or unions from. * @param {object} stage - one aggregation stage * @returns {string[]} target collection names referenced by join/union operators */ @@ -140,47 +214,16 @@ function joinTargetsOf(stage) { } /** - * Does this array look like an aggregation sub-pipeline — a non-empty array - * whose every element is a stage object (an object carrying a recognized stage - * operator key)? Expression arrays (e.g. $concat operands) do not match. - * @param {*} arr - candidate value - * @returns {boolean} true if it is a sub-pipeline - */ -function isSubPipeline(arr) { - if (!Array.isArray(arr) || arr.length === 0) { - return false; - } - for (var i = 0; i < arr.length; i++) { - var el = arr[i]; - if (!el || typeof el !== "object" || Array.isArray(el)) { - return false; - } - var isStage = false; - for (var k in el) { - if (Object.prototype.hasOwnProperty.call(el, k) && KNOWN_STAGE_OPERATORS[k] === true) { - isStage = true; - break; - } - } - if (!isStage) { - return false; - } - } - return true; -} - -/** - * Deep-walk a node for a hard-rule violation that must reject the whole request, - * regardless of role: a server-side-JS operator, or a join/union into a - * protected (redacted) collection. Detection-only (no mutation), so a blanket - * deep walk is safe and stays correct for any (incl. future) nested shape. - * @param {*} node - pipeline / stage / expression node - * @returns {{type: string, name: string}|null} the violation, or null + * Deep-scan for a join or union into a protected collection, at any depth. This one + * inspects VALUES (the "from"/"coll" names) rather than keys, so it stays a + * separate pass from the operator check. + * @param {*} node - pipeline / stage / expression node (not mutated) + * @returns {object|null} { name } of the protected collection, or null */ -function findHardViolation(node) { +function findProtectedJoin(node) { if (Array.isArray(node)) { for (var i = 0; i < node.length; i++) { - var inArr = findHardViolation(node[i]); + var inArr = findProtectedJoin(node[i]); if (inArr) { return inArr; } @@ -191,17 +234,14 @@ function findHardViolation(node) { var targets = joinTargetsOf(node); for (var t = 0; t < targets.length; t++) { if (PROTECTED_JOIN_COLLECTIONS[targets[t]] === true) { - return { type: "join", name: targets[t] }; + return { name: targets[t] }; } } for (var key in node) { if (!Object.prototype.hasOwnProperty.call(node, key)) { continue; } - if (DENIED_OPERATORS[key] === true) { - return { type: "operator", name: key }; - } - var inVal = findHardViolation(node[key]); + var inVal = findProtectedJoin(node[key]); if (inVal) { return inVal; } @@ -211,112 +251,94 @@ function findHardViolation(node) { } /** - * Walk a kept stage's value and strip disallowed stages from any sub-pipeline - * nested anywhere within it (structural — $facet branches, a .pipeline field, - * or any other nested-pipeline shape). A sub-pipeline emptied by stripping is - * removed from its parent object (Mongo rejects an empty $facet branch). - * @param {*} value - the stage value (or any nested node) - * @param {object} allowedStages - the role's allow-list - * @param {object} changes - accumulator: keys are removed stage names - * @returns {void} + * Deep-scan for a "$"-prefixed KEY that is not on the allow-list. + * + * Blind traversal: no assumption about which arrays are sub-pipelines. Keys only, + * never values. Exact match, so $mergeObjects is not confused with $merge. + * + * @param {*} node - pipeline / stage / expression node (not mutated) + * @param {object} allowedOperators - allow-list for the caller's role + * @param {string} path - position of the current node, for the error message + * @returns {object|null} { name, where } of the first offending key, or null */ -function stripNested(value, allowedStages, changes) { - if (Array.isArray(value)) { - if (isSubPipeline(value)) { - stripStages(value, allowedStages, changes); - } - else { - for (var i = 0; i < value.length; i++) { - stripNested(value[i], allowedStages, changes); +function findDisallowedOperator(node, allowedOperators, path) { + if (Array.isArray(node)) { + for (var i = 0; i < node.length; i++) { + var inArr = findDisallowedOperator(node[i], allowedOperators, path + "[" + i + "]"); + if (inArr) { + return inArr; } } - return; + return null; } - if (value && typeof value === "object") { - for (var k in value) { - if (!Object.prototype.hasOwnProperty.call(value, k)) { + if (node && typeof node === "object") { + for (var key in node) { + if (!Object.prototype.hasOwnProperty.call(node, key)) { continue; } - var child = value[k]; - if (Array.isArray(child) && isSubPipeline(child)) { - stripStages(child, allowedStages, changes); - if (child.length === 0) { - delete value[k]; - } + // require an explicit `true` so inherited Object.prototype keys + // (constructor, __proto__, ...) are never treated as allow-listed + if (key.charAt(0) === "$" && allowedOperators[key] !== true) { + return { name: key, where: path + "." + key }; } - else { - stripNested(child, allowedStages, changes); + var inVal = findDisallowedOperator(node[key], allowedOperators, path + "." + key); + if (inVal) { + return inVal; } } } + return null; } /** - * Recursively remove stages not in `allowedStages` from a pipeline, at every - * depth. Mutates the pipeline in place. - * @param {Array} pipeline - aggregation pipeline - * @param {object} allowedStages - the role's allow-list (values must be === true) - * @param {object} changes - accumulator: keys are removed stage names - * @returns {void} + * Validate an aggregation pipeline. The pipeline is never modified: one that uses + * something it may not use is rejected, so the caller either runs exactly what was + * asked for or gets an error explaining why not. + * + * @param {Array} pipeline - parsed aggregation pipeline (not mutated) + * @param {object} allowedOperators - ALLOWED_OPERATORS_USER or _GLOBAL_ADMIN + * @returns {{changes: object, error: ({type: string, name: string, where: string}|null)}} + * When error is set the caller must reject the request. changes is always + * empty and is kept only so the response shape does not change. */ -function stripStages(pipeline, allowedStages, changes) { - if (!Array.isArray(pipeline)) { - return; +function sanitizeAggregation(pipeline, allowedOperators) { + var join = findProtectedJoin(pipeline); + if (join) { + return { changes: {}, error: { type: "join", name: join.name } }; } - for (var z = 0; z < pipeline.length; z++) { - var stage = pipeline[z]; - if (!stage || typeof stage !== "object") { - continue; - } - for (var key in stage) { - if (!Object.prototype.hasOwnProperty.call(stage, key)) { - continue; - } - // require an explicit `true` so inherited Object.prototype keys - // (constructor, __proto__, …) are never treated as allow-listed - if (allowedStages[key] !== true) { - changes[key] = true; - delete stage[key]; + var operator = findDisallowedOperator(pipeline, allowedOperators, "pipeline"); + if (operator) { + return { changes: {}, error: { type: "operator", name: operator.name, where: operator.where } }; + } + // Top-level elements are stages by position, so each must actually name one. + // This is not structural inference: it only says that a stage object carries a + // stage operator. It catches an element whose only keys are ordinary names + // ("constructor", "__proto__", a stray field) with a clear message, rather than + // handing it to MongoDB to reject. + if (Array.isArray(pipeline)) { + for (var i = 0; i < pipeline.length; i++) { + var stage = pipeline[i]; + if (!stage || typeof stage !== "object" || Array.isArray(stage)) { + return { changes: {}, error: { type: "stage", name: String(stage), where: "pipeline[" + i + "]" } }; } - else { - stripNested(stage[key], allowedStages, changes); - var v = stage[key]; - if (v && typeof v === "object" && !Array.isArray(v) && Object.keys(v).length === 0) { - delete stage[key]; + var named = false; + for (var key in stage) { + if (Object.prototype.hasOwnProperty.call(stage, key) && key.charAt(0) === "$") { + named = true; + break; } } + if (!named) { + return { changes: {}, error: { type: "stage", name: Object.keys(stage).join(","), where: "pipeline[" + i + "]" } }; + } } - if (Object.keys(stage).length === 0) { - pipeline.splice(z, 1); - z--; - } - } -} - -/** - * Validate and sanitize an aggregation pipeline for a given role's allow-list. - * Rejects (without mutating) when a hard rule is violated; otherwise strips any - * stage not in the allow-list and returns what was removed. - * @param {Array} pipeline - aggregation pipeline (mutated in place when valid) - * @param {object} allowedStages - ALLOWED_STAGES_USER or ALLOWED_STAGES_GLOBAL_ADMIN - * @returns {{changes: object, error: ({type: string, name: string}|null)}} - * When error is set the caller must reject the request and not run the - * pipeline. Otherwise changes lists the removed stage names. - */ -function sanitizeAggregation(pipeline, allowedStages) { - var violation = findHardViolation(pipeline); - if (violation) { - return { changes: {}, error: violation }; } - var changes = {}; - stripStages(pipeline, allowedStages, changes); - return { changes: changes, error: null }; + return { changes: {}, error: null }; } module.exports = { - ALLOWED_STAGES_USER, - ALLOWED_STAGES_GLOBAL_ADMIN, - DENIED_OPERATORS, + ALLOWED_OPERATORS_USER, + ALLOWED_OPERATORS_GLOBAL_ADMIN, PROTECTED_JOIN_COLLECTIONS, sanitizeAggregation }; diff --git a/plugins/dbviewer/api/parts/query_guard.js b/plugins/dbviewer/api/parts/query_guard.js index db0bbd85dd8..aa2359cafd3 100644 --- a/plugins/dbviewer/api/parts/query_guard.js +++ b/plugins/dbviewer/api/parts/query_guard.js @@ -7,36 +7,39 @@ 'use strict'; /** - * Restrict a find() projection to plain field inclusion / exclusion. + * Check that a find() projection is plain field inclusion / exclusion. * - * A projection value is only allowed to be 0, 1 or a boolean (strict - * include/exclude). Any other value is dropped: + * A projection value may only be 0, 1 or a boolean (strict include/exclude). + * Anything else rejects the request: * - expressions and field-path aliases — e.g. { leak: "$password" } or - * { x: { $function: ... } } — would compute new fields from, or rename, - * fields the viewer otherwise removes from the response (MongoDB 4.4+ find() + * { x: { $function: ... } } — would compute new fields from, or rename, fields + * the viewer otherwise removes from the response (MongoDB 4.4+ find() * projections accept expressions); - * - other numbers (2, NaN, …) are not valid include/exclude values and can - * make the query throw. - * Keeping projections to strict include/exclude removes that whole avenue. + * - other numbers (2, NaN, …) are not valid include/exclude values and can make + * the query throw. * - * @param {object} projection - parsed projection object (mutated in place) - * @returns {object} changes - keys are the projection fields that were dropped + * The offending field used to be deleted from the projection and the query run + * anyway, which meant a caller asking for something they may not have silently got + * different results instead of being told. The projection is now left untouched and + * the caller is rejected, matching how the aggregation guard behaves. + * + * @param {object} projection - parsed projection object (not mutated) + * @returns {object|null} { name } of the first offending field, or null when the + * projection is acceptable */ -function sanitizeProjection(projection) { - var changes = {}; +function findDisallowedProjectionValue(projection) { if (!projection || typeof projection !== "object" || Array.isArray(projection)) { - return changes; + return null; } for (var key in projection) { if (Object.prototype.hasOwnProperty.call(projection, key)) { var value = projection[key]; if (value !== 0 && value !== 1 && value !== true && value !== false) { - changes[key] = true; - delete projection[key]; + return { name: key }; } } } - return changes; + return null; } /** @@ -52,6 +55,6 @@ function escapeRegExp(str) { } module.exports = { - sanitizeProjection, + findDisallowedProjectionValue, escapeRegExp }; diff --git a/plugins/dbviewer/api/parts/verify_operators.js b/plugins/dbviewer/api/parts/verify_operators.js new file mode 100644 index 00000000000..d7d1f20e4cd --- /dev/null +++ b/plugins/dbviewer/api/parts/verify_operators.js @@ -0,0 +1,166 @@ +/** + * Verifies the operator allow-lists in aggregation_guard.js against a running + * MongoDB, so they are not maintained by hand and by hope. + * + * WHY THIS EXISTS + * The guard rejects any "$"-prefixed key it does not recognise. That fails safe, + * but it means a missing entry rejects a valid query, and a misspelled entry is + * both useless and invisible. The previous version of the guard carried + * "$sharedDataDistribution" where MongoDB spells it "$shardedDataDistribution"; + * this script is what found that. + * + * RUN IT on a MongoDB upgrade, and whenever the lists are edited: + * + * mongosh "" --quiet --file verify_operators.js + * + * It reports entries MongoDB does not recognise (remove or fix those) and + * confirms the deliberately excluded operators still exist (so the exclusions are + * still meaningful). It does not and cannot report operators a new MongoDB release + * has ADDED: those surface as a rejected query, which is the safe direction, and + * are added here after review. + * + * HOW THE PROBE WORKS + * Every probe is bounded with maxTimeMS and never iterates a cursor. Both matter: + * $changeStream opens a tailable cursor, so iterating it blocks forever. An + * unrecognised name is identified by MongoDB's own error code rather than by + * message text - Location40324 for a stage, Location31325 for an expression, + * code 15952 for a $group accumulator - because the wording differs between + * "Unrecognized pipeline stage name" and "Unknown expression" and is not stable. + * + * Each category has a control: a name that must be reported fake and one that must + * be reported real. If a control fails, the discriminator is wrong and the run's + * output means nothing. + */ + +/* eslint-disable no-undef, no-console */ +'use strict'; + +var probeDb = db.getSiblingDB("dbviewer_operator_probe"); +probeDb.probe.insertOne({ a: 1, arr: [1, 2], s: "x" }); + +/** + * Run a pipeline, bounded, without iterating a cursor. + * @param {Array} pipeline - pipeline to attempt + * @returns {object} { ok, code } + */ +function attempt(pipeline) { + try { + var res = probeDb.runCommand({ aggregate: "probe", pipeline: pipeline, cursor: {}, maxTimeMS: 400 }); + return { ok: res.ok === 1, code: res.code }; + } + catch (e) { + return { ok: false, code: e.code }; + } +} + +/** + * Whether MongoDB recognises a pipeline stage name. + * @param {string} op - operator name + * @returns {boolean} true when recognised + */ +function stageExists(op) { + var stage = {}; + stage[op] = {}; + var r = attempt([stage]); + return r.ok || r.code !== 40324; +} + +/** + * Whether MongoDB recognises an expression operator, in $project or as a $group + * accumulator (several are only valid in one position). + * @param {string} op - operator name + * @returns {boolean} true when recognised + */ +function expressionExists(op) { + var inner = {}; + inner[op] = []; + var r = attempt([{ $project: { x: inner } }]); + if (r.ok || r.code !== 31325) { + return true; + } + var acc = {}; + acc[op] = "$a"; + var g = attempt([{ $group: { _id: null, v: acc } }]); + return g.ok || g.code !== 15952; +} + +/** + * Whether MongoDB recognises a query operator. + * @param {string} op - operator name + * @returns {boolean} true when recognised + */ +function queryExists(op) { + var candidates = [{ f: {} }, {}]; + candidates[0].f[op] = 1; + candidates[1][op] = 1; + for (var i = 0; i < candidates.length; i++) { + try { + probeDb.runCommand({ find: "probe", filter: candidates[i], limit: 1, maxTimeMS: 400 }); + return true; + } + catch (e) { + if (!/unknown operator|unknown top level operator/i.test(e.message)) { + return true; + } + } + } + return false; +} + +var guard = { ALLOWED_OPERATORS_USER: {}, ALLOWED_OPERATORS_GLOBAL_ADMIN: {} }; +try { + guard = require("./aggregation_guard.js"); +} +catch (e) { + print("Could not require aggregation_guard.js (" + e.message + ")."); + print("Run this from plugins/dbviewer/api/parts, or paste the lists in manually."); +} + +var allowed = Object.keys(guard.ALLOWED_OPERATORS_GLOBAL_ADMIN || {}); + +// Operators the guard deliberately omits. Confirming these still exist keeps the +// exclusions meaningful: an exclusion of something MongoDB no longer has is dead +// weight, and one that was never real (a typo) is a hole. +var EXCLUDED = [ + "$out", "$merge", "$function", "$accumulator", "$where", + "$currentOp", "$collStats", "$indexStats", "$planCacheStats", + "$listSessions", "$listLocalSessions", "$listSampledQueries", + "$listSearchIndexes", "$shardedDataDistribution", "$mergeCursors", + "$documents", "$listClusterCatalog" +]; + +/** + * Whether MongoDB recognises a name in any position. + * @param {string} op - operator name + * @returns {boolean} true when recognised anywhere + */ +function existsAnywhere(op) { + return stageExists(op) || expressionExists(op) || queryExists(op); +} + +var controlsOk = + !existsAnywhere("$notARealOperatorAtAll") + && stageExists("$match") + && expressionExists("$add") + && queryExists("$gt"); + +print("controls " + (controlsOk ? "passed" : "FAILED - the results below are meaningless")); + +var unrecognised = allowed.filter(function(op) { + return !existsAnywhere(op); +}); +print("allow-listed operators checked: " + allowed.length); +print("NOT recognised by this MongoDB (fix or remove): " + (unrecognised.length ? unrecognised.join(" ") : "(none)")); + +var goneExclusions = EXCLUDED.filter(function(op) { + return !existsAnywhere(op); +}); +print("excluded operators checked: " + EXCLUDED.length); +print("excluded but NOT recognised (typo, or gone from this version): " + (goneExclusions.length ? goneExclusions.join(" ") : "(none)")); + +var wronglyAllowed = EXCLUDED.filter(function(op) { + return (guard.ALLOWED_OPERATORS_GLOBAL_ADMIN || {})[op] === true; +}); +print("excluded operators present in the allow-list (must be none): " + (wronglyAllowed.length ? wronglyAllowed.join(" ") : "(none)")); + +probeDb.dropDatabase(); diff --git a/test/unit-tests/plugins.dbviewer.aggregation-guard.js b/test/unit-tests/plugins.dbviewer.aggregation-guard.js index 541ecaaa3f5..ef87b7df473 100644 --- a/test/unit-tests/plugins.dbviewer.aggregation-guard.js +++ b/test/unit-tests/plugins.dbviewer.aggregation-guard.js @@ -1,78 +1,124 @@ require("should"); var guard = require("../../plugins/dbviewer/api/parts/aggregation_guard.js"); -var USER = guard.ALLOWED_STAGES_USER; -var ADMIN = guard.ALLOWED_STAGES_GLOBAL_ADMIN; +var USER = guard.ALLOWED_OPERATORS_USER; +var ADMIN = guard.ALLOWED_OPERATORS_GLOBAL_ADMIN; -// sanitizeAggregation(pipeline, allowedStages) -> { changes, error } -// - strips stages not in the role's allow-list (recursively, at every depth) -// - rejects (error) server-side-JS operators and joins into redacted -// collections, for any role, at any depth +// sanitizeAggregation(pipeline, allowedOperators) -> { changes, error } +// +// The pipeline is never modified. Every "$"-prefixed KEY, at any depth, must be on +// the role's allow-list, or the request is rejected. Values are never inspected, +// and matching is exact. +// +// The guard used to strip disallowed stages instead, which required guessing which +// nested arrays were sub-pipelines. That guess is what the reported bypass defeated: +// one unrecognised sibling stage made a whole branch invisible to the filter. describe("dbviewer aggregation guard", function() { - describe("stage allow-list (non-global user)", function() { - it("keeps allow-listed stages untouched", function() { - var p = [{$match: {a: 1}}, {$group: {_id: "$x"}}, {$limit: 5}]; + describe("the reported bypass", function() { + it("rejects a $lookup hidden behind an unrecognised sibling stage", function() { + // $_internalInhibitOptimization is a real but undocumented MongoDB + // stage. Because it was not in the old list of known stage names, the + // old guard stopped treating this array as a pipeline and left the + // sibling $lookup in place. + var p = [ + {$limit: 1}, + { + $facet: { + leak: [ + {$lookup: {from: "password_reset", pipeline: [{$project: {prid: 1}}], as: "docs"}}, + {$_internalInhibitOptimization: {}}, + {$unwind: "$docs"}, + {$replaceRoot: {newRoot: "$docs"}} + ] + } + } + ]; var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - Object.keys(res.changes).length.should.equal(0); - p.length.should.equal(3); + res.error.type.should.equal("operator"); + res.error.name.should.equal("$lookup"); + res.error.where.should.containEql("$facet"); }); - it("strips a non-allowed stage ($lookup) for a normal user", function() { - var p = [{$lookup: {from: "events", as: "e"}}, {$limit: 5}]; - var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - res.changes.should.have.property("$lookup"); - p.length.should.equal(1); - p[0].should.have.property("$limit"); + it("rejects the unrecognised stage on its own too", function() { + var res = guard.sanitizeAggregation([{$limit: 1}, {$_internalInhibitOptimization: {}}], USER); + res.error.name.should.equal("$_internalInhibitOptimization"); }); - it("strips write stages ($out) for everyone (in no list)", function() { - var p = [{$match: {a: 1}}, {$out: "stolen"}]; - var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - res.changes.should.have.property("$out"); - JSON.stringify(p).indexOf("$out").should.equal(-1); + it("rejects a $lookup nested arbitrarily deep", function() { + var p = [{$facet: {a: [{$facet: {b: [{$lookup: {from: "apps", as: "d"}}]}}]}}]; + guard.sanitizeAggregation(p, USER).error.name.should.equal("$lookup"); }); - it("strips inherited Object.prototype keys (constructor / __proto__)", function() { - var p = [JSON.parse('{"constructor": {"x": 1}}'), JSON.parse('{"__proto__": {"y": 1}}'), {$limit: 5}]; + it("leaves the pipeline untouched when rejecting", function() { + var p = [{$lookup: {from: "apps", as: "d"}}, {$limit: 5}]; guard.sanitizeAggregation(p, USER); - p.length.should.equal(1); - p[0].should.have.property("$limit"); + p.length.should.equal(2); + p[0].should.have.property("$lookup"); }); }); - describe("nested stage stripping (structural, any depth)", function() { - it("strips a non-allowed stage nested in $facet", function() { - var p = [{$facet: {leak: [{$lookup: {from: "events", as: "e"}}]}}]; - var res = guard.sanitizeAggregation(p, USER); - res.changes.should.have.property("$lookup"); - JSON.stringify(p).indexOf("$lookup").should.equal(-1); - }); - it("strips a non-allowed stage in an arbitrary (non-$facet/.pipeline) nested shape", function() { - var p = [{$facet: {b: [{$set: {ok: 1}}]}}]; - // craft an allowed stage carrying a sub-pipeline under a custom field - p[0].$facet.b.push({$project: {z: 1}}); - var weird = [{$group: {_id: 1}}]; - weird.push({$lookup: {from: "events", as: "e"}}); - p[0].$facet.b.push({$set: {nested: {anything: weird}}}); // expression object holding a pipeline-shaped array - var res = guard.sanitizeAggregation(p, USER); - res.changes.should.have.property("$lookup"); - JSON.stringify(p).indexOf("$lookup").should.equal(-1); + describe("operator allow-list (non-global user)", function() { + it("accepts allow-listed stages", function() { + var res = guard.sanitizeAggregation([{$match: {a: 1}}, {$group: {_id: "$x"}}, {$limit: 5}], USER); + (res.error === null).should.equal(true); }); - it("does not mistake an ordinary expression array for a sub-pipeline", function() { - var p = [{$project: {full: {$concat: ["$a", "$b"]}}}]; - var res = guard.sanitizeAggregation(p, USER); - Object.keys(res.changes).length.should.equal(0); - p[0].$project.full.$concat.length.should.equal(2); + it("rejects $lookup", function() { + guard.sanitizeAggregation([{$lookup: {from: "events", as: "e"}}], USER).error.name.should.equal("$lookup"); }); - it("drops a $facet branch emptied by stripping", function() { - var p = [{$facet: {leak: [{$lookup: {from: "events", as: "e"}}]}}]; - guard.sanitizeAggregation(p, USER); - // leak emptied -> removed; $facet emptied -> stage removed - p.length.should.equal(0); + it("rejects write stages ($out, $merge) for everyone", function() { + guard.sanitizeAggregation([{$match: {a: 1}}, {$out: "stolen"}], USER).error.name.should.equal("$out"); + guard.sanitizeAggregation([{$merge: {into: "stolen"}}], ADMIN).error.name.should.equal("$merge"); + }); + it("rejects server introspection stages", function() { + guard.sanitizeAggregation([{$currentOp: {}}], USER).error.name.should.equal("$currentOp"); + guard.sanitizeAggregation([{$collStats: {}}], ADMIN).error.name.should.equal("$collStats"); + }); + it("rejects a stage element that names no operator", function() { + var res = guard.sanitizeAggregation([JSON.parse('{"constructor": {"x": 1}}')], USER); + res.error.type.should.equal("stage"); + }); + it("rejects an own __proto__ key used as a stage name", function() { + var res = guard.sanitizeAggregation([JSON.parse('{"__proto__": {"y": 1}}')], USER); + res.error.type.should.equal("stage"); + }); + }); + + describe("does not reject legitimate queries", function() { + it("accepts expression operators inside stages", function() { + var p = [{$project: {t: {$add: ["$a", "$b"]}, m: {$mergeObjects: [{x: 1}, {y: 2}]}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts $mergeObjects, which must not be confused with $merge", function() { + var p = [{$project: {m: {$mergeObjects: [{a: 1}, {b: 2}]}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts a value that looks like a disallowed operator", function() { + // {$literal: "$lookup"} returns the STRING "$lookup". Only keys are + // checked, so this is fine. + var p = [{$project: {lit: {$literal: "$lookup"}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts $setField naming a $-prefixed field", function() { + // MongoDB allows documents with $-prefixed field names, reachable only + // through $getField / $setField where the name is a value. + var p = [{$project: {b: {$setField: {field: {$literal: "$lookup"}, input: {}, value: 1}}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts complex $match operators", function() { + var p = [{$match: {$and: [{a: {$gt: 1}}, {b: {$in: [1, 2]}}], c: {$elemMatch: {d: 1}}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts accumulators and window operators", function() { + var p = [ + {$group: {_id: "$a", n: {$sum: 1}, all: {$push: "$b"}}}, + {$setWindowFields: {sortBy: {a: 1}, output: {r: {$rank: {}}}}} + ]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts a real $facet", function() { + var p = [{$facet: {byA: [{$group: {_id: "$a", n: {$sum: 1}}}], byB: [{$sortByCount: "$b"}]}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); }); }); - describe("hard rule: server-side JavaScript (any role, any depth)", function() { + describe("server-side JavaScript (any role, any depth)", function() { it("rejects $function inside a $project expression", function() { var p = [{$project: {x: {$function: {body: "f", args: [], lang: "js"}}}}]; var res = guard.sanitizeAggregation(p, ADMIN); @@ -92,8 +138,8 @@ describe("dbviewer aggregation guard", function() { }); }); - describe("hard rule: joins into redacted collections (any role, any depth)", function() { - it("rejects a $lookup into members (even for global admin)", function() { + describe("joins into redacted collections (any role, any depth)", function() { + it("rejects a $lookup into members even for a global admin", function() { var res = guard.sanitizeAggregation([{$lookup: {from: "members", as: "m"}}], ADMIN); res.error.type.should.equal("join"); res.error.name.should.equal("members"); @@ -110,12 +156,12 @@ describe("dbviewer aggregation guard", function() { it("rejects a join into members nested inside $facet", function() { guard.sanitizeAggregation([{$facet: {leak: [{$lookup: {from: "members", as: "m"}}]}}], ADMIN).error.name.should.equal("members"); }); - it("rejects a $lookup into members via the cross-db object form {db, coll}", function() { + it("rejects a $lookup into members via the cross-db object form", function() { var res = guard.sanitizeAggregation([{$lookup: {from: {db: "countly", coll: "members"}, as: "m"}}], ADMIN); res.error.type.should.equal("join"); res.error.name.should.equal("members"); }); - it("rejects a $graphLookup into members via the cross-db object form {db, coll}", function() { + it("rejects a $graphLookup into members via the cross-db object form", function() { var res = guard.sanitizeAggregation([{$graphLookup: {from: {db: "countly", coll: "members"}, startWith: "$x", connectFromField: "a", connectToField: "b", as: "m"}}], ADMIN); res.error.type.should.equal("join"); res.error.name.should.equal("members"); @@ -123,25 +169,37 @@ describe("dbviewer aggregation guard", function() { }); describe("global admin allow-list", function() { - it("allows $lookup into a non-protected collection (kept, no error)", function() { + it("allows $lookup into a non-protected collection", function() { var p = [{$lookup: {from: "events", pipeline: [{$match: {a: 1}}], as: "e"}}, {$limit: 5}]; - var res = guard.sanitizeAggregation(p, ADMIN); - (res.error === null).should.equal(true); - Object.keys(res.changes).length.should.equal(0); - p[0].should.have.property("$lookup"); + (guard.sanitizeAggregation(p, ADMIN).error === null).should.equal(true); }); - it("still strips a non-allowed stage ($out) for an admin", function() { - var p = [{$match: {a: 1}}, {$out: "x"}]; - var res = guard.sanitizeAggregation(p, ADMIN); - (res.error === null).should.equal(true); - res.changes.should.have.property("$out"); + it("allows $unionWith into a non-protected collection", function() { + var p = [{$unionWith: {coll: "events", pipeline: [{$limit: 1}]}}]; + (guard.sanitizeAggregation(p, ADMIN).error === null).should.equal(true); }); - it("does NOT allow $lookup for a normal user (stripped)", function() { - var p = [{$lookup: {from: "events", as: "e"}}]; - var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - res.changes.should.have.property("$lookup"); - p.length.should.equal(0); + it("rejects the same $lookup for a non-global user", function() { + guard.sanitizeAggregation([{$lookup: {from: "events", as: "e"}}], USER).error.name.should.equal("$lookup"); + }); + }); + + describe("allow-list integrity", function() { + it("does not contain the operators it is meant to exclude", function() { + ["$out", "$merge", "$function", "$accumulator", "$where", "$currentOp", + "$collStats", "$indexStats", "$planCacheStats", "$listSessions", + "$mergeCursors", "$documents", "$changeStream"].forEach(function(op) { + (ADMIN[op] === true).should.equal(false); + }); + }); + it("does not carry the misspelling the previous version had", function() { + // the real stage is $shardedDataDistribution; the old list said + // $sharedDataDistribution, so the real one was never recognised + (ADMIN.$sharedDataDistribution === true).should.equal(false); + (ADMIN.$shardedDataDistribution === true).should.equal(false); + }); + it("gives non-global users no join operators", function() { + ["$lookup", "$graphLookup", "$unionWith"].forEach(function(op) { + (USER[op] === true).should.equal(false); + }); }); }); }); From 74e10d5caad3c4a4d3d9ec66f8c4c5fce4bbfaec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:49:48 +0300 Subject: [PATCH 2/5] docs(dbviewer): note why the empty changes object is still passed Co-Authored-By: Claude --- plugins/dbviewer/api/api.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/dbviewer/api/api.js b/plugins/dbviewer/api/api.js index 27bf7be1a94..f265522c1e5 100644 --- a/plugins/dbviewer/api/api.js +++ b/plugins/dbviewer/api/api.js @@ -603,6 +603,9 @@ var spawn = require('child_process').spawn, common.returnMessage(params, 400, msg); return; } + //guard.changes is always empty now that nothing is stripped. It is + //still passed so the response keeps its "removed" field and the UI + //contract does not change aggregate(params.qstring.collection, aggregation, guard.changes); } catch (e) { From b28bf81d90ec9bc62615e5011aaee5e42c3ac6a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:52:53 +0300 Subject: [PATCH 3/5] test(dbviewer): update query guard tests for rejection instead of stripping 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 --- .../plugins.dbviewer.query-guard.js | 71 +++++++++---------- 1 file changed, 33 insertions(+), 38 deletions(-) diff --git a/test/unit-tests/plugins.dbviewer.query-guard.js b/test/unit-tests/plugins.dbviewer.query-guard.js index 358e93ea061..728f840c3c0 100644 --- a/test/unit-tests/plugins.dbviewer.query-guard.js +++ b/test/unit-tests/plugins.dbviewer.query-guard.js @@ -1,46 +1,41 @@ require("should"); var qguard = require("../../plugins/dbviewer/api/parts/query_guard.js"); +// findDisallowedProjectionValue(projection) -> { name } | null +// +// A find() projection may only include or exclude fields. It used to have offending +// entries deleted and the query run anyway, which returned something other than what +// was asked for without saying so. It now returns the first offending field and the +// caller rejects the request, matching the aggregation guard. describe("dbviewer query guard", function() { - describe("sanitizeProjection", function() { - it("keeps plain include/exclude projections", function() { + describe("findDisallowedProjectionValue", function() { + it("accepts plain include/exclude projections", function() { var p = {name: 1, _id: 0, "a.b": 1, ok: true, no: false}; - var changes = qguard.sanitizeProjection(p); - Object.keys(changes).length.should.equal(0); - p.should.have.property("name", 1); - p.should.have.property("a.b", 1); - }); - it("drops a field-path alias value (e.g. {leak: \"$password\"})", function() { - var p = {leak: "$password", k: "$api_key", name: 1}; - var changes = qguard.sanitizeProjection(p); - changes.should.have.property("leak"); - changes.should.have.property("k"); - p.should.not.have.property("leak"); - p.should.not.have.property("k"); - p.should.have.property("name", 1); - }); - it("drops an expression-object value (e.g. {x: {$function: ...}})", function() { - var p = {x: {$function: {body: "f", args: [], lang: "js"}}, y: {$concat: ["$password", ""]}, name: 1}; - var changes = qguard.sanitizeProjection(p); - changes.should.have.property("x"); - changes.should.have.property("y"); - p.should.not.have.property("x"); - p.should.not.have.property("y"); - p.should.have.property("name", 1); - }); - it("drops numeric values that are not strictly 0 or 1", function() { - var p = {a: 2, b: NaN, c: -1, ok: 1, off: 0, t: true, f: false}; - var changes = qguard.sanitizeProjection(p); - changes.should.have.property("a"); - changes.should.have.property("b"); - changes.should.have.property("c"); - p.should.not.have.property("a"); - p.should.not.have.property("b"); - p.should.not.have.property("c"); - p.should.have.property("ok", 1); - p.should.have.property("off", 0); - p.should.have.property("t", true); - p.should.have.property("f", false); + (qguard.findDisallowedProjectionValue(p) === null).should.equal(true); + }); + it("rejects a field-path alias value (e.g. {leak: \"$password\"})", function() { + qguard.findDisallowedProjectionValue({leak: "$password", name: 1}).name.should.equal("leak"); + qguard.findDisallowedProjectionValue({name: 1, k: "$api_key"}).name.should.equal("k"); + }); + it("rejects an expression-object value (e.g. {x: {$function: ...}})", function() { + qguard.findDisallowedProjectionValue({x: {$function: {body: "f", args: [], lang: "js"}}, name: 1}).name.should.equal("x"); + qguard.findDisallowedProjectionValue({y: {$concat: ["$password", ""]}}).name.should.equal("y"); + }); + it("rejects numeric values that are not strictly 0 or 1", function() { + [2, NaN, -1, "1"].forEach(function(bad) { + qguard.findDisallowedProjectionValue({ok: 1, a: bad}).name.should.equal("a"); + }); + }); + it("does not modify the projection it rejects", function() { + var p = {leak: "$password", name: 1}; + qguard.findDisallowedProjectionValue(p); + p.should.have.property("leak", "$password"); + Object.keys(p).length.should.equal(2); + }); + it("accepts a missing or non-object projection", function() { + (qguard.findDisallowedProjectionValue(undefined) === null).should.equal(true); + (qguard.findDisallowedProjectionValue(null) === null).should.equal(true); + (qguard.findDisallowedProjectionValue({}) === null).should.equal(true); }); }); From 3143a859957a17579df4ac37f14b8f27caccead9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:02:20 +0300 Subject: [PATCH 4/5] fix(dbviewer): withhold the password-reset token from reads and joins 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 --- plugins/dbviewer/api/api.js | 21 ++--- .../dbviewer/api/parts/aggregation_guard.js | 3 +- plugins/dbviewer/api/parts/redaction.js | 81 +++++++++++++++++++ .../plugins.dbviewer.aggregation-guard.js | 26 ++++++ test/unit-tests/plugins.dbviewer.redaction.js | 69 ++++++++++++++++ 5 files changed, 189 insertions(+), 11 deletions(-) create mode 100644 plugins/dbviewer/api/parts/redaction.js create mode 100644 test/unit-tests/plugins.dbviewer.redaction.js diff --git a/plugins/dbviewer/api/api.js b/plugins/dbviewer/api/api.js index f265522c1e5..edb89723b59 100644 --- a/plugins/dbviewer/api/api.js +++ b/plugins/dbviewer/api/api.js @@ -20,6 +20,10 @@ const MAX_DBVIEWER_LIMIT = 10000; // unit-tested in isolation. const { sanitizeAggregation, ALLOWED_OPERATORS_USER, ALLOWED_OPERATORS_GLOBAL_ADMIN } = require('./parts/aggregation_guard.js'); const { findDisallowedProjectionValue, escapeRegExp } = require('./parts/query_guard.js'); +// Fields the viewer must never return, and the helpers that remove them. Held in one +// module because the same redaction applies in three places (document read, +// collection read, aggregation) and a per-site list drifts. +const { hasRedactedFields, redactFields, redactionStage } = require('./parts/redaction.js'); var spawn = require('child_process').spawn, child; @@ -139,10 +143,8 @@ var spawn = require('child_process').spawn, } dbs[dbNameOnParam].collection(params.qstring.collection).findOne(docFilter, function(err, results) { if (!err) { - if (params.qstring.collection === 'members' && results) { - delete results.password; - delete results.api_key; - delete results.two_factor_auth; + if (hasRedactedFields(params.qstring.collection)) { + redactFields(params.qstring.collection, results); } else if (params.qstring.collection === 'auth_tokens' && results) { if (results._id) { @@ -289,10 +291,8 @@ var spawn = require('child_process').spawn, var stream = cursor.skip(skip).limit(limit).stream({ transform: function(doc) { - if (params.qstring.collection === 'members' && doc) { - delete doc.password; - delete doc.api_key; - delete doc.two_factor_auth; + if (hasRedactedFields(params.qstring.collection)) { + redactFields(params.qstring.collection, doc); } else if (params.qstring.collection === 'auth_tokens' && doc) { if (doc._id) { @@ -423,13 +423,14 @@ var spawn = require('child_process').spawn, aggregation.push({ "$limit": Math.min(iDisplayLength, MAX_DBVIEWER_LIMIT) }); } } - if (collection === 'members') { + var redaction = redactionStage(collection); + if (redaction) { // Insert the redaction as the very first stage so no // user-supplied stage — including a leading $match using // $expr, or a $project/$group that aliases or references the // field — can read the raw credential fields before they are // removed. - aggregation.splice(0, 0, {"$project": {"password": 0, "api_key": 0, "two_factor_auth": 0}}); + aggregation.splice(0, 0, redaction); } else if (collection === 'auth_tokens') { aggregation.splice(0, 0, {"$addFields": {"_id": "***redacted***"}}); diff --git a/plugins/dbviewer/api/parts/aggregation_guard.js b/plugins/dbviewer/api/parts/aggregation_guard.js index a714c82c29d..c7ee510a27b 100644 --- a/plugins/dbviewer/api/parts/aggregation_guard.js +++ b/plugins/dbviewer/api/parts/aggregation_guard.js @@ -167,7 +167,8 @@ const ALLOWED_OPERATORS_GLOBAL_ADMIN = toMap(STAGES_USER, STAGES_GLOBAL_ADMIN_ON // source collection, so those are rejected for everyone including global admins. const PROTECTED_JOIN_COLLECTIONS = { "members": true, - "auth_tokens": true + "auth_tokens": true, + "password_reset": true }; /** diff --git a/plugins/dbviewer/api/parts/redaction.js b/plugins/dbviewer/api/parts/redaction.js new file mode 100644 index 00000000000..cf96b7a577c --- /dev/null +++ b/plugins/dbviewer/api/parts/redaction.js @@ -0,0 +1,81 @@ +/** + * @module plugins/dbviewer/api/parts/redaction + * @description Fields DB Viewer must never return, and the helpers that remove them. + * + * A field belongs here when holding it is enough on its own to act as somebody else: + * - members.password / api_key / two_factor_auth authenticate a dashboard user; + * - 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 same redaction has to be applied in three places (single-document read, + * collection read, aggregation). Keeping the list here rather than at each call site + * is deliberate: a per-site copy drifts, and a field missed at one of the three is a + * field that leaks. Adding a collection here covers all three. + * + * auth_tokens is deliberately absent. Its secret is the _id itself, which cannot be + * dropped without breaking the row, so the viewer replaces the value instead. That + * stays at the call sites. + */ + +'use strict'; + +const REDACTED_FIELDS = Object.freeze({ + members: Object.freeze(["password", "api_key", "two_factor_auth"]), + password_reset: Object.freeze(["prid"]) +}); + +/** + * Whether a collection has fields that must be withheld. + * + * @param {string} collection - collection name + * @returns {boolean} true when the collection has redacted fields + */ +function hasRedactedFields(collection) { + return Object.prototype.hasOwnProperty.call(REDACTED_FIELDS, collection); +} + +/** + * Remove a collection's redacted fields from a document, in place. + * + * @param {string} collection - collection the document came from + * @param {object} doc - document to redact (mutated); a falsy doc is returned as is + * @returns {object} the same document + */ +function redactFields(collection, doc) { + if (!doc || !hasRedactedFields(collection)) { + return doc; + } + var fields = REDACTED_FIELDS[collection]; + for (var i = 0; i < fields.length; i++) { + delete doc[fields[i]]; + } + return doc; +} + +/** + * Build a $project stage excluding a collection's redacted fields. + * + * The caller must splice this in as the very first stage, so no user-supplied stage + * can read the raw values before they are removed. + * + * @param {string} collection - collection being aggregated + * @returns {object|null} the stage, or null when the collection has none + */ +function redactionStage(collection) { + if (!hasRedactedFields(collection)) { + return null; + } + var fields = REDACTED_FIELDS[collection]; + var projection = {}; + for (var i = 0; i < fields.length; i++) { + projection[fields[i]] = 0; + } + return { "$project": projection }; +} + +module.exports = { + REDACTED_FIELDS, + hasRedactedFields, + redactFields, + redactionStage +}; diff --git a/test/unit-tests/plugins.dbviewer.aggregation-guard.js b/test/unit-tests/plugins.dbviewer.aggregation-guard.js index ef87b7df473..f6f58c563ff 100644 --- a/test/unit-tests/plugins.dbviewer.aggregation-guard.js +++ b/test/unit-tests/plugins.dbviewer.aggregation-guard.js @@ -33,6 +33,27 @@ describe("dbviewer aggregation guard", function() { } } ]; + // Two independent checks now catch this. The join check runs first and + // reports the collection, since password_reset is protected for every + // role; the operator check would reject it anyway. + var res = guard.sanitizeAggregation(p, USER); + res.error.type.should.equal("join"); + res.error.name.should.equal("password_reset"); + }); + it("rejects the same shape aimed at an unprotected collection, on the operator", function() { + var p = [ + {$limit: 1}, + { + $facet: { + leak: [ + {$lookup: {from: "apps", pipeline: [{$project: {key: 1}}], as: "docs"}}, + {$_internalInhibitOptimization: {}}, + {$unwind: "$docs"}, + {$replaceRoot: {newRoot: "$docs"}} + ] + } + } + ]; var res = guard.sanitizeAggregation(p, USER); res.error.type.should.equal("operator"); res.error.name.should.equal("$lookup"); @@ -161,6 +182,11 @@ describe("dbviewer aggregation guard", function() { res.error.type.should.equal("join"); res.error.name.should.equal("members"); }); + it("rejects a $lookup into password_reset, whose prid is a reset token", function() { + var res = guard.sanitizeAggregation([{$lookup: {from: "password_reset", as: "d"}}], ADMIN); + res.error.type.should.equal("join"); + res.error.name.should.equal("password_reset"); + }); it("rejects a $graphLookup into members via the cross-db object form", function() { var res = guard.sanitizeAggregation([{$graphLookup: {from: {db: "countly", coll: "members"}, startWith: "$x", connectFromField: "a", connectToField: "b", as: "m"}}], ADMIN); res.error.type.should.equal("join"); diff --git a/test/unit-tests/plugins.dbviewer.redaction.js b/test/unit-tests/plugins.dbviewer.redaction.js new file mode 100644 index 00000000000..2ebff99aea3 --- /dev/null +++ b/test/unit-tests/plugins.dbviewer.redaction.js @@ -0,0 +1,69 @@ +require("should"); +var redaction = require("../../plugins/dbviewer/api/parts/redaction.js"); + +describe("dbviewer redaction", function() { + describe("hasRedactedFields", function() { + it("knows the collections that hold secrets", function() { + redaction.hasRedactedFields("members").should.equal(true); + redaction.hasRedactedFields("password_reset").should.equal(true); + }); + it("is false for ordinary collections", function() { + redaction.hasRedactedFields("events_data").should.equal(false); + redaction.hasRedactedFields("apps").should.equal(false); + }); + it("is false for Object.prototype keys, not truthy by inheritance", function() { + redaction.hasRedactedFields("constructor").should.equal(false); + redaction.hasRedactedFields("toString").should.equal(false); + redaction.hasRedactedFields("__proto__").should.equal(false); + }); + }); + + describe("redactFields", function() { + it("removes member credentials and keeps the rest", function() { + var doc = {_id: "1", email: "a@b.c", password: "hash", api_key: "k", two_factor_auth: {}, full_name: "A"}; + redaction.redactFields("members", doc); + doc.should.not.have.property("password"); + doc.should.not.have.property("api_key"); + doc.should.not.have.property("two_factor_auth"); + doc.should.have.property("email", "a@b.c"); + doc.should.have.property("full_name", "A"); + }); + it("removes the password-reset token and keeps the rest", function() { + var doc = {_id: "1", prid: "tok", user_id: "u", timestamp: 12345}; + redaction.redactFields("password_reset", doc); + doc.should.not.have.property("prid"); + doc.should.have.property("user_id", "u"); + doc.should.have.property("timestamp", 12345); + }); + it("leaves documents from other collections alone", function() { + var doc = {a: 1, password: "not a member doc"}; + redaction.redactFields("events_data", doc); + doc.should.have.property("password"); + }); + it("tolerates a missing document", function() { + (redaction.redactFields("members", null) === null).should.equal(true); + (redaction.redactFields("members", undefined) === undefined).should.equal(true); + }); + }); + + describe("redactionStage", function() { + it("excludes every member credential", function() { + redaction.redactionStage("members").should.eql({$project: {password: 0, api_key: 0, two_factor_auth: 0}}); + }); + it("excludes the password-reset token", function() { + redaction.redactionStage("password_reset").should.eql({$project: {prid: 0}}); + }); + it("is null for a collection with nothing to hide", function() { + (redaction.redactionStage("events_data") === null).should.equal(true); + (redaction.redactionStage("constructor") === null).should.equal(true); + }); + it("covers exactly the same fields the document path removes", function() { + // the two paths must not drift: a field dropped from a single-document + // read but not from an aggregation is a field that leaks + Object.keys(redaction.REDACTED_FIELDS).forEach(function(collection) { + var stage = redaction.redactionStage(collection); + Object.keys(stage.$project).sort().should.eql(redaction.REDACTED_FIELDS[collection].slice().sort()); + }); + }); + }); +}); From 7396cebf40a1e5f4d852071f166449cbaa765b3b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 17:03:32 +0300 Subject: [PATCH 5/5] chore(dbviewer): drop an unused eslint-disable from the operator probe 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 --- plugins/dbviewer/api/parts/verify_operators.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/dbviewer/api/parts/verify_operators.js b/plugins/dbviewer/api/parts/verify_operators.js index d7d1f20e4cd..b9e8d46c2b6 100644 --- a/plugins/dbviewer/api/parts/verify_operators.js +++ b/plugins/dbviewer/api/parts/verify_operators.js @@ -32,7 +32,7 @@ * output means nothing. */ -/* eslint-disable no-undef, no-console */ +/* eslint-disable no-undef */ 'use strict'; var probeDb = db.getSiblingDB("dbviewer_operator_probe");