fix: Transactional batch request can roll back or block writes of other clients (GHSA-jhh9-hrgh-c9gv)

spec/vulnerabilities.spec.js

@@ -7804,4 +7804,182 @@ describe('Vulnerabilities', () => {
       }
     });
   });
+
+  describe('(GHSA-jhh9-hrgh-c9gv) Transactional batch request can roll back or block writes of other clients', () => {
+    const AppCache = require('../lib/cache').AppCache;
+    const headers = {
+      'X-Parse-Application-Id': 'test',
+      'X-Parse-REST-API-Key': 'rest',
+      'Content-Type': 'application/json',
+    };
+    const post = (path, body) =>
+      request({
+        method: 'POST',
+        url: `http://localhost:8378/1${path}`,
+        headers,
+        body,
+      });
+    const get = path =>
+      request({
+        url: `http://localhost:8378/1${path}`,
+        headers,
+      });
+    const sleep = ms => new Promise(resolve => setTimeout(resolve, ms));
+    const expectSeparateDatabaseControllers = () => {
+      expect(Config.get('test').database).not.toBe(Config.get('test').database);
+    };
+    const expectServerConfigInCache = () => {
+      const cachedConfig = AppCache.get('test');
+      expect(cachedConfig.databaseController).toBeDefined();
+      expect(cachedConfig.database).toBeUndefined();
+    };
+
+    it('does not share the database controller after the master key is loaded from a function', async () => {
+      await reconfigureServer({ masterKey: () => 'test' });
+      await post('/classes/TestObject', { key: 'value' });
+      expect(Config.get('test').masterKeyCache.masterKey).toBe('test');
+      expectServerConfigInCache();
+      expectSeparateDatabaseControllers();
+    });
+
+    it('does not share the database controller after the master key is reloaded', async () => {
+      await reconfigureServer({ masterKey: () => 'test', masterKeyTtl: 1000 });
+      await post('/classes/TestObject', { key: 'value' });
+      Config.get('test').masterKeyCache.expiresAt = new Date(0);
+      await post('/classes/TestObject', { key: 'value' });
+      expect(Config.get('test').masterKeyCache.expiresAt.getTime()).toBeGreaterThan(Date.now());
+      expectServerConfigInCache();
+      expectSeparateDatabaseControllers();
+    });
+
+    it('does not share the database controller after a Cloud Code rate limit is registered', async () => {
+      Parse.Cloud.define('rateLimitedFunction', () => 'ok', {
+        rateLimit: { requestTimeWindow: 10000, requestCount: 1 },
+      });
+      expectSeparateDatabaseControllers();
+    });
+
+    it('does not share the database controller after the server config is set via Parse.Server', async () => {
+      const config = Parse.Server;
+      config.silent = false;
+      Parse.Server = config;
+      expectSeparateDatabaseControllers();
+    });
+
+    it('rejects a transactional session while another one is active on the same database controller', async () => {
+      const database = Config.get('test').database;
+      spyOn(database.adapter, 'createTransactionalSession').and.resolveTo({});
+      await database.createTransactionalSession();
+      await expectAsync(database.createTransactionalSession()).toBeRejectedWithError(
+        'There is already an active transactional session'
+      );
+      expect(database.adapter.createTransactionalSession).toHaveBeenCalledTimes(1);
+    });
+
+    it('rejects concurrent transactional sessions on the same database controller', async () => {
+      const database = Config.get('test').database;
+      spyOn(database.adapter, 'createTransactionalSession').and.resolveTo({});
+      const results = await Promise.allSettled([
+        database.createTransactionalSession(),
+        database.createTransactionalSession(),
+      ]);
+      expect(results.map(result => result.status)).toEqual(['fulfilled', 'rejected']);
+      expect(results[1].reason.message).toBe('There is already an active transactional session');
+      expect(database.adapter.createTransactionalSession).toHaveBeenCalledTimes(1);
+    });
+
+    it('allows a transactional session after the creation of a transactional session failed', async () => {
+      const database = Config.get('test').database;
+      spyOn(database.adapter, 'createTransactionalSession').and.returnValues(
+        Promise.reject(new Error('creation failed')),
+        Promise.resolve({})
+      );
+      await expectAsync(database.createTransactionalSession()).toBeRejectedWithError(
+        'creation failed'
+      );
+      await expectAsync(database.createTransactionalSession()).toBeResolved();
+    });
+
+    it('allows a transactional session after the creation of a transactional session threw', async () => {
+      const database = Config.get('test').database;
+      let calls = 0;
+      spyOn(database.adapter, 'createTransactionalSession').and.callFake(() => {
+        if (++calls === 1) {
+          throw new Error('creation threw');
+        }
+        return Promise.resolve({});
+      });
+      await expectAsync(database.createTransactionalSession()).toBeRejectedWithError(
+        'creation threw'
+      );
+      await expectAsync(database.createTransactionalSession()).toBeResolved();
+    });
+
+    if (
+      ['replicaset', 'replset'].includes(process.env.MONGODB_TOPOLOGY) ||
+      process.env.PARSE_SERVER_TEST_DB === 'postgres'
+    ) {
+      describe('transactions', () => {
+        beforeEach(async () => {
+          await reconfigureServer({ masterKey: () => 'test' });
+          // Transactions only work on existing classes
+          for (const className of ['SlowObject', 'FailingObject', 'OtherObject']) {
+            await post(`/classes/${className}`, { key: 'value' });
+          }
+          Parse.Cloud.beforeSave('SlowObject', () => sleep(500));
+        });
+
+        it('keeps a write of another client that runs during a failing transactional batch', async () => {
+          const batch = expectAsync(
+            post('/batch', {
+              transaction: true,
+              requests: [
+                { method: 'POST', path: '/1/classes/SlowObject', body: { key: 'value' } },
+                { method: 'POST', path: '/1/classes/FailingObject', body: { key: 10 } },
+              ],
+            })
+          ).toBeRejected();
+          await sleep(150);
+          const response = await post('/classes/OtherObject', { key: 'other client' });
+          await batch;
+          const object = await get(`/classes/OtherObject/${response.data.objectId}`);
+          expect(object.data.key).toBe('other client');
+        });
+
+        it('completes concurrent transactional batches', async () => {
+          const batch = () =>
+            post('/batch', {
+              transaction: true,
+              requests: [
+                { method: 'POST', path: '/1/classes/SlowObject', body: { key: 'value' } },
+                { method: 'POST', path: '/1/classes/OtherObject', body: { key: 'value' } },
+              ],
+            });
+          const first = batch();
+          await sleep(100);
+          const responses = await Promise.all([first, batch()]);
+          for (const response of responses) {
+            expect(response.data.length).toBe(2);
+            expect(response.data.every(result => result.success)).toBeTrue();
+          }
+          const objects = await get('/classes/OtherObject');
+          expect(objects.data.results.length).toBe(3);
+        });
+
+        it('does not fail writes of other clients after a failing transactional batch', async () => {
+          await expectAsync(
+            post('/batch', {
+              transaction: true,
+              requests: [
+                { method: 'POST', path: '/1/classes/OtherObject', body: { key: 'value' } },
+                { method: 'POST', path: '/1/classes/FailingObject', body: { key: 10 } },
+              ],
+            })
+          ).toBeRejected();
+          const response = await post('/classes/OtherObject', { key: 'other client' });
+          expect(response.data.objectId).toBeDefined();
+        });
+      });
+    }
+  });
 });

src/Config.js

@@ -52,12 +52,17 @@ export class Config {
     const config = new Config();
     config.applicationId = applicationId;
     Object.keys(cacheInfo).forEach(key => {
-      if (key == 'databaseController') {
-        config.database = new DatabaseController(cacheInfo.databaseController.adapter, config);
-      } else {
+      if (key != 'databaseController' && key != 'database') {
         config[key] = cacheInfo[key];
       }
     });
+    // Always create a new database controller, as it holds request-scoped state such as the
+    // transactional session; a request-scoped config in the cache has `database` instead of
+    // `databaseController`
+    const databaseController = cacheInfo.databaseController || cacheInfo.database;
+    if (databaseController) {
+      config.database = new DatabaseController(databaseController.adapter, config);
+    }
     config.mount = removeTrailingSlash(mount);
     config.generateSessionExpiresAt = config.generateSessionExpiresAt.bind(config);
     config.generateEmailVerifyTokenExpiresAt = config.generateEmailVerifyTokenExpiresAt.bind(
@@ -989,7 +994,11 @@ export class Config {
 
       const expiresAt = this.masterKeyTtl ? new Date(Date.now() + 1000 * this.masterKeyTtl) : null
       this.masterKeyCache = { masterKey, expiresAt };
-      Config.put(this);
+      // Update only the cached server config, as this config is request-scoped
+      const serverConfig = AppCache.get(this.applicationId);
+      if (serverConfig) {
+        serverConfig.masterKeyCache = this.masterKeyCache;
+      }
 
       return this.masterKeyCache.masterKey;
     }

src/Controllers/DatabaseController.js

@@ -451,6 +451,7 @@ class DatabaseController {
     // multiple schemas, so instead use loadSchema to get a schema.
     this.schemaPromise = null;
     this._transactionalSession = null;
+    this._transactionalSessionPending = false;
     this.options = options;
   }
 
@@ -1925,9 +1926,20 @@ class DatabaseController {
   }
 
   createTransactionalSession() {
-    return this.adapter.createTransactionalSession().then(transactionalSession => {
-      this._transactionalSession = transactionalSession;
-    });
+    if (this._transactionalSession || this._transactionalSessionPending) {
+      return Promise.reject(new Error('There is already an active transactional session'));
+    }
+    // Reserve the session before it is created, without setting `_transactionalSession`, which
+    // concurrent writes on this controller would otherwise use
+    this._transactionalSessionPending = true;
+    return Promise.resolve()
+      .then(() => this.adapter.createTransactionalSession())
+      .then(transactionalSession => {
+        this._transactionalSession = transactionalSession;
+      })
+      .finally(() => {
+        this._transactionalSessionPending = false;
+      });
   }
 
   commitTransactionalSession() {