Stop cleanup() and removeInactiveDevices from touching other tenants

.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:

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/<domain>/ 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]<type>/<domain>/<hash>` (e.g. 'lc' + node/<dom>/...).
+        // 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
     }
 

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.

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/<name>` 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.

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