fix: Object update with a dot notation key below a pointer field can terminate the server process (GHSA-wc3c-6843-q7mv)

spec/vulnerabilities.spec.js

@@ -11030,4 +11030,223 @@ describe('Vulnerabilities', () => {
       expect((await readInstallation(own)).localeIdentifier).toBe('self');
     });
   });
+
+  describe('(GHSA-wc3c-6843-q7mv) Dot notation key below a pointer field with an afterSave trigger', () => {
+    const serverURL = 'http://localhost:8378/1';
+    const masterHeaders = {
+      'X-Parse-Application-Id': 'test',
+      'X-Parse-Master-Key': 'test',
+      'Content-Type': 'application/json',
+    };
+    const clientHeaders = {
+      'X-Parse-Application-Id': 'test',
+      'X-Parse-REST-API-Key': 'rest',
+      'Content-Type': 'application/json',
+    };
+
+    let rejections;
+    const onRejection = reason => rejections.push(reason);
+    beforeEach(() => {
+      rejections = [];
+      process.on('unhandledRejection', onRejection);
+    });
+    afterEach(() => {
+      process.removeListener('unhandledRejection', onRejection);
+    });
+
+    // Resolves with the response, or with { hung: true } if the request does not complete in time.
+    const within = (promise, ms = 7000) =>
+      Promise.race([
+        promise.then(
+          r => ({ status: r.status, data: r.data }),
+          e => ({ status: e.status, data: e.data })
+        ),
+        new Promise(resolve => setTimeout(() => resolve({ hung: true }), ms)),
+      ]);
+
+    async function makeTarget() {
+      const target = new Parse.Object('Target');
+      await target.save({ n: 1 }, { useMasterKey: true });
+      return target;
+    }
+    async function makeHolder(fields) {
+      const holder = new Parse.Object('Holder');
+      await holder.save(fields, { useMasterKey: true });
+      return holder;
+    }
+
+    it_only_db('mongo')('completes a dot notation delete of a pointer id', async () => {
+      const target = await makeTarget();
+      const holder = await makeHolder({ owner: target });
+      let seenOwnerId = 'not-called';
+      Parse.Cloud.afterSave('Holder', req => {
+        const owner = req.object.get('owner');
+        seenOwnerId = owner ? owner.id : null;
+      });
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: masterHeaders,
+          body: JSON.stringify({ 'owner.id': { __op: 'Delete' } }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(seenOwnerId).toBe(target.id);
+      expect(rejections).toEqual([]);
+    });
+
+    it_only_db('mongo')('completes a dot notation null on a pointer id', async () => {
+      const target = await makeTarget();
+      const holder = await makeHolder({ owner: target });
+      let seenOwnerId = 'not-called';
+      Parse.Cloud.afterSave('Holder', req => {
+        const owner = req.object.get('owner');
+        seenOwnerId = owner ? owner.id : null;
+      });
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: masterHeaders,
+          body: JSON.stringify({ 'owner.id': null }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(seenOwnerId).toBe(target.id);
+      expect(rejections).toEqual([]);
+    });
+
+    it('completes when beforeSave reverts a dot notation key below a pointer', async () => {
+      const target = await makeTarget();
+      const holder = await makeHolder({ owner: target });
+      Parse.Cloud.beforeSave('Holder', ({ object }) => {
+        object.revert('owner.id');
+      });
+      let seenOwnerId = 'not-called';
+      Parse.Cloud.afterSave('Holder', req => {
+        const owner = req.object.get('owner');
+        seenOwnerId = owner ? owner.id : null;
+      });
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: masterHeaders,
+          body: JSON.stringify({ 'owner.id': { __op: 'Delete' } }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(seenOwnerId).toBe(target.id);
+      expect(rejections).toEqual([]);
+    });
+
+    it('completes a dot notation delete of a nested pointer id', async () => {
+      const target = await makeTarget();
+      const holder = await makeHolder({ meta: { ptr: target.toPointer() } });
+      Parse.Cloud.afterSave('Holder', () => {});
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: masterHeaders,
+          body: JSON.stringify({ 'meta.ptr.id': { __op: 'Delete' } }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(rejections).toEqual([]);
+    });
+
+    it('still deletes a dot notation key below a plain object field', async () => {
+      const holder = await makeHolder({ meta: { note: 'keep', other: 'x' } });
+      let seenMeta = 'not-called';
+      Parse.Cloud.afterSave('Holder', req => {
+        seenMeta = req.object.get('meta');
+      });
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: masterHeaders,
+          body: JSON.stringify({ 'meta.note': { __op: 'Delete' } }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(seenMeta).toEqual({ other: 'x' });
+      expect(rejections).toEqual([]);
+    });
+
+    it_only_db('mongo')('completes a dot notation delete of a pointer id in a batch request', async () => {
+      const target = await makeTarget();
+      const holder = await makeHolder({ owner: target });
+      Parse.Cloud.afterSave('Holder', () => {});
+      const result = await within(
+        request({
+          method: 'POST',
+          url: `${serverURL}/batch`,
+          headers: masterHeaders,
+          body: JSON.stringify({
+            requests: [
+              {
+                method: 'PUT',
+                path: `/1/classes/Holder/${holder.id}`,
+                body: { 'owner.id': { __op: 'Delete' } },
+              },
+            ],
+          }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(result.data[0].success).toBeDefined();
+      expect(rejections).toEqual([]);
+    });
+
+    it_only_db('mongo')('completes for a non-master caller on a publicly writable object', async () => {
+      const target = await makeTarget();
+      const holder = new Parse.Object('Holder');
+      holder.set('owner', target);
+      holder.setACL(new Parse.ACL({ '*': { read: true, write: true } }));
+      await holder.save(null, { useMasterKey: true });
+      Parse.Cloud.afterSave('Holder', () => {});
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: clientHeaders,
+          body: JSON.stringify({ 'owner.id': { __op: 'Delete' } }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(rejections).toEqual([]);
+    });
+
+    it_only_db('mongo')('passes the stored pointer to the afterSave trigger unchanged', async () => {
+      const target = await makeTarget();
+      const holder = await makeHolder({ owner: target });
+      let ownerPointer = 'not-called';
+      Parse.Cloud.afterSave('Holder', req => {
+        const owner = req.object.get('owner');
+        ownerPointer = owner ? owner.toPointer() : null;
+      });
+      const result = await within(
+        request({
+          method: 'PUT',
+          url: `${serverURL}/classes/Holder/${holder.id}`,
+          headers: masterHeaders,
+          body: JSON.stringify({ 'owner.id': { __op: 'Delete' }, label: 'x' }),
+        })
+      );
+      expect(result.hung).toBeUndefined();
+      expect(result.status).toBe(200);
+      expect(ownerPointer).toEqual({ __type: 'Pointer', className: 'Target', objectId: target.id });
+      expect(rejections).toEqual([]);
+    });
+  });
 });

src/RestWrite.js

@@ -2036,6 +2036,21 @@ RestWrite.prototype.sanitizedData = function () {
   return Parse._decode(undefined, data);
 };
 
+// A raw view of the object's fields (stored values overlaid with whole-field
+// writes in this request) used to resolve whether a dot notation key targets a
+// pointer's object ID.
+RestWrite.prototype._dotNotationFieldRoot = function () {
+  const root = Object.assign({}, this.originalData);
+  if (this.data) {
+    for (const key of Object.keys(this.data)) {
+      if (key.indexOf('.') < 0) {
+        root[key] = this.data[key];
+      }
+    }
+  }
+  return root;
+};
+
 // Returns an updated copy of the object
 RestWrite.prototype.buildParseObjects = function () {
   const extraData = { className: this.className, objectId: this.query?.objectId };
@@ -2062,11 +2077,14 @@ RestWrite.prototype.buildParseObjects = function () {
     }
   }
   const updatedObject = triggers.inflate(extraData, this.originalData);
+  const pointerRoot = this._dotNotationFieldRoot();
   Object.keys(this.data).reduce(function (data, key) {
     if (key.indexOf('.') > 0) {
       // Null and undefined are set at their own dot notation path, like an operation
       if (data[key] == null || typeof data[key].__op === 'string') {
-        if (!readOnlyAttributes.includes(key)) {
+        // A dot notation key that removes a pointer's object ID would leave the
+        // pointer unserializable and break the trigger object, so skip it.
+        if (!readOnlyAttributes.includes(key) && !targetsPointerId(pointerRoot, key)) {
           // The SDK key name rules apply to top-level fields only
           updatedObject.set(key, data[key], { ignoreValidation: true });
         }
@@ -2159,6 +2177,22 @@ const getOwnPath = (object, path) => {
   return value;
 };
 
+// Whether a dot notation key targets a pointer's object ID (`id`/`objectId`).
+// Removing it leaves the pointer unserializable (`toPointer` throws), which
+// corrupts the trigger object; other sub-keys below a pointer are left to the
+// existing handling.
+const targetsPointerId = (root, key) => {
+  const segments = key.split('.');
+  for (let depth = 1; depth < segments.length; depth++) {
+    const parent = getOwnPath(root, segments.slice(0, depth));
+    if (parent !== null && typeof parent === 'object' && parent.__type === 'Pointer') {
+      const next = segments[depth];
+      return next === 'id' || next === 'objectId';
+    }
+  }
+  return false;
+};
+
 // Restores a reverted dot notation key in the response
 RestWrite.prototype._restoreRevertedPath = function (response, writeData, written, key, finalObject) {
   // A write at or beneath the key governs

src/triggers.js

@@ -925,6 +925,18 @@ async function builtInTriggerValidator(options, request, auth) {
 // Resolves to an object, empty or containing an object key. A beforeSave
 // trigger will set the object key to the rest format object to save.
 // originalParseObject is optional, we only need that for before/afterSave functions
+// Serializes a Parse object for logging only. Serialization can throw (e.g. a
+// pointer left without an object ID by a dot notation write), so it must never
+// prevent the trigger promise from settling; fall back to undefined on failure.
+function safeToJSON(parseObject) {
+  try {
+    return parseObject.toJSON();
+  } catch {
+    /* istanbul ignore next: defensive fallback, logging must not break the trigger */
+    return undefined;
+  }
+}
+
 export function maybeRunTrigger(
   triggerType,
   auth,
@@ -953,7 +965,7 @@ export function maybeRunTrigger(
         logTriggerSuccessBeforeHook(
           triggerType,
           parseObject.className,
-          parseObject.toJSON(),
+          safeToJSON(parseObject),
           object,
           auth,
           triggerType.startsWith('after')
@@ -974,7 +986,7 @@ export function maybeRunTrigger(
         logTriggerErrorBeforeHook(
           triggerType,
           parseObject.className,
-          parseObject.toJSON(),
+          safeToJSON(parseObject),
           auth,
           error,
           config.logLevels.triggerBeforeError
@@ -1005,7 +1017,7 @@ export function maybeRunTrigger(
           logTriggerAfterHook(
             triggerType,
             parseObject.className,
-            parseObject.toJSON(),
+            safeToJSON(parseObject),
             auth,
             config.logLevels.triggerAfter
           );